Fix error handling in OBJ_add_object

This fixes the possible memory leak in OBJ_add_object
when a pre-existing object is replaced by a new one,
with identical NID, OID, and/or short/long name.
We do not try to delete any orphans, but only mark
them as type == -1, because the previously returned
pointers from OBJ_nid2obj/OBJ_nid2sn/OBJ_nid2ln
may be cached by applications and can thus not
be cleaned up before the application terminates.

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/22534)

diff --git a/crypto/objects/obj_dat.c b/crypto/objects/obj_dat.c
index 493b0e11de44f5fd948ae07450f8242d577b56f2..8bc4fa8ec2d530fcfd02e8f274882b31d48f7a0a 100644 (file)
--- a/crypto/objects/obj_dat.c
+++ b/crypto/objects/obj_dat.c
@@ -263,7 +263,7 @@ int OBJ_new_nid(int num)
 static int ossl_obj_add_object(const ASN1_OBJECT *obj, int lock)
 {
     ASN1_OBJECT *o = NULL;
-    ADDED_OBJ *ao[4] = { NULL, NULL, NULL, NULL }, *aop;
+    ADDED_OBJ *ao[4] = { NULL, NULL, NULL, NULL }, *aop[4];
     int i;
 
     if ((o = OBJ_dup(obj)) == NULL)
@@ -294,9 +294,21 @@ static int ossl_obj_add_object(const ASN1_OBJECT *obj, int lock)
         if (ao[i] != NULL) {
             ao[i]->type = i;
             ao[i]->obj = o;
-            aop = lh_ADDED_OBJ_insert(added, ao[i]);
-            /* memory leak, but should not normally matter */
-            OPENSSL_free(aop);
+            aop[i] = lh_ADDED_OBJ_retrieve(added, ao[i]);
+            if (aop[i] != NULL)
+                aop[i]->type = -1;
+            (void)lh_ADDED_OBJ_insert(added, ao[i]);
+            if (lh_ADDED_OBJ_error(added)) {
+                if (aop[i] != NULL)
+                    aop[i]->type = i;
+                while (i-- > ADDED_DATA) {
+                    lh_ADDED_OBJ_delete(added, ao[i]);
+                    if (aop[i] != NULL)
+                        aop[i]->type = i;
+                }
+                ERR_raise(ERR_LIB_OBJ, ERR_R_CRYPTO_LIB);
+                goto err;
+            }
         }
     }
     o->flags &=

diff --git a/test/evp_extra_test.c b/test/evp_extra_test.c
index 1b9b440d8c558151a5969b55e8fb3ff3756b4f64..af19b45239667da44441bc5281a85b25e37d3f25 100644 (file)
--- a/test/evp_extra_test.c
+++ b/test/evp_extra_test.c
@@ -4944,6 +4944,7 @@ static int custom_md_cleanup(EVP_MD_CTX *ctx)
 
 static int test_custom_md_meth(void)
 {
+    ASN1_OBJECT *o = NULL;
     EVP_MD_CTX *mdctx = NULL;
     EVP_MD *tmp = NULL;
     char mess[] = "Test Message\n";
@@ -4989,8 +4990,21 @@ static int test_custom_md_meth(void)
             || !TEST_int_eq(custom_md_cleanup_called, 1))
         goto err;
 
+    if (!TEST_int_eq(OBJ_create("1.3.6.1.4.1.16604.998866.1",
+                                "custom-md", "custom-md"), NID_undef)
+            || !TEST_int_eq(ERR_GET_LIB(ERR_peek_error()), ERR_LIB_OBJ)
+            || !TEST_int_eq(ERR_GET_REASON(ERR_get_error()), OBJ_R_OID_EXISTS))
+        goto err;
+
+    o = ASN1_OBJECT_create(nid, (unsigned char *)
+                                "\53\6\1\4\1\201\201\134\274\373\122\1", 12,
+                                "custom-md", "custom-md");
+    if (!TEST_int_eq(OBJ_add_object(o), nid))
+        goto err;
+
     testresult = 1;
  err:
+    ASN1_OBJECT_free(o);
     EVP_MD_CTX_free(mdctx);
     EVP_MD_meth_free(tmp);
     return testresult;