Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recovery signing: add to constant time test, and eliminate non ct operators #755

Merged
merged 3 commits into from
Jun 8, 2020
Merged

Recovery signing: add to constant time test, and eliminate non ct operators #755

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2876af4
Split ecdsa_sign logic into a new function and use it from ecdsa_sign…
elichai May 26, 2020
73596a8
Add ecdsa_sign_recoverable to the ctime tests
elichai May 26, 2020
2860950
Add tests for the cmov implementations
elichai May 31, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 3 additions & 36 deletions src/modules/recovery/main_impl.h
View file
Open in desktop
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
View file
Open in desktop
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? recid is not secret.

Copy link
Contributor Author

@elichai elichai Jun 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem isn't recid being secret, it's ret being secret.
(ret includes the validity of seckey so we don't want to branch over it)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there is much of an issue with declassifying ret. If the API is used correctly, ret is always 1 anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we assume a user won't call sign_* without verifying the secret key first then yeah we can declassify is_sec_valid (which is what contaminates ret)

secp256k1/src/secp256k1.c

Lines 510 to 513 in a39c2b0

/* We don't want to declassify is_sec_valid and therefore the range of
* seckey. As a result is_sec_valid is included in ret only after ret was
* used as a branching variable. */
ret &= is_sec_valid;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sipa This is not the scope of this PR. If we want to change this, I think that's a different discussion.

The current signing code on master is overly cautious and assumes is_sec_valid to be a secret. That's exactly the reason why we cmov r and s (introduced by #710), see also the corresponding code comment and the exact reasoning in the PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@real-or-random Thanks for the references, that makes sense. My point was mostly that at some point, the complexity of removing decreasingly relevant forms of non-ctness aren't worth it, but this isn't too bad yet.

}
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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