723740 - CKM_DH_DERIVE does not respect VALUE_LEN if VALUE_LEN is greater than the unpadded generated key.

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Robert Relyea]

CKM_DH_PKCS_DERIVE is supposed to choose the key length as follows:

    CKA_VALUE_LEN is set, the key is set to the value given.
    If it id not set, it is set to the PKCS #3 definition.

Unfortunately, because of both openSSL and NSS incorrectly implemented the PKCS #3 definition, which says the resulting DH key is always the length of Prime p, CKM_DH_PCKS_DERIVE has to return the incorrect NSS/openSSL definition. In several cases, however, we need to return the true PKCS #3 key. This is supposed to be supported by setting CKA_VALUE_LEN to the length of p, but freebl does not return the properly padded key in this case.

Comment 1

[Robert Relyea]

Attachment: Properly pad the key the key outlen is given and the key is smaller than outlen

Comment 2

[Wan-Teh Chang]

Comment on attachment 593988 [details] [diff] [review]
Properly pad the key the key outlen is given and the key is smaller than outlen

Review of attachment 593988 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc.  Please update the comments and parameter name for DH_Derive
in blapi.h and attach a new patch.

::: security/nss/lib/freebl/dh.c
@@ -262,1 +264,4 @@
> > -    memcpy(derivedSecret->data, secret, nb);
> > +    if (len < nb) {
> > +	unsigned int offset = nb - len;
> > +	memset(derivedSecret->data, 0, offset);
> > +	memcpy(derivedSecret->data+offset, secret, len);

Nit: add spaces around '+'.

Comment 3

[Wan-Teh Chang]

Bob: I have two questions.

1. If outBytes is 0. and the derived secret happens to be a
small integer, should we also pad it to the length of prime p?

I guess we intentionally do not pad it when outBytes is 0 to
preserve backward compatibility?

2. PKCS #11 v2.20 Section 12.4.10 says:

    (The truncation removes bytes
    from the leading end of the secret value.)

If this means we throw away bytes from the leading end of the
secret value, then our memcpy call

    memcpy(derivedSecret->data, secret, nb);

is wrong.  We should do

    memcpy(derivedSecret->data, secret + len - nb, nb);

Comment 4

[Robert Relyea]

> I guess we intentionally do not pad it when outBytes is 0 to preserve backward compatibility?

Yes.

> 2. PKCS #11 v2.20 Section 12.4.10 says:

Good catch. I'll supply an update patch for this issue as well and get a second review.

Thanks, 

bob

Comment 5

[Robert Relyea]

Attachment: Incorporate wtc's review comments.

The biggest change is to grab the bits from the right rather than from the left when grabbing a partial key.

Comment 6

[Wan-Teh Chang]

Comment on attachment 595564 [details] [diff] [review]
Incorporate wtc's review comments.

Review of attachment 595564 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc.  Just some typos in the new comments in blapi.h.

::: security/nss/lib/freebl/blapi.h
@@ -214,3 +214,3 @@
> >  ** secret, and derivedSecret->len is the size of the secret produced.
> > -** The size of the secret produced will never be larger than the length
> > +** The size of the secret produced will depend on the value of outBytes.
> > -** of the prime, and it may be smaller than maxOutBytes.
> > +** If out bytes is 0, the key length will be all the significant bytes of

out bytes => outBytes

Nit: The use of "significant bytes" is easily confused with
"most significant bytes").  Here it essentially means nonzero bytes
(unless the derived secret is 0).  I don't have a better suggestion.

@@ -215,2 +215,6 @@
> > -** The size of the secret produced will never be larger than the length
> > +** The size of the secret produced will depend on the value of outBytes.
> > -** of the prime, and it may be smaller than maxOutBytes.
> > +** If out bytes is 0, the key length will be all the significant bytes of
> > +** the derived secret (leading zeros are dropped). This length could be less
> > +** than the length of the prime. If outBytes is nonzero, the length of the
NaN more ...

It it => If it

Comment 7

[Robert Relyea]

hcp-227.sjc.redhat.com(434) cvs commit blapi.h blapit.h dh.c
Checking in blapi.h;
/cvsroot/mozilla/security/nss/lib/freebl/blapi.h,v  <--  blapi.h
new revision: 1.44; previous revision: 1.43
done
Checking in blapit.h;
/cvsroot/mozilla/security/nss/lib/freebl/blapit.h,v  <--  blapit.h
new revision: 1.26; previous revision: 1.25
done
Checking in dh.c;
/cvsroot/mozilla/security/nss/lib/freebl/dh.c,v  <--  dh.c
new revision: 1.10; previous revision: 1.9
done
/cvsroot/mozilla/security/nss/lib/freebl/blapi.h,v  <--  blapi.h
new revision: 1.45; previous revision: 1.44
done