From: Loup Vaillant <loup@loup-vaillant.fr>
Date: Sat, 12 Feb 2022 10:13:22 +0000 (+0100)
Subject: Fix invsqrt() boolean return
X-Git-Url: https://git.codecow.com/?a=commitdiff_plain;h=25f56775b3cc845d4de23873fd53ca72f8f4ca89;p=Monocypher.git

Fix invsqrt() boolean return

The invsqrt() function used to return true when its operand was a
non-zero square.  Thing is, it was used to determine whether it was a
square, *period*.  If the operand was zero, we had the wrong answer.

This introduced two "errors" visible by the end user:

* The all-zero X25519 point could not be mapped back to an Elligator 2
  representative.  In practice that point is never used, but that does
  not comply with the original paper.

* The (0, 1) EdDSa public key could not be serialised, causing any
  signature verification associated with it to fail. This one we really
  don't care, because even though it is on the curve, that point has low
  order, and legitimate public keys all have prime order.

Fixes #231
---

diff --git a/src/monocypher.c b/src/monocypher.c
index 31f1f59..a58a691 100644
--- a/src/monocypher.c
+++ b/src/monocypher.c
@@ -1375,7 +1375,7 @@ static int fe_isequal(const fe f, const fe g)
 }
 
 // Inverse square root.
-// Returns true if x is a non zero square, false otherwise.
+// Returns true if x is a square, false otherwise.
 // After the call:
 //   isr = sqrt(1/x)        if x is a non-zero square.
 //   isr = sqrt(sqrt(-1)/x) if x is not a square.
@@ -1457,6 +1457,7 @@ static int invsqrt(fe isr, const fe x)
     fe_mul(quartic, quartic, x);
 
     i32 *check = t2;
+    fe_0  (check);          int z0 = fe_isequal(x      , check);
     fe_1  (check);          int p1 = fe_isequal(quartic, check);
     fe_neg(check, check );  int m1 = fe_isequal(quartic, check);
     fe_neg(check, sqrtm1);  int ms = fe_isequal(quartic, check);
@@ -1470,7 +1471,7 @@ static int invsqrt(fe isr, const fe x)
     WIPE_BUFFER(t0);
     WIPE_BUFFER(t1);
     WIPE_BUFFER(t2);
-    return p1 | m1;
+    return p1 | m1 | z0;
 }
 
 // Inverse in terms of inverse square root.
@@ -1760,12 +1761,6 @@ static void ge_tobytes(u8 s[32], const ge *h)
 //   isr = invsqrt(num * den)  // abort if not square
 //   x   = num * isr
 // Finally, negate x if its sign is not as specified.
-//
-// Note that using invsqrt causes this function to fail when y = 1.
-// The point (0, 1) *is* on the curve, so in principle we should not
-// reject it.  However, we are only using it to read EdDSA public keys,
-// And the legitimate ones never have low order. Indeed, some libraries
-// reject *all* low order points, on purpose.
 static int ge_frombytes_neg_vartime(ge *h, const u8 s[32])
 {
     fe_frombytes(h->Y, s, 1);
diff --git a/tests/gen/elligator-inverse.py b/tests/gen/elligator-inverse.py
index 8a7245c..d8c0cd0 100755
--- a/tests/gen/elligator-inverse.py
+++ b/tests/gen/elligator-inverse.py
@@ -94,7 +94,7 @@ for cofactor in range(8):
             u.print()
             print(format(tweak, '02x') + ":")
             print('ff:') # Failure
-            print('00:') # dummy value for the hash
+            print(':') # dummy value for the hash
             print()
             break
 
diff --git a/tests/gen/vectors/elligator_inv b/tests/gen/vectors/elligator_inv
index cc53d6b..1948f97 100644
--- a/tests/gen/vectors/elligator_inv
+++ b/tests/gen/vectors/elligator_inv
@@ -1,7 +1,7 @@
 e6f66fdf6e230c603c5e6e59a254ea1476a13eb9511b9549846781e12e52230a:
 00:
 ff:
-00:
+:
 
 46951964003c940878063ccfd0348af42150ca16d2646f2c5856e8338377d800:
 00:
@@ -15,5 +15,5 @@ ff:
 
 0000000000000000000000000000000000000000000000000000000000000000:
 00:
-ff:
 00:
+0000000000000000000000000000000000000000000000000000000000000000:
diff --git a/tests/test.c b/tests/test.c
index 29beb3e..4eb1211 100644
--- a/tests/test.c
+++ b/tests/test.c
@@ -328,9 +328,6 @@ static void elligator_inv(vector_reader *reader)
         printf("Elligator inverse map: failure mismatch\n");
         exit(1);
     }
-    if (check) {
-        out.buf[0] = 0;
-    }
 }
 
 //////////////////////////////