commit 198c5f56f6573d2f8e62168044f7c584d9aec58c
author David Benjamin <davidben@google.com> Wed May 05 18:12:00 2021 -0400
committer CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> Thu May 06 18:20:08 2021 +0000
tree 325cfcf37475f6cf43a1ed0af75dd11a9a3ec364
parent e4d65564836ded46f9538dfaf25a531e17afa602

Fix a memory leak with d2i_ASN1_OBJECT object reuse.

(Imported from upstream's 65b88a75921533ada8b465bc8d5c0817ad927947 and
7c65179ad95d0f6f598ee82e763fce2567fe5802.)

Change-Id: Id6a9604231d3cacc5e20af07e40d09e20dc9d3c0
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47332
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>

crypto/asn1/a_object.c

diff --git a/crypto/asn1/a_object.c b/crypto/asn1/a_object.c
index 1557435..665e0d6 100644
--- a/crypto/asn1/a_object.c
+++ b/crypto/asn1/a_object.c
@@ -180,16 +180,13 @@
         }
     }
 
-    /*
-     * only the ASN1_OBJECTs from the 'table' will have values for ->sn or
-     * ->ln
-     */
     if ((a == NULL) || ((*a) == NULL) ||
         !((*a)->flags & ASN1_OBJECT_FLAG_DYNAMIC)) {
         if ((ret = ASN1_OBJECT_new()) == NULL)
             return (NULL);
-    } else
+    } else {
         ret = (*a);
+    }
 
     p = *pp;
     /* detach data from object */
@@ -208,12 +205,17 @@
         ret->flags |= ASN1_OBJECT_FLAG_DYNAMIC_DATA;
     }
     OPENSSL_memcpy(data, p, length);
+    /* If there are dynamic strings, free them here, and clear the flag */
+    if ((ret->flags & ASN1_OBJECT_FLAG_DYNAMIC_STRINGS) != 0) {
+        OPENSSL_free((char *)ret->sn);
+        OPENSSL_free((char *)ret->ln);
+        ret->flags &= ~ASN1_OBJECT_FLAG_DYNAMIC_STRINGS;
+    }
     /* reattach data to object, after which it remains const */
     ret->data = data;
     ret->length = length;
     ret->sn = NULL;
     ret->ln = NULL;
-    /* ret->flags=ASN1_OBJECT_FLAG_DYNAMIC; we know it is dynamic */
     p += length;
 
     if (a != NULL)

crypto/asn1/asn1_test.cc

diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc
index 90a6d7c..30d6091 100644
--- a/crypto/asn1/asn1_test.cc
+++ b/crypto/asn1/asn1_test.cc
@@ -215,6 +215,29 @@
   EXPECT_FALSE(val->value.ptr);
 }
 
+TEST(ASN1Test, ASN1ObjectReuse) {
+  // 1.2.840.113554.4.1.72585.2, an arbitrary unknown OID.
+  static const uint8_t kOID[] = {0x2a, 0x86, 0x48, 0x86, 0xf7, 0x12,
+                                 0x04, 0x01, 0x84, 0xb7, 0x09, 0x02};
+  ASN1_OBJECT *obj = ASN1_OBJECT_create(NID_undef, kOID, sizeof(kOID),
+                                        "short name", "long name");
+  ASSERT_TRUE(obj);
+
+  // OBJECT_IDENTIFIER { 1.3.101.112 }
+  static const uint8_t kDER[] = {0x06, 0x03, 0x2b, 0x65, 0x70};
+  const uint8_t *ptr = kDER;
+  EXPECT_TRUE(d2i_ASN1_OBJECT(&obj, &ptr, sizeof(kDER)));
+  EXPECT_EQ(NID_ED25519, OBJ_obj2nid(obj));
+  ASN1_OBJECT_free(obj);
+
+  // Repeat the test, this time overriding a static |ASN1_OBJECT|.
+  obj = OBJ_nid2obj(NID_rsaEncryption);
+  ptr = kDER;
+  EXPECT_TRUE(d2i_ASN1_OBJECT(&obj, &ptr, sizeof(kDER)));
+  EXPECT_EQ(NID_ED25519, OBJ_obj2nid(obj));
+  ASN1_OBJECT_free(obj);
+}
+
 // The ASN.1 macros do not work on Windows shared library builds, where usage of
 // |OPENSSL_EXPORT| is a bit stricter.
 #if !defined(OPENSSL_WINDOWS) || !defined(BORINGSSL_SHARED_LIBRARY)