From a0980389a7fecd5f00ec3f21235a92ce2b49438b Mon Sep 17 00:00:00 2001
From: Andrew <36489577+recvfrom@users.noreply.github.com>
Date: Fri, 19 Jul 2019 12:24:23 -0400
Subject: [PATCH] Fix uninitialized memory usage in PE cert parsing

Fixes:

==123806== Conditional jump or move depends on uninitialised value(s)
==123806==    at 0x50C4A65: crtmgr_whitelist_lookup (crtmgr.c:107)
==123806==    by 0x50C4F36: crtmgr_lookup (crtmgr.c:161)
==123806==    by 0x50CC003: asn1_get_x509 (asn1.c:1053)
...
==123806==  Uninitialised value was created by a stack allocation
==123806==    at 0x50CA335: asn1_get_x509 (asn1.c:723)

hashtype and issuer were not getting set prior to the check
for duplicates when processing embedded certs, which means
some certs that were actually duplicates could have been added
multiple times to the list of trusted certs based on the
contents of the unitialized memory backing those (harmless,
but not as efficient).
---
 libclamav/asn1.c   | 12 ++++++------
 libclamav/crtmgr.c | 11 +++++++++--
 2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/libclamav/asn1.c b/libclamav/asn1.c
index 73c04bd98..58f4bbc57 100644
--- a/libclamav/asn1.c
+++ b/libclamav/asn1.c
@@ -1050,12 +1050,6 @@ static int asn1_get_x509(fmap_t *map, const void **asn1data, unsigned int *size,
             cli_dbgmsg("asn1_get_x509: encountered a certificate with no cert, code, or time signing capabilities\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;
-        }
-
         if (map_raw(map, issuer, issuersize, x509.raw_issuer))
             break;
         if (map_sha1(map, issuer, issuersize, x509.issuer))
@@ -1070,6 +1064,12 @@ static int asn1_get_x509(fmap_t *map, const void **asn1data, unsigned int *size,
         }
         x509.hashtype = hashtype1;
 
+        if (crtmgr_lookup(crts, &x509)) {
+            cli_dbgmsg("asn1_get_x509: duplicate embedded certificates detected\n");
+            cli_crt_clear(&x509);
+            return ASN1_GET_X509_SUCCESS;
+        }
+
         if (asn1_expect_objtype(map, tbs.next, &crt.size, &obj, ASN1_TYPE_BIT_STRING)) { /* signature */
             cli_dbgmsg("asn1_get_x509: Failed to parse x509 signature BIT STRING\n");
             break;
diff --git a/libclamav/crtmgr.c b/libclamav/crtmgr.c
index a7a42e680..a933b4406 100644
--- a/libclamav/crtmgr.c
+++ b/libclamav/crtmgr.c
@@ -77,10 +77,17 @@ void cli_crt_clear(cli_crt *x509)
  * function:
  * 
  *  - To see whether x509 already exists in m (when adding new CRB sig certs
- *    and when adding certs that are embedded in Authenticode signatures)
+ *    and when adding certs that are embedded in Authenticode signatures) to
+ *    prevent duplicate entries. In this case, we want to take x509's
+ *    hashtype and issuer field into account, so a CRB sig cert entry isn't
+ *    returned for an embedded cert duplicate check, and so that two embedded
+ *    certs with different hash types or issuers aren't treated as being the
+ *    same.
  * 
  *  - To see whether a CRB sig matches against x509, deeming it worthy to be
- *    added to the trust store
+ *    added to the trust store.  In this case, we don't want to compare
+ *    hashtype and issuer, since the embedded sig will have the actual values
+ *    and the CRB sig cert will have placeholder values.
  * 
  * Use crb_crts_only to distinguish between the two cases.  If True, it will
  * ignore all crts not added from CRB rules and ignore x509's issuer and