Open Bug 291498 Opened 16 years ago Updated 10 years ago
_DH _PKCS _DERIVE not PKCS3 compliant
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.
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.
Priority: -- → P1
Target Milestone: --- → 3.10.1
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.
Attachment #181599 - Flags: review?(julien.pierre.bugs)
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.
Attachment #181600 - Flags: review?(julien.pierre.bugs)
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.
Attachment #181599 - Flags: review?(julien.pierre.bugs) → review+
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.
Attachment #181600 - Flags: review?(julien.pierre.bugs) → review+
QA Contact: bishakhabanerjee → jason.m.reid
Do you still need this fixed in 3.10.2, or is 3.11 okay?
Target Milestone: 3.10.1 → 3.10.2
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!
Attachment #181600 - Attachment is obsolete: true
(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.
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.
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.
Priority: P1 → P2
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?
Target Milestone: 3.11 → 3.11.2
QA Contact: jason.m.reid → libraries
Retargetting all P2s to 3.11.3 .
Target Milestone: 3.11.2 → 3.11.3
Target Milestone: 3.11.3 → ---
You need to log in before you can comment on or make changes to this bug.