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>
openssl · Feb 3, 2023 · 23985ba · 23985ba
1 parent 67813d8
commit 23985ba
Show file tree

Hide file tree

Showing 3 changed files with 41 additions and 0 deletions.
diff --git 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
@@ -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
@@ -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);