Skip to content

Commit

Permalink
Merge bitcoin#755: Recovery signing: add to constant time test, and e…
Browse files Browse the repository at this point in the history
…liminate non ct operators

2860950 Add tests for the cmov implementations (Elichai Turkel)
73596a8 Add ecdsa_sign_recoverable to the ctime tests (Elichai Turkel)
2876af4 Split ecdsa_sign logic into a new function and use it from ecdsa_sign and recovery (Elichai Turkel)

Pull request description:

  Hi,
  The recovery module was overlooked in bitcoin#708 and bitcoin#710, so this adds it to the `valgrind_ctime_test` and replaces the secret dependent branching with the cmovs,
  I created a new function `secp256k1_ecdsa_sign_inner` (feel free to bikeshed) which does the logic both for ecdsa_sign and for ecdsa_sign_recoverable, such that next time when things get changed/improved in ecdsa it will affect the recoverable signing too.

ACKs for top commit:
  jonasnick:
    ACK 2860950
  real-or-random:
    ACK 2860950 read the diff, tested with valgrind including ctime tests

Tree-SHA512: 4730301dcb62241d79f18eb8fed7e9ab0e20d1663a788832cb6cf4126baa7075807dc31896764b6f82d52742fdb636abc6b75e4344c6f117305904c628a5ad59
  • Loading branch information
real-or-random committed Jun 8, 2020
2 parents 5e1c885 + 2860950 commit 2ed54da
Show file tree
Hide file tree
Showing 5 changed files with 221 additions and 48 deletions.
39 changes: 3 additions & 36 deletions src/modules/recovery/main_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -122,48 +122,15 @@ static int secp256k1_ecdsa_sig_recover(const secp256k1_ecmult_context *ctx, cons

int secp256k1_ecdsa_sign_recoverable(const secp256k1_context* ctx, secp256k1_ecdsa_recoverable_signature *signature, const unsigned char *msg32, const unsigned char *seckey, secp256k1_nonce_function noncefp, const void* noncedata) {
secp256k1_scalar r, s;
secp256k1_scalar sec, non, msg;
int recid;
int ret = 0;
int overflow = 0;
int ret, recid;
VERIFY_CHECK(ctx != NULL);
ARG_CHECK(secp256k1_ecmult_gen_context_is_built(&ctx->ecmult_gen_ctx));
ARG_CHECK(msg32 != NULL);
ARG_CHECK(signature != NULL);
ARG_CHECK(seckey != NULL);
if (noncefp == NULL) {
noncefp = secp256k1_nonce_function_default;
}

secp256k1_scalar_set_b32(&sec, seckey, &overflow);
/* Fail if the secret key is invalid. */
if (!overflow && !secp256k1_scalar_is_zero(&sec)) {
unsigned char nonce32[32];
unsigned int count = 0;
secp256k1_scalar_set_b32(&msg, msg32, NULL);
while (1) {
ret = noncefp(nonce32, msg32, seckey, NULL, (void*)noncedata, count);
if (!ret) {
break;
}
secp256k1_scalar_set_b32(&non, nonce32, &overflow);
if (!overflow && !secp256k1_scalar_is_zero(&non)) {
if (secp256k1_ecdsa_sig_sign(&ctx->ecmult_gen_ctx, &r, &s, &sec, &msg, &non, &recid)) {
break;
}
}
count++;
}
memset(nonce32, 0, 32);
secp256k1_scalar_clear(&msg);
secp256k1_scalar_clear(&non);
secp256k1_scalar_clear(&sec);
}
if (ret) {
secp256k1_ecdsa_recoverable_signature_save(signature, &r, &s, recid);
} else {
memset(signature, 0, sizeof(*signature));
}
ret = secp256k1_ecdsa_sign_inner(ctx, &r, &s, &recid, msg32, seckey, noncefp, noncedata);
secp256k1_ecdsa_recoverable_signature_save(signature, &r, &s, recid);
return ret;
}

Expand Down
38 changes: 27 additions & 11 deletions src/secp256k1.c
Original file line number Diff line number Diff line change
Expand Up @@ -467,19 +467,18 @@ static int nonce_function_rfc6979(unsigned char *nonce32, const unsigned char *m
const secp256k1_nonce_function secp256k1_nonce_function_rfc6979 = nonce_function_rfc6979;
const secp256k1_nonce_function secp256k1_nonce_function_default = nonce_function_rfc6979;

int secp256k1_ecdsa_sign(const secp256k1_context* ctx, secp256k1_ecdsa_signature *signature, const unsigned char *msg32, const unsigned char *seckey, secp256k1_nonce_function noncefp, const void* noncedata) {
/* Default initialization here is important so we won't pass uninit values to the cmov in the end */
secp256k1_scalar r = secp256k1_scalar_zero, s = secp256k1_scalar_zero;
static int secp256k1_ecdsa_sign_inner(const secp256k1_context* ctx, secp256k1_scalar* r, secp256k1_scalar* s, int* recid, const unsigned char *msg32, const unsigned char *seckey, secp256k1_nonce_function noncefp, const void* noncedata) {
secp256k1_scalar sec, non, msg;
int ret = 0;
int is_sec_valid;
unsigned char nonce32[32];
unsigned int count = 0;
VERIFY_CHECK(ctx != NULL);
ARG_CHECK(secp256k1_ecmult_gen_context_is_built(&ctx->ecmult_gen_ctx));
ARG_CHECK(msg32 != NULL);
ARG_CHECK(signature != NULL);
ARG_CHECK(seckey != NULL);
/* Default initialization here is important so we won't pass uninit values to the cmov in the end */
*r = secp256k1_scalar_zero;
*s = secp256k1_scalar_zero;
if (recid) {
*recid = 0;
}
if (noncefp == NULL) {
noncefp = secp256k1_nonce_function_default;
}
Expand All @@ -498,7 +497,7 @@ int secp256k1_ecdsa_sign(const secp256k1_context* ctx, secp256k1_ecdsa_signature
/* The nonce is still secret here, but it being invalid is is less likely than 1:2^255. */
secp256k1_declassify(ctx, &is_nonce_valid, sizeof(is_nonce_valid));
if (is_nonce_valid) {
ret = secp256k1_ecdsa_sig_sign(&ctx->ecmult_gen_ctx, &r, &s, &sec, &msg, &non, NULL);
ret = secp256k1_ecdsa_sig_sign(&ctx->ecmult_gen_ctx, r, s, &sec, &msg, &non, recid);
/* The final signature is no longer a secret, nor is the fact that we were successful or not. */
secp256k1_declassify(ctx, &ret, sizeof(ret));
if (ret) {
Expand All @@ -515,8 +514,25 @@ int secp256k1_ecdsa_sign(const secp256k1_context* ctx, secp256k1_ecdsa_signature
secp256k1_scalar_clear(&msg);
secp256k1_scalar_clear(&non);
secp256k1_scalar_clear(&sec);
secp256k1_scalar_cmov(&r, &secp256k1_scalar_zero, !ret);
secp256k1_scalar_cmov(&s, &secp256k1_scalar_zero, !ret);
secp256k1_scalar_cmov(r, &secp256k1_scalar_zero, !ret);
secp256k1_scalar_cmov(s, &secp256k1_scalar_zero, !ret);
if (recid) {
const int zero = 0;
secp256k1_int_cmov(recid, &zero, !ret);
}
return ret;
}

int secp256k1_ecdsa_sign(const secp256k1_context* ctx, secp256k1_ecdsa_signature *signature, const unsigned char *msg32, const unsigned char *seckey, secp256k1_nonce_function noncefp, const void* noncedata) {
secp256k1_scalar r, s;
int ret;
VERIFY_CHECK(ctx != NULL);
ARG_CHECK(secp256k1_ecmult_gen_context_is_built(&ctx->ecmult_gen_ctx));
ARG_CHECK(msg32 != NULL);
ARG_CHECK(signature != NULL);
ARG_CHECK(seckey != NULL);

ret = secp256k1_ecdsa_sign_inner(ctx, &r, &s, NULL, msg32, seckey, noncefp, noncedata);
secp256k1_ecdsa_signature_save(signature, &r, &s);
return ret;
}
Expand Down
159 changes: 158 additions & 1 deletion src/tests.c
Original file line number Diff line number Diff line change
Expand Up @@ -3118,7 +3118,7 @@ void test_ecmult_multi_batching(void) {
data.pt = pt;
secp256k1_gej_neg(&r2, &r2);

/* Test with empty scratch space. It should compute the correct result using
/* Test with empty scratch space. It should compute the correct result using
* ecmult_mult_simple algorithm which doesn't require a scratch space. */
scratch = secp256k1_scratch_create(&ctx->error_callback, 0);
CHECK(secp256k1_ecmult_multi_var(&ctx->error_callback, &ctx->ecmult_ctx, scratch, &r, &scG, ecmult_multi_callback, &data, n_points));
Expand Down Expand Up @@ -5292,6 +5292,161 @@ void run_memczero_test(void) {
CHECK(memcmp(buf1, buf2, sizeof(buf1)) == 0);
}

void int_cmov_test(void) {
int r = INT_MAX;
int a = 0;

secp256k1_int_cmov(&r, &a, 0);
CHECK(r == INT_MAX);

r = 0; a = INT_MAX;
secp256k1_int_cmov(&r, &a, 1);
CHECK(r == INT_MAX);

a = 0;
secp256k1_int_cmov(&r, &a, 1);
CHECK(r == 0);

a = 1;
secp256k1_int_cmov(&r, &a, 1);
CHECK(r == 1);

r = 1; a = 0;
secp256k1_int_cmov(&r, &a, 0);
CHECK(r == 1);

}

void fe_cmov_test(void) {
static const secp256k1_fe zero = SECP256K1_FE_CONST(0, 0, 0, 0, 0, 0, 0, 0);
static const secp256k1_fe one = SECP256K1_FE_CONST(0, 0, 0, 0, 0, 0, 0, 1);
static const secp256k1_fe max = SECP256K1_FE_CONST(
0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL,
0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL
);
secp256k1_fe r = max;
secp256k1_fe a = zero;

secp256k1_fe_cmov(&r, &a, 0);
CHECK(memcmp(&r, &max, sizeof(r)) == 0);

r = zero; a = max;
secp256k1_fe_cmov(&r, &a, 1);
CHECK(memcmp(&r, &max, sizeof(r)) == 0);

a = zero;
secp256k1_fe_cmov(&r, &a, 1);
CHECK(memcmp(&r, &zero, sizeof(r)) == 0);

a = one;
secp256k1_fe_cmov(&r, &a, 1);
CHECK(memcmp(&r, &one, sizeof(r)) == 0);

r = one; a = zero;
secp256k1_fe_cmov(&r, &a, 0);
CHECK(memcmp(&r, &one, sizeof(r)) == 0);
}

void fe_storage_cmov_test(void) {
static const secp256k1_fe_storage zero = SECP256K1_FE_STORAGE_CONST(0, 0, 0, 0, 0, 0, 0, 0);
static const secp256k1_fe_storage one = SECP256K1_FE_STORAGE_CONST(0, 0, 0, 0, 0, 0, 0, 1);
static const secp256k1_fe_storage max = SECP256K1_FE_STORAGE_CONST(
0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL,
0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL
);
secp256k1_fe_storage r = max;
secp256k1_fe_storage a = zero;

secp256k1_fe_storage_cmov(&r, &a, 0);
CHECK(memcmp(&r, &max, sizeof(r)) == 0);

r = zero; a = max;
secp256k1_fe_storage_cmov(&r, &a, 1);
CHECK(memcmp(&r, &max, sizeof(r)) == 0);

a = zero;
secp256k1_fe_storage_cmov(&r, &a, 1);
CHECK(memcmp(&r, &zero, sizeof(r)) == 0);

a = one;
secp256k1_fe_storage_cmov(&r, &a, 1);
CHECK(memcmp(&r, &one, sizeof(r)) == 0);

r = one; a = zero;
secp256k1_fe_storage_cmov(&r, &a, 0);
CHECK(memcmp(&r, &one, sizeof(r)) == 0);
}

void scalar_cmov_test(void) {
static const secp256k1_scalar zero = SECP256K1_SCALAR_CONST(0, 0, 0, 0, 0, 0, 0, 0);
static const secp256k1_scalar one = SECP256K1_SCALAR_CONST(0, 0, 0, 0, 0, 0, 0, 1);
static const secp256k1_scalar max = SECP256K1_SCALAR_CONST(
0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL,
0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL
);
secp256k1_scalar r = max;
secp256k1_scalar a = zero;

secp256k1_scalar_cmov(&r, &a, 0);
CHECK(memcmp(&r, &max, sizeof(r)) == 0);

r = zero; a = max;
secp256k1_scalar_cmov(&r, &a, 1);
CHECK(memcmp(&r, &max, sizeof(r)) == 0);

a = zero;
secp256k1_scalar_cmov(&r, &a, 1);
CHECK(memcmp(&r, &zero, sizeof(r)) == 0);

a = one;
secp256k1_scalar_cmov(&r, &a, 1);
CHECK(memcmp(&r, &one, sizeof(r)) == 0);

r = one; a = zero;
secp256k1_scalar_cmov(&r, &a, 0);
CHECK(memcmp(&r, &one, sizeof(r)) == 0);
}

void ge_storage_cmov_test(void) {
static const secp256k1_ge_storage zero = SECP256K1_GE_STORAGE_CONST(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0);
static const secp256k1_ge_storage one = SECP256K1_GE_STORAGE_CONST(0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1);
static const secp256k1_ge_storage max = SECP256K1_GE_STORAGE_CONST(
0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL,
0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL,
0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL,
0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL, 0xFFFFFFFFUL
);
secp256k1_ge_storage r = max;
secp256k1_ge_storage a = zero;

secp256k1_ge_storage_cmov(&r, &a, 0);
CHECK(memcmp(&r, &max, sizeof(r)) == 0);

r = zero; a = max;
secp256k1_ge_storage_cmov(&r, &a, 1);
CHECK(memcmp(&r, &max, sizeof(r)) == 0);

a = zero;
secp256k1_ge_storage_cmov(&r, &a, 1);
CHECK(memcmp(&r, &zero, sizeof(r)) == 0);

a = one;
secp256k1_ge_storage_cmov(&r, &a, 1);
CHECK(memcmp(&r, &one, sizeof(r)) == 0);

r = one; a = zero;
secp256k1_ge_storage_cmov(&r, &a, 0);
CHECK(memcmp(&r, &one, sizeof(r)) == 0);
}

void run_cmov_tests(void) {
int_cmov_test();
fe_cmov_test();
fe_storage_cmov_test();
scalar_cmov_test();
ge_storage_cmov_test();
}

int main(int argc, char **argv) {
unsigned char seed16[16] = {0};
unsigned char run32[32] = {0};
Expand Down Expand Up @@ -5431,6 +5586,8 @@ int main(int argc, char **argv) {
/* util tests */
run_memczero_test();

run_cmov_tests();

secp256k1_rand256(run32);
printf("random run = %02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x%02x\n", run32[0], run32[1], run32[2], run32[3], run32[4], run32[5], run32[6], run32[7], run32[8], run32[9], run32[10], run32[11], run32[12], run32[13], run32[14], run32[15]);

Expand Down
14 changes: 14 additions & 0 deletions src/util.h
Original file line number Diff line number Diff line change
Expand Up @@ -194,4 +194,18 @@ static SECP256K1_INLINE void memczero(void *s, size_t len, int flag) {
}
}

/** If flag is true, set *r equal to *a; otherwise leave it. Constant-time. Both *r and *a must be initialized and non-negative.*/
static SECP256K1_INLINE void secp256k1_int_cmov(int *r, const int *a, int flag) {
unsigned int mask0, mask1, r_masked, a_masked;
/* Casting a negative int to unsigned and back to int is implementation defined behavior */
VERIFY_CHECK(*r >= 0 && *a >= 0);

mask0 = (unsigned int)flag + ~0u;
mask1 = ~mask0;
r_masked = ((unsigned int)*r & mask0);
a_masked = ((unsigned int)*a & mask1);

*r = (int)(r_masked | a_masked);
}

#endif /* SECP256K1_UTIL_H */
19 changes: 19 additions & 0 deletions src/valgrind_ctime_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@
# include "include/secp256k1_ecdh.h"
#endif

#if ENABLE_MODULE_RECOVERY
# include "include/secp256k1_recovery.h"
#endif

int main(void) {
secp256k1_context* ctx;
secp256k1_ecdsa_signature signature;
Expand All @@ -24,6 +28,10 @@ int main(void) {
unsigned char key[32];
unsigned char sig[74];
unsigned char spubkey[33];
#if ENABLE_MODULE_RECOVERY
secp256k1_ecdsa_recoverable_signature recoverable_signature;
int recid;
#endif

if (!RUNNING_ON_VALGRIND) {
fprintf(stderr, "This test can only usefully be run inside valgrind.\n");
Expand Down Expand Up @@ -67,6 +75,17 @@ int main(void) {
CHECK(ret == 1);
#endif

#if ENABLE_MODULE_RECOVERY
/* Test signing a recoverable signature. */
VALGRIND_MAKE_MEM_UNDEFINED(key, 32);
ret = secp256k1_ecdsa_sign_recoverable(ctx, &recoverable_signature, msg, key, NULL, NULL);
VALGRIND_MAKE_MEM_DEFINED(&recoverable_signature, sizeof(recoverable_signature));
VALGRIND_MAKE_MEM_DEFINED(&ret, sizeof(ret));
CHECK(ret);
CHECK(secp256k1_ecdsa_recoverable_signature_serialize_compact(ctx, sig, &recid, &recoverable_signature));
CHECK(recid >= 0 && recid <= 3);
#endif

VALGRIND_MAKE_MEM_UNDEFINED(key, 32);
ret = secp256k1_ec_seckey_verify(ctx, key);
VALGRIND_MAKE_MEM_DEFINED(&ret, sizeof(ret));
Expand Down

0 comments on commit 2ed54da

Please sign in to comment.