338599 - NSS ECDSA signature length incompatible with other implementations for some curves

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

This bug is a "second hand" report of a failure found by members of the 
ECC interoperability forum.  I may have misunderstood a detail or two.

In interoperability testing with another vendor's TLS+ECC server 
implementation, failures were experienced with these cipher suites:
TLS_ECDH_ECDSA_WITH_RC4_128_SHA  and 
TLS_ECDHE_ECDSA_WITH_RC4_128_SHA. 
when the third party server's cert used either of these curves:
sect233k1
sect409k1 

The problem reportedly is that NSS computed the acceptable signature 
length incorrectly, and then found the signature to be invalid.

Wan-Teh wrote:  

> When NSS decodes the signed integers [...], it incorrectly uses the 
> curve's field size as opposed to the base point order size 
> [for the signature length].

> The attached patch for NSS is enough to make Firefox connect to [the test 
> server] with the sect233k1 curve.  I haven't done a thorough review of 
> NSS to find other places that also need to be fixed.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Wan-Teh's patch for NSS_3_11_BRANCH

Comment 2

[Wan-Teh Chang]

I don't have time to work on this bug. Any volunteer?

My patch points out all the places in our code that we need
to fix, but SECKEY_ECParamsToBasePointOrderLen may not
be the right function to export.  One problem is that
it requires us to use a member of SECKEYPublicKey
(&key->u.ec.DEREncodedParams), which probably should be
treated as an opaque structure.  I think a new function
(SECKEY_PublicKeySignatureLen?) that takes 'SECKEYPublicKey *'
as argument would be more appropriate. (cf: PK11_SignatureLen)

One solution that doesn't require exporting any function
is to modify common_DecodeDerSig to handle len=0 as a
special case -- if len is 0, it will pad the r and s
to the same length (but restrict the length to be <=
MAX_ECKEY_LEN).  The drawback of this approach is
that it violates the recommendation of PKCS #11 v2.20
Sec. 12.3.1 "EC Signatures":

  Note: For applications, it is recommended to encode
  the signature as an octet string of length two times
  nLen if possible. This ensures that the application
  works with PKCS#11 modules which have been implemented
  based on an older version of this document. Older
  versions required all signatures to have length two
  times nLen.  It may be impossible to encode the
  signature with the maximum length of two times nLen
  if the application just gets the integer values of r
  and s (i.e. without leading zeros), but does not know
  the base point order n, because r and s can have any
  value between zero and the base point order n.

Comment 3

[Wan-Teh Chang]

Attachment: Proposed patch

I added a new function SECKEY_PublicKeySignatureLen.
It's modeled after SECKEY_PublicKeyStrength, returning
unsigned (int).  This function could also be in
lib/pk11wrap and modeled after PK11_SignatureLen,
returning int.  Not sure which directory it is more
appropriate for.

Comment 4

[Wan-Teh Chang]

Attachment: Proposed patch for NSS_3_11_BRANCH

secvfy.c and nss.def differ enough between the trunk and
NSS_3_11_BRANCH that I had to create a separate patch for
the NSS_3_11_BRANCH.

Comment 5

[Wan-Teh Chang]

Attachment: More conservative patch for NSS_3_11_BRANCH

This patch may be more suitable for the NSS_3_11_BRANCH.
This patch doesn't change the DSA-handling code.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

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

r=nelson
Since this new function can return zero (error case), it seems like 
the callers should explicitly test for that return value. 
But this patch is no worse than the code it replaces in that respect.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 223645 [details] [diff] [review]
Proposed patch for NSS_3_11_BRANCH

One concern I have with this patch is for function VFY_EndWithSignature
Here, we effectively change the variable upon which our choice of signature
size is based from the context type to the key type.  Can we get into a 
case where the key type suggests one signature size and the context type
suggests another?  

The "more conservative" patch has this problem SLIGHTLY less than this 
patch.  I think I value keeping the branch and trunk in sync more 
highly than this conservatism though.  And in any case, my biggest 
concern is error handling (or lack thereof).

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 223646 [details] [diff] [review]
More conservative patch for NSS_3_11_BRANCH

On, one other thought I had.  Seems the term "PublicKey" in the name of the new function is redundant.  But I can live with it.

Comment 9

[Wan-Teh Chang]

Attachment: Proposed patch v2

This new patch addresses Nelson's concerns.

1. Rename the new function SECKEY_SignatureLen.
Originally I named it SECKEY_PublicKeySignatureLen
to hint that the argument is a SECKEYPublicKey *
as opposed to a SECKEYPrivateKey *.  (PK11_SignatureLen
takes a SECKEYPrivateKey * argument.)

2. Make sure the new function and related existing
functions set an error code on failure (when they
return 0).

3. In secvfy.c, the context type (cx->type) is the
same as the key type (cx->key->keyType). To eliminate
this concern in the future, I removed the 'type'
member from the VFYContextStr structure.  So the
patch clearly shows that the old code initializes
cx->type to key->keyType and never changes it.
(Note that 'cx->key' is a copy of 'key'.)

4. I handle the failure of SECKEY_SignatureLen
(when it returns 0).

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

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

Lots of good improvements in this patch!
Thanks, Wan-Teh!

Comment 11

[Wan-Teh Chang]

Attachment: More conservative patch for NSS_3_11_BRANCH, v2

The only change from the v1 patch is that the new function
has been renamed SECKEY_SignatureLen.

I deliberately do not add code to handle SECKEY_SignatureLen
failure (0 return value) and do not use the SECKEY_SignatureLen
function for the DSA cases, because I want to make the diffs
small.  (As Nelson noted, the lack of error handling is no
worse than the original code.) Smaller diffs make it easier
to see that this patch won't affect the non-ECDSA cases.

Comment 12

[Wan-Teh Chang]

Attachment: More conservative patch for NSS_3_11_BRANCH, v3

OK, to save Nelson some anxiety, I added code to
set error codes and handle failures.  This patch
is still conservative because it doesn't use the
new SECKEY_SignatureLen function for the DSA cases.
Unfortunately I can't remove the 'type' member of
the VFYContextStr structure easily on the NSS_3_11_BRANCH
because it has a different type from key->keyType.

Comment 13

[Wan-Teh Chang]

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

I checked in the proposed patch v2 on the NSS trunk
(3.12).

Checking in cryptohi/keyhi.h;
/cvsroot/mozilla/security/nss/lib/cryptohi/keyhi.h,v  <--  keyhi.h
new revision: 1.16; previous revision: 1.15
done
Checking in cryptohi/seckey.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/seckey.c,v  <--  seckey.c
new revision: 1.42; previous revision: 1.41
done
Checking in cryptohi/secvfy.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/secvfy.c,v  <--  secvfy.c
new revision: 1.18; previous revision: 1.17
done
Checking in nss/nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.165; previous revision: 1.164
done
Checking in ssl/ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.91; previous revision: 1.90
done

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 223998 [details] [diff] [review]
More conservative patch for NSS_3_11_BRANCH, v3

sr=nelson.  Good patch.

Comment 15

[Wan-Teh Chang]

I checked in the "more conservative patch for NSS_3_11_BRANCH, v3"
on the NSS_3_11_BRANCH.

Checking in cryptohi/keyhi.h;
/cvsroot/mozilla/security/nss/lib/cryptohi/keyhi.h,v  <--  keyhi.h
new revision: 1.13.2.2; previous revision: 1.13.2.1
done
Checking in cryptohi/seckey.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/seckey.c,v  <--  seckey.c
new revision: 1.36.2.7; previous revision: 1.36.2.6
done
Checking in cryptohi/secvfy.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/secvfy.c,v  <--  secvfy.c
new revision: 1.16.2.2; previous revision: 1.16.2.1
done
Checking in nss/nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.158.2.5; previous revision: 1.158.2.4
done
Checking in ssl/ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.76.2.13; previous revision: 1.76.2.12
done

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Wan-Teh fixed this bug.