From a0601cd79c8717577f90962dd51832604a5eddd1 Mon Sep 17 00:00:00 2001 From: Peter Dettman Date: Sat, 4 Jul 2015 13:50:15 +0930 Subject: [PATCH 1/2] Fix VERIFY calculations in _fe_cmov methods --- src/field_10x26_impl.h | 6 ++++-- src/field_5x52_impl.h | 6 ++++-- src/tests.c | 18 ++++++++++++++---- 3 files changed, 22 insertions(+), 8 deletions(-) diff --git a/src/field_10x26_impl.h b/src/field_10x26_impl.h index 871b91f9123..0fc0b8633a4 100644 --- a/src/field_10x26_impl.h +++ b/src/field_10x26_impl.h @@ -1083,8 +1083,10 @@ static SECP256K1_INLINE void secp256k1_fe_cmov(secp256k1_fe_t *r, const secp256k r->n[8] = (r->n[8] & mask0) | (a->n[8] & mask1); r->n[9] = (r->n[9] & mask0) | (a->n[9] & mask1); #ifdef VERIFY - r->magnitude = (r->magnitude & mask0) | (a->magnitude & mask1); - r->normalized = (r->normalized & mask0) | (a->normalized & mask1); + if (a->magnitude > r->magnitude) { + r->magnitude = a->magnitude; + } + r->normalized &= a->normalized; #endif } diff --git a/src/field_5x52_impl.h b/src/field_5x52_impl.h index bda4c3dfc2d..768e7c9a0af 100644 --- a/src/field_5x52_impl.h +++ b/src/field_5x52_impl.h @@ -414,8 +414,10 @@ static SECP256K1_INLINE void secp256k1_fe_cmov(secp256k1_fe_t *r, const secp256k r->n[3] = (r->n[3] & mask0) | (a->n[3] & mask1); r->n[4] = (r->n[4] & mask0) | (a->n[4] & mask1); #ifdef VERIFY - r->magnitude = (r->magnitude & mask0) | (a->magnitude & mask1); - r->normalized = (r->normalized & mask0) | (a->normalized & mask1); + if (a->magnitude > r->magnitude) { + r->magnitude = a->magnitude; + } + r->normalized &= a->normalized; #endif } diff --git a/src/tests.c b/src/tests.c index bcd2d62a4af..71cbe69ab30 100644 --- a/src/tests.c +++ b/src/tests.c @@ -737,6 +737,15 @@ void run_field_convert(void) { CHECK(memcmp(&fes2, &fes, sizeof(fes)) == 0); } +int fe_memcmp(const secp256k1_fe_t *a, const secp256k1_fe_t *b) { + secp256k1_fe_t t = *b; +#ifdef VERIFY + t.magnitude = a->magnitude; + t.normalized = a->normalized; +#endif + return memcmp(a, &t, sizeof(secp256k1_fe_t)); +} + void run_field_misc(void) { secp256k1_fe_t x; secp256k1_fe_t y; @@ -757,12 +766,13 @@ void run_field_misc(void) { q = x; secp256k1_fe_cmov(&x, &z, 0); secp256k1_fe_cmov(&x, &x, 1); - CHECK(memcmp(&x, &z, sizeof(x)) != 0); - CHECK(memcmp(&x, &q, sizeof(x)) == 0); + CHECK(fe_memcmp(&x, &z) != 0); + CHECK(fe_memcmp(&x, &q) == 0); secp256k1_fe_cmov(&q, &z, 1); - CHECK(memcmp(&q, &z, sizeof(q)) == 0); + CHECK(fe_memcmp(&q, &z) == 0); /* Test storage conversion and conditional moves. */ - secp256k1_fe_normalize(&z); + secp256k1_fe_normalize_var(&x); + secp256k1_fe_normalize_var(&z); CHECK(!secp256k1_fe_equal_var(&x, &z)); secp256k1_fe_to_storage(&xs, &x); secp256k1_fe_to_storage(&ys, &y); From 3f3964e49cc163c12a014d60d188aa3835c151a9 Mon Sep 17 00:00:00 2001 From: Peter Dettman Date: Mon, 6 Jul 2015 12:41:30 +0930 Subject: [PATCH 2/2] Add specific VERIFY tests for _fe_cmov --- src/tests.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/src/tests.c b/src/tests.c index 71cbe69ab30..40e2898a2d7 100644 --- a/src/tests.c +++ b/src/tests.c @@ -47,9 +47,7 @@ void random_field_element_magnitude(secp256k1_fe_t *fe) { secp256k1_fe_negate(&zero, &zero, 0); secp256k1_fe_mul_int(&zero, n - 1); secp256k1_fe_add(fe, &zero); -#ifdef VERIFY - CHECK(fe->magnitude == n); -#endif + VERIFY_CHECK(fe->magnitude == n); } void random_group_element_test(secp256k1_ge_t *ge) { @@ -752,7 +750,7 @@ void run_field_misc(void) { secp256k1_fe_t z; secp256k1_fe_t q; secp256k1_fe_t fe5 = SECP256K1_FE_CONST(0, 0, 0, 0, 0, 0, 0, 5); - int i; + int i, j; for (i = 0; i < 5*count; i++) { secp256k1_fe_storage_t xs, ys, zs; random_fe(&x); @@ -765,15 +763,27 @@ void run_field_misc(void) { /* Test fe conditional move; z is not normalized here. */ q = x; secp256k1_fe_cmov(&x, &z, 0); + VERIFY_CHECK(!x.normalized && x.magnitude == z.magnitude); secp256k1_fe_cmov(&x, &x, 1); CHECK(fe_memcmp(&x, &z) != 0); CHECK(fe_memcmp(&x, &q) == 0); secp256k1_fe_cmov(&q, &z, 1); + VERIFY_CHECK(!q.normalized && q.magnitude == z.magnitude); CHECK(fe_memcmp(&q, &z) == 0); - /* Test storage conversion and conditional moves. */ secp256k1_fe_normalize_var(&x); secp256k1_fe_normalize_var(&z); CHECK(!secp256k1_fe_equal_var(&x, &z)); + secp256k1_fe_normalize_var(&q); + secp256k1_fe_cmov(&q, &z, (i&1)); + VERIFY_CHECK(q.normalized && q.magnitude == 1); + for (j = 0; j < 6; j++) { + secp256k1_fe_negate(&z, &z, j+1); + secp256k1_fe_normalize_var(&q); + secp256k1_fe_cmov(&q, &z, (j&1)); + VERIFY_CHECK(!q.normalized && q.magnitude == (j+2)); + } + secp256k1_fe_normalize_var(&z); + /* Test storage conversion and conditional moves. */ secp256k1_fe_to_storage(&xs, &x); secp256k1_fe_to_storage(&ys, &y); secp256k1_fe_to_storage(&zs, &z); @@ -1671,7 +1681,7 @@ void test_ecdsa_end_to_end(void) { extra[31] = 0; extra[0] = 1; CHECK(secp256k1_ecdsa_sign(ctx, message, signature4, &signaturelen4, privkey, NULL, extra) == 1); - CHECK(signaturelen3 > 0); + CHECK(signaturelen4 > 0); CHECK((signaturelen != signaturelen2) || (memcmp(signature, signature2, signaturelen) != 0)); CHECK((signaturelen != signaturelen3) || (memcmp(signature, signature3, signaturelen) != 0)); CHECK((signaturelen3 != signaturelen2) || (memcmp(signature3, signature2, signaturelen3) != 0));