From fff1da43be2236995cdf5ef2f3e2a51be232ba85 Mon Sep 17 00:00:00 2001 From: Nicola Tuveri Date: Fri, 7 Sep 2018 00:44:36 +0300 Subject: [PATCH] Access `group->mont_data` conditionally in EC_GROUP_set_generator() It appears that, in FIPS mode, `ec_precompute_mont_data()` always failed but the error was ignored until commit e3ab8cc from #6810. The actual problem lies in the fact that access to the `mont_data` field of an `EC_GROUP` struct should always be guarded by an `EC_GROUP_VERSION(group)` check to avoid OOB accesses, because `group` might come from the FIPS module, which does not define the `mont_data` field inside the EC_GROUP structure. This commit adds the required check before any access to `group->mont_data` in `EC_GROUP_set_generator()`. Fixes #7127 Reviewed-by: Tim Hudson Reviewed-by: Matthias St. Pierre (Merged from https://github.com/openssl/openssl/pull/7135) --- CHANGES | 5 ++++- crypto/ec/ec_lcl.h | 3 +-- crypto/ec/ec_lib.c | 41 +++++++++++++++++++++++++++++------------ 3 files changed, 34 insertions(+), 15 deletions(-) diff --git a/CHANGES b/CHANGES index bfcd7b3dca..b574074728 100644 --- a/CHANGES +++ b/CHANGES @@ -9,7 +9,10 @@ Changes between 1.0.2p and 1.0.2q [xx XXX xxxx] - *) + *) Resolve a compatibility issue in EC_GROUP handling with the FIPS Object + Module, accidentally introduced while backporting security fixes from the + development branch and hindering the use of ECC in FIPS mode. + [Nicola Tuveri] Changes between 1.0.2o and 1.0.2p [14 Aug 2018] diff --git a/crypto/ec/ec_lcl.h b/crypto/ec/ec_lcl.h index 969fd147ef..2d604fab7c 100644 --- a/crypto/ec/ec_lcl.h +++ b/crypto/ec/ec_lcl.h @@ -214,7 +214,7 @@ struct ec_group_st { int asn1_flag; /* flag to control the asn1 encoding */ /* * Kludge: upper bit of ans1_flag is used to denote structure - * version. Is set, then last field is present. This is done + * version. If set, then last field is present. This is done * for interoperation with FIPS code. */ #define EC_GROUP_ASN1_FLAG_MASK 0x7fffffff @@ -549,7 +549,6 @@ void ec_GFp_nistp_points_make_affine_internal(size_t num, void *point_array, void ec_GFp_nistp_recode_scalar_bits(unsigned char *sign, unsigned char *digit, unsigned char in); #endif -int ec_precompute_mont_data(EC_GROUP *); #ifdef ECP_NISTZ256_ASM /** Returns GFp methods using montgomery multiplication, with x86-64 optimized diff --git a/crypto/ec/ec_lib.c b/crypto/ec/ec_lib.c index 933745248d..df56484b5e 100644 --- a/crypto/ec/ec_lib.c +++ b/crypto/ec/ec_lib.c @@ -70,6 +70,10 @@ const char EC_version[] = "EC" OPENSSL_VERSION_PTEXT; +/* local function prototypes */ + +static int ec_precompute_mont_data(EC_GROUP *group); + /* functions for EC_GROUP objects */ EC_GROUP *EC_GROUP_new(const EC_METHOD *meth) @@ -318,17 +322,25 @@ int EC_GROUP_set_generator(EC_GROUP *group, const EC_POINT *generator, } else BN_zero(&group->cofactor); - /* - * Some groups have an order with - * factors of two, which makes the Montgomery setup fail. - * |group->mont_data| will be NULL in this case. + /*- + * Access to the `mont_data` field of an EC_GROUP struct should always be + * guarded by an EC_GROUP_VERSION(group) check to avoid OOB accesses, as the + * group might come from the FIPS module, which does not define the + * `mont_data` field inside the EC_GROUP structure. */ - if (BN_is_odd(&group->order)) { - return ec_precompute_mont_data(group); + if (EC_GROUP_VERSION(group)) { + /*- + * Some groups have an order with + * factors of two, which makes the Montgomery setup fail. + * |group->mont_data| will be NULL in this case. + */ + if (BN_is_odd(&group->order)) + return ec_precompute_mont_data(group); + + BN_MONT_CTX_free(group->mont_data); + group->mont_data = NULL; } - BN_MONT_CTX_free(group->mont_data); - group->mont_data = NULL; return 1; } @@ -1098,18 +1110,23 @@ int EC_GROUP_have_precompute_mult(const EC_GROUP *group) * been performed */ } -/* +/*- * ec_precompute_mont_data sets |group->mont_data| from |group->order| and * returns one on success. On error it returns zero. + * + * Note: this function must be called only after verifying that + * EC_GROUP_VERSION(group) returns true. + * The reason for this is that access to the `mont_data` field of an EC_GROUP + * struct should always be guarded by an EC_GROUP_VERSION(group) check to avoid + * OOB accesses, as the group might come from the FIPS module, which does not + * define the `mont_data` field inside the EC_GROUP structure. */ +static int ec_precompute_mont_data(EC_GROUP *group) { BN_CTX *ctx = BN_CTX_new(); int ret = 0; - if (!EC_GROUP_VERSION(group)) - goto err; - if (group->mont_data) { BN_MONT_CTX_free(group->mont_data); group->mont_data = NULL; -- 2.25.1