From 23985bac83fd50c8e29431009302b5442f985096 Mon Sep 17 00:00:00 2001 From: slontis Date: Wed, 11 Jan 2023 11:05:04 +1000 Subject: [PATCH 10/18] Fix NULL deference when validating FFC public key. Fixes CVE-2023-0217 When attempting to do a BN_Copy of params->p there was no NULL check. Since BN_copy does not check for NULL this is a NULL reference. As an aside BN_cmp() does do a NULL check, so there are other checks that fail because a NULL is passed. A more general check for NULL params has been added for both FFC public and private key validation instead. Reviewed-by: Matt Caswell Reviewed-by: Paul Dale Reviewed-by: Tomas Mraz --- crypto/ffc/ffc_key_validate.c | 9 +++++++++ include/internal/ffc.h | 1 + test/ffc_internal_test.c | 31 +++++++++++++++++++++++++++++++ 3 files changed, 41 insertions(+) diff --git a/crypto/ffc/ffc_key_validate.c b/crypto/ffc/ffc_key_validate.c index 9f6525a2c8..442303e4b3 100644 --- a/crypto/ffc/ffc_key_validate.c +++ b/crypto/ffc/ffc_key_validate.c @@ -24,6 +24,11 @@ int ossl_ffc_validate_public_key_partial(const FFC_PARAMS *params, BN_CTX *ctx = NULL; *ret = 0; + if (params == NULL || pub_key == NULL || params->p == NULL) { + *ret = FFC_ERROR_PASSED_NULL_PARAM; + return 0; + } + ctx = BN_CTX_new_ex(NULL); if (ctx == NULL) goto err; @@ -107,6 +112,10 @@ int ossl_ffc_validate_private_key(const BIGNUM *upper, const BIGNUM *priv, *ret = 0; + if (priv == NULL || upper == NULL) { + *ret = FFC_ERROR_PASSED_NULL_PARAM; + goto err; + } if (BN_cmp(priv, BN_value_one()) < 0) { *ret |= FFC_ERROR_PRIVKEY_TOO_SMALL; goto err; diff --git a/include/internal/ffc.h b/include/internal/ffc.h index 732514a6c2..b8b7140857 100644 --- a/include/internal/ffc.h +++ b/include/internal/ffc.h @@ -76,6 +76,7 @@ # define FFC_ERROR_NOT_SUITABLE_GENERATOR 0x08 # define FFC_ERROR_PRIVKEY_TOO_SMALL 0x10 # define FFC_ERROR_PRIVKEY_TOO_LARGE 0x20 +# define FFC_ERROR_PASSED_NULL_PARAM 0x40 /* * Finite field cryptography (FFC) domain parameters are used by DH and DSA. diff --git a/test/ffc_internal_test.c b/test/ffc_internal_test.c index 2c97293573..9f67bd29b9 100644 --- a/test/ffc_internal_test.c +++ b/test/ffc_internal_test.c @@ -510,6 +510,27 @@ static int ffc_public_validate_test(void) if (!TEST_true(ossl_ffc_validate_public_key(params, pub, &res))) goto err; + /* Fail if params is NULL */ + if (!TEST_false(ossl_ffc_validate_public_key(NULL, pub, &res))) + goto err; + if (!TEST_int_eq(FFC_ERROR_PASSED_NULL_PARAM, res)) + goto err; + res = -1; + /* Fail if pubkey is NULL */ + if (!TEST_false(ossl_ffc_validate_public_key(params, NULL, &res))) + goto err; + if (!TEST_int_eq(FFC_ERROR_PASSED_NULL_PARAM, res)) + goto err; + res = -1; + + BN_free(params->p); + params->p = NULL; + /* Fail if params->p is NULL */ + if (!TEST_false(ossl_ffc_validate_public_key(params, pub, &res))) + goto err; + if (!TEST_int_eq(FFC_ERROR_PASSED_NULL_PARAM, res)) + goto err; + ret = 1; err: DH_free(dh); @@ -567,6 +588,16 @@ static int ffc_private_validate_test(void) if (!TEST_true(ossl_ffc_validate_private_key(params->q, priv, &res))) goto err; + if (!TEST_false(ossl_ffc_validate_private_key(NULL, priv, &res))) + goto err; + if (!TEST_int_eq(FFC_ERROR_PASSED_NULL_PARAM, res)) + goto err; + res = -1; + if (!TEST_false(ossl_ffc_validate_private_key(params->q, NULL, &res))) + goto err; + if (!TEST_int_eq(FFC_ERROR_PASSED_NULL_PARAM, res)) + goto err; + ret = 1; err: DH_free(dh); -- 2.39.1 From c1b4467a7cc129a74fc5205b80a5c47556b99416 Mon Sep 17 00:00:00 2001 From: Tomas Mraz Date: Fri, 13 Jan 2023 17:57:59 +0100 Subject: [PATCH 11/18] Prevent creating DSA and DH keys without parameters through import Reviewed-by: Matt Caswell Reviewed-by: Paul Dale --- providers/implementations/keymgmt/dh_kmgmt.c | 4 ++-- providers/implementations/keymgmt/dsa_kmgmt.c | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/providers/implementations/keymgmt/dh_kmgmt.c b/providers/implementations/keymgmt/dh_kmgmt.c index 58a5fd009f..c2d87b4a7f 100644 --- a/providers/implementations/keymgmt/dh_kmgmt.c +++ b/providers/implementations/keymgmt/dh_kmgmt.c @@ -198,8 +198,8 @@ static int dh_import(void *keydata, int selection, const OSSL_PARAM params[]) if ((selection & DH_POSSIBLE_SELECTIONS) == 0) return 0; - if ((selection & OSSL_KEYMGMT_SELECT_ALL_PARAMETERS) != 0) - ok = ok && ossl_dh_params_fromdata(dh, params); + /* a key without parameters is meaningless */ + ok = ok && ossl_dh_params_fromdata(dh, params); if ((selection & OSSL_KEYMGMT_SELECT_KEYPAIR) != 0) ok = ok && ossl_dh_key_fromdata(dh, params); diff --git a/providers/implementations/keymgmt/dsa_kmgmt.c b/providers/implementations/keymgmt/dsa_kmgmt.c index 100e917167..881680c085 100644 --- a/providers/implementations/keymgmt/dsa_kmgmt.c +++ b/providers/implementations/keymgmt/dsa_kmgmt.c @@ -199,8 +199,9 @@ static int dsa_import(void *keydata, int selection, const OSSL_PARAM params[]) if ((selection & DSA_POSSIBLE_SELECTIONS) == 0) return 0; - if ((selection & OSSL_KEYMGMT_SELECT_ALL_PARAMETERS) != 0) - ok = ok && ossl_dsa_ffc_params_fromdata(dsa, params); + /* a key without parameters is meaningless */ + ok = ok && ossl_dsa_ffc_params_fromdata(dsa, params); + if ((selection & OSSL_KEYMGMT_SELECT_KEYPAIR) != 0) ok = ok && ossl_dsa_key_fromdata(dsa, params); -- 2.39.1 From fab4973801bdc11c29c4c8ccf65cf39cbc63ce9b Mon Sep 17 00:00:00 2001 From: Tomas Mraz Date: Fri, 13 Jan 2023 17:59:52 +0100 Subject: [PATCH 12/18] Do not create DSA keys without parameters by decoder Reviewed-by: Matt Caswell Reviewed-by: Paul Dale --- crypto/x509/x_pubkey.c | 24 +++++++++++++++++++ include/crypto/x509.h | 3 +++ .../encode_decode/decode_der2key.c | 2 +- 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/crypto/x509/x_pubkey.c b/crypto/x509/x_pubkey.c index bc90ddd89b..77790faa1f 100644 --- a/crypto/x509/x_pubkey.c +++ b/crypto/x509/x_pubkey.c @@ -745,6 +745,30 @@ DSA *d2i_DSA_PUBKEY(DSA **a, const unsigned char **pp, long length) return key; } +/* Called from decoders; disallows provided DSA keys without parameters. */ +DSA *ossl_d2i_DSA_PUBKEY(DSA **a, const unsigned char **pp, long length) +{ + DSA *key = NULL; + const unsigned char *data; + const BIGNUM *p, *q, *g; + + data = *pp; + key = d2i_DSA_PUBKEY(NULL, &data, length); + if (key == NULL) + return NULL; + DSA_get0_pqg(key, &p, &q, &g); + if (p == NULL || q == NULL || g == NULL) { + DSA_free(key); + return NULL; + } + *pp = data; + if (a != NULL) { + DSA_free(*a); + *a = key; + } + return key; +} + int i2d_DSA_PUBKEY(const DSA *a, unsigned char **pp) { EVP_PKEY *pktmp; diff --git a/include/crypto/x509.h b/include/crypto/x509.h index 1f00178e89..0c42730ee9 100644 --- a/include/crypto/x509.h +++ b/include/crypto/x509.h @@ -339,6 +339,9 @@ void ossl_X509_PUBKEY_INTERNAL_free(X509_PUBKEY *xpub); RSA *ossl_d2i_RSA_PSS_PUBKEY(RSA **a, const unsigned char **pp, long length); int ossl_i2d_RSA_PSS_PUBKEY(const RSA *a, unsigned char **pp); +# ifndef OPENSSL_NO_DSA +DSA *ossl_d2i_DSA_PUBKEY(DSA **a, const unsigned char **pp, long length); +# endif /* OPENSSL_NO_DSA */ # ifndef OPENSSL_NO_DH DH *ossl_d2i_DH_PUBKEY(DH **a, const unsigned char **pp, long length); int ossl_i2d_DH_PUBKEY(const DH *a, unsigned char **pp); diff --git a/providers/implementations/encode_decode/decode_der2key.c b/providers/implementations/encode_decode/decode_der2key.c index ebc2d24833..d6ad738ef3 100644 --- a/providers/implementations/encode_decode/decode_der2key.c +++ b/providers/implementations/encode_decode/decode_der2key.c @@ -374,7 +374,7 @@ static void *dsa_d2i_PKCS8(void **key, const unsigned char **der, long der_len, (key_from_pkcs8_t *)ossl_dsa_key_from_pkcs8); } -# define dsa_d2i_PUBKEY (d2i_of_void *)d2i_DSA_PUBKEY +# define dsa_d2i_PUBKEY (d2i_of_void *)ossl_d2i_DSA_PUBKEY # define dsa_free (free_key_fn *)DSA_free # define dsa_check NULL -- 2.39.1 From 7e37185582995b35f885fec9dcc3670af9ffcbef Mon Sep 17 00:00:00 2001 From: Tomas Mraz Date: Fri, 13 Jan 2023 18:46:15 +0100 Subject: [PATCH 13/18] Add test for DSA pubkey without param import and check Reviewed-by: Matt Caswell Reviewed-by: Paul Dale --- test/recipes/91-test_pkey_check.t | 48 ++++++++++++++---- .../91-test_pkey_check_data/dsapub.pem | 12 +++++ .../dsapub_noparam.der | Bin 0 -> 108 bytes 3 files changed, 49 insertions(+), 11 deletions(-) create mode 100644 test/recipes/91-test_pkey_check_data/dsapub.pem create mode 100644 test/recipes/91-test_pkey_check_data/dsapub_noparam.der --- openssl-3.0.1/test/recipes/91-test_pkey_check.t 2023-02-08 13:43:56.228487948 +0100 +++ openssl-3.0.7/test/recipes/91-test_pkey_check.t 2023-02-08 12:47:13.531027540 +0100 @@ -1,5 +1,5 @@ #! /usr/bin/env perl -# Copyright 2017-2021 The OpenSSL Project Authors. All Rights Reserved. +# Copyright 2017-2022 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 @@ -11,24 +11,37 @@ use warnings; use File::Spec; -use OpenSSL::Test qw/:DEFAULT data_file/; +use OpenSSL::Test qw/:DEFAULT data_file with/; use OpenSSL::Test::Utils; -sub check_key { +sub pkey_check { my $f = shift; + my $pubcheck = shift; + my @checkopt = ('-check'); + + @checkopt = ('-pubcheck', '-pubin') if $pubcheck; - return run(app(['openssl', 'pkey', '-check', '-text', + return run(app(['openssl', 'pkey', @checkopt, '-text', '-in', $f])); } -sub check_key_notok { +sub check_key { my $f = shift; - my $str = "$f should fail validation"; + my $should_fail = shift; + my $pubcheck = shift; + my $str; + + + $str = "$f should fail validation" if $should_fail; + $str = "$f should pass validation" unless $should_fail; $f = data_file($f); if ( -s $f ) { - ok(!check_key($f), $str); + with({ exit_checker => sub { return shift == $should_fail; } }, + sub { + ok(pkey_check($f, $pubcheck), $str); + }); } else { fail("Missing file $f"); } @@ -36,26 +49,54 @@ setup("test_pkey_check"); -my @tests = (); +my @negative_tests = (); -push(@tests, ( +push(@negative_tests, ( # For EC keys the range for the secret scalar `k` is `1 <= k <= n-1` "ec_p256_bad_0.pem", # `k` set to `n` (equivalent to `0 mod n`, invalid) "ec_p256_bad_1.pem", # `k` set to `n+1` (equivalent to `1 mod n`, invalid) )) unless disabled("ec"); -push(@tests, ( +push(@negative_tests, ( # For SM2 keys the range for the secret scalar `k` is `1 <= k < n-1` "sm2_bad_neg1.pem", # `k` set to `n-1` (invalid, because SM2 range) "sm2_bad_0.pem", # `k` set to `n` (equivalent to `0 mod n`, invalid) "sm2_bad_1.pem", # `k` set to `n+1` (equivalent to `1 mod n`, invalid) )) unless disabled("sm2"); +my @positive_tests = (); + +my @negative_pubtests = (); + +push(@negative_pubtests, ( + "dsapub_noparam.der" + )) unless disabled("dsa"); + +my @positive_pubtests = (); + +push(@positive_pubtests, ( + "dsapub.pem" + )) unless disabled("dsa"); + plan skip_all => "No tests within the current enabled feature set" - unless @tests; + unless @negative_tests && @positive_tests + && @negative_pubtests && @positive_pubtests; + +plan tests => scalar(@negative_tests) + scalar(@positive_tests) + + scalar(@negative_pubtests) + scalar(@positive_pubtests); + +foreach my $t (@negative_tests) { + check_key($t, 1, 0); +} + +foreach my $t (@positive_tests) { + check_key($t, 0, 0); +} -plan tests => scalar(@tests); +foreach my $t (@negative_pubtests) { + check_key($t, 1, 1); +} -foreach my $t (@tests) { - check_key_notok($t); +foreach my $t (@positive_pubtests) { + check_key($t, 0, 1); } diff --git a/test/recipes/91-test_pkey_check_data/dsapub.pem b/test/recipes/91-test_pkey_check_data/dsapub.pem new file mode 100644 index 0000000000..0ff4bd83ed --- /dev/null +++ b/test/recipes/91-test_pkey_check_data/dsapub.pem @@ -0,0 +1,12 @@ +-----BEGIN PUBLIC KEY----- +MIIBvzCCATQGByqGSM44BAEwggEnAoGBAIjbXpOVVciVNuagg26annKkghIIZFI4 +4WdMomnV+I/oXyxHbZTBBBpW9xy/E1+yMjbp4GmX+VxyDj3WxUWxXllzL+miEkzD +9Xz638VzIBhjFbMvk1/N4kS4bKVUd9yk7HfvYzAdnRphk0WI+RoDiDrBNPPxSoQD +CEWgvwgsLIDhAh0A6dbz1IQpQwGF4+Ca28x6OO+UfJJv3ggeZ++fNwKBgQCA9XKV +lRrTY8ALBxS0KbZjpaIXuUj5nr3i1lIDyP3ISksDF0ekyLtn6eK9VijX6Pm65Np+ +4ic9Nr5WKLKhPaUSpLNRx1gDqo3sd92hYgiEUifzEuhLYfK/CsgFED+l2hDXtJUq +bISNSHVwI5lsyNXLu7HI1Fk8F5UO3LqsboFAngOBhAACgYATxFY89nEYcUhgHGgr +YDHhXBQfMKnTKYdvon4DN7WQ9ip+t4VUsLpTD1ZE9zrM2R/B04+8C6KGoViwyeER +kS4dxWOkX71x4X2DlNpYevcR53tNcTDqmMD7YKfDDmrb0lftMyfW8aESaiymVMys +DRjhKHBjdo0rZeSM8DAk3ctrXA== +-----END PUBLIC KEY----- diff --git a/test/recipes/91-test_pkey_check_data/dsapub_noparam.der b/test/recipes/91-test_pkey_check_data/dsapub_noparam.der new file mode 100644 index 0000000000000000000000000000000000000000..b8135f1ca94da914b6829421e0c13f6daa731862 GIT binary patch literal 108 zcmXpIGT>xm*J|@PXTieE%*wz71|F5F-Nv0Bz9(=Kufz literal 0 HcmV?d00001 -- 2.39.1 From 2ad9928170768653d19d81881deabc5f9c1665c0 Mon Sep 17 00:00:00 2001 From: Tomas Mraz Date: Fri, 3 Feb 2023 14:57:04 +0100 Subject: [PATCH 18/18] Internaly declare the DSA type for no-deprecated builds Reviewed-by: Hugo Landau Reviewed-by: Richard Levitte (cherry picked from commit 7a21a1b5fa2dac438892cf3292d1f9c445d870d9) --- include/crypto/types.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/include/crypto/types.h b/include/crypto/types.h index 0d81404091..0a75f03a3f 100644 --- a/include/crypto/types.h +++ b/include/crypto/types.h @@ -20,6 +20,9 @@ typedef struct rsa_meth_st RSA_METHOD; typedef struct ec_key_st EC_KEY; typedef struct ec_key_method_st EC_KEY_METHOD; # endif +# ifndef OPENSSL_NO_DSA +typedef struct dsa_st DSA; +# endif # endif # ifndef OPENSSL_NO_EC -- 2.39.1