fix X509V3_EXT_METHODs for ocsp nonce extension (aws#1603)

samuel40791765 · web-flow · commit 308dca9be4b3 · 2024-05-23T16:09:42.000-07:00
This backports more of the changes done in 736a283 to reintroduce the "old-style" `X509V3_EXT_METHOD`s. Although we're reintroducing the methods we discourage using, I've added checks to ensure that these are only used with `NID_id_pkix_OCSP_Nonce`. The assertion in `X509V3_EXT_add` is kept so that we continue to only allow |ASN1_ITEM|-based extensions for consumers that add their own. I've abstracted out the freeing logic to `x509v3_ext_free_with_method`. This used to be all over the place. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.
diff --git a/crypto/asn1/tasn_dec.c b/crypto/asn1/tasn_dec.c
@@ -170,7 +170,7 @@ static int asn1_item_ex_d2i(ASN1_VALUE **pval, const unsigned char **in,
   ASN1_VALUE **pchptr;
   int combine = aclass & ASN1_TFLG_COMBINE;
   aclass &= ~ASN1_TFLG_COMBINE;
-  if (!pval) {
+  if (pval == NULL || it == NULL) {
     return 0;
   }
 
diff --git a/crypto/asn1/tasn_fre.c b/crypto/asn1/tasn_fre.c
@@ -78,7 +78,7 @@ void asn1_item_combine_free(ASN1_VALUE **pval, const ASN1_ITEM *it,
   const ASN1_TEMPLATE *tt = NULL, *seqtt;
   const ASN1_EXTERN_FUNCS *ef;
   int i;
-  if (!pval) {
+  if (pval == NULL || it == NULL) {
     return;
   }
   if ((it->itype != ASN1_ITYPE_PRIMITIVE) && !*pval) {
diff --git a/crypto/x509/internal.h b/crypto/x509/internal.h
@@ -480,6 +480,10 @@ typedef struct {
 int x509V3_add_value_asn1_string(const char *name, const ASN1_STRING *value,
                                  STACK_OF(CONF_VALUE) **extlist);
 
+// x509v3_ext_free_with_method frees |ext_data| with |ext_method|.
+int x509v3_ext_free_with_method(const X509V3_EXT_METHOD *ext_method,
+                                void *ext_data);
+
 // X509V3_NAME_from_section adds attributes to |nm| by interpreting the
 // key/value pairs in |dn_sk|. It returns one on success and zero on error.
 // |chtype|, which should be one of |MBSTRING_*| constants, determines the
diff --git a/crypto/x509/v3_conf.c b/crypto/x509/v3_conf.c
@@ -205,20 +205,19 @@ static X509_EXTENSION *do_ext_i2d(const X509V3_EXT_METHOD *method, int ext_nid,
     if (ext_len < 0) {
       return NULL;
     }
-  } else {
-    // This is using the "old-style" ASN.1 callbacks. The only X509v3 extension
-    // that's still dependent on this code internally are OCSP nonce extensions.
-    // We can't easily migrate OCSP nonce extensions to use the "new" callbacks
-    // either, since OCSP nonces are handled differently in the code (according
-    // to OpenSSL and us having to maintain backwards compatibility with them).
-    // Every other |X509V3_EXT_METHOD|, both inside and outside the library, has
-    // and should have an |ASN1_ITEM|.
+  } else if (method->ext_nid == NID_id_pkix_OCSP_Nonce && method->i2d != NULL) {
+    // |NID_id_pkix_OCSP_Nonce| is the only extension using the "old-style"
+    // ASN.1 callbacks for backwards compatibility reasons.
+    // Note: See |v3_ext_method| under "include/openssl/x509.h".
     ext_len = method->i2d(ext_struc, NULL);
     if (!(ext_der = OPENSSL_malloc(ext_len))) {
       return NULL;
     }
     unsigned char *p = ext_der;
     method->i2d(ext_struc, &p);
+  } else {
+    OPENSSL_PUT_ERROR(X509, X509V3_R_OPERATION_NOT_DEFINED);
+    return NULL;
   }
 
   ASN1_OCTET_STRING *ext_oct = ASN1_OCTET_STRING_new();
diff --git a/crypto/x509/v3_lib.c b/crypto/x509/v3_lib.c
@@ -98,6 +98,27 @@ static int ext_cmp(const void *void_a, const void *void_b) {
   return ext_stack_cmp(a, b);
 }
 
+static int x509v3_ext_method_validate(const X509V3_EXT_METHOD *ext_method) {
+  if (ext_method == NULL) {
+    return 0;
+  }
+
+  if (ext_method->ext_nid == NID_id_pkix_OCSP_Nonce &&
+      ext_method->d2i != NULL && ext_method->i2d != NULL &&
+      ext_method->ext_new != NULL && ext_method->ext_free != NULL) {
+    // |NID_id_pkix_OCSP_Nonce| is the only extension using the "old-style"
+    // ASN.1 callbacks for backwards compatibility reasons.
+    // Note: See |v3_ext_method| under "include/openssl/x509.h".
+    return 1;
+  }
+
+  if (ext_method->it == NULL) {
+    OPENSSL_PUT_ERROR(X509V3, X509V3_R_OPERATION_NOT_DEFINED);
+    return 0;
+  }
+  return 1;
+}
+
 const X509V3_EXT_METHOD *X509V3_EXT_get_nid(int nid) {
   X509V3_EXT_METHOD tmp;
   const X509V3_EXT_METHOD *t = &tmp, *const * ret;
@@ -109,7 +130,7 @@ const X509V3_EXT_METHOD *X509V3_EXT_get_nid(int nid) {
   tmp.ext_nid = nid;
   ret = bsearch(&t, standard_exts, STANDARD_EXTENSION_COUNT,
                 sizeof(X509V3_EXT_METHOD *), ext_cmp);
-  if (ret) {
+  if (ret != NULL && x509v3_ext_method_validate(*ret)) {
     return *ret;
   }
   if (!ext_list) {
@@ -119,7 +140,12 @@ const X509V3_EXT_METHOD *X509V3_EXT_get_nid(int nid) {
   if (!sk_X509V3_EXT_METHOD_find_awslc(ext_list, &idx, &tmp)) {
     return NULL;
   }
-  return sk_X509V3_EXT_METHOD_value(ext_list, idx);
+
+  const X509V3_EXT_METHOD *method = sk_X509V3_EXT_METHOD_value(ext_list, idx);
+  if (method != NULL && x509v3_ext_method_validate(method)) {
+    return method;
+  }
+  return NULL;
 }
 
 const X509V3_EXT_METHOD *X509V3_EXT_get(const X509_EXTENSION *ext) {
@@ -130,19 +156,34 @@ const X509V3_EXT_METHOD *X509V3_EXT_get(const X509_EXTENSION *ext) {
   return X509V3_EXT_get_nid(nid);
 }
 
-int X509V3_EXT_free(int nid, void *ext_data) {
-  const X509V3_EXT_METHOD *ext_method = X509V3_EXT_get_nid(nid);
+int x509v3_ext_free_with_method(const X509V3_EXT_METHOD *ext_method,
+                                void *ext_data) {
   if (ext_method == NULL) {
     OPENSSL_PUT_ERROR(X509V3, X509V3_R_CANNOT_FIND_FREE_FUNCTION);
     return 0;
   }
 
-  ASN1_item_free(ext_data, ASN1_ITEM_ptr(ext_method->it));
+  if (ext_method->it != NULL) {
+    ASN1_item_free(ext_data, ASN1_ITEM_ptr(ext_method->it));
+  } else if (ext_method->ext_nid == NID_id_pkix_OCSP_Nonce &&
+             ext_method->ext_free != NULL) {
+    // |NID_id_pkix_OCSP_Nonce| is the only extension using the "old-style"
+    // ASN.1 callbacks for backwards compatibility reasons.
+    // Note: See |v3_ext_method| under "include/openssl/x509.h".
+    ext_method->ext_free(ext_data);
+  } else {
+    OPENSSL_PUT_ERROR(X509V3, X509V3_R_CANNOT_FIND_FREE_FUNCTION);
+    return 0;
+  }
   return 1;
 }
 
+int X509V3_EXT_free(int nid, void *ext_data) {
+  return x509v3_ext_free_with_method(X509V3_EXT_get_nid(nid), ext_data);
+}
+
 int X509V3_EXT_add_alias(int nid_to, int nid_from) {
-OPENSSL_BEGIN_ALLOW_DEPRECATED
+  OPENSSL_BEGIN_ALLOW_DEPRECATED
   const X509V3_EXT_METHOD *ext;
   X509V3_EXT_METHOD *tmpext;
 
@@ -161,7 +202,7 @@ OPENSSL_BEGIN_ALLOW_DEPRECATED
     return 0;
   }
   return 1;
-OPENSSL_END_ALLOW_DEPRECATED
+  OPENSSL_END_ALLOW_DEPRECATED
 }
 
 // Legacy function: we don't need to add standard extensions any more because
@@ -179,14 +220,25 @@ void *X509V3_EXT_d2i(const X509_EXTENSION *ext) {
     return NULL;
   }
   p = ext->value->data;
-  void *ret =
-      ASN1_item_d2i(NULL, &p, ext->value->length, ASN1_ITEM_ptr(method->it));
+  void *ret = NULL;
+  if (method->it) {
+    ret =
+        ASN1_item_d2i(NULL, &p, ext->value->length, ASN1_ITEM_ptr(method->it));
+  } else if (method->ext_nid == NID_id_pkix_OCSP_Nonce && method->d2i != NULL) {
+    // |NID_id_pkix_OCSP_Nonce| is the only extension using the "old-style"
+    // ASN.1 callbacks for backwards compatibility reasons.
+    // Note: See |v3_ext_method| under "include/openssl/x509.h".
+    ret = method->d2i(NULL, &p, ext->value->length);
+  } else {
+    assert(0);
+  }
+
   if (ret == NULL) {
     return NULL;
   }
   // Check for trailing data.
   if (p != ext->value->data + ext->value->length) {
-    ASN1_item_free(ret, ASN1_ITEM_ptr(method->it));
+    x509v3_ext_free_with_method(method, ret);
     OPENSSL_PUT_ERROR(X509V3, X509V3_R_TRAILING_DATA_IN_EXTENSION);
     return NULL;
   }
diff --git a/crypto/x509/v3_prn.c b/crypto/x509/v3_prn.c
@@ -63,6 +63,8 @@
 #include <openssl/mem.h>
 #include <openssl/x509.h>
 
+#include "internal.h"
+
 // Extension printing routines
 
 static int unknown_ext_print(BIO *out, const X509_EXTENSION *ext,
@@ -114,8 +116,14 @@ int X509V3_EXT_print(BIO *out, const X509_EXTENSION *ext, unsigned long flag,
   if (method->it) {
     ext_str = ASN1_item_d2i(NULL, &p, ASN1_STRING_length(ext_data),
                             ASN1_ITEM_ptr(method->it));
-  } else {
+  } else if (method->ext_nid == NID_id_pkix_OCSP_Nonce && method->d2i != NULL) {
+    // |NID_id_pkix_OCSP_Nonce| is the only extension using the "old-style"
+    // ASN.1 callbacks for backwards compatibility reasons.
+    // Note: See |v3_ext_method| under "include/openssl/x509.h".
     ext_str = method->d2i(NULL, &p, ASN1_STRING_length(ext_data));
+  } else {
+    OPENSSL_PUT_ERROR(X509V3, X509V3_R_OPERATION_NOT_DEFINED);
+    return 0;
   }
 
   if (!ext_str) {
@@ -150,11 +158,7 @@ int X509V3_EXT_print(BIO *out, const X509_EXTENSION *ext, unsigned long flag,
 err:
   sk_CONF_VALUE_pop_free(nval, X509V3_conf_free);
   OPENSSL_free(value);
-  if (method->it) {
-    ASN1_item_free(ext_str, ASN1_ITEM_ptr(method->it));
-  } else {
-    method->ext_free(ext_str);
-  }
+  x509v3_ext_free_with_method(method, ext_str);
   return ok;
 }
 
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
@@ -42,6 +42,23 @@
 #include <thread>
 #endif
 
+static const char kX509ExtensionsCert[] = R"(
+-----BEGIN CERTIFICATE-----
+MIICwDCCAimgAwIBAgIEAJNOazANBgkqhkiG9w0BAQEFADAQMQ4wDAYDVQQKEwVjYXNzYTAeFw0xMTAz
+MzAxMTEwMzZaFw0xNDAzMzAxNTQwMzZaMC0xDjAMBgNVBAoMBWNhc9ijMRswGQYDVQQDExJlbXMuZ3J1
+cG9jYXNzYdeckJIwgZ8wDQYJKoZIhvcNAQEJBQADgY0AMIGJAoGBAFUEB/i0583rnah0hLRk1hleI5T0
+xw+naVIxs/h4ZHu19xva671kvXfN97PZBmzAwFAWXmfs5MUrviy6RjZjhc0Ad2510cmqWy9FUx4D7kjy
+iuZZIaA0xL0AAfvJbDkXrGpHZWz8BkANFZ/RHl961BRB3fTGvPWiZK6C4mCwPgIDaQABjwGjggEIMIIB
+BDALBgNVHRAEBAMCBaAwKwYDVR0VBCQKCf8O1s/Ozk3MzM8xNTo7MzZaMIIBBDALBgNVHRAEBAMCBaAw
+KwYDVR0VBCQKCf8O1s/OKoUDBwExNTo7MzZagQ8BBDALBgNVHRAEBAMCBaAwKwYDVR0VBCQKAYPxKTDO
+zs3MzM8xNTo7MzZagQ8yMDEzMDAxMzAxKjUwNlowEQYJKwYBBQUHMAECBAQDAgEHMBEGCWCGSAGG+EJZ
+AQQEAwIGQDAbBgNVHQ0EFDASMBAGCSqGSIb2fQcQBAQDAgWgMCsGA1UdFQQkCiFD8Skwzs7NzMzPMTU6
+OzM2WoEPAQQwCwYDVR0QBAQDAgWgMA0GCSsGAQUFBzABBQUAA4GBAKTNw5fTOnPCcOFMARmAD1RtaAN8
+0TBKIy2A0hG/2dlNeI6s0dqZe6juverYmC5sOResakdlbPwGQA0Vn9EeX8Yr67/d9Ma89aJkroLiXbA+
+j2kCAwG+LLpGNmNwcHBwcHBwcHBwcHBwcHBwcHBwcHBwcHBw5lmgITTEvXIj+8ls
+-----END CERTIFICATE-----
+)";
+
 
 std::string GetTestData(const char *path);
 
@@ -1550,6 +1567,20 @@ static int Verify(
   return X509_V_OK;
 }
 
+TEST(X509Test, X509Extensions) {
+  bssl::UniquePtr<X509> cert(CertFromPEM(kX509ExtensionsCert));
+  ASSERT_TRUE(cert);
+
+  for (int i = 0; i < X509_get_ext_count(cert.get()); i++) {
+    const X509_EXTENSION *ext = X509_get_ext(cert.get(), i);
+    void *parsed = X509V3_EXT_d2i(ext);
+    if (parsed != nullptr) {
+      int nid = OBJ_obj2nid(X509_EXTENSION_get_object(ext));
+      ASSERT_TRUE(X509V3_EXT_free(nid, parsed));
+    }
+  }
+}
+
 TEST(X509Test, TestVerify) {
   //  cross_signing_root
   //         |

Original file line number	Diff line number	Diff line change
`@@ -170,7 +170,7 @@ static int asn1_item_ex_d2i(ASN1_VALUE pval, const unsigned char in,`
`170`	`170`	`ASN1_VALUE **pchptr;`
`171`	`171`	`int combine = aclass & ASN1_TFLG_COMBINE;`
`172`	`172`	`aclass &= ~ASN1_TFLG_COMBINE;`
`173`		`- if (!pval) {`
	`173`	`+ if (pval == NULL \|\| it == NULL) {`
`174`	`174`	`return 0;`
`175`	`175`	`}`
`176`	`176`
Original file line number	Diff line number	Diff line change
`@@ -78,7 +78,7 @@ void asn1_item_combine_free(ASN1_VALUE *pval, const ASN1_ITEM it,`
`78`	`78`	`const ASN1_TEMPLATE tt = NULL, seqtt;`
`79`	`79`	`const ASN1_EXTERN_FUNCS *ef;`
`80`	`80`	`int i;`
`81`		`- if (!pval) {`
	`81`	`+ if (pval == NULL \|\| it == NULL) {`
`82`	`82`	`return;`
`83`	`83`	`}`
`84`	`84`	`if ((it->itype != ASN1_ITYPE_PRIMITIVE) && !*pval) {`