From 63794b048cbe46ac9abb883df4dd703f522e4643 Mon Sep 17 00:00:00 2001 From: Shane Lontis Date: Thu, 9 Jul 2020 13:43:10 +1000 Subject: Add multiple fixes for ffc key generation using invalid p,q,g parameters. Fixes #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 (Merged from https://github.com/openssl/openssl/pull/12176) --- crypto/dh/dh_key.c | 4 + crypto/dsa/dsa_key.c | 7 ++ crypto/ffc/ffc_params_generate.c | 11 +- crypto/ffc/ffc_params_validate.c | 26 ++++ include/internal/ffc.h | 1 + test/build.info | 6 +- test/evp_libctx_test.c | 253 ++++++++++++++++++++++++++++++++++++++ test/ffc_internal_test.c | 7 -- test/recipes/30-test_evp_libctx.t | 46 +++++++ 9 files changed, 352 insertions(+), 9 deletions(-) create mode 100644 test/evp_libctx_test.c create mode 100644 test/recipes/30-test_evp_libctx.t diff --git a/crypto/dh/dh_key.c b/crypto/dh/dh_key.c index 5d2acca25c..3b4da19cd2 100644 --- a/crypto/dh/dh_key.c +++ b/crypto/dh/dh_key.c @@ -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, diff --git a/crypto/dsa/dsa_key.c b/crypto/dsa/dsa_key.c index 7bd9c5ff2e..b537ec0b3c 100644 --- a/crypto/dsa/dsa_key.c +++ b/crypto/dsa/dsa_key.c @@ -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, @@ -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; } diff --git a/crypto/ffc/ffc_params_generate.c b/crypto/ffc/ffc_params_generate.c index 325eb6768f..8a0b77e7f8 100644 --- a/crypto/ffc/ffc_params_generate.c +++ b/crypto/ffc/ffc_params_generate.c @@ -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)) @@ -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; @@ -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; diff --git a/crypto/ffc/ffc_params_validate.c b/crypto/ffc/ffc_params_validate.c index f3df0c2b39..821ff3e88a 100644 --- a/crypto/ffc/ffc_params_validate.c +++ b/crypto/ffc/ffc_params_validate.c @@ -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; +} diff --git a/include/internal/ffc.h b/include/internal/ffc.h index 2ed5d72c5c..b352b8d345 100644 --- a/include/internal/ffc.h +++ b/include/internal/ffc.h @@ -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, diff --git a/test/build.info b/test/build.info index 88b35d4d3c..ed547d1488 100644 --- a/test/build.info +++ b/test/build.info @@ -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 \ @@ -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 diff --git a/test/evp_libctx_test.c b/test/evp_libctx_test.c new file mode 100644 index 0000000000..77054f93a2 --- /dev/null +++ b/test/evp_libctx_test.c @@ -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 +#include +#include +#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); +} diff --git a/test/ffc_internal_test.c b/test/ffc_internal_test.c index 632cead926..1acc342f6e 100644 --- a/test/ffc_internal_test.c +++ b/test/ffc_internal_test.c @@ -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, ¶ms, - 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 diff --git a/test/recipes/30-test_evp_libctx.t b/test/recipes/30-test_evp_libctx.t new file mode 100644 index 0000000000..8fcc71a1cd --- /dev/null +++ b/test/recipes/30-test_evp_libctx.t @@ -0,0 +1,46 @@ +#! /usr/bin/env perl +# 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 + +use strict; +use warnings; + +use OpenSSL::Test qw(:DEFAULT bldtop_dir srctop_dir srctop_file bldtop_file); +use OpenSSL::Test::Utils; + +BEGIN { + setup("test_evp_libctx"); +} + +my $no_fips = disabled('fips') || ($ENV{NO_FIPS} // 0); + +use lib srctop_dir('Configurations'); +use lib bldtop_dir('.'); +use platform; + +my $infile = bldtop_file('providers', platform->dso('fips')); +# If no fips then run the test with no extra arguments. +my @test_args = ( ); + +plan tests => + ($no_fips ? 0 : 1) # FIPS install test + + 1; + +unless ($no_fips) { + @test_args = ("-config", srctop_file("test","fips.cnf"), + "-provider", "fips"); + + ok(run(app(['openssl', 'fipsinstall', + '-out', bldtop_file('providers', 'fipsmodule.cnf'), + '-module', $infile, + '-provider_name', 'fips', '-mac_name', 'HMAC', + '-macopt', 'digest:SHA256', '-macopt', 'hexkey:00', + '-section_name', 'fips_sect'])), + "fipsinstall"); +} + +ok(run(test(["evp_libctx_test", @test_args])), "running evp_libctx_test"); -- cgit v1.2.3