Certutil/blapitest usage mentions unimplemented curves

RESOLVED FIXED in 3.11.6

Status

NSS
Tools
P2
minor
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Vipul Gupta, Assigned: Neil Williams)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

12 years ago
Some higher level applications like certutil and blapitest mention ONB elliptic curves in their usage instructions but these curves are not implemented by the underlying elliptic curve library. The usage instructions should not mention these curves.
(Reporter)

Comment 1

12 years ago
This bug was first discussed in late Feb 2006 (see attached emails) but I unintentionally dropped the ball and never got around to filing a bug in bugzilla.

vipul

Message thread 1:

Begin forwarded message:
From: Douglas Stebila <douglas@stebila.ca>
Date: February 20, 2006 5:49:15 PM PST
To: Wan-Teh Chang <wtchang@redhat.com>
Cc: Vipul Gupta <Vipul.Gupta@sun.com>, Nelson B Bolyard <Nelson.Bolyard@sun.com>
Subject: Re: Not all of the ANSI X9.62 curves are listed in lib/freebl/ecl/ecl-curve.h

> Yes, the following are also missing.

> +    case SEC_OID_ANSIX962_EC_C2ONB191V4: /* XXX */
> +    case SEC_OID_ANSIX962_EC_C2ONB191V5: /* XXX */
> +    case SEC_OID_ANSIX962_EC_C2ONB239V4: /* XXX */
> +    case SEC_OID_ANSIX962_EC_C2ONB239V5: /* XXX */

These curves use an ONB, optimal normal basis, representation for the irreducible binary polynomial.  I think we made a decision to not implement these curves because the arithmetic for them is more difficult to implement, but I don't remember for sure.  It may be the case that these curves can use the normal arithmetic code, but that there are other more optimized algorithms; I don't know.  Vipul, Sheueling might know the answer to this.

However, I wouldn't be too worried about these curves being missing from the implementation.  I think most people are leaning toward the NIST and SEC curves in terms of favoured standards.

Douglas


Message thread 2:

Begin forwarded message:
From: Douglas Stebila <douglas@stebila.ca>
Date: February 28, 2006 3:12:12 PM PST
To: Vipul Gupta <Vipul.Gupta@sun.com>
Subject: Re: ONB curves in NSS

Removing them seems fine with me.  I doubt anyone will actually use the ANSI X9.62 curves anyway.

Douglas

On 28-Feb-06, at 6:09 PM, Vipul Gupta wrote:

Yes. I wanted to bring this up for today but we ran out of time.
Instead of trying to add code for these curves, I'd lobby for
simply removing these names from the higher level apps, e.g.
certutil.

What do you think?

vipul

On Feb 28, 2006, at 3:04 PM, Douglas Stebila wrote:

Should this be filed in bugzilla?

Douglas

On 22-Feb-06, at 8:46 PM, Vipul Gupta wrote:

I noticed that the certutil command:

http://lxr.mozilla.org/security/source/security/nss/cmd/certutil/certutil.c#1147
http://lxr.mozilla.org/security/source/security/nss/cmd/certutil/certutil.c#1149

lists four onb curves but, from what I understand now, these aren't
really implemented by ecl.

Do you know what happens today if one invokes certutil with one
of these curves?

Looks like there are several places where NSS makes references to these
curves. Any recommendations on how to address this issue?

vipul



(Reporter)

Comment 2

12 years ago
I investigated this in late Feb/early Mar and here
are my notes on what needs to be done:

In blapitest.c:
  - fix Usage() to not mention any onb curves (~line 193)
  - fix nameTagPair[] by removing the lines for onb curves 
   (~lines 520-527)

In certutil.c/keystuff.c:

  - fix LongUsage() (~lines 1148-1150) to remove mention of onb curves
  - fix nameTagPair[] by removing the lines for onb curves 
   (~lines 433-440)

In fipstest:
  - fix nameTagPair by removing the lines for onb curves
    (~lines 1951-1958)

In seckey.c:
  - remove onb curves from SECKEY_ECParamsToKeySize

(One could also use this opportunity to refactor
nameTagPair so there is just one instance of it).

vipul
OS: Mac OS X 10.3 → All
Priority: -- → P2
Target Milestone: --- → 3.11.2
Version: unspecified → 3.11
Vipul, besides the onb curves, do we support ANY of these?

433   { "c2onb191v4", SEC_OID_ANSIX962_EC_C2ONB191V4 },
434   { "c2onb191v5", SEC_OID_ANSIX962_EC_C2ONB191V5 },
439   { "c2onb239v4", SEC_OID_ANSIX962_EC_C2ONB239V4 },
440   { "c2onb239v5", SEC_OID_ANSIX962_EC_C2ONB239V5 },

426   { "c2pnb163v1", SEC_OID_ANSIX962_EC_C2PNB163V1 },
427   { "c2pnb163v2", SEC_OID_ANSIX962_EC_C2PNB163V2 },
428   { "c2pnb163v3", SEC_OID_ANSIX962_EC_C2PNB163V3 },
429   { "c2pnb176v1", SEC_OID_ANSIX962_EC_C2PNB176V1 },
435   { "c2pnb208w1", SEC_OID_ANSIX962_EC_C2PNB208W1 },
441   { "c2pnb272w1", SEC_OID_ANSIX962_EC_C2PNB272W1 },
442   { "c2pnb304w1", SEC_OID_ANSIX962_EC_C2PNB304W1 },
444   { "c2pnb368w1", SEC_OID_ANSIX962_EC_C2PNB368W1 },

430   { "c2tnb191v1", SEC_OID_ANSIX962_EC_C2TNB191V1 },
431   { "c2tnb191v2", SEC_OID_ANSIX962_EC_C2TNB191V2 },
432   { "c2tnb191v3", SEC_OID_ANSIX962_EC_C2TNB191V3 },
436   { "c2tnb239v1", SEC_OID_ANSIX962_EC_C2TNB239V1 },
437   { "c2tnb239v2", SEC_OID_ANSIX962_EC_C2TNB239V2 },
438   { "c2tnb239v3", SEC_OID_ANSIX962_EC_C2TNB239V3 },
443   { "c2tnb359v1", SEC_OID_ANSIX962_EC_C2TNB359V1 },
445   { "c2tnb431r1", SEC_OID_ANSIX962_EC_C2TNB431R1 },

419   { "prime192v1", SEC_OID_ANSIX962_EC_PRIME192V1 },
420   { "prime192v2", SEC_OID_ANSIX962_EC_PRIME192V2 },
421   { "prime192v3", SEC_OID_ANSIX962_EC_PRIME192V3 },
422   { "prime239v1", SEC_OID_ANSIX962_EC_PRIME239V1 },
423   { "prime239v2", SEC_OID_ANSIX962_EC_PRIME239V2 },
424   { "prime239v3", SEC_OID_ANSIX962_EC_PRIME239V3 },

447   { "secp112r1", SEC_OID_SECG_EC_SECP112R1},
448   { "secp112r2", SEC_OID_SECG_EC_SECP112R2},
449   { "secp128r1", SEC_OID_SECG_EC_SECP128R1},
450   { "secp128r2", SEC_OID_SECG_EC_SECP128R2},

452   { "sect113r1", SEC_OID_SECG_EC_SECT113R1},
453   { "sect113r2", SEC_OID_SECG_EC_SECT113R2},
454   { "sect131r1", SEC_OID_SECG_EC_SECT131R1},
455   { "sect131r2", SEC_OID_SECG_EC_SECT131R2},

Comment 4

12 years ago
(In reply to comment #3)
> Vipul, besides the onb curves, do we support ANY of these?

We support all of those curves except the ONB ones.  You can see the list of curves supported in http://lxr.mozilla.org/security/source/security/nss/lib/freebl/ecl/ecl-exp.h.
Both usage messages also mention one other curve that does not exist:

c2tnb359w1

This appears to just be a typo.  The correct name should be:

c2tnb359v1
        ^  v, not w
Summary: Certutil/blapitest usage mentions unimplemented ONB curves → Certutil/blapitest usage mentions unimplemented curves

Comment 6

12 years ago
c2pnb176v1 is another typo.  The correct name should be:

c2pnb176w1
        ^  w, not v

Comment 7

12 years ago
I should point out that the typo c2pnb176v1 is more
serious because its OID tag SEC_OID_ANSIX962_EC_C2PNB176V1
and string "ANSI X9.62 elliptic curve c2pnb176v1" have the
same typo, and this typo has propagated to PSM.  Since this
curve is disallowed in X9.62-2005, perhaps it's not worth
our time to fix this typo.

Comment 8

12 years ago
Retargetting all P2s to 3.11.3 .
Target Milestone: 3.11.2 → 3.11.3

Comment 9

12 years ago
I listed all elliptic curves supported by NSS and
documented how to generate that list in the NSS
Security Policy:
http://wiki.mozilla.org/Security_Policy#Non-NIST-Recommended_Elliptic_Curves.
Neil, here's another bug that should be quick to fix.
Assignee: nobody → neil.williams
Target Milestone: 3.11.3 → 3.11.5
While you're fixing certutil's list of supported curves, by removing the 
names of unsupported curves and correcting typos in others, here's another
thing to fix.  

certutil's list of curve names is the "extended" list, not the "basic" list.
Even when certutil is being built to feature "basic" ECC support, it 
displays the list of names for "extended" support.  That needs to be fixed,
so that for "basic" support builds, certutil shows only the names of the 
three basic curves.  
(Assignee)

Comment 12

12 years ago
Created attachment 252131 [details] [diff] [review]
fixes 3 problems with help message

This patch ifdefs curve names listed by the help message. Curve c2tnb359w1 has already been renamed to c2tnb359v1; no change was made to c2pnb176v1 after comment 7.

Also added a note to -A and -R to indicate applicable options that are described in other commands (-E and -S, respectively).
Attachment #252131 - Flags: review?
(Assignee)

Updated

12 years ago
Attachment #252131 - Flags: review? → review?(nelson)
Comment on attachment 252131 [details] [diff] [review]
fixes 3 problems with help message

Neil, did you mean to delete this line?  I suspect you did not.  
r+ if you did, r- if you didn't.  

>     FPS "%-20s nistb163, sect193r1, sect193r2, sect233k1, nistk233,\n", "");
>-    FPS "%-20s sect233r1, nistb233, sect239k1, sect283k1, nistk283,\n", "");
>     FPS "%-20s sect283r1, nistb283, sect409k1, nistk409, sect409r1,\n", "");
(Assignee)

Comment 14

12 years ago
Created attachment 252393 [details] [diff] [review]
patch with 2 more fixes in easily checkable form

This patch and the next are equivalent. The next one, which is the one to be checked in, merely has excess blank space removed from it. This is done in hopes it will be easier to review.
(Assignee)

Updated

12 years ago
Attachment #252131 - Flags: review?(nelson)
(Assignee)

Comment 15

12 years ago
Created attachment 252395 [details] [diff] [review]
final patch

This is equivalent to previous patch. It fixes the accidental deletion Nelson mentioned (thanks). It also removes the ONB curves. I have verified that if you try to create a CSR with one of the ONB curves EC_DecodeParams() returns SEC_ERROR_UNSUPPORTED_ELLIPTIC_CURVE but certutil fails with the unhelpful ": An I/O error occurred during security authorization".
Attachment #252131 - Attachment is obsolete: true
Attachment #252395 - Flags: review?(nelson)
Comment on attachment 252395 [details] [diff] [review]
final patch

r=nelson
Requesting second review for 3.11 branch.
Attachment #252395 - Flags: review?(nelson)
Attachment #252395 - Flags: review?(alexei.volkov.bugs)
Attachment #252395 - Flags: review+
(Assignee)

Comment 17

12 years ago
Checking in certutil.c;
/cvsroot/mozilla/security/nss/cmd/certutil/certutil.c,v  <--  certutil.c
new revision: 1.107; previous revision: 1.106
done

checked in on trunk.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Attachment #252395 - Flags: review?(alexei.volkov.bugs) → review+
(Assignee)

Comment 18

12 years ago
Checking in certutil.c;
/cvsroot/mozilla/security/nss/cmd/certutil/certutil.c,v  <--  certutil.c
new revision: 1.97.2.8; previous revision: 1.97.2.7
done

Checked in on 3.11 branch.
(Assignee)

Updated

12 years ago
Target Milestone: 3.11.5 → 3.11.6
You need to log in before you can comment on or make changes to this bug.