From 9ac29bc8579dd9632bd50900675ff7900f5f7ef1 Mon Sep 17 00:00:00 2001 From: herbenderbler Date: Mon, 16 Mar 2026 11:14:52 -0600 Subject: [PATCH] Fix memory leak in load_key_certs_crls() when add/push fails When X509_add_cert() or sk_X509_CRL_push() failed, the cert or CRL from OSSL_STORE was not freed. Free on failure to avoid a leak. Fix 90-test_memfail.t parsing of count output so the memfail suite runs correctly: parse 'skip: N count M' with a regex (handles '# ' prefix), return (0,0) if the count file cannot be opened, and skip with a clear message when total malloc count is 0 instead of planning 0 tests. Apply clang-format to test/load_key_certs_crls_memfail.c. - apps/lib/apps.c: free cert/CRL on add/push failure - test/build.info: add load_key_certs_crls_memfail (allocfail-tests) - test/load_key_certs_crls_memfail.c: regression test for issue #30364 - test/recipes/90-test_memfail.t: fix get_count_info parsing and plan Issue #30364 Fixes: 6d382c74b375 "Use OSSL_STORE for load_{,pub}key() and load_cert() in apps/lib/apps.c" Fixes: d7fcee3b3b5fa "OSSL_HTTP_parse_url(): add optional port number return parameter and strengthen documentation" Reviewed-by: Eugene Syromiatnikov Reviewed-by: Nikola Pajkovsky MergeDate: Tue Apr 21 08:50:18 2026 (Merged from https://github.com/openssl/openssl/pull/30428) --- apps/lib/apps.c | 15 +++-- test/build.info | 9 ++- test/load_key_certs_crls_memfail.c | 95 ++++++++++++++++++++++++++++++ test/recipes/90-test_memfail.t | 33 ++++++----- 4 files changed, 133 insertions(+), 19 deletions(-) create mode 100644 test/load_key_certs_crls_memfail.c diff --git a/apps/lib/apps.c b/apps/lib/apps.c index 986f5954f4a..ce4fa9d3836 100644 --- a/apps/lib/apps.c +++ b/apps/lib/apps.c @@ -1057,9 +1057,12 @@ int load_key_certs_crls(const char *uri, int format, int maybe_stdin, if (ok) pcert = NULL; } else if (pcerts != NULL) { - ok = X509_add_cert(*pcerts, - OSSL_STORE_INFO_get1_CERT(info), - X509_ADD_FLAG_DEFAULT); + X509 *cert = OSSL_STORE_INFO_get1_CERT(info); + + ok = cert != NULL + && X509_add_cert(*pcerts, cert, X509_ADD_FLAG_DEFAULT); + if (!ok) + X509_free(cert); } ncerts += ok; break; @@ -1069,7 +1072,11 @@ int load_key_certs_crls(const char *uri, int format, int maybe_stdin, if (ok) pcrl = NULL; } else if (pcrls != NULL) { - ok = sk_X509_CRL_push(*pcrls, OSSL_STORE_INFO_get1_CRL(info)); + X509_CRL *crl = OSSL_STORE_INFO_get1_CRL(info); + + ok = crl != NULL && sk_X509_CRL_push(*pcrls, crl); + if (!ok) + X509_CRL_free(crl); } ncrls += ok; break; diff --git a/test/build.info b/test/build.info index 742737d8736..6255b93e404 100644 --- a/test/build.info +++ b/test/build.info @@ -82,7 +82,7 @@ IF[{- !$disabled{tests} -}] ENDIF IF[{- !$disabled{'allocfail-tests'} -}] - PROGRAMS{noninst}=handshake-memfail x509-memfail + PROGRAMS{noinst}=handshake-memfail x509-memfail load_key_certs_crls_memfail ENDIF IF[{- !$disabled{quic} -}] @@ -621,6 +621,13 @@ IF[{- !$disabled{tests} -}] INCLUDE[x509-memfail]=../include ../apps/include DEPEND[x509-memfail]=../libcrypto.a libtestutil.a + SOURCE[load_key_certs_crls_memfail]=load_key_certs_crls_memfail.c ../apps/lib/apps.c \ + ../apps/lib/app_rand.c ../apps/lib/app_provider.c ../apps/lib/app_libctx.c \ + ../apps/lib/fmt.c ../apps/lib/apps_ui.c ../apps/lib/app_x509.c \ + ../crypto/asn1/a_time.c ../crypto/ctype.c + INCLUDE[load_key_certs_crls_memfail]=.. ../include ../apps/include + DEPEND[load_key_certs_crls_memfail]=libtestutil.a ../libcrypto.a ../libssl.a + SOURCE[ssl_handshake_rtt_test]=ssl_handshake_rtt_test.c helpers/ssltestlib.c INCLUDE[ssl_handshake_rtt_test]=../include ../apps/include .. DEPEND[ssl_handshake_rtt_test]=../libcrypto.a ../libssl.a libtestutil.a diff --git a/test/load_key_certs_crls_memfail.c b/test/load_key_certs_crls_memfail.c new file mode 100644 index 00000000000..94297c00193 --- /dev/null +++ b/test/load_key_certs_crls_memfail.c @@ -0,0 +1,95 @@ +/* + * Copyright 2025 The OpenSSL Project Authors. All Rights Reserved. + * + * Licensed under the Apache License 2.0 (the "License"); you may not use + * this file except in compliance with the License. You may obtain a copy + * in the file LICENSE in the source distribution or at + * https://www.openssl.org/source/license.html + * + * Regression test for issue #30364: memory leak in load_key_certs_crls() + * when X509_add_cert() or sk_X509_CRL_push() fails. Exercises the add/push + * path under OPENSSL_MALLOC_FAILURES so that with the fix the cert/CRL is + * freed on failure (memory_sanitizer would report a leak without the fix). + */ + +#include +#include + +#include +#include +#include "apps.h" +#include "app_libctx.h" +#include "testutil.h" + +char *default_config_file = NULL; + +static char *certfile = NULL; +static int mcount, rcount, fcount, scount; + +static int do_load_key_certs_crls(int allow_failure) +{ + STACK_OF(X509) *certs = NULL; + int ret = (allow_failure == 1) ? 0 : 1; + char uri[1024]; + + if (certfile == NULL) + return 0; + + (void)snprintf(uri, sizeof(uri), "file:%s", certfile); + if (!TEST_true(load_key_certs_crls(uri, FORMAT_UNDEF, 0, NULL, "cert", + 1, NULL, NULL, NULL, NULL, &certs, + NULL, NULL, NULL))) + goto err; + + ret = 1; +err: + sk_X509_pop_free(certs, X509_free); + return ret; +} + +static int test_record_alloc_counts(void) +{ + return do_load_key_certs_crls(1); +} + +static int test_alloc_failures(void) +{ + return do_load_key_certs_crls(0); +} + +static int test_report_alloc_counts(void) +{ + CRYPTO_get_alloc_counts(&mcount, &rcount, &fcount); + TEST_info("skip: %d count %d\n", scount, mcount - scount); + return 1; +} + +int setup_tests(void) +{ + int ret = 0; + char *opmode = NULL; + + if (app_create_libctx() == NULL) + return 0; + + if (!TEST_ptr(opmode = test_get_argument(0))) + goto err; + + if (!TEST_ptr(certfile = test_get_argument(1))) + goto err; + + if (strcmp(opmode, "count") == 0) { + CRYPTO_get_alloc_counts(&scount, &rcount, &fcount); + ADD_TEST(test_record_alloc_counts); + ADD_TEST(test_report_alloc_counts); + } else { + ADD_TEST(test_alloc_failures); + } + ret = 1; +err: + return ret; +} + +void cleanup_tests(void) +{ +} diff --git a/test/recipes/90-test_memfail.t b/test/recipes/90-test_memfail.t index fd168fe45ab..d8bf1e7a4b9 100644 --- a/test/recipes/90-test_memfail.t +++ b/test/recipes/90-test_memfail.t @@ -30,28 +30,25 @@ run(test(["handshake-memfail", "count", srctop_dir("test", "certs")], stderr => run(test(["x509-memfail", "count", srctop_file("test", "certs", "servercert.pem")], stderr => "$resultdir/x509countinfo.txt")); +run(test(["load_key_certs_crls_memfail", "count", srctop_file("test", "certs", "servercert.pem")], stderr => "$resultdir/load_key_certs_crls_countinfo.txt")); + sub get_count_info { my ($infile) = @_; - my @vals; + my ($skipcount, $malloccount) = (0, 0); - # Read in our input file - open my $handle, '<', "$infile"; + open my $handle, '<', "$infile" or return (0, 0); chomp(my @lines = <$handle>); close $handle; - # parse the input file - foreach(@lines) { - if ($_ =~/skip:/) { - @vals = split ' ', $_; + # Match the test program output: "skip: count " + # Stderr may be captured with a "# " prefix per line (TAP-style). + foreach (@lines) { + if (/\bskip:\s*(\d+)\s+count\s+(\d+)/) { + $skipcount = $1; + $malloccount = $2; last; } } - # - #The number of allocations we skip is in argument 2 - #The number of mallocs we should test is in argument 4 - # - my $skipcount = $vals[2]; - my $malloccount = $vals[4]; return ($skipcount, $malloccount); } @@ -59,11 +56,17 @@ my ($hsskipcount, $hsmalloccount) = get_count_info("$resultdir/hscountinfo.txt") my ($x509skipcount, $x509malloccount) = get_count_info("$resultdir/x509countinfo.txt"); +my ($load_key_certs_crls_skipcount, $load_key_certs_crls_malloccount) = get_count_info("$resultdir/load_key_certs_crls_countinfo.txt"); + +my $total_malloccount = $hsmalloccount + $x509malloccount + $load_key_certs_crls_malloccount; +plan skip_all => "could not get malloc counts (one or more count runs failed or output format changed)" + if $total_malloccount == 0; + # # Now we can plan our tests. We plan to run malloccount iterations of this # test # -plan tests => $hsmalloccount + $x509malloccount; +plan tests => $total_malloccount; sub run_memfail_test { my $skipcount = $_[0]; @@ -88,3 +91,5 @@ run_memfail_test($hsskipcount, $hsmalloccount, ["handshake-memfail", "run", srct run_memfail_test($x509skipcount, $x509malloccount, ["x509-memfail", "run", srctop_file("test", "certs", "servercert.pem")]); +run_memfail_test($load_key_certs_crls_skipcount, $load_key_certs_crls_malloccount, ["load_key_certs_crls_memfail", "run", srctop_file("test", "certs", "servercert.pem")]); + -- 2.47.3