@@ -1365,7 +1365,7 @@ static int asn1_parse_countersignature(fmap_t *map, const void **asn1data, unsig

     return 1;

 }

-static cl_error_t asn1_parse_mscat(fmap_t *map, size_t offset, unsigned int size, crtmgr *cmgr, int embedded, const void **hashes, unsigned int *hashes_size, struct cl_engine *engine)

+static cl_error_t asn1_parse_mscat(struct cl_engine *engine, fmap_t *map, size_t offset, unsigned int size, crtmgr *cmgr, int embedded, const void **hashes, unsigned int *hashes_size, char **certname)

 {

     struct cli_asn1 asn1, deep, deeper;

     uint8_t issuer[SHA1_HASH_SIZE], serial[SHA1_HASH_SIZE];

@@ -1555,6 +1555,11 @@ static cl_error_t asn1_parse_mscat(fmap_t *map, size_t offset, unsigned int size

                             sprintf(&serial[j * 2], "%02x", x509->serial[j]);

                         }

+                        // TODO The raw information we print out here isn't

+                        // very helpful, since it's only the first 64-bytes...

+                        // Change this so that raw is only populated when the

+                        // debug flag is set, and then copy/display the full

+                        // contents.

                         cli_dbgmsg_internal("cert:\n");

                         cli_dbgmsg_internal("  subject: %s\n", subject);

                         cli_dbgmsg_internal("  serial: %s\n", serial);

@@ -1593,7 +1598,26 @@ static cl_error_t asn1_parse_mscat(fmap_t *map, size_t offset, unsigned int size

                             // in cmgr for this certificate, not a blacklist

                             // entry for this certificate's parent

                             ret = CL_VIRUS;

-                            cli_dbgmsg("asn1_parse_mscat: Authenticode certificate %s is revoked. Flagging sample as virus.\n", (parent->name ? parent->name : "(no name)"));

+                            cli_dbgmsg("asn1_parse_mscat: Authenticode certificate %s is revoked. Flagging sample as a virus.\n", (parent->name ? parent->name : "(no name)"));

+                            if (NULL != certname) {

+                                // We need to pass back the certname from a

+                                // place where it won't go away (and nothing

+                                // above here knows to free it) so just find it

+                                // again in the engine->cmgr.

+                                // TODO Refactor this so that blacklist cert

+                                // lookups happen against engine->cmgr so we

+                                // don't have to lookup twice

+                                if (cmgr != &(engine->cmgr)) {

+                                    parent = crtmgr_verify_crt(&(engine->cmgr), x509);

+                                }

+

+                                if (NULL == parent) {

+                                    cli_dbgmsg("asn1_parse_mscat: Unexpected case - blacklist cert can't be found in the global list\n");

+                                    *certname = "(error fetching name)";

+                                } else {

+                                    *certname = parent->name ? parent->name : "(no name)";

+                                }

+                            }

                             crtmgr_free(&newcerts);

                             goto finish;

                         }

@@ -2046,7 +2070,7 @@ int asn1_load_mscat(fmap_t *map, struct cl_engine *engine)

     // TODO Since we pass engine->cmgr directly here, the whole chain of trust

     // for this .cat file will get added to the global trust store assuming it

     // verifies successfully.  Is this a bug for a feature?

-    if (CL_CLEAN != asn1_parse_mscat(map, 0, map->len, &engine->cmgr, 0, &c.next, &size, engine))

+    if (CL_CLEAN != asn1_parse_mscat(engine, map, 0, map->len, &engine->cmgr, 0, &c.next, &size, NULL))

         return 1;

     if (asn1_expect_objtype(map, c.next, &size, &c, ASN1_TYPE_SEQUENCE))

@@ -2179,6 +2203,9 @@ int asn1_load_mscat(fmap_t *map, struct cl_engine *engine)

                 engine->hm_fp->mempool = engine->mempool;

 #endif

             }

+            /* Load the trusted hashes into hm_fp, using the size values

+             * 1 and 2 as sentinel values corresponding to CAB and PE hashes

+             * from .cat files respectively. */

             if (hm_addhash_bin(engine->hm_fp, tagval3.content, CLI_HASH_SHA1, hashtype, NULL)) {

                 cli_warnmsg("asn1_load_mscat: failed to add hash\n");

                 return 1;

@@ -2190,11 +2217,14 @@ int asn1_load_mscat(fmap_t *map, struct cl_engine *engine)

 }

 /* Check an embedded PE Authenticode section to determine whether it's trusted.

- * This will return CL_CLEAN if the file should be trusted, CL_EPARSE if an

+ * This will return CL_VERIFIED if the file should be trusted, CL_EPARSE if an

  * error occurred while parsing the signature, CL_EVERIFY if parsing was

  * successful but there were no whitelist rules for the signature, and

- * CL_VIRUS if a blacklist rule was found for an embedded certificate. */

-cl_error_t asn1_check_mscat(struct cl_engine *engine, fmap_t *map, size_t offset, unsigned int size, struct cli_mapped_region *regions, uint32_t nregions)

+ * CL_VIRUS if a blacklist rule was found for an embedded certificate.

+ *

+ * If CL_VIRUS is returned, certname will be set to the certname of blacklist

+ * rule that matched (unless certname is NULL). */

+cl_error_t asn1_check_mscat(struct cl_engine *engine, fmap_t *map, size_t offset, unsigned int size, struct cli_mapped_region *regions, uint32_t nregions, char **certname)

 {

     unsigned int content_size;

     struct cli_asn1 c;

@@ -2207,7 +2237,7 @@ cl_error_t asn1_check_mscat(struct cl_engine *engine, fmap_t *map, size_t offset

     void *ctx;

     unsigned int i;

-    // TODO Move these into cli_checkfp_pe

+    // TODO Move these into cli_check_auth_header

     if (!(engine->dconf->pe & PE_CONF_CERTS))

         return CL_EVERIFY;

     if (engine->engine_options & ENGINE_OPTIONS_DISABLE_PE_CERTS)

@@ -2219,7 +2249,7 @@ cl_error_t asn1_check_mscat(struct cl_engine *engine, fmap_t *map, size_t offset

         crtmgr_free(&certs);

         return CL_EVERIFY;

     }

-    ret = asn1_parse_mscat(map, offset, size, &certs, 1, &content, &content_size, engine);

+    ret = asn1_parse_mscat(engine, map, offset, size, &certs, 1, &content, &content_size, certname);

     crtmgr_free(&certs);

     if (CL_CLEAN != ret)

         return ret;

@@ -2288,5 +2318,5 @@ cl_error_t asn1_check_mscat(struct cl_engine *engine, fmap_t *map, size_t offset

     }

     cli_dbgmsg("asn1_check_mscat: file with valid authenticode signature, whitelisted\n");

-    return CL_CLEAN;

+    return CL_VERIFIED;

 }

@@ -31,6 +31,6 @@ struct cli_mapped_region {

 };

 int asn1_load_mscat(fmap_t *map, struct cl_engine *engine);

-cl_error_t asn1_check_mscat(struct cl_engine *engine, fmap_t *map, size_t offset, unsigned int size, struct cli_mapped_region *regions, uint32_t nregions);

+cl_error_t asn1_check_mscat(struct cl_engine *engine, fmap_t *map, size_t offset, unsigned int size, struct cli_mapped_region *regions, uint32_t nregions, char **certname);

 #endif

@@ -117,6 +117,8 @@ typedef enum cl_error_t {

     CL_EBUSY,

     CL_ESTATE,

+    CL_VERIFIED, /* The binary has been deemed trusted */

+

     /* no error codes below this line please */

     CL_ELAST_ERROR

 } cl_error_t;

@@ -107,8 +107,13 @@ cli_crt *crtmgr_blacklist_lookup(crtmgr *m, cli_crt *x509)

         // The CRB rules are based on subject, serial, and public key,

         // so do blacklist queries based on those fields

+        // TODO the rule format specifies CodeSign / TimeSign / CertSign

+        // which we could also match on, but we just ignore those fields

+        // for blacklist certs for now

+

         // TODO Handle the case where these items aren't specified in a CRB

-        // rule entry - substitute in default values instead.

+        // rule entry - substitute in default values instead (or make the

+        // crb parser not permit leaving these fields blank).

         if (i->isBlacklisted &&

             !memcmp(i->subject, x509->subject, sizeof(i->subject)) &&

@@ -407,16 +412,24 @@ cli_crt *crtmgr_verify_crt(crtmgr *m, cli_crt *x509)

     int score             = 0;

     unsigned int possible = 0;

+    // For a blacklist cert, if it exists in the signature then that is

+    // enough to report back a match.  We can't whitelist this way, though,

+    // or an attacker could just include a known-good certificate in the

+    // signature and just not use it.

     if (NULL != (i = crtmgr_blacklist_lookup(m, x509))) {

         return i;

     }

+    // Loop through each of the certificates in our trust store and see whether

+    // x509 is signed with it.  If it is, it's trusted

+

     // TODO Technically we should loop through all of the blacklisted certs

     // first to see whether one of those is used to sign x509.  This case

     // will get handled if the blacklisted certificate is embedded, since we

     // will call crtmgr_verify_crt on it and match against the blacklist entry

     // that way, but the cert doesn't HAVE to be embedded.  This case seems

-    // unlikely enough to ignore, though.

+    // unlikely enough to ignore, though.  If we ever want to blacklist a

+    // stolen CA cert or something, then we will need to revisit this.

     for (i = m->crts; i; i = i->next) {

         if (i->certSign &&

@@ -36,7 +36,10 @@ typedef enum { CLI_SHA1RSA,

 typedef enum { VRFY_CODE,

                VRFY_TIME } cli_vrfy_type;

+#ifndef CRT_RAWMAXLEN

 #define CRT_RAWMAXLEN 64

+#endif

+

 typedef struct cli_crt_t {

     char *name;

     uint8_t raw_subject[CRT_RAWMAXLEN];

@@ -176,7 +176,7 @@ CLAMAV_PRIVATE {

     cli_initroots;

     cli_scanbuff;

     cli_fmap_scandesc;

-    cli_checkfp_pe;

+    cli_check_auth_header;

     cli_genhash_pe;

     html_screnc_decode;

     mpool_create;

@@ -566,7 +566,7 @@ int cli_checkfp_virus(unsigned char *digest, size_t size, cli_ctx *ctx, const ch

     const char *ptr;

     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;

+    int have_sha1, have_sha256;

     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);

@@ -584,13 +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);

-

     map         = *ctx->fmap;

-    have_sha1   = cli_hm_have_size(ctx->engine->hm_fp, CLI_HASH_SHA1, size) || cli_hm_have_wild(ctx->engine->hm_fp, CLI_HASH_SHA1) || (cli_hm_have_size(ctx->engine->hm_fp, CLI_HASH_SHA1, 1) && do_dsig_check);

+    have_sha1   = cli_hm_have_size(ctx->engine->hm_fp, CLI_HASH_SHA1, size) || cli_hm_have_wild(ctx->engine->hm_fp, CLI_HASH_SHA1) || cli_hm_have_size(ctx->engine->hm_fp, CLI_HASH_SHA1, 1);

     have_sha256 = cli_hm_have_size(ctx->engine->hm_fp, CLI_HASH_SHA256, size) || cli_hm_have_wild(ctx->engine->hm_fp, CLI_HASH_SHA256);

     if (have_sha1 || have_sha256) {

         if ((ptr = fmap_need_off_once(map, 0, size))) {

@@ -605,7 +600,9 @@ int cli_checkfp_virus(unsigned char *digest, size_t size, cli_ctx *ctx, const ch

                     cli_dbgmsg("cli_checkfp(sha1): Found false positive detection (fp sig: %s)\n", virname);

                     return CL_CLEAN;

                 }

-                if (do_dsig_check && cli_hm_scan(&shash1[SHA1_HASH_SIZE], 1, &virname, ctx->engine->hm_fp, CLI_HASH_SHA1) == CL_VIRUS) {

+                /* See whether the hash matches those loaded in from .cat files

+                 * (associated with the .CAB file type) */

+                if (cli_hm_scan(&shash1[SHA1_HASH_SIZE], 1, &virname, ctx->engine->hm_fp, CLI_HASH_SHA1) == CL_VIRUS) {

                     cli_dbgmsg("cli_checkfp(sha1): Found false positive detection via catalog file\n");

                     return CL_CLEAN;

                 }

@@ -653,16 +650,6 @@ int cli_checkfp_virus(unsigned char *digest, size_t size, cli_ctx *ctx, const ch

     }

 #endif

-    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");

-                return CL_CLEAN;

-            default:

-                break;

-        }

-    }

-

     if (ctx->engine->cb_hash)

         ctx->engine->cb_hash(fmap_fd(*ctx->fmap), size, (const unsigned char *)md5, vname ? vname : "noname", ctx->cb_ctx);

@@ -918,7 +905,7 @@ int cli_fmap_scandesc(cli_ctx *ctx, cli_file_t ftype, uint8_t ftonly, struct cli

     struct cli_target_info info;

     fmap_t *map = *ctx->fmap;

     struct cli_matcher *hdb, *fp;

-    const char *virname    = NULL;

+    const char *virname;

     uint32_t viruses_found = 0;

     void *md5ctx, *sha1ctx, *sha256ctx;

@@ -987,6 +974,33 @@ int cli_fmap_scandesc(cli_ctx *ctx, cli_file_t ftype, uint8_t ftonly, struct cli

                    "and bytecode sigs that require exe metadata\n");

     }

+    /* If it's a PE, check the Authenticode header.  This would be more

+     * appropriate in cli_scanpe, but cli_scanraw->cli_fmap_scandesc gets

+     * called first for PEs, and we want to determine the whitelist/blacklist

+     * status early on so we can skip things like embedded PE extraction

+     * (which is broken for signed binaries within signed binaries).

+     * 

+     * If we want to add support for more signature parsing in the future

+     * (Ex: MachO sigs), do that here too. */

+    if (1 == info.status && i == 1) {

+        char *certname = NULL;

+        ret            = cli_check_auth_header(ctx, &(info.exeinfo), &certname);

+

+        if (CL_VIRUS == ret) {

+            ret = cli_append_virus(ctx, certname);

+        }

+

+        if ((ret == CL_VIRUS || ret == CL_VERIFIED) && !SCAN_ALLMATCHES) {

+            cli_targetinfo_destroy(&info);

+            cl_hash_destroy(md5ctx);

+            cl_hash_destroy(sha1ctx);

+            cl_hash_destroy(sha256ctx);

+            return ret;

+        }

+

+        ret = CL_CLEAN;

+    }

+

     if (!ftonly) {

         if ((ret = cli_ac_initdata(&gdata, groot->ac_partsigs, groot->ac_lsigs, groot->ac_reloff_num, CLI_DEFAULT_AC_TRACKLEN)) || (ret = cli_ac_caloff(groot, &gdata, &info))) {

             cli_targetinfo_destroy(&info);

@@ -282,6 +282,8 @@ const char *cl_strerror(int clerror)

             return "Scanner still active";

         case CL_ESTATE:

             return "Bad state (engine not initialized, or already initialized)";

+        case CL_VERIFIED:

+            return "The scanned object was verified and deemed trusted";

         default:

             return "Unknown error code";

     }

@@ -2,7 +2,7 @@

  *  Copyright (C) 2013-2019 Cisco Systems, Inc. and/or its affiliates. All rights reserved.

  *  Copyright (C) 2007-2013 Sourcefire, Inc.

  *

- *  Authors: Alberto Wu, Tomasz Kojm

+ *  Authors: Alberto Wu, Tomasz Kojm, Andrew Williams

  *

  *  Acknowledgements: The header structures were based upon a PE format 

  *                    analysis by B. Luevelsmeyer.

@@ -5505,11 +5505,16 @@ static int sort_sects(const void *first, const void *second)

  * - A PE file has an embedded Authenticode section

  * - The PE file has no embedded Authenticode section but is covered by a

  *   catalog file that was loaded in via a -d 

- * CL_CLEAN will be returned if the file was whitelisted based on its

+ * 

+ * If peinfo is NULL, one will be created internally and used

+ * 

+ * CL_VERIFIED 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.

- */

-cl_error_t cli_checkfp_pe(cli_ctx *ctx)

+ * 

+ * If CL_VIRUS is returned, certname will be set to the certname of blacklist

+ * rule that matched (unless certname is NULL). */

+cl_error_t cli_check_auth_header(cli_ctx *ctx, struct cli_exe_info *peinfo, char **certname)

 {

     size_t at;

     unsigned int i, hlen;

@@ -5524,7 +5529,6 @@ cl_error_t cli_checkfp_pe(cli_ctx *ctx)

     uint32_t sec_dir_offset;

     uint32_t sec_dir_size;

     struct cli_exe_info _peinfo;

-    struct cli_exe_info *peinfo = &_peinfo;

     // If Authenticode parsing has been disabled via DCONF, then don't

     // continue on.

@@ -5533,12 +5537,15 @@ cl_error_t cli_checkfp_pe(cli_ctx *ctx)

     if (!(DCONF & PE_CONF_CATALOG))

         return CL_EFORMAT;

-    // 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 peinfo is NULL, initialize one.  This makes it so that this function

+    // can be used easily by sigtool

+    if (NULL == peinfo) {

+        peinfo = &_peinfo;

+        cli_exe_info_init(peinfo, 0);

-    if (cli_peheader(*ctx->fmap, peinfo, CLI_PEHEADER_OPT_NONE, NULL) != CLI_PEHEADER_RET_SUCCESS) {

-        return CL_EFORMAT;

+        if (cli_peheader(*ctx->fmap, peinfo, CLI_PEHEADER_OPT_NONE, NULL) != CLI_PEHEADER_RET_SUCCESS) {

+            return CL_EFORMAT;

+        }

     }

     sec_dir_offset = EC32(peinfo->dirs[4].VirtualAddress);

@@ -5549,21 +5556,20 @@ cl_error_t cli_checkfp_pe(cli_ctx *ctx)

     // 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)) {

-        cli_exe_info_destroy(peinfo);

-        return CL_BREAK;

+        ret = CL_BREAK;

+        goto finish;

     }

     fsize = map->len;

-    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

-        regions = (struct cli_mapped_region *)cli_calloc(4, sizeof(struct cli_mapped_region));

-        if (!regions) {

-            cli_exe_info_destroy(peinfo);

-            return CL_EMEM;

-        }

-        nregions = 0;

+    // 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

+    regions = (struct cli_mapped_region *)cli_calloc(4, sizeof(struct cli_mapped_region));

+    if (!regions) {

+        ret = CL_EMEM;

+        goto finish;

+    }

+    nregions = 0;

 #define add_chunk_to_hash_list(_offset, _size) \

     do {                                       \

@@ -5572,74 +5578,74 @@ cl_error_t cli_checkfp_pe(cli_ctx *ctx)

         nregions++;                            \

     } while (0)

-        // Pretty much every case below should return CL_EFORMAT

-        ret = CL_EFORMAT;

+    // Pretty much every case below should return CL_EFORMAT

+    ret = CL_EFORMAT;

-        /* MZ to checksum */

-        at   = 0;

-        hlen = peinfo->e_lfanew + sizeof(struct pe_image_file_hdr) + (peinfo->is_pe32plus ? offsetof(struct pe_image_optional_hdr64, CheckSum) : offsetof(struct pe_image_optional_hdr32, CheckSum));

-        add_chunk_to_hash_list(0, hlen);

-        at = hlen + 4;

+    /* MZ to checksum */

+    at   = 0;

+    hlen = peinfo->e_lfanew + sizeof(struct pe_image_file_hdr) + (peinfo->is_pe32plus ? offsetof(struct pe_image_optional_hdr64, CheckSum) : offsetof(struct pe_image_optional_hdr32, CheckSum));

+    add_chunk_to_hash_list(0, hlen);

+    at = hlen + 4;

-        /* Checksum to security */

-        if (peinfo->is_pe32plus)

-            hlen = sizeof(struct pe_image_optional_hdr64) - offsetof(struct pe_image_optional_hdr64, CheckSum) - 4;

-        else

-            hlen = sizeof(struct pe_image_optional_hdr32) - offsetof(struct pe_image_optional_hdr32, CheckSum) - 4;

+    /* Checksum to security */

+    if (peinfo->is_pe32plus)

+        hlen = sizeof(struct pe_image_optional_hdr64) - offsetof(struct pe_image_optional_hdr64, CheckSum) - 4;

+    else

+        hlen = sizeof(struct pe_image_optional_hdr32) - offsetof(struct pe_image_optional_hdr32, CheckSum) - 4;

-        hlen += sizeof(struct pe_image_data_dir) * 4;

-        add_chunk_to_hash_list(at, hlen);

-        at += hlen + 8;

+    hlen += sizeof(struct pe_image_data_dir) * 4;

+    add_chunk_to_hash_list(at, hlen);

+    at += hlen + 8;

-        if (at > peinfo->hdr_size) {

-            break;

-        }

+    if (at > peinfo->hdr_size) {

+        goto finish;

+    }

-        /* Security to End of header */

-        hlen = peinfo->hdr_size - at;

-        add_chunk_to_hash_list(at, hlen);

-        at += hlen;

+    /* Security to End of header */

+    hlen = peinfo->hdr_size - at;

+    add_chunk_to_hash_list(at, hlen);

+    at += hlen;

-        if (sec_dir_offset) {

+    if (sec_dir_offset) {

-            // Verify that we have all the bytes we expect in the authenticode sig

-            // and that the certificate table is the last thing in the file

-            // (according to the MS13-098 bulletin, this is a requirement)

-            if (fsize != sec_dir_size + sec_dir_offset) {

-                cli_dbgmsg("cli_checkfp_pe: expected authenticode data at the end of the file\n");

-                break;

-            }

+        // Verify that we have all the bytes we expect in the authenticode sig

+        // and that the certificate table is the last thing in the file

+        // (according to the MS13-098 bulletin, this is a requirement)

+        if (fsize != sec_dir_size + sec_dir_offset) {

+            cli_dbgmsg("cli_check_auth_header: expected authenticode data at the end of the file\n");

+            goto finish;

+        }

-            // Hash everything from the end of the header to the start of the

-            // security section

-            if (at < sec_dir_offset) {

-                hlen = sec_dir_offset - at;

-                add_chunk_to_hash_list(at, hlen);

-            } else {

-                cli_dbgmsg("cli_checkfp_pe: security directory offset appears to overlap with the PE header\n");

-                break;

-            }

+        // Hash everything from the end of the header to the start of the

+        // security section

+        if (at < sec_dir_offset) {

+            hlen = sec_dir_offset - at;

+            add_chunk_to_hash_list(at, hlen);

+        } else {

+            cli_dbgmsg("cli_check_auth_header: security directory offset appears to overlap with the PE header\n");

+            goto finish;

+        }

-            // Parse the security directory header

+        // Parse the security directory header

-            if (fmap_readn(map, &cert_hdr, sec_dir_offset, sizeof(cert_hdr)) != sizeof(cert_hdr)) {

-                break;

-            }

+        if (fmap_readn(map, &cert_hdr, sec_dir_offset, sizeof(cert_hdr)) != sizeof(cert_hdr)) {

+            goto finish;

+        }

-            if (EC16(cert_hdr.revision) != WIN_CERT_REV_2) {

-                cli_dbgmsg("cli_checkfp_pe: unsupported authenticode data revision\n");

-                break;

-            }

+        if (EC16(cert_hdr.revision) != WIN_CERT_REV_2) {

+            cli_dbgmsg("cli_check_auth_header: unsupported authenticode data revision\n");

+            goto finish;

+        }

-            if (EC16(cert_hdr.type) != WIN_CERT_TYPE_PKCS7) {

-                cli_dbgmsg("cli_checkfp_pe: unsupported authenticode data type\n");

-                break;

-            }

+        if (EC16(cert_hdr.type) != WIN_CERT_TYPE_PKCS7) {

+            cli_dbgmsg("cli_check_auth_header: unsupported authenticode data type\n");

+            goto finish;

+        }

-            hlen = sec_dir_size;

+        hlen = sec_dir_size;

-            if (EC32(cert_hdr.length) != hlen) {

-                /* This is the case that MS13-098 aimed to address, but it got

+        if (EC32(cert_hdr.length) != hlen) {

+            /* This is the case that MS13-098 aimed to address, but it got

                  * pushback to where the fix (not allowing additional, non-zero

                  * bytes in the security directory) is now opt-in via a registry

                  * key.  Given that most machines will treat these binaries as

@@ -5647,76 +5653,74 @@ cl_error_t cli_checkfp_pe(cli_ctx *ctx)

                  * our whitelist signatures are tailored enough to where any

                  * instances of this are reasonable (for instance, I saw one

                  * binary that appeared to use this to embed a license key.) */

-                cli_dbgmsg("cli_checkfp_pe: MS13-098 violation detected, but continuing on to verify certificate\n");

-            }

-

-            at = sec_dir_offset + sizeof(cert_hdr);

-            hlen -= sizeof(cert_hdr);

+            cli_dbgmsg("cli_check_auth_header: MS13-098 violation detected, but continuing on to verify certificate\n");

+        }

-            ret = asn1_check_mscat((struct cl_engine *)(ctx->engine), map, at, hlen, regions, nregions);

+        at = sec_dir_offset + sizeof(cert_hdr);

+        hlen -= sizeof(cert_hdr);

-            if (CL_CLEAN == ret) {

-                // We validated the embedded signature.  Hooray!

-                break;

-            } else if (CL_VIRUS == ret) {

-                // A blacklist rule hit - don't continue on to check hm_fp for a match

-                break;

-            }

+        ret = asn1_check_mscat((struct cl_engine *)(ctx->engine), map, at, hlen, regions, nregions, certname);

-            // Otherwise, we still need to check to see whether this file is

-            // covered by a .cat file (it's common these days for driver files

-            // to have .cat files covering PEs with embedded signatures)

+        if (CL_VERIFIED == ret) {

+            // We validated the embedded signature.  Hooray!

+            goto finish;

+        } else if (CL_VIRUS == ret) {

+            // A blacklist rule hit - don't continue on to check hm_fp for a match

+            goto finish;

+        }

-        } else {

+        // Otherwise, we still need to check to see whether this file is

+        // covered by a .cat file (it's common these days for driver files

+        // to have .cat files covering PEs with embedded signatures)

-            // Hash everything from the end of the header to the end of the

-            // file

-            if (at < fsize) {

-                hlen = fsize - at;

-                add_chunk_to_hash_list(at, hlen);

-            }

-        }

+    } else {

-        // At this point we should compute the SHA1 authenticode hash to see

-        // whether we've had any hashes added from external catalog files

-        // TODO Is it gauranteed that the hashing algorithm will be SHA1?  If

-        // not, figure out how to handle that case

-        hashctx = cl_hash_init("sha1");

-        if (NULL == hashctx) {

-            ret = CL_EMEM;

-            break;

+        // Hash everything from the end of the header to the end of the

+        // file

+        if (at < fsize) {

+            hlen = fsize - at;

+            add_chunk_to_hash_list(at, hlen);

         }

+    }

-        for (i = 0; i < nregions; i++) {

-            const uint8_t *hptr;

-            if (0 == regions[i].size) {

-                continue;

-            }

-            if (!(hptr = fmap_need_off_once(map, regions[i].offset, regions[i].size))) {

-                break;

-            }

+    // At this point we should compute the SHA1 authenticode hash to see

+    // whether we've had any hashes added from external catalog files

+    // TODO Is it gauranteed that the hashing algorithm will be SHA1?  If

+    // not, figure out how to handle that case

+    hashctx = cl_hash_init("sha1");

+    if (NULL == hashctx) {

+        ret = CL_EMEM;

+        goto finish;

+    }

-            cl_update_hash(hashctx, hptr, regions[i].size);

+    for (i = 0; i < nregions; i++) {

+        const uint8_t *hptr;

+        if (0 == regions[i].size) {

+            continue;

         }

-

-        if (i != nregions) {

+        if (!(hptr = fmap_need_off_once(map, regions[i].offset, regions[i].size))) {

             break;

         }

-        cl_finish_hash(hashctx, authsha1);

-        hashctx = NULL;

+        cl_update_hash(hashctx, hptr, regions[i].size);

+    }

-        if (cli_hm_scan(authsha1, 2, NULL, ctx->engine->hm_fp, CLI_HASH_SHA1) == CL_VIRUS) {

-            cli_dbgmsg("cli_checkfp_pe: PE file whitelisted by catalog file\n");

-            ret = CL_CLEAN;

-            break;

-        }

+    if (i != nregions) {

+        goto finish;

+    }

-        ret = CL_EVERIFY;

-        break;

+    cl_finish_hash(hashctx, authsha1);

+    hashctx = NULL;

+

+    if (cli_hm_scan(authsha1, 2, NULL, ctx->engine->hm_fp, CLI_HASH_SHA1) == CL_VIRUS) {

+        cli_dbgmsg("cli_check_auth_header: PE file whitelisted by catalog file\n");

+        ret = CL_CLEAN;

+        goto finish;

+    }

-    } while (0);

+    ret = CL_EVERIFY;

+finish:

     if (NULL != hashctx) {

         cl_hash_destroy(hashctx);

     }

@@ -5725,7 +5729,10 @@ cl_error_t cli_checkfp_pe(cli_ctx *ctx)

         free(regions);

     }

-    cli_exe_info_destroy(peinfo);

+    // If we created the peinfo, then destroy it.  Otherwise we don't own it

+    if (&_peinfo == peinfo) {

+        cli_exe_info_destroy(peinfo);

+    }

     return ret;

 }

@@ -2,7 +2,7 @@

  *  Copyright (C) 2013-2019 Cisco Systems, Inc. and/or its affiliates. All rights reserved.

  *  Copyright (C) 2007-2013 Sourcefire, Inc.

  *

- *  Authors: Alberto Wu, Tomasz Kojm

+ *  Authors: Alberto Wu, Tomasz Kojm, Andrew Williams

  * 

  *  Acknowledgements: The header structures were based upon a PE format 

  *                    analysis by B. Luevelsmeyer.

@@ -98,7 +98,7 @@ 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);

+cl_error_t cli_check_auth_header(cli_ctx *ctx, struct cli_exe_info *peinfo, char **certname);

 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);

@@ -2402,6 +2402,9 @@ static int cli_scanraw(cli_ctx *ctx, cli_file_t type, uint8_t typercg, cli_file_

     ret = cli_fmap_scandesc(ctx, type == CL_TYPE_TEXT_ASCII ? 0 : type, 0, &ftoffset, acmode, NULL, refhash);

     perf_stop(ctx, PERFT_RAW);

+    // TODO I think this causes embedded file extraction to stop when a

+    // signature has matched in cli_fmap_scandesc, which wouldn't be what

+    // we want if allmatch is specified.

     if (ret >= CL_TYPENO) {

         perf_nested_start(ctx, PERFT_RAWTYPENO, PERFT_SCAN);

         ctx->recursion++;

@@ -2625,6 +2628,25 @@ static int cli_scanraw(cli_ctx *ctx, cli_file_t type, uint8_t typercg, cli_file_

                                              * be found recursively

                                              * through the above call

                                              */

+                                // TODO This method of embedded PE extraction

+                                // is kinda gross in that:

+                                //   - if you have an executable that contains

+                                //     20 other exes, the bytes associated with

+                                //     the last exe will have been included in

+                                //     hash computations and things 20 times

+                                //     (as overlay data to the previously

+                                //     extracted exes).

+                                //   - if you have a signed embedded exe, it

+                                //     will fail to validate after extraction

+                                //     bc it has overlay data, which is a

+                                //     violation of the Authenticode spec.

+                                //   - this method of extraction is subject to

+                                //     the recursion limit, which is fairly

+                                //     low by default (I think 16)

+                                //

+                                // It'd be awesome if we could compute the PE

+                                // size from the PE header and just extract

+                                // that.

                             }

                         }

                         break;

@@ -3403,7 +3425,15 @@ static int magic_scandesc(cli_ctx *ctx, cli_file_t type)

                     cli_bitset_free(ctx->hook_lsig_matches);

                     ctx->hook_lsig_matches = old_hook_lsig_matches;

                     return magic_scandesc_cleanup(ctx, type, hash, hashed_size, cache_clean, res, parent_property);

-                /* CL_VIRUS = malware found, check FP and report */

+                /* CL_VIRUS = malware found, check FP and report.

+                 * Likewise, if the file was determined to be trusted, then we

+                 * can also finish with the scan. (Ex: EXE with a valid

+                 * Authenticode sig.) */

+                case CL_VERIFIED:

+                    // For now just conver CL_VERIFIED to CL_CLEAN, since

+                    // CL_VERIFIED isn't used elsewhere

+                    res = CL_CLEAN;

+                    // Fall through

                 case CL_VIRUS:

                     ret = res;

                     if (SCAN_ALLMATCHES)

@@ -3455,26 +3455,25 @@ static int dumpcerts(const struct optstruct *opts)

         return -1;

     }

-    ret = cli_checkfp_pe(&ctx);

+    ret = cli_check_auth_header(&ctx, NULL, NULL);

     switch (ret) {

-        case CL_CLEAN:

-            mprintf("*dumpcerts: CL_CLEAN after cli_checkfp_pe()!\n");

-            break;

+        case CL_VERIFIED:

         case CL_VIRUS:

-            mprintf("*dumpcerts: CL_VIRUS after cli_checkfp_pe()!\n");

-            break;

-        case CL_BREAK:

-            mprintf("*dumpcerts: CL_BREAK after cli_checkfp_pe()!\n");

+            // These shouldn't happen, since sigtool doesn't load in any sigs

             break;

         case CL_EVERIFY:

-            mprintf("!dumpcerts: CL_EVERIFY after cli_checkfp_pe()!\n");

+            // The Authenticode header was parsed successfully but there were

+            // no applicable whitelist/blacklist rules

+            break;

+        case CL_BREAK:

+            mprintf("*dumpcerts: No Authenticode signature detected\n");

             break;

         case CL_EFORMAT:

             mprintf("!dumpcerts: An error occurred when parsing the file\n");

             break;

         default:

-            mprintf("!dumpcerts: Other error %d inside cli_checkfp_pe.\n", ret);

+            mprintf("!dumpcerts: Other error %d inside cli_check_auth_header.\n", ret);

             break;

     }

@@ -163,7 +163,7 @@ EXPORTS cli_fmap_scandesc @44265 NONAME

 EXPORTS cli_hashset_destroy @44266 NONAME

 EXPORTS cli_detect_environment @44267 NONAME

 EXPORTS cli_filecopy @44268 NONAME

-EXPORTS cli_checkfp_pe @44369 NONAME

+EXPORTS cli_check_auth_header @44369 NONAME

 EXPORTS cli_bytefunc_describe @44370 NONAME

 EXPORTS cli_bytetype_describe @44371 NONAME

 EXPORTS cli_bytevalue_describe @44372 NONAME