296410 - NSS 3.9.3 not support SHA-512

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Thomas Kwan]

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

Comment 1

[Thomas Kwan]

Updated version number to 3.9.3

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

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; 

Comment 3

[Wan-Teh Chang]

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

Comment 4

[Wan-Teh Chang]

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

Comment 5

[Robert Relyea]

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.

Comment 6

[Wan-Teh Chang]

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.

Comment 7

[Wan-Teh Chang]

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

I checked in the minimal patch on the NSS trunk (NSS 3.11).

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 9

[Robert Relyea]

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

bob

Comment 10

[Wan-Teh Chang]

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.

Comment 11

[Wan-Teh Chang]

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

Comment 12

[Wan-Teh Chang]

All patches have been checked in on the NSS trunk for NSS 3.11.

Comment 13

[Wan-Teh Chang]

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

Comment 14

[Wan-Teh Chang]

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.