NSS ECDSA signature length incompatible with other implementations for some curves

RESOLVED FIXED in 3.11.2

Status

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

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Wan-Teh Chang)

Tracking

3.11.1
3.11.2

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

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.
(Reporter)

Comment 1

12 years ago
Created attachment 222694 [details] [diff] [review]
Wan-Teh's patch for NSS_3_11_BRANCH
(Reporter)

Updated

12 years ago
Assignee: nobody → wtchang
Priority: -- → P1
Target Milestone: --- → 3.11.2
Version: 3.11 → 3.11.1
(Assignee)

Comment 2

12 years ago
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.
Assignee: wtchang → libraries
(Assignee)

Comment 3

12 years ago
Created attachment 223642 [details] [diff] [review]
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.
Attachment #222694 - Attachment is obsolete: true
Attachment #223642 - Flags: review?(nelson)
(Assignee)

Comment 4

12 years ago
Created attachment 223645 [details] [diff] [review]
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.
Attachment #223645 - Flags: review?(nelson)
(Assignee)

Comment 5

12 years ago
Created attachment 223646 [details] [diff] [review]
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.
Attachment #223646 - Flags: review?(nelson)
(Reporter)

Comment 6

12 years ago
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.
Attachment #223642 - Flags: review?(nelson) → review+
(Reporter)

Comment 7

12 years ago
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).
Attachment #223645 - Flags: review?(nelson) → review+
(Reporter)

Comment 8

12 years ago
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.
Attachment #223646 - Flags: review?(nelson) → review+
(Assignee)

Comment 9

12 years ago
Created attachment 223987 [details] [diff] [review]
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).
Attachment #223642 - Attachment is obsolete: true
Attachment #223987 - Flags: review?(nelson)
(Reporter)

Comment 10

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

Lots of good improvements in this patch!
Thanks, Wan-Teh!
Attachment #223987 - Flags: review?(nelson) → review+
(Assignee)

Comment 11

12 years ago
Created attachment 223995 [details] [diff] [review]
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.
Attachment #223646 - Attachment is obsolete: true
Attachment #223995 - Flags: review?(julien.pierre.bugs)
(Assignee)

Comment 12

12 years ago
Created attachment 223998 [details] [diff] [review]
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.
Attachment #223995 - Attachment is obsolete: true
Attachment #223998 - Flags: superreview?(nelson)
Attachment #223998 - Flags: review?(julien.pierre.bugs)
Attachment #223995 - Flags: review?(julien.pierre.bugs)
(Assignee)

Comment 13

12 years ago
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

Updated

12 years ago
Attachment #223998 - Flags: review?(julien.pierre.bugs) → review+
(Reporter)

Comment 14

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

sr=nelson.  Good patch.
Attachment #223998 - Flags: superreview?(nelson) → superreview+
(Assignee)

Comment 15

12 years ago
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
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Reporter)

Comment 16

12 years ago
Wan-Teh fixed this bug.
Assignee: libraries → wtchang
You need to log in before you can comment on or make changes to this bug.