From bac7e687d71b124b09ad6ad3e15be9b38c08a1ba Mon Sep 17 00:00:00 2001 From: Neil Horman Date: Tue, 2 Jan 2024 15:48:00 -0500 Subject: [PATCH] Validate config options during x509 extension creation There are several points during x509 extension creation which rely on configuration options which may have been incorrectly parsed due to invalid settings. Preform a value check for null in those locations to avoid various crashes/undefined behaviors Reviewed-by: Tomas Mraz Reviewed-by: Matt Caswell (Merged from https://github.com/openssl/openssl/pull/23183) --- crypto/x509/v3_addr.c | 4 ++++ crypto/x509/v3_asid.c | 5 +++++ crypto/x509/v3_crld.c | 5 +++++ crypto/x509/v3_ist.c | 16 ++++++++++++---- test/invalid-x509.cnf | 6 ++++++ test/recipes/25-test_x509.t | 10 +++++++++- 6 files changed, 41 insertions(+), 5 deletions(-) create mode 100644 test/invalid-x509.cnf diff --git a/crypto/x509/v3_addr.c b/crypto/x509/v3_addr.c index da9604cf96..bd937388f3 100644 --- a/crypto/x509/v3_addr.c +++ b/crypto/x509/v3_addr.c @@ -988,6 +988,10 @@ static void *v2i_IPAddrBlocks(const struct v3_ext_method *method, * the other input values. */ if (safi != NULL) { + if (val->value == NULL) { + ERR_raise(ERR_LIB_X509V3, X509V3_R_MISSING_VALUE); + goto err; + } *safi = strtoul(val->value, &t, 0); t += strspn(t, " \t"); if (*safi > 0xFF || *t++ != ':') { diff --git a/crypto/x509/v3_asid.c b/crypto/x509/v3_asid.c index 251243b723..1cb892df67 100644 --- a/crypto/x509/v3_asid.c +++ b/crypto/x509/v3_asid.c @@ -545,6 +545,11 @@ static void *v2i_ASIdentifiers(const struct v3_ext_method *method, goto err; } + if (val->value == NULL) { + ERR_raise(ERR_LIB_X509V3, X509V3_R_EXTENSION_VALUE_ERROR); + goto err; + } + /* * Handle inheritance. */ diff --git a/crypto/x509/v3_crld.c b/crypto/x509/v3_crld.c index 08df3faf86..e9f6e08e27 100644 --- a/crypto/x509/v3_crld.c +++ b/crypto/x509/v3_crld.c @@ -70,6 +70,11 @@ static int set_dist_point_name(DIST_POINT_NAME **pdp, X509V3_CTX *ctx, STACK_OF(GENERAL_NAME) *fnm = NULL; STACK_OF(X509_NAME_ENTRY) *rnm = NULL; + if (cnf->value == NULL) { + ERR_raise(ERR_LIB_X509V3, X509V3_R_MISSING_VALUE); + goto err; + } + if (HAS_PREFIX(cnf->name, "fullname")) { fnm = gnames_from_sectname(ctx, cnf->value); if (!fnm) diff --git a/crypto/x509/v3_ist.c b/crypto/x509/v3_ist.c index 978a0f3ed8..4d5fe82f32 100644 --- a/crypto/x509/v3_ist.c +++ b/crypto/x509/v3_ist.c @@ -50,25 +50,33 @@ static ISSUER_SIGN_TOOL *v2i_issuer_sign_tool(X509V3_EXT_METHOD *method, X509V3_ } if (strcmp(cnf->name, "signTool") == 0) { ist->signTool = ASN1_UTF8STRING_new(); - if (ist->signTool == NULL || !ASN1_STRING_set(ist->signTool, cnf->value, strlen(cnf->value))) { + if (ist->signTool == NULL + || cnf->value == NULL + || !ASN1_STRING_set(ist->signTool, cnf->value, strlen(cnf->value))) { ERR_raise(ERR_LIB_X509V3, ERR_R_ASN1_LIB); goto err; } } else if (strcmp(cnf->name, "cATool") == 0) { ist->cATool = ASN1_UTF8STRING_new(); - if (ist->cATool == NULL || !ASN1_STRING_set(ist->cATool, cnf->value, strlen(cnf->value))) { + if (ist->cATool == NULL + || cnf->value == NULL + || !ASN1_STRING_set(ist->cATool, cnf->value, strlen(cnf->value))) { ERR_raise(ERR_LIB_X509V3, ERR_R_ASN1_LIB); goto err; } } else if (strcmp(cnf->name, "signToolCert") == 0) { ist->signToolCert = ASN1_UTF8STRING_new(); - if (ist->signToolCert == NULL || !ASN1_STRING_set(ist->signToolCert, cnf->value, strlen(cnf->value))) { + if (ist->signToolCert == NULL + || cnf->value == NULL + || !ASN1_STRING_set(ist->signToolCert, cnf->value, strlen(cnf->value))) { ERR_raise(ERR_LIB_X509V3, ERR_R_ASN1_LIB); goto err; } } else if (strcmp(cnf->name, "cAToolCert") == 0) { ist->cAToolCert = ASN1_UTF8STRING_new(); - if (ist->cAToolCert == NULL || !ASN1_STRING_set(ist->cAToolCert, cnf->value, strlen(cnf->value))) { + if (ist->cAToolCert == NULL + || cnf->value == NULL + || !ASN1_STRING_set(ist->cAToolCert, cnf->value, strlen(cnf->value))) { ERR_raise(ERR_LIB_X509V3, ERR_R_ASN1_LIB); goto err; } diff --git a/test/invalid-x509.cnf b/test/invalid-x509.cnf new file mode 100644 index 0000000000..f982edb979 --- /dev/null +++ b/test/invalid-x509.cnf @@ -0,0 +1,6 @@ +[ext] +issuerSignTool = signTool +sbgp-autonomousSysNum = AS +issuingDistributionPoint = fullname +sbgp-ipAddrBlock = IPv4-SAFI + diff --git a/test/recipes/25-test_x509.t b/test/recipes/25-test_x509.t index 9bf011c188..9b11169a98 100644 --- a/test/recipes/25-test_x509.t +++ b/test/recipes/25-test_x509.t @@ -16,7 +16,7 @@ use OpenSSL::Test qw/:DEFAULT srctop_file/; setup("test_x509"); -plan tests => 43; +plan tests => 44; # Prevent MSys2 filename munging for arguments that look like file paths but # aren't @@ -217,6 +217,14 @@ ok(run(app(["openssl", "x509", "-in", $a_cert, "-CA", $ca_cert, # verify issuer is CA ok (get_issuer($a2_cert) =~ /CN=ca.example.com/); +my $in_csr = srctop_file('test', 'certs', 'x509-check.csr'); +my $in_key = srctop_file('test', 'certs', 'x509-check-key.pem'); +my $invextfile = srctop_file('test', 'invalid-x509.cnf'); +# Test that invalid extensions settings fail +ok(!run(app(["openssl", "x509", "-req", "-in", $in_csr, "-signkey", $in_key, + "-out", "/dev/null", "-days", "3650" , "-extensions", "ext", + "-extfile", $invextfile]))); + # Tests for issue #16080 (fixed in 1.1.1o) my $b_key = "b-key.pem"; my $b_csr = "b-cert.csr";