Closed Bug 951455 Opened 9 years ago Closed 3 months ago

Implement official PKCS #11 mechanisms for TLS 1.2

Categories

(NSS :: Libraries, enhancement, P1)

3.15
enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: wtc, Assigned: KaiE)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached patch Patch v1 (obsolete) — Splinter Review
In bug 577019, we added NSS vendor-defined PKCS #11 mechanisms
for PKCS #11, which are hardcoded to use the default TLS 1.2 PRF
based on SHA-256.

In this bug we will implement the official PKCS #11 mechanisms
for TLS 1.2 specified in PKCS #11 v2.40 draft.

The attached patch implements the following mechanisms:
  CKM_TLS12_MASTER_KEY_DERIVE
  CKM_TLS12_MASTER_KEY_DERIVE_DH
  CKM_TLS12_KEY_AND_MAC_DERIVE
  CKM_TLS_MAC

I still need to implement CKM_TLS_KDF. It is only used by the
TLS keying material exporters (RFC 5705).

I don't plan to implement CKM_TLS12_KEY_SAFE_DERIVE because it
doesn't support AES-GCM cipher suites.
Attachment #8349101 - Flags: review?(rrelyea)
Comment on attachment 8349101 [details] [diff] [review]
Patch v1

The PKCS #11 v2.40 draft I used for this patch is:

  PKCS #11 Cryptographic Token Interface
  Current Mechanisms Specification
  Version 2.40
  Committee Specification Draft 01 /
  Public Review Draft 01
  30 October 2013
Comment on attachment 8349101 [details] [diff] [review]
Patch v1

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

This looks like what we started with for the sha384 cipher support. I think it best we check this in first and land the whole thing in pieces.

There is one thing I would like to add here, and that is code that handles the case where only that NSS TLS1.2 SHA256 mechanism is available in softoken. We would need that for our RHEL-5, and RHEL-6 system. Preferably I'd like a runtime check, but we could do an ifdef.

I think it makes more sense to handle this additional add by a separate patch rather than kicking this patch back and adding it.

bob
Attachment #8349101 - Flags: review?(rrelyea) → review+
Attachment #8349101 - Attachment is obsolete: true
Pushed: https://hg.mozilla.org/projects/nss/rev/f9b7bdf6e68b
on Wan-Teh's behalf
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Will revise the patches for Bug 923089 accordingly.
The patch introduced a build failure on Windows at this line:
https://hg.mozilla.org/projects/nss/file/f9b7bdf6e68b/lib/softoken/pkcs11c.c#l2560

Error is:
c:/slavedir/3-win7-x64-DBG/hg/nss/lib/softoken/pkcs11c.c(2560) : error C4090: 'function' : different 'const' qualifiers

(Full log e.g. at https://bot.nss-crypto.org:8011/builders/3-winxp-x64-OPT/builds/568/steps/shell_1/logs/stdio )


Also: Please remember to set the target milestone correctly on checkin, to ensure the bugs will be found by release notes query links. Thanks.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Can you please look into the build failure? Thanks
Flags: needinfo?(wtc)
Target Milestone: 3.15.5 → 3.20
The API expects the parameter to be non-const (modifyable).
(Which seems weird for an API that only reads data to calculate a hash???)

The "label" parameter is declared const (which makes sense, as constant strings are assigned to it), therefore the non-const check fails.
Are we allowed to change the second parameter of typedef SFTKHash to const?

This patch builds on Linux.
Attachment #8642564 - Flags: review?(rrelyea)
Attachment #8642564 - Flags: review?(wtc)
Attachment #8642564 - Flags: feedback-
Attachment #8642564 - Flags: feedback-
Comment on attachment 8642564 [details] [diff] [review]
bustage-951455-v1.patch

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

r=wtc. Thank you, Kai. I just suggest a potential small change.

::: lib/softoken/pkcs11i.h
@@ +724,5 @@
>  	CK_MECHANISM_PTR mech, SFTKObject *key);
>  sftk_MACConstantTimeCtx* sftk_SSLv3MACConstantTime_New(
>  	CK_MECHANISM_PTR mech, SFTKObject *key);
> +void sftk_HMACConstantTime_Update(void *pctx, const void *data, unsigned int len);
> +void sftk_SSLv3MACConstantTime_Update(void *pctx, const void *data, unsigned int len);

If this line is longer than 80 characters, please fold it.
Attachment #8642564 - Flags: review?(wtc) → review+
Attachment #8642564 - Flags: review?(rrelyea)
The bustage fix worked, Windows builds are green.
Marking resolved agagin.

https://hg.mozilla.org/projects/nss/rev/185e4bdc3775
Flags: needinfo?(wtc)
Target Milestone: 3.20 → 3.21
Apparently I had made a mistake in comment 11. I accidentally didn't resolve this as fixed. Resolving now.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
My bad. This patch does not work with old softokens that don't have the official TLS 1.2 code.It needs a runtime check and fall back to the old code. I should have caught that in the review.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

The bug assignee didn't login in Bugzilla in the last months and this bug has priority 'P1'.
:beurdouche, could you have a look please?
For more information, please visit auto_nag documentation.

Assignee: wtc → nobody
Flags: needinfo?(bbeurdouche)

I think that this is OBE; we use all of those mechanisms exclusively. Bob, do you have any need to support softoken without these mechanisms? It seems like maybe 7 years is enough time for that need to have lapsed.

Flags: needinfo?(bbeurdouche) → needinfo?(rrelyea)

Well technically 7 years is 3 years short of enough time;). That being said, I believe we already have to correct code to use those mechanism, which is what the bug is about, and the current code is working on all our platforms either as is, our with an appropriate downstream patch, so yes this bug can be closed.

Flags: needinfo?(rrelyea)

Thanks Bob, (especially for the reminder about RHEL).

Re-marking this as resolved/fixed thanks to Kai.

Assignee: nobody → kaie
Status: REOPENED → RESOLVED
Closed: 7 years ago3 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.