On Wed, 2019-05-15 at 10:48 -0400, Simo Sorce wrote:
> On Wed, 2019-05-15 at 11:42 +0200, Niels Möller wrote:
> > Simo Sorce <s...@redhat.com> writes:
> > 
> > > Attached find patch that adds points checks to the ECDH test case.
> > > Let me know if that's ok or if you prefer a whole new test.
> > 
> > I think it's ok to have it in the same file.
> > 
> > > -static void
> > > -set_point (struct ecc_point *p,
> > > -    const char *x, const char *y)
> > > +static int
> > > +ret_set_point (struct ecc_point *p,
> > > +               const char *x, const char *y)
> > >  {
> > 
> > I think it's nicer to just change set_point to return int, and wrap
> > all existing calls in ASSERT, e.g,
> > 
> > -  set_point (&A, ax, ay);
> > +  ASSERT (set_point (&A, ax, ay));
> > 
> > in test_dh. Or name functions as int set_point(...), void
> > set_point_or_die (...), but I think ASSERT is still clearer, in this
> > case.
> 
> Ok, will change.

Attached patch that does this.
Nothing else was changed.

> > > +  test_public_key ("(0,0) with secp-192r1", &_nettle_secp_192r1, "0", 
> > > "0", 0);
> > > +  test_public_key (
> > > +    "(P,0) with secp-192r1", &_nettle_secp_192r1,
> > > +    "6277101735386680763835789423207666416083908700390324961279",
> > > +    "0", 0);
> > 
> > Any particular reason the tests are all for secp_192r1 ?
> 
> Less copy-pasting as the numbers are smaller, the curve used really
> makes no difference.
> 
> Nioks,
> is the fact we do not enable 192r1 in some distribution a problem?
> 
> Simo.
> 

-- 
Simo Sorce
Sr. Principal Software Engineer
Red Hat, Inc

From 458e69336f89e818580d3b1674a560ea39880c14 Mon Sep 17 00:00:00 2001
From: Simo Sorce <s...@redhat.com>
Date: Mon, 13 May 2019 15:24:56 -0400
Subject: [PATCH] Add tests that exercise public key checks for ECDH

When performing ECDH the peer provided public key needs to be checked
for validity. FIPS requires basic tests be performed to insure the
provided points are in fact on the selected curve. Those checks already
exists in the ecc_point_set() function.
Add an explicit test that checks the boundaries so that any regression
in checks will be caught.

Signed-off-by: Simo Sorce <s...@redhat.com>
---
 testsuite/ecdh-test.c | 58 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 49 insertions(+), 9 deletions(-)

diff --git a/testsuite/ecdh-test.c b/testsuite/ecdh-test.c
index 2bfffd68..0b39319d 100644
--- a/testsuite/ecdh-test.c
+++ b/testsuite/ecdh-test.c
@@ -31,20 +31,21 @@
 
 #include "testutils.h"
 
-static void
-set_point (struct ecc_point *p,
-	   const char *x, const char *y)
+static int
+set_point (struct ecc_point *p, const char *x, const char *y)
 {
   mpz_t X, Y;
+  int ret;
+
   mpz_init_set_str (X, x, 0);
   mpz_init_set_str (Y, y, 0);
-  if (!ecc_point_set (p, X, Y))
-    die ("Test point not on curve!\n");
+  ret = ecc_point_set (p, X, Y);
 
   mpz_clear (X);
   mpz_clear (Y);
+  return ret;
 }
-  
+
 static void
 set_scalar (struct ecc_scalar *s,
 	    const char *x)
@@ -102,15 +103,15 @@ test_dh (const char *name, const struct ecc_curve *ecc,
   ecc_scalar_init (&A_priv, ecc);
   set_scalar (&A_priv, a_priv);
   ecc_point_init (&A, ecc);
-  set_point (&A, ax, ay);
+  ASSERT (set_point (&A, ax, ay));
 
   ecc_scalar_init (&B_priv, ecc);
   set_scalar (&B_priv, b_priv);
   ecc_point_init (&B, ecc);
-  set_point (&B, bx, by);
+  ASSERT (set_point (&B, bx, by));
 
   ecc_point_init (&S, ecc);
-  set_point (&S, sx, sy);
+  ASSERT (set_point (&S, sx, sy));
 
   ecc_point_init (&T, ecc);
 
@@ -135,9 +136,48 @@ test_dh (const char *name, const struct ecc_curve *ecc,
   ecc_point_clear (&T);  
 }
 
+static void
+test_public_key (const char *label, const struct ecc_curve *ecc,
+                 const char *x, const char *y, int expect_success)
+{
+  struct ecc_point P;
+  int ret;
+
+  ecc_point_init (&P, ecc);
+  ret = set_point (&P, x, y);
+
+  if (!ret && expect_success)
+    die ("Test point '%s' not on curve!\n", label);
+
+  if (ret && !expect_success)
+    die ("Expected failure to set point '%s'!", label);
+
+  ecc_point_clear (&P);
+}
+
 void
 test_main(void)
 {
+  test_public_key ("(0,0) with secp-192r1", &_nettle_secp_192r1, "0", "0", 0);
+  test_public_key (
+    "(P,0) with secp-192r1", &_nettle_secp_192r1,
+    "6277101735386680763835789423207666416083908700390324961279",
+    "0", 0);
+  test_public_key (
+    "(0,P) with secp-192r1", &_nettle_secp_192r1, "0",
+    "6277101735386680763835789423207666416083908700390324961279",
+    0);
+  test_public_key (
+    "(P,P) with secp-192r1", &_nettle_secp_192r1,
+    "6277101735386680763835789423207666416083908700390324961279",
+    "6277101735386680763835789423207666416083908700390324961279",
+    0);
+  test_public_key ("(1,2) with secp-192r1", &_nettle_secp_192r1, "1", "2", 0);
+  test_public_key ("(X,Y) with secp-192r1", &_nettle_secp_192r1,
+    "1050363442265225480786760666329560655512990381040021438562",
+    "5298249600854377235107392014200406283816103564916230704184",
+    1);
+
   test_dh ("secp-192r1", &_nettle_secp_192r1,
 	   "3406157206141798348095184987208239421004566462391397236532",
 	   "1050363442265225480786760666329560655512990381040021438562",
-- 
2.21.0

_______________________________________________
nettle-bugs mailing list
nettle-bugs@lists.lysator.liu.se
http://lists.lysator.liu.se/mailman/listinfo/nettle-bugs

Reply via email to