commit 3c7053975b35a631f477f42f07502003d35aa2ff
author David Benjamin <davidben@google.com> Thu Feb 02 17:25:07 2023 -0500
committer Boringssl LUCI CQ <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com> Thu Feb 02 22:39:35 2023 +0000
tree b3e1230efa59b8ea5bb0a5a7ecfcb42eafdfb073
parent 50de086abd0f23b58320d6aa310bacdd48e80e53

Fix leak on error in v2i_POLICY_MAPPINGS

If obj2 were invalid, obj1 leaks. Also both leak if creating the
POLICY_MAPPINGS object fails on allocation error. Just swap the order,
so the ASN1_OBJECTs go to an owned pointer from the start.

Bug: oss-fuzz:55636
Change-Id: Ibf0bf58f44db510623035004f6eb1e00961a5454
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56805
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Adam Langley <agl@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>

crypto/x509/x509_test.cc

diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index cd231c8..16c47d4 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -5762,6 +5762,14 @@
       {"subjectAltName", "otherName:invalid_oid;BOOLEAN:TRUE", nullptr, {}},
       {"subjectAltName", "otherName:1.2.3.4;invalid_value", nullptr, {}},
 
+      {"policyMappings",
+       "1.1.1.1:2.2.2.2",
+       nullptr,
+       {0x30, 0x15, 0x06, 0x03, 0x55, 0x1d, 0x21, 0x04, 0x0e, 0x30, 0x0c, 0x30,
+        0x0a, 0x06, 0x03, 0x29, 0x01, 0x01, 0x06, 0x03, 0x52, 0x02, 0x02}},
+      {"policyMappings", "invalid_oid:2.2.2.2", nullptr, {}},
+      {"policyMappings", "1.1.1.1:invalid_oid", nullptr, {}},
+
       // The "DER:" prefix just specifies an arbitrary byte string. Colons
       // separators are ignored.
       {kTestOID, "DER:0001020304", nullptr, {0x30, 0x15, 0x06, 0x0c, 0x2a, 0x86,

crypto/x509v3/v3_pmaps.c

diff --git a/crypto/x509v3/v3_pmaps.c b/crypto/x509v3/v3_pmaps.c
index b6f3384..dae4b66 100644
--- a/crypto/x509v3/v3_pmaps.c
+++ b/crypto/x509v3/v3_pmaps.c
@@ -124,28 +124,29 @@
   for (size_t i = 0; i < sk_CONF_VALUE_num(nval); i++) {
     const CONF_VALUE *val = sk_CONF_VALUE_value(nval, i);
     if (!val->value || !val->name) {
-      sk_POLICY_MAPPING_pop_free(pmaps, POLICY_MAPPING_free);
       OPENSSL_PUT_ERROR(X509V3, X509V3_R_INVALID_OBJECT_IDENTIFIER);
       X509V3_conf_err(val);
-      return NULL;
+      goto err;
     }
-    ASN1_OBJECT *obj1 = OBJ_txt2obj(val->name, 0);
-    ASN1_OBJECT *obj2 = OBJ_txt2obj(val->value, 0);
-    if (!obj1 || !obj2) {
-      sk_POLICY_MAPPING_pop_free(pmaps, POLICY_MAPPING_free);
-      OPENSSL_PUT_ERROR(X509V3, X509V3_R_INVALID_OBJECT_IDENTIFIER);
-      X509V3_conf_err(val);
-      return NULL;
-    }
+
     POLICY_MAPPING *pmap = POLICY_MAPPING_new();
     if (pmap == NULL || !sk_POLICY_MAPPING_push(pmaps, pmap)) {
       POLICY_MAPPING_free(pmap);
-      sk_POLICY_MAPPING_pop_free(pmaps, POLICY_MAPPING_free);
       OPENSSL_PUT_ERROR(X509V3, ERR_R_MALLOC_FAILURE);
-      return NULL;
+      goto err;
     }
-    pmap->issuerDomainPolicy = obj1;
-    pmap->subjectDomainPolicy = obj2;
+
+    pmap->issuerDomainPolicy = OBJ_txt2obj(val->name, 0);
+    pmap->subjectDomainPolicy = OBJ_txt2obj(val->value, 0);
+    if (!pmap->issuerDomainPolicy || !pmap->subjectDomainPolicy) {
+      OPENSSL_PUT_ERROR(X509V3, X509V3_R_INVALID_OBJECT_IDENTIFIER);
+      X509V3_conf_err(val);
+      goto err;
+    }
   }
   return pmaps;
+
+err:
+  sk_POLICY_MAPPING_pop_free(pmaps, POLICY_MAPPING_free);
+  return NULL;
 }