From 6ccfb18ab16e52209aa3a3d5573757a95f87b288 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 12 Feb 2019 11:52:10 +0000 Subject: [PATCH] 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