NSS 3.9.3 not support SHA-512

RESOLVED FIXED in 3.10.2

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
13 years ago
12 years ago

People

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

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

9.77 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Details | Diff | Splinter Review
1.81 KB, patch
Robert Relyea
: review+
Julien Pierre
: review+
Details | Diff | Splinter Review
5.08 KB, patch
Robert Relyea
: review+
Nelson Bolyard (seldom reads bugmail)
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

13 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

This could be a NSS bug. We have the following:

a) a self-signed CA certificate that is signed with SHA512withRSA
   signing alogrithm.
b) a server certificate that is signed by a) with SHA512withRSA

After some debugging, I think I have located the following error
in cryptohi/secvfy.c

DecryptSigBlock(....)
{
  ...
    if (di->digest.len > 32) {
    PORT_SetError(SEC_ERROR_OUTPUT_LEN);
    goto loser;
    }
  ...

In our case, di->digest.len is 64. I tried commenting out
the check. Then, the server crashed. I then saw the following
in the same file:

struct VFYContextStr {
 ...
    /* digest holds the full dsa signature... 40 bytes */
    unsigned char digest[DSA_SIGNATURE_LEN];
 ...
};

I think DSA_SIGNATURE_LEN is less than 64. So
the digest buffer is not big enough for 
SHA-512 digest.

There could be other code that we need to adjust
to fully support SHA-512.

Reproducible: Always

Steps to Reproduce:
1. Create a self-signed SHA512 CA with CS7.1
2.
3.

Actual Results:  
CS7.1 will not start, and the error is

[05/Apr/2005:15:13:16] failure (26920): Invalid configuration: File
/export/ckannan/cs71-root/cert-zion-ca-0405-lunasa-1/config/server.xml, line 22,
column 428: SEC_ERROR_BAD_SIGNATURE - Certificate has invalid signature : Unable
to verifycertificate lunasa1-drm:Server-Cert cert-zion-ca-0405-lunasa-1. Add
"EnforceValidCerts off" to magnus.conf to start the server until the problem can
be resolved.


Re: raidzilla #57434
(Reporter)

Comment 1

13 years ago
Updated version number to 3.9.3
Version: unspecified → 3.9.3
Two good catches, Tom!    
In both cases the code should be using HASH_LENGTH_MAX 
instead of 32 or DSA_SIGNATURE_LEN.  
(that value is never a signature, always a hash)

Here's another place that potentially makes the same mistake.
/security/manager/ssl/public/nsIHash.idl, line 60 -- 
     const unsigned long MAX_HASH_LEN = SHA1_LEN; 
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

13 years ago
Status: NEW → ASSIGNED
OS: Linux → All
Priority: -- → P2
Target Milestone: --- → 3.11
(Assignee)

Comment 3

13 years ago
Created attachment 191630 [details] [diff] [review]
Proposed patch (checked in)

Thomas, could you test this patch with a cert signed with
RSA SHA-512?

Here are the changes in this patch.  Only change 1 and
change 4 are needed to fix this bug, but I want to make
the other changes to improve code maintainability.

1. Use HASH_LENGTH_MAX instead of the magic number 32 as
   the size of buffers that hold digests.
2. Removed dead code SEC_SignFile and SEC_VerifyFile.
3. Correct and improve comments.
4. Add a new parameter 'len' to the DecryptSigBlock function.
   'len' is the length of the 'digest' buffer.
5. In the VFYContextStr structure, combine the 'digest' and
   'ecdsadigest' buffers into a union.	The reason is not to
   save space, but rather to improve code maintainability.

   The old 'digest' buffer is used to store the digest in a
   decrypted RSA signature or a decoded DSA signature.	This
   dual purpose is very confusing and the variable name 'digest'
   is wrong for the second use (decoded DSA signature).
6. Rename the 'digest' parameter of the decodeECorDSASignature
   function as 'dsig' because that parameter is the decoded
   signature, not a digest.
(Assignee)

Updated

13 years ago
Attachment #191630 - Flags: review?(nelson)
(Assignee)

Comment 4

13 years ago
Created attachment 191634 [details] [diff] [review]
Minimal patch (checked in)

This patch is the absolutely minimum changes needed to
fix this bug.  It may be more appropriate for maintenance
branches or easier to get a code review.
Attachment #191634 - Flags: review?(rrelyea)

Updated

12 years ago
Attachment #191634 - Flags: review?(rrelyea) → review+

Comment 5

12 years ago
minimal patch OK for 3.10.2.

I like some of the concepts of the longer patch, particularly the use of the
union to make sure we 'always have enough space', but I don't think we need to
reference each of the digests separately depending on usage. In particular, we
don't need to select u.dsadigest versus u.ecdigest in the if statement. We
should be able to remove the temp variable and just pass the 'digest space' into
the decode function.

I presume SEC_SignFile and SEC_VerifyFile are dead code that aren't exported.
(Assignee)

Comment 6

12 years ago
Bob, you mean you want me to just say:

  rv = decodeECorDSASignature(algid,sig,(unsigned char*)&cx->u,sigLen);

The reason I (continue to) use the tmp variable is to avoid that
(unsigned char*) cast.  (The tmp variable is from the original code,
which must use tmp because the original code has different buffers
for DSA and ECDSA.)

Alternatively, I can do this to avoid the cast:

struct VFYContextStr {
...
    union {
        unsigned char rsadigest[HASH_LENGTH_MAX];
        unsigned char dsasig[DSA_SIGNATURE_LEN];
        unsigned char ecdsasig[2 * MAX_ECKEY_LEN];
        unsigned char buffer[1];
    } u;
...
};
...
  rv = decodeECorDSASignature(algid,sig,cx->u.buffer,sigLen);

Yes, SEC_SignFile and SEC_VerifyFile aren't used internally and aren't
exported.
(Assignee)

Comment 7

12 years ago
Comment on attachment 191634 [details] [diff] [review]
Minimal patch (checked in)

I checked in the minimal patch on the NSS trunk (NSS 3.11).
Attachment #191634 - Attachment description: Minimal patch → Minimal patch (checked in)
Comment on attachment 191630 [details] [diff] [review]
Proposed patch (checked in)

r=nelson@bolyard.
The additional refinement proposed in comment 6 is more elegant, in that it
eliminates an unnecessary pointer "tmp".  But I think this patch is clearer to
a reader of the code.  Either way is fine with me.
Attachment #191630 - Flags: review?(nelson) → review+

Comment 9

12 years ago
re comment 6. Something like the latter. just plain digest as a name is fine.

bob
(Assignee)

Comment 10

12 years ago
Comment on attachment 191630 [details] [diff] [review]
Proposed patch (checked in)

I've checked in this patch on the NSS trunk for
NSS 3.11.  This bug is fixed.  I will submit a
patch that implements Bob's suggestion next.
Attachment #191630 - Attachment description: Proposed patch → Proposed patch (checked in)
(Assignee)

Comment 11

12 years ago
Created attachment 193086 [details] [diff] [review]
Code simplification

This patch implements what Bob suggested.  Please let me
know if this is what you have in mind.

I don't want to name the union member "digest" because for
DSA and ECDSA the buffer doesn't hold the digest but rather
the full signature.

Note that I use the name of the array (cx->u.buffer) as a
pointer to the buffer, instead of the &cx->u.buffer[0]
notation that the original code uses.  I hope this is
allowed by the NSS coding style.
Attachment #193086 - Flags: superreview?(nelson)
Attachment #193086 - Flags: review?(rrelyea)

Updated

12 years ago
Attachment #193086 - Flags: review?(rrelyea) → review+
(Assignee)

Comment 12

12 years ago
All patches have been checked in on the NSS trunk for NSS 3.11.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Attachment #193086 - Flags: superreview?(nelson) → superreview+
(Assignee)

Comment 13

12 years ago
Comment on attachment 191634 [details] [diff] [review]
Minimal patch (checked in)

Requesting Sun's approval to check this simpler fix in
on the NSS_3_10_BRANCH for NSS 3.10.2.

This patch does just enough work to fix the bug.  It
changes the magic constant 32 (= 256 bits) and
DSA_SIGNATURE_LEN (=40 bytes or 320 bits) to
HASH_LENGTH_MAX (= 64 bytes or 512 bits).
Attachment #191634 - Flags: superreview?(nelson)
Attachment #191634 - Flags: review?(julien.pierre.bugs)

Updated

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

Comment 14

12 years ago
Comment on attachment 191634 [details] [diff] [review]
Minimal patch (checked in)

I checked in the minimal patch on the NSS_3_10_BRANCH for
NSS 3.10.2.
Attachment #191634 - Flags: superreview?(nelson)
(Assignee)

Updated

12 years ago
Target Milestone: 3.11 → 3.10.2
You need to log in before you can comment on or make changes to this bug.