Skip to content

Commit 34a67c7

Browse files
gmaxwellgmaxwell
committed
Eliminate harmless non-constant time operations on secret data.
There were several places where the code was non-constant time for invalid secret inputs. These are harmless under sane use but get in the way of automatic const-time validation. (Nonce overflow in signing is not addressed, nor is s==0 in signing)
1 parent 856a01d commit 34a67c7

16 files changed

+157
-104
lines changed

‎src/ecdsa_impl.h‎

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -280,6 +280,7 @@ static int secp256k1_ecdsa_sig_sign(const secp256k1_ecmult_gen_context *ctx, sec
280280
secp256k1_ge r;
281281
secp256k1_scalar n;
282282
int overflow = 0;
283+
int high;
283284

284285
secp256k1_ecmult_gen(ctx, &rp, nonce);
285286
secp256k1_ge_set_gej(&r, &rp);
@@ -295,7 +296,7 @@ static int secp256k1_ecdsa_sig_sign(const secp256k1_ecmult_gen_context *ctx, sec
295296
/* The overflow condition is cryptographically unreachable as hitting it requires finding the discrete log
296297
* of some P where P.x >= order, and only 1 in about 2^127 points meet this criteria.
297298
*/
298-
*recid = (overflow ? 2 : 0) | (secp256k1_fe_is_odd(&r.y) ? 1 : 0);
299+
*recid = (overflow << 1) | secp256k1_fe_is_odd(&r.y);
299300
}
300301
secp256k1_scalar_mul(&n, sigr, seckey);
301302
secp256k1_scalar_add(&n, &n, message);
@@ -304,16 +305,12 @@ static int secp256k1_ecdsa_sig_sign(const secp256k1_ecmult_gen_context *ctx, sec
304305
secp256k1_scalar_clear(&n);
305306
secp256k1_gej_clear(&rp);
306307
secp256k1_ge_clear(&r);
307-
if (secp256k1_scalar_is_zero(sigs)) {
308-
return 0;
309-
}
310-
if (secp256k1_scalar_is_high(sigs)) {
311-
secp256k1_scalar_negate(sigs, sigs);
312-
if (recid) {
313-
*recid ^= 1;
314-
}
308+
high = secp256k1_scalar_is_high(sigs);
309+
secp256k1_scalar_cond_negate(sigs, high);
310+
if (recid) {
311+
*recid ^= high;
315312
}
316-
return 1;
313+
return !secp256k1_scalar_is_zero(sigs);
317314
}
318315

319316
#endif /* SECP256K1_ECDSA_IMPL_H */

‎src/eckey_impl.h‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -75,12 +75,11 @@ static int secp256k1_eckey_pubkey_tweak_add(const secp256k1_ecmult_context *ctx,
7575
}
7676

7777
static int secp256k1_eckey_privkey_tweak_mul(secp256k1_scalar *key, const secp256k1_scalar *tweak) {
78-
if (secp256k1_scalar_is_zero(tweak)) {
79-
return 0;
80-
}
78+
int ret;
79+
ret = !secp256k1_scalar_is_zero(tweak);
8180

8281
secp256k1_scalar_mul(key, key, tweak);
83-
return 1;
82+
return ret;
8483
}
8584

8685
static int secp256k1_eckey_pubkey_tweak_mul(const secp256k1_ecmult_context *ctx, secp256k1_ge *key, const secp256k1_scalar *tweak) {

‎src/ecmult_gen_impl.h‎

Lines changed: 10 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,7 @@ static void secp256k1_ecmult_gen_blind(secp256k1_ecmult_gen_context *ctx, const
163163
secp256k1_fe s;
164164
unsigned char nonce32[32];
165165
secp256k1_rfc6979_hmac_sha256 rng;
166-
int retry;
166+
int overflow;
167167
unsigned char keydata[64] = {0};
168168
if (seed32 == NULL) {
169169
/* When seed is NULL, reset the initial point and blinding value. */
@@ -183,21 +183,18 @@ static void secp256k1_ecmult_gen_blind(secp256k1_ecmult_gen_context *ctx, const
183183
}
184184
secp256k1_rfc6979_hmac_sha256_initialize(&rng, keydata, seed32 ? 64 : 32);
185185
memset(keydata, 0, sizeof(keydata));
186-
/* Retry for out of range results to achieve uniformity. */
187-
do {
188-
secp256k1_rfc6979_hmac_sha256_generate(&rng, nonce32, 32);
189-
retry = !secp256k1_fe_set_b32(&s, nonce32);
190-
retry = retry || secp256k1_fe_is_zero(&s);
191-
} while (retry); /* This branch true is cryptographically unreachable. Requires sha256_hmac output > Fp. */
186+
/* Accept unobservably small non-uniformity. */
187+
secp256k1_rfc6979_hmac_sha256_generate(&rng, nonce32, 32);
188+
overflow = !secp256k1_fe_set_b32(&s, nonce32);
189+
overflow |= secp256k1_fe_is_zero(&s);
190+
secp256k1_fe_cmov(&s, &secp256k1_fe_one, overflow);
192191
/* Randomize the projection to defend against multiplier sidechannels. */
193192
secp256k1_gej_rescale(&ctx->initial, &s);
194193
secp256k1_fe_clear(&s);
195-
do {
196-
secp256k1_rfc6979_hmac_sha256_generate(&rng, nonce32, 32);
197-
secp256k1_scalar_set_b32(&b, nonce32, &retry);
198-
/* A blinding value of 0 works, but would undermine the projection hardening. */
199-
retry = retry || secp256k1_scalar_is_zero(&b);
200-
} while (retry); /* This branch true is cryptographically unreachable. Requires sha256_hmac output > order. */
194+
secp256k1_rfc6979_hmac_sha256_generate(&rng, nonce32, 32);
195+
secp256k1_scalar_set_b32(&b, nonce32, NULL);
196+
/* A blinding value of 0 works, but would undermine the projection hardening. */
197+
secp256k1_scalar_cmov(&b, &secp256k1_scalar_one, secp256k1_scalar_is_zero(&b));
201198
secp256k1_rfc6979_hmac_sha256_finalize(&rng);
202199
memset(nonce32, 0, 32);
203200
secp256k1_ecmult_gen(ctx, &gb, &b);

‎src/field_10x26_impl.h‎

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -320,6 +320,7 @@ static int secp256k1_fe_cmp_var(const secp256k1_fe *a, const secp256k1_fe *b) {
320320
}
321321

322322
static int secp256k1_fe_set_b32(secp256k1_fe *r, const unsigned char *a) {
323+
int ret;
323324
r->n[0] = (uint32_t)a[31] | ((uint32_t)a[30] << 8) | ((uint32_t)a[29] << 16) | ((uint32_t)(a[28] & 0x3) << 24);
324325
r->n[1] = (uint32_t)((a[28] >> 2) & 0x3f) | ((uint32_t)a[27] << 6) | ((uint32_t)a[26] << 14) | ((uint32_t)(a[25] & 0xf) << 22);
325326
r->n[2] = (uint32_t)((a[25] >> 4) & 0xf) | ((uint32_t)a[24] << 4) | ((uint32_t)a[23] << 12) | ((uint32_t)(a[22] & 0x3f) << 20);
@@ -331,15 +332,17 @@ static int secp256k1_fe_set_b32(secp256k1_fe *r, const unsigned char *a) {
331332
r->n[8] = (uint32_t)a[5] | ((uint32_t)a[4] << 8) | ((uint32_t)a[3] << 16) | ((uint32_t)(a[2] & 0x3) << 24);
332333
r->n[9] = (uint32_t)((a[2] >> 2) & 0x3f) | ((uint32_t)a[1] << 6) | ((uint32_t)a[0] << 14);
333334

334-
if (r->n[9] == 0x3FFFFFUL && (r->n[8] & r->n[7] & r->n[6] & r->n[5] & r->n[4] & r->n[3] & r->n[2]) == 0x3FFFFFFUL && (r->n[1] + 0x40UL + ((r->n[0] + 0x3D1UL) >> 26)) > 0x3FFFFFFUL) {
335-
return 0;
336-
}
335+
ret = !((r->n[9] == 0x3FFFFFUL) & ((r->n[8] & r->n[7] & r->n[6] & r->n[5] & r->n[4] & r->n[3] & r->n[2]) == 0x3FFFFFFUL) & ((r->n[1] + 0x40UL + ((r->n[0] + 0x3D1UL) >> 26)) > 0x3FFFFFFUL));
337336
#ifdef VERIFY
338337
r->magnitude = 1;
339-
r->normalized = 1;
340-
secp256k1_fe_verify(r);
338+
if (ret) {
339+
r->normalized = 1;
340+
secp256k1_fe_verify(r);
341+
} else {
342+
r->normalized = 0;
343+
}
341344
#endif
342-
return 1;
345+
return ret;
343346
}
344347

345348
/** Convert a field element to a 32-byte big endian value. Requires the input to be normalized */
@@ -1107,10 +1110,10 @@ static SECP256K1_INLINE void secp256k1_fe_cmov(secp256k1_fe *r, const secp256k1_
11071110
r->n[8] = (r->n[8] & mask0) | (a->n[8] & mask1);
11081111
r->n[9] = (r->n[9] & mask0) | (a->n[9] & mask1);
11091112
#ifdef VERIFY
1110-
if (a->magnitude > r->magnitude) {
1113+
if (flag) {
11111114
r->magnitude = a->magnitude;
1115+
r->normalized = a->normalized;
11121116
}
1113-
r->normalized &= a->normalized;
11141117
#endif
11151118
}
11161119

‎src/field_5x52_impl.h‎

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -283,6 +283,7 @@ static int secp256k1_fe_cmp_var(const secp256k1_fe *a, const secp256k1_fe *b) {
283283
}
284284

285285
static int secp256k1_fe_set_b32(secp256k1_fe *r, const unsigned char *a) {
286+
int ret;
286287
r->n[0] = (uint64_t)a[31]
287288
| ((uint64_t)a[30] << 8)
288289
| ((uint64_t)a[29] << 16)
@@ -317,15 +318,17 @@ static int secp256k1_fe_set_b32(secp256k1_fe *r, const unsigned char *a) {
317318
| ((uint64_t)a[2] << 24)
318319
| ((uint64_t)a[1] << 32)
319320
| ((uint64_t)a[0] << 40);
320-
if (r->n[4] == 0x0FFFFFFFFFFFFULL && (r->n[3] & r->n[2] & r->n[1]) == 0xFFFFFFFFFFFFFULL && r->n[0] >= 0xFFFFEFFFFFC2FULL) {
321-
return 0;
322-
}
321+
ret = !((r->n[4] == 0x0FFFFFFFFFFFFULL) & ((r->n[3] & r->n[2] & r->n[1]) == 0xFFFFFFFFFFFFFULL) & (r->n[0] >= 0xFFFFEFFFFFC2FULL));
323322
#ifdef VERIFY
324323
r->magnitude = 1;
325-
r->normalized = 1;
326-
secp256k1_fe_verify(r);
324+
if (ret) {
325+
r->normalized = 1;
326+
secp256k1_fe_verify(r);
327+
} else {
328+
r->normalized = 0;
329+
}
327330
#endif
328-
return 1;
331+
return ret;
329332
}
330333

331334
/** Convert a field element to a 32-byte big endian value. Requires the input to be normalized */
@@ -454,10 +457,10 @@ static SECP256K1_INLINE void secp256k1_fe_cmov(secp256k1_fe *r, const secp256k1_
454457
r->n[3] = (r->n[3] & mask0) | (a->n[3] & mask1);
455458
r->n[4] = (r->n[4] & mask0) | (a->n[4] & mask1);
456459
#ifdef VERIFY
457-
if (a->magnitude > r->magnitude) {
460+
if (flag) {
458461
r->magnitude = a->magnitude;
462+
r->normalized = a->normalized;
459463
}
460-
r->normalized &= a->normalized;
461464
#endif
462465
}
463466

‎src/field_impl.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -315,4 +315,6 @@ static int secp256k1_fe_is_quad_var(const secp256k1_fe *a) {
315315
#endif
316316
}
317317

318+
static const secp256k1_fe secp256k1_fe_one = SECP256K1_FE_CONST(0, 0, 0, 0, 0, 0, 0, 1);
319+
318320
#endif /* SECP256K1_FIELD_IMPL_H */

‎src/modules/ecdh/main_impl.h‎

Lines changed: 22 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -32,36 +32,40 @@ int secp256k1_ecdh(const secp256k1_context* ctx, unsigned char *output, const se
3232
secp256k1_gej res;
3333
secp256k1_ge pt;
3434
secp256k1_scalar s;
35+
unsigned char x[32];
36+
unsigned char y[32];
37+
3538
VERIFY_CHECK(ctx != NULL);
3639
ARG_CHECK(output != NULL);
3740
ARG_CHECK(point != NULL);
3841
ARG_CHECK(scalar != NULL);
42+
3943
if (hashfp == NULL) {
4044
hashfp = secp256k1_ecdh_hash_function_default;
4145
}
4246

4347
secp256k1_pubkey_load(ctx, &pt, point);
4448
secp256k1_scalar_set_b32(&s, scalar, &overflow);
45-
if (overflow || secp256k1_scalar_is_zero(&s)) {
46-
ret = 0;
47-
} else {
48-
unsigned char x[32];
49-
unsigned char y[32];
50-
51-
secp256k1_ecmult_const(&res, &pt, &s, 256);
52-
secp256k1_ge_set_gej(&pt, &res);
53-
54-
/* Compute a hash of the point */
55-
secp256k1_fe_normalize(&pt.x);
56-
secp256k1_fe_normalize(&pt.y);
57-
secp256k1_fe_get_b32(x, &pt.x);
58-
secp256k1_fe_get_b32(y, &pt.y);
59-
60-
ret = hashfp(output, x, y, data);
61-
}
6249

50+
overflow |= secp256k1_scalar_is_zero(&s);
51+
secp256k1_scalar_cmov(&s, &secp256k1_scalar_one, overflow);
52+
53+
secp256k1_ecmult_const(&res, &pt, &s, 256);
54+
secp256k1_ge_set_gej(&pt, &res);
55+
56+
/* Compute a hash of the point */
57+
secp256k1_fe_normalize(&pt.x);
58+
secp256k1_fe_normalize(&pt.y);
59+
secp256k1_fe_get_b32(x, &pt.x);
60+
secp256k1_fe_get_b32(y, &pt.y);
61+
62+
ret = hashfp(output, x, y, data);
63+
64+
memset(x, 0, 32);
65+
memset(y, 0, 32);
6366
secp256k1_scalar_clear(&s);
64-
return ret;
67+
68+
return !!ret & !overflow;
6569
}
6670

6771
#endif /* SECP256K1_MODULE_ECDH_MAIN_H */

‎src/scalar.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,4 +107,7 @@ static void secp256k1_scalar_split_lambda(secp256k1_scalar *r1, secp256k1_scalar
107107
/** Multiply a and b (without taking the modulus!), divide by 2**shift, and round to the nearest integer. Shift must be at least 256. */
108108
static void secp256k1_scalar_mul_shift_var(secp256k1_scalar *r, const secp256k1_scalar *a, const secp256k1_scalar *b, unsigned int shift);
109109

110+
/** If flag is true, set *r equal to *a; otherwise leave it. Constant-time. */
111+
static void secp256k1_scalar_cmov(secp256k1_scalar *r, const secp256k1_scalar *a, int flag);
112+
110113
#endif /* SECP256K1_SCALAR_H */

‎src/scalar_4x64_impl.h‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -946,4 +946,14 @@ SECP256K1_INLINE static void secp256k1_scalar_mul_shift_var(secp256k1_scalar *r,
946946
secp256k1_scalar_cadd_bit(r, 0, (l[(shift - 1) >> 6] >> ((shift - 1) & 0x3f)) & 1);
947947
}
948948

949+
static SECP256K1_INLINE void secp256k1_scalar_cmov(secp256k1_scalar *r, const secp256k1_scalar *a, int flag) {
950+
uint64_t mask0, mask1;
951+
mask0 = flag + ~((uint64_t)0);
952+
mask1 = ~mask0;
953+
r->d[0] = (r->d[0] & mask0) | (a->d[0] & mask1);
954+
r->d[1] = (r->d[1] & mask0) | (a->d[1] & mask1);
955+
r->d[2] = (r->d[2] & mask0) | (a->d[2] & mask1);
956+
r->d[3] = (r->d[3] & mask0) | (a->d[3] & mask1);
957+
}
958+
949959
#endif /* SECP256K1_SCALAR_REPR_IMPL_H */

‎src/scalar_8x32_impl.h‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -718,4 +718,18 @@ SECP256K1_INLINE static void secp256k1_scalar_mul_shift_var(secp256k1_scalar *r,
718718
secp256k1_scalar_cadd_bit(r, 0, (l[(shift - 1) >> 5] >> ((shift - 1) & 0x1f)) & 1);
719719
}
720720

721+
static SECP256K1_INLINE void secp256k1_scalar_cmov(secp256k1_scalar *r, const secp256k1_scalar *a, int flag) {
722+
uint32_t mask0, mask1;
723+
mask0 = flag + ~((uint32_t)0);
724+
mask1 = ~mask0;
725+
r->d[0] = (r->d[0] & mask0) | (a->d[0] & mask1);
726+
r->d[1] = (r->d[1] & mask0) | (a->d[1] & mask1);
727+
r->d[2] = (r->d[2] & mask0) | (a->d[2] & mask1);
728+
r->d[3] = (r->d[3] & mask0) | (a->d[3] & mask1);
729+
r->d[4] = (r->d[4] & mask0) | (a->d[4] & mask1);
730+
r->d[5] = (r->d[5] & mask0) | (a->d[5] & mask1);
731+
r->d[6] = (r->d[6] & mask0) | (a->d[6] & mask1);
732+
r->d[7] = (r->d[7] & mask0) | (a->d[7] & mask1);
733+
}
734+
721735
#endif /* SECP256K1_SCALAR_REPR_IMPL_H */

0 commit comments

Comments
 (0)