339387 - Remove the domain parameters of the WTLS-1, WTLS-8, and WTLS-9 elliptic curves

Status: RESOLVED WONTFIX

(NSS :: Libraries, defect)

Description

[Wan-Teh Chang]

The lib/freebl/ecl library has the domain parameters for
the WTLS-1, WTLS-8, and WTLS-9 elliptic curves in the
ecl-curve.h file, but the NSS libraries can't use these
three curves because
1) we don't have their OIDs in lib/util/secoid.c, and
2) we don't have their OID tags in the big switch statement
in lib/softoken/ecdecode.c, function EC_FillParams.

Since WTLS seems to have been deprecated according to
the "WTLS" entry in wikipedia, I propose that we remove
the domain parameters of these three WTLS curves from
ecl-curve.h rather than adding their OIDs and OID tags
to NSS.

Comment 1

[Wan-Teh Chang]

Attachment: Proposed patch

We could also remove the ECCurveName enumeration constants
for the three WTLS curves, but they are defined in the
exported header file ecl-exp.h, so I just added "not implemented"
comments next to them.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Does this patch remove curves from the list of supported curves?
Do the lists of supported curve names that appear in other places,
such as usage messages and in command line parsing code for certutil
and bltest, also need to be changed? 

Comment 3

[Vipul Gupta]

(In reply to comment #2)

> Do the lists of supported curve names that appear in other places,
> such as usage messages and in command line parsing code for certutil
> and bltest, also need to be changed? 
 
  Usage messages for both certutil and bltest already omit
  these WTLS curves. These curves were only ever visible
  under freebl/ecl (not at the higher layers).

Comment 4

[Wan-Teh Chang]

Attachment: Proposed patch v2

Doing a case-insensitive search for the string "WTLS"
showed that these WTLS curves are only used in lib/freebl/ecl.
But I found that I also need to remove them from the ecl
test programs ec2_test.c and ecp_test.c.

By the way, the comments in
mozilla/security/jss/org/mozilla/jss/pkcs11/PK11KeyPairGenerator.java
showed that Bob also found that we can't get to these three
WTLS curves because we don't have their OIDs.

I'm willing to mark this bug WONTFIX and just treat lib/freebl/ecl
as a third-party library.

Comment 5

[Douglas Stebila]

Comment on attachment 223648 [details] [diff] [review]
Proposed patch v2

The patch looks fine to me.  

However, if it doesn't impact any approval requirements (e.g., FIPS), then, as suggested in comment #4, I'd prefer to see the WTLS curves left enabled in ECL for completeness of ECL as a stand-alone library, disabling them in NSS only above ECL.

Comment 6

[Wan-Teh Chang]

Douglas, Vipul,

OK, I won't remove the domain parameters of the WTLS
curves from the ECL library.  I filed this bug because
I wanted to save some time for the next person who has
to find out which curves are implemented by NSS.  I can
do that by documentation.  I added the following note
to the NSS Security Policy (a document required for FIPS
validation):

http://wiki.mozilla.org/Security_Policy#Non-NIST-Recommended_Elliptic_Curves

  Note: The list of elliptic curves supported by the NSS
  cryptographic module was obtained by taking the intersection
  of the list of curves implemented by the low-level ECL
  library (defined by the ecCurve_map array in
  lib/freebl/ecl/ecl-curve.h) and the list of curves that
  the NSS cryptographic module gives access to (defined by
  the switch statement in the EC_FillParams function in
  lib/softoken/ecdecode.c).

If you could review this note for correctness, I'd appreciate
it.

Comment 7

[Vipul Gupta]

(In reply to comment #6)

> If you could review this note for correctness, I'd appreciate
> it.

  Looks good. 

 One can also gather this info (perhaps more easily) 
 from lines 1168-1407 in lib/util/secoid.c 
 [one would need to know that the ONB curves
  listed in secoid.c aren't really implemented]

  vipul

Comment 8

[Wan-Teh Chang]

Vipul,

You're right.  The switch statement in lib/softoken/ecdecode.c
uses OID tags.  An OID tag may not have a corresponding
entry in the 'oids' table in lib/util/secoid.c.  (This
oversight/programming error is unlikely but possible.)
I did look at the OIDs in lib/util/secoid.c when I
generated the list of supported curves.

I changed my note to say:

  Note: The list of elliptic curves supported by the NSS
  cryptographic module was obtained by taking the
  intersection of

    * the list of curves implemented by the low-level ECL
      library (defined by the ecCurve_map array in
      lib/freebl/ecl/ecl-curve.h),
    * the list of curves whose OIDs NSS recognizes (defined
      by the section "Named curves" in the oids array in
      lib/util/secoid.c), and
    * the list of curves that the NSS cryptographic module
      gives access to (defined by the switch statement in
      the EC_FillParams function in lib/softoken/ecdecode.c). 

Comment 9

[Wan-Teh Chang]

Attachment: Add a comment marking the end of named curve OIDs

This patch adds a comment to mark the end of named curve
OIDs in the 'oids' array.  (The comment /* Named curves */
marks the beginning of those OIDs in the array.)

Comment 10

[Wan-Teh Chang]

Comment on attachment 223864 [details] [diff] [review]
Add a comment marking the end of named curve OIDs

I just noticed that the order of the entries in
the 'oids' array is important and new OIDS must
be added to the end of the array, so this comment
won't make sense when we add new named curve OIDs.
Marked it obsolete.

Comment 11

[Vipul Gupta]

(In reply to comment #10)
> (From update of attachment 223864 [details] [diff] [review] [edit])
> I just noticed that the order of the entries in
> the 'oids' array is important and new OIDS must
> be added to the end of the array

  Yes. This is correct. The ordering in secoid.c needs to
  match the ordering in secoidt.h and inserting
  things in the middle will break backward compatibility.

>, so this comment
> won't make sense when we add new named curve OIDs.
> Marked it obsolete.

  Sorry, I didn't fully realize the intention behind
  your patch (gave it r+ because it seemed harmless).
  Any additional named curves would need to be
  added to the end of secoidt.h and secoid.c

  vipul