commit c4f885dc028959726e7bc9c19975400024d1962b
author hanno-becker <hanno.becker@arm.com> Wed Feb 08 08:50:01 2023 -0500
committer Andrzej Kurek <andrzej.kurek@arm.com> Wed Feb 08 08:50:01 2023 -0500
tree 427845f504efb0cd4729850887dfca1fa4d60122
parent 75ab0769710694ef7899a4ebc8bcac31d8d173dd

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 <andrzej.kurek@arm.com>

library/x509_crt.c

diff --git a/library/x509_crt.c b/library/x509_crt.c
index d224e2a..cb2740f 100644
--- a/library/x509_crt.c
+++ b/library/x509_crt.c
@@ -1742,11 +1742,6 @@
         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 @@
         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 @@
     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 @@
     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);
     }