Merge #1348: tighten group magnitude limits, save normalize_weak call…

…s 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 (09dbba5) 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

src/field.h

@@ -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 */

src/field_impl.h

@@ -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);
@@ -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);
@@ -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
@@ -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);
@@ -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;
@@ -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);
@@ -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;
@@ -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;

src/group.h

@@ -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);