327405 - Error generating EC keypairs (c2pnb163v? curves)

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

Andreas Sterbenz wrote:

> I recompiled NSS with EC_DEBUG set. See attached for output when trying 
> keypair generation for these three c2pnb163v? curves. Apparently it tries 
> to sign and verify some data, but the verification fails.

Douglas Stebilla has written a patch to correct this. 

Attachments forthcoming.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Andreas's test output which revealed the bug

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Douglas's patch with some whitespace changes

Douglas,  I found that the patch you emailed didn't apply cleanly, apparently
because of whitespace changes.  So, I applied it with patch --ignore-whitespace 
and then edited it to restore the tabs and spaces to match the rest of the file.
Please confirm that the attached patch is the same as yours, except for whitespace.  I will ask for two other reviews when you've done that.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 212085 [details] [diff] [review]
Douglas's patch with some whitespace changes

Vipul, please review.

Comment 4

[Vipul Gupta]

(In reply to comment #3)
> (From update of attachment 212085 [details] [diff] [review] [edit])
> Vipul, please review. 

Douglas,

   It looks like this patch is missing the CHECK_GROUP calls
Nelson identified in a separate email. Could you please add 
those? Also, is line 310 in 
http://lxr.mozilla.org/security/source/security/nss/lib/freebl/ecl/ecl.c
missing a call to MP_CHECKOK?
Is there a good reason why the tests in ecl/tests
shouldn't always include all of the relevant curves?

vipul

Comment 5

[Douglas Stebila]

Attachment: Revised patch

This patch addresses Vipul's request in comment #4 that all curves be tested and also has checks in all the right places in ecl.c.  Note that I have removed the CHECK_GROUP and CONS_GF2M macros from ecl.c and replaced them with the actual code.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 212141 [details] [diff] [review]
Revised patch

Douglas, with regard to the changes to ecl.c, 
I think this patch is a big improvement in readability over the original code.  
It makes each section more "regular", so that they are easily compared by 
reviewers looking for unexpected differences.  

Because of that added regularity, it is now even more apparent that, in 
the GFp section of that code, there are 7 places where the "CHECK_GROUP" 
code is immediately followed by a call to MP_CHECKOK(ec_group_set_XXXXXX...)
and 3 places where the "CHEKC_GROUP" code is NOT immediately followed with 
a call to ec_group_set_XXXXX.  That makes it appear that something has 
been omitted from those 3 places, a red flag to a reviewer.

So, let me suggest that you add a comment in each of those 3 places, to 
assure a reviewer that the omission is not accidental, e.g. something like
/* No need to call ec_group_set_XXXX here because <explanation> */

Comment 7

[Douglas Stebila]

Attachment: Revised for comment #6

Patch revised to accomodate requests from comment #6.  Also realized that one of the if cases wasn't necessary so it was removed.

Comment 8

[Vipul Gupta]

(In reply to comment #7)
> Created an attachment (id=212162) [edit]
> Revised for comment #6
> 
> Patch revised to accomodate requests from comment #6.  Also realized that one
> of the if cases wasn't necessary so it was removed.

Douglas,

At the risk of sounding nit picky, I'd like to make a couple of suggestions:

1) In the README file, please add a note on how to test the ec library
    code (e.g. gmake tests; ./ecp_test --time; ./ec2_test ...). I see two
    other programs (ec_naft, ecp_fpt) in the tests so you might want to
    describe those as well.
    If one needs to build the tests in ../mpi first, please mention that
    as well.

    This information will be useful for anyone who wishes to test the ec library
    (e.g. after they add new code etc).

2) At least on Mac OS X, attempts to do "gmake tests" in /mpi as well as /ecl
    fail with 
    "/usr/bin/ld: Undefined symbols:_s_mpi_getProcessorLineSize"

    Please document how to deal with this. I know you have a workaround
    so, at the very least, include it in the README. But if you can fix it,
    even better.

    thank you,

    vipul

Comment 9

[Douglas Stebila]

Attachment: Patch for MPI to build on Mac OS X

This patch address note 2 of comment #8.  It seems like the code that defines the s_mpi_getProcessorLineSize function was not being compiled in the Makefile.  I have changed nss/lib/freebl/mpi/Makefile to compile and link mpcpucache.c.  This patch should be applied in addition to the "Revised for comment #6" patch.

Comment 10

[Douglas Stebila]

(In reply to comment #8)
> 1) In the README file, please add a note on how to test the ec library
>     code (e.g. gmake tests; ./ecp_test --time; ./ec2_test ...). I see two
>     other programs (ec_naft, ecp_fpt) in the tests so you might want to
>     describe those as well.
>     If one needs to build the tests in ../mpi first, please mention that
>     as well.

This is already documented at the end of the README file.  See lines 305 to 330.

Comment 11

[Vipul Gupta]

Comment on attachment 212162 [details] [diff] [review]
Revised for comment #6

Looks fine but I haven't tested it.
Is there a test script to reproduce the bug?

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

I believe Andreas would be willing to test any fix we have.  
If we can supply him with an NSS build containing the fix, 
I believe he is willing to repeat his test to confirm the fix.
(Andreas, please let me know if I am mistaken.)

Comment 13

[Douglas Stebila]

(In reply to comment #11)
> (From update of attachment 212162 [details] [diff] [review] [edit])
> Looks fine but I haven't tested it.
> Is there a test script to reproduce the bug?

If you apply the patch just to lib/freebl/ecl/ecp_test.c but not to the rest of the files, then the ecp_test program will have a test on the c2pnb163v? curves and these tests will fail.  Applying the rest of the patch will result in these tests passing.

Comment 14

[Robert Relyea]

Comment on attachment 213260 [details] [diff] [review]
Patch for MPI to build on Mac OS X

r+= relyea

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 212162 [details] [diff] [review]
Revised for comment #6

sr=nelson
Douglas, do you need someone ot commit this for you?

Comment 16

[Douglas Stebila]

(In reply to comment #15)
> (From update of attachment 212162 [details] [diff] [review] [edit])
> sr=nelson
> Douglas, do you need someone ot commit this for you?

Presumably.  I have never been involved in the checkin process.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 212162 [details] [diff] [review]
Revised for comment #6

Patch committed on trunk.

ecl.c;            new revision: 1.6; previous revision: 1.5
tests/ec2_test.c; new revision: 1.6; previous revision: 1.5
tests/ecp_test.c; new revision: 1.6; previous revision: 1.5

Comment 18

[Vipul Gupta]

(In reply to comment #13)
> (In reply to comment #11)
> > (From update of attachment 212162 [details] [diff] [review] [edit] [edit])
> > Looks fine but I haven't tested it.
> > Is there a test script to reproduce the bug?
> 
> If you apply the patch just to lib/freebl/ecl/ecp_test.c but not to the rest of
> the files, then the ecp_test program will have a test on the c2pnb163v? curves
> and these tests will fail.  Applying the rest of the patch will result in these
> tests passing.

I followed your instruction but didn't see any errors. Then I realized that
you meant to say ec2_test.c (not ecp_test.c) because the curve in question
is a GF(2m) curve. :-)

I have now tested the patch titled "Revised for comment #6" as per the
instructions in the README file and all tests pass.

[I tried sending in this comment earlier but bugzilla detected what it
calls a "mid-air collision" with Nelson's comment# 15 and I didn't
notice it until just now.]

vipul

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 212162 [details] [diff] [review]
Revised for comment #6

Committed on 3.11 branch

ecl.c;            new revision: 1.5.2.1; previous revision: 1.5
tests/ec2_test.c; new revision: 1.5.2.1; previous revision: 1.5
tests/ecp_test.c; new revision: 1.5.2.1; previous revision: 1.5

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

I'm assignign this to Douglas, since he did all the work on it, so that
the bug will give him the credit (or blame :) for the work.
When the last review is done, I'll check it in for him and resolve it fixed.

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 213260 [details] [diff] [review]
Patch for MPI to build on Mac OS X

r=nelson

Comment 22

[Vipul Gupta]

The second patch is ready for check-in and this bug
can be marked resolved/fixed.

vipul

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

Makefile fix checked in on branch and tip

Checking in Makefile;  new revision: 1.22.2.1; previous revision: 1.22
Checking in Makefile;  new revision: 1.23;     previous revision: 1.22

Markign Resolved/Fixed.  Thanks, Douglas.