| 1 |
From 8818064ce3c3c0f1b740a5aaba2a987e75bfbafd Mon Sep 17 00:00:00 2001 |
| 2 |
From: Matt Caswell <matt@openssl.org> |
| 3 |
Date: Wed, 14 Dec 2022 16:18:14 +0000 |
| 4 |
Subject: [PATCH 06/18] Fix a UAF resulting from a bug in BIO_new_NDEF |
| 5 |
|
| 6 |
If the aux->asn1_cb() call fails in BIO_new_NDEF then the "out" BIO will |
| 7 |
be part of an invalid BIO chain. This causes a "use after free" when the |
| 8 |
BIO is eventually freed. |
| 9 |
|
| 10 |
Based on an original patch by Viktor Dukhovni and an idea from Theo |
| 11 |
Buehler. |
| 12 |
|
| 13 |
Thanks to Octavio Galland for reporting this issue. |
| 14 |
|
| 15 |
Reviewed-by: Paul Dale <pauli@openssl.org> |
| 16 |
Reviewed-by: Tomas Mraz <tomas@openssl.org> |
| 17 |
--- |
| 18 |
crypto/asn1/bio_ndef.c | 40 ++++++++++++++++++++++++++++++++-------- |
| 19 |
1 file changed, 32 insertions(+), 8 deletions(-) |
| 20 |
|
| 21 |
diff --git a/crypto/asn1/bio_ndef.c b/crypto/asn1/bio_ndef.c |
| 22 |
index d94e3a3644..b9df3a7a47 100644 |
| 23 |
--- a/crypto/asn1/bio_ndef.c |
| 24 |
+++ b/crypto/asn1/bio_ndef.c |
| 25 |
@@ -49,13 +49,19 @@ static int ndef_suffix(BIO *b, unsigned char **pbuf, int *plen, void *parg); |
| 26 |
static int ndef_suffix_free(BIO *b, unsigned char **pbuf, int *plen, |
| 27 |
void *parg); |
| 28 |
|
| 29 |
-/* unfortunately cannot constify this due to CMS_stream() and PKCS7_stream() */ |
| 30 |
+/* |
| 31 |
+ * On success, the returned BIO owns the input BIO as part of its BIO chain. |
| 32 |
+ * On failure, NULL is returned and the input BIO is owned by the caller. |
| 33 |
+ * |
| 34 |
+ * Unfortunately cannot constify this due to CMS_stream() and PKCS7_stream() |
| 35 |
+ */ |
| 36 |
BIO *BIO_new_NDEF(BIO *out, ASN1_VALUE *val, const ASN1_ITEM *it) |
| 37 |
{ |
| 38 |
NDEF_SUPPORT *ndef_aux = NULL; |
| 39 |
BIO *asn_bio = NULL; |
| 40 |
const ASN1_AUX *aux = it->funcs; |
| 41 |
ASN1_STREAM_ARG sarg; |
| 42 |
+ BIO *pop_bio = NULL; |
| 43 |
|
| 44 |
if (!aux || !aux->asn1_cb) { |
| 45 |
ERR_raise(ERR_LIB_ASN1, ASN1_R_STREAMING_NOT_SUPPORTED); |
| 46 |
@@ -70,21 +76,39 @@ BIO *BIO_new_NDEF(BIO *out, ASN1_VALUE *val, const ASN1_ITEM *it) |
| 47 |
out = BIO_push(asn_bio, out); |
| 48 |
if (out == NULL) |
| 49 |
goto err; |
| 50 |
+ pop_bio = asn_bio; |
| 51 |
|
| 52 |
- BIO_asn1_set_prefix(asn_bio, ndef_prefix, ndef_prefix_free); |
| 53 |
- BIO_asn1_set_suffix(asn_bio, ndef_suffix, ndef_suffix_free); |
| 54 |
+ if (BIO_asn1_set_prefix(asn_bio, ndef_prefix, ndef_prefix_free) <= 0 |
| 55 |
+ || BIO_asn1_set_suffix(asn_bio, ndef_suffix, ndef_suffix_free) <= 0 |
| 56 |
+ || BIO_ctrl(asn_bio, BIO_C_SET_EX_ARG, 0, ndef_aux) <= 0) |
| 57 |
+ goto err; |
| 58 |
|
| 59 |
/* |
| 60 |
- * Now let callback prepends any digest, cipher etc BIOs ASN1 structure |
| 61 |
- * needs. |
| 62 |
+ * Now let the callback prepend any digest, cipher, etc., that the BIO's |
| 63 |
+ * ASN1 structure needs. |
| 64 |
*/ |
| 65 |
|
| 66 |
sarg.out = out; |
| 67 |
sarg.ndef_bio = NULL; |
| 68 |
sarg.boundary = NULL; |
| 69 |
|
| 70 |
- if (aux->asn1_cb(ASN1_OP_STREAM_PRE, &val, it, &sarg) <= 0) |
| 71 |
+ /* |
| 72 |
+ * The asn1_cb(), must not have mutated asn_bio on error, leaving it in the |
| 73 |
+ * middle of some partially built, but not returned BIO chain. |
| 74 |
+ */ |
| 75 |
+ if (aux->asn1_cb(ASN1_OP_STREAM_PRE, &val, it, &sarg) <= 0) { |
| 76 |
+ /* |
| 77 |
+ * ndef_aux is now owned by asn_bio so we must not free it in the err |
| 78 |
+ * clean up block |
| 79 |
+ */ |
| 80 |
+ ndef_aux = NULL; |
| 81 |
goto err; |
| 82 |
+ } |
| 83 |
+ |
| 84 |
+ /* |
| 85 |
+ * We must not fail now because the callback has prepended additional |
| 86 |
+ * BIOs to the chain |
| 87 |
+ */ |
| 88 |
|
| 89 |
ndef_aux->val = val; |
| 90 |
ndef_aux->it = it; |
| 91 |
@@ -92,11 +116,11 @@ BIO *BIO_new_NDEF(BIO *out, ASN1_VALUE *val, const ASN1_ITEM *it) |
| 92 |
ndef_aux->boundary = sarg.boundary; |
| 93 |
ndef_aux->out = out; |
| 94 |
|
| 95 |
- BIO_ctrl(asn_bio, BIO_C_SET_EX_ARG, 0, ndef_aux); |
| 96 |
- |
| 97 |
return sarg.ndef_bio; |
| 98 |
|
| 99 |
err: |
| 100 |
+ /* BIO_pop() is NULL safe */ |
| 101 |
+ (void)BIO_pop(pop_bio); |
| 102 |
BIO_free(asn_bio); |
| 103 |
OPENSSL_free(ndef_aux); |
| 104 |
return NULL; |
| 105 |
-- |
| 106 |
2.39.1 |
| 107 |
|
| 108 |
From f596ec8a6f9f5fcfa8e46a73b60f78a609725294 Mon Sep 17 00:00:00 2001 |
| 109 |
From: Matt Caswell <matt@openssl.org> |
| 110 |
Date: Wed, 14 Dec 2022 17:15:18 +0000 |
| 111 |
Subject: [PATCH 07/18] Check CMS failure during BIO setup with -stream is |
| 112 |
handled correctly |
| 113 |
|
| 114 |
Test for the issue fixed in the previous commit |
| 115 |
|
| 116 |
Reviewed-by: Paul Dale <pauli@openssl.org> |
| 117 |
Reviewed-by: Tomas Mraz <tomas@openssl.org> |
| 118 |
--- |
| 119 |
test/recipes/80-test_cms.t | 15 +++++++++++++-- |
| 120 |
test/smime-certs/badrsa.pem | 18 ++++++++++++++++++ |
| 121 |
2 files changed, 31 insertions(+), 2 deletions(-) |
| 122 |
create mode 100644 test/smime-certs/badrsa.pem |
| 123 |
|
| 124 |
diff --git a/test/recipes/80-test_cms.t b/test/recipes/80-test_cms.t |
| 125 |
index 610f1cbc51..fd53683e6b 100644 |
| 126 |
--- a/test/recipes/80-test_cms.t |
| 127 |
+++ b/test/recipes/80-test_cms.t |
| 128 |
@@ -13,7 +13,7 @@ use warnings; |
| 129 |
use POSIX; |
| 130 |
use File::Spec::Functions qw/catfile/; |
| 131 |
use File::Compare qw/compare_text compare/; |
| 132 |
-use OpenSSL::Test qw/:DEFAULT srctop_dir srctop_file bldtop_dir bldtop_file/; |
| 133 |
+use OpenSSL::Test qw/:DEFAULT srctop_dir srctop_file bldtop_dir bldtop_file with/; |
| 134 |
|
| 135 |
use OpenSSL::Test::Utils; |
| 136 |
|
| 137 |
@@ -50,7 +50,7 @@ my ($no_des, $no_dh, $no_dsa, $no_ec, $no_ec2m, $no_rc2, $no_zlib) |
| 138 |
|
| 139 |
$no_rc2 = 1 if disabled("legacy"); |
| 140 |
|
| 141 |
-plan tests => 12; |
| 142 |
+plan tests => 13; |
| 143 |
|
| 144 |
ok(run(test(["pkcs7_test"])), "test pkcs7"); |
| 145 |
|
| 146 |
@@ -972,3 +972,14 @@ ok(!run(app(['openssl', 'cms', '-verify', |
| 147 |
|
| 148 |
return ""; |
| 149 |
} |
| 150 |
+ |
| 151 |
+# Check that we get the expected failure return code |
| 152 |
+with({ exit_checker => sub { return shift == 6; } }, |
| 153 |
+ sub { |
| 154 |
+ ok(run(app(['openssl', 'cms', '-encrypt', |
| 155 |
+ '-in', srctop_file("test", "smcont.txt"), |
| 156 |
+ '-stream', '-recip', |
| 157 |
+ srctop_file("test/smime-certs", "badrsa.pem"), |
| 158 |
+ ])), |
| 159 |
+ "Check failure during BIO setup with -stream is handled correctly"); |
| 160 |
+ }); |
| 161 |
diff --git a/test/smime-certs/badrsa.pem b/test/smime-certs/badrsa.pem |
| 162 |
new file mode 100644 |
| 163 |
index 0000000000..f824fc2267 |
| 164 |
--- /dev/null |
| 165 |
+++ b/test/smime-certs/badrsa.pem |
| 166 |
@@ -0,0 +1,18 @@ |
| 167 |
+-----BEGIN CERTIFICATE----- |
| 168 |
+MIIDbTCCAlWgAwIBAgIToTV4Z0iuK08vZP20oTh//hC8BDANBgkqhkiG9w0BAQ0FADAtMSswKQYD |
| 169 |
+VfcDEyJTYW1wbGUgTEFNUFMgQ2VydGlmaWNhdGUgQXV0aG9yaXR5MCAXDTE5MTEyMDA2NTQxOFoY |
| 170 |
+DzIwNTIwOTI3MDY1NDE4WjAZMRcwFQYDVQQDEw5BbGljZSBMb3ZlbGFjZTCCASIwDQYJKoZIhvcN |
| 171 |
+AQEBBQADggEPADCCAQoCggEBALT0iehYOBY+TZp/T5K2KNI05Hwr+E3wP6XTvyi6WWyTgBK9LCOw |
| 172 |
+I2juwdRrjFBmXkk7pWpjXwsA3A5GOtz0FpfgyC7OxsVcF7q4WHWZWleYXFKlQHJD73nQwXP968+A |
| 173 |
+/3rBX7PhO0DBbZnfitOLPgPEwjTtdg0VQQ6Wz+CRQ/YbHPKaw7aRphZO63dKvIKp4cQVtkWQHi6s |
| 174 |
+yTjGsgkLcLNau5LZDQUdsGV+SAo3nBdWCRYV+I65x8Kf4hCxqqmjV3d/2NKRu0BXnDe/N+iDz3X0 |
| 175 |
+zEoj0fqXgq4SWcC0nsG1lyyXt1TL270I6ATKRGJWiQVCCpDtc0NT6vdJ45bCSxgCAwEAAaOBlzCB |
| 176 |
+lDAMBgNVHRMBAf8EAjAAMB4GA1UdEQQXMBWBE2FsaWNlQHNtaW1lLmV4YW1wbGUwEwYDVR0lBAww |
| 177 |
+CgYIKwYBBQUHAwQwDwYDVR0PAQH/BAUDAwfAADAdBgNVHQ4EFgQUu/bMsi0dBhIcl64papAQ0yBm |
| 178 |
+ZnMwHwYDVR0jBBgwFoAUeF8OWnjYa+RUcD2z3ez38fL6wEcwDQYJKoZIhvcNAQENBQADggEBABbW |
| 179 |
+eonR6TMTckehDKNOabwaCIcekahAIL6l9tTzUX5ew6ufiAPlC6I/zQlmUaU0iSyFDG1NW14kNbFt |
| 180 |
+5CAokyLhMtE4ASHBIHbiOp/ZSbUBTVYJZB61ot7w1/ol5QECSs08b8zrxIncf+t2DHGuVEy/Qq1d |
| 181 |
+rBz8d4ay8zpqAE1tUyL5Da6ZiKUfWwZQXSI/JlbjQFzYQqTRDnzHWrg1xPeMTO1P2/cplFaseTiv |
| 182 |
+yk4cYwOp/W9UAWymOZXF8WcJYCIUXkdcG/nEZxr057KlScrJmFXOoh7Y+8ON4iWYYcAfiNgpUFo/ |
| 183 |
+j8BAwrKKaFvdlZS9k1Ypb2+UQY75mKJE9Bg= |
| 184 |
+-----END CERTIFICATE----- |
| 185 |
-- |
| 186 |
2.39.1 |
| 187 |
|
Parent Directory
| 
Revision Log
| 
Revision Graph
