On Sat, 2019-05-11 at 10:00 +0200, Niels Möller wrote:

Simo Sorce simo@redhat.com writes:

...

While reviewing FIPS requirements for public key checks in Ephemeral Diffie-Hellman key exchanges it came out that FIPS requires checks that the public key point is not the (0, 0) coordinate and nettle is not doing it (only checks that neither point is negative.

ecc_point_set also checks that the point is on the curve, i.e., satisfies the curve equation. That should rule out (0, 0), except if we have some curve with constant term b == 0, which I don't think makes sense.

Ah you are right the later check would catch it. I was just following the checks FIPS explicitly requires in order and didn't think about that ...

Not sure how FIPS requirements are formulated, but maybe it would be better to add a test case to check that ecc_point_set rejects (0,0) ?

Yes, I will drop my patch and add a test case.

Simo.

Simo Sorce simo@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.

• 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 ?

Regards, /Niels

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 simo@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.

On Tue, 2019-07-02 at 22:12 +0200, Niels Möller wrote:

Simo Sorce simo@redhat.com writes:

...

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 simo@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.

Merged now.

Thank you!

On Fri, 2019-05-17 at 08:47 +0200, Nikos Mavrogiannopoulos wrote:

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 simo@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.

...

...

• 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?

I replied in private previously,

sorry, never received that reply.

making a point that in fedora we remove the code and disable everything but secp256r1, 384r1 and 521r1. So any tests that use 192r1 or 224r1 will not be executed at all in that platform.

Understood, are you asking to add some tests with other curves ?

Simo.