1 |
From 23985bac83fd50c8e29431009302b5442f985096 Mon Sep 17 00:00:00 2001 |
2 |
From: slontis <shane.lontis@oracle.com> |
3 |
Date: Wed, 11 Jan 2023 11:05:04 +1000 |
4 |
Subject: [PATCH 10/18] Fix NULL deference when validating FFC public key. |
5 |
|
6 |
Fixes CVE-2023-0217 |
7 |
|
8 |
When attempting to do a BN_Copy of params->p there was no NULL check. |
9 |
Since BN_copy does not check for NULL this is a NULL reference. |
10 |
|
11 |
As an aside BN_cmp() does do a NULL check, so there are other checks |
12 |
that fail because a NULL is passed. A more general check for NULL params |
13 |
has been added for both FFC public and private key validation instead. |
14 |
|
15 |
Reviewed-by: Matt Caswell <matt@openssl.org> |
16 |
Reviewed-by: Paul Dale <pauli@openssl.org> |
17 |
Reviewed-by: Tomas Mraz <tomas@openssl.org> |
18 |
--- |
19 |
crypto/ffc/ffc_key_validate.c | 9 +++++++++ |
20 |
include/internal/ffc.h | 1 + |
21 |
test/ffc_internal_test.c | 31 +++++++++++++++++++++++++++++++ |
22 |
3 files changed, 41 insertions(+) |
23 |
|
24 |
diff --git a/crypto/ffc/ffc_key_validate.c b/crypto/ffc/ffc_key_validate.c |
25 |
index 9f6525a2c8..442303e4b3 100644 |
26 |
--- a/crypto/ffc/ffc_key_validate.c |
27 |
+++ b/crypto/ffc/ffc_key_validate.c |
28 |
@@ -24,6 +24,11 @@ int ossl_ffc_validate_public_key_partial(const FFC_PARAMS *params, |
29 |
BN_CTX *ctx = NULL; |
30 |
|
31 |
*ret = 0; |
32 |
+ if (params == NULL || pub_key == NULL || params->p == NULL) { |
33 |
+ *ret = FFC_ERROR_PASSED_NULL_PARAM; |
34 |
+ return 0; |
35 |
+ } |
36 |
+ |
37 |
ctx = BN_CTX_new_ex(NULL); |
38 |
if (ctx == NULL) |
39 |
goto err; |
40 |
@@ -107,6 +112,10 @@ int ossl_ffc_validate_private_key(const BIGNUM *upper, const BIGNUM *priv, |
41 |
|
42 |
*ret = 0; |
43 |
|
44 |
+ if (priv == NULL || upper == NULL) { |
45 |
+ *ret = FFC_ERROR_PASSED_NULL_PARAM; |
46 |
+ goto err; |
47 |
+ } |
48 |
if (BN_cmp(priv, BN_value_one()) < 0) { |
49 |
*ret |= FFC_ERROR_PRIVKEY_TOO_SMALL; |
50 |
goto err; |
51 |
diff --git a/include/internal/ffc.h b/include/internal/ffc.h |
52 |
index 732514a6c2..b8b7140857 100644 |
53 |
--- a/include/internal/ffc.h |
54 |
+++ b/include/internal/ffc.h |
55 |
@@ -76,6 +76,7 @@ |
56 |
# define FFC_ERROR_NOT_SUITABLE_GENERATOR 0x08 |
57 |
# define FFC_ERROR_PRIVKEY_TOO_SMALL 0x10 |
58 |
# define FFC_ERROR_PRIVKEY_TOO_LARGE 0x20 |
59 |
+# define FFC_ERROR_PASSED_NULL_PARAM 0x40 |
60 |
|
61 |
/* |
62 |
* Finite field cryptography (FFC) domain parameters are used by DH and DSA. |
63 |
diff --git a/test/ffc_internal_test.c b/test/ffc_internal_test.c |
64 |
index 2c97293573..9f67bd29b9 100644 |
65 |
--- a/test/ffc_internal_test.c |
66 |
+++ b/test/ffc_internal_test.c |
67 |
@@ -510,6 +510,27 @@ static int ffc_public_validate_test(void) |
68 |
if (!TEST_true(ossl_ffc_validate_public_key(params, pub, &res))) |
69 |
goto err; |
70 |
|
71 |
+ /* Fail if params is NULL */ |
72 |
+ if (!TEST_false(ossl_ffc_validate_public_key(NULL, pub, &res))) |
73 |
+ goto err; |
74 |
+ if (!TEST_int_eq(FFC_ERROR_PASSED_NULL_PARAM, res)) |
75 |
+ goto err; |
76 |
+ res = -1; |
77 |
+ /* Fail if pubkey is NULL */ |
78 |
+ if (!TEST_false(ossl_ffc_validate_public_key(params, NULL, &res))) |
79 |
+ goto err; |
80 |
+ if (!TEST_int_eq(FFC_ERROR_PASSED_NULL_PARAM, res)) |
81 |
+ goto err; |
82 |
+ res = -1; |
83 |
+ |
84 |
+ BN_free(params->p); |
85 |
+ params->p = NULL; |
86 |
+ /* Fail if params->p is NULL */ |
87 |
+ if (!TEST_false(ossl_ffc_validate_public_key(params, pub, &res))) |
88 |
+ goto err; |
89 |
+ if (!TEST_int_eq(FFC_ERROR_PASSED_NULL_PARAM, res)) |
90 |
+ goto err; |
91 |
+ |
92 |
ret = 1; |
93 |
err: |
94 |
DH_free(dh); |
95 |
@@ -567,6 +588,16 @@ static int ffc_private_validate_test(void) |
96 |
if (!TEST_true(ossl_ffc_validate_private_key(params->q, priv, &res))) |
97 |
goto err; |
98 |
|
99 |
+ if (!TEST_false(ossl_ffc_validate_private_key(NULL, priv, &res))) |
100 |
+ goto err; |
101 |
+ if (!TEST_int_eq(FFC_ERROR_PASSED_NULL_PARAM, res)) |
102 |
+ goto err; |
103 |
+ res = -1; |
104 |
+ if (!TEST_false(ossl_ffc_validate_private_key(params->q, NULL, &res))) |
105 |
+ goto err; |
106 |
+ if (!TEST_int_eq(FFC_ERROR_PASSED_NULL_PARAM, res)) |
107 |
+ goto err; |
108 |
+ |
109 |
ret = 1; |
110 |
err: |
111 |
DH_free(dh); |
112 |
-- |
113 |
2.39.1 |
114 |
|
115 |
From c1b4467a7cc129a74fc5205b80a5c47556b99416 Mon Sep 17 00:00:00 2001 |
116 |
From: Tomas Mraz <tomas@openssl.org> |
117 |
Date: Fri, 13 Jan 2023 17:57:59 +0100 |
118 |
Subject: [PATCH 11/18] Prevent creating DSA and DH keys without parameters |
119 |
through import |
120 |
|
121 |
Reviewed-by: Matt Caswell <matt@openssl.org> |
122 |
Reviewed-by: Paul Dale <pauli@openssl.org> |
123 |
--- |
124 |
providers/implementations/keymgmt/dh_kmgmt.c | 4 ++-- |
125 |
providers/implementations/keymgmt/dsa_kmgmt.c | 5 +++-- |
126 |
2 files changed, 5 insertions(+), 4 deletions(-) |
127 |
|
128 |
diff --git a/providers/implementations/keymgmt/dh_kmgmt.c b/providers/implementations/keymgmt/dh_kmgmt.c |
129 |
index 58a5fd009f..c2d87b4a7f 100644 |
130 |
--- a/providers/implementations/keymgmt/dh_kmgmt.c |
131 |
+++ b/providers/implementations/keymgmt/dh_kmgmt.c |
132 |
@@ -198,8 +198,8 @@ static int dh_import(void *keydata, int selection, const OSSL_PARAM params[]) |
133 |
if ((selection & DH_POSSIBLE_SELECTIONS) == 0) |
134 |
return 0; |
135 |
|
136 |
- if ((selection & OSSL_KEYMGMT_SELECT_ALL_PARAMETERS) != 0) |
137 |
- ok = ok && ossl_dh_params_fromdata(dh, params); |
138 |
+ /* a key without parameters is meaningless */ |
139 |
+ ok = ok && ossl_dh_params_fromdata(dh, params); |
140 |
|
141 |
if ((selection & OSSL_KEYMGMT_SELECT_KEYPAIR) != 0) { |
142 |
int include_private = |
143 |
diff --git a/providers/implementations/keymgmt/dsa_kmgmt.c b/providers/implementations/keymgmt/dsa_kmgmt.c |
144 |
index 100e917167..881680c085 100644 |
145 |
--- a/providers/implementations/keymgmt/dsa_kmgmt.c |
146 |
+++ b/providers/implementations/keymgmt/dsa_kmgmt.c |
147 |
@@ -199,8 +199,9 @@ static int dsa_import(void *keydata, int selection, const OSSL_PARAM params[]) |
148 |
if ((selection & DSA_POSSIBLE_SELECTIONS) == 0) |
149 |
return 0; |
150 |
|
151 |
- if ((selection & OSSL_KEYMGMT_SELECT_ALL_PARAMETERS) != 0) |
152 |
- ok = ok && ossl_dsa_ffc_params_fromdata(dsa, params); |
153 |
+ /* a key without parameters is meaningless */ |
154 |
+ ok = ok && ossl_dsa_ffc_params_fromdata(dsa, params); |
155 |
+ |
156 |
if ((selection & OSSL_KEYMGMT_SELECT_KEYPAIR) != 0) { |
157 |
int include_private = |
158 |
selection & OSSL_KEYMGMT_SELECT_PRIVATE_KEY ? 1 : 0; |
159 |
-- |
160 |
2.39.1 |
161 |
|
162 |
From fab4973801bdc11c29c4c8ccf65cf39cbc63ce9b Mon Sep 17 00:00:00 2001 |
163 |
From: Tomas Mraz <tomas@openssl.org> |
164 |
Date: Fri, 13 Jan 2023 17:59:52 +0100 |
165 |
Subject: [PATCH 12/18] Do not create DSA keys without parameters by decoder |
166 |
|
167 |
Reviewed-by: Matt Caswell <matt@openssl.org> |
168 |
Reviewed-by: Paul Dale <pauli@openssl.org> |
169 |
--- |
170 |
crypto/x509/x_pubkey.c | 24 +++++++++++++++++++ |
171 |
include/crypto/x509.h | 3 +++ |
172 |
.../encode_decode/decode_der2key.c | 2 +- |
173 |
3 files changed, 28 insertions(+), 1 deletion(-) |
174 |
|
175 |
diff --git a/crypto/x509/x_pubkey.c b/crypto/x509/x_pubkey.c |
176 |
index bc90ddd89b..77790faa1f 100644 |
177 |
--- a/crypto/x509/x_pubkey.c |
178 |
+++ b/crypto/x509/x_pubkey.c |
179 |
@@ -745,6 +745,30 @@ DSA *d2i_DSA_PUBKEY(DSA **a, const unsigned char **pp, long length) |
180 |
return key; |
181 |
} |
182 |
|
183 |
+/* Called from decoders; disallows provided DSA keys without parameters. */ |
184 |
+DSA *ossl_d2i_DSA_PUBKEY(DSA **a, const unsigned char **pp, long length) |
185 |
+{ |
186 |
+ DSA *key = NULL; |
187 |
+ const unsigned char *data; |
188 |
+ const BIGNUM *p, *q, *g; |
189 |
+ |
190 |
+ data = *pp; |
191 |
+ key = d2i_DSA_PUBKEY(NULL, &data, length); |
192 |
+ if (key == NULL) |
193 |
+ return NULL; |
194 |
+ DSA_get0_pqg(key, &p, &q, &g); |
195 |
+ if (p == NULL || q == NULL || g == NULL) { |
196 |
+ DSA_free(key); |
197 |
+ return NULL; |
198 |
+ } |
199 |
+ *pp = data; |
200 |
+ if (a != NULL) { |
201 |
+ DSA_free(*a); |
202 |
+ *a = key; |
203 |
+ } |
204 |
+ return key; |
205 |
+} |
206 |
+ |
207 |
int i2d_DSA_PUBKEY(const DSA *a, unsigned char **pp) |
208 |
{ |
209 |
EVP_PKEY *pktmp; |
210 |
diff --git a/include/crypto/x509.h b/include/crypto/x509.h |
211 |
index 1f00178e89..0c42730ee9 100644 |
212 |
--- a/include/crypto/x509.h |
213 |
+++ b/include/crypto/x509.h |
214 |
@@ -339,6 +339,9 @@ void ossl_X509_PUBKEY_INTERNAL_free(X509_PUBKEY *xpub); |
215 |
|
216 |
RSA *ossl_d2i_RSA_PSS_PUBKEY(RSA **a, const unsigned char **pp, long length); |
217 |
int ossl_i2d_RSA_PSS_PUBKEY(const RSA *a, unsigned char **pp); |
218 |
+# ifndef OPENSSL_NO_DSA |
219 |
+DSA *ossl_d2i_DSA_PUBKEY(DSA **a, const unsigned char **pp, long length); |
220 |
+# endif /* OPENSSL_NO_DSA */ |
221 |
# ifndef OPENSSL_NO_DH |
222 |
DH *ossl_d2i_DH_PUBKEY(DH **a, const unsigned char **pp, long length); |
223 |
int ossl_i2d_DH_PUBKEY(const DH *a, unsigned char **pp); |
224 |
diff --git a/providers/implementations/encode_decode/decode_der2key.c b/providers/implementations/encode_decode/decode_der2key.c |
225 |
index ebc2d24833..d6ad738ef3 100644 |
226 |
--- a/providers/implementations/encode_decode/decode_der2key.c |
227 |
+++ b/providers/implementations/encode_decode/decode_der2key.c |
228 |
@@ -374,7 +374,7 @@ static void *dsa_d2i_PKCS8(void **key, const unsigned char **der, long der_len, |
229 |
(key_from_pkcs8_t *)ossl_dsa_key_from_pkcs8); |
230 |
} |
231 |
|
232 |
-# define dsa_d2i_PUBKEY (d2i_of_void *)d2i_DSA_PUBKEY |
233 |
+# define dsa_d2i_PUBKEY (d2i_of_void *)ossl_d2i_DSA_PUBKEY |
234 |
# define dsa_free (free_key_fn *)DSA_free |
235 |
# define dsa_check NULL |
236 |
|
237 |
-- |
238 |
2.39.1 |
239 |
|
240 |
From 7e37185582995b35f885fec9dcc3670af9ffcbef Mon Sep 17 00:00:00 2001 |
241 |
From: Tomas Mraz <tomas@openssl.org> |
242 |
Date: Fri, 13 Jan 2023 18:46:15 +0100 |
243 |
Subject: [PATCH 13/18] Add test for DSA pubkey without param import and check |
244 |
|
245 |
Reviewed-by: Matt Caswell <matt@openssl.org> |
246 |
Reviewed-by: Paul Dale <pauli@openssl.org> |
247 |
--- |
248 |
test/recipes/91-test_pkey_check.t | 48 ++++++++++++++---- |
249 |
.../91-test_pkey_check_data/dsapub.pem | 12 +++++ |
250 |
.../dsapub_noparam.der | Bin 0 -> 108 bytes |
251 |
3 files changed, 49 insertions(+), 11 deletions(-) |
252 |
create mode 100644 test/recipes/91-test_pkey_check_data/dsapub.pem |
253 |
create mode 100644 test/recipes/91-test_pkey_check_data/dsapub_noparam.der |
254 |
|
255 |
diff --git a/test/recipes/91-test_pkey_check.t b/test/recipes/91-test_pkey_check.t |
256 |
index 612a3e3d6c..015d7805db 100644 |
257 |
--- a/test/recipes/91-test_pkey_check.t |
258 |
+++ b/test/recipes/91-test_pkey_check.t |
259 |
@@ -11,19 +11,24 @@ use strict; |
260 |
use warnings; |
261 |
|
262 |
use File::Spec; |
263 |
-use OpenSSL::Test qw/:DEFAULT data_file/; |
264 |
+use OpenSSL::Test qw/:DEFAULT data_file with/; |
265 |
use OpenSSL::Test::Utils; |
266 |
|
267 |
sub pkey_check { |
268 |
my $f = shift; |
269 |
+ my $pubcheck = shift; |
270 |
+ my @checkopt = ('-check'); |
271 |
|
272 |
- return run(app(['openssl', 'pkey', '-check', '-text', |
273 |
+ @checkopt = ('-pubcheck', '-pubin') if $pubcheck; |
274 |
+ |
275 |
+ return run(app(['openssl', 'pkey', @checkopt, '-text', |
276 |
'-in', $f])); |
277 |
} |
278 |
|
279 |
sub check_key { |
280 |
my $f = shift; |
281 |
my $should_fail = shift; |
282 |
+ my $pubcheck = shift; |
283 |
my $str; |
284 |
|
285 |
|
286 |
@@ -33,11 +38,10 @@ sub check_key { |
287 |
$f = data_file($f); |
288 |
|
289 |
if ( -s $f ) { |
290 |
- if ($should_fail) { |
291 |
- ok(!pkey_check($f), $str); |
292 |
- } else { |
293 |
- ok(pkey_check($f), $str); |
294 |
- } |
295 |
+ with({ exit_checker => sub { return shift == $should_fail; } }, |
296 |
+ sub { |
297 |
+ ok(pkey_check($f, $pubcheck), $str); |
298 |
+ }); |
299 |
} else { |
300 |
fail("Missing file $f"); |
301 |
} |
302 |
@@ -66,15 +70,37 @@ push(@positive_tests, ( |
303 |
"dhpkey.pem" |
304 |
)) unless disabled("dh"); |
305 |
|
306 |
+my @negative_pubtests = (); |
307 |
+ |
308 |
+push(@negative_pubtests, ( |
309 |
+ "dsapub_noparam.der" |
310 |
+ )) unless disabled("dsa"); |
311 |
+ |
312 |
+my @positive_pubtests = (); |
313 |
+ |
314 |
+push(@positive_pubtests, ( |
315 |
+ "dsapub.pem" |
316 |
+ )) unless disabled("dsa"); |
317 |
+ |
318 |
plan skip_all => "No tests within the current enabled feature set" |
319 |
- unless @negative_tests && @positive_tests; |
320 |
+ unless @negative_tests && @positive_tests |
321 |
+ && @negative_pubtests && @positive_pubtests; |
322 |
|
323 |
-plan tests => scalar(@negative_tests) + scalar(@positive_tests); |
324 |
+plan tests => scalar(@negative_tests) + scalar(@positive_tests) |
325 |
+ + scalar(@negative_pubtests) + scalar(@positive_pubtests); |
326 |
|
327 |
foreach my $t (@negative_tests) { |
328 |
- check_key($t, 1); |
329 |
+ check_key($t, 1, 0); |
330 |
} |
331 |
|
332 |
foreach my $t (@positive_tests) { |
333 |
- check_key($t, 0); |
334 |
+ check_key($t, 0, 0); |
335 |
+} |
336 |
+ |
337 |
+foreach my $t (@negative_pubtests) { |
338 |
+ check_key($t, 1, 1); |
339 |
+} |
340 |
+ |
341 |
+foreach my $t (@positive_pubtests) { |
342 |
+ check_key($t, 0, 1); |
343 |
} |
344 |
diff --git a/test/recipes/91-test_pkey_check_data/dsapub.pem b/test/recipes/91-test_pkey_check_data/dsapub.pem |
345 |
new file mode 100644 |
346 |
index 0000000000..0ff4bd83ed |
347 |
--- /dev/null |
348 |
+++ b/test/recipes/91-test_pkey_check_data/dsapub.pem |
349 |
@@ -0,0 +1,12 @@ |
350 |
+-----BEGIN PUBLIC KEY----- |
351 |
+MIIBvzCCATQGByqGSM44BAEwggEnAoGBAIjbXpOVVciVNuagg26annKkghIIZFI4 |
352 |
+4WdMomnV+I/oXyxHbZTBBBpW9xy/E1+yMjbp4GmX+VxyDj3WxUWxXllzL+miEkzD |
353 |
+9Xz638VzIBhjFbMvk1/N4kS4bKVUd9yk7HfvYzAdnRphk0WI+RoDiDrBNPPxSoQD |
354 |
+CEWgvwgsLIDhAh0A6dbz1IQpQwGF4+Ca28x6OO+UfJJv3ggeZ++fNwKBgQCA9XKV |
355 |
+lRrTY8ALBxS0KbZjpaIXuUj5nr3i1lIDyP3ISksDF0ekyLtn6eK9VijX6Pm65Np+ |
356 |
+4ic9Nr5WKLKhPaUSpLNRx1gDqo3sd92hYgiEUifzEuhLYfK/CsgFED+l2hDXtJUq |
357 |
+bISNSHVwI5lsyNXLu7HI1Fk8F5UO3LqsboFAngOBhAACgYATxFY89nEYcUhgHGgr |
358 |
+YDHhXBQfMKnTKYdvon4DN7WQ9ip+t4VUsLpTD1ZE9zrM2R/B04+8C6KGoViwyeER |
359 |
+kS4dxWOkX71x4X2DlNpYevcR53tNcTDqmMD7YKfDDmrb0lftMyfW8aESaiymVMys |
360 |
+DRjhKHBjdo0rZeSM8DAk3ctrXA== |
361 |
+-----END PUBLIC KEY----- |
362 |
diff --git a/test/recipes/91-test_pkey_check_data/dsapub_noparam.der b/test/recipes/91-test_pkey_check_data/dsapub_noparam.der |
363 |
new file mode 100644 |
364 |
index 0000000000000000000000000000000000000000..b8135f1ca94da914b6829421e0c13f6daa731862 |
365 |
GIT binary patch |
366 |
literal 108 |
367 |
zcmXpIGT>xm*J|@PXTieE%*wz71<Xv0AT}3_&&0^YB*etj0OvEYF$n`XLd*y;pgagL |
368 |
U3o&W4F|x9<gY>|F5F-Nv0Bz9(=Kufz |
369 |
|
370 |
literal 0 |
371 |
HcmV?d00001 |
372 |
|
373 |
-- |
374 |
2.39.1 |
375 |
|
376 |
From 2ad9928170768653d19d81881deabc5f9c1665c0 Mon Sep 17 00:00:00 2001 |
377 |
From: Tomas Mraz <tomas@openssl.org> |
378 |
Date: Fri, 3 Feb 2023 14:57:04 +0100 |
379 |
Subject: [PATCH 18/18] Internaly declare the DSA type for no-deprecated builds |
380 |
|
381 |
Reviewed-by: Hugo Landau <hlandau@openssl.org> |
382 |
Reviewed-by: Richard Levitte <levitte@openssl.org> |
383 |
(cherry picked from commit 7a21a1b5fa2dac438892cf3292d1f9c445d870d9) |
384 |
--- |
385 |
include/crypto/types.h | 3 +++ |
386 |
1 file changed, 3 insertions(+) |
387 |
|
388 |
diff --git a/include/crypto/types.h b/include/crypto/types.h |
389 |
index 0d81404091..0a75f03a3f 100644 |
390 |
--- a/include/crypto/types.h |
391 |
+++ b/include/crypto/types.h |
392 |
@@ -20,6 +20,9 @@ typedef struct rsa_meth_st RSA_METHOD; |
393 |
typedef struct ec_key_st EC_KEY; |
394 |
typedef struct ec_key_method_st EC_KEY_METHOD; |
395 |
# endif |
396 |
+# ifndef OPENSSL_NO_DSA |
397 |
+typedef struct dsa_st DSA; |
398 |
+# endif |
399 |
# endif |
400 |
|
401 |
# ifndef OPENSSL_NO_EC |
402 |
-- |
403 |
2.39.1 |
404 |
|