From 12f62fb82c81aa3f52bed49dff0f9e038d115f75 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 12 Feb 2019 17:22:36 +0000 Subject: [PATCH 1/5] Obey bounds of ASN.1 substructures When parsing a substructure of an ASN.1 structure, no field within the substructure must exceed the bounds of the substructure. Concretely, the `end` pointer passed to the ASN.1 parsing routines must be updated to point to the end of the substructure while parsing the latter. This was previously not the case for the routines - x509_get_attr_type_and_value(), - mbedtls_x509_get_crt_ext(), - mbedtls_x509_get_crl_ext(). These functions kept using the end of the parent structure as the `end` pointer and would hence allow substructure fields to cross the substructure boundary. This could lead to successful parsing of ill-formed X.509 CRTs. This commit fixes this. Care has to be taken when adapting `mbedtls_x509_get_crt_ext()` and `mbedtls_x509_get_crl_ext()`, as the underlying function `mbedtls_x509_get_ext()` returns `0` if no extensions are present but doesn't set the variable which holds the bounds of the Extensions structure in case the latter is present. This commit addresses this by returning early from `mbedtls_x509_get_crt_ext()` and `mbedtls_x509_get_crl_ext()` if parsing has reached the end of the input buffer. The following X.509 parsing tests need to be adapted: - "TBSCertificate, issuer two inner set datas" This test exercises the X.509 CRT parser with a Subject name which has two empty `AttributeTypeAndValue` structures. This is supposed to fail with `MBEDTLS_ERR_ASN1_OUT_OF_DATA` because the parser should attempt to parse the first structure and fail because of a lack of data. Previously, it failed to obey the (0-length) bounds of the first AttributeTypeAndValue structure and would try to interpret the beginning of the second AttributeTypeAndValue structure as the first field of the first AttributeTypeAndValue structure, returning an UNEXPECTED_TAG error. - "TBSCertificate, issuer, no full following string" This test exercises the parser's behaviour on an AttributeTypeAndValue structure which contains more data than expected; it should therefore fail with MBEDTLS_ERR_ASN1_LENGTH_MISMATCH. Because of the missing bounds check, it previously failed with UNEXPECTED_TAG because it interpreted the remaining byte in the first AttributeTypeAndValue structure as the first byte in the second AttributeTypeAndValue structure. - "SubjectAltName repeated" This test should exercise two SubjectAltNames extensions in succession, but a wrong length values makes the second SubjectAltNames extension appear outside of the Extensions structure. With the new bounds in place, this therefore fails with a LENGTH_MISMATCH error. This commit adapts the test data to put the 2nd SubjectAltNames extension inside the Extensions structure, too. --- library/x509.c | 8 ++++++++ library/x509_crl.c | 5 +++++ library/x509_crt.c | 4 ++++ tests/suites/test_suite_x509parse.data | 6 +++--- 4 files changed, 20 insertions(+), 3 deletions(-) diff --git a/library/x509.c b/library/x509.c index 380fec237..1826f1d7b 100644 --- a/library/x509.c +++ b/library/x509.c @@ -361,6 +361,8 @@ static int x509_get_attr_type_value( unsigned char **p, MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE ) ) != 0 ) return( MBEDTLS_ERR_X509_INVALID_NAME + ret ); + end = *p + len; + if( ( end - *p ) < 1 ) return( MBEDTLS_ERR_X509_INVALID_NAME + MBEDTLS_ERR_ASN1_OUT_OF_DATA ); @@ -394,6 +396,12 @@ static int x509_get_attr_type_value( unsigned char **p, val->p = *p; *p += val->len; + if( *p != end ) + { + return( MBEDTLS_ERR_X509_INVALID_NAME + + MBEDTLS_ERR_ASN1_LENGTH_MISMATCH ); + } + cur->next = NULL; return( 0 ); diff --git a/library/x509_crl.c b/library/x509_crl.c index 8450f87e0..64fac0e0c 100644 --- a/library/x509_crl.c +++ b/library/x509_crl.c @@ -103,6 +103,9 @@ static int x509_get_crl_ext( unsigned char **p, { int ret; + if( *p == end ) + return( 0 ); + /* * crlExtensions [0] EXPLICIT Extensions OPTIONAL * -- if present, version MUST be v2 @@ -115,6 +118,8 @@ static int x509_get_crl_ext( unsigned char **p, return( ret ); } + end = ext->p + ext->len; + while( *p < end ) { /* diff --git a/library/x509_crt.c b/library/x509_crt.c index dfd22f6e5..0287b5b86 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -891,6 +891,9 @@ static int x509_get_crt_ext( unsigned char **p, size_t len; unsigned char *end_ext_data, *end_ext_octet; + if( *p == end ) + return( 0 ); + if( ( ret = mbedtls_x509_get_ext( p, end, &crt->v3_ext, 3 ) ) != 0 ) { if( ret == MBEDTLS_ERR_ASN1_UNEXPECTED_TAG ) @@ -899,6 +902,7 @@ static int x509_get_crt_ext( unsigned char **p, return( ret ); } + end = crt->v3_ext.p + crt->v3_ext.len; while( *p < end ) { /* diff --git a/tests/suites/test_suite_x509parse.data b/tests/suites/test_suite_x509parse.data index b12a68d38..5cefb960c 100644 --- a/tests/suites/test_suite_x509parse.data +++ b/tests/suites/test_suite_x509parse.data @@ -1096,7 +1096,7 @@ x509parse_crt:"30223020a0030201028204deadbeef300d06092a864886f70d01010b050030043 X509 Certificate ASN1 (TBSCertificate, issuer two inner set datas) depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C -x509parse_crt:"30243022a0030201028204deadbeef300d06092a864886f70d01010b05003006310430003000":"":MBEDTLS_ERR_X509_INVALID_NAME + MBEDTLS_ERR_ASN1_UNEXPECTED_TAG +x509parse_crt:"30243022a0030201028204deadbeef300d06092a864886f70d01010b05003006310430003000":"":MBEDTLS_ERR_X509_INVALID_NAME + MBEDTLS_ERR_ASN1_OUT_OF_DATA X509 Certificate ASN1 (TBSCertificate, issuer no oid data) depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C @@ -1112,7 +1112,7 @@ x509parse_crt:"30253023a0030201028204deadbeef300d06092a864886f70d01010b050030073 X509 Certificate ASN1 (TBSCertificate, issuer, no full following string) depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C -x509parse_crt:"302b3029a0030201028204deadbeef300d06092a864886f70d01010b0500300d310b3009060013045465737400":"":MBEDTLS_ERR_X509_INVALID_NAME+MBEDTLS_ERR_ASN1_UNEXPECTED_TAG +x509parse_crt:"302b3029a0030201028204deadbeef300d06092a864886f70d01010b0500300d310b3009060013045465737400":"":MBEDTLS_ERR_X509_INVALID_NAME+MBEDTLS_ERR_ASN1_LENGTH_MISMATCH X509 Certificate ASN1 (TBSCertificate, valid issuer, no validity) depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C @@ -1264,7 +1264,7 @@ x509parse_crt:"3081de3081dba003020102020900ebdbcd14105e1839300906072a8648ce3d040 X509 Certificate ASN1 (SubjectAltName repeated) depends_on:MBEDTLS_ECDSA_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED:MBEDTLS_SHA1_C -x509parse_crt:"3081fd3081faa003020102020900a8b31ff37d09a37f300906072a8648ce3d0401300f310d300b0603550403130454657374301e170d3134313131313231333731365a170d3234313130383231333731365a300f310d300b06035504031304546573743059301306072a8648ce3d020106082a8648ce3d0301070342000437cc56d976091e5a723ec7592dff206eee7cf9069174d0ad14b5f768225962924ee500d82311ffea2fd2345d5d16bd8a88c26b770d55cd8a2a0efa01c8b4edffa321301f301d0603551d11041630148208666f6f2e7465737482086261722e74657374301d0603551d11041630148208666f6f2e7465737482086261722e74657374":"":MBEDTLS_ERR_X509_INVALID_EXTENSIONS +x509parse_crt:"3081fd3081faa003020102020900a8b31ff37d09a37f300906072a8648ce3d0401300f310d300b0603550403130454657374301e170d3134313131313231333731365a170d3234313130383231333731365a300f310d300b06035504031304546573743059301306072a8648ce3d020106082a8648ce3d0301070342000437cc56d976091e5a723ec7592dff206eee7cf9069174d0ad14b5f768225962924ee500d82311ffea2fd2345d5d16bd8a88c26b770d55cd8a2a0efa01c8b4edffa340303e301d0603551d11041630148208666f6f2e7465737482086261722e74657374301d0603551d11041630148208666f6f2e7465737482086261722e74657374":"":MBEDTLS_ERR_X509_INVALID_EXTENSIONS X509 Certificate ASN1 (ExtKeyUsage repeated) depends_on:MBEDTLS_ECDSA_C:MBEDTLS_ECP_DP_SECP256R1_ENABLED:MBEDTLS_SHA1_C From 6ccfb18ab16e52209aa3a3d5573757a95f87b288 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 12 Feb 2019 11:52:10 +0000 Subject: [PATCH 2/5] Always return a high-level error code from X.509 module Some functions within the X.509 module return an ASN.1 low level error code where instead this error code should be wrapped by a high-level X.509 error code as in the bulk of the module. Specifically, the following functions are affected: - mbedtls_x509_get_ext() - x509_get_version() - x509_get_uid() This commit modifies these functions to always return an X.509 high level error code. Care has to be taken when adapting `mbetls_x509_get_ext()`: Currently, the callers `mbedtls_x509_crt_ext()` treat the return code `MBEDTLS_ERR_ASN1_UNEXPECTED_TAG` specially to gracefully detect and continue if the extension structure is not present. Wrapping the ASN.1 error with `MBEDTLS_ERR_X509_INVALID_EXTENSIONS` and adapting the check accordingly would mean that an unexpected tag somewhere down the extension parsing would be ignored by the caller. The way out of this is the following: Luckily, the extension structure is always the last field in the surrounding structure, so if there is some data remaining, it must be an Extension structure, so we don't need to deal with a tag mismatch gracefully in the first place. We may therefore wrap the return code from the initial call to `mbedtls_asn1_get_tag()` in `mbedtls_x509_get_ext()` by `MBEDTLS_ERR_X509_INVALID_EXTENSIONS` and simply remove the special treatment of `MBEDTLS_ERR_ASN1_UNEXPECTED_TAG` in the callers `x509_crl_get_ext()` and `x509_crt_get_ext()`. This renders `mbedtls_x509_get_ext()` unsuitable if it ever happened that an Extension structure is optional and does not occur at the end of its surrounding structure, but for CRTs and CRLs, it's fine. The following tests need to be adapted: - "TBSCertificate v3, issuerID wrong tag" The issuerID is optional, so if we look for its presence but find a different tag, we silently continue and try parsing the subjectID, and then the extensions. The tag '00' used in this test doesn't match either of these, and the previous code would hence return LENGTH_MISMATCH after unsucessfully trying issuerID, subjectID and Extensions. With the new code, any data remaining after issuerID and subjectID _must_ be Extension data, so we fail with UNEXPECTED_TAG when trying to parse the Extension data. - "TBSCertificate v3, UIDs, invalid length" The test hardcodes the expectation of MBEDTLS_ERR_ASN1_INVALID_LENGTH, which needs to be wrapped in MBEDTLS_ERR_X509_INVALID_FORMAT now. Fixes #2431. --- library/x509.c | 19 ++++++++----------- library/x509_crl.c | 5 ----- library/x509_crt.c | 9 ++------- tests/suites/test_suite_x509parse.data | 4 ++-- 4 files changed, 12 insertions(+), 25 deletions(-) diff --git a/library/x509.c b/library/x509.c index 1826f1d7b..2239f3bcc 100644 --- a/library/x509.c +++ b/library/x509.c @@ -708,22 +708,19 @@ int mbedtls_x509_get_sig_alg( const mbedtls_x509_buf *sig_oid, const mbedtls_x50 * be either manually updated or extensions should be parsed!) */ int mbedtls_x509_get_ext( unsigned char **p, const unsigned char *end, - mbedtls_x509_buf *ext, int tag ) + mbedtls_x509_buf *ext, int tag ) { int ret; size_t len; - if( *p == end ) - return( 0 ); + ret = mbedtls_asn1_get_tag( p, end, &ext->len, + MBEDTLS_ASN1_CONTEXT_SPECIFIC | MBEDTLS_ASN1_CONSTRUCTED | tag ); + if( ret != 0 ) + return( MBEDTLS_ERR_X509_INVALID_EXTENSIONS + ret ); - ext->tag = **p; - - if( ( ret = mbedtls_asn1_get_tag( p, end, &ext->len, - MBEDTLS_ASN1_CONTEXT_SPECIFIC | MBEDTLS_ASN1_CONSTRUCTED | tag ) ) != 0 ) - return( ret ); - - ext->p = *p; - end = *p + ext->len; + ext->tag = MBEDTLS_ASN1_CONTEXT_SPECIFIC | MBEDTLS_ASN1_CONSTRUCTED | tag; + ext->p = *p; + end = *p + ext->len; /* * Extensions ::= SEQUENCE SIZE (1..MAX) OF Extension diff --git a/library/x509_crl.c b/library/x509_crl.c index 64fac0e0c..00f8545d7 100644 --- a/library/x509_crl.c +++ b/library/x509_crl.c @@ -111,12 +111,7 @@ static int x509_get_crl_ext( unsigned char **p, * -- if present, version MUST be v2 */ if( ( ret = mbedtls_x509_get_ext( p, end, ext, 0 ) ) != 0 ) - { - if( ret == MBEDTLS_ERR_ASN1_UNEXPECTED_TAG ) - return( 0 ); - return( ret ); - } end = ext->p + ext->len; diff --git a/library/x509_crt.c b/library/x509_crt.c index 0287b5b86..d101bc748 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -402,7 +402,7 @@ static int x509_get_version( unsigned char **p, return( 0 ); } - return( ret ); + return( MBEDTLS_ERR_X509_INVALID_FORMAT + ret ); } end = *p + len; @@ -469,7 +469,7 @@ static int x509_get_uid( unsigned char **p, if( ret == MBEDTLS_ERR_ASN1_UNEXPECTED_TAG ) return( 0 ); - return( ret ); + return( MBEDTLS_ERR_X509_INVALID_FORMAT + ret ); } uid->p = *p; @@ -895,12 +895,7 @@ static int x509_get_crt_ext( unsigned char **p, return( 0 ); if( ( ret = mbedtls_x509_get_ext( p, end, &crt->v3_ext, 3 ) ) != 0 ) - { - if( ret == MBEDTLS_ERR_ASN1_UNEXPECTED_TAG ) - return( 0 ); - return( ret ); - } end = crt->v3_ext.p + crt->v3_ext.len; while( *p < end ) diff --git a/tests/suites/test_suite_x509parse.data b/tests/suites/test_suite_x509parse.data index 5cefb960c..3363101a2 100644 --- a/tests/suites/test_suite_x509parse.data +++ b/tests/suites/test_suite_x509parse.data @@ -1188,7 +1188,7 @@ x509parse_crt:"308183308180a0030201028204deadbeef300d06092a864886f70d01010b05003 X509 Certificate ASN1 (TBSCertificate v3, issuerID wrong tag) depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C -x509parse_crt:"308184308181a0030201028204deadbeef300d06092a864886f70d01010b0500300c310a30080600130454657374301c170c303930313031303030303030170c303931323331323335393539300c310a30080600130454657374302a300d06092a864886f70d010101050003190030160210ffffffffffffffffffffffffffffffff0202ffff00":"":MBEDTLS_ERR_X509_INVALID_FORMAT + MBEDTLS_ERR_ASN1_LENGTH_MISMATCH +x509parse_crt:"308184308181a0030201028204deadbeef300d06092a864886f70d01010b0500300c310a30080600130454657374301c170c303930313031303030303030170c303931323331323335393539300c310a30080600130454657374302a300d06092a864886f70d010101050003190030160210ffffffffffffffffffffffffffffffff0202ffff00":"":MBEDTLS_ERR_X509_INVALID_EXTENSIONS + MBEDTLS_ERR_ASN1_UNEXPECTED_TAG X509 Certificate ASN1 (TBSCertificate v3, UIDs, no ext) depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C @@ -1196,7 +1196,7 @@ x509parse_crt:"308189308186a0030201028204deadbeef300d06092a864886f70d01010b05003 X509 Certificate ASN1 (TBSCertificate v3, UIDs, invalid length) depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C -x509parse_crt:"308189308186a0030201028204deadbeef300d06092a864886f70d01010b0500300c310a30080600130454657374301c170c303930313031303030303030170c303931323331323335393539300c310a30080600130454657374302a300d06092a864886f70d010101050003190030160210ffffffffffffffffffffffffffffffff0202ffffa185aaa201bb":"":MBEDTLS_ERR_ASN1_INVALID_LENGTH +x509parse_crt:"308189308186a0030201028204deadbeef300d06092a864886f70d01010b0500300c310a30080600130454657374301c170c303930313031303030303030170c303931323331323335393539300c310a30080600130454657374302a300d06092a864886f70d010101050003190030160210ffffffffffffffffffffffffffffffff0202ffffa185aaa201bb":"":MBEDTLS_ERR_X509_INVALID_FORMAT + MBEDTLS_ERR_ASN1_INVALID_LENGTH X509 Certificate ASN1 (TBSCertificate v3, ext empty) depends_on:MBEDTLS_RSA_C:MBEDTLS_SHA256_C From d57a3a6a4f2b52129f7cb17b27c1b76802504ec8 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Thu, 14 Feb 2019 17:19:23 +0000 Subject: [PATCH 3/5] Adapt ChangeLog --- ChangeLog | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/ChangeLog b/ChangeLog index 458a90f5b..ea1dfa0a9 100644 --- a/ChangeLog +++ b/ChangeLog @@ -130,6 +130,11 @@ Bugfix extensions in CSRs and CRTs that caused these bitstrings to not be encoded correctly as trailing zeroes were not accounted for as unused bits in the leading content octet. Fixes #1610. + * Fix missing bounds checks in X.509 parsing functions that could + lead to successful parsing of ill-formed X.509 CRTs. Fixes #2437. + * Fix multiple X.509 functions previously returning ASN.1 low-level error + codes to always wrap these codes into X.509 high level error codes before + returning. Fixes #2431. Changes * Reduce RAM consumption during session renegotiation by not storing From 3cddba887a1e293d79c98da41f366c69883c8bfc Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 11 Feb 2019 14:33:36 +0000 Subject: [PATCH 4/5] Improve documentation of mbedtls_x509_get_ext() - Explain the use of explicit ASN.1 tagging for the extensions structuree - Remove misleading comment which suggests that mbedtls_x509_get_ext() also parsed the header of the first extension, which is not the case. --- library/x509.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/library/x509.c b/library/x509.c index 2239f3bcc..2e0b0e8f6 100644 --- a/library/x509.c +++ b/library/x509.c @@ -713,6 +713,9 @@ int mbedtls_x509_get_ext( unsigned char **p, const unsigned char *end, int ret; size_t len; + /* Extension structure use EXPLICIT tagging. That is, the actual + * `Extensions` structure is wrapped by a tag-length pair using + * the respective context-specific tag. */ ret = mbedtls_asn1_get_tag( p, end, &ext->len, MBEDTLS_ASN1_CONTEXT_SPECIFIC | MBEDTLS_ASN1_CONSTRUCTED | tag ); if( ret != 0 ) @@ -724,11 +727,6 @@ int mbedtls_x509_get_ext( unsigned char **p, const unsigned char *end, /* * Extensions ::= SEQUENCE SIZE (1..MAX) OF Extension - * - * Extension ::= SEQUENCE { - * extnID OBJECT IDENTIFIER, - * critical BOOLEAN DEFAULT FALSE, - * extnValue OCTET STRING } */ if( ( ret = mbedtls_asn1_get_tag( p, end, &len, MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE ) ) != 0 ) From 3c03a881eb37b0ad63d7cdb6813e1106d5f30d74 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 4 Jun 2019 10:27:43 +0100 Subject: [PATCH 5/5] Correct placement of ChangeLog entry --- ChangeLog | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/ChangeLog b/ChangeLog index ea1dfa0a9..4c45565cf 100644 --- a/ChangeLog +++ b/ChangeLog @@ -49,6 +49,11 @@ Bugfix * Set the next sequence of the subject_alt_name to NULL when deleting sequence on failure. Found and fix suggested by Philippe Antoine. Credit to OSS-Fuzz. + * Fix missing bounds checks in X.509 parsing functions that could + lead to successful parsing of ill-formed X.509 CRTs. Fixes #2437. + * Fix multiple X.509 functions previously returning ASN.1 low-level error + codes to always wrap these codes into X.509 high level error codes before + returning. Fixes #2431. API Changes * Extend the MBEDTLS_SSL_EXPORT_KEYS to export the handshake randbytes, @@ -130,11 +135,6 @@ Bugfix extensions in CSRs and CRTs that caused these bitstrings to not be encoded correctly as trailing zeroes were not accounted for as unused bits in the leading content octet. Fixes #1610. - * Fix missing bounds checks in X.509 parsing functions that could - lead to successful parsing of ill-formed X.509 CRTs. Fixes #2437. - * Fix multiple X.509 functions previously returning ASN.1 low-level error - codes to always wrap these codes into X.509 high level error codes before - returning. Fixes #2431. Changes * Reduce RAM consumption during session renegotiation by not storing