Add option to not remove missing sections (PE)

This addresses a regression with sample 848092559:

LDB sig (Win.Virus.Virut-5898123-1) that uses 'NumberOfSections:3-3'
started matching on a PE that has 4 sections, but one is totally outside
of the file and gets removed. Previously, two of the ClamAV PE header
parsing implementations handled this case differently, and the NDB/LDB
matching code would be told there were 4 sections while the bytecode
and unpacking code would only see 3 sections. When consolidating the
PE header parsing code, I made it so that the section always gets
removed.

For now we just replicate the original behavior by providing a new
flag to the PE header parsing code. We should re-evaluate the effects
that this has later, once we have better tests for the bytecode API
and we have test samples for each of the hardcoded detection cases in
cli_scanpe.

Also, fixes some memory leaks based on the changes in my last commit x_x
pull/111/head
Andrew 6 years ago committed by Micah Snyder
parent df8dfda9cd
commit 8b72234369
  1. 77
      libclamav/pe.c
  2. 5
      libclamav/pe.h

@ -2781,8 +2781,7 @@ int cli_scanpe(cli_ctx *ctx)
struct cli_exe_info _peinfo;
struct cli_exe_info *peinfo = &_peinfo;
uint32_t opts = CLI_PEHEADER_OPT_DBG_PRINT_INFO;
;
uint32_t opts = CLI_PEHEADER_OPT_DBG_PRINT_INFO | CLI_PEHEADER_OPT_REMOVE_MISSING_SECTIONS;
#if HAVE_JSON
if (SCAN_COLLECT_METADATA) {
@ -2808,16 +2807,20 @@ int cli_scanpe(cli_ctx *ctx)
if (DETECT_BROKEN_PE) {
// TODO Handle allmatch
ret = cli_append_virus(ctx, "Heuristics.Broken.Executable");
cli_exe_info_destroy(peinfo);
return ret;
}
cli_dbgmsg("cli_scanpe: PE header appears broken - " PE_HDR_PARSE_FAIL_CONSEQUENCE);
cli_exe_info_destroy(peinfo);
return CL_CLEAN;
} else if (CLI_PEHEADER_RET_JSON_TIMEOUT == ret) {
cli_dbgmsg("cli_scanpe: JSON creation timed out - " PE_HDR_PARSE_FAIL_CONSEQUENCE);
cli_exe_info_destroy(peinfo);
return CL_ETIMEOUT;
} else if (CLI_PEHEADER_RET_GENERIC_ERROR == ret) {
cli_dbgmsg("cli_scanpe: An error occurred when parsing the PE header - " PE_HDR_PARSE_FAIL_CONSEQUENCE);
cli_exe_info_destroy(peinfo);
return CL_CLEAN;
}
@ -4460,7 +4463,7 @@ int cli_pe_targetinfo(fmap_t *map, struct cli_exe_info *peinfo)
* parsing. The options are (prefixed with CLI_PEHEADER_OPT_):
* - NONE - Do default parsing
* - COLLECT_JSON - Populate ctx's json obj with PE header
* info
* info
* - DBG_PRINT_INFO - Print debug information about the
* PE file. Right now, cli_peheader is
* called multiple times for a given PE,
@ -4473,6 +4476,10 @@ int cli_pe_targetinfo(fmap_t *map, struct cli_exe_info *peinfo)
* executable cause RET_BROKEN_PE
* to be returned, but otherwise
* these will be tolerated.
* - REMOVE_MISSING_SECTIONS - If a section exists outside of the
* file, remove it from
* peinfo->sections. Otherwise, the
* rsz is just set to 0 for it.
* @param ctx The overaching cli_ctx. This is required with certain opts, but
* optional otherwise.
* @return If the PE header is parsed successfully, CLI_PEHEADER_RET_SUCCESS
@ -4490,6 +4497,13 @@ int cli_pe_targetinfo(fmap_t *map, struct cli_exe_info *peinfo)
* TODO Simplify and get rid of CLI_PEHEADER_OPT_STRICT_ON_PE_ERRORS if
* possible. We should either fail always or ignore always, IMO.
*
* TODO Simplify and get rid of CLI_PEHEADER_OPT_REMOVE_MISSING_SECTIONS if
* possible. I don't think it makes sense to have pieces of the code work
* off of incomplete representations of the sections (for instance, I wonder
* if this makes any of the bytecode APIs return unexpected values). This
* appears to have been implemented to prevent ClamAV from crashing, though,
* (bb11155) so we need to ensure the underlying issues are addressed.
*
* TODO Consolidate when information about the PE is printed (after successful
* PE parsing). This will allow us to simplify the code. Some fail cases,
* then, will cause PE info to not be printed at all, but I think this is
@ -5126,39 +5140,46 @@ int cli_peheader(fmap_t *map, struct cli_exe_info *peinfo, uint32_t opts, cli_ct
section->uraw = EC32(section_hdr->PointerToRawData);
section->ursz = EC32(section_hdr->SizeOfRawData);
// First, if a section exists totally outside of a file, remove the
// section from the list.
// TODO Document that this happens in the function documentation
/* First, if a section exists totally outside of a file, remove the
* section from the list or zero out it's size. */
if (section->rsz) { /* Don't bother with virtual only sections */
if (section->raw >= fsize || section->uraw >= fsize) {
cli_dbgmsg("cli_peheader: Broken PE file - Section %d starts or exists beyond the end of file (Offset@ %lu, Total filesize %lu)\n", section_pe_idx, (unsigned long)section->raw, (unsigned long)fsize);
if (peinfo->nsections == 1) {
ret = CLI_PEHEADER_RET_BROKEN_PE;
goto done;
}
for (j = i; j < peinfo->nsections - 1; j++)
memcpy(&(peinfo->sections[j]), &(peinfo->sections[j + 1]), sizeof(struct cli_exe_section));
if (opts & CLI_PEHEADER_OPT_REMOVE_MISSING_SECTIONS) {
if (peinfo->nsections == 1) {
ret = CLI_PEHEADER_RET_BROKEN_PE;
goto done;
}
for (j = i; j < peinfo->nsections - 1; j++)
memcpy(&section_hdrs[j], &section_hdrs[j + 1], sizeof(struct pe_image_section_hdr));
for (j = i; j < peinfo->nsections - 1; j++)
memcpy(&(peinfo->sections[j]), &(peinfo->sections[j + 1]), sizeof(struct cli_exe_section));
peinfo->nsections--;
for (j = i; j < peinfo->nsections - 1; j++)
memcpy(&section_hdrs[j], &section_hdrs[j + 1], sizeof(struct pe_image_section_hdr));
// Adjust i since we removed a section and continue on
i--;
continue;
}
peinfo->nsections--;
// Verify that the section is fully contained within the file
if (!CLI_ISCONTAINED(0, fsize, section->raw, section->rsz)) {
cli_dbgmsg("cli_peheader: PE Section %d raw+rsz extends past the end of the file by %lu bytes\n", section_pe_idx, (section->raw + section->rsz) - fsize);
section->rsz = fsize - section->raw;
}
// Adjust i since we removed a section and continue on
i--;
continue;
} else {
section->rsz = 0;
section->ursz = 0;
}
} else {
if (!CLI_ISCONTAINED(0, fsize, section->uraw, section->ursz)) {
cli_dbgmsg("cli_peheader: PE Section %d uraw+ursz extends past the end of the file by %lu bytes\n", section_pe_idx, (section->uraw + section->ursz) - fsize);
section->ursz = fsize - section->uraw;
/* If a section is truncated, adjust it's size value */
if (!CLI_ISCONTAINED(0, fsize, section->raw, section->rsz)) {
cli_dbgmsg("cli_peheader: PE Section %d raw+rsz extends past the end of the file by %lu bytes\n", section_pe_idx, (section->raw + section->rsz) - fsize);
section->rsz = fsize - section->raw;
}
if (!CLI_ISCONTAINED(0, fsize, section->uraw, section->ursz)) {
cli_dbgmsg("cli_peheader: PE Section %d uraw+ursz extends past the end of the file by %lu bytes\n", section_pe_idx, (section->uraw + section->ursz) - fsize);
section->ursz = fsize - section->uraw;
}
}
}
@ -5548,6 +5569,7 @@ cl_error_t cli_check_auth_header(cli_ctx *ctx, struct cli_exe_info *peinfo)
cli_exe_info_init(peinfo, 0);
if (cli_peheader(*ctx->fmap, peinfo, CLI_PEHEADER_OPT_NONE, NULL) != CLI_PEHEADER_RET_SUCCESS) {
cli_exe_info_destroy(peinfo);
return CL_EFORMAT;
}
}
@ -5786,6 +5808,7 @@ int cli_genhash_pe(cli_ctx *ctx, unsigned int class, int type, stats_section_t *
cli_exe_info_init(peinfo, 0);
if (cli_peheader(*ctx->fmap, peinfo, CLI_PEHEADER_OPT_NONE, NULL) != CLI_PEHEADER_RET_SUCCESS) {
cli_exe_info_destroy(peinfo);
return CL_EFORMAT;
}

@ -72,10 +72,6 @@ struct cli_pe_hook_data {
int cli_scanpe(cli_ctx *ctx);
#define CL_CHECKFP_PE_FLAG_NONE 0x00000000
#define CL_CHECKFP_PE_FLAG_STATS 0x00000001
#define CL_CHECKFP_PE_FLAG_AUTHENTICODE 0x00000002
enum {
CL_GENHASH_PE_CLASS_SECTION,
CL_GENHASH_PE_CLASS_IMPTBL,
@ -89,6 +85,7 @@ enum {
#define CLI_PEHEADER_OPT_DBG_PRINT_INFO 0x2
#define CLI_PEHEADER_OPT_EXTRACT_VINFO 0x4
#define CLI_PEHEADER_OPT_STRICT_ON_PE_ERRORS 0x8
#define CLI_PEHEADER_OPT_REMOVE_MISSING_SECTIONS 0x10
#define CLI_PEHEADER_RET_SUCCESS 0
#define CLI_PEHEADER_RET_GENERIC_ERROR -1

Loading…
Cancel
Save