66558 - assertion violation when generating DSA keypair

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Jamie Nicolson]

In prng_fips1861.c, in the dsa_reduce_mod_q() macro, we assert that the first
byte is >= 0x80. But if our Q was passed in as a PKCS #11 attribute, it might
have had a zero byte prepended to it by pk11_Attribute2SSecItem.

One solution is to check Q when it is passed into the DSA code with code like
this:

if( q && (*q == 0) ) {
        ++q;
}

I'll attach a patch that does this.

Comment 1

[Jamie Nicolson]

Attachment: proposed patch

Comment 2

[Jamie Nicolson]

Target milestone 3.2. This break DSA key generation in JSS. I talked to Bob
about this before but I guess I never made it into a bug.

Comment 3

[Wan-Teh Chang]

Ian: please review this patch.

Bob: does this need to be fixed in NSS 3.2?

Comment 4

[Ian McGreer]

I had to take similar measures in the RSA code, when dealing with the RSA 
modulus.  When encountering code like this:
RSA_XXXX(SECItem *modulus, ...)
{
   modlen = modulus->len - !modulus->data[0];
   ...
}

The question I have about the patch is, what about the size arguments?  For 
example,
DSA_GenerateGlobalRandomBytes(void *dest, size_t len, unsigned char *q)
{
    /* if q is incremented to the next byte here because of a leading 0,
       what happens to len?  should it be decremented? Or is it already
       the correct size? */
}
}

Comment 5

[Jamie Nicolson]

I think the "bytes" and "len" parameters refer to the size of the "data" and
"dest" parameters, respectively, not to the length of the "q" parameter.

Comment 6

[Ian McGreer]

Good point (I love it when I don't remember how my own code works).  q is always 
DSA_SUBPRIME_LEN.  I think that your patch is okay, dunno if wtc or relyea needs 
to approve it before you check in.

Comment 7

[Wan-Teh Chang]

Ian, you can check this in.  Thanks.

Comment 8

[Ian McGreer]

done. marking fixed.