291498 - NSS CKM_DH_PKCS_DERIVE not PKCS3 compliant

Status: NEW

(NSS :: Libraries, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

It seems that many developers of SSL3/TLS have independently read PKCS3 
ftp://ftp.rsasecurity.com/pub/pkcs/ascii/pkcs-3.asc
and all missed the fact that it defines the derived output of "Phase II"
to have exactly the same number of octets and the number of significant
octets of prime P.  Consequently, those developers independently all
implemented DH to strip leading zero octets from the result, and their
SSL3/TLS implementations of DH and DHE ciphersuites all interoperated 
due to their common (arguably mistaken) interpretations of PKCS3.  
This is observed in SSLeay (forerunner of OpenSSL), OpenSSL, in NSS,
and in all presently interoperable implementations of SSL3/TLS DHE 
ciphersuites.  The current draft of the next TLS RFC explicitly 
requires leading zeros to be removed from the DH output before being
treated as the pre-master secret.  See section 8.1.2 of 
ftp://ftp.rfc-editor.org/in-notes/internet-drafts/draft-ietf-tls-rfc2246-bis-10.txt

NSS now implements its DH code in its pure software PKCS 11 module in 
the CKM_DH_PKCS_DERIVE mechanism.  As implemented, that mechanism 
returns a result from which all leading zero octets have been stripped.
However, PKCS 11 specifies that the result conforms to PKCS 3, and
PKCS 3 specifies that the result has k octets, where k is the minimum
number of octets required to represent p, the prime parameter.  

So, the output of NSS's CKM_DH_PKCS_DERIVE mechanism does not conform
to PKCS 3.  

Further, NSS's mechanism CKM_SSL3_MASTER_KEY_DERIVE_DH, which derives 
the master secret from a DH pre-master secret does not remove leading
zeros from the DH result that is input.  It is dependent on the non-PKCS3
behavior of NSS's CKM_DH_PKCS_DERIVE, and therefore would not work with
a value returned by a PKCS11 module that did conform to PKCS3.

I propose to change NSS so that 
a) the output of NSS's CKM_DH_PKCS_DERIVE does conform to PKCS3, and
b) CKM_SSL3_MASTER_KEY_DERIVE_DH does remove leading zero octets from
the input DH result.

There will be no net effect for users of NSS's libSSL.  Therefore, IMO,
this is not urgent for mozilla.  But it is somewhat urgent for users of
NSS's "softoken" PKCS11 module.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

We have not (yet) decided to respin 3.10 RTM for this, but that could yet 
happen, I think.  So I have targetted this at 3.10.1.  IMO, if we were to 
respin 3.10 for this, we would also want to include the fix for bug 191470
in that respin.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Make CKM_DH_PKCS_DERIVE comply with PKCS3

This is the first of two relevant patches.  
This one makes CKM_DH_PKCS_DERIVE produce output of length required by PKCS#3. 

This will break the DHE ciphersuites unless the next patch is also applied.
Any of Julien, Bob, Wan-Teh: please review.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Change CKM_SSL3_MASTER_KEY_DERIVE_DH to discard leading zero bytes from PMS

This patch changes CKM_SSL3_MASTER_KEY_DERIVE_DH and
CKM_TLS_MASTER_KEY_DERIVE_DH to discard leading zero bytes from the PSM derived
by DH or DHE.  
This patch does not require the CKM_DH_PKCS_DERIVE patch to be checked in, 
but is required if that patch is checked in.
Julien, Bob or Wan-Teh, please review.

Comment 4

[Julien Pierre]

Comment on attachment 181599 [details] [diff] [review]
Make CKM_DH_PKCS_DERIVE comply with PKCS3

The fix appears to be correct; however there are questions about the opportune
target release for it, given the implications on applications compatibility.

Comment 5

[Julien Pierre]

Comment on attachment 181600 [details] [diff] [review]
Change CKM_SSL3_MASTER_KEY_DERIVE_DH to discard leading zero bytes from PMS

s to be correct; however there are questions about the opportune release for
it, given the implications on applications compatibility.

Comment 6

[Wan-Teh Chang]

Do you still need this fixed in 3.10.2, or is 3.11 okay?

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 181600 [details] [diff] [review]
Change CKM_SSL3_MASTER_KEY_DERIVE_DH to discard leading zero bytes from PMS

We need to collectively decide the right way to fix this completely.  
The answer has to work for SSL3 and TLS, for RSA, DH, and ECDH.  

I think my patch (attached) would break ECDH.  IINM and IIRC, our DH and ECDH
code paths both use the same mechanisms (CKM_TLS_MASTER_KEY_DERIVE_DH and
CKM_SSL3_MASTER_KEY_DERIVE_DH) to derive the Master Secret (MS) from the 
Pre-Master Secret (PMS).  For DH, it is proper to strip leading zeros.	
For ECDH, it is not.  Leading zeros should be preserved for ECDH.  

Somehow, the code that derives the MS from the PMS needs to know whether 
it the PMS came from RSA, or from DHE, or from ECDHE.  Some possible
solutions include:

A) create a new pair of mechanisms for ECDH, named 
CKM_TLS_MASTER_KEY_DERIVE_ECDH and CKM_SSL3_MASTER_KEY_DERIVE_ECDH and 
change our SSL code to use them when appropriate.  Then change the 
two _DH mechanisms to strip leading zeros.  The DH and ECDH versions
would differ only by whether they do stripping or not.

B) Differentiate the three types of PMSes with different values for some
attribute.  This requires that the PMSes have that attribute set at the 
time they are derived (via RSA unwrap, or DH, or ECDH), so that the 
CKM_XXX_MASTER_KEY_DERIVE_DH code can tell them apart.	

The question for proposal B is:
what attribute would we use for this purpose?  
Would we invent 3 new key object types?  (I think currently the PMSes 
are "generic secret" type keys.)
Would we invent another attribute for generic secret keys?
Is there already some attribute that we can use for this purpose?

Is there a third alternative I've not considered? 
Suggestions invited!

Comment 8

[Andreas Sterbenz]

(In reply to comment #7)
> 
> Somehow, the code that derives the MS from the PMS needs to know whether 
> it the PMS came from RSA, or from DHE, or from ECDHE.  Some possible
> solutions include:

Yes. Note we already differentiate RSA from non-RSA today as they use different
mechanisms (CKM_TLS_MASTER_KEY_DERIVE vs. CKM_TLS_MASTER_KEY_DERIVE_DH).

> A) create a new pair of mechanisms for ECDH, named 
> CKM_TLS_MASTER_KEY_DERIVE_ECDH and CKM_SSL3_MASTER_KEY_DERIVE_ECDH and 
> change our SSL code to use them when appropriate.  Then change the 
> two _DH mechanisms to strip leading zeros.  The DH and ECDH versions
> would differ only by whether they do stripping or not.

I prefer this solution because it fits with the existing separation of RSA from
non-RSA master secret derivation that I mentioned above.

Comment 9

[Andreas Sterbenz]

Another obvious solution would be to define a new DH mechanism
(CKM_TLS_PKCS_DERIVE) that is specified to strip the leading 0x00 bytes as
required by SSL/TLS. NSS and other SSL implementation would use that mechanism
instead of CKM_DH_PKCS_DERIVE and no changes to the CKM_TLS_MASTER_KEY_DERIVE_DH
mechanism would be required.

That would in many ways seem preferable. The only problem is that that other
crypto implementations (such as hardware accelerators for DH) would have to be
updated as well.

Comment 10

[Julien Pierre]

Per our meeting today, there hasn't been agreement from the PKCS#11 working group. We can't stop our 3.11 release waiting for it. Lowering to P2.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Bob, Andreas, Vipul,
in a recent converence call, we agreed on a solution to this problem, but I 
do not now recall exactly what we agreed on.  I beleive it was either:

a) Change the definition of CKM_DH_PKCS_DERIVE to produce output that lacks
leading zero bytes.  This means that mechanism will no longer be defined to 
conform to PKCS#3.  I recall we thought some existing implementations would 
complain about that.  

b) Change the definition of CKM_SSL3_MASTER_KEY_DERIVE_DH to require it to 
strip leading zeros from the DH PMS.  However, IIRC, the problem with that 
proposal was that if it strips leading zeros, then it cannot be used with 
ECDH, because TLS+ECDH defines the PMS to be the entire result (with leading
zeros intact) and forbids the removal of the leading zeros in the derivation
of the MS from the PMS.  That might necessitate the creation of yet another
derivation mechanism, e.g. CKM_SSL3_MASTER_KEY_DERIVE_ECDH.  

Do you recall a solution that doesn't have any of these difficulties?

Comment 12

[Julien Pierre]

Retargetting all P2s to 3.11.3 .