Commit fff1da43 authored by Nicola Tuveri's avatar Nicola Tuveri
Browse files

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 e3ab8cc4 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: default avatarTim Hudson <tjh@openssl.org>
Reviewed-by: default avatarMatthias St. Pierre <Matthias.St.Pierre@ncp-e.com>
(Merged from https://github.com/openssl/openssl/pull/7135)
parent 788d2fa0

CHANGES

+4 −1
Original line number Diff line number Diff line
@@ -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]

crypto/ec/ec_lcl.h

+1 −2
Original line number Diff line number Diff line
@@ -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

crypto/ec/ec_lib.c

+29 −12
Original line number Diff line number Diff line
@@ -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);



    /*

    /*-

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

        if (BN_is_odd(&group->order))

            return ec_precompute_mont_data(group);

    }



        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;