From 23985bac83fd50c8e29431009302b5442f985096 Mon Sep 17 00:00:00 2001

From: slontis <shane.lontis@oracle.com>

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 <matt@openssl.org>

Reviewed-by: Paul Dale <pauli@openssl.org>

Reviewed-by: Tomas Mraz <tomas@openssl.org>

---

 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 <tomas@openssl.org>

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 <matt@openssl.org>

Reviewed-by: Paul Dale <pauli@openssl.org>

---

 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) {

         int include_private =

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) {

         int include_private =

             selection & OSSL_KEYMGMT_SELECT_PRIVATE_KEY ? 1 : 0;

-- 

2.39.1

From fab4973801bdc11c29c4c8ccf65cf39cbc63ce9b Mon Sep 17 00:00:00 2001

From: Tomas Mraz <tomas@openssl.org>

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 <matt@openssl.org>

Reviewed-by: Paul Dale <pauli@openssl.org>

---

 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 <tomas@openssl.org>

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 <matt@openssl.org>

Reviewed-by: Paul Dale <pauli@openssl.org>

---

 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

diff --git a/test/recipes/91-test_pkey_check.t b/test/recipes/91-test_pkey_check.t

index 612a3e3d6c..015d7805db 100644

--- a/test/recipes/91-test_pkey_check.t

+++ b/test/recipes/91-test_pkey_check.t

@@ -11,19 +11,24 @@ use strict;

 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 pkey_check {

     my $f = shift;

+    my $pubcheck = shift;

+    my @checkopt = ('-check');

-    return run(app(['openssl', 'pkey', '-check', '-text',

+    @checkopt = ('-pubcheck', '-pubin') if $pubcheck;

+

+    return run(app(['openssl', 'pkey', @checkopt, '-text',

                     '-in', $f]));

 }

 sub check_key {

     my $f = shift;

     my $should_fail = shift;

+    my $pubcheck = shift;

     my $str;

@@ -33,11 +38,10 @@ sub check_key {

     $f = data_file($f);

     if ( -s $f ) {

-        if ($should_fail) {

-            ok(!pkey_check($f), $str);

-        } else {

-            ok(pkey_check($f), $str);

-        }

+        with({ exit_checker => sub { return shift == $should_fail; } },

+            sub {

+                ok(pkey_check($f, $pubcheck), $str);

+            });

     } else {

         fail("Missing file $f");

     }

@@ -66,15 +70,37 @@ push(@positive_tests, (

     "dhpkey.pem"

     )) unless disabled("dh");

+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 @negative_tests && @positive_tests;

+    unless @negative_tests && @positive_tests

+           && @negative_pubtests && @positive_pubtests;

-plan tests => scalar(@negative_tests) + scalar(@positive_tests);

+plan tests => scalar(@negative_tests) + scalar(@positive_tests)

+              + scalar(@negative_pubtests) + scalar(@positive_pubtests);

 foreach my $t (@negative_tests) {

-    check_key($t, 1);

+    check_key($t, 1, 0);

 }

 foreach my $t (@positive_tests) {

-    check_key($t, 0);

+    check_key($t, 0, 0);

+}

+

+foreach my $t (@negative_pubtests) {

+    check_key($t, 1, 1);

+}

+

+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

U3o&W4F|x9<gY>|F5F-Nv0Bz9(=Kufz

literal 0

HcmV?d00001

-- 

2.39.1

From 2ad9928170768653d19d81881deabc5f9c1665c0 Mon Sep 17 00:00:00 2001

From: Tomas Mraz <tomas@openssl.org>

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 <hlandau@openssl.org>

Reviewed-by: Richard Levitte <levitte@openssl.org>

(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