Bits vs bytes mixup in DSA2 keysize calculation

RESOLVED FIXED

Status

RESOLVED FIXED
6 years ago
5 years ago

People

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

Tracking

3.13.4
x86_64
Linux

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
Created attachment 719984 [details]
Fix DSA2 keysize calculation on verify - by Panu Matilainen

As originally reported by  Panu Matilainen in Fedora:

Fix DSA2 keysize calculation on verify

Description of problem:

There seems to be a bits vs bytes mixup in the way DSA2 keysize is calculated in PK11_Verify(), causing it to fail due to not finding a slot that can handle it:

        unsigned int length =  0;
        if ((mech.mechanism == CKM_DSA) &&
                                /* 129 is 1024 bits translated to bytes and
                                 * padded with an optional '0' to maintain a
                                 * positive sign */
                                (key->u.dsa.params.prime.len > 129)) {
            /* we need to get a slot that not only can do DSA, but can do DSA2
             * key lengths */
            length = key->u.dsa.params.prime.len;
            if (key->u.dsa.params.prime.data[0] == 0) {
                length --;
            }
         }

        slot = PK11_GetBestSlotWithAttributes(mech.mechanism,
                                                CKF_VERIFY,length,wincx);


Note the comment about 129 bytes vs 1024 bits translation. However the prime length in bytes is passed as-is to PK11_GetBestSlotWithAttributes() which expects the key size in bits (if specified). 

With a bit of debugging output added, an attempt to verify a signature created with 2048bit DSA key:

vfy_VerifyDigest: dsa key (2), len 64
PK11_Verify: slot (nil)
PK11_Verify: DSA length 256
PK11_GetBestSlotMultipleWithAttributes: mech_count 1
PK11_GetBestSlotMultipleWithAttributes: looking at le 0x1d8b2a0
pk11_filterSlot: need ks 256, min 512 max 3072
  -> keysize bad

256 bit keysize is of course invalid for DSA so it goes downhill from there. With the attached patch to convert the keysize to bits before the slot lookup the above debug foobar looks like:

vfy_VerifyDigest: dsa key (2), len 64
PK11_Verify: slot (nil)
PK11_Verify: DSA length 2048
PK11_GetBestSlotMultipleWithAttributes: mech_count 1
PK11_GetBestSlotMultipleWithAttributes: looking at le 0x1d500c0
pk11_filterSlot: need ks 2048, min 512 max 3072
PK11_GetBestSlotMultipleWithAttributes: returning slot 0x1cdcbf0
PK11_Verify: best slot 0x1cdcbf0, length 2048
[...]

...and the verification actually succeeds.

Version-Release number of selected component (if applicable):

nss-3.14-7.fc18.x86_64, versions prior to 3.14 didn't support DSA2 at all AFAIK.

How reproducible:

Fully reproducable. Unfortunately I dont have a nice isolated test-case I could represent here, I stumbled on this while adding DSA2 support to upstream RPM.

Without the DSA2 keysize patch:
[pmatilai@localhost rpm]$ ./rpmkeys -Kv /tmp/telnet-0.17-51.fc16.x86_64.rpm 
/tmp/telnet-0.17-51.fc16.x86_64.rpm:
    Header V4 DSA/SHA256 Signature, key ID 0e5b82e0: BAD
    Header SHA1 digest: OK (2a2c6961886dc49a1d71c4613c9e7b79ceea3c49)
    MD5 digest: OK (bc60b6d23e72d68765be41012f4f5758)
    V4 DSA/SHA256 Signature, key ID 0e5b82e0: BAD
[pmatilai@localhost rpm]$

...and with it:
[pmatilai@localhost rpm]$ ./rpmkeys -Kv /tmp/telnet-0.17-51.fc16.x86_64.rpm 
/tmp/telnet-0.17-51.fc16.x86_64.rpm:
    Header V4 DSA/SHA256 Signature, key ID 0e5b82e0: OK
    Header SHA1 digest: OK (2a2c6961886dc49a1d71c4613c9e7b79ceea3c49)
    MD5 digest: OK (bc60b6d23e72d68765be41012f4f5758)
    V4 DSA/SHA256 Signature, key ID 0e5b82e0: OK
[pmatilai@localhost rpm]$

[reply] [-]
Private
Comment 1 Panu Matilainen 2012-12-11 05:56:31 EST

BTW the bytes -> bits conversion is present in the patch at https://bugzilla.mozilla.org/show_bug.cgi?id=475578#c34: 

Note the "length*BITS_PER_BYTE" calculation in https://bugzilla.mozilla.org/attachment.cgi?id=636377&action=diff#mozilla/security/nss/lib/pk11wrap/pk11obj.c_sec7, that's exactly the same thing I'm talking about here.

However that calculation is not present in what eventually got committed to nss cvs and so its not in 3.14 either.
(Reporter)

Updated

6 years ago
Attachment #719984 - Flags: review?(rrelyea)
(Reporter)

Updated

6 years ago
Assignee: nobody → rrelyea
(Assignee)

Comment 1

6 years ago
Comment on attachment 719984 [details]
Fix DSA2 keysize calculation on verify - by Panu Matilainen

r+ rrelyea.

Part of the confusion is PKCS #11 has different standards depending on the mechanism. Asymmetric mechanisms like DSA and RSA are in bits, while symetric mechanisms are in bytes. That means the length means something different depending on the mechanism. In this case the patch is correct as DSA is in bits.

bob
Attachment #719984 - Flags: review?(rrelyea) → review+
(Reporter)

Comment 2

5 years ago
This has been fixed for quite some time: https://hg.mozilla.org/projects/nss/rev/bb3b86c8570c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.