Skip to content

Commit

Permalink
Remove secret-dependant non-constant time operation in ecmult_const.
Browse files Browse the repository at this point in the history
ECMULT_CONST_TABLE_GET_GE was branching on its secret input.

Also makes secp256k1_gej_double_var implemented as a wrapper
 on secp256k1_gej_double_nonzero instead of the other way
 around.  This wasn't a constant time bug but it was fragile
 and could easily become one in the future if the double_var
 algorithm is changed.
  • Loading branch information
gmaxwell committed Jan 9, 2020
1 parent f45d897 commit 2241ae6
Show file tree
Hide file tree
Showing 5 changed files with 38 additions and 33 deletions.
8 changes: 5 additions & 3 deletions src/ecmult_const_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,9 @@
/* This is like `ECMULT_TABLE_GET_GE` but is constant time */
#define ECMULT_CONST_TABLE_GET_GE(r,pre,n,w) do { \
int m; \
int abs_n = (n) * (((n) > 0) * 2 - 1); \
int idx_n = abs_n / 2; \
int mask = (n) >> (sizeof(n) * CHAR_BIT - 1); \
int abs_n = ((n) + mask) ^ mask; \
int idx_n = abs_n >> 1; \
secp256k1_fe neg_y; \
VERIFY_CHECK(((n) & 1) == 1); \
VERIFY_CHECK((n) >= -((1 << ((w)-1)) - 1)); \
Expand Down Expand Up @@ -172,6 +173,7 @@ static void secp256k1_ecmult_const(secp256k1_gej *r, const secp256k1_ge *a, cons
for (i = 0; i < ECMULT_TABLE_SIZE(WINDOW_A); i++) {
secp256k1_ge_mul_lambda(&pre_a_lam[i], &pre_a[i]);
}

}
#endif

Expand All @@ -195,7 +197,7 @@ static void secp256k1_ecmult_const(secp256k1_gej *r, const secp256k1_ge *a, cons
int n;
int j;
for (j = 0; j < WINDOW_A - 1; ++j) {
secp256k1_gej_double_nonzero(r, r, NULL);
secp256k1_gej_double_nonzero(r, r);
}

n = wnaf_1[i];
Expand Down
5 changes: 2 additions & 3 deletions src/group.h
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,8 @@ static int secp256k1_gej_is_infinity(const secp256k1_gej *a);
/** Check whether a group element's y coordinate is a quadratic residue. */
static int secp256k1_gej_has_quad_y_var(const secp256k1_gej *a);

/** Set r equal to the double of a. If rzr is not-NULL, r->z = a->z * *rzr (where infinity means an implicit z = 0).
* a may not be zero. Constant time. */
static void secp256k1_gej_double_nonzero(secp256k1_gej *r, const secp256k1_gej *a, secp256k1_fe *rzr);
/** Set r equal to the double of a, a cannot be infinity. Constant time. */
static void secp256k1_gej_double_nonzero(secp256k1_gej *r, const secp256k1_gej *a);

/** Set r equal to the double of a. If rzr is not-NULL, r->z = a->z * *rzr (where infinity means an implicit z = 0). */
static void secp256k1_gej_double_var(secp256k1_gej *r, const secp256k1_gej *a, secp256k1_fe *rzr);
Expand Down
55 changes: 29 additions & 26 deletions src/group_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ static int secp256k1_ge_is_valid_var(const secp256k1_ge *a) {
return secp256k1_fe_equal_var(&y2, &x3);
}

static void secp256k1_gej_double_var(secp256k1_gej *r, const secp256k1_gej *a, secp256k1_fe *rzr) {
static SECP256K1_INLINE void secp256k1_gej_double_nonzero(secp256k1_gej *r, const secp256k1_gej *a) {
/* Operations: 3 mul, 4 sqr, 0 normalize, 12 mul_int/add/negate.
*
* Note that there is an implementation described at
Expand All @@ -312,29 +312,9 @@ static void secp256k1_gej_double_var(secp256k1_gej *r, const secp256k1_gej *a, s
* mainly because it requires more normalizations.
*/
secp256k1_fe t1,t2,t3,t4;
/** For secp256k1, 2Q is infinity if and only if Q is infinity. This is because if 2Q = infinity,
* Q must equal -Q, or that Q.y == -(Q.y), or Q.y is 0. For a point on y^2 = x^3 + 7 to have
* y=0, x^3 must be -7 mod p. However, -7 has no cube root mod p.
*
* Having said this, if this function receives a point on a sextic twist, e.g. by
* a fault attack, it is possible for y to be 0. This happens for y^2 = x^3 + 6,
* since -6 does have a cube root mod p. For this point, this function will not set
* the infinity flag even though the point doubles to infinity, and the result
* point will be gibberish (z = 0 but infinity = 0).
*/
r->infinity = a->infinity;
if (r->infinity) {
if (rzr != NULL) {
secp256k1_fe_set_int(rzr, 1);
}
return;
}

if (rzr != NULL) {
*rzr = a->y;
secp256k1_fe_normalize_weak(rzr);
secp256k1_fe_mul_int(rzr, 2);
}
VERIFY_CHECK(!secp256k1_gej_is_infinity(a));
r->infinity = 0;

secp256k1_fe_mul(&r->z, &a->z, &a->y);
secp256k1_fe_mul_int(&r->z, 2); /* Z' = 2*Y*Z (2) */
Expand All @@ -358,9 +338,32 @@ static void secp256k1_gej_double_var(secp256k1_gej *r, const secp256k1_gej *a, s
secp256k1_fe_add(&r->y, &t2); /* Y' = 36*X^3*Y^2 - 27*X^6 - 8*Y^4 (4) */
}

static SECP256K1_INLINE void secp256k1_gej_double_nonzero(secp256k1_gej *r, const secp256k1_gej *a, secp256k1_fe *rzr) {
VERIFY_CHECK(!secp256k1_gej_is_infinity(a));
secp256k1_gej_double_var(r, a, rzr);
static void secp256k1_gej_double_var(secp256k1_gej *r, const secp256k1_gej *a, secp256k1_fe *rzr) {
/** For secp256k1, 2Q is infinity if and only if Q is infinity. This is because if 2Q = infinity,
* Q must equal -Q, or that Q.y == -(Q.y), or Q.y is 0. For a point on y^2 = x^3 + 7 to have
* y=0, x^3 must be -7 mod p. However, -7 has no cube root mod p.
*
* Having said this, if this function receives a point on a sextic twist, e.g. by
* a fault attack, it is possible for y to be 0. This happens for y^2 = x^3 + 6,
* since -6 does have a cube root mod p. For this point, this function will not set
* the infinity flag even though the point doubles to infinity, and the result
* point will be gibberish (z = 0 but infinity = 0).
*/
if (a->infinity) {
r->infinity = 1;
if (rzr != NULL) {
secp256k1_fe_set_int(rzr, 1);
}
return;
}

if (rzr != NULL) {
*rzr = a->y;
secp256k1_fe_normalize_weak(rzr);
secp256k1_fe_mul_int(rzr, 2);
}

secp256k1_gej_double_nonzero(r, a);
}

static void secp256k1_gej_add_var(secp256k1_gej *r, const secp256k1_gej *a, const secp256k1_gej *b, secp256k1_fe *rzr) {
Expand Down
2 changes: 1 addition & 1 deletion src/tests_exhaustive.c
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ void test_exhaustive_addition(const secp256k1_ge *group, const secp256k1_gej *gr
for (i = 0; i < order; i++) {
secp256k1_gej tmp;
if (i > 0) {
secp256k1_gej_double_nonzero(&tmp, &groupj[i], NULL);
secp256k1_gej_double_nonzero(&tmp, &groupj[i]);
ge_equals_gej(&group[(2 * i) % order], &tmp);
}
secp256k1_gej_double_var(&tmp, &groupj[i], NULL);
Expand Down
1 change: 1 addition & 0 deletions src/util.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include <stdlib.h>
#include <stdint.h>
#include <stdio.h>
#include <limits.h>

typedef struct {
void (*fn)(const char *text, void* data);
Expand Down

0 comments on commit 2241ae6

Please sign in to comment.