From b2281e1cf02455b1675ab66f6345bb0e6b726466 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 16 Jul 2020 18:18:22 +0200 Subject: [PATCH 1/3] x509parse_crl: more negative test cases Add a few more negative test cases for mbedtls_x509_crl_parse. The test data is manually adapted from the existing positive test case "X509 CRL ASN1 (TBSCertList, sig present)" which decomposes as 305c 3047 tbsCertList TBSCertList 020100 version INTEGER OPTIONAL 300d signatureAlgorithm AlgorithmIdentifier 06092a864886f70d01010e 0500 300f issuer Name 310d300b0603550403130441424344 170c303930313031303030303030 thisUpdate Time 3014 revokedCertificates 3012 entry 1 8202abcd userCertificate CertificateSerialNumber 170c303831323331323335393539 revocationDate Time 300d signatureAlgorithm AlgorithmIdentifier 06092a864886f70d01010e 0500 03020001 signatureValue BIT STRING Signed-off-by: Gilles Peskine --- tests/suites/test_suite_x509parse.data | 32 ++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/tests/suites/test_suite_x509parse.data b/tests/suites/test_suite_x509parse.data index d5f538b22..cb530c729 100644 --- a/tests/suites/test_suite_x509parse.data +++ b/tests/suites/test_suite_x509parse.data @@ -2104,6 +2104,38 @@ X509 CRL ASN1 (TBSCertList, sig present, len mismatch) depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C x509parse_crl:"305d3047020100300d06092a864886f70d01010e0500300f310d300b0603550403130441424344170c303930313031303030303030301430128202abcd170c303831323331323335393539300d06092a864886f70d01010e05000302000100":"":MBEDTLS_ERR_X509_INVALID_FORMAT + MBEDTLS_ERR_ASN1_LENGTH_MISMATCH +X509 CRL ASN1 (TBSCertList, signatureValue missing) +depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C +x509parse_crl:"30583047020100300d06092a864886f70d01010e0500300f310d300b0603550403130441424344170c303930313031303030303030301430128202abcd170c303831323331323335393539300d06092a864886f70d01010e0500":"":MBEDTLS_ERR_X509_INVALID_SIGNATURE + MBEDTLS_ERR_ASN1_OUT_OF_DATA + +X509 CRL ASN1 (TBSCertList, signatureAlgorithm missing) +depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C +x509parse_crl:"30493047020100300d06092a864886f70d01010e0500300f310d300b0603550403130441424344170c303930313031303030303030301430128202abcd170c303831323331323335393539":"":MBEDTLS_ERR_X509_INVALID_ALG + MBEDTLS_ERR_ASN1_OUT_OF_DATA + +X509 CRL ASN1 (TBSCertList, single empty entry at end) +depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C +x509parse_crl:"30373035020100300d06092a864886f70d01010e0500300f310d300b0603550403130441424344170c30393031303130303030303030023000":"":MBEDTLS_ERR_X509_INVALID_SERIAL + MBEDTLS_ERR_ASN1_OUT_OF_DATA + +X509 CRL ASN1 (TBSCertList, good entry then empty entry at end) +depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C +x509parse_crl:"304b3049020100300d06092a864886f70d01010e0500300f310d300b0603550403130441424344170c303930313031303030303030301630128202abcd170c3038313233313233353935393000":"":MBEDTLS_ERR_X509_INVALID_SERIAL + MBEDTLS_ERR_ASN1_OUT_OF_DATA + +X509 CRL ASN1 (TBSCertList, missing time in entry) +depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C +x509parse_crl:"304e3039020100300d06092a864886f70d01010e0500300f310d300b0603550403130441424344170c303930313031303030303030300630048202abcd300d06092a864886f70d01010e050003020001":"":MBEDTLS_ERR_X509_INVALID_DATE + MBEDTLS_ERR_ASN1_OUT_OF_DATA + +X509 CRL ASN1 (TBSCertList, missing time in entry at end) +depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C +x509parse_crl:"303b3039020100300d06092a864886f70d01010e0500300f310d300b0603550403130441424344170c303930313031303030303030300630048202abcd":"":MBEDTLS_ERR_X509_INVALID_DATE + MBEDTLS_ERR_ASN1_OUT_OF_DATA + +X509 CRL ASN1 (TBSCertList, invalid tag for time in entry) +depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C +x509parse_crl:"305c3047020100300d06092a864886f70d01010e0500300f310d300b0603550403130441424344170c303930313031303030303030301430128202abcd190c303831323331323335393539300d06092a864886f70d01010e050003020001":"":MBEDTLS_ERR_X509_INVALID_DATE + MBEDTLS_ERR_ASN1_UNEXPECTED_TAG + +X509 CRL ASN1 (TBSCertList, invalid tag for serial) +depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C +x509parse_crl:"305c3047020100300d06092a864886f70d01010e0500300f310d300b0603550403130441424344170c303930313031303030303030301430128402abcd170c303831323331323335393539300d06092a864886f70d01010e050003020001":"":MBEDTLS_ERR_X509_INVALID_SERIAL + MBEDTLS_ERR_ASN1_UNEXPECTED_TAG + X509 CRL ASN1 (TBSCertList, sig present) depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C x509parse_crl:"305c3047020100300d06092a864886f70d01010e0500300f310d300b0603550403130441424344170c303930313031303030303030301430128202abcd170c303831323331323335393539300d06092a864886f70d01010e050003020001":"CRL version \: 1\nissuer name \: CN=ABCD\nthis update \: 2009-01-01 00\:00\:00\nnext update \: 0000-00-00 00\:00\:00\nRevoked certificates\:\nserial number\: AB\:CD revocation date\: 2008-12-31 23\:59\:59\nsigned using \: RSA with SHA-224\n":0 From 5dd5a491dab7c344621a46d68b58d347ed224074 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 16 Jul 2020 18:26:29 +0200 Subject: [PATCH 2/3] x509_crl_parse: fix 1-byte buffer overflow and entry->raw.tag In the entries (mbedtls_x509_crl_entry values) on the list constructed by mbedtls_x509_crl_parse_der(), set entry->raw.tag to (SEQUENCE | CONSTRUCTED) rather than to the tag of the first ASN.1 element of the entry (which happens to be the tag of the serial number, so INTEGER or INTEGER | CONTEXT_SPECIFIC). This is doesn't really matter in practice (and in particular the value is never used in Mbed TLS itself), and isn't documented, but at least it's consistent with how mbedtls_x509_buf is normally used. The primary importance of this change is that the old code tried to access the tag of the first element of the entry even when the entry happened to be empty. If the entry was empty and not followed by anything else in the CRL, this could cause a read 1 byte after the end of the buffer containing the CRL. The test case "X509 CRL ASN1 (TBSCertList, single empty entry at end)" hit the problematic buffer overflow, which is detected with ASan. Credit to OSS-Fuzz for detecting the problem. Signed-off-by: Gilles Peskine --- ChangeLog.d/x509parse_crl-empty_entry.txt | 4 ++++ library/x509_crl.c | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) create mode 100644 ChangeLog.d/x509parse_crl-empty_entry.txt diff --git a/ChangeLog.d/x509parse_crl-empty_entry.txt b/ChangeLog.d/x509parse_crl-empty_entry.txt new file mode 100644 index 000000000..483abb10a --- /dev/null +++ b/ChangeLog.d/x509parse_crl-empty_entry.txt @@ -0,0 +1,4 @@ +Security + * Fix a 1-byte buffer overread in mbedtls_x509_crl_parse_der(). + Credit to OSS-Fuzz for detecting the problem and to Philippe Antoine + for pinpointing the problematic code. diff --git a/library/x509_crl.c b/library/x509_crl.c index 371c446be..67eb5c1a1 100644 --- a/library/x509_crl.c +++ b/library/x509_crl.c @@ -259,13 +259,13 @@ static int x509_get_entries( unsigned char **p, size_t len2; const unsigned char *end2; + cur_entry->raw.tag = **p; if( ( ret = mbedtls_asn1_get_tag( p, end, &len2, MBEDTLS_ASN1_SEQUENCE | MBEDTLS_ASN1_CONSTRUCTED ) ) != 0 ) { return( ret ); } - cur_entry->raw.tag = **p; cur_entry->raw.p = *p; cur_entry->raw.len = len2; end2 = *p + len2; From 210bea38f36757035b5005b8d48394fa3ec8a180 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 6 Aug 2020 16:05:35 +0200 Subject: [PATCH 3/3] Add the decomposition of the base case as a comment Put the base good case first, then the bad cases derived from it. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_x509parse.data | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/tests/suites/test_suite_x509parse.data b/tests/suites/test_suite_x509parse.data index cb530c729..49ff073e8 100644 --- a/tests/suites/test_suite_x509parse.data +++ b/tests/suites/test_suite_x509parse.data @@ -2104,6 +2104,28 @@ X509 CRL ASN1 (TBSCertList, sig present, len mismatch) depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C x509parse_crl:"305d3047020100300d06092a864886f70d01010e0500300f310d300b0603550403130441424344170c303930313031303030303030301430128202abcd170c303831323331323335393539300d06092a864886f70d01010e05000302000100":"":MBEDTLS_ERR_X509_INVALID_FORMAT + MBEDTLS_ERR_ASN1_LENGTH_MISMATCH +# 305c +# 3047 tbsCertList TBSCertList +# 020100 version INTEGER OPTIONAL +# 300d signatureAlgorithm AlgorithmIdentifi +# 06092a864886f70d01010e +# 0500 +# 300f issuer Name +# 310d300b0603550403130441424344 +# 170c303930313031303030303030 thisUpdate Time +# 3014 revokedCertificates +# 3012 entry 1 +# 8202abcd userCertificate CertificateSerialNum +# 170c303831323331323335393539 revocationDate Time +# 300d signatureAlgorithm AlgorithmIdentifi +# 06092a864886f70d01010e +# 0500 +# 03020001 signatureValue BIT STRING +# The subsequent TBSCertList negative tests remove or modify some elements. +X509 CRL ASN1 (TBSCertList, sig present) +depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C +x509parse_crl:"305c3047020100300d06092a864886f70d01010e0500300f310d300b0603550403130441424344170c303930313031303030303030301430128202abcd170c303831323331323335393539300d06092a864886f70d01010e050003020001":"CRL version \: 1\nissuer name \: CN=ABCD\nthis update \: 2009-01-01 00\:00\:00\nnext update \: 0000-00-00 00\:00\:00\nRevoked certificates\:\nserial number\: AB\:CD revocation date\: 2008-12-31 23\:59\:59\nsigned using \: RSA with SHA-224\n":0 + X509 CRL ASN1 (TBSCertList, signatureValue missing) depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C x509parse_crl:"30583047020100300d06092a864886f70d01010e0500300f310d300b0603550403130441424344170c303930313031303030303030301430128202abcd170c303831323331323335393539300d06092a864886f70d01010e0500":"":MBEDTLS_ERR_X509_INVALID_SIGNATURE + MBEDTLS_ERR_ASN1_OUT_OF_DATA @@ -2136,10 +2158,6 @@ X509 CRL ASN1 (TBSCertList, invalid tag for serial) depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C x509parse_crl:"305c3047020100300d06092a864886f70d01010e0500300f310d300b0603550403130441424344170c303930313031303030303030301430128402abcd170c303831323331323335393539300d06092a864886f70d01010e050003020001":"":MBEDTLS_ERR_X509_INVALID_SERIAL + MBEDTLS_ERR_ASN1_UNEXPECTED_TAG -X509 CRL ASN1 (TBSCertList, sig present) -depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C -x509parse_crl:"305c3047020100300d06092a864886f70d01010e0500300f310d300b0603550403130441424344170c303930313031303030303030301430128202abcd170c303831323331323335393539300d06092a864886f70d01010e050003020001":"CRL version \: 1\nissuer name \: CN=ABCD\nthis update \: 2009-01-01 00\:00\:00\nnext update \: 0000-00-00 00\:00\:00\nRevoked certificates\:\nserial number\: AB\:CD revocation date\: 2008-12-31 23\:59\:59\nsigned using \: RSA with SHA-224\n":0 - X509 CRL ASN1 (TBSCertList, no entries) depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C x509parse_crl:"30463031020100300d06092a864886f70d01010e0500300f310d300b0603550403130441424344170c303930313031303030303030300d06092a864886f70d01010e050003020001":"CRL version \: 1\nissuer name \: CN=ABCD\nthis update \: 2009-01-01 00\:00\:00\nnext update \: 0000-00-00 00\:00\:00\nRevoked certificates\:\nsigned using \: RSA with SHA-224\n":0