The default bug view has changed. See this FAQ.

Code cleanup in lib/freebl/ec.c

RESOLVED FIXED in 3.11.1

Status

NSS
Libraries
P1
normal
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Wan-Teh Chang, Assigned: Wan-Teh Chang)

Tracking

({coverity})

3.11
3.11.1
coverity
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: ECC FIPS [CID 413])

Attachments

(3 attachments, 6 obsolete attachments)

(Assignee)

Description

11 years ago
While reviewing lib/freebl/ec.c, I found some
problems that are not bugs but should be fixed.
I will attach a patch.
(Assignee)

Comment 1

11 years ago
Created attachment 206145 [details] [diff] [review]
Proposed patch

1. Declare i as unsigned to fix a signed/unsigned compiler
warning.

2. If the output signature SECItem's buffer length is
too short, use the more precise error code
SEC_ERROR_OUTPUT_LEN.

3. Remove unused mp_int variables y1, x2, y2.

4. Remove unused SECItem variables pointA and pointB.
With only one SECItem (pointC) remaining, it is an
overkill to use an arena, so remove the arena as well.

5. If the input signature SECItem's data is too short
or too long, use the more precise error code
SEC_ERROR_INPUT_LEN.  Note that the current code won't
work if signature->len is > 2*len, so I changed the
< 2*len test to the != 2*len test.

6. Make sure we set the error code on all error paths.
Attachment #206145 - Flags: superreview?(nelson)
Attachment #206145 - Flags: review?(rrelyea)
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → 3.11.1

Comment 2

11 years ago
Comment on attachment 206145 [details] [diff] [review]
Proposed patch

r=relyea
Attachment #206145 - Flags: review?(rrelyea) → review+
(Assignee)

Comment 3

11 years ago
Created attachment 208786 [details] [diff] [review]
Proposed patch v2

Summary of this patch.

1. The local variable 'len' is split into two variable
'flen' and 'olen', for the length of the field size
and the length of the base point, respectively.

Recall that the coordinates of the points are elements
of the field, so the length of the field size should be
used with the coordinates of points and EC public keys
(which are points).

The length of the base point order should be used with
the EC private key, signature components r and s, and
to truncate the hash output.

2. Miscellaneous changes to report better error codes.

3. In ECDSA_VerifyDigest, y1, x2, and y2, and pointA
and pointB are not necessary.

4. In ECDSA_VerifyDigest, I only require that
signature->len be even.  I allow signature->len
to be shorter or longer than the expected length (2*len)
to allow adding extraneous leading zeros or omitting leading
zeros.  This flexibility in the signature's length
simplifies the FIPS algorithm test program.  Please
let me know if you object to this change.
Attachment #206145 - Attachment is obsolete: true
Attachment #208786 - Flags: superreview?(nelson)
Attachment #208786 - Flags: review?(rrelyea)
Attachment #206145 - Flags: superreview?(nelson)

Comment 4

11 years ago
Comment on attachment 208786 [details] [diff] [review]
Proposed patch v2

r=relyea.

On inspection of the PKCS #11 spec, the code is correct for r & s <= nLen (length  of order in octects). PKCS #11 does specify applications should not expect r + s > nLen*2 should work.

The hash truncation needs some tweaking for the case where order has leading zeros (both bits and bytes). That is a subject of a separate bug 323817 .
Attachment #208786 - Flags: review?(rrelyea) → review+
Comment on attachment 208786 [details] [diff] [review]
Proposed patch v2

I agree that this change can go in. The fix for bug bug 323817 can be done separately later.
sr=nelson
Attachment #208786 - Flags: superreview?(nelson) → superreview+
(Assignee)

Comment 6

11 years ago
I checked in the patch on the NSS trunk (3.12) and the
NSS_3_11_BRANCH (3.11.1).  My checkin also included changes
to eliminate compiler warnings about signed/unsigned
comparisons and to reconcile the differences (mostly in
comments and whitespaces) between the trunk and NSS_3_11_BRANCH.
(Apparently Bob manually backported his patch for
bug 320583 from the trunk to the NSS_3_11_BRANCH, which
caused those differences.)

Checking in ec.c;
/cvsroot/mozilla/security/nss/lib/freebl/ec.c,v  <--  ec.c
new revision: 1.12; previous revision: 1.11
done

Checking in ec.c;
/cvsroot/mozilla/security/nss/lib/freebl/ec.c,v  <--  ec.c
new revision: 1.9.2.3; previous revision: 1.9.2.2
done
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Reopening.  It seems that the patch (Proposed patch v2) above broke the ECC
version of cert.sh.  The first attempt to sign the first self-signed ECC cert
fails.  The command that fails is:

cert.sh: Creating EC CA Cert TestCA-ec  --------------------------
certutil -s "CN=NSS Test CA (ECC), O=BOGUS NSS, L=Mountain View, ST=California,
C=US" -S -n TestCA-ec -k ec -q secp160r1 -t CTu,CTu,CTu -v 60 -x -d . -1 -2 -5 -
f ../tests.pw.2004 -z ../tests_noise.2004 -m 1

The failure occurs in ec.c at 

>    ecParams = &(key->ecParams);
>    flen = (ecParams->fieldID.size + 7) >> 3;
>    olen = ecParams->order.len;  
>    if (signature->len < 2*olen) {            <====== fails here
>	PORT_SetError(SEC_ERROR_OUTPUT_LEN);
>	goto cleanup;
>    }

signature.len = 40, flen = 20, olen = 21, ecParams->fieldID.size = 160

The stack trace at that point is:

ECDSA_SignDigestWithSeed() line 643
ECDSA_SignDigest() line 839 + 25 bytes
ECDSA_SignDigest() line 1384 + 23 bytes
nsc_ECDSASignStub() line 1646 + 20 bytes
NSC_Sign() line 2013 + 33 bytes
PK11_Sign() line 763 + 40 bytes
SGN_End() line 259 + 17 bytes
SEC_SignData() line 313 + 13 bytes
SEC_DerSignData() line 385 + 25 bytes
CertReq() line 465 + 35 bytes
certutil_main() line 2970 + 114 bytes
main() line 3161 + 15 bytes

I'm afraid this is P1 now, because of the regression.
Status: RESOLVED → REOPENED
Priority: P2 → P1
Resolution: FIXED → ---
Blocks: 324742
(Assignee)

Updated

11 years ago
Blocks: 325498
(Assignee)

Comment 8

11 years ago
Created attachment 212281 [details] [diff] [review]
Patch for PK11_SignatureLen to return the exact ECDSA signature length

This patch fixes PK11_SignatureLen so that it returns
the exact length of ECDSA signatures.

While working on this patch, I tripped over bugs and
missing error handling in NSS, so it took me longer
than expected.  This patch contains the fixes for
those problems.

Writing the new SECKEY_ECParamsToBasePointOrderSize
function required examining the 'n' parameter of every
elliptic curve we support.

Next week I will submit a smaller patch that allows
PK11_SignatureLen to return an inexact signature
length (>= the actual signature length).
Comment on attachment 212281 [details] [diff] [review]
Patch for PK11_SignatureLen to return the exact ECDSA signature length

Wan-Teh, what is the significance of the XXX comments on these lines?
>+    case SEC_OID_ANSIX962_EC_C2ONB191V4: /* XXX */
>+    case SEC_OID_ANSIX962_EC_C2ONB191V5: /* XXX */
>+    case SEC_OID_ANSIX962_EC_PRIME192V1: /* XXX */
>+    case SEC_OID_ANSIX962_EC_C2ONB239V4: /* XXX */
>+    case SEC_OID_ANSIX962_EC_C2ONB239V5: /* XXX */
>+    case SEC_OID_ANSIX962_EC_PRIME256V1: /* XXX */
(Assignee)

Comment 10

11 years ago
Nelson,  The "XXX" comments mean I can't find the
values of base point order n for those curves.
For those curves, SECKEY_ECParamsToBasePointOrderSize
simply returns the length of their key size (= field
size), which is the number in their names.  I will
either find the n values for those curves, or add
a comment to explain what "XXX" means.

If you look at the SECKEY_ECParamsToBasePointOrderSize
code, you will see that for all curves,

  length of base point order n <= length of key size + 1

The alternate patch I plan to submit next week
will take advantage of this fact.
Status: REOPENED → ASSIGNED
(Assignee)

Comment 11

11 years ago
Created attachment 212539 [details] [diff] [review]
Patch for PK11_SignatureLen to return the exact ECDSA signature length, v2

I looked up the parameters for the ANSI X9.62 elliptic
curves marked with the "XXX" comments.  The patch is now
ready for review.
Attachment #212281 - Attachment is obsolete: true
(Assignee)

Comment 12

11 years ago
Created attachment 212545 [details] [diff] [review]
Alternate patch: allow PK11_SignatureLen to return inexact signature length

This is the alternate patch I talked about.  Please
briefly review the original patch and this patch,
and let me know which approach you like better.  We
may want to take the good ideas from both patches.

In this patch, PK11_SignatureLen no longer returns
the signature length.  Instead, it returns the
signature *buffer* length.

Then, we take advantage of the fact that for all the
curves we support,

    base point order length <= key size + 1

This eliminates the need for a new
SECKEY_ECParamsToBasePointOrderLen function.

Finally, we were passing the return value of
PK11_SignatureLen to DSAU_EncodeDerSigWithLen
as the third argument (signature length).  Now
that PK11_SignatureLen may return a length longer
than the signature length, we need to pass the
signature SECItem's 'len' field instead.  This
makes me wonder why DSAU_EncodeDerSigWithLen
needs the third argument.
(Assignee)

Updated

11 years ago
Blocks: 318968
Wan-Teh, please request my review to these patches in bugzilla so they 
show up in my review request list.
Whiteboard: ECC FIPS
(Assignee)

Comment 14

11 years ago
Created attachment 213366 [details] [diff] [review]
Patch for PK11_SignatureLen to return the exact ECDSA signature length, v3

This patch combines the good ideas from the two alternates.

PK11_SignatureLen will return the exact ECDSA signature length,
but the callers of PK11_SignatureLen will allow it to return
inexact signature length.

Please read comment 8 and comment 12 for more information
about this patch.
Attachment #212539 - Attachment is obsolete: true
Attachment #212545 - Attachment is obsolete: true
Attachment #213366 - Flags: superreview?(rrelyea)
Attachment #213366 - Flags: review?(nelson)
(Assignee)

Comment 15

11 years ago
Created attachment 213376 [details] [diff] [review]
Patch for PK11_SignatureLen to return the exact ECDSA signature length, v4

In this version, ECDSA_VerifyDigest enforces the signature
length required by PKCS #11 (see Bob's comment 4 and Tables
57 and 58 of PKCS #11 v2.20 on pages 220 and 221).
Attachment #213366 - Attachment is obsolete: true
Attachment #213376 - Flags: superreview?(rrelyea)
Attachment #213376 - Flags: review?(nelson)
Attachment #213366 - Flags: superreview?(rrelyea)
Attachment #213366 - Flags: review?(nelson)

Comment 16

11 years ago
Comment on attachment 213376 [details] [diff] [review]
Patch for PK11_SignatureLen to return the exact ECDSA signature length, v4

r+ = relyea
Attachment #213376 - Flags: superreview?(rrelyea) → superreview+
Comment on attachment 213376 [details] [diff] [review]
Patch for PK11_SignatureLen to return the exact ECDSA signature length, v4

I have a request/suggestion about the huge switch in function
SECKEY_ECParamsToBasePointOrderLen.  
Please reorder the cases so that:

a) each unique value is returned by only one return statement,
and all cases that return that value are all grouped together
before that return statement. and

b) the return statements (and their respective groups of cases)
are in order of increasing numeric value.

> /*
>- * get the length of a signature object based on the key
>+ * get the length of a buffer large enough to hold a signature object
>+ * based on the key
>  */
> int
> PK11_SignatureLen(SECKEYPrivateKey *key)

My only concern about this change is one of binary compatibility.
It may be that there are programs that rely on this public function to 
return the exact length.  If it no longer does that, some programs might 
begin to produce incorrect results.  I thought there were places in NSS
that expected and depended upon that exactness of length, but lxr doesn't
show any such places in NSS itself.  So, r=nelson
Attachment #213376 - Flags: review?(nelson) → review+
(Assignee)

Comment 18

11 years ago
Created attachment 213687 [details] [diff] [review]
Patch for PK11_SignatureLen to return the exact ECDSA signature length (as checked in)

I omitted the PK11_SignatureLen comment changes that
would allow it to return a length larger than the
signature's exact length.

I checked in this patch on the NSS trunk (3.12) and
NSS_3_11_BRANCH (3.11.1).
Attachment #213376 - Attachment is obsolete: true
(Assignee)

Comment 19

11 years ago
Created attachment 213688 [details]
Diffs between SECKEY_ECParamsToKeySize and SECKEY_ECParamsToBasePointOrderLen

However, I didn't make Nelson's suggested change to
the switch statement in SECKEY_ECParamsToBasePointOrderLen.
The cases in that switch statement are deliberately in
the same order as the cases in SECKEY_ECParamsToKeySize.
This allows an NSS maintainer to see clearly the symmetry
of these two functions, as this diff demonstrates.  Ease
of maintenance is more important than the ease of compiler
optimization.
(Assignee)

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
Keywords: coverity
Whiteboard: ECC FIPS → ECC FIPS [CID 413]
You need to log in before you can comment on or make changes to this bug.