Skip to content

Commit

Permalink
Merge bitcoin-core/secp256k1#1348: tighten group magnitude limits, sa…
Browse files Browse the repository at this point in the history 
…ve normalize_weak calls in group add methods (revival of #1032)

b7c685e Save _normalize_weak calls in group add methods (Peter Dettman)
c83afa6 Tighten group magnitude limits (Peter Dettman)
173e8d0 Implement current magnitude assumptions (Peter Dettman)
49afd2f Take use of _fe_verify_magnitude in field_impl.h (Sebastian Falbesoner)
4e9661f Add _fe_verify_magnitude (no-op unless VERIFY is enabled) (Peter Dettman)
690b0fc add missing group element invariant checks (Sebastian Falbesoner)

Pull request description:

  This PR picks up #1032 by peterdettman. It's essentially a rebase on master; the original first commit (09dbba561fdb9d57a2cc9842ce041d9ba29a6189) which introduced group verification methods has mostly been replaced by PR #1299 (commit f202667) and what remains now is only adding a few missing checks at some places. The remaining commits are unchanged, though some (easy-to-solve) conflicts appeared through cherry-picking. The last commit which actually removes the `normalize_weak` calls is obviously the critical one and needs the most attention for review.

ACKs for top commit:
  sipa:
    utACK b7c685e
  real-or-random:
    ACK b7c685e
  jonasnick:
    ACK b7c685e

Tree-SHA512: f15167eff7ef6ed971c726a4d738de9a15be95b0c947d7e38329e7b16656202b7113497d36625304e784866349f2293f6f1d8cb97df35393af9ea465a4156da3
  • Loading branch information
@real-or-random
real-or-random committed Aug 16, 2023
2 parents b2f6712 + b7c685e commit eedd781
Show file tree
Hide file tree
Showing 5 changed files with 182 additions and 73 deletions.
3 changes: 3 additions & 0 deletions src/field.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -352,4 +352,7 @@ static int secp256k1_fe_is_square_var(const secp256k1_fe *a);
/** Check invariants on a field element (no-op unless VERIFY is enabled). */
static void secp256k1_fe_verify(const secp256k1_fe *a);

/** Check that magnitude of a is at most m (no-op unless VERIFY is enabled). */
static void secp256k1_fe_verify_magnitude(const secp256k1_fe *a, int m);

#endif /* SECP256K1_FIELD_H */
31 changes: 19 additions & 12 deletions src/field_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ SECP256K1_INLINE static int secp256k1_fe_equal(const secp256k1_fe *a, const secp
#ifdef VERIFY
secp256k1_fe_verify(a);
secp256k1_fe_verify(b);
VERIFY_CHECK(a->magnitude <= 1);
VERIFY_CHECK(b->magnitude <= 31);
secp256k1_fe_verify_magnitude(a, 1);
secp256k1_fe_verify_magnitude(b, 31);
#endif
secp256k1_fe_negate(&na, a, 1);
secp256k1_fe_add(&na, b);
Expand All @@ -36,8 +36,8 @@ SECP256K1_INLINE static int secp256k1_fe_equal_var(const secp256k1_fe *a, const
#ifdef VERIFY
secp256k1_fe_verify(a);
secp256k1_fe_verify(b);
VERIFY_CHECK(a->magnitude <= 1);
VERIFY_CHECK(b->magnitude <= 31);
secp256k1_fe_verify_magnitude(a, 1);
secp256k1_fe_verify_magnitude(b, 31);
#endif
secp256k1_fe_negate(&na, a, 1);
secp256k1_fe_add(&na, b);
Expand All @@ -60,7 +60,7 @@ static int secp256k1_fe_sqrt(secp256k1_fe * SECP256K1_RESTRICT r, const secp256k
#ifdef VERIFY
VERIFY_CHECK(r != a);
secp256k1_fe_verify(a);
VERIFY_CHECK(a->magnitude <= 8);
secp256k1_fe_verify_magnitude(a, 8);
#endif

/** The binary representation of (p + 1)/4 has 3 blocks of 1s, with lengths in
Expand Down Expand Up @@ -159,19 +159,26 @@ static int secp256k1_fe_sqrt(secp256k1_fe * SECP256K1_RESTRICT r, const secp256k

#ifndef VERIFY
static void secp256k1_fe_verify(const secp256k1_fe *a) { (void)a; }
static void secp256k1_fe_verify_magnitude(const secp256k1_fe *a, int m) { (void)a; (void)m; }
#else
static void secp256k1_fe_impl_verify(const secp256k1_fe *a);
static void secp256k1_fe_verify(const secp256k1_fe *a) {
/* Magnitude between 0 and 32. */
VERIFY_CHECK((a->magnitude >= 0) && (a->magnitude <= 32));
secp256k1_fe_verify_magnitude(a, 32);
/* Normalized is 0 or 1. */
VERIFY_CHECK((a->normalized == 0) || (a->normalized == 1));
/* If normalized, magnitude must be 0 or 1. */
if (a->normalized) VERIFY_CHECK(a->magnitude <= 1);
if (a->normalized) secp256k1_fe_verify_magnitude(a, 1);
/* Invoke implementation-specific checks. */
secp256k1_fe_impl_verify(a);
}

static void secp256k1_fe_verify_magnitude(const secp256k1_fe *a, int m) {
VERIFY_CHECK(m >= 0);
VERIFY_CHECK(m <= 32);
VERIFY_CHECK(a->magnitude <= m);
}

static void secp256k1_fe_impl_normalize(secp256k1_fe *r);
SECP256K1_INLINE static void secp256k1_fe_normalize(secp256k1_fe *r) {
secp256k1_fe_verify(r);
Expand Down Expand Up @@ -293,7 +300,7 @@ static void secp256k1_fe_impl_negate_unchecked(secp256k1_fe *r, const secp256k1_
SECP256K1_INLINE static void secp256k1_fe_negate_unchecked(secp256k1_fe *r, const secp256k1_fe *a, int m) {
secp256k1_fe_verify(a);
VERIFY_CHECK(m >= 0 && m <= 31);
VERIFY_CHECK(a->magnitude <= m);
secp256k1_fe_verify_magnitude(a, m);
secp256k1_fe_impl_negate_unchecked(r, a, m);
r->magnitude = m + 1;
r->normalized = 0;
Expand Down Expand Up @@ -326,8 +333,8 @@ static void secp256k1_fe_impl_mul(secp256k1_fe *r, const secp256k1_fe *a, const
SECP256K1_INLINE static void secp256k1_fe_mul(secp256k1_fe *r, const secp256k1_fe *a, const secp256k1_fe * SECP256K1_RESTRICT b) {
secp256k1_fe_verify(a);
secp256k1_fe_verify(b);
VERIFY_CHECK(a->magnitude <= 8);
VERIFY_CHECK(b->magnitude <= 8);
secp256k1_fe_verify_magnitude(a, 8);
secp256k1_fe_verify_magnitude(b, 8);
VERIFY_CHECK(r != b);
VERIFY_CHECK(a != b);
secp256k1_fe_impl_mul(r, a, b);
Expand All @@ -339,7 +346,7 @@ SECP256K1_INLINE static void secp256k1_fe_mul(secp256k1_fe *r, const secp256k1_f
static void secp256k1_fe_impl_sqr(secp256k1_fe *r, const secp256k1_fe *a);
SECP256K1_INLINE static void secp256k1_fe_sqr(secp256k1_fe *r, const secp256k1_fe *a) {
secp256k1_fe_verify(a);
VERIFY_CHECK(a->magnitude <= 8);
secp256k1_fe_verify_magnitude(a, 8);
secp256k1_fe_impl_sqr(r, a);
r->magnitude = 1;
r->normalized = 0;
Expand Down Expand Up @@ -418,7 +425,7 @@ SECP256K1_INLINE static void secp256k1_fe_get_bounds(secp256k1_fe* r, int m) {
static void secp256k1_fe_impl_half(secp256k1_fe *r);
SECP256K1_INLINE static void secp256k1_fe_half(secp256k1_fe *r) {
secp256k1_fe_verify(r);
VERIFY_CHECK(r->magnitude < 32);
secp256k1_fe_verify_magnitude(r, 31);
secp256k1_fe_impl_half(r);
r->magnitude = (r->magnitude >> 1) + 1;
r->normalized = 0;
Expand Down
8 changes: 8 additions & 0 deletions src/group.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,14 @@ typedef struct {

#define SECP256K1_GE_STORAGE_CONST_GET(t) SECP256K1_FE_STORAGE_CONST_GET(t.x), SECP256K1_FE_STORAGE_CONST_GET(t.y)

/** Maximum allowed magnitudes for group element coordinates
* in affine (x, y) and jacobian (x, y, z) representation. */
#define SECP256K1_GE_X_MAGNITUDE_MAX 4
#define SECP256K1_GE_Y_MAGNITUDE_MAX 3
#define SECP256K1_GEJ_X_MAGNITUDE_MAX 4
#define SECP256K1_GEJ_Y_MAGNITUDE_MAX 4
#define SECP256K1_GEJ_Z_MAGNITUDE_MAX 1

/** Set a group element equal to the point with given X and Y coordinates */
static void secp256k1_ge_set_xy(secp256k1_ge *r, const secp256k1_fe *x, const secp256k1_fe *y);

Expand Down
Loading

0 comments on commit eedd781

Please sign in to comment.