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 <tjh@openssl.org>
Reviewed-by: Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com>
(Merged from https://github.com/openssl/openssl/pull/7135)

diff --git a/CHANGES b/CHANGES
index bfcd7b3dca3064e2cc358583a23ddb708b116ac9..b574074728748a42ae046b56fe858649e3f5c1fa 100644 (file)
--- 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 969fd147ef936909d43c22558297c1dfe4201d4c..2d604fab7c588c8e79a6d7aa1b2e3e22d1a353d1 100644 (file)
--- 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 933745248d8d63659b13c4d233bf478eee189c36..df56484b5ee27a6450df0bbebd055f0f083ce67e 100644 (file)
--- 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;