320589 - Code cleanup in lib/freebl/ec.c

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

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

Comment 1

[Wan-Teh Chang]

Attachment: 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.

Comment 2

[Robert Relyea]

Comment on attachment 206145 [details] [diff] [review]
Proposed patch

r=relyea

Comment 3

[Wan-Teh Chang]

Attachment: 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.

Comment 4

[Robert Relyea]

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 .

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 6

[Wan-Teh Chang]

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

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 8

[Wan-Teh Chang]

Attachment: 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 9

[Nelson Bolyard (seldom reads bugmail)]

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 */

Comment 10

[Wan-Teh Chang]

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.

Comment 11

[Wan-Teh Chang]

Attachment: 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.

Comment 12

[Wan-Teh Chang]

Attachment: 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.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh, please request my review to these patches in bugzilla so they 
show up in my review request list.

Comment 14

[Wan-Teh Chang]

Attachment: 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.

Comment 15

[Wan-Teh Chang]

Attachment: 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).

Comment 16

[Robert Relyea]

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

r+ = relyea

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 18

[Wan-Teh Chang]

Attachment: 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).

Comment 19

[Wan-Teh Chang]

Attachment: 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.