1. clamav-devel
  2. Commit a0980389a7fecd5f00ec3f21235a92ce2b49438b
Browse code

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

Andrew authored on 2019/07/20 01:24:23
Showing 2 changed files
libclamav/asn1.c
History View file @ a098038
... ... 
@@ -1050,12 +1050,6 @@ static int asn1_get_x509(fmap_t *map, const void **asn1data, unsigned int *size,
1050 1050 
             cli_dbgmsg("asn1_get_x509: encountered a certificate with no cert, code, or time signing capabilities\n");
1051 1051 
         }
1052 1052
1053 
-        if (crtmgr_lookup(crts, &x509)) {
1054 
-            cli_dbgmsg("asn1_get_x509: duplicate embedded certificates detected\n");
1055 
-            cli_crt_clear(&x509);
1056 
-            return ASN1_GET_X509_SUCCESS;
1057 
-        }
1058 
-
1059 1053 
         if (map_raw(map, issuer, issuersize, x509.raw_issuer))
1060 1054 
             break;
1061 1055 
         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,
1070 1070 
         }
1071 1071 
         x509.hashtype = hashtype1;
1072 1072
1073 
+        if (crtmgr_lookup(crts, &x509)) {
1074 
+            cli_dbgmsg("asn1_get_x509: duplicate embedded certificates detected\n");
1075 
+            cli_crt_clear(&x509);
1076 
+            return ASN1_GET_X509_SUCCESS;
1077 
+        }
1078 
+
1073 1079 
         if (asn1_expect_objtype(map, tbs.next, &crt.size, &obj, ASN1_TYPE_BIT_STRING)) { /* signature */
1074 1080 
             cli_dbgmsg("asn1_get_x509: Failed to parse x509 signature BIT STRING\n");
1075 1081 
             break;
libclamav/crtmgr.c
History View file @ a098038
... ... 
@@ -77,10 +77,17 @@ void cli_crt_clear(cli_crt *x509)
77 77 
  * function:
78 78 
  *
79 79 
  *  - To see whether x509 already exists in m (when adding new CRB sig certs
80 
- *    and when adding certs that are embedded in Authenticode signatures)
80 
+ *    and when adding certs that are embedded in Authenticode signatures) to
81 
+ *    prevent duplicate entries. In this case, we want to take x509's
82 
+ *    hashtype and issuer field into account, so a CRB sig cert entry isn't
83 
+ *    returned for an embedded cert duplicate check, and so that two embedded
84 
+ *    certs with different hash types or issuers aren't treated as being the
85 
+ *    same.
81 86 
  *
82 87 
  *  - To see whether a CRB sig matches against x509, deeming it worthy to be
83 
- *    added to the trust store
88 
+ *    added to the trust store.  In this case, we don't want to compare
89 
+ *    hashtype and issuer, since the embedded sig will have the actual values
90 
+ *    and the CRB sig cert will have placeholder values.
84 91 
  *
85 92 
  * Use crb_crts_only to distinguish between the two cases.  If True, it will
86 93 
  * ignore all crts not added from CRB rules and ignore x509's issuer and