Remove |EC_POINT::Z_is_one|.

Having |Z_is_one| be out of sync with |Z| could potentially be a very
bad thing, and in the past there have been multiple bugs of this sort,
including one currently in p256-x86_64.c (type confusion: Montgomery-
encoded vs unencoded). Avoid the issue entirely by getting rid of
|Z_is_one|.

Change-Id: Icb5aa0342df41d6bc443f15f952734295d0ee4ba
Reviewed-on: https://boringssl-review.googlesource.com/6576
Reviewed-by: David Benjamin <davidben@google.com>
diff --git a/crypto/ec/ec_montgomery.c b/crypto/ec/ec_montgomery.c
index 62b0d7f..9524323 100644
--- a/crypto/ec/ec_montgomery.c
+++ b/crypto/ec/ec_montgomery.c
@@ -79,23 +79,18 @@
 
   ok = ec_GFp_simple_group_init(group);
   group->mont = NULL;
-  group->one = NULL;
   return ok;
 }
 
 void ec_GFp_mont_group_finish(EC_GROUP *group) {
   BN_MONT_CTX_free(group->mont);
   group->mont = NULL;
-  BN_free(group->one);
-  group->one = NULL;
   ec_GFp_simple_group_finish(group);
 }
 
 int ec_GFp_mont_group_copy(EC_GROUP *dest, const EC_GROUP *src) {
   BN_MONT_CTX_free(dest->mont);
   dest->mont = NULL;
-  BN_clear_free(dest->one);
-  dest->one = NULL;
 
   if (!ec_GFp_simple_group_copy(dest, src)) {
     return 0;
@@ -110,12 +105,6 @@
       goto err;
     }
   }
-  if (src->one != NULL) {
-    dest->one = BN_dup(src->one);
-    if (dest->one == NULL) {
-      goto err;
-    }
-  }
 
   return 1;
 
@@ -129,13 +118,10 @@
                                 const BIGNUM *a, const BIGNUM *b, BN_CTX *ctx) {
   BN_CTX *new_ctx = NULL;
   BN_MONT_CTX *mont = NULL;
-  BIGNUM *one = NULL;
   int ret = 0;
 
   BN_MONT_CTX_free(group->mont);
   group->mont = NULL;
-  BN_free(group->one);
-  group->one = NULL;
 
   if (ctx == NULL) {
     ctx = new_ctx = BN_CTX_new();
@@ -152,29 +138,20 @@
     OPENSSL_PUT_ERROR(EC, ERR_R_BN_LIB);
     goto err;
   }
-  one = BN_new();
-  if (one == NULL || !BN_to_montgomery(one, BN_value_one(), mont, ctx)) {
-    goto err;
-  }
 
   group->mont = mont;
   mont = NULL;
-  group->one = one;
-  one = NULL;
 
   ret = ec_GFp_simple_group_set_curve(group, p, a, b, ctx);
 
   if (!ret) {
     BN_MONT_CTX_free(group->mont);
     group->mont = NULL;
-    BN_free(group->one);
-    group->one = NULL;
   }
 
 err:
   BN_CTX_free(new_ctx);
   BN_MONT_CTX_free(mont);
-  BN_free(one);
   return ret;
 }
 
@@ -218,19 +195,6 @@
   return BN_from_montgomery(r, a, group->mont, ctx);
 }
 
-int ec_GFp_mont_field_set_to_one(const EC_GROUP *group, BIGNUM *r,
-                                 BN_CTX *ctx) {
-  if (group->one == NULL) {
-    OPENSSL_PUT_ERROR(EC, EC_R_NOT_INITIALIZED);
-    return 0;
-  }
-
-  if (!BN_copy(r, group->one)) {
-    return 0;
-  }
-  return 1;
-}
-
 static int ec_GFp_mont_check_pub_key_order(const EC_GROUP *group,
                                            const EC_POINT* pub_key,
                                            BN_CTX *ctx) {
@@ -264,7 +228,6 @@
     ec_GFp_mont_field_sqr,
     ec_GFp_mont_field_encode,
     ec_GFp_mont_field_decode,
-    ec_GFp_mont_field_set_to_one,
   };
 
   return &ret;
diff --git a/crypto/ec/internal.h b/crypto/ec/internal.h
index 2b788c1..240d24f 100644
--- a/crypto/ec/internal.h
+++ b/crypto/ec/internal.h
@@ -116,7 +116,6 @@
                       BN_CTX *); /* e.g. to Montgomery */
   int (*field_decode)(const EC_GROUP *, BIGNUM *r, const BIGNUM *a,
                       BN_CTX *); /* e.g. from Montgomery */
-  int (*field_set_to_one)(const EC_GROUP *, BIGNUM *r, BN_CTX *);
 } /* EC_METHOD */;
 
 const EC_METHOD* EC_GFp_mont_method(void);
@@ -141,7 +140,8 @@
   int a_is_minus3; /* enable optimized point arithmetics for special case */
 
   BN_MONT_CTX *mont; /* Montgomery structure. */
-  BIGNUM *one; /* The value one */
+
+  BIGNUM one; /* The value one. */
 } /* EC_GROUP */;
 
 struct ec_point_st {
@@ -151,7 +151,6 @@
   BIGNUM Y;
   BIGNUM Z; /* Jacobian projective coordinates:
              * (X, Y, Z)  represents  (X/Z^2, Y/Z^3)  if  Z != 0 */
-  int Z_is_one; /* enable optimized point arithmetics for special case */
 } /* EC_POINT */;
 
 EC_GROUP *ec_group_new(const EC_METHOD *meth);
@@ -227,7 +226,6 @@
                              BN_CTX *);
 int ec_GFp_mont_field_decode(const EC_GROUP *, BIGNUM *r, const BIGNUM *a,
                              BN_CTX *);
-int ec_GFp_mont_field_set_to_one(const EC_GROUP *, BIGNUM *r, BN_CTX *);
 
 int ec_point_set_Jprojective_coordinates_GFp(const EC_GROUP *group,
                                              EC_POINT *point, const BIGNUM *x,
diff --git a/crypto/ec/p224-64.c b/crypto/ec/p224-64.c
index 9de6cd4..dac20e0 100644
--- a/crypto/ec/p224-64.c
+++ b/crypto/ec/p224-64.c
@@ -1235,8 +1235,7 @@
                                 ec_GFp_simple_field_mul,
                                 ec_GFp_simple_field_sqr,
                                 0 /* field_encode */,
-                                0 /* field_decode */,
-                                0 /* field_set_to_one */};
+                                0 /* field_decode */};
 
   return &ret;
 }
diff --git a/crypto/ec/p256-64.c b/crypto/ec/p256-64.c
index b94e226..629c0c3 100644
--- a/crypto/ec/p256-64.c
+++ b/crypto/ec/p256-64.c
@@ -1807,7 +1807,7 @@
       ec_GFp_nistp256_points_mul,
       0 /* check_pub_key_order */,
       ec_GFp_simple_field_mul, ec_GFp_simple_field_sqr,
-      0 /* field_encode */, 0 /* field_decode */, 0 /* field_set_to_one */
+      0 /* field_encode */, 0 /* field_decode */,
   };
 
   return &ret;
diff --git a/crypto/ec/p256-x86_64.c b/crypto/ec/p256-x86_64.c
index ca967b3..acbf455 100644
--- a/crypto/ec/p256-x86_64.c
+++ b/crypto/ec/p256-x86_64.c
@@ -502,7 +502,6 @@
   bn_correct_top(&r->X);
   bn_correct_top(&r->Y);
   bn_correct_top(&r->Z);
-  r->Z_is_one = BN_is_one(&r->Z);
 
   ret = 1;
 
@@ -576,7 +575,6 @@
       ec_GFp_mont_field_sqr,
       ec_GFp_mont_field_encode,
       ec_GFp_mont_field_decode,
-      ec_GFp_mont_field_set_to_one,
   };
 
   return &ret;
diff --git a/crypto/ec/simple.c b/crypto/ec/simple.c
index fc8d1d8..1f35388 100644
--- a/crypto/ec/simple.c
+++ b/crypto/ec/simple.c
@@ -92,6 +92,7 @@
   BN_init(&group->field);
   BN_init(&group->a);
   BN_init(&group->b);
+  BN_init(&group->one);
   group->a_is_minus3 = 0;
   return 1;
 }
@@ -100,12 +101,14 @@
   BN_free(&group->field);
   BN_free(&group->a);
   BN_free(&group->b);
+  BN_free(&group->one);
 }
 
 int ec_GFp_simple_group_copy(EC_GROUP *dest, const EC_GROUP *src) {
   if (!BN_copy(&dest->field, &src->field) ||
       !BN_copy(&dest->a, &src->a) ||
-      !BN_copy(&dest->b, &src->b)) {
+      !BN_copy(&dest->b, &src->b) ||
+      !BN_copy(&dest->one, &src->one)) {
     return 0;
   }
 
@@ -172,6 +175,14 @@
   }
   group->a_is_minus3 = (0 == BN_cmp(tmp_a, &group->field));
 
+  if (group->meth->field_encode != NULL) {
+    if (!group->meth->field_encode(group, &group->one, BN_value_one(), ctx)) {
+      goto err;
+    }
+  } else if (!BN_copy(&group->one, BN_value_one())) {
+    goto err;
+  }
+
   ret = 1;
 
 err:
@@ -228,7 +239,6 @@
   BN_init(&point->X);
   BN_init(&point->Y);
   BN_init(&point->Z);
-  point->Z_is_one = 0;
 
   return 1;
 }
@@ -243,7 +253,6 @@
   BN_clear_free(&point->X);
   BN_clear_free(&point->Y);
   BN_clear_free(&point->Z);
-  point->Z_is_one = 0;
 }
 
 int ec_GFp_simple_point_copy(EC_POINT *dest, const EC_POINT *src) {
@@ -252,14 +261,12 @@
       !BN_copy(&dest->Z, &src->Z)) {
     return 0;
   }
-  dest->Z_is_one = src->Z_is_one;
 
   return 1;
 }
 
 int ec_GFp_simple_point_set_to_infinity(const EC_GROUP *group,
                                         EC_POINT *point) {
-  point->Z_is_one = 0;
   BN_zero(&point->Z);
   return 1;
 }
@@ -298,22 +305,19 @@
   }
 
   if (z != NULL) {
-    int Z_is_one;
-
     if (!BN_nnmod(&point->Z, z, &group->field, ctx)) {
       goto err;
     }
-    Z_is_one = BN_is_one(&point->Z);
+    int Z_is_one = BN_is_one(&point->Z);
     if (group->meth->field_encode) {
-      if (Z_is_one && (group->meth->field_set_to_one != 0)) {
-        if (!group->meth->field_set_to_one(group, &point->Z, ctx)) {
+      if (Z_is_one) {
+        if (BN_copy(&point->Z, &group->one) == NULL) {
           goto err;
         }
       } else if (!group->meth->field_encode(group, &point->Z, &point->Z, ctx)) {
         goto err;
       }
     }
-    point->Z_is_one = Z_is_one;
   }
 
   ret = 1;
@@ -531,7 +535,9 @@
    */
 
   /* n1, n2 */
-  if (b->Z_is_one) {
+  int b_Z_is_one = BN_cmp(&b->Z, &group->one) == 0;
+
+  if (b_Z_is_one) {
     if (!BN_copy(n1, &a->X) || !BN_copy(n2, &a->Y)) {
       goto end;
     }
@@ -552,7 +558,8 @@
   }
 
   /* n3, n4 */
-  if (a->Z_is_one) {
+  int a_Z_is_one = BN_cmp(&a->Z, &group->one) == 0;
+  if (a_Z_is_one) {
     if (!BN_copy(n3, &b->X) || !BN_copy(n4, &b->Y)) {
       goto end;
     }
@@ -590,7 +597,6 @@
     } else {
       /* a is the inverse of b */
       BN_zero(&r->Z);
-      r->Z_is_one = 0;
       ret = 1;
       goto end;
     }
@@ -605,16 +611,16 @@
   /* 'n8' = n2 + n4 */
 
   /* Z_r */
-  if (a->Z_is_one && b->Z_is_one) {
+  if (a_Z_is_one && b_Z_is_one) {
     if (!BN_copy(&r->Z, n5)) {
       goto end;
     }
   } else {
-    if (a->Z_is_one) {
+    if (a_Z_is_one) {
       if (!BN_copy(n0, &b->Z)) {
         goto end;
       }
-    } else if (b->Z_is_one) {
+    } else if (b_Z_is_one) {
       if (!BN_copy(n0, &a->Z)) {
         goto end;
       }
@@ -625,7 +631,7 @@
       goto end;
     }
   }
-  r->Z_is_one = 0;
+
   /* Z_r = Z_a * Z_b * n5 */
 
   /* X_r */
@@ -685,7 +691,6 @@
 
   if (EC_POINT_is_at_infinity(group, a)) {
     BN_zero(&r->Z);
-    r->Z_is_one = 0;
     return 1;
   }
 
@@ -715,7 +720,7 @@
    */
 
   /* n1 */
-  if (a->Z_is_one) {
+  if (BN_cmp(&a->Z, &group->one) == 0) {
     if (!field_sqr(group, n0, &a->X, ctx) ||
         !BN_mod_lshift1_quick(n1, n0, p) ||
         !BN_mod_add_quick(n0, n0, n1, p) ||
@@ -748,7 +753,7 @@
   }
 
   /* Z_r */
-  if (a->Z_is_one) {
+  if (BN_cmp(&a->Z, &group->one) == 0) {
     if (!BN_copy(n0, &a->Y)) {
       goto err;
     }
@@ -758,7 +763,6 @@
   if (!BN_mod_lshift1_quick(&r->Z, n0, p)) {
     goto err;
   }
-  r->Z_is_one = 0;
   /* Z_r = 2 * Y_a * Z_a */
 
   /* n2 */
@@ -810,7 +814,7 @@
 }
 
 int ec_GFp_simple_is_at_infinity(const EC_GROUP *group, const EC_POINT *point) {
-  return !point->Z_is_one && BN_is_zero(&point->Z);
+  return BN_is_zero(&point->Z);
 }
 
 int ec_GFp_simple_is_on_curve(const EC_GROUP *group, const EC_POINT *point,
@@ -862,7 +866,7 @@
     goto err;
   }
 
-  if (!point->Z_is_one) {
+  if (BN_cmp(&point->Z, &group->one) != 0) {
     if (!field_sqr(group, tmp, &point->Z, ctx) ||
         !field_sqr(group, Z4, tmp, ctx) ||
         !field_mul(group, Z6, Z4, tmp, ctx)) {
@@ -891,8 +895,6 @@
       goto err;
     }
   } else {
-    /* point->Z_is_one */
-
     /* rh := (rh + a)*X */
     if (!BN_mod_add_quick(rh, rh, &group->a, p) ||
         !field_mul(group, rh, rh, &point->X, ctx)) {
@@ -941,7 +943,10 @@
     return 1;
   }
 
-  if (a->Z_is_one && b->Z_is_one) {
+  int a_Z_is_one = BN_cmp(&a->Z, &group->one) == 0;
+  int b_Z_is_one = BN_cmp(&b->Z, &group->one) == 0;
+
+  if (a_Z_is_one && b_Z_is_one) {
     return ((BN_cmp(&a->X, &b->X) == 0) && BN_cmp(&a->Y, &b->Y) == 0) ? 0 : 1;
   }
 
@@ -970,7 +975,7 @@
    *     (X_a*Z_b^2, Y_a*Z_b^3) = (X_b*Z_a^2, Y_b*Z_a^3).
    */
 
-  if (!b->Z_is_one) {
+  if (!b_Z_is_one) {
     if (!field_sqr(group, Zb23, &b->Z, ctx) ||
         !field_mul(group, tmp1, &a->X, Zb23, ctx)) {
       goto end;
@@ -979,7 +984,7 @@
   } else {
     tmp1_ = &a->X;
   }
-  if (!a->Z_is_one) {
+  if (!a_Z_is_one) {
     if (!field_sqr(group, Za23, &a->Z, ctx) ||
         !field_mul(group, tmp2, &b->X, Za23, ctx)) {
       goto end;
@@ -996,7 +1001,7 @@
   }
 
 
-  if (!b->Z_is_one) {
+  if (!b_Z_is_one) {
     if (!field_mul(group, Zb23, Zb23, &b->Z, ctx) ||
         !field_mul(group, tmp1, &a->Y, Zb23, ctx)) {
       goto end;
@@ -1005,7 +1010,7 @@
   } else {
     tmp1_ = &a->Y;
   }
-  if (!a->Z_is_one) {
+  if (!a_Z_is_one) {
     if (!field_mul(group, Za23, Za23, &a->Z, ctx) ||
         !field_mul(group, tmp2, &b->Y, Za23, ctx)) {
       goto end;
@@ -1036,7 +1041,8 @@
   BIGNUM *x, *y;
   int ret = 0;
 
-  if (point->Z_is_one || EC_POINT_is_at_infinity(group, point)) {
+  if (BN_cmp(&point->Z, &group->one) == 0 ||
+      EC_POINT_is_at_infinity(group, point)) {
     return 1;
   }
 
@@ -1058,7 +1064,7 @@
       !EC_POINT_set_affine_coordinates_GFp(group, point, x, y, ctx)) {
     goto err;
   }
-  if (!point->Z_is_one) {
+  if (BN_cmp(&point->Z, &group->one) != 0) {
     OPENSSL_PUT_ERROR(EC, ERR_R_INTERNAL_ERROR);
     goto err;
   }
@@ -1117,14 +1123,8 @@
       goto err;
     }
   } else {
-    if (group->meth->field_set_to_one != 0) {
-      if (!group->meth->field_set_to_one(group, prod_Z[0], ctx)) {
-        goto err;
-      }
-    } else {
-      if (!BN_one(prod_Z[0])) {
-        goto err;
-      }
+    if (BN_copy(prod_Z[0], &group->one) == NULL) {
+      goto err;
     }
   }
 
@@ -1195,16 +1195,9 @@
         goto err;
       }
 
-      if (group->meth->field_set_to_one != NULL) {
-        if (!group->meth->field_set_to_one(group, &p->Z, ctx)) {
-          goto err;
-        }
-      } else {
-        if (!BN_one(&p->Z)) {
-          goto err;
-        }
+      if (BN_copy(&p->Z, &group->one) == NULL) {
+        goto err;
       }
-      p->Z_is_one = 1;
     }
   }