Closed
Bug 327405
Opened 18 years ago
Closed 18 years ago
Error generating EC keypairs (c2pnb163v? curves)
Categories
(NSS :: Libraries, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
3.11.1
People
(Reporter: nelson, Assigned: douglas)
References
Details
(Whiteboard: ECC)
Attachments
(3 files, 2 obsolete files)
9.06 KB,
text/plain
|
Details | |
11.60 KB,
patch
|
vipul.gupta
:
review+
nelson
:
superreview+
|
Details | Diff | Splinter Review |
1.02 KB,
patch
|
rrelyea
:
review+
nelson
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•18 years ago
|
Severity: normal → major
Priority: -- → P1
Reporter | ||
Comment 1•18 years ago
|
||
Reporter | ||
Comment 2•18 years ago
|
||
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.
Attachment #212085 -
Flags: review?(douglas)
Assignee | ||
Updated•18 years ago
|
Attachment #212085 -
Flags: review?(douglas) → review+
Reporter | ||
Comment 3•18 years ago
|
||
Comment on attachment 212085 [details] [diff] [review] Douglas's patch with some whitespace changes Vipul, please review.
Attachment #212085 -
Flags: review?(vipul.gupta)
Comment 4•18 years ago
|
||
(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
Assignee | ||
Updated•18 years ago
|
Attachment #212085 -
Flags: review+ → review-
Assignee | ||
Comment 5•18 years ago
|
||
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.
Attachment #212085 -
Attachment is obsolete: true
Attachment #212141 -
Flags: review?(vipul.gupta)
Attachment #212085 -
Flags: review?(vipul.gupta)
Reporter | ||
Comment 6•18 years ago
|
||
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> */
Assignee | ||
Comment 7•18 years ago
|
||
Patch revised to accomodate requests from comment #6. Also realized that one of the if cases wasn't necessary so it was removed.
Attachment #212141 -
Attachment is obsolete: true
Attachment #212162 -
Flags: review?(vipul.gupta)
Attachment #212141 -
Flags: review?(vipul.gupta)
Updated•18 years ago
|
Assignee: wtchang → nelson
Comment 8•18 years ago
|
||
(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
Assignee | ||
Comment 9•18 years ago
|
||
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.
Attachment #213260 -
Flags: review?(vipul.gupta)
Assignee | ||
Comment 10•18 years ago
|
||
(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.
Reporter | ||
Updated•18 years ago
|
Whiteboard: ECC
Comment 11•18 years ago
|
||
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?
Attachment #212162 -
Flags: review?(vipul.gupta) → review+
Reporter | ||
Comment 12•18 years ago
|
||
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.)
Assignee | ||
Comment 13•18 years ago
|
||
(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•18 years ago
|
||
Comment on attachment 213260 [details] [diff] [review] Patch for MPI to build on Mac OS X r+= relyea
Attachment #213260 -
Flags: review?(vipul.gupta) → review+
Assignee | ||
Updated•18 years ago
|
Attachment #212162 -
Flags: superreview?(nelson)
Reporter | ||
Comment 15•18 years ago
|
||
Comment on attachment 212162 [details] [diff] [review] Revised for comment #6 sr=nelson Douglas, do you need someone ot commit this for you?
Attachment #212162 -
Flags: superreview?(nelson) → superreview+
Assignee | ||
Comment 16•18 years ago
|
||
(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.
Reporter | ||
Comment 17•18 years ago
|
||
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•18 years ago
|
||
(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
Reporter | ||
Comment 19•18 years ago
|
||
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
Reporter | ||
Updated•18 years ago
|
Attachment #213260 -
Flags: review?(nelson)
Reporter | ||
Comment 20•18 years ago
|
||
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.
Assignee: nelson → douglas
Reporter | ||
Comment 21•18 years ago
|
||
Comment on attachment 213260 [details] [diff] [review] Patch for MPI to build on Mac OS X r=nelson
Attachment #213260 -
Flags: review?(nelson) → review+
Comment 22•18 years ago
|
||
The second patch is ready for check-in and this bug can be marked resolved/fixed. vipul
Reporter | ||
Comment 23•18 years ago
|
||
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.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•