Check returns of sk_POLICY_MAPPING_push, sk_GENERAL_NAME_push, sk_ACC…

…ESS_DESCRIPTION_push, sk_X509_push, sk_X509_NAME_push, sk_OPENSSL_CSTRING_push, sk_SCT_push, sk_DIST_POINT_push, sk_OSSL_CMP_CRLSTATUS_push, sk_ASN1_UTF8STRING_push and sk_ASN1_OBJECT_push and handle appropriately.
fwh-dc · Dec 21, 2024 · c8f57d0 · c8f57d0
1 parent d992e87
commit c8f57d0
Show file tree

Hide file tree

Showing 20 changed files with 81 additions and 38 deletions.
diff --git a/apps/cmp.c b/apps/cmp.c
@@ -1975,8 +1975,8 @@ static int add_certProfile(OSSL_CMP_CTX *ctx, const char *name)
         goto err;
     }
     /* Due to sk_ASN1_UTF8STRING_new_reserve(NULL, 1), this surely succeeds: */
-    (void)sk_ASN1_UTF8STRING_push(sk, utf8string);
-    if ((itav = OSSL_CMP_ITAV_new0_certProfile(sk)) == NULL)
+    if (!ossl_assert(sk_ASN1_UTF8STRING_push(sk, utf8string))
+        || (itav = OSSL_CMP_ITAV_new0_certProfile(sk)) == NULL)
         goto err;
     if (OSSL_CMP_CTX_push0_geninfo_ITAV(ctx, itav))
         return 1;

diff --git a/apps/crl2pkcs7.c b/apps/crl2pkcs7.c
@@ -216,7 +216,10 @@ static int add_certs_from_file(STACK_OF(X509) *stack, char *certfile)
     while (sk_X509_INFO_num(sk)) {
         xi = sk_X509_INFO_shift(sk);
         if (xi->x509 != NULL) {
-            sk_X509_push(stack, xi->x509);
+            if (!sk_X509_push(stack, xi->x509)) {
+                X509_INFO_free(xi);
+                goto end;
+            }
             xi->x509 = NULL;
             count++;
         }

diff --git a/apps/engine.c b/apps/engine.c
@@ -316,7 +316,8 @@ int engine_main(int argc, char **argv)
      * names, and then setup to parse the rest of the line as flags. */
     prog = argv[0];
     while ((argv1 = argv[1]) != NULL && *argv1 != '-') {
-        sk_OPENSSL_CSTRING_push(engines, argv1);
+        if (!sk_OPENSSL_CSTRING_push(engines, argv1))
+            goto end;
         argc--;
         argv++;
     }
@@ -370,12 +371,14 @@ int engine_main(int argc, char **argv)
             BIO_printf(bio_err, "%s: Use -help for summary.\n", prog);
             goto end;
         }
-        sk_OPENSSL_CSTRING_push(engines, *argv);
+        if (!sk_OPENSSL_CSTRING_push(engines, *argv))
+            goto end;
     }
 
     if (sk_OPENSSL_CSTRING_num(engines) == 0) {
         for (e = ENGINE_get_first(); e != NULL; e = ENGINE_get_next(e)) {
-            sk_OPENSSL_CSTRING_push(engines, ENGINE_get_id(e));
+            if (!sk_OPENSSL_CSTRING_push(engines, ENGINE_get_id(e)))
+                goto end;
         }
     }
 

diff --git a/apps/lib/names.c b/apps/lib/names.c
@@ -22,6 +22,7 @@ void collect_names(const char *name, void *vdata)
 {
     STACK_OF(OPENSSL_CSTRING) *names = vdata;
 
+    /* TODO: Handle a failure? */
     sk_OPENSSL_CSTRING_push(names, name);
 }
 

diff --git a/apps/x509.c b/apps/x509.c
@@ -453,7 +453,8 @@ int x509_main(int argc, char **argv)
                            prog, opt_arg());
                 goto opthelp;
             }
-            sk_ASN1_OBJECT_push(trust, objtmp);
+            if (!sk_ASN1_OBJECT_push(trust, objtmp))
+                goto end;
             trustout = 1;
             break;
         case OPT_ADDREJECT:
@@ -464,7 +465,8 @@ int x509_main(int argc, char **argv)
                            prog, opt_arg());
                 goto opthelp;
             }
-            sk_ASN1_OBJECT_push(reject, objtmp);
+            if (!sk_ASN1_OBJECT_push(trust, objtmp))
+                goto end;
             trustout = 1;
             break;
         case OPT_SETALIAS:

diff --git a/crypto/cmp/cmp_asn.c b/crypto/cmp/cmp_asn.c
@@ -665,11 +665,12 @@ static GENERAL_NAMES *gennames_new(const X509_NAME *nm)
 
     if ((names = sk_GENERAL_NAME_new_reserve(NULL, 1)) == NULL)
         return NULL;
-    if (!GENERAL_NAME_set1_X509_NAME(&name, nm)) {
+    if (!GENERAL_NAME_set1_X509_NAME(&name, nm)
+            /* sk_GENERAL_NAME_push() cannot fail */
+        || !ossl_assert(sk_GENERAL_NAME_push(names, name);)) {
         sk_GENERAL_NAME_free(names);
         return NULL;
     }
-    (void)sk_GENERAL_NAME_push(names, name); /* cannot fail */
     return names;
 }
 

diff --git a/crypto/cmp/cmp_genm.c b/crypto/cmp/cmp_genm.c
@@ -371,9 +371,9 @@ int OSSL_CMP_get1_crlUpdate(OSSL_CMP_CTX *ctx, const X509 *crlcert,
         ERR_raise(ERR_LIB_CMP, CMP_R_GENERATE_CRLSTATUS);
         goto end;
     }
-    (void)sk_OSSL_CMP_CRLSTATUS_push(list, status); /* cannot fail */
 
-    if ((req = OSSL_CMP_ITAV_new0_crlStatusList(list)) == NULL)
+    if (!ossl_assert(sk_OSSL_CMP_CRLSTATUS_push(list, status)) /* cannot fail */
+        || (req = OSSL_CMP_ITAV_new0_crlStatusList(list)) == NULL)
         goto end;
     status = NULL;
     list = NULL;

diff --git a/crypto/conf/conf_lib.c b/crypto/conf/conf_lib.c
@@ -228,6 +228,7 @@ static void collect_section_name(const CONF_VALUE *v, SECTION_NAMES *names)
 {
     /* A section is a CONF_VALUE with name == NULL */
     if (v->name == NULL)
+        /* TODO: Handle a failure? */
         sk_OPENSSL_CSTRING_push(names, v->section);
 }
 

diff --git a/crypto/ocsp/ocsp_ext.c b/crypto/ocsp/ocsp_ext.c
@@ -400,7 +400,8 @@ X509_EXTENSION *OCSP_accept_responses_new(char **oids)
         goto err;
     while (oids && *oids) {
         if ((nid = OBJ_txt2nid(*oids)) != NID_undef && (o = OBJ_nid2obj(nid)))
-            sk_ASN1_OBJECT_push(sk, o);
+            if (!sk_ASN1_OBJECT_push(sk, o))
+                goto err;
         oids++;
     }
     x = X509V3_EXT_i2d(NID_id_pkix_OCSP_acceptableResponses, 0, sk);

diff --git a/crypto/x509/v3_crld.c b/crypto/x509/v3_crld.c
@@ -264,9 +264,10 @@ static void *v2i_crld(const X509V3_EXT_METHOD *method,
                 goto err;
             point = crldp_from_section(ctx, dpsect);
             X509V3_section_free(ctx, dpsect);
-            if (point == NULL)
+            if (point == NULL
+                   /* no failure as it was reserved */
+                || !ossl_assert(sk_DIST_POINT_push(crld, point)))
                 goto err;
-            sk_DIST_POINT_push(crld, point); /* no failure as it was reserved */
         } else {
             if ((gen = v2i_GENERAL_NAME(method, ctx, cnf)) == NULL)
                 goto err;
@@ -279,11 +280,13 @@ static void *v2i_crld(const X509V3_EXT_METHOD *method,
                 goto err;
             }
             gen = NULL;
-            if ((point = DIST_POINT_new()) == NULL) {
+            if ((point = DIST_POINT_new()) == NULL
+                /* no failure as it was reserved */
+                || !ossl_assert(sk_DIST_POINT_push(crld, point))) {
+                DIST_POINT_free(point);
                 ERR_raise(ERR_LIB_X509V3, ERR_R_ASN1_LIB);
                 goto err;
             }
-            sk_DIST_POINT_push(crld, point); /* no failure as it was reserved */
             if ((point->distpoint = DIST_POINT_NAME_new()) == NULL) {
                 ERR_raise(ERR_LIB_X509V3, ERR_R_ASN1_LIB);
                 goto err;

diff --git a/crypto/x509/v3_extku.c b/crypto/x509/v3_extku.c
@@ -120,7 +120,11 @@ static void *v2i_EXTENDED_KEY_USAGE(const X509V3_EXT_METHOD *method,
                            "%s", extval);
             return NULL;
         }
-        sk_ASN1_OBJECT_push(extku, objtmp);  /* no failure as it was reserved */
+        if (!ossl_assert(sk_ASN1_OBJECT_push(extku, objtmp))) {
+            sk_ASN1_OBJECT_pop_free(extku, ASN1_OBJECT_free);
+            ERR_raise(ERR_LIB_X509V3, ERR_R_CRYPTO_LIB);
+            return NULL;
+        }
     }
     return extku;
 }
diff --git a/crypto/x509/v3_info.c b/crypto/x509/v3_info.c
@@ -121,7 +121,9 @@ static AUTHORITY_INFO_ACCESS *v2i_AUTHORITY_INFO_ACCESS(X509V3_EXT_METHOD
             ERR_raise(ERR_LIB_X509V3, ERR_R_ASN1_LIB);
             goto err;
         }
-        sk_ACCESS_DESCRIPTION_push(ainfo, acc); /* Cannot fail due to reserve */
+        /* Cannot fail due to reserve */
+        if (!ossl_assert(sk_ACCESS_DESCRIPTION_push(ainfo, acc)))
+            goto err;
         ptmp = strchr(cnf->name, ';');
         if (ptmp == NULL) {
             ERR_raise(ERR_LIB_X509V3, X509V3_R_INVALID_SYNTAX);

diff --git a/crypto/x509/v3_pmaps.c b/crypto/x509/v3_pmaps.c
@@ -99,7 +99,11 @@ static void *v2i_POLICY_MAPPINGS(const X509V3_EXT_METHOD *method,
         pmap->issuerDomainPolicy = obj1;
         pmap->subjectDomainPolicy = obj2;
         obj1 = obj2 = NULL;
-        sk_POLICY_MAPPING_push(pmaps, pmap); /* no failure as it was reserved */
+        /* no failure as it was reserved */
+        if (!ossl_assert(sk_POLICY_MAPPING_push(pmaps, pmap))) {
+            POLICY_MAPPING_free(pmap);
+            goto err;
+        }
     }
     return pmaps;
  err:

diff --git a/crypto/x509/v3_san.c b/crypto/x509/v3_san.c
@@ -320,9 +320,10 @@ static GENERAL_NAMES *v2i_issuer_alt(X509V3_EXT_METHOD *method,
         } else {
             GENERAL_NAME *gen = v2i_GENERAL_NAME(method, ctx, cnf);
 
-            if (gen == NULL)
+            if (gen == NULL
+                /* no failure as it was reserved */
+                || ! ossl_assert(sk_GENERAL_NAME_push(gens, gen)))
                 goto err;
-            sk_GENERAL_NAME_push(gens, gen); /* no failure as it was reserved */
         }
     }
     return gens;
@@ -363,7 +364,9 @@ static int copy_issuer(X509V3_CTX *ctx, GENERAL_NAMES *gens)
 
     for (i = 0; i < num; i++) {
         gen = sk_GENERAL_NAME_value(ialt, i);
-        sk_GENERAL_NAME_push(gens, gen);     /* no failure as it was reserved */
+        /* no failure as it was reserved */
+        if (!ossl_assert(sk_GENERAL_NAME_push(gens, gen)))
+            goto err;
     }
     sk_GENERAL_NAME_free(ialt);
 
@@ -402,9 +405,10 @@ static GENERAL_NAMES *v2i_subject_alt(X509V3_EXT_METHOD *method,
                 goto err;
         } else {
             GENERAL_NAME *gen;
-            if ((gen = v2i_GENERAL_NAME(method, ctx, cnf)) == NULL)
+            if ((gen = v2i_GENERAL_NAME(method, ctx, cnf)) == NULL
+                   /* no failure as it was reserved */
+                || !ossl_assert(sk_GENERAL_NAME_push(gens, gen)))
                 goto err;
-            sk_GENERAL_NAME_push(gens, gen); /* no failure as it was reserved */
         }
     }
     return gens;
@@ -487,9 +491,10 @@ GENERAL_NAMES *v2i_GENERAL_NAMES(const X509V3_EXT_METHOD *method,
 
     for (i = 0; i < num; i++) {
         cnf = sk_CONF_VALUE_value(nval, i);
-        if ((gen = v2i_GENERAL_NAME(method, ctx, cnf)) == NULL)
+        if ((gen = v2i_GENERAL_NAME(method, ctx, cnf)) == NULL
+               /* no failure as it was reserved */
+            || !ossl_assert(sk_GENERAL_NAME_push(gens, gen)))
             goto err;
-        sk_GENERAL_NAME_push(gens, gen);    /* no failure as it was reserved */
     }
     return gens;
  err:

diff --git a/engines/e_capi.c b/engines/e_capi.c
@@ -1771,7 +1771,10 @@ static int capi_load_ssl_client_cert(ENGINE *e, SSL *ssl,
             if (!certs)
                 certs = sk_X509_new_null();
 
-            sk_X509_push(certs, x);
+            if (!sk_X509_push(certs, x)){
+                X509_free(x);
+                continue;
+            }
         } else {
             X509_free(x);
         }

diff --git a/fuzz/x509.c b/fuzz/x509.c
@@ -115,12 +115,11 @@ int FuzzerTestOneInput(const uint8_t *buf, size_t len)
         ASN1_GENERALIZEDTIME *revtime, *thisupd, *nextupd;
 
         certs = sk_X509_new_null();
-        if (certs == NULL)
+        if (certs == NULL
+            || !sk_X509_push(certs, x509_1)
+            || !sk_X509_push(certs, x509_2))
             goto err;
 
-        sk_X509_push(certs, x509_1);
-        sk_X509_push(certs, x509_2);
-
         OCSP_basic_verify(bs, certs, store, OCSP_PARTIAL_CHAIN);
 
         id = OCSP_cert_to_id(NULL, x509_1, x509_2);

diff --git a/ssl/ssl_cert.c b/ssl/ssl_cert.c
@@ -566,12 +566,14 @@ STACK_OF(X509_NAME) *SSL_dup_CA_list(const STACK_OF(X509_NAME) *sk)
     }
     for (i = 0; i < num; i++) {
         name = X509_NAME_dup(sk_X509_NAME_value(sk, i));
-        if (name == NULL) {
+        if (name == NULL
+                /* sk_X509_NAME_push() cannot fail after reserve call */
+                || !ossl_assert(sk_X509_NAME_push(ret, name))) {
             ERR_raise(ERR_LIB_SSL, ERR_R_X509_LIB);
             sk_X509_NAME_pop_free(ret, X509_NAME_free);
+            X509_NAME_free(name);
             return NULL;
         }
-        sk_X509_NAME_push(ret, name);   /* Cannot fail after reserve call */
     }
     return ret;
 }

diff --git a/test/cmp_client_test.c b/test/cmp_client_test.c
@@ -187,8 +187,11 @@ static int test_exec_IR_ses(void)
     fixture->req_type = OSSL_CMP_PKIBODY_IR;
     fixture->expected = OSSL_CMP_PKISTATUS_accepted;
     fixture->caPubs = sk_X509_new_null();
-    sk_X509_push(fixture->caPubs, server_cert);
-    sk_X509_push(fixture->caPubs, server_cert);
+    if (!sk_X509_push(fixture->caPubs, server_cert)
+        || !sk_X509_push(fixture->caPubs, server_cert)) {
+        tear_down(fixture);
+        return 0;
+    }
     ossl_cmp_mock_srv_set1_caPubsOut(fixture->srv_ctx, fixture->caPubs);
     EXECUTE_TEST(execute_exec_certrequest_ses_test, tear_down);
     return result;

diff --git a/test/ct_test.c b/test/ct_test.c
@@ -463,7 +463,12 @@ static int test_encode_tls_sct(void)
         return 0;
     }
 
-    sk_SCT_push(fixture->sct_list, sct);
+    if (!sk_SCT_push(fixture->sct_list, sct))
+    {
+        tear_down(fixture);
+        return 0;
+    }
+
     fixture->sct_dir = ct_dir;
     fixture->sct_text_file = "tls1.sct";
     EXECUTE_CT_TEST();

diff --git a/test/v3nametest.c b/test/v3nametest.c
@@ -157,7 +157,8 @@ static int set_altname(X509 *crt, ...)
         default:
             abort();
         }
-        sk_GENERAL_NAME_push(gens, gen);
+        if (!sk_GENERAL_NAME_push(gens, gen))
+            goto out;
         gen = NULL;
     }
     if (!X509_add1_ext_i2d(crt, NID_subject_alt_name, gens, 0, 0))
-Original file line number
+Diff line change
@@ Expand Up / @@ -22,6 +22,7 @@ void collect_names(const char *name, void *vdata) @@
     {
         STACK_OF(OPENSSL_CSTRING) *names = vdata;
+        /* TODO: Handle a failure? */
         sk_OPENSSL_CSTRING_push(names, name);
     }
@@ Expand Down @@