Skip to content

Commit

Permalink
Add multiple fixes for ffc key generation using invalid p,q,g paramet…
Browse files Browse the repository at this point in the history
…ers.

Fixes openssl#11864

- The dsa keygen assumed valid p, q, g values were being passed. If this is not correct then it is
  possible that dsa keygen can either hang or segfault.
  The fix was to do a partial validation of p, q, and g inside the keygen.
- Fixed a potential double free in the dsa keypair test in the case when in failed (It should never fail!).
  It freed internal object members without setting them to NULL.
- Changed the FFC key validation to accept 1024 bit keys in non fips mode.
- Added tests that use both the default provider & fips provider to test these cases.

Reviewed-by: Dmitry Belyavskiy <[email protected]>
(Merged from openssl#12176)
  • Loading branch information
slontis committed Jul 9, 2020
1 parent eae4a00 commit 63794b0
Show file tree
Hide file tree
Showing 9 changed files with 352 additions and 9 deletions.
4 changes: 4 additions & 0 deletions crypto/dh/dh_key.c
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,10 @@ static int generate_key(DH *dh)
} else
#endif
{
/* Do a partial check for invalid p, q, g */
if (!ffc_params_simple_validate(dh->libctx, &dh->params,
FFC_PARAM_TYPE_DH))
goto err;
/*
* For FFC FIPS 186-4 keygen
* security strength s = 112,
Expand Down
7 changes: 7 additions & 0 deletions crypto/dsa/dsa_key.c
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,11 @@ static int dsa_keygen(DSA *dsa, int pairwise_test)
priv_key = dsa->priv_key;
}

/* Do a partial check for invalid p, q, g */
if (!ffc_params_simple_validate(dsa->libctx, &dsa->params,
FFC_PARAM_TYPE_DSA))
goto err;

/*
* For FFC FIPS 186-4 keygen
* security strength s = 112,
Expand Down Expand Up @@ -110,6 +115,8 @@ static int dsa_keygen(DSA *dsa, int pairwise_test)
if (!ok) {
BN_free(dsa->pub_key);
BN_clear_free(dsa->priv_key);
dsa->pub_key = NULL;
dsa->priv_key = NULL;
BN_CTX_free(ctx);
return ok;
}
Expand Down
11 changes: 10 additions & 1 deletion crypto/ffc/ffc_params_generate.c
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,11 @@
*/
static int ffc_validate_LN(size_t L, size_t N, int type)
{
#ifndef FIPS_MODULE
if (L == 1024 && N == 160)
return 80;
#endif

if (type == FFC_PARAM_TYPE_DH) {
/* Valid DH L,N parameters from SP800-56Ar3 5.5.1 Table 1 */
if (L == 2048 && (N == 224 || N == 256))
Expand Down Expand Up @@ -498,6 +503,7 @@ int ffc_params_FIPS186_4_gen_verify(OPENSSL_CTX *libctx, FFC_PARAMS *params,
EVP_MD *md = NULL;
int verify = (mode == FFC_PARAM_MODE_VERIFY);
unsigned int flags = verify ? params->flags : 0;
const char *def_name;

*res = 0;

Expand All @@ -506,7 +512,10 @@ int ffc_params_FIPS186_4_gen_verify(OPENSSL_CTX *libctx, FFC_PARAMS *params,
} else {
if (N == 0)
N = (L >= 2048 ? SHA256_DIGEST_LENGTH : SHA_DIGEST_LENGTH) * 8;
md = EVP_MD_fetch(libctx, default_mdname(N), NULL);
def_name = default_mdname(N);
if (def_name == NULL)
goto err;
md = EVP_MD_fetch(libctx, def_name, NULL);
}
if (md == NULL)
goto err;
Expand Down
26 changes: 26 additions & 0 deletions crypto/ffc/ffc_params_validate.c
Original file line number Diff line number Diff line change
Expand Up @@ -78,3 +78,29 @@ int ffc_params_FIPS186_2_validate(OPENSSL_CTX *libctx, const FFC_PARAMS *params,
FFC_PARAM_MODE_VERIFY, type,
L, N, res, cb);
}

/*
* This does a simple check of L and N and partial g.
* It makes no attempt to do a full validation of p, q or g since these require
* extra parameters such as the digest and seed, which may not be available for
* this test.
*/
int ffc_params_simple_validate(OPENSSL_CTX *libctx, FFC_PARAMS *params, int type)
{
int ret, res = 0;
int save_gindex;
unsigned int save_flags;

if (params == NULL)
return 0;

save_flags = params->flags;
save_gindex = params->gindex;
params->flags = FFC_PARAM_FLAG_VALIDATE_G;
params->gindex = FFC_UNVERIFIABLE_GINDEX;

ret = ffc_params_FIPS186_4_validate(libctx, params, type, &res, NULL);
params->flags = save_flags;
params->gindex = save_gindex;
return ret != FFC_PARAM_RET_STATUS_FAILED;
}
1 change: 1 addition & 0 deletions include/internal/ffc.h
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ int ffc_params_FIPS186_2_gen_verify(OPENSSL_CTX *libctx, FFC_PARAMS *params,
int mode, int type, size_t L, size_t N,
int *res, BN_GENCB *cb);

int ffc_params_simple_validate(OPENSSL_CTX *libctx, FFC_PARAMS *params, int type);
int ffc_params_FIPS186_4_validate(OPENSSL_CTX *libctx, const FFC_PARAMS *params,
int type, int *res, BN_GENCB *cb);
int ffc_params_FIPS186_2_validate(OPENSSL_CTX *libctx, const FFC_PARAMS *params,
Expand Down
6 changes: 5 additions & 1 deletion test/build.info
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ IF[{- !$disabled{tests} -}]
destest mdc2test \
enginetest exptest \
evp_pkey_provided_test evp_test evp_extra_test evp_extra_test2 \
evp_fetch_prov_test acvp_test \
evp_fetch_prov_test acvp_test evp_libctx_test \
v3nametest v3ext \
evp_pkey_provided_test evp_test evp_extra_test evp_extra_test2 \
evp_fetch_prov_test v3nametest v3ext \
Expand Down Expand Up @@ -141,6 +141,10 @@ IF[{- !$disabled{tests} -}]
INCLUDE[evp_extra_test2]=../include ../apps/include
DEPEND[evp_extra_test2]=../libcrypto libtestutil.a

SOURCE[evp_libctx_test]=evp_libctx_test.c
INCLUDE[evp_libctx_test]=../include ../apps/include
DEPEND[evp_libctx_test]=../libcrypto.a libtestutil.a

SOURCE[evp_fetch_prov_test]=evp_fetch_prov_test.c
INCLUDE[evp_fetch_prov_test]=../include ../apps/include
DEPEND[evp_fetch_prov_test]=../libcrypto libtestutil.a
Expand Down
253 changes: 253 additions & 0 deletions test/evp_libctx_test.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,253 @@
/*
* Copyright 2020 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
* in the file LICENSE in the source distribution or at
* https://www.openssl.org/source/license.html
*/

/*
* These tests are setup to load null into the default library context.
* Any tests are expected to use the created 'libctx' to find algorithms.
* The framework runs the tests twice using the 'default' provider or
* 'fips' provider as inputs.
*/

/*
* DSA/DH low level APIs are deprecated for public use, but still ok for
* internal use.
*/
#include "internal/deprecated.h"
#include <openssl/evp.h>
#include <openssl/provider.h>
#include <openssl/dsa.h>
#include "testutil.h"
#include "internal/nelem.h"
#include "crypto/bn_dh.h" /* _bignum_ffdhe2048_p */

static OPENSSL_CTX *libctx = NULL;
static OSSL_PROVIDER *nullprov = NULL;
static OSSL_PROVIDER *libprov = NULL;

typedef enum OPTION_choice {
OPT_ERR = -1,
OPT_EOF = 0,
OPT_CONFIG_FILE,
OPT_PROVIDER_NAME,
OPT_TEST_ENUM
} OPTION_CHOICE;

const OPTIONS *test_get_options(void)
{
static const OPTIONS test_options[] = {
OPT_TEST_OPTIONS_DEFAULT_USAGE,
{ "config", OPT_CONFIG_FILE, '<',
"The configuration file to use for the libctx" },
{ "provider", OPT_PROVIDER_NAME, 's',
"The provider to load (The default value is 'default'" },
{ NULL }
};
return test_options;
}

#if !defined(OPENSSL_NO_DSA) || !defined(OPENSSL_NO_DH)
static const char *getname(int id)
{
const char *name[] = {"p", "q", "g" };

if (id >= 0 && id < 3)
return name[id];
return "?";
}
#endif

#ifndef OPENSSL_NO_DSA

static int test_dsa_param_keygen(int tstid)
{
int ret = 0;
int expected;
EVP_PKEY_CTX *gen_ctx = NULL;
EVP_PKEY *pkey_parm = NULL;
EVP_PKEY *pkey = NULL;
DSA *dsa = NULL;
int pind, qind, gind;
BIGNUM *p = NULL, *q = NULL, *g = NULL;

/*
* Just grab some fixed dh p, q, g values for testing,
* these 'safe primes' should not be used normally for dsa *.
*/
static const BIGNUM *bn[] = {
&_bignum_dh2048_256_p, &_bignum_dh2048_256_q, &_bignum_dh2048_256_g
};

/*
* These tests are using bad values for p, q, g by reusing the values.
* A value of 0 uses p, 1 uses q and 2 uses g.
* There are 27 different combinations, with only the 1 valid combination.
*/
pind = tstid / 9;
qind = (tstid / 3) % 3;
gind = tstid % 3;
expected = (pind == 0 && qind == 1 && gind == 2);

TEST_note("Testing with (p, q, g) = (%s, %s, %s)\n", getname(pind),
getname(qind), getname(gind));

if (!TEST_ptr(pkey_parm = EVP_PKEY_new())
|| !TEST_ptr(dsa = DSA_new())
|| !TEST_ptr(p = BN_dup(bn[pind]))
|| !TEST_ptr(q = BN_dup(bn[qind]))
|| !TEST_ptr(g = BN_dup(bn[gind]))
|| !TEST_true(DSA_set0_pqg(dsa, p, q, g)))
goto err;
p = q = g = NULL;

if (!TEST_true(EVP_PKEY_assign_DSA(pkey_parm, dsa)))
goto err;
dsa = NULL;

if (!TEST_ptr(gen_ctx = EVP_PKEY_CTX_new_from_pkey(libctx, pkey_parm, NULL))
|| !TEST_int_gt(EVP_PKEY_keygen_init(gen_ctx), 0)
|| !TEST_int_eq(EVP_PKEY_keygen(gen_ctx, &pkey), expected))
goto err;
ret = 1;
err:
EVP_PKEY_free(pkey);
EVP_PKEY_CTX_free(gen_ctx);
EVP_PKEY_free(pkey_parm);
DSA_free(dsa);
BN_free(g);
BN_free(q);
BN_free(p);
return ret;
}
#endif /* OPENSSL_NO_DSA */

#ifndef OPENSSL_NO_DH
static int do_dh_param_keygen(int tstid, const BIGNUM **bn)
{
int ret = 0;
int expected;
EVP_PKEY_CTX *gen_ctx = NULL;
EVP_PKEY *pkey_parm = NULL;
EVP_PKEY *pkey = NULL;
DH *dh = NULL;
int pind, qind, gind;
BIGNUM *p = NULL, *q = NULL, *g = NULL;

/*
* These tests are using bad values for p, q, g by reusing the values.
* A value of 0 uses p, 1 uses q and 2 uses g.
* There are 27 different combinations, with only the 1 valid combination.
*/
pind = tstid / 9;
qind = (tstid / 3) % 3;
gind = tstid % 3;
expected = (pind == 0 && qind == 1 && gind == 2);

TEST_note("Testing with (p, q, g) = (%s, %s, %s)", getname(pind),
getname(qind), getname(gind));

if (!TEST_ptr(pkey_parm = EVP_PKEY_new())
|| !TEST_ptr(dh = DH_new())
|| !TEST_ptr(p = BN_dup(bn[pind]))
|| !TEST_ptr(q = BN_dup(bn[qind]))
|| !TEST_ptr(g = BN_dup(bn[gind]))
|| !TEST_true(DH_set0_pqg(dh, p, q, g)))
goto err;
p = q = g = NULL;

if (!TEST_true(EVP_PKEY_assign_DH(pkey_parm, dh)))
goto err;
dh = NULL;

if (!TEST_ptr(gen_ctx = EVP_PKEY_CTX_new_from_pkey(libctx, pkey_parm, NULL))
|| !TEST_int_gt(EVP_PKEY_keygen_init(gen_ctx), 0)
|| !TEST_int_eq(EVP_PKEY_keygen(gen_ctx, &pkey), expected))
goto err;
ret = 1;
err:
EVP_PKEY_free(pkey);
EVP_PKEY_CTX_free(gen_ctx);
EVP_PKEY_free(pkey_parm);
DH_free(dh);
BN_free(g);
BN_free(q);
BN_free(p);
return ret;
}

/*
* Note that we get the fips186-4 path being run for most of these cases since
* the internal code will detect that the p, q, g does not match a safe prime
* group (Except for when tstid = 5, which sets the correct p, q, g)
*/
static int test_dh_safeprime_param_keygen(int tstid)
{
static const BIGNUM *bn[] = {
&_bignum_ffdhe2048_p, &_bignum_ffdhe2048_q, &_bignum_const_2
};
return do_dh_param_keygen(tstid, bn);
}

#endif /* OPENSSL_NO_DH */

int setup_tests(void)
{
const char *prov_name = "default";
char *config_file = NULL;
OPTION_CHOICE o;

while ((o = opt_next()) != OPT_EOF) {
switch (o) {
case OPT_PROVIDER_NAME:
prov_name = opt_arg();
break;
case OPT_CONFIG_FILE:
config_file = opt_arg();
break;
case OPT_TEST_CASES:
break;
default:
case OPT_ERR:
return 0;
}
}

nullprov = OSSL_PROVIDER_load(NULL, "null");
if (!TEST_ptr(nullprov))
return 0;

libctx = OPENSSL_CTX_new();

if (!TEST_ptr(libctx))
return 0;

if (config_file != NULL) {
if (!TEST_true(OPENSSL_CTX_load_config(libctx, config_file)))
return 0;
}

libprov = OSSL_PROVIDER_load(libctx, prov_name);
if (!TEST_ptr(libprov))
return 0;

#ifndef OPENSSL_NO_DSA
ADD_ALL_TESTS(test_dsa_param_keygen, 3 * 3 * 3);
#endif
#ifndef OPENSSL_NO_DH
ADD_ALL_TESTS(test_dh_safeprime_param_keygen, 3 * 3 * 3);
#endif
return 1;
}

void cleanup_tests(void)
{
OSSL_PROVIDER_unload(libprov);
OPENSSL_CTX_free(libctx);
OSSL_PROVIDER_unload(nullprov);
}
7 changes: 0 additions & 7 deletions test/ffc_internal_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -399,13 +399,6 @@ static int ffc_params_fips186_2_gen_validate_test(void)
FFC_PARAM_TYPE_DH,
&res, NULL)))
goto err;
/* FIPS 186-4 L,N pair test will fail for DH */
if (!TEST_false(ffc_params_FIPS186_4_validate(NULL, &params,
FFC_PARAM_TYPE_DH,
&res, NULL)))
goto err;
if (!TEST_int_eq(res, FFC_CHECK_BAD_LN_PAIR))
goto err;

/*
* The fips186-2 generation should produce a different q compared to
Expand Down
Loading

0 comments on commit 63794b0

Please sign in to comment.