850905 - shlibsign fails digest/signature verification unless default keysize is used

Status: RESOLVED FIXED

(NSS :: Tools, defect, P2)

Description

[Elio Maldonado]

shlibsign now has the -k keySize option but it fails as digest and signature verification unless default is used. The length of not what is exepected

Try 1024:
$ ./shlibsign -i ./libsoftokn3.so -o ./libsoftokn3.chk -d sql:/etc/pki/nssdb -V -k 1024
digestLen has incorrect length 20 it should be 32 

Try 2048 explicetly:
$./shlibsign -i ./libsoftokn3.so -o ./libsoftokn3.chk -d sql:/etc/pki/nssdb -V -k 2048
digestLen has incorrect length 20 it should be 32 

Using default or spcifying -k 0 it works:
$ ./shlibsign -i ./libsoftokn3.so -o ./libsoftokn3.chk -d sql:/etc/pki/nssdb -V -k 0
$ ./shlibsign -i ./libsoftokn3.so -o ./libsoftokn3.chk -d sql:/etc/pki/nssdb -V -k

The problem is that that the digst and signature buffers are for DSA with SHA256
.... near the top in main
    CK_BYTE digest[32]; /* SHA256_LENGTH */
    CK_BYTE sign[64];   /* DSA SIGNATURE LENGTH */
...
and the digest lenght verification is against the size of the buffer
    if (digestLen != sizeof(digest)) {
        PR_fprintf(PR_STDERR, "digestLen has incorrect length %lu "
     ...
similarly on signature verification
    if (signLen != sizeof(sign)) {
        PR_fprintf(PR_STDERR, "signLen has ...

Comment 1

[Elio Maldonado]

Attachment: set expected digest and signature lenght according to the key size

Comment 2

[Robert Relyea]

I'm wondering if we shouldn't just have sanity checks on the length (did we blow away our buffer size). We shouldn't blow up if we suddenly changes to SHA512_LENGTH or something. The actual arrays should be something like MAX_HASH_LENGTH.

Also, I don't understand why explicitly setting 2048 didn't work, since those were the lengths we ended up defaulting...

bob

Comment 3

[Elio Maldonado]

(In reply to Robert Relyea from comment #2)
> 
> Also, I don't understand why explicitly setting 2048 didn't work, since
> those were the lengths we ended up defaulting...

I think I know why: When I used -k 2048  -- unpatched code

916	if ((keySize == 0) && mechInfo.ulMaxKeySize >=2048 ) {
917	    keySize = 2048;
918	} else {
919	    keySize = 1024;
9120	}
 the else branch is taken.

Comment 4

[Elio Maldonado]

(In reply to Robert Relyea from comment #2)
> SHA512_LENGTH or something. The actual arrays should be something like
> MAX_HASH_LENGTH.

We do have these two constants
lib/freebl/blapit.h:#define HASH_LENGTH_MAX         SHA512_LENGTH
lib/freebl/blapit.h:#define DSA_MAX_SIGNATURE_LEN 	(DSA_MAX_SUBPRIME_LEN*2)/* Bytes */
that we could use fot the arrays

Comment 5

[Robert Relyea]

Ah... the if statement should be:

 if (keysize == 0) {
     if (mechInfo.ulMaxKeySize >= 2048) {
           keySize = 2048;
     } else {
           keySize = 1024;
     }
  }
  if (keySize > mechInfo.ulMaxKeySize) {
     /* print an error and fail */
  }


> We do have these two constants

yes, use those constants.

Neither of these problems are of your patches making, the are pre-existing (read introduced by me last time I touched this file upstream), but we should get this fixed (-k 2048 should generate a 2048 bit key or fail, not silently build 1024 bit key).

Please file a bug upstream about this as well.

bob

Comment 6

[Robert Relyea]

BTW the rest of your patch is fine.

Comment 7

[Robert Relyea]

oops, this is the upstream bug, ignore my upstream comments....

Comment 8

[Elio Maldonado]

Attachment: V2 - address review requests from Comment 5

Comment 9

[Robert Relyea]

Comment on attachment 727753 [details] [diff] [review]
V2 - address review requests from Comment 5

r+ but change the following magic numbers:

expectedDigestLen = 20;
expectedSignatureLen = 40;

expectedDigestLen = 32;
expectedSignatureLen = 64;

to

expectedDigestLen = SHA1_LENGTH;
expectedSignatureLen = sizeof(subprime)*2;   /* length of q*2 */

expectedDigestLen = SHA256_LENGTH;
expectedSignatureLen = sizeof(subprime2)*2; /* length of q*2 */


respectively

bob

Comment 10

[Elio Maldonado]

Checked in
https://hg.mozilla.org/projects/nss/rev/b786ac75c1c7