From a552c23c6502592c1b3c67d93dd7e5ffbe958aa4 Mon Sep 17 00:00:00 2001 From: Neil Horman Date: Tue, 5 Dec 2023 15:24:20 -0500 Subject: [PATCH] Harden asn1 oid loader to invalid inputs In the event that a config file contains this sequence: ======= openssl_conf = openssl_init config_diagnostics = 1 [openssl_init] oid_section = oids [oids] testoid1 = 1.2.3.4.1 testoid2 = A Very Long OID Name, 1.2.3.4.2 testoid3 = ,1.2.3.4.3 ====== The leading comma in testoid3 can cause a heap buffer overflow, as the parsing code will move the string pointer back 1 character, thereby pointing to an invalid memory space correct the parser to detect this condition and handle it by treating it as if the comma doesn't exist (i.e. an empty long oid name) Reviewed-by: Hugo Landau Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/22957) --- apps/asn1parse.c | 2 +- crypto/asn1/asn_moid.c | 4 ++++ test/recipes/04-test_asn1_parse.t | 26 ++++++++++++++++++++++++++ test/test_asn1_parse.cnf | 12 ++++++++++++ 4 files changed, 43 insertions(+), 1 deletion(-) create mode 100644 test/recipes/04-test_asn1_parse.t create mode 100644 test/test_asn1_parse.cnf diff --git a/apps/asn1parse.c b/apps/asn1parse.c index 6597a6180b..bf62f85947 100644 --- a/apps/asn1parse.c +++ b/apps/asn1parse.c @@ -178,7 +178,7 @@ int asn1parse_main(int argc, char **argv) if ((buf = BUF_MEM_new()) == NULL) goto end; - if (genstr == NULL && informat == FORMAT_PEM) { + if (genconf == NULL && genstr == NULL && informat == FORMAT_PEM) { if (PEM_read_bio(in, &name, &header, &str, &num) != 1) { BIO_printf(bio_err, "Error reading PEM file\n"); ERR_print_errors(bio_err); diff --git a/crypto/asn1/asn_moid.c b/crypto/asn1/asn_moid.c index 6f816307af..1e183f4f18 100644 --- a/crypto/asn1/asn_moid.c +++ b/crypto/asn1/asn_moid.c @@ -67,6 +67,10 @@ static int do_create(const char *value, const char *name) if (p == NULL) { ln = name; ostr = value; + } else if (p == value) { + /* we started with a leading comma */ + ln = name; + ostr = p + 1; } else { ln = value; ostr = p + 1; diff --git a/test/recipes/04-test_asn1_parse.t b/test/recipes/04-test_asn1_parse.t new file mode 100644 index 0000000000..f3af436592 --- /dev/null +++ b/test/recipes/04-test_asn1_parse.t @@ -0,0 +1,26 @@ +#! /usr/bin/env perl +# Copyright 2023 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 can obtain a copy +# in the file LICENSE in the source distribution or at +# https://www.openssl.org/source/license.html + +use strict; +use OpenSSL::Test qw(:DEFAULT srctop_file); +use OpenSSL::Test::Utils; + +setup("test_asn1_parse"); + +plan tests => 3; + +$ENV{OPENSSL_CONF} = srctop_file("test", "test_asn1_parse.cnf"); + +ok(run(app(([ 'openssl', 'asn1parse', + '-genstr', 'OID:1.2.3.4.1'])))); + +ok(run(app(([ 'openssl', 'asn1parse', + '-genstr', 'OID:1.2.3.4.2'])))); + +ok(run(app(([ 'openssl', 'asn1parse', + '-genstr', 'OID:1.2.3.4.3'])))); diff --git a/test/test_asn1_parse.cnf b/test/test_asn1_parse.cnf new file mode 100644 index 0000000000..5f0305657e --- /dev/null +++ b/test/test_asn1_parse.cnf @@ -0,0 +1,12 @@ +openssl_conf = openssl_init + +# Comment out the next line to ignore configuration errors +config_diagnostics = 1 + +[openssl_init] +oid_section = oids + +[oids] +testoid1 = 1.2.3.4.1 +testoid2 = A Very Long OID Name, 1.2.3.4.2 +testoid3 = ,1.2.3.4.3