302286 - NSS misinterprets the CKA_PRIME_BITS attribute for DSA's p parameter

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Wan-Teh Chang]

PKCS #11 v2.20 Sec. 12.2.4 "DSA domain parameter objects"
and Sec. 12.2.6 "DSA domain parameter generation" define
the CKA_PRIME_BITS to mean the length in bits of the prime
p.

But NSS's PK11_PQG_ParamGen sets the value of CKA_PRIME_BITS
to be the "index" j of prime p's length L:
    j = (L-512)/64

Fortunately, this CKA_PRIME_BITS value can only be consumed
by PQG_ParamGenSeedLen (via NSC_GenerateKey => nsc_parameter_gen
=> PQG_ParamGen), which interprets it the same way.

So this bug only affects those who invoke the C_GenerateKey
method of our softoken with the CKM_DSA_PARAMETER_GEN mechanism
directly.

Comment 1

[Wan-Teh Chang]

Attachment: Proposed patch

Change both the producer and the consumer of CKA_PRIME_BITS in NSS.

I am using the PQG_INDEX_TO_PBITS and PQG_PBITS_TO_INDEX macros
defined in "blapit.h".	Not sure if these macros are really public.
If not, I can easily write 512+64*j and (primeBits-512)/64.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 190655 [details] [diff] [review]
Proposed patch

Note that the macro-functions PQG_INDEX_TO_PBITS(j) and 
PQG_PBITS_TO_INDEX(p) return -1 if the argument value is not one 
of those defined as valid by FIPS 186.	I think this code should
test the values returned by those macros, and not proceed if 
either returned value is -1.

Comment 3

[Wan-Teh Chang]

Attachment: Proposed patch v2

I added the invalid value checking that Nelson suggested.

I didn't use the PQG macros for two reasons.

1. The PQG macros return -1 on invalid input, but the j
and primeBits variables are both unsigned, so I'd need to
compare them with (unsigned int)-1 or (CK_ULONG)-1.

2. The invalid value checking done in PQG_PBITS_TO_INDEX
is insufficient.  Perhaps I should fix that, too.

Comment 4

[Robert Relyea]

Comment on attachment 190881 [details] [diff] [review]
Proposed patch v2

Looks OK, though I have one question, why is your if check more efficient than
the ? check in the macro. It seems we are only replacing a subraction with 2
compares.

I don't have a problem with your code, and I would be a little.

blapit.h is already included in pk11pqg.h (probably for the structures) which
is public, so the cat is already out of the bag for using the macros.

If you don't use the macros, a comment just before a mysterious equations would
be nice;).

Comment 5

[Wan-Teh Chang]

Attachment: Proposed patch v3

OK, OK, I'll use the macros. :-)

Comment 6

[Wan-Teh Chang]

Comment on attachment 191012 [details] [diff] [review]
Proposed patch v3

>-#define PQG_PBITS_TO_INDEX(bits) ((((bits)-512) % 64) ? -1 : (int)((bits)-512)/64)
>+#define PQG_PBITS_TO_INDEX(bits) \
>+    (((bits) < 512 || (bits) > 1024 || (bits) % 64) ? \
>+    -1 : (int)((bits)-512)/64)

This change is necessary because the original test is equivalent
to "(bits) % 64" and doesn't check that "bits" is >= 512 and
<= 1024.

Comment 7

[Robert Relyea]

Comment on attachment 191012 [details] [diff] [review]
Proposed patch v3

r=relyea looks good.

Comment 8

[Wan-Teh Chang]

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

Checking in freebl/blapit.h;
/cvsroot/mozilla/security/nss/lib/freebl/blapit.h,v  <--  blapit.h
new revision: 1.17; previous revision: 1.16
done
Checking in pk11wrap/pk11pqg.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pqg.c,v  <--  pk11pqg.c
new revision: 1.11; previous revision: 1.10
done
Checking in softoken/pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.67; previous revision: 1.66
done