From 14d52d0c63a0cc7966bc404832ef1281b0befed4 Mon Sep 17 00:00:00 2001 From: Andrew <36489577+recvfrom@users.noreply.github.com> Date: Mon, 4 Feb 2019 18:48:22 -0500 Subject: [PATCH] Use genhash_pe instead of checkfp_pe for section hash computation cli_checkfp_pe is now effectively the function that just checks the Authenticode hash. This makes the code less complicated, and adds some minor improvements: - section hashes are no longer computed if there is no stats callback function (at least in that part of the code) - We now actually set the len field in the stats_section_t structure - If an error occurs when computing a section hash, we skip that section instead of not computing any hashes --- libclamav/matcher.c | 31 ++++--- libclamav/pe.c | 191 ++++++++++++++++++-------------------------- libclamav/pe.h | 4 +- sigtool/sigtool.c | 9 ++- 4 files changed, 102 insertions(+), 133 deletions(-) diff --git a/libclamav/matcher.c b/libclamav/matcher.c index 5a96ca925..a3c730048 100644 --- a/libclamav/matcher.c +++ b/libclamav/matcher.c @@ -567,7 +567,6 @@ int cli_checkfp_virus(unsigned char *digest, size_t size, cli_ctx *ctx, const ch uint8_t shash1[SHA1_HASH_SIZE * 2 + 1]; uint8_t shash256[SHA256_HASH_SIZE * 2 + 1]; int have_sha1, have_sha256, do_dsig_check = 1; - stats_section_t sections; if (cli_hm_scan(digest, size, &virname, ctx->engine->hm_fp, CLI_HASH_MD5) == CL_VIRUS) { cli_dbgmsg("cli_checkfp(md5): Found false positive detection (fp sig: %s), size: %d\n", virname, (int)size); @@ -585,6 +584,8 @@ int cli_checkfp_virus(unsigned char *digest, size_t size, cli_ctx *ctx, const ch vname ? vname : "Name"); } + // TODO Replace this with the ability to actually perform detection with + // the blacklisted sig entries if (vname) do_dsig_check = strncmp("W32S.", vname, 5); @@ -652,17 +653,10 @@ int cli_checkfp_virus(unsigned char *digest, size_t size, cli_ctx *ctx, const ch } #endif - memset(§ions, 0x00, sizeof(stats_section_t)); - if (do_dsig_check || ctx->engine->cb_stats_add_sample) { - uint32_t flags = (do_dsig_check ? CL_CHECKFP_PE_FLAG_AUTHENTICODE : 0); - if (!(ctx->engine->engine_options & ENGINE_OPTIONS_DISABLE_PE_STATS) && !(ctx->engine->dconf->stats & (DCONF_STATS_DISABLED | DCONF_STATS_PE_SECTION_DISABLED))) - flags |= CL_CHECKFP_PE_FLAG_STATS; - - switch (cli_checkfp_pe(ctx, §ions, flags)) { + if (do_dsig_check) { + switch (cli_checkfp_pe(ctx)) { case CL_CLEAN: cli_dbgmsg("cli_checkfp(pe): PE file whitelisted due to valid digital signature\n"); - if (sections.sections) - free(sections.sections); return CL_CLEAN; default: break; @@ -672,11 +666,22 @@ int cli_checkfp_virus(unsigned char *digest, size_t size, cli_ctx *ctx, const ch if (ctx->engine->cb_hash) ctx->engine->cb_hash(fmap_fd(*ctx->fmap), size, (const unsigned char *)md5, vname ? vname : "noname", ctx->cb_ctx); - if (ctx->engine->cb_stats_add_sample) + if (ctx->engine->cb_stats_add_sample) { + stats_section_t sections; + memset(§ions, 0x00, sizeof(stats_section_t)); + + if (!(ctx->engine->engine_options & ENGINE_OPTIONS_DISABLE_PE_STATS) && + !(ctx->engine->dconf->stats & (DCONF_STATS_DISABLED | DCONF_STATS_PE_SECTION_DISABLED))) + cli_genhash_pe(ctx, CL_GENHASH_PE_CLASS_SECTION, 1, §ions); + + // TODO We probably only want to call cb_stats_add_sample when + // sections.section != NULL... leaving as is for now ctx->engine->cb_stats_add_sample(vname ? vname : "noname", digest, size, §ions, ctx->engine->stats_data); - if (sections.sections) - free(sections.sections); + if (sections.sections) { + free(sections.sections); + } + } return CL_VIRUS; } diff --git a/libclamav/pe.c b/libclamav/pe.c index a3cb54f5a..8b2b52b3a 100644 --- a/libclamav/pe.c +++ b/libclamav/pe.c @@ -5508,29 +5508,8 @@ static int sort_sects(const void *first, const void *second) * CL_CLEAN will be returned if the file was whitelisted based on its * signature. CL_VIRUS will be returned if the file was blacklisted based on * its signature. Otherwise, an cl_error_t error value will be returned. - * - * Also, this function computes the hashes of each section (sorted based on the - * RVAs of the sections) if the CL_CHECKFP_PE_FLAG_STATS flag exists in flags - * - * TODO The code to compute the section hashes is copied from - * cli_genhash_pe - we should use that function instead where this - * functionality is needed, since we no longer need to compute the section - * hashes as part of the authenticode hash calculation. - * - * If the section hashes are to be computed and returned, this function - * allocates memory for the section hashes, and it's up to the caller to free - * it. hashes->sections will be initialized to NULL at the beginning of the - * function, and if after the call it's value is non-NULL, the memory should be - * freed. Furthermore, if hashes->sections is non-NULL, the hashes can assume - * to be valid regardless of the return code. - * - * Also, a few other notes: - * - If a section has a virtual size of zero, it's corresponding hash value - * will not be computed and the hash contents will be all zeroes. - * - TODO Instead of not providing back any hashes when an invalid section is - * encountered, would it be better to still compute hashes for the valid - * sections? */ -cl_error_t cli_checkfp_pe(cli_ctx *ctx, stats_section_t *hashes, uint32_t flags) + */ +cl_error_t cli_checkfp_pe(cli_ctx *ctx) { size_t at; unsigned int i, hlen; @@ -5554,15 +5533,6 @@ cl_error_t cli_checkfp_pe(cli_ctx *ctx, stats_section_t *hashes, uint32_t flags) if (!(DCONF & PE_CONF_CATALOG)) return CL_EFORMAT; - if (flags == CL_CHECKFP_PE_FLAG_NONE) - return CL_BREAK; - - if (flags & CL_CHECKFP_PE_FLAG_STATS) { - if (!(hashes)) - return CL_ENULLARG; - hashes->sections = NULL; - } - // TODO see if peinfo can be passed in (or lives in ctx or something) and // if so, use that to avoid having to re-parse the header cli_exe_info_init(peinfo, 0); @@ -5579,68 +5549,12 @@ cl_error_t cli_checkfp_pe(cli_ctx *ctx, stats_section_t *hashes, uint32_t flags) // the section hashes), bail out if we don't have any Authenticode hashes // loaded from .cat files if (sec_dir_size < 8 && !cli_hm_have_size(ctx->engine->hm_fp, CLI_HASH_SHA1, 2)) { - if (flags & CL_CHECKFP_PE_FLAG_STATS) { - /* If stats is enabled, continue parsing the sample */ - flags ^= CL_CHECKFP_PE_FLAG_AUTHENTICODE; - } else { - cli_exe_info_destroy(peinfo); - return CL_BREAK; - } + cli_exe_info_destroy(peinfo); + return CL_BREAK; } fsize = map->len; - if (flags & CL_CHECKFP_PE_FLAG_STATS) { - hashes->nsections = peinfo->nsections; - hashes->sections = cli_calloc(peinfo->nsections, sizeof(struct cli_section_hash)); - ; - if (!(hashes->sections)) { - cli_exe_info_destroy(peinfo); - return CL_EMEM; - } - } - -#define free_section_hashes() \ - do { \ - if (flags & CL_CHECKFP_PE_FLAG_STATS) { \ - free(hashes->sections); \ - hashes->sections = NULL; \ - } \ - } while (0) - - // TODO This likely isn't needed anymore, since we no longer compute - // the authenticode hash like the 2008 spec doc says (sort sections - // and use the section info to compute the hash) - cli_qsort(peinfo->sections, peinfo->nsections, sizeof(*peinfo->sections), sort_sects); - - /* Hash the sections */ - if (flags & CL_CHECKFP_PE_FLAG_STATS) { - - for (i = 0; i < peinfo->nsections; i++) { - const uint8_t *hptr; - void *md5ctx; - - if (!peinfo->sections[i].rsz) - continue; - - if (!(hptr = fmap_need_off_once(map, peinfo->sections[i].raw, peinfo->sections[i].rsz))) { - cli_exe_info_destroy(peinfo); - free_section_hashes(); - return CL_EFORMAT; - } - md5ctx = cl_hash_init("md5"); - if (md5ctx) { - cl_update_hash(md5ctx, (void *)hptr, peinfo->sections[i].rsz); - cl_finish_hash(md5ctx, hashes->sections[i].md5); - } - } - } - - /* After this point it's the caller's responsibility to free - * hashes->sections. Also, in the case where we are just computing the - * stats, we are finished */ - - while (flags & CL_CHECKFP_PE_FLAG_AUTHENTICODE) { - + do { // We'll build a list of the regions that need to be hashed and pass it to // asn1_check_mscat to do hash verification there (the hash algorithm is // specified in the PKCS7 structure). We need to hash up to 4 regions @@ -5651,13 +5565,11 @@ cl_error_t cli_checkfp_pe(cli_ctx *ctx, stats_section_t *hashes, uint32_t flags) } nregions = 0; -#define add_chunk_to_hash_list(_offset, _size) \ - do { \ - if (flags & CL_CHECKFP_PE_FLAG_AUTHENTICODE) { \ - regions[nregions].offset = (_offset); \ - regions[nregions].size = (_size); \ - nregions++; \ - } \ +#define add_chunk_to_hash_list(_offset, _size) \ + do { \ + regions[nregions].offset = (_offset); \ + regions[nregions].size = (_size); \ + nregions++; \ } while (0) // Pretty much every case below should return CL_EFORMAT @@ -5802,7 +5714,8 @@ cl_error_t cli_checkfp_pe(cli_ctx *ctx, stats_section_t *hashes, uint32_t flags) ret = CL_EVERIFY; break; - } /* while(flags & CL_CHECKFP_PE_FLAG_AUTHENTICODE) */ + + } while (0); if (NULL != hashctx) { cl_hash_destroy(hashctx); @@ -5816,7 +5729,26 @@ cl_error_t cli_checkfp_pe(cli_ctx *ctx, stats_section_t *hashes, uint32_t flags) return ret; } -int cli_genhash_pe(cli_ctx *ctx, unsigned int class, int type) +/* Print out either the MD5, SHA1, or SHA256 associated with the imphash or + * the individual sections. Also, this function computes the hashes of each + * section (sorted based on the RVAs of the sections) if hashes is non-NULL. + * + * If the section hashes are to be computed and returned, this function + * allocates memory for the section hashes, and it's up to the caller to free + * it. hashes->sections will be initialized to NULL at the beginning of the + * function, and if after the call it's value is non-NULL, the memory should be + * freed. Furthermore, if hashes->sections is non-NULL, the hashes can assume + * to be valid regardless of the return code. + * + * Also, a few other notes: + * - If a section has a virtual size of zero, it's corresponding hash value + * will not be computed and the hash contents will be all zeroes. + * - If a section extends beyond the end of the file, the section data and + * length will be truncated, and the hash generated accordingly + * - If a section exists completely outside of the file, it won't be included + * in the list of sections, and nsections will be adjusted accordingly. + */ +int cli_genhash_pe(cli_ctx *ctx, unsigned int class, int type, stats_section_t *hashes) { unsigned int i; struct cli_exe_info _peinfo; @@ -5826,9 +5758,20 @@ int cli_genhash_pe(cli_ctx *ctx, unsigned int class, int type) int genhash[CLI_HASH_AVAIL_TYPES]; int hlen = 0; + if (hashes) { + hashes->sections = NULL; + + if (class != CL_GENHASH_PE_CLASS_SECTION || type != 1) { + cli_dbgmsg("`hashes` can only be populated with MD5 PE section data\n"); + return CL_EARG; + } + } + if (class >= CL_GENHASH_PE_CLASS_LAST) return CL_EARG; + // TODO see if peinfo can be passed in (or lives in ctx or something) and + // if so, use that to avoid having to re-parse the header cli_exe_info_init(peinfo, 0); if (cli_peheader(*ctx->fmap, peinfo, CLI_PEHEADER_OPT_NONE, NULL) != CLI_PEHEADER_RET_SUCCESS) { @@ -5864,35 +5807,54 @@ int cli_genhash_pe(cli_ctx *ctx, unsigned int class, int type) return CL_EMEM; } + if (hashes) { + hashes->nsections = peinfo->nsections; + hashes->sections = cli_calloc(peinfo->nsections, sizeof(struct cli_section_hash)); + + if (!(hashes->sections)) { + cli_exe_info_destroy(peinfo); + free(hash); + return CL_EMEM; + } + } + if (class == CL_GENHASH_PE_CLASS_SECTION) { - char *dstr = NULL; + char *dstr; for (i = 0; i < peinfo->nsections; i++) { /* Generate hashes */ if (cli_hashsect(*ctx->fmap, &peinfo->sections[i], hashset, genhash, genhash) == 1) { - dstr = cli_str2hex((char *)hash, hlen); - cli_dbgmsg("Section{%u}: %u:%s\n", i, peinfo->sections[i].rsz, dstr ? (char *)dstr : "(NULL)"); - if (dstr != NULL) { - free(dstr); - dstr = NULL; + if (cli_debug_flag) { + dstr = cli_str2hex((char *)hash, hlen); + cli_dbgmsg("Section{%u}: %u:%s\n", i, peinfo->sections[i].rsz, dstr ? (char *)dstr : "(NULL)"); + if (dstr != NULL) { + free(dstr); + } } - } else { + if (hashes) { + memcpy(hashes->sections[i].md5, hash, sizeof(hashes->sections[i].md5)); + hashes->sections[i].len = peinfo->sections[i].rsz; + } + } else if (peinfo->sections[i].rsz) { cli_dbgmsg("Section{%u}: failed to generate hash for section\n", i); + } else { + cli_dbgmsg("Section{%u}: section contains no data\n", i); } } } else if (class == CL_GENHASH_PE_CLASS_IMPTBL) { - char *dstr = NULL; + char *dstr; uint32_t impsz = 0; int ret; /* Generate hash */ ret = hash_imptbl(ctx, hashset, &impsz, genhash, peinfo); if (ret == CL_SUCCESS) { - dstr = cli_str2hex((char *)hash, hlen); - cli_dbgmsg("Imphash: %s:%u\n", dstr ? (char *)dstr : "(NULL)", impsz); - if (dstr != NULL) { - free(dstr); - dstr = NULL; + if (cli_debug_flag) { + dstr = cli_str2hex((char *)hash, hlen); + cli_dbgmsg("Imphash: %s:%u\n", dstr ? (char *)dstr : "(NULL)", impsz); + if (dstr != NULL) { + free(dstr); + } } } else { cli_dbgmsg("Imphash: failed to generate hash for import table (%d)\n", ret); @@ -5901,8 +5863,7 @@ int cli_genhash_pe(cli_ctx *ctx, unsigned int class, int type) cli_dbgmsg("cli_genhash_pe: unknown pe genhash class: %u\n", class); } - if (hash) - free(hash); + free(hash); cli_exe_info_destroy(peinfo); return CL_SUCCESS; } diff --git a/libclamav/pe.h b/libclamav/pe.h index 4895f3fa5..b7bcc0352 100644 --- a/libclamav/pe.h +++ b/libclamav/pe.h @@ -98,8 +98,8 @@ enum { int cli_pe_targetinfo(fmap_t *map, struct cli_exe_info *peinfo); int cli_peheader(fmap_t *map, struct cli_exe_info *peinfo, uint32_t opts, cli_ctx *ctx); -cl_error_t cli_checkfp_pe(cli_ctx *ctx, stats_section_t *hashes, uint32_t flags); -int cli_genhash_pe(cli_ctx *ctx, unsigned int class, int type); +cl_error_t cli_checkfp_pe(cli_ctx *ctx); +int cli_genhash_pe(cli_ctx *ctx, unsigned int class, int type, stats_section_t *hashes); uint32_t cli_rawaddr(uint32_t, const struct cli_exe_section *, uint16_t, unsigned int *, size_t, uint32_t); void findres(uint32_t, uint32_t, fmap_t *map, struct cli_exe_info *, int (*)(void *, uint32_t, uint32_t, uint32_t, uint32_t), void *); diff --git a/sigtool/sigtool.c b/sigtool/sigtool.c index a31ba4275..70c517be9 100644 --- a/sigtool/sigtool.c +++ b/sigtool/sigtool.c @@ -255,10 +255,10 @@ static int hashpe(const char *filename, unsigned int class, int type) /* Send to PE-specific hasher */ switch (class) { case 1: - ret = cli_genhash_pe(&ctx, CL_GENHASH_PE_CLASS_SECTION, type); + ret = cli_genhash_pe(&ctx, CL_GENHASH_PE_CLASS_SECTION, type, NULL); break; case 2: - ret = cli_genhash_pe(&ctx, CL_GENHASH_PE_CLASS_IMPTBL, type); + ret = cli_genhash_pe(&ctx, CL_GENHASH_PE_CLASS_IMPTBL, type, NULL); break; default: mprintf("!hashpe: unknown classification(%u) for pe hash!\n", class); @@ -3455,7 +3455,7 @@ static int dumpcerts(const struct optstruct *opts) return -1; } - ret = cli_checkfp_pe(&ctx, NULL, CL_CHECKFP_PE_FLAG_AUTHENTICODE); + ret = cli_checkfp_pe(&ctx); switch (ret) { case CL_CLEAN: @@ -3467,6 +3467,9 @@ static int dumpcerts(const struct optstruct *opts) case CL_BREAK: mprintf("*dumpcerts: CL_BREAK after cli_checkfp_pe()!\n"); break; + case CL_EVERIFY: + mprintf("!dumpcerts: CL_EVERIFY after cli_checkfp_pe()!\n"); + break; case CL_EFORMAT: mprintf("!dumpcerts: An error occurred when parsing the file\n"); break;