Handle two more Authenticode parsing edge cases
Certs can omit the boolean field in the Basic Constraints section,
since the RFC specifies a default value for this field. This fixes
the following error:
LibClamAV debug: asn1_expect_objtype: expected type 01, got 02
LibClamAV debug: asn1_get_x509: An error occurred when parsing x509 extensions
LibClamAV debug: asn1_parse_mscat: skipping x509 certificate with errors
Ex: 05de45fd6a406dc147a4c8040a54eee947cd6eba02f28c0279ffd1a229e17130
Allow UTCDate fields in x509 certs to omit the seconds. Technically
this is disallowed by RFC5280, but Windows Authenticode verification
routines don't seem to mind it, so we'll allow it too. This fixes
the following error:
LibClamAV debug: asn1_getnum: expecting digits, found 'Z'
LibClamAV debug: asn1_get_time: invalid second 4294967295
LibClamAV debug: asn1_get_x509: unable to extract the notBefore time
LibClamAV debug: asn1_parse_mscat: skipping x509 certificate with errors
Ex: d577010638f208ad8b6dab1a33dc583b2ec6b0c719021fb9f083dd595ede27e8
Also, add a check on CRT_RAWMAXLEN, since if it's > 256 problems
will arise
Andrew authored on 2019/11/13 12:30:31Showing 2 changed files
| ... | ... |
@@ -611,17 +611,33 @@ static int asn1_get_time(fmap_t *map, const void **asn1data, unsigned int *size, |
| 611 | 611 |
t.tm_min = n; |
| 612 | 612 |
ptr += 2; |
| 613 | 613 |
|
| 614 |
- n = asn1_getnum(ptr); |
|
| 615 |
- if (n < 0 || n > 59) {
|
|
| 616 |
- cli_dbgmsg("asn1_get_time: invalid second %u\n", n);
|
|
| 617 |
- return 1; |
|
| 618 |
- } |
|
| 619 |
- t.tm_sec = n; |
|
| 620 |
- ptr += 2; |
|
| 614 |
+ if (*ptr == 'Z') {
|
|
| 615 |
+ /* NOTE: RFC5280 requires that the UTCDate fields in X509 certs |
|
| 616 |
+ * include the seconds (it's optional in the UTCDate definition), |
|
| 617 |
+ * but one CA time-stamping cert used by ~1,700 samples on VirusTotal |
|
| 618 |
+ * omits the seconds. These samples still validate successfully, |
|
| 619 |
+ * though, so allow it here. |
|
| 620 |
+ * |
|
| 621 |
+ * In this case we will have fmap'd in two extra bytes unrelated to |
|
| 622 |
+ * the UTCDate (and failed if there weren't two bytes afterward), |
|
| 623 |
+ * but that shouldn't have an affect in practice since there will |
|
| 624 |
+ * always be more signature data following the UTCDate data that we |
|
| 625 |
+ * parse. */ |
|
| 626 |
+ t.tm_sec = 0; |
|
| 621 | 627 |
|
| 622 |
- if (*ptr != 'Z') {
|
|
| 623 |
- cli_dbgmsg("asn1_get_time: expected UTC time 'Z', got '%c'\n", *ptr);
|
|
| 624 |
- return 1; |
|
| 628 |
+ } else {
|
|
| 629 |
+ n = asn1_getnum(ptr); |
|
| 630 |
+ if (n < 0 || n > 59) {
|
|
| 631 |
+ cli_dbgmsg("asn1_get_time: invalid second %u\n", n);
|
|
| 632 |
+ return 1; |
|
| 633 |
+ } |
|
| 634 |
+ t.tm_sec = n; |
|
| 635 |
+ ptr += 2; |
|
| 636 |
+ |
|
| 637 |
+ if (*ptr != 'Z') {
|
|
| 638 |
+ cli_dbgmsg("asn1_get_time: expected UTC time 'Z', got '%c'\n", *ptr);
|
|
| 639 |
+ return 1; |
|
| 640 |
+ } |
|
| 625 | 641 |
} |
| 626 | 642 |
|
| 627 | 643 |
*tm = mktime(&t); |
| ... | ... |
@@ -986,7 +1002,11 @@ static int asn1_get_x509(fmap_t *map, const void **asn1data, unsigned int *size, |
| 986 | 986 |
continue; |
| 987 | 987 |
} |
| 988 | 988 |
if (!memcmp("\x55\x1d\x13", id.content, 3)) {
|
| 989 |
- /* Basic Constraints 2.5.29.19 */ |
|
| 989 |
+ /* Basic Constraints 2.5.29.19 |
|
| 990 |
+ * |
|
| 991 |
+ * BasicConstraints ::= SEQUENCE {
|
|
| 992 |
+ * cA BOOLEAN DEFAULT FALSE, |
|
| 993 |
+ * pathLenConstraint INTEGER (0..MAX) OPTIONAL } */ |
|
| 990 | 994 |
struct cli_asn1 constr; |
| 991 | 995 |
if (asn1_expect_objtype(map, value.content, &value.size, &constr, ASN1_TYPE_SEQUENCE)) {
|
| 992 | 996 |
exts.size = 1; |
| ... | ... |
@@ -995,20 +1015,33 @@ static int asn1_get_x509(fmap_t *map, const void **asn1data, unsigned int *size, |
| 995 | 995 |
if (!constr.size) |
| 996 | 996 |
x509.certSign = 0; |
| 997 | 997 |
else {
|
| 998 |
- if (asn1_expect_objtype(map, constr.content, &constr.size, &ext, ASN1_TYPE_BOOLEAN)) {
|
|
| 999 |
- exts.size = 1; |
|
| 1000 |
- break; |
|
| 1001 |
- } |
|
| 1002 |
- if (ext.size != 1) {
|
|
| 1003 |
- cli_dbgmsg("asn1_get_x509: wrong bool size in basic constraint %u\n", ext.size);
|
|
| 998 |
+ if (asn1_get_obj(map, constr.content, &constr.size, &ext)) {
|
|
| 1004 | 999 |
exts.size = 1; |
| 1005 | 1000 |
break; |
| 1006 | 1001 |
} |
| 1007 |
- if (!fmap_need_ptr_once(map, ext.content, 1)) {
|
|
| 1002 |
+ if (ext.type == ASN1_TYPE_BOOLEAN) {
|
|
| 1003 |
+ |
|
| 1004 |
+ if (ext.size != 1) {
|
|
| 1005 |
+ cli_dbgmsg("asn1_get_x509: wrong bool size in basic constraint %u\n", ext.size);
|
|
| 1006 |
+ exts.size = 1; |
|
| 1007 |
+ break; |
|
| 1008 |
+ } |
|
| 1009 |
+ |
|
| 1010 |
+ if (!fmap_need_ptr_once(map, ext.content, 1)) {
|
|
| 1011 |
+ exts.size = 1; |
|
| 1012 |
+ break; |
|
| 1013 |
+ } |
|
| 1014 |
+ x509.certSign = (((uint8_t *)(ext.content))[0] != 0); |
|
| 1015 |
+ |
|
| 1016 |
+ } else if (ext.type == ASN1_TYPE_INTEGER) {
|
|
| 1017 |
+ /* In this case, assume cA is missing and |
|
| 1018 |
+ * pathLenConstraint is present. Default cA |
|
| 1019 |
+ * to False. */ |
|
| 1020 |
+ x509.certSign = 0; |
|
| 1021 |
+ } else {
|
|
| 1008 | 1022 |
exts.size = 1; |
| 1009 | 1023 |
break; |
| 1010 | 1024 |
} |
| 1011 |
- x509.certSign = (((uint8_t *)(ext.content))[0] != 0); |
|
| 1012 | 1025 |
} |
| 1013 | 1026 |
} |
| 1014 | 1027 |
} |
| ... | ... |
@@ -41,6 +41,13 @@ typedef enum { VRFY_CODE,
|
| 41 | 41 |
#define CRT_RAWMAXLEN 64 |
| 42 | 42 |
#endif |
| 43 | 43 |
|
| 44 |
+/* If CRT_RAWMAXLEN is > 256 it will break the way raw data is stored. If |
|
| 45 |
+ larger values are needed, we will need to update the code (ex: look at |
|
| 46 |
+ map_raw) */ |
|
| 47 |
+#if CRT_RAWMAXLEN > 256 |
|
| 48 |
+#error CRT_RAWMAXLEN cannot be greater than 256 |
|
| 49 |
+#endif |
|
| 50 |
+ |
|
| 44 | 51 |
typedef struct cli_crt_t {
|
| 45 | 52 |
char *name; |
| 46 | 53 |
uint8_t raw_subject[CRT_RAWMAXLEN]; |