From 4e1bfc19cc290be576615f7a2825d8aaea66bf74 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 12 Feb 2019 17:22:36 +0000 Subject: [PATCH] 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 aaf230105..58d40eba0 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 ebd118db0..7dae26e18 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -699,6 +699,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 ) @@ -707,6 +710,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 d0786efec..6dc44e4fe 100644 --- a/tests/suites/test_suite_x509parse.data +++ b/tests/suites/test_suite_x509parse.data @@ -1016,7 +1016,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 @@ -1032,7 +1032,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 @@ -1164,7 +1164,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