Browse code

Fix support for authenticode signatures from external .cat files This commit adds back in support for whitelisting files based on signatures from .cat files loaded in via a '-d' flag to clamscan. This also makes it so that a .crb blacklist rule match can't be overruled by a signature in a .cat file

Andrew authored on 2018/09/15 03:39:47
Showing 6 changed files
... ...
@@ -1360,7 +1360,7 @@ static int asn1_parse_countersignature(fmap_t *map, const void **asn1data, unsig
1360 1360
     return 1;
1361 1361
 }
1362 1362
 
1363
-static int 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) {
1363
+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) {
1364 1364
     struct cli_asn1 asn1, deep, deeper;
1365 1365
     uint8_t issuer[SHA1_HASH_SIZE], serial[SHA1_HASH_SIZE];
1366 1366
     const uint8_t *message, *attrs;
... ...
@@ -1374,7 +1374,7 @@ static int asn1_parse_mscat(fmap_t *map, size_t offset, unsigned int size, crtmg
1374 1374
     cli_crt *x509;
1375 1375
     void *ctx;
1376 1376
     int result;
1377
-    int isBlacklisted = 0;
1377
+    cl_error_t ret = CL_EPARSE;
1378 1378
 
1379 1379
     cli_dbgmsg("in asn1_parse_mscat\n");
1380 1380
 
... ...
@@ -1589,9 +1589,10 @@ static int asn1_parse_mscat(fmap_t *map, size_t offset, unsigned int size, crtmg
1589 1589
                             // NOTE: In this case, parent is a blacklist entry
1590 1590
                             // in cmgr for this certificate, not a blacklist
1591 1591
                             // entry for this certificate's parent
1592
-                            isBlacklisted = 1;
1592
+                            ret = CL_VIRUS;
1593 1593
                             cli_dbgmsg("asn1_parse_mscat: Authenticode certificate %s is revoked. Flagging sample as virus.\n", (parent->name ? parent->name : "(no name)"));
1594
-                            break;
1594
+                            crtmgr_free(&newcerts);
1595
+                            goto finish;
1595 1596
                         }
1596 1597
 
1597 1598
                         // TODO Why is this done?
... ...
@@ -1874,6 +1875,7 @@ static int asn1_parse_mscat(fmap_t *map, size_t offset, unsigned int size, crtmg
1874 1874
         // Verify the authenticatedAttributes
1875 1875
         if(!(x509 = crtmgr_verify_pkcs7(cmgr, issuer, serial, asn1.content, asn1.size, hashtype, hash, VRFY_CODE))) {
1876 1876
             cli_dbgmsg("asn1_parse_mscat: pkcs7 signature verification failed\n");
1877
+            ret = CL_EVERIFY;
1877 1878
             break;
1878 1879
         }
1879 1880
         message = asn1.content;
... ...
@@ -1895,10 +1897,12 @@ static int asn1_parse_mscat(fmap_t *map, size_t offset, unsigned int size, crtmg
1895 1895
 
1896 1896
             if(now < x509->not_before || now > x509->not_after) {
1897 1897
                 cli_dbgmsg("asn1_parse_mscat: no countersignature (unauthAttrs missing) and signing certificate has expired\n");
1898
+                ret = CL_EVERIFY;
1898 1899
                 break;
1899 1900
             }
1900 1901
 
1901 1902
             cli_dbgmsg("asn1_parse_mscat: no countersignature (unauthAttrs missing) but the signing certificate is still valid\n");
1903
+            ret = CL_CLEAN;
1902 1904
             goto finish;
1903 1905
         }
1904 1906
 
... ...
@@ -1998,6 +2002,8 @@ static int asn1_parse_mscat(fmap_t *map, size_t offset, unsigned int size, crtmg
1998 1998
         if(dsize)
1999 1999
             break;
2000 2000
 
2001
+        cli_dbgmsg("asn1_parse_mscat: unauthenticatedAttributes successfully parsed\n");
2002
+
2001 2003
         if (1 != (result & 1)) {
2002 2004
             time_t now;
2003 2005
 
... ...
@@ -2006,23 +2012,22 @@ static int asn1_parse_mscat(fmap_t *map, size_t offset, unsigned int size, crtmg
2006 2006
 
2007 2007
             if(now < x509->not_before || now > x509->not_after) {
2008 2008
                 cli_dbgmsg("asn1_parse_mscat: no countersignature and signing certificate has expired\n");
2009
+                ret = CL_EVERIFY;
2009 2010
                 break;
2010 2011
             }
2011 2012
 
2012 2013
             cli_dbgmsg("asn1_parse_mscat: no countersignature but the signing certificate is still valid\n");
2013 2014
         }
2014 2015
 
2015
-        cli_dbgmsg("asn1_parse_mscat: unauthenticatedAttributes successfully parsed\n");
2016
+        ret = CL_CLEAN;
2016 2017
 
2017
-finish:
2018
-        if (isBlacklisted) {
2019
-            return 1;
2020
-        }
2021
-        return 0;
2022 2018
     } while(0);
2023 2019
 
2024
-    cli_dbgmsg("asn1_parse_mscat: failed to parse catalog\n");
2025
-    return 1;
2020
+finish:
2021
+    if (CL_EPARSE == ret) {
2022
+        cli_dbgmsg("asn1_parse_mscat: failed to parse authenticode section\n");
2023
+    }
2024
+    return ret;
2026 2025
 }
2027 2026
 
2028 2027
 int asn1_load_mscat(fmap_t *map, struct cl_engine *engine) {
... ...
@@ -2031,7 +2036,15 @@ int asn1_load_mscat(fmap_t *map, struct cl_engine *engine) {
2031 2031
     struct cli_matcher *db;
2032 2032
     int i;
2033 2033
 
2034
-    if(asn1_parse_mscat(map, 0, map->len, &engine->cmgr, 0, &c.next, &size, engine))
2034
+    // TODO As currently implemented, loading in a .cat file with -d requires
2035
+    // an accompanying .crb with whitelist entries that will cause the .cat
2036
+    // file signatures to verify successfully.  If a user is specifying a .cat
2037
+    // file to use, though, we should assume they trust it and at least add the
2038
+    // covered hashes from it to hm_fp
2039
+    // TODO Since we pass engine->cmgr directly here, the whole chain of trust
2040
+    // for this .cat file will get added to the global trust store assuming it
2041
+    // verifies successfully.  Is this a bug for a feature?
2042
+    if(CL_CLEAN != asn1_parse_mscat(map, 0, map->len, &engine->cmgr, 0, &c.next, &size, engine))
2035 2043
         return 1;
2036 2044
 
2037 2045
     if(asn1_expect_objtype(map, c.next, &size, &c, ASN1_TYPE_SEQUENCE))
... ...
@@ -2172,7 +2185,12 @@ int asn1_load_mscat(fmap_t *map, struct cl_engine *engine) {
2172 2172
     return 0;
2173 2173
 }
2174 2174
 
2175
-int asn1_check_mscat(struct cl_engine *engine, fmap_t *map, size_t offset, unsigned int size, struct cli_mapped_region *regions, uint32_t nregions) {
2175
+/* Check an embedded PE Authenticode section to determine whether it's trusted.
2176
+ * This will return CL_CLEAN if the file should be trusted, CL_EPARSE if an
2177
+ * error occurred while parsing the signature, CL_EVERIFY if parsing was
2178
+ * successful but there were no whitelist rules for the signature, and
2179
+ * CL_VIRUS if a blacklist rule was found for an embedded certificate. */
2180
+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) {
2176 2181
     unsigned int content_size;
2177 2182
     struct cli_asn1 c;
2178 2183
     cli_crt_hashtype hashtype;
... ...
@@ -2183,29 +2201,30 @@ int asn1_check_mscat(struct cl_engine *engine, fmap_t *map, size_t offset, unsig
2183 2183
     int ret;
2184 2184
     void *ctx;
2185 2185
 
2186
+    // TODO Move these into cli_checkfp_pe
2186 2187
     if (!(engine->dconf->pe & PE_CONF_CERTS))
2187
-        return CL_VIRUS;
2188
+        return CL_EVERIFY;
2188 2189
     if (engine->engine_options & ENGINE_OPTIONS_DISABLE_PE_CERTS)
2189
-        return CL_VIRUS;
2190
+        return CL_EVERIFY;
2190 2191
 
2191 2192
     cli_dbgmsg("in asn1_check_mscat (offset: %llu)\n", (long long unsigned)offset);
2192 2193
     crtmgr_init(&certs);
2193 2194
     if(crtmgr_add_roots(engine, &certs)) {
2194 2195
         crtmgr_free(&certs);
2195
-        return CL_VIRUS;
2196
+        return CL_EVERIFY;
2196 2197
     }
2197 2198
     ret = asn1_parse_mscat(map, offset, size, &certs, 1, &content, &content_size, engine);
2198 2199
     crtmgr_free(&certs);
2199
-    if(ret)
2200
-        return CL_VIRUS;
2200
+    if(CL_CLEAN != ret)
2201
+        return ret;
2201 2202
 
2202 2203
     if(asn1_expect_objtype(map, content, &content_size, &c, ASN1_TYPE_SEQUENCE)) {
2203 2204
         cli_dbgmsg("asn1_check_mscat: expected SEQUENCE at top level of hash container\n");
2204
-        return CL_VIRUS;
2205
+        return CL_EPARSE;
2205 2206
     }
2206 2207
     if(asn1_expect_obj(map, &c.content, &c.size, ASN1_TYPE_OBJECT_ID, lenof(OID_SPC_PE_IMAGE_DATA_OBJID), OID_SPC_PE_IMAGE_DATA_OBJID)) {
2207 2208
         cli_dbgmsg("asn1_check_mscat: expected spcPEImageData OID in the first hash SEQUENCE\n");
2208
-        return CL_VIRUS;
2209
+        return CL_EPARSE;
2209 2210
     }
2210 2211
 
2211 2212
     // TODO Should we do anything with the underlying SEQUENCE and data?  From
... ...
@@ -2214,28 +2233,31 @@ int asn1_check_mscat(struct cl_engine *engine, fmap_t *map, size_t offset, unsig
2214 2214
 
2215 2215
     if(asn1_expect_objtype(map, c.next, &content_size, &c, ASN1_TYPE_SEQUENCE)) {
2216 2216
         cli_dbgmsg("asn1_check_mscat: expected second hash container object to be a SEQUENCE\n");
2217
-        return CL_VIRUS;
2217
+        return CL_EPARSE;
2218 2218
     }
2219 2219
     if(content_size) {
2220 2220
         cli_dbgmsg("asn1_check_mscat: extra data in hash SEQUENCE\n");
2221
-        return CL_VIRUS;
2221
+        return CL_EPARSE;
2222 2222
     }
2223 2223
 
2224 2224
     if(asn1_expect_hash_algo(map, &c.content, &c.size, &hashtype, &hashsize)) {
2225 2225
         cli_dbgmsg("asn1_check_mscat: unexpected file hash algo\n");
2226
-        return CL_VIRUS;
2226
+        return CL_EPARSE;
2227 2227
     }
2228 2228
 
2229 2229
     if (NULL == (ctx = get_hash_ctx(hashtype))) {
2230
-        return CL_VIRUS;
2230
+        return CL_EPARSE;
2231 2231
     }
2232 2232
 
2233 2233
     // Now that we know the hash algorithm, compute the authenticode hash
2234 2234
     // across the required regions of memory.
2235 2235
     for(unsigned int i = 0; i < nregions; i++) {
2236
-        const uint8_t *hptr; \
2236
+        const uint8_t *hptr;
2237
+        if (0 == regions[i].size) {
2238
+            continue;
2239
+        }
2237 2240
         if(!(hptr = fmap_need_off_once(map, regions[i].offset, regions[i].size))){
2238
-            return CL_VIRUS;
2241
+            return CL_EVERIFY;
2239 2242
         }
2240 2243
 
2241 2244
         cl_update_hash(ctx, hptr, regions[i].size);
... ...
@@ -2252,11 +2274,11 @@ int asn1_check_mscat(struct cl_engine *engine, fmap_t *map, size_t offset, unsig
2252 2252
 
2253 2253
     if(asn1_expect_obj(map, &c.content, &c.size, ASN1_TYPE_OCTET_STRING, hashsize, hash)) {
2254 2254
         cli_dbgmsg("asn1_check_mscat: computed authenticode hash did not match stored value\n");
2255
-        return CL_VIRUS;
2255
+        return CL_EVERIFY;
2256 2256
     }
2257 2257
     if(c.size) {
2258 2258
         cli_dbgmsg("asn1_check_mscat: extra data after the stored authenticode hash\n");
2259
-        return CL_VIRUS;
2259
+        return CL_EPARSE;
2260 2260
     }
2261 2261
 
2262 2262
     cli_dbgmsg("asn1_check_mscat: file with valid authenticode signature, whitelisted\n");
... ...
@@ -31,6 +31,6 @@ struct cli_mapped_region {
31 31
 };
32 32
 
33 33
 int asn1_load_mscat(fmap_t *map, struct cl_engine *engine);
34
-int asn1_check_mscat(struct cl_engine *engine, fmap_t *map, size_t offset, unsigned int size, struct cli_mapped_region *regions, uint32_t nregions);
34
+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);
35 35
 
36 36
 #endif
... ...
@@ -632,19 +632,14 @@ int cli_checkfp_virus(unsigned char *digest, size_t size, cli_ctx *ctx, const ch
632 632
         if (!(ctx->engine->engine_options & ENGINE_OPTIONS_DISABLE_PE_STATS) && !(ctx->engine->dconf->stats & (DCONF_STATS_DISABLED | DCONF_STATS_PE_SECTION_DISABLED)))
633 633
             flags |= CL_CHECKFP_PE_FLAG_STATS;
634 634
 
635
-        switch(cli_checkfp_pe(ctx, shash1, &sections, flags)) {
635
+        switch(cli_checkfp_pe(ctx, &sections, flags)) {
636 636
         case CL_CLEAN:
637
-            cli_dbgmsg("cli_checkfp(pe): PE file whitelisted due to valid embedded digital signature\n");
637
+            cli_dbgmsg("cli_checkfp(pe): PE file whitelisted due to valid digital signature\n");
638 638
             if (sections.sections)
639 639
                 free(sections.sections);
640 640
             return CL_CLEAN;
641
-        case CL_VIRUS:
642
-            if(cli_hm_scan(shash1, 2, &virname, ctx->engine->hm_fp, CLI_HASH_SHA1) == CL_VIRUS) {
643
-                cli_dbgmsg("cli_checkfp(pe): PE file whitelisted by catalog file\n");
644
-                if (sections.sections)
645
-                    free(sections.sections);
646
-                return CL_CLEAN;
647
-            }
641
+        default:
642
+            break;
648 643
         }
649 644
     }
650 645
 
... ...
@@ -5391,10 +5391,23 @@ static int sort_sects(const void *first, const void *second) {
5391 5391
 }
5392 5392
 
5393 5393
 /* Check the given PE file for an authenticode signature and return CL_CLEAN if
5394
- * the signature is valid.  Also, this function computes the hashes of each
5395
- * section (sorted based on the RVAs of the sections) if the
5396
- * CL_CHECKFP_PE_FLAG_STATS flag exists in flags.
5394
+ * the signature is valid.  There are two cases that this function should
5395
+ * handle:
5396
+ * - A PE file has an embedded Authenticode section
5397
+ * - The PE file has no embedded Authenticode section but is covered by a
5398
+ *   catalog file that was loaded in via a -d 
5399
+ * CL_CLEAN will be returned if the file was whitelisted based on its
5400
+ * signature.  CL_VIRUS will be returned if the file was blacklisted based on
5401
+ * its signature.  Otherwise, an cl_error_t error value will be returned.
5402
+ * 
5403
+ * Also, this function computes the hashes of each section (sorted based on the
5404
+ * RVAs of the sections) if the CL_CHECKFP_PE_FLAG_STATS flag exists in flags
5397 5405
  *
5406
+ * TODO The code to compute the section hashes is copied from
5407
+ * cli_genhash_pe - we should use that function instead where this
5408
+ * functionality is needed, since we no longer need to compute the section
5409
+ * hashes as part of the authenticode hash calculation.
5410
+ * 
5398 5411
  * If the section hashes are to be computed and returned, this function
5399 5412
  * allocates memory for the section hashes, and it's up to the caller to free
5400 5413
  * it.  hashes->sections will be initialized to NULL at the beginning of the
... ...
@@ -5408,7 +5421,7 @@ static int sort_sects(const void *first, const void *second) {
5408 5408
  *  - TODO Instead of not providing back any hashes when an invalid section is
5409 5409
  *    encountered, would it be better to still compute hashes for the valid
5410 5410
  *    sections? */
5411
-int cli_checkfp_pe(cli_ctx *ctx, uint8_t *authsha1, stats_section_t *hashes, uint32_t flags) {
5411
+cl_error_t cli_checkfp_pe(cli_ctx *ctx, stats_section_t *hashes, uint32_t flags) {
5412 5412
     uint16_t e_magic; /* DOS signature ("MZ") */
5413 5413
     uint16_t nsections;
5414 5414
     uint32_t e_lfanew; /* address of new exe header */
... ...
@@ -5425,20 +5438,25 @@ int cli_checkfp_pe(cli_ctx *ctx, uint8_t *authsha1, stats_section_t *hashes, uin
5425 5425
     struct cli_exe_section *exe_sections;
5426 5426
     struct pe_image_data_dir *dirs;
5427 5427
     fmap_t *map = *ctx->fmap;
5428
+    void *hashctx=NULL;
5428 5429
     struct pe_certificate_hdr cert_hdr;
5429
-    struct cli_mapped_region *regions;
5430
+    struct cli_mapped_region *regions = NULL;
5430 5431
     unsigned int nregions;
5431
-    int ret;
5432
+    cl_error_t ret = CL_EVERIFY;
5433
+    uint8_t authsha1[SHA1_HASH_SIZE];
5434
+    uint32_t sec_dir_offset;
5435
+    uint32_t sec_dir_size;
5436
+
5437
+    if (flags == CL_CHECKFP_PE_FLAG_NONE)
5438
+        return CL_BREAK;
5432 5439
 
5433 5440
     if (flags & CL_CHECKFP_PE_FLAG_STATS) {
5434 5441
         if (!(hashes))
5435
-            return CL_EFORMAT;
5442
+            return CL_ENULLARG;
5436 5443
         hashes->sections = NULL;
5437 5444
     }
5438 5445
 
5439
-    if (flags == CL_CHECKFP_PE_FLAG_NONE)
5440
-        return CL_VIRUS;
5441
-
5446
+    // TODO What does this do?
5442 5447
     if(!(DCONF & PE_CONF_CATALOG))
5443 5448
         return CL_EFORMAT;
5444 5449
 
... ...
@@ -5507,15 +5525,19 @@ int cli_checkfp_pe(cli_ctx *ctx, uint8_t *authsha1, stats_section_t *hashes, uin
5507 5507
         dirs = optional_hdr64.DataDirectory;
5508 5508
     }
5509 5509
 
5510
+    sec_dir_offset = EC32(dirs[4].VirtualAddress);
5511
+    sec_dir_size = EC32(dirs[4].Size);
5512
+
5510 5513
     // As an optimization, check the security DataDirectory here and if
5511 5514
     // it's less than 8-bytes (and we aren't relying on this code to compute
5512
-    // the section hashes), bail out
5513
-    if (EC32(dirs[4].Size) < 8) {
5515
+    // the section hashes), bail out if we don't have any Authenticode hashes
5516
+    // loaded from .cat files
5517
+    if (sec_dir_size < 8 && !cli_hm_have_size(ctx->engine->hm_fp, CLI_HASH_SHA1, 2)) {
5514 5518
         if (flags & CL_CHECKFP_PE_FLAG_STATS) {
5515 5519
             /* If stats is enabled, continue parsing the sample */
5516 5520
             flags ^= CL_CHECKFP_PE_FLAG_AUTHENTICODE;
5517 5521
         } else {
5518
-            return CL_VIRUS;
5522
+            return CL_BREAK;
5519 5523
         }
5520 5524
     }
5521 5525
     fsize = map->len;
... ...
@@ -5627,42 +5649,21 @@ int cli_checkfp_pe(cli_ctx *ctx, uint8_t *authsha1, stats_section_t *hashes, uin
5627 5627
         }
5628 5628
     }
5629 5629
 
5630
-    /* After this point it's the caller's responsibility to free hashes->sections */
5630
+    /* After this point it's the caller's responsibility to free
5631
+     * hashes->sections. Also, in the case where we are just computing the
5632
+     * stats, we are finished */
5631 5633
     free(exe_sections);
5632 5634
 
5633
-    if (flags & CL_CHECKFP_PE_FLAG_AUTHENTICODE) {
5634
-        /* Check to see if we have a security section. */
5635
-        if(!cli_hm_have_size(ctx->engine->hm_fp, CLI_HASH_SHA1, 2) && EC32(dirs[4].Size) < 8) {
5636
-            if (flags & CL_CHECKFP_PE_FLAG_STATS) {
5637
-                /* If stats is enabled, continue parsing the sample */
5638
-                flags ^= CL_CHECKFP_PE_FLAG_AUTHENTICODE;
5639
-            } else {
5640
-                return CL_BREAK;
5641
-            }
5642
-        }
5643
-
5635
+    while (flags & CL_CHECKFP_PE_FLAG_AUTHENTICODE) {
5644 5636
 
5645
-        // Verify that we have all the bytes we expect in the authenticode sig
5646
-        // and that the certificate table is the last thing in the file
5647
-        // (according to the MS13-098 bulletin, this is a requirement)
5648
-        if (fsize != EC32(dirs[4].Size) + EC32(dirs[4].VirtualAddress)) {
5649
-            cli_dbgmsg("cli_checkfp_pe: expected authenticode data at the end of the file\n");
5650
-            if (flags & CL_CHECKFP_PE_FLAG_STATS) {
5651
-                flags ^= CL_CHECKFP_PE_FLAG_AUTHENTICODE;
5652
-            } else {
5653
-                return CL_EFORMAT;
5654
-            }
5637
+        // We'll build a list of the regions that need to be hashed and pass it to
5638
+        // asn1_check_mscat to do hash verification there (the hash algorithm is
5639
+        // specified in the PKCS7 structure).  We need to hash up to 4 regions
5640
+        regions = (struct cli_mapped_region *) cli_calloc(4, sizeof(struct cli_mapped_region));
5641
+        if(!regions) {
5642
+            return CL_EMEM;
5655 5643
         }
5656
-    }
5657
-
5658
-    // We'll build a list of the regions that need to be hashed and pass it to
5659
-    // asn1_check_mscat to do hash verification there (the hash algorithm is
5660
-    // specified in the PKCS7 structure).  We need to hash up to 4 regions
5661
-    regions = (struct cli_mapped_region *) cli_calloc(4, sizeof(struct cli_mapped_region));
5662
-    if(!regions) {
5663
-        return CL_EMEM;
5664
-    }
5665
-    nregions = 0;
5644
+        nregions = 0;
5666 5645
 
5667 5646
 #define add_chunk_to_hash_list(_offset, _size) \
5668 5647
     do { \
... ...
@@ -5673,7 +5674,9 @@ int cli_checkfp_pe(cli_ctx *ctx, uint8_t *authsha1, stats_section_t *hashes, uin
5673 5673
         } \
5674 5674
     } while(0)
5675 5675
 
5676
-    while (flags & CL_CHECKFP_PE_FLAG_AUTHENTICODE) {
5676
+        // Pretty much every case below should return CL_EFORMAT
5677
+        ret = CL_EFORMAT;
5678
+
5677 5679
         /* MZ to checksum */
5678 5680
         at = 0;
5679 5681
         hlen = e_lfanew + sizeof(struct pe_image_file_hdr) + (pe_plus ? offsetof(struct pe_image_optional_hdr64, CheckSum) : offsetof(struct pe_image_optional_hdr32, CheckSum));
... ...
@@ -5689,76 +5692,138 @@ int cli_checkfp_pe(cli_ctx *ctx, uint8_t *authsha1, stats_section_t *hashes, uin
5689 5689
         at += hlen + 8;
5690 5690
 
5691 5691
         if(at > hdr_size) {
5692
-            if (flags & CL_CHECKFP_PE_FLAG_STATS) {
5693
-                flags ^= CL_CHECKFP_PE_FLAG_AUTHENTICODE;
5694
-                break;
5695
-            } else {
5696
-                free(regions);
5697
-                return CL_EFORMAT;
5698
-            }
5692
+            break;
5699 5693
         }
5700 5694
 
5701 5695
         /* Security to End of header */
5702 5696
         hlen = hdr_size - at;
5703 5697
         add_chunk_to_hash_list(at, hlen);
5704 5698
         at += hlen;
5705
-        break;
5706
-    }
5707 5699
 
5708
-    /* Finally, hash everything from the end of the header to the start of
5709
-     * the security section, which must be the last thing in a file
5710
-     */
5711
-    if (at < EC32(dirs[4].VirtualAddress)) {
5712
-        hlen = EC32(dirs[4].VirtualAddress)-at;
5713
-        add_chunk_to_hash_list(at, hlen);
5714
-    }
5700
+        if (sec_dir_offset) {
5701
+
5702
+            // Verify that we have all the bytes we expect in the authenticode sig
5703
+            // and that the certificate table is the last thing in the file
5704
+            // (according to the MS13-098 bulletin, this is a requirement)
5705
+            if (fsize != sec_dir_size + sec_dir_offset) {
5706
+                cli_dbgmsg("cli_checkfp_pe: expected authenticode data at the end of the file\n");
5707
+                break;
5708
+            }
5715 5709
 
5716
-    if (flags & CL_CHECKFP_PE_FLAG_AUTHENTICODE) {
5710
+            // Hash everything from the end of the header to the start of the
5711
+            // security section
5712
+            if (at < sec_dir_offset) {
5713
+                hlen = sec_dir_offset - at;
5714
+                add_chunk_to_hash_list(at, hlen);
5715
+            } else {
5716
+                cli_dbgmsg("cli_checkfp_pe: security directory offset appears to overlap with the PE header\n");
5717
+                break;
5718
+            }
5717 5719
 
5718
-        hlen = EC32(dirs[4].Size);
5720
+            // Parse the security directory header
5719 5721
 
5720
-        if(fmap_readn(map, &cert_hdr, EC32(dirs[4].VirtualAddress), sizeof(cert_hdr)) != sizeof(cert_hdr)) {
5721
-            free(regions);
5722
-            return CL_EFORMAT;
5722
+            if(fmap_readn(map, &cert_hdr, sec_dir_offset, sizeof(cert_hdr)) != sizeof(cert_hdr)) {
5723
+                break;
5724
+            }
5725
+
5726
+            if (EC16(cert_hdr.revision) != WIN_CERT_REV_2) {
5727
+                cli_dbgmsg("cli_checkfp_pe: unsupported authenticode data revision\n");
5728
+                break;
5729
+            }
5730
+
5731
+            if (EC16(cert_hdr.type) != WIN_CERT_TYPE_PKCS7) {
5732
+                cli_dbgmsg("cli_checkfp_pe: unsupported authenticode data type\n");
5733
+                break;
5734
+            }
5735
+
5736
+            hlen = sec_dir_size;
5737
+
5738
+            if (EC32(cert_hdr.length) != hlen) {
5739
+                /* This is the case that MS13-098 aimed to address, but it got
5740
+                 * pushback to where the fix (not allowing additional, non-zero
5741
+                 * bytes in the security directory) is now opt-in via a registry
5742
+                 * key.  Given that most machines will treat these binaries as
5743
+                 * valid, we'll still parse the signature and just trust that
5744
+                 * our whitelist signatures are tailored enough to where any
5745
+                 * instances of this are reasonable (for instance, I saw one
5746
+                 * binary that appeared to use this to embed a license key.) */
5747
+                cli_dbgmsg("cli_checkfp_pe: MS13-098 violation detected, but continuing on to verify certificate\n");
5748
+            }
5749
+
5750
+            at = sec_dir_offset + sizeof(cert_hdr);
5751
+            hlen -= sizeof(cert_hdr);
5752
+
5753
+            ret = asn1_check_mscat((struct cl_engine *)(ctx->engine), map, at, hlen, regions, nregions);
5754
+
5755
+            if (CL_CLEAN == ret) {
5756
+                // We validated the embedded signature.  Hooray!
5757
+                break;
5758
+            } else if(CL_VIRUS == ret) {
5759
+                // A blacklist rule hit - don't continue on to check hm_fp for a match
5760
+                break;
5761
+            }
5762
+
5763
+            // Otherwise, we still need to check to see whether this file is
5764
+            // covered by a .cat file (it's common these days for driver files
5765
+            // to have .cat files covering PEs with embedded signatures)
5766
+
5767
+        } else {
5768
+
5769
+            // Hash everything from the end of the header to the end of the
5770
+            // file
5771
+            if (at < fsize) {
5772
+                hlen = fsize - at;
5773
+                add_chunk_to_hash_list(at, hlen);
5774
+            }
5723 5775
         }
5724 5776
 
5725
-        if (EC16(cert_hdr.revision) != WIN_CERT_REV_2) {
5726
-            cli_dbgmsg("cli_checkfp_pe: unsupported authenticode data revision\n");
5727
-            free(regions);
5728
-            return CL_VIRUS;
5777
+        // At this point we should compute the SHA1 authenticode hash to see
5778
+        // whether we've had any hashes added from external catalog files
5779
+        // TODO Is it gauranteed that the hashing algorithm will be SHA1?  If
5780
+        // not, figure out how to handle that case
5781
+        hashctx = cl_hash_init("sha1");
5782
+        if (NULL == hashctx) {
5783
+            ret = CL_EMEM;
5784
+            break;
5729 5785
         }
5730 5786
 
5731
-        if (EC16(cert_hdr.type) != WIN_CERT_TYPE_PKCS7) {
5732
-            cli_dbgmsg("cli_checkfp_pe: unsupported authenticode data type\n");
5733
-            free(regions);
5734
-            return CL_VIRUS;
5787
+        for(i = 0; i < nregions; i++) {
5788
+            const uint8_t *hptr;
5789
+            if (0 == regions[i].size) {
5790
+                continue;
5791
+            }
5792
+            if(!(hptr = fmap_need_off_once(map, regions[i].offset, regions[i].size))){
5793
+                break;
5794
+            }
5795
+
5796
+            cl_update_hash(hashctx, hptr, regions[i].size);
5735 5797
         }
5736 5798
 
5737
-        if (EC32(cert_hdr.length) != hlen) {
5738
-            /* This is the case that MS13-098 aimed to address, but it got
5739
-             * pushback to where the fix (not allowing additional, non-zero
5740
-             * bytes in the security directory) is now opt-in via a registry
5741
-             * key.  Given that most machines will treat these binaries as
5742
-             * valid, we'll still parse the signature and just trust that
5743
-             * our whitelist signatures are tailored enough to where any
5744
-             * instances of this are reasonable (for instance, I saw one
5745
-             * binary that appeared to use this to embed a license key.) */
5746
-            cli_dbgmsg("cli_checkfp_pe: MS13-098 violation detected, but continuing on to verify certificate\n");
5799
+        if (i != nregions) {
5800
+            break;
5747 5801
         }
5748 5802
 
5749
-        at = EC32(dirs[4].VirtualAddress) + sizeof(cert_hdr);
5750
-        hlen -= sizeof(cert_hdr);
5803
+        cl_finish_hash(hashctx, authsha1);
5804
+        hashctx = NULL;
5751 5805
 
5752
-        ret = asn1_check_mscat((struct cl_engine *)(ctx->engine), map, at, hlen, regions, nregions);
5806
+        if(cli_hm_scan(authsha1, 2, NULL, ctx->engine->hm_fp, CLI_HASH_SHA1) == CL_VIRUS) {
5807
+            cli_dbgmsg("cli_checkfp_pe: PE file whitelisted by catalog file\n");
5808
+            ret = CL_CLEAN;
5809
+            break;
5810
+        }
5753 5811
 
5754
-        free(regions);
5812
+        ret = CL_EVERIFY;
5813
+        break;
5814
+    } /* while(flags & CL_CHECKFP_PE_FLAG_AUTHENTICODE) */
5755 5815
 
5756
-        return ret;
5816
+    if (NULL != hashctx) {
5817
+        cl_hash_destroy(hashctx);
5818
+    }
5757 5819
 
5758
-    } else {
5820
+    if (NULL != regions) {
5759 5821
         free(regions);
5760
-        return CL_VIRUS;
5761 5822
     }
5823
+    return ret;
5762 5824
 }
5763 5825
 
5764 5826
 int cli_genhash_pe(cli_ctx *ctx, unsigned int class, int type)
... ...
@@ -187,7 +187,7 @@ enum {
187 187
 };
188 188
 
189 189
 int cli_peheader(fmap_t *map, struct cli_exe_info *peinfo);
190
-int cli_checkfp_pe(cli_ctx *ctx, uint8_t *authsha1, stats_section_t *hashes, uint32_t flags);
190
+cl_error_t cli_checkfp_pe(cli_ctx *ctx, stats_section_t *hashes, uint32_t flags);
191 191
 int cli_genhash_pe(cli_ctx *ctx, unsigned int class, int type);
192 192
 
193 193
 uint32_t cli_rawaddr(uint32_t, const struct cli_exe_section *, uint16_t, unsigned int *, size_t, uint32_t);
... ...
@@ -3392,13 +3392,12 @@ static int dumpcerts(const struct optstruct *opts)
3392 3392
 {
3393 3393
     char * filename = NULL;
3394 3394
     STATBUF sb;
3395
-    const char * fmptr;
3396 3395
     struct cl_engine *engine;
3397 3396
     cli_ctx ctx;
3398 3397
     struct cl_scan_options options;
3399
-    int fd, ret;
3400
-    uint8_t shash1[SHA1_HASH_SIZE];
3401
-	
3398
+    int fd;
3399
+    cl_error_t ret;
3400
+
3402 3401
     logg_file = NULL;
3403 3402
 
3404 3403
     filename = optget(opts, "print-certs")->strarg;
... ...
@@ -3474,20 +3473,7 @@ static int dumpcerts(const struct optstruct *opts)
3474 3474
 	return -1;
3475 3475
     }
3476 3476
 
3477
-    fmptr = fmap_need_off_once(*ctx.fmap, 0, sb.st_size);
3478
-    if(!fmptr) {
3479
-        mprintf("!dumpcerts: fmap_need_off_once failed!\n");
3480
-        free(ctx.containers);
3481
-        free(ctx.fmap);
3482
-        close(fd);
3483
-        cl_engine_free(engine);
3484
-	return -1;
3485
-    }
3486
-
3487
-    /* Generate SHA1 */
3488
-    cl_sha1(fmptr, sb.st_size, shash1, NULL);
3489
-
3490
-    ret = cli_checkfp_pe(&ctx, shash1, NULL, CL_CHECKFP_PE_FLAG_AUTHENTICODE);
3477
+    ret = cli_checkfp_pe(&ctx, NULL, CL_CHECKFP_PE_FLAG_AUTHENTICODE);
3491 3478
     
3492 3479
     switch(ret) {
3493 3480
         case CL_CLEAN:
... ...
@@ -3500,7 +3486,7 @@ static int dumpcerts(const struct optstruct *opts)
3500 3500
             mprintf("*dumpcerts: CL_BREAK after cli_checkfp_pe()!\n");
3501 3501
             break;
3502 3502
         case CL_EFORMAT:
3503
-            mprintf("!dumpcerts: Not a valid PE file!\n");
3503
+            mprintf("!dumpcerts: An error occurred when parsing the file\n");
3504 3504
             break;
3505 3505
         default:
3506 3506
             mprintf("!dumpcerts: Other error %d inside cli_checkfp_pe.\n", ret);