Create a test to check all EC curves

RESOLVED FIXED in 3.11.2

Status

NSS
Test
P1
enhancement
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Christophe Ravel, Assigned: Christophe Ravel)

Tracking

3.11.1
3.11.2

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

12 years ago
We need a test to check all EC curves.

For each curve
- create a request
- sign the request
- import the certificate
(Assignee)

Comment 1

12 years ago
Created attachment 221881 [details] [diff] [review]
Add cert_eccurves to cert.sh

This test does create/sign/import. Is it an overkill ?
Attachment #221881 - Flags: review?(nelson)
Comment on attachment 221881 [details] [diff] [review]
Add cert_eccurves to cert.sh

The only problem I see with this script is that it still cites
the 4 "onb" curves (and it has some strange indentation).
Remove those 4 onb curves, and it should be good to go, I think.
The code snippet below shows what I think the indentation should
look like, and it points out the 4 onb curves.

>+  if [ -n "${NSS_ECC_MORE_THAN_SUITE_B} ] ; then
>+	CURVE_LIST="sect163k1 nistk163 sect163r1 sect163r2 \
>+         nistb163  sect193r1 sect193r2 sect233k1 nistk233 \
>+	   sect233r1 nistb233 sect239k1 sect283k1 nistk283 \
>+	   sect283r1 nistb283 sect409k1 nistk409 sect409r1 \
>+	   nistb409 sect571k1 nistk571 sect571r1 nistb571 \
>+	   secp160k1 secp160r1 secp160r2 secp192k1 secp192r1 \
>+	   nistp192 secp224k1 secp224r1 nistp224 secp256k1 \
>+	   secp256r1 nistp256 secp384r1 nistp384 secp521r1 \
>+	   nistp521 prime192v1 prime192v2 prime192v3 \
>+	   prime239v1 prime239v2 prime239v3 c2pnb163v1 \
>+	   c2pnb163v2 c2pnb163v3 c2pnb176v1 c2tnb191v1 \
>+	   c2tnb191v2 c2tnb191v3 c2onb191v4 c2onb191v5 \
                                 ^^^^^^^^^^^^^^^^^^^^^
>+	   c2pnb208w1 c2tnb239v1 c2tnb239v2 c2tnb239v3 \
>+	   c2onb239v4 c2onb239v5 c2pnb272w1 c2pnb304w1 \
           ^^^^^^^^^^^^^^^^^^^^^
>+	   c2tnb359w1 c2pnb368w1 c2tnb431r1 secp112r1 \
>+	   secp112r2 secp128r1 secp128r2 sect113r1 sect113r2\
>+	   sect131r1 sect131r2"
>+    else
>+	CURVE_LIST="nistp256 nistp384 nistp521"
>+    fi
>+	CERTSERIAL=2000
Attachment #221881 - Flags: review?(nelson) → review-
There one other error in that list of curve names to be tested:

>+	   c2tnb359w1 c2pnb368w1 c2tnb431r1 secp112r1 \
                   ^ should be v, not w
c2tnb359w1 does not exist
c2tnb359v1 does exist, and is (er, should be) implemented.
Assignee: nobody → christophe.ravel.bugs
Severity: normal → enhancement
Priority: -- → P1
Here's an alternative curve list, sorted by family, and size within family.
It omits the "onb" curves, and (hopefully) fixes the one name typo.

CURVE_LIST="\
c2pnb163v1 c2pnb163v2 c2pnb163v3 c2pnb176v1 \
c2pnb208w1 c2pnb272w1 c2pnb304w1 c2pnb368w1 \
c2tnb191v1 c2tnb191v2 c2tnb191v3 c2tnb239v1 \
c2tnb239v2 c2tnb239v3 c2tnb359v1 c2tnb431r1 \
nistb163 nistb233 nistb283 nistb409 nistb571 \
nistk163 nistk233 nistk283 nistk409 nistk571 \
nistp192 nistp224 nistp256 nistp384 nistp521 \
prime192v1 prime192v2 prime192v3 \
prime239v1 prime239v2 prime239v3 \
secp112r1 secp112r2 secp128r1 secp128r2 secp160k1 \
secp160r1 secp160r2 secp192k1 secp192r1 secp224k1 \
secp224r1 secp256k1 secp256r1 secp384r1 secp521r1 \
sect113r1 sect113r2 sect131r1 sect131r2 sect163k1 sect163r1 \
sect163r2 sect193r1 sect193r2 sect233k1 sect233r1 sect239k1 \
sect283k1 sect283r1 sect409k1 sect409r1 sect571k1 sect571r1"
Comment on attachment 221881 [details] [diff] [review]
Add cert_eccurves to cert.sh

Here's yet another problem with this patch, that I found through testing.

>+  if [ -n "${NSS_ECC_MORE_THAN_SUITE_B} ] ; then

Noticed the unbalanced quote.  This causes a syntax error that, 
strangely enough, gets reported against another line about 100 lines down.
Took me about an hour to find this unbalanced quote and fix it.
Am testing a modified version of this patch now.
Created attachment 222304 [details] [diff] [review]
patch v2

This patch is the previous one with a very few changes:
a) list of cipher suites changed as suggested above
b) problem with unbalanced quotes fixed
c) white space changed to match rest of script
This script passes all tests when run against the trunk.

Christophe, please review.
Attachment #221881 - Attachment is obsolete: true
Attachment #222304 - Flags: review?(christophe.ravel.bugs)
(Assignee)

Comment 7

12 years ago
(In reply to comment #3)
> There one other error in that list of curve names to be tested:
> 
> >+	   c2tnb359w1 c2pnb368w1 c2tnb431r1 secp112r1 \
>                    ^ should be v, not w
> c2tnb359w1 does not exist
> c2tnb359v1 does exist, and is (er, should be) implemented.
> 

I took the curve list from "certutil.c"

$ pwd
mozilla/security/nss/cmd

$ grep c2tnb359w1 */*
bltest/blapitest.c:    PRINTUSAGE("",  "",   "  c2tnb359w1, c2pnb368w1, c2tnb431r1, secp112r1,");
certutil/certutil.c:    FPS "%-20s c2tnb359w1, c2pnb368w1, c2tnb431r1, secp112r1, \n", "");

$ grep c2onb */*
bltest/blapitest.c:    PRINTUSAGE("",  "",   "  c2tnb191v2, c2tnb191v3, c2onb191v4, c2onb191v5,");
bltest/blapitest.c:    PRINTUSAGE("",  "",   "  c2onb239v4, c2onb239v5, c2pnb272w1, c2pnb304w1,");
bltest/blapitest.c:  { "c2onb191v4", SEC_OID_ANSIX962_EC_C2ONB191V4 },
bltest/blapitest.c:  { "c2onb191v5", SEC_OID_ANSIX962_EC_C2ONB191V5 },
bltest/blapitest.c:  { "c2onb239v4", SEC_OID_ANSIX962_EC_C2ONB239V4 },
bltest/blapitest.c:  { "c2onb239v5", SEC_OID_ANSIX962_EC_C2ONB239V5 },
certutil/certutil.c:    FPS "%-20s c2tnb191v2, c2tnb191v3, c2onb191v4, c2onb191v5, \n", "");
certutil/certutil.c:    FPS "%-20s c2onb239v4, c2onb239v5, c2pnb272w1, c2pnb304w1, \n", "");
certutil/keystuff.c:  { "c2onb191v4", SEC_OID_ANSIX962_EC_C2ONB191V4 },
certutil/keystuff.c:  { "c2onb191v5", SEC_OID_ANSIX962_EC_C2ONB191V5 },
certutil/keystuff.c:  { "c2onb239v4", SEC_OID_ANSIX962_EC_C2ONB239V4 },
certutil/keystuff.c:  { "c2onb239v5", SEC_OID_ANSIX962_EC_C2ONB239V5 },
fipstest/fipstest.c:  { "c2onb191v4", SEC_OID_ANSIX962_EC_C2ONB191V4 },
fipstest/fipstest.c:  { "c2onb191v5", SEC_OID_ANSIX962_EC_C2ONB191V5 },
fipstest/fipstest.c:  { "c2onb239v4", SEC_OID_ANSIX962_EC_C2ONB239V4 },
fipstest/fipstest.c:  { "c2onb239v5", SEC_OID_ANSIX962_EC_C2ONB239V5 },

I see 2 issues:
- the list of EC curves is duplicated in our code
- c2tnb359w1 needs to be fixed in 2 other files (certutil.c and blapitest.c)
(Assignee)

Comment 8

12 years ago
Comment on attachment 222304 [details] [diff] [review]
patch v2

Changes in mozilla/security/nss/tests/common/init.sh are missing from this patch. Otherwise, the patch is correct.

I have tested the following scenarios:
- NSS_ENABLE_ECC not set
- NSS_ENABLE_ECC set, but not NSS_ECC_MORE_THAN_SUITE_B
- NSS_ENABLE_ECC and NSS_ECC_MORE_THAN_SUITE_B set

The results are correct in all 3 cases.
Attachment #222304 - Flags: review?(christophe.ravel.bugs) → review-
In reply to comment 7, Christophe, see bug 337519
Created attachment 222550 [details] [diff] [review]
marge above two patches

I've combined the missing part from patch 1 with patch 2.
Christophe, please review
Attachment #222304 - Attachment is obsolete: true
Attachment #222550 - Flags: review?(christophe.ravel.bugs)
(Assignee)

Updated

12 years ago
Attachment #222550 - Flags: review?(christophe.ravel.bugs) → review+

Comment 11

12 years ago
Moving to 3.11.3.
Target Milestone: 3.11.2 → 3.11.3
(Assignee)

Comment 12

12 years ago
Committed on the trunk on behalf of Nelson.

Checking in cert/cert.sh;
/cvsroot/mozilla/security/nss/tests/cert/cert.sh,v  <--  cert.sh
new revision: 1.34; previous revision: 1.33
done
Checking in common/init.sh;
/cvsroot/mozilla/security/nss/tests/common/init.sh,v  <--  init.sh
new revision: 1.45; previous revision: 1.44
done
(Assignee)

Comment 13

12 years ago
Committed on NSS_3_11_BRANCH on behalf of Nelson.

Checking in cert/cert.sh;
/cvsroot/mozilla/security/nss/tests/cert/cert.sh,v  <--  cert.sh
new revision: 1.28.2.5; previous revision: 1.28.2.4
done
Checking in common/init.sh;
/cvsroot/mozilla/security/nss/tests/common/init.sh,v  <--  init.sh
new revision: 1.40.24.5; previous revision: 1.40.24.4
done
(Assignee)

Comment 14

12 years ago
Marked as fixed for 3.11.2
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Target Milestone: 3.11.3 → 3.11.2
You need to log in before you can comment on or make changes to this bug.