diff --git a/library/x509_crt.c b/library/x509_crt.c index 637ce7f336..093d733c1c 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -512,6 +512,7 @@ static int x509_get_basic_constraints(unsigned char **p, } if (*p == end) { + /* Empty basicConstraints: valid, not a CA. */ return 0; } @@ -523,18 +524,21 @@ static int x509_get_basic_constraints(unsigned char **p, } if ((ret = mbedtls_asn1_get_bool(p, end, ca_istrue)) != 0) { - if (ret == MBEDTLS_ERR_ASN1_UNEXPECTED_TAG) { - ret = mbedtls_asn1_get_int(p, end, ca_istrue); - } - - if (ret != 0) { - return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_X509_INVALID_EXTENSIONS, ret); - } - - if (*ca_istrue != 0) { - *ca_istrue = 1; - } + /* If the SEQUENCE starts with an INTEGER and not a BOOLEAN, + * according to RFC 5280, it's syntactically valid, but it's + * something that a CA MUST NOT produce. So we reject it. + * + * Note: historically, from XySSL 0.9 to Mbed TLS 3.6.6/4.1.0, + * `SEQUENCE { INTEGER n }` was interpreted as cA=FALSE if n == 0 + * and cA=TRUE if n != 0. This was dangerous since + * `SEQUENCE { INTEGER n }` should be parsed as cA=FALSE + * according to RFC 5280, so we stopped doing it. + */ + return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_X509_INVALID_EXTENSIONS, ret); } + /* `SEQUENCE { BOOLEAN FALSE }` is not DER since default-value fields + * must be omitted in DER. But it seems harmless, and Mbed TLS has + * always accepted it, so we continue to accept it. */ if (*p == end) { return 0; diff --git a/tests/suites/test_suite_x509parse.data b/tests/suites/test_suite_x509parse.data index ff4ecaedc1..a2e3e9c67e 100644 --- a/tests/suites/test_suite_x509parse.data +++ b/tests/suites/test_suite_x509parse.data @@ -2060,6 +2060,14 @@ X509 CRT ASN1: basicConstraints overflow would make CA=1 (good self signature) depends_on:MBEDTLS_PK_CAN_ECDSA_SOME:MBEDTLS_ECP_HAVE_SECP256R1:MBEDTLS_MD_CAN_SHA256 mbedtls_x509_crt_parse_file:"../framework/data_files/parse_input/server5.basic-constraints-sequence-overflow.selfsigned.crt":MBEDTLS_ERR_X509_INVALID_EXTENSIONS:0 +X509 CRT ASN1: basicConstraints: no cA but pathLenConstraint (bad signature) +depends_on:MBEDTLS_PK_CAN_ECDSA_SOME:MBEDTLS_ECP_HAVE_SECP256R1:MBEDTLS_MD_CAN_SHA256 +mbedtls_x509_crt_parse_file:"../framework/data_files/parse_input/server5.basic-constraints-integer-first.badsign.crt":MBEDTLS_ERROR_ADD(MBEDTLS_ERR_X509_INVALID_EXTENSIONS, MBEDTLS_ERR_ASN1_UNEXPECTED_TAG):0 + +X509 CRT ASN1: basicConstraints: no cA but pathLenConstraint (good self signature) +depends_on:MBEDTLS_PK_CAN_ECDSA_SOME:MBEDTLS_ECP_HAVE_SECP256R1:MBEDTLS_MD_CAN_SHA256 +mbedtls_x509_crt_parse_file:"../framework/data_files/parse_input/server5.basic-constraints-integer-first.selfsigned.crt":MBEDTLS_ERROR_ADD(MBEDTLS_ERR_X509_INVALID_EXTENSIONS, MBEDTLS_ERR_ASN1_UNEXPECTED_TAG):0 + X509 CRT ASN1 (TBS, inv extBasicConstraint, no pathlen length) depends_on:PSA_HAVE_ALG_SOME_RSA_VERIFY:PSA_WANT_ALG_SHA_256 x509parse_crt:"3081b030819aa0030201028204deadbeef300d06092a864886f70d01010b0500300c310a30080600130454657374301c170c303930313031303030303030170c303931323331323335393539300c310a30080600130454657374302a300d06092A864886F70D01010105000319003016021100ffffffffffffffffffffffffffffffff020103a100a200a314301230100603551d13010101040630040101ff02300d06092a864886f70d01010b0500030200ff":"":MBEDTLS_ERROR_ADD(MBEDTLS_ERR_X509_INVALID_EXTENSIONS, MBEDTLS_ERR_ASN1_OUT_OF_DATA)