Closed Bug 850905 Opened 8 years ago Closed 8 years ago

shlibsign fails digest/signature verification unless default keysize is used

Categories

(NSS :: Tools, defect, P2)

3.14
x86
Linux
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: elio.maldonado.batiz, Assigned: elio.maldonado.batiz)

Details

Attachments

(1 file, 1 obsolete file)

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 ...
Assignee: nobody → emaldona
Attachment #724686 - Flags: review?(rrelyea)
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
(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.
Priority: -- → P2
(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
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
BTW the rest of your patch is fine.
oops, this is the upstream bug, ignore my upstream comments....
Attachment #724686 - Attachment is obsolete: true
Attachment #724686 - Flags: review?(rrelyea)
Attachment #727753 - Flags: review?(rrelyea)
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
Attachment #727753 - Flags: review?(rrelyea) → review+
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.15
You need to log in before you can comment on or make changes to this bug.