[PATCH] Move bounds check inside ECC scalar multiply.

[Nick]

The project's README says that contributions are welcome, but it's not that obvious to me how. With that said, I hope this is acceptable:

I've submitted a merge request to your mirror. It makes `compute_kG` and `compute_kP` safer and more consistent to use.

https://salsa.debian.org/gnuk-team/gnuk/gnuk/merge_requests/3

I welcome feedback to help improve this patch to a point where it would be acceptable for inclusion in your great fully free software project. As I said on the MR, I assign my copyright to the FSIJ, assuming it gets accepted.

It's also included below

---
 src/ecc.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/src/ecc.c b/src/ecc.c
index 2d637e9..4437e3e 100644
--- a/src/ecc.c
+++ b/src/ecc.c
@@ -119,6 +119,12 @@ FUNC(compute_kG) (ac *X, const bn256 *K)
   jpc Q[1], tmp[1], *dst;
   int i;
   int vk;
+
+  if (bn256_is_zero(K)) /* < 1, it's too small. */
+    return -1;
+  if (bn256_sub(K_dash, K, N) == 0) /* >= N, it's too big.  */
+    return -1;
+
   uint32_t k_is_even = bn256_is_even (K);
 
   bn256_sub_uint (K_dash, K, k_is_even);
@@ -241,7 +247,6 @@ FUNC(compute_kP) (ac *X, const bn256 *K, const ac *P)
   uint8_t index[86]; /* Lower 2-bit for index absolute value, msb is
 			for sign (encoded as: 0 means 1, 1 means -1).  */
   bn256 K_dash[1];
-  uint32_t k_is_even = bn256_is_even (K);
   jpc Q[1], tmp[1], *dst;
   int i;
   int vk;
@@ -251,9 +256,13 @@ FUNC(compute_kP) (ac *X, const bn256 *K, const ac *P)
   if (point_is_on_the_curve (P) < 0)
     return -1;
 
-  if (bn256_sub (K_dash, K, N) == 0)	/* >= N, it's too big.  */
+  if (bn256_is_zero(K)) /* < 1, it's too small. */
+    return -1;
+  if (bn256_sub(K_dash, K, N) == 0) /* >= N, it's too big.  */
     return -1;
 
+  uint32_t k_is_even = bn256_is_even (K);
+
   bn256_sub_uint (K_dash, K, k_is_even);
   /* It keeps the condition: 1 <= K' <= N - 2, and K' is odd.  */
 
@@ -337,12 +346,9 @@ FUNC(ecdsa) (bn256 *r, bn256 *s, const bn256 *z, const bn256 *d)
       do
 	{
 	  bn256_random (k);
-	  if (bn256_add_uint (k, k, 1))
-	    continue;
-	  if (bn256_sub (tmp_k, k, N) == 0)	/* >= N, it's too big.  */
+	  if(FUNC(compute_kG) (KG, k) < 0)
 	    continue;
 	  /* 1 <= k <= N - 1 */
-	  FUNC(compute_kG) (KG, k);
 	  borrow = bn256_sub (r, KG->x, N);
 	  if (borrow)
 	    memcpy (r, KG->x, sizeof (bn256));
@@ -372,8 +378,8 @@ FUNC(ecdsa) (bn256 *r, bn256 *s, const bn256 *z, const bn256 *d)
 /**
  * @brief Check if a secret d0 is valid or not
  *
- * @param D0	scalar D0: secret
- * @param D1	scalar D1: secret candidate N-D0
+ * @param D0	scalar D0: secret candidate
+ * @param D1	scalar D1: secret candidate projected to N-D0 (output)
  *
  * Return 0 on error.
  * Return -1 when D1 should be used as the secret
@@ -384,12 +390,12 @@ FUNC(check_secret) (const bn256 *d0, bn256 *d1)
 {
   ac Q0[1], Q1[1];
 
-  if (bn256_is_zero (d0) || bn256_sub (d1, N, d0) != 0)
-    /* == 0 or >= N, it's not valid.  */
+  if(FUNC(compute_kG) (Q0, d0) < 0)
+    return 0;
+  bn256_sub (d1, N, d0); // carry is not checked here;
+                         // the bounds check in compute_kG should hace caught the carry case
+  if(FUNC(compute_kG) (Q1, d1) < 0)
     return 0;
-
-  FUNC(compute_kG) (Q0, d0);
-  FUNC(compute_kG) (Q1, d1);
 
   /*
    * Jivsov compliant key check
-- 
2.20.1 (Apple Git-117)