951455 - Implement official PKCS #11 mechanisms for TLS 1.2

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[Wan-Teh Chang]

Attachment: Patch v1

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.

Comment 1

[Wan-Teh Chang]

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 2

[Robert Relyea]

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

Comment 3

[Elio Maldonado]

Attachment: Patch v2 - updated for latest sources

Comment 4

[Elio Maldonado]

Pushed: https://hg.mozilla.org/projects/nss/rev/f9b7bdf6e68b
on Wan-Teh's behalf

Comment 5

[Elio Maldonado]

Will revise the patches for Bug 923089 accordingly.

Comment 6

[Kai Engert [:KaiE:]]

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.

Comment 7

[Kai Engert [:KaiE:]]

Can you please look into the build failure? Thanks

Comment 8

[Kai Engert [:KaiE:]]

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.

Comment 9

[Kai Engert [:KaiE:]]

Attachment: bustage-951455-v1.patch

Are we allowed to change the second parameter of typedef SFTKHash to const?

This patch builds on Linux.

Comment 10

[Wan-Teh Chang]

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.

Comment 11

[Kai Engert [:KaiE:]]

The bustage fix worked, Windows builds are green.
Marking resolved agagin.

https://hg.mozilla.org/projects/nss/rev/185e4bdc3775

Comment 12

[Kai Engert [:KaiE:]]

Apparently I had made a mistake in comment 11. I accidentally didn't resolve this as fixed. Resolving now.

Comment 13

[Robert Relyea]

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.

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

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.

Comment 15

[Martin Thomson [:mt:]]

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.

Comment 16

[Robert Relyea]

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.

Comment 17

[Martin Thomson [:mt:]]

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

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