Closed Bug 723740 Opened 9 years ago Closed 9 years ago

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

Categories

(NSS :: Libraries, defect, P2)

3.13
defect

Tracking

(Not tracked)

RESOLVED FIXED
3.13.4

People

(Reporter: rrelyea, Assigned: rrelyea)

References

Details

Attachments

(2 files)

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.
Assignee: nobody → rrelyea
Status: NEW → ASSIGNED
Attachment #593988 - Flags: review?(wtc)
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 '+'.
Attachment #593988 - Flags: review?(wtc) → review+
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);
> 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
The biggest change is to grab the bits from the right rather than from the left when grabbing a partial key.
Attachment #595564 - Flags: review?(wtc)
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
Attachment #595564 - Flags: review?(wtc) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Duplicate of this bug: 734441
Priority: -- → P2
Target Milestone: --- → 3.13.4
You need to log in before you can comment on or make changes to this bug.