@@ -695,12 +695,12 @@ static int asn1_get_rsa_pubkey(fmap_t *map, const void **asn1data, unsigned int

 #define ASN1_GET_X509_UNRECOVERABLE_ERROR 2

 /* Parse the asn1data associated with an x509 certificate and add the cert

- * to the crtmgr other if it doesn't already exist in master or other.

+ * to the crtmgr certs if it doesn't already exist there.

  * ASN1_GET_X509_CERT_ERROR will be returned in the case that an invalid x509

  * certificate is encountered but asn1data and size are suitable for continued

  * signature parsing.  ASN1_GET_X509_UNRECOVERABLE_ERROR will be returned in

  * the case where asn1data and size are not suitable for continued use. */

-static int asn1_get_x509(fmap_t *map, const void **asn1data, unsigned int *size, crtmgr *master, crtmgr *other) {

+static int asn1_get_x509(fmap_t *map, const void **asn1data, unsigned int *size, crtmgr *crts) {

     struct cli_asn1 crt, tbs, obj;

     unsigned int avail, tbssize, issuersize;

     cli_crt_hashtype hashtype1, hashtype2;

@@ -1029,8 +1029,8 @@ static int asn1_get_x509(fmap_t *map, const void **asn1data, unsigned int *size,

         }

-        if(crtmgr_lookup(master, &x509) || crtmgr_lookup(other, &x509)) {

-            cli_dbgmsg("asn1_get_x509: certificate already exists\n");

+        if(crtmgr_lookup(crts, &x509)) {

+            cli_dbgmsg("asn1_get_x509: duplicate embedded certificates detected\n");

             cli_crt_clear(&x509);

             return ASN1_GET_X509_SUCCESS;

         }

@@ -1076,7 +1076,7 @@ static int asn1_get_x509(fmap_t *map, const void **asn1data, unsigned int *size,

         }

-        if(crtmgr_add(other, &x509))

+        if(crtmgr_add(crts, &x509))

             break;

         cli_crt_clear(&x509);

         return ASN1_GET_X509_SUCCESS;

@@ -1510,7 +1510,7 @@ static int asn1_parse_mscat(fmap_t *map, size_t offset, unsigned int size, crtmg

             crtmgr newcerts;

             crtmgr_init(&newcerts);

             while(dsize) {

-                result = asn1_get_x509(map, &asn1.content, &dsize, cmgr, &newcerts);

+                result = asn1_get_x509(map, &asn1.content, &dsize, &newcerts);

                 if(ASN1_GET_X509_UNRECOVERABLE_ERROR == result) {

                     dsize = 1;

                     break;

@@ -1526,12 +1526,10 @@ static int asn1_parse_mscat(fmap_t *map, size_t offset, unsigned int size, crtmg

             }

             if(newcerts.crts) {

                 x509 = newcerts.crts;

-                cli_dbgmsg("asn1_parse_mscat: %u new certificates collected\n", newcerts.items);

-                while(x509) {

-                    cli_crt *parent = crtmgr_verify_crt(cmgr, x509);

-

-                    /* Dump the cert if requested before anything happens to it */

-                    if (engine->engine_options & ENGINE_OPTIONS_PE_DUMPCERTS) {

+                cli_dbgmsg("asn1_parse_mscat: %u embedded certificates collected\n", newcerts.items);

+                if (engine->engine_options & ENGINE_OPTIONS_PE_DUMPCERTS) {

+                    /* Dump the certs if requested before anything happens to them */

+                    while(x509) {

                         char raw_issuer[CRT_RAWMAXLEN*2+1], raw_subject[CRT_RAWMAXLEN*2+1], raw_serial[CRT_RAWMAXLEN*3+1];

                         char issuer[SHA1_HASH_SIZE*2+1], subject[SHA1_HASH_SIZE*2+1], serial[SHA1_HASH_SIZE*2+1];

                         char mod[1024+1], exp[1024+1];

@@ -1563,8 +1561,29 @@ static int asn1_parse_mscat(fmap_t *map, size_t offset, unsigned int size, crtmg

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

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

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

+

+                        x509 = x509->next;

+                    }

+                    x509 = newcerts.crts;

+                }

+

+                while(x509) {

+                    cli_crt *parent;

+

+                    /* If the certificate is in the trust store already, remove

+                     * it from the newcerts list */

+                    if (crtmgr_lookup(cmgr, x509)) {

+                        cli_crt *tmp = x509->next;

+                        cli_dbgmsg("asn1_parse_mscat: found embedded certificate matching one in the trust store\n");

+                        crtmgr_del(&newcerts, x509);

+                        x509 = tmp;

+                        continue;

                     }

+                    /* Determine whether the cert is signed by one in our trust

+                     * store or has a blacklist entry */

+                    parent = crtmgr_verify_crt(cmgr, x509);

+

                     if(parent) {

                         if (parent->isBlacklisted) {

                             // NOTE: In this case, parent is a blacklist entry

@@ -1572,10 +1591,7 @@ static int asn1_parse_mscat(fmap_t *map, size_t offset, unsigned int size, crtmg

                             // entry for this certificate's parent

                             isBlacklisted = 1;

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

-

-                            // TODO In this case cmgr already has a blacklisted

-                            // cert for this x509, so I don't think we need to

-                            // continue on below and try to add it to cmgr

+                            break;

                         }

                         // TODO Why is this done?

@@ -1587,9 +1603,16 @@ static int asn1_parse_mscat(fmap_t *map, size_t offset, unsigned int size, crtmg

                             break;

                         }

                         crtmgr_del(&newcerts, x509);

+

+                        /* Start at the beginning of newcerts so that we can see

+                         * whether adding this new trusted cert causes more

+                         * certs to be trusted (via chaining).  Otherwise we

+                         * might miss valid certs if the ordering in the binary

+                         * doesn't align with the chain ordering. */

                         x509 = newcerts.crts;

                         continue;

                     }

+

                     x509 = x509->next;

                 }

                 if(x509) {

@@ -1895,7 +1918,6 @@ static int asn1_parse_mscat(fmap_t *map, size_t offset, unsigned int size, crtmg

         deep.next = asn1.content;

         result = 0;

         while(dsize) {

-            struct cli_asn1 cobj;

             int content;

             if(asn1_expect_objtype(map, deep.next, &dsize, &deep, ASN1_TYPE_SEQUENCE)) {

                 cli_dbgmsg("asn1_parse_mscat: expected SEQUENCE starting an unauthenticatedAttribute\n");

@@ -2210,7 +2232,7 @@ int asn1_check_mscat(struct cl_engine *engine, fmap_t *map, size_t offset, unsig

     // Now that we know the hash algorithm, compute the authenticode hash

     // across the required regions of memory.

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

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

         const uint8_t *hptr; \

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

             return CL_VIRUS;

@@ -2223,7 +2245,7 @@ int asn1_check_mscat(struct cl_engine *engine, fmap_t *map, size_t offset, unsig

     if(cli_debug_flag) {

         char hashtxt[MAX_HASH_SIZE*2+1];

-        for(int i=0; i<hashsize; i++)

+        for(unsigned int i=0; i<hashsize; i++)

             sprintf(&hashtxt[i*2], "%02x", hash[i]);

         cli_dbgmsg("Authenticode: %s\n", hashtxt);

     }

@@ -62,66 +62,85 @@ void cli_crt_clear(cli_crt *x509) {

     mp_clear_multi(&x509->n, &x509->e, &x509->sig, NULL);

 }

-cli_crt *crtmgr_lookup(crtmgr *m, cli_crt *x509) {

+/* Look for an existing certificate in the trust store m.  This search allows

+ * the not_before / not_after / certSign / codeSign / timeSign fields to be

+ * more restrictive than the values associated with a cert in the trust store,

+ * but not less.  It's probably overkill to not do exact matching on those

+ * fields... TODO Is there a case where this is needed

+ *

+ * There are two ways that things get added to the whitelist - through the CRB

+ * rules, and through embedded signatures / catalog files that we parse.  CRB

+ * rules only specify the subject, serial, public key, and whether the cert

+ * can be used for cert/code/time signing, so in those cases the issuer and

+ * hashtype get set to a hardcoded value.  Those values are important for

+ * doing signature verification, though, so we include them when doing this

+ * lookup.  That way, certs with more specific values can get added to the

+ * whitelist by functions like crtmgr_add and increase the probability of

+ * successful signature verification. */

+cli_crt *crtmgr_whitelist_lookup(crtmgr *m, cli_crt *x509) {

     cli_crt *i;

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

-        if(x509->not_before >= i->not_before &&

+        if(!i->isBlacklisted &&

+           x509->not_before >= i->not_before &&

            x509->not_after <= i->not_after &&

            (i->certSign | x509->certSign) == i->certSign &&

            (i->codeSign | x509->codeSign) == i->codeSign &&

            (i->timeSign | x509->timeSign) == i->timeSign &&

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

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

+           !memcmp(x509->issuer, i->issuer, sizeof(i->issuer)) &&

+           x509->hashtype == i->hashtype &&

            !mp_cmp(&x509->n, &i->n) &&

-           !mp_cmp(&x509->e, &i->e) && !(i->isBlacklisted)) {

+           !mp_cmp(&x509->e, &i->e)) {

             return i;

         }

     }

     return NULL;

 }

-int crtmgr_add(crtmgr *m, cli_crt *x509) {

+cli_crt *crtmgr_blacklist_lookup(crtmgr *m, cli_crt *x509) {

     cli_crt *i;

-    int ret = 0;

-

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

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

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

-           !mp_cmp(&x509->n, &i->n) &&

-           !mp_cmp(&x509->e, &i->e)) {

-            if(x509->not_before >= i->not_before && x509->not_after <= i->not_after) {

-                /* Already same or broader */

-                ret = 1;

-            }

-            if(i->not_before > x509->not_before && i->not_before <= x509->not_after) {

-                /* Extend left */

-                i->not_before = x509->not_before;

-                ret = 1;

-            }

-            if(i->not_after >= x509->not_before && i->not_after < x509->not_after) {

-                /* Extend right */

-                i->not_after = x509->not_after;

-                ret = 1;

-            }

-            if(!ret)

-                continue;

-            i->certSign |= x509->certSign;

-            i->codeSign |= x509->codeSign;

-            i->timeSign |= x509->timeSign;

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

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

+        // so do blacklist queries based on those fields

-            return 0;

-        }

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

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

-        /* If certs match, we're likely just revoking it */

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

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

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

+        if (i->isBlacklisted &&

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

+            !memcmp(i->serial, x509->serial, sizeof(i->serial)) &&

             !mp_cmp(&x509->n, &i->n) &&

             !mp_cmp(&x509->e, &i->e)) {

-                if (i->isBlacklisted != x509->isBlacklisted)

-                    i->isBlacklisted = x509->isBlacklisted;

+            return i;

+        }

+    }

+    return NULL;

+}

-                return 0;

+/* Determine whether x509 already exists in m. The fields compared depend on

+ * whether x509 is a blacklist entry or a trusted certificate */

+cli_crt *crtmgr_lookup(crtmgr *m, cli_crt *x509) {

+    if (x509->isBlacklisted) {

+        return crtmgr_blacklist_lookup(m, x509);

+    } else {

+        return crtmgr_whitelist_lookup(m, x509);

+    }

+}

+

+int crtmgr_add(crtmgr *m, cli_crt *x509) {

+    cli_crt *i;

+    int ret = 0;

+

+    if (x509->isBlacklisted) {

+        if (crtmgr_blacklist_lookup(m, x509)) {

+            cli_dbgmsg("crtmgr_add: duplicate blacklist entry detected - not adding\n");

+            return 0;

+        }

+    } else {

+        if (crtmgr_whitelist_lookup(m, x509)) {

+            cli_dbgmsg("crtmgr_add: duplicate trusted certificate detected - not adding\n");

+            return 0;

         }

     }

@@ -239,7 +258,7 @@ static int crtmgr_rsa_verify(cli_crt *x509, mp_int *sig, cli_crt_hashtype hashty

             cli_dbgmsg("crtmgr_rsa_verify: keylen-1 doesn't match expected size of exptmod result\n");

             break;

         }

-        if(mp_unsigned_bin_size(&x) > sizeof(d)) {

+        if(((unsigned int) mp_unsigned_bin_size(&x)) > sizeof(d)) {

             cli_dbgmsg("crtmgr_rsa_verify: exptmod result would overrun working buffer\n");

             break;

         }

@@ -376,29 +395,28 @@ static int crtmgr_rsa_verify(cli_crt *x509, mp_int *sig, cli_crt_hashtype hashty

 cli_crt *crtmgr_verify_crt(crtmgr *m, cli_crt *x509) {

     cli_crt *i = m->crts, *best = NULL;

     int score = 0;

+    unsigned int possible = 0;

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

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

-            !memcmp(i->serial, x509->serial, sizeof(i->serial))) {

-

-            // TODO Shouldn't we compare public keys in this case as well?  I'd

-            // think that it's the public key that really makes a certificate

-            // unique (not subject/serial).  Otherwise, you could have malware

-            // use the subject/serial from a popular company's cert and if we

-            // blacklisted that it would cause FPs on the popular software.

-

-            if (i->isBlacklisted)

-                return i;

-        }

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

+        return i;

     }

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

+

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

         if(i->certSign &&

+           !i->isBlacklisted &&

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

            !crtmgr_rsa_verify(i, &x509->sig, x509->hashtype, x509->tbshash)) {

             int curscore;

             if((x509->codeSign & i->codeSign) == x509->codeSign && (x509->timeSign & i->timeSign) == x509->timeSign)

                 return i;

+            possible++;

             curscore = (x509->codeSign & i->codeSign) + (x509->timeSign & i->timeSign);

             if(curscore > score) {

                 best = i;

@@ -406,6 +424,12 @@ cli_crt *crtmgr_verify_crt(crtmgr *m, cli_crt *x509) {

             }

         }

     }

+

+    if (possible > 1) {

+        // If this is ever triggered, it's probably an indication of an error

+        // in the CRB being used.

+        cli_warnmsg("crtmgr_verify_crt: choosing between codeSign cert and timeSign cert without enough info - errors may result\n");

+    }

     return best;

 }