You can not select more than 25 topics
Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
187 lines
6.8 KiB
187 lines
6.8 KiB
From 8818064ce3c3c0f1b740a5aaba2a987e75bfbafd Mon Sep 17 00:00:00 2001 |
|
From: Matt Caswell <matt@openssl.org> |
|
Date: Wed, 14 Dec 2022 16:18:14 +0000 |
|
Subject: [PATCH 06/18] Fix a UAF resulting from a bug in BIO_new_NDEF |
|
|
|
If the aux->asn1_cb() call fails in BIO_new_NDEF then the "out" BIO will |
|
be part of an invalid BIO chain. This causes a "use after free" when the |
|
BIO is eventually freed. |
|
|
|
Based on an original patch by Viktor Dukhovni and an idea from Theo |
|
Buehler. |
|
|
|
Thanks to Octavio Galland for reporting this issue. |
|
|
|
Reviewed-by: Paul Dale <pauli@openssl.org> |
|
Reviewed-by: Tomas Mraz <tomas@openssl.org> |
|
--- |
|
crypto/asn1/bio_ndef.c | 40 ++++++++++++++++++++++++++++++++-------- |
|
1 file changed, 32 insertions(+), 8 deletions(-) |
|
|
|
diff --git a/crypto/asn1/bio_ndef.c b/crypto/asn1/bio_ndef.c |
|
index d94e3a3644..b9df3a7a47 100644 |
|
--- a/crypto/asn1/bio_ndef.c |
|
+++ b/crypto/asn1/bio_ndef.c |
|
@@ -49,13 +49,19 @@ static int ndef_suffix(BIO *b, unsigned char **pbuf, int *plen, void *parg); |
|
static int ndef_suffix_free(BIO *b, unsigned char **pbuf, int *plen, |
|
void *parg); |
|
|
|
-/* unfortunately cannot constify this due to CMS_stream() and PKCS7_stream() */ |
|
+/* |
|
+ * On success, the returned BIO owns the input BIO as part of its BIO chain. |
|
+ * On failure, NULL is returned and the input BIO is owned by the caller. |
|
+ * |
|
+ * Unfortunately cannot constify this due to CMS_stream() and PKCS7_stream() |
|
+ */ |
|
BIO *BIO_new_NDEF(BIO *out, ASN1_VALUE *val, const ASN1_ITEM *it) |
|
{ |
|
NDEF_SUPPORT *ndef_aux = NULL; |
|
BIO *asn_bio = NULL; |
|
const ASN1_AUX *aux = it->funcs; |
|
ASN1_STREAM_ARG sarg; |
|
+ BIO *pop_bio = NULL; |
|
|
|
if (!aux || !aux->asn1_cb) { |
|
ERR_raise(ERR_LIB_ASN1, ASN1_R_STREAMING_NOT_SUPPORTED); |
|
@@ -70,21 +76,39 @@ BIO *BIO_new_NDEF(BIO *out, ASN1_VALUE *val, const ASN1_ITEM *it) |
|
out = BIO_push(asn_bio, out); |
|
if (out == NULL) |
|
goto err; |
|
+ pop_bio = asn_bio; |
|
|
|
- BIO_asn1_set_prefix(asn_bio, ndef_prefix, ndef_prefix_free); |
|
- BIO_asn1_set_suffix(asn_bio, ndef_suffix, ndef_suffix_free); |
|
+ if (BIO_asn1_set_prefix(asn_bio, ndef_prefix, ndef_prefix_free) <= 0 |
|
+ || BIO_asn1_set_suffix(asn_bio, ndef_suffix, ndef_suffix_free) <= 0 |
|
+ || BIO_ctrl(asn_bio, BIO_C_SET_EX_ARG, 0, ndef_aux) <= 0) |
|
+ goto err; |
|
|
|
/* |
|
- * Now let callback prepends any digest, cipher etc BIOs ASN1 structure |
|
- * needs. |
|
+ * Now let the callback prepend any digest, cipher, etc., that the BIO's |
|
+ * ASN1 structure needs. |
|
*/ |
|
|
|
sarg.out = out; |
|
sarg.ndef_bio = NULL; |
|
sarg.boundary = NULL; |
|
|
|
- if (aux->asn1_cb(ASN1_OP_STREAM_PRE, &val, it, &sarg) <= 0) |
|
+ /* |
|
+ * The asn1_cb(), must not have mutated asn_bio on error, leaving it in the |
|
+ * middle of some partially built, but not returned BIO chain. |
|
+ */ |
|
+ if (aux->asn1_cb(ASN1_OP_STREAM_PRE, &val, it, &sarg) <= 0) { |
|
+ /* |
|
+ * ndef_aux is now owned by asn_bio so we must not free it in the err |
|
+ * clean up block |
|
+ */ |
|
+ ndef_aux = NULL; |
|
goto err; |
|
+ } |
|
+ |
|
+ /* |
|
+ * We must not fail now because the callback has prepended additional |
|
+ * BIOs to the chain |
|
+ */ |
|
|
|
ndef_aux->val = val; |
|
ndef_aux->it = it; |
|
@@ -92,11 +116,11 @@ BIO *BIO_new_NDEF(BIO *out, ASN1_VALUE *val, const ASN1_ITEM *it) |
|
ndef_aux->boundary = sarg.boundary; |
|
ndef_aux->out = out; |
|
|
|
- BIO_ctrl(asn_bio, BIO_C_SET_EX_ARG, 0, ndef_aux); |
|
- |
|
return sarg.ndef_bio; |
|
|
|
err: |
|
+ /* BIO_pop() is NULL safe */ |
|
+ (void)BIO_pop(pop_bio); |
|
BIO_free(asn_bio); |
|
OPENSSL_free(ndef_aux); |
|
return NULL; |
|
-- |
|
2.39.1 |
|
|
|
From f596ec8a6f9f5fcfa8e46a73b60f78a609725294 Mon Sep 17 00:00:00 2001 |
|
From: Matt Caswell <matt@openssl.org> |
|
Date: Wed, 14 Dec 2022 17:15:18 +0000 |
|
Subject: [PATCH 07/18] Check CMS failure during BIO setup with -stream is |
|
handled correctly |
|
|
|
Test for the issue fixed in the previous commit |
|
|
|
Reviewed-by: Paul Dale <pauli@openssl.org> |
|
Reviewed-by: Tomas Mraz <tomas@openssl.org> |
|
--- |
|
test/recipes/80-test_cms.t | 15 +++++++++++++-- |
|
test/smime-certs/badrsa.pem | 18 ++++++++++++++++++ |
|
2 files changed, 31 insertions(+), 2 deletions(-) |
|
create mode 100644 test/smime-certs/badrsa.pem |
|
|
|
diff --git a/test/recipes/80-test_cms.t b/test/recipes/80-test_cms.t |
|
index 610f1cbc51..fd53683e6b 100644 |
|
--- a/test/recipes/80-test_cms.t |
|
+++ b/test/recipes/80-test_cms.t |
|
@@ -13,7 +13,7 @@ use warnings; |
|
use POSIX; |
|
use File::Spec::Functions qw/catfile/; |
|
use File::Compare qw/compare_text compare/; |
|
-use OpenSSL::Test qw/:DEFAULT srctop_dir srctop_file bldtop_dir bldtop_file/; |
|
+use OpenSSL::Test qw/:DEFAULT srctop_dir srctop_file bldtop_dir bldtop_file with/; |
|
|
|
use OpenSSL::Test::Utils; |
|
|
|
@@ -50,7 +50,7 @@ my ($no_des, $no_dh, $no_dsa, $no_ec, $no_ec2m, $no_rc2, $no_zlib) |
|
|
|
$no_rc2 = 1 if disabled("legacy"); |
|
|
|
-plan tests => 12; |
|
+plan tests => 13; |
|
|
|
ok(run(test(["pkcs7_test"])), "test pkcs7"); |
|
|
|
@@ -972,3 +972,14 @@ ok(!run(app(['openssl', 'cms', '-verify', |
|
|
|
return ""; |
|
} |
|
+ |
|
+# Check that we get the expected failure return code |
|
+with({ exit_checker => sub { return shift == 6; } }, |
|
+ sub { |
|
+ ok(run(app(['openssl', 'cms', '-encrypt', |
|
+ '-in', srctop_file("test", "smcont.txt"), |
|
+ '-stream', '-recip', |
|
+ srctop_file("test/smime-certs", "badrsa.pem"), |
|
+ ])), |
|
+ "Check failure during BIO setup with -stream is handled correctly"); |
|
+ }); |
|
diff --git a/test/smime-certs/badrsa.pem b/test/smime-certs/badrsa.pem |
|
new file mode 100644 |
|
index 0000000000..f824fc2267 |
|
--- /dev/null |
|
+++ b/test/smime-certs/badrsa.pem |
|
@@ -0,0 +1,18 @@ |
|
+-----BEGIN CERTIFICATE----- |
|
+MIIDbTCCAlWgAwIBAgIToTV4Z0iuK08vZP20oTh//hC8BDANBgkqhkiG9w0BAQ0FADAtMSswKQYD |
|
+VfcDEyJTYW1wbGUgTEFNUFMgQ2VydGlmaWNhdGUgQXV0aG9yaXR5MCAXDTE5MTEyMDA2NTQxOFoY |
|
+DzIwNTIwOTI3MDY1NDE4WjAZMRcwFQYDVQQDEw5BbGljZSBMb3ZlbGFjZTCCASIwDQYJKoZIhvcN |
|
+AQEBBQADggEPADCCAQoCggEBALT0iehYOBY+TZp/T5K2KNI05Hwr+E3wP6XTvyi6WWyTgBK9LCOw |
|
+I2juwdRrjFBmXkk7pWpjXwsA3A5GOtz0FpfgyC7OxsVcF7q4WHWZWleYXFKlQHJD73nQwXP968+A |
|
+/3rBX7PhO0DBbZnfitOLPgPEwjTtdg0VQQ6Wz+CRQ/YbHPKaw7aRphZO63dKvIKp4cQVtkWQHi6s |
|
+yTjGsgkLcLNau5LZDQUdsGV+SAo3nBdWCRYV+I65x8Kf4hCxqqmjV3d/2NKRu0BXnDe/N+iDz3X0 |
|
+zEoj0fqXgq4SWcC0nsG1lyyXt1TL270I6ATKRGJWiQVCCpDtc0NT6vdJ45bCSxgCAwEAAaOBlzCB |
|
+lDAMBgNVHRMBAf8EAjAAMB4GA1UdEQQXMBWBE2FsaWNlQHNtaW1lLmV4YW1wbGUwEwYDVR0lBAww |
|
+CgYIKwYBBQUHAwQwDwYDVR0PAQH/BAUDAwfAADAdBgNVHQ4EFgQUu/bMsi0dBhIcl64papAQ0yBm |
|
+ZnMwHwYDVR0jBBgwFoAUeF8OWnjYa+RUcD2z3ez38fL6wEcwDQYJKoZIhvcNAQENBQADggEBABbW |
|
+eonR6TMTckehDKNOabwaCIcekahAIL6l9tTzUX5ew6ufiAPlC6I/zQlmUaU0iSyFDG1NW14kNbFt |
|
+5CAokyLhMtE4ASHBIHbiOp/ZSbUBTVYJZB61ot7w1/ol5QECSs08b8zrxIncf+t2DHGuVEy/Qq1d |
|
+rBz8d4ay8zpqAE1tUyL5Da6ZiKUfWwZQXSI/JlbjQFzYQqTRDnzHWrg1xPeMTO1P2/cplFaseTiv |
|
+yk4cYwOp/W9UAWymOZXF8WcJYCIUXkdcG/nEZxr057KlScrJmFXOoh7Y+8ON4iWYYcAfiNgpUFo/ |
|
+j8BAwrKKaFvdlZS9k1Ypb2+UQY75mKJE9Bg= |
|
+-----END CERTIFICATE----- |
|
-- |
|
2.39.1 |
|
|
|
|