From c4f885dc028959726e7bc9c19975400024d1962b Mon Sep 17 00:00:00 2001 From: hanno-becker Date: Wed, 8 Feb 2023 08:50:01 -0500 Subject: [PATCH] X.509: Remove red'n bounds checks and zeroiz'n in OtherName parsing - ASN.1 parsing functions check that length don't exceed buffer bounds, so checks `p + len > end` are redundant. - If `p + len == end`, this is erroneous because we expect further fields, which is automatically caught by the next ASN.1 parsing call. Hence, the two branches handling `p + len >= end` in x509_get_other_name() can be removed. Further, zeroization of the `other_name` structure isn't necessary because it's not confidential (and it's also not performed on other error conditions in this function). Signed-off-by: Andrzej Kurek --- library/x509_crt.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/library/x509_crt.c b/library/x509_crt.c index d224e2a4f..cb2740fba 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -1742,11 +1742,6 @@ static int x509_get_other_name(const mbedtls_x509_buf *subject_alt_name, return MBEDTLS_ERR_X509_FEATURE_UNAVAILABLE; } - if (p + len >= end) { - mbedtls_platform_zeroize(other_name, sizeof(*other_name)); - return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_X509_INVALID_EXTENSIONS, - MBEDTLS_ERR_ASN1_LENGTH_MISMATCH); - } p += len; if ((ret = mbedtls_asn1_get_tag(&p, end, &len, MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_CONTEXT_SPECIFIC)) != @@ -1754,11 +1749,21 @@ static int x509_get_other_name(const mbedtls_x509_buf *subject_alt_name, return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_X509_INVALID_EXTENSIONS, ret); } + if (end != p + len) { + return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_X509_INVALID_EXTENSIONS, + MBEDTLS_ERR_ASN1_LENGTH_MISMATCH); + } + if ((ret = mbedtls_asn1_get_tag(&p, end, &len, MBEDTLS_ASN1_CONSTRUCTED | MBEDTLS_ASN1_SEQUENCE)) != 0) { return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_X509_INVALID_EXTENSIONS, ret); } + if (end != p + len) { + return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_X509_INVALID_EXTENSIONS, + MBEDTLS_ERR_ASN1_LENGTH_MISMATCH); + } + if ((ret = mbedtls_asn1_get_tag(&p, end, &len, MBEDTLS_ASN1_OID)) != 0) { return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_X509_INVALID_EXTENSIONS, ret); } @@ -1767,11 +1772,6 @@ static int x509_get_other_name(const mbedtls_x509_buf *subject_alt_name, other_name->value.hardware_module_name.oid.p = p; other_name->value.hardware_module_name.oid.len = len; - if (p + len >= end) { - mbedtls_platform_zeroize(other_name, sizeof(*other_name)); - return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_X509_INVALID_EXTENSIONS, - MBEDTLS_ERR_ASN1_LENGTH_MISMATCH); - } p += len; if ((ret = mbedtls_asn1_get_tag(&p, end, &len, MBEDTLS_ASN1_OCTET_STRING)) != 0) { @@ -1783,8 +1783,6 @@ static int x509_get_other_name(const mbedtls_x509_buf *subject_alt_name, other_name->value.hardware_module_name.val.len = len; p += len; if (p != end) { - mbedtls_platform_zeroize(other_name, - sizeof(*other_name)); return MBEDTLS_ERROR_ADD(MBEDTLS_ERR_X509_INVALID_EXTENSIONS, MBEDTLS_ERR_ASN1_LENGTH_MISMATCH); }