1117022 - Implement draft-ietf-tls-session-hash

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[Eric Rescorla (:ekr)]

We should implement:

https://tools.ietf.org/html/draft-ietf-tls-session-hash-03

Comment 1

[Eric Rescorla (:ekr)]

I'm starting to look at this. Wan-Teh, I don't see an implementation of this in the Chrome patches, but I wanted to double check and see if this is something anyone at Google had done before I started the work

Comment 2

[Wan-Teh Chang]

Eric,

Nobody at Google has implemented the TLS Session Hash Extension in NSS.
What has been implemented in Chromium is a fix specific to TLS Channel
ID, not the more general Session Hash.

The TLS Channel ID-specific fix is in
src/net/third_party/nss/patches/channelid.patch in the Chromium source
tree. Search for "originalHandshakeHash" in that patch.

Comment 3

[Eric Rescorla (:ekr)]

Wan-Teh,

I have started implementing this and am trying to decide how much
to expose to the programmer.

1. Do you think we should expose the ability to require session hash?
2. Do you think we should expose whether session hash was used?

Comment 4

[Ryan Sleevi]

The latter seems essential if Mozilla is going to gather metrics on its adoption, particularly if they hope to adopt the former.

Comment 5

[Richard Barnes [:rbarnes]]

Attachment: bug-1117022.pkcs11.patch

This patch implements a general mechanism to derive a TLS master secret using the TLS PRF.  This mechanism still assumes that it is deriving a TLS master secret, but it allows the caller to directly specify the non-secret inputs to the PRF (label and seed), rather than relying on the PKCS#11 implementation to fill them in based on the mechanism chosen.

This should let the session-hash implementation pass in the new label ("extended master secret") and the session hash as the seed value.

The implementation is pretty brute force, copying code from the older uses of the PRF below and making simplifications based on the fact that it's TLS-only.

Comment 6

[Eric Rescorla (:ekr)]

Thanks Richard. I note that this doesn't necessarily cover the "bypass"
case. Bob, Wan-Teh, how important is it to retain that in the face of
session hash or can we just use the non-bypass code path?

Comment 7

[Eric Rescorla (:ekr)]

Richard, unfortunately I mislead you: you need to do hashes other than SHA-256 to support TLS 1.0 and 1.1.

Comment 8

[Robert Relyea]

I don't compile with bypass on, so I don't think it's a problem.

Richard, PKCS #11 2.40 is (almost) out, It's an approved committee spec and in it's final 60 day review. Ideally we should implement the new TLS mechanisms specified by 2.40:

http://docs.oasis-open.org/pkcs11/pkcs11-curr/v2.40/cos01/pkcs11-curr-v2.40-cos01.html

specifically:
http://docs.oasis-open.org/pkcs11/pkcs11-curr/v2.40/cos01/pkcs11-curr-v2.40-cos01.html#_Toc408227088

Comment 9

[Richard Barnes [:rbarnes]]

Bob, thanks for the pointers to PCKS#11.  It looks like CKM_TLS_KDF is roughly what we want, but there's a fairly major ambiguity in the spec.  The parameter struct for CKM_TLS_KDF is as follows:

> typedef struct CK_TLS_KDF_PARAMS {
>   CK_MECHANISM_TYPE prfMechanism;
>   CK_BYTE_PTR pLabel;
>   CK_ULONG ulLabelLength;
>   CK_SSL3_RANDOM_DATA RandomInfo;
>   CK_BYTE_PTR pContextData;
>   CK_ULONG ulContextDataLength;
> } CK_TLS_KDF_PARAMS;

The prfMechanism, label, and randomInfo parts seem self-explanatory.  However, it's not clear at all what the "context data" elements are supposed to contain.  There does not appear to be a definition in the PKCS#11 spec or any of the relevant RFCs (5246, 4346, 2246, or 6101)

In order to be usable for this bug, it would have to be the case that the "context data" is the same as the "seed" field used in the TLS specifications (PRF(secret, label, seed)).  In which case, the implication would be:

> if (params.pContextData)
>   /* seed = params.pContextData */
> else 
>   /* seed = ClientRandom + ServerRandom */

Does that match your understanding?  What do we need to do to get the spec clarified?

If "context data" means something else, then we may need a new mechanism.

Comment 10

[Richard Barnes [:rbarnes]]

We discussed this on the NSS call.  The context data question turns out to be explained by RFC 5705, which says that if context data is provided, the output is as follows:

> PRF(SecurityParameters.master_secret, label,
>     SecurityParameters.client_random +
>     SecurityParameters.server_random +
>     context_value_length + context_value
> )[length]

That is not a construct that we can use for this bug, since the session-hash spec requires that the session hash be input directly to the PRF.

It does look like the mechanism CKM_TLS_PRF from PKCS#11 v2.20 does what we want, so we will proceed with implementing that mechanism.  This is a little bit of a spec compliance problem, since that mechanism has been deprecated in the latest version of PKCS#11.  So we should follow up with PKCS#11 to see what we need to do to get some flavor of this back in the spec (possibly with additional constraints).

Comment 11

[Richard Barnes [:rbarnes]]

Attachment: bug-1117022.pkcs11.1.patch

This patch updates the previous patch in two important ways:

* It re-uses the mechanism CKM_TLS_PRF_GENERAL that is already used for verifying finish messages.  This mechanism is proprietary, but general enough to cover our needs.

* It adds support for all hash algorithms, and pre-TLS1.2 versions of the PRF.

Comment 12

[Richard Barnes [:rbarnes]]

Attachment: bug-1117022.pkcs11.2.patch

Adds support for reflecting the version number from the PMS, when the input is a PMS and the requestor asks for a version number.

Bob: It looks to me like this line is a NOOP (as is the corresponding one below in the current code):

> ((sessKey == NULL) || sessKey->wasDerived)

... since sessKey is never null (key is not a token object) and sessKey->wasDerived is never true (since it was just created, with default value PR_FALSE).

Does that seem right to you?

Comment 13

[Robert Relyea]

Comment on attachment 8559282 [details] [diff] [review]
bug-1117022.pkcs11.2.patch

feedback+. 
Re the test. Hmmm It looks like you are right. I the test should have been on the source key rather than the target key.

Comment 14

[Eric Rescorla (:ekr)]

Bob, WTC, are you OK with making this option on by default?

Comment 15

[Eric Rescorla (:ekr)]

Bob, Wan-Teh, ping?

Barnes, do you want to have two master_derive mechanisms to PKCS#11, one for
RSA and one for non-RSA, or do you just want to make it contingent on whether
we pass a nonzero outparam?

Comment 16

[Richard Barnes [:rbarnes]]

I think it's cleaner to have one mechanism.  Even though externally there are separate mechanisms for the others right now, internally, they all use the same code and branch on whether there's a nonzero outparam.  Might as well have the external reflect the internal.

https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/softoken/pkcs11c.c#5926
https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/softoken/pkcs11c.c#5977

Bob, do you think this patch looks ready to land?

Comment 17

[Robert Relyea]

Comment on attachment 8559282 [details] [diff] [review]
bug-1117022.pkcs11.2.patch

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

Three things I'd like to have fixed before check in.

1) since we didn't use one of the standard params, we should go ahead and make our own mechanism.
2) There's a already a function that maps mechanism to hashalg's for us.
3) We shouldn't implicitly set these attributes. Normally PKCS #11 will get these from the supplied template.

::: lib/softoken/pkcs11c.c
@@ +5923,5 @@
>      /*
>       * generate the master secret 
>       */
> +    case CKM_TLS_PRF_GENERAL:
> +      {

Since we are creating our own parameters, we should probably create an NSS specific mechanism for this. We should also propose this mechanism to the PKCS #11 working group to get a real mechanism value for this.
 
If you could put together a right up on how this is used and why as well as why the existing 3.40 mechanisms don't quite match what you want, I can bring that to the working group.

@@ +5976,5 @@
> +            case CKM_SHA_1: hash = HASH_AlgSHA1; break;
> +            case CKM_SHA256: hash = HASH_AlgSHA256; break;
> +            case CKM_SHA384: hash = HASH_AlgSHA384; break;
> +            case CKM_SHA512: hash = HASH_AlgSHA512; break;
> +          }

You can use GetHashTypeFromMachanism() here, instead

@@ +6017,5 @@
> +        crv = sftk_forceAttribute(key,CKA_VERIFY,  &cktrue,sizeof(CK_BBOOL));
> +        if (crv != CKR_OK) break;
> +        crv = sftk_forceAttribute(key,CKA_DERIVE,  &cktrue,sizeof(CK_BBOOL));
> +        if (crv != CKR_OK) break;
> +        break;

These should probably be set explicitly by the user's template, rather than setting everything to true implicitly. NSC_DeriveKey already does this. validateSecretKey will guarrentee that these values are set to something, if you want to override those default use sftk_defaultAttribute().
 
NOTE: validateSecretKey will not default CKA_KEY_TYPE, but will fail if it's not supplied, so defaulting it would have a different semantic than for other cases where keytype isn't implicitly set. (like other derive operations.
 
My recommendation is just to remove these and set the desired attributes in the template.

NOTE: In case it isn't clear this comment applies to everything from skft_forceAttribute(key, CKA_KEY_TYPE, ...) to sftk_forceAttribute(key, CKA_DERIVE, ...)

Comment 18

[Robert Relyea]

Eric, I'm OK with this as default as long as it can be easily reverted if we have to.

Comment 19

[Robert Relyea]

ooks, I didn't clear the need info...

Comment 20

[Eric Rescorla (:ekr)]

Wan-Teh's comments on the PKCS#11 mechanism follow below:

[e-mail]
5:18 PM (2 minutes ago)

to ekr-webrtc, martin.thomson, rbarnes, reply 
Comments on patch set 4: the new PKCS #11 mechanism

Please forward these comments to Richard Barnes and Bob Relyea.

I took a closer look at the PKCS #11 mechanism. I describe my proposal
below. If you like the proposal, we should also submit it to the PKCS
#11 Working Group for review.

I went for a less general design that's consistent with the
current TLS master key derivation mechanisms. An alternative is to
generalize and extend the CKM_TLS_KDF mechanism (recently added in PKCS
#11 v2.40 for RFC 5705). That design would closer to what Richard
designed.

Eric: do we need to update RFC 5705 to use session_hash instead of
SecurityParameters.client_random + SecurityParameters.server_random?


https://codereview.appspot.com/248480044/diff/60001/lib/util/pkcs11n.h
File lib/util/pkcs11n.h (right):

https://codereview.appspot.com/248480044/diff/60001/lib/util/pkcs11n.h#newcode327
lib/util/pkcs11n.h:327: * For the TLS 1.2 PRF, the hashAlg parameter
determines the hash function used.
hashAlg => prfHashMechanism

https://codereview.appspot.com/248480044/diff/60001/lib/util/pkcs11n.h#newcode333
lib/util/pkcs11n.h:333: CK_MECHANISM_TYPE prfMechanism;
Please name this member "prfHashMechanism".

https://codereview.appspot.com/248480044/diff/60001/lib/util/pkcs11n.h#newcode339
lib/util/pkcs11n.h:339: } CK_NSS_TLSPRFParams;
The corresponding struct for TLS <= 1.2 master key derivation is:

typedef struct CK_TLS12_MASTER_KEY_DERIVE_PARAMS {
  CK_SSL3_RANDOM_DATA RandomInfo;
  CK_VERSION_PTR pVersion;
  CK_MECHANISM_TYPE prfHashMechanism;
} CK_TLS12_MASTER_KEY_DERIVE_PARAMS;

The prfHashMechanism field was put at the end so that this struct
can be viewed as an extension of the CK_SSL3_MASTER_KEY_DERIVE_PARAMS
struct:

typedef struct CK_SSL3_MASTER_KEY_DERIVE_PARAMS {
  CK_SSL3_RANDOM_DATA RandomInfo;
  CK_VERSION_PTR pVersion;
} CK_SSL3_MASTER_KEY_DERIVE_PARAMS;

That kind of struct layout compatibility is not useful here, so
we can put the the prfHashMechanism field first. I suggest the
following struct name and fields:

typedef struct CK_NSS_TLS12_EXTENDED_MASTER_KEY_DERIVE_PARAMS {
    CK_MECHANISM_TYPE prfHashMechanism;
    CK_BYTE_PTR pSessionHash;
    CK_ULONG ulSessionHashLen;
    CK_VERSION_PTR pVersion;
} CK_NSS_TLS12_EXTENDED_MASTER_KEY_DERIVE_PARAMS;

Notes:

1. I omitted pLabel and ulLabelLen. The mechanisms should be hardcoded
to use the "extended master secret" label.

2. I suggest we add two mechanisms:

  CKM_NSS_TLS12_EXTENDED_MASTER_KEY_DERIVE
  CKM_NSS_TLS12_EXTENDED_MASTER_KEY_DERIVE_DH

3. The mechanisms should verify ulSessionHashLen is consistent with
the hash algorithm specified in prfHashMechanism.

4. Key derivation in TLS 1.3 looks very different, so I don't try
to make these mechanisms work for TLS 1.3 as well. This means we
can probably remove the "12" from the mechanism and struct names.

Comment 21

[Eric Rescorla (:ekr)]

Wan-Teh,

WRT RFC 5705, my understanding is that because the master_secret derives from the session hash, no update to RFC 5705's mechanisms is needed, but it would be wise to recommend using it with the session hash.

Comment 22

[Wan-Teh Chang]

(In reply to Eric Rescorla (:ekr) from comment #21)
> Wan-Teh,
> 
> WRT RFC 5705, my understanding is that because the master_secret derives
> from the session hash, no update to RFC 5705's mechanisms is needed, ...

Ah, you're right. I'm sorry I missed that.

Comment 23

[Richard Barnes [:rbarnes]]

Attachment: bug-1117022.pkcs11.3.patch

This patch implements Wan-Teh's suggested structure, making a mechanism specific to the TLS extended master secret derivation, rather than a way to derive keys with arbitrary labels.

(In reply to Robert Relyea from comment #17)
> Comment on attachment 8559282 [details] [diff] [review]
> bug-1117022.pkcs11.2.patch
> 
> Review of attachment 8559282 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Three things I'd like to have fixed before check in.
> 
> 1) since we didn't use one of the standard params, we should go ahead and
> make our own mechanism.
> 2) There's a already a function that maps mechanism to hashalg's for us.
> 3) We shouldn't implicitly set these attributes. Normally PKCS #11 will get
> these from the supplied template.
> 
> ::: lib/softoken/pkcs11c.c
> @@ +5923,5 @@
> >      /*
> >       * generate the master secret 
> >       */
> > +    case CKM_TLS_PRF_GENERAL:
> > +      {
> 
> Since we are creating our own parameters, we should probably create an NSS
> specific mechanism for this. We should also propose this mechanism to the
> PKCS #11 working group to get a real mechanism value for this.

As above, I have implemented WTC's suggestion and added two mechanisms:

* CKM_NSS_TLS_EXTENDED_MASTER_KEY_DERIVE
* CKM_NSS_TLS_EXTENDED_MASTER_KEY_DERIVE_DH

These follow the more narrow course of defining a derivation specific to the TLS extended master secret computation.  If we were going to imagine proposing something to the PKCS#11 WG, I might consider doing it as a more general thing, in case we need to be able to derive with other labels in the future.  But then again, it might be enough to just factor out the common parts of the derivation inside of softoken, i.e., use the same code to satisfy multiple mechanisms.  That way the application isn't trusted to get the label right.


> If you could put together a right up on how this is used and why as well as
> why the existing 3.40 mechanisms don't quite match what you want, I can
> bring that to the working group.

There's obviously not a mechanism for extended master secret derivation in particular.  I'm not seeing any general TLS PRF mechanism  at all in the current PKCS#11 mechanisms spec (v2.40) -- not even CKM_TLS_PRF_GENERAL:

http://docs.oasis-open.org/pkcs11/pkcs11-curr/v2.40/pkcs11-curr-v2.40.html

Maybe CKM_TLS_PRF_GENERAL is actually NSS specific?  That's what Google searches tell me, anyway.

The closest thing I see in v2.4 is CKM_WTLS_PRF (WTLS?  whoa, blast from the past!).  As far as I can tell from the only WTLS spec I could find, the WTLS PRF is the same as the TLS 1.2 PRF, so maybe this would be OK.  See section 11.3.2 of this document:

http://technical.openmobilealliance.org/tech/affiliates/wap/wap-261-wtls-20010406-a.pdf

But there's still some issues.  This mechanism lacks the version output that we need, and it has an output parameter that's not necessary for these applications.  And honestly, referring to WTLS is kind of a downer.



> @@ +5976,5 @@
> > +            case CKM_SHA_1: hash = HASH_AlgSHA1; break;
> > +            case CKM_SHA256: hash = HASH_AlgSHA256; break;
> > +            case CKM_SHA384: hash = HASH_AlgSHA384; break;
> > +            case CKM_SHA512: hash = HASH_AlgSHA512; break;
> > +          }
> 
> You can use GetHashTypeFromMachanism() here, instead

Done.

Also, in order to address WTC's comment about checking the length of the input session hash, I added a call to HASH_GetRawHashObject.


> @@ +6017,5 @@
> > +        crv = sftk_forceAttribute(key,CKA_VERIFY,  &cktrue,sizeof(CK_BBOOL));
> > +        if (crv != CKR_OK) break;
> > +        crv = sftk_forceAttribute(key,CKA_DERIVE,  &cktrue,sizeof(CK_BBOOL));
> > +        if (crv != CKR_OK) break;
> > +        break;
> 
> These should probably be set explicitly by the user's template, rather than
> setting everything to true implicitly. NSC_DeriveKey already does this.
> validateSecretKey will guarrentee that these values are set to something, if
> you want to override those default use sftk_defaultAttribute().
>  
> NOTE: validateSecretKey will not default CKA_KEY_TYPE, but will fail if it's
> not supplied, so defaulting it would have a different semantic than for
> other cases where keytype isn't implicitly set. (like other derive
> operations.
>  
> My recommendation is just to remove these and set the desired attributes in
> the template.
> 
> NOTE: In case it isn't clear this comment applies to everything from
> skft_forceAttribute(key, CKA_KEY_TYPE, ...) to sftk_forceAttribute(key,
> CKA_DERIVE, ...)

Done.

Comment 24

[Wan-Teh Chang]

(In reply to Richard Barnes [:rbarnes] from comment #23)
>
> (In reply to Robert Relyea from comment #17)
> > Comment on attachment 8559282 [details] [diff] [review]
> > bug-1117022.pkcs11.2.patch
> >
> > If you could put together a right up on how this is used and why as well as
> > why the existing [2.40] mechanisms don't quite match what you want, I can
> > bring that to the working group.
> 
> There's obviously not a mechanism for extended master secret derivation in
> particular.  I'm not seeing any general TLS PRF mechanism  at all in the
> current PKCS#11 mechanisms spec (v2.40) -- not even CKM_TLS_PRF_GENERAL:
> 
> http://docs.oasis-open.org/pkcs11/pkcs11-curr/v2.40/pkcs11-curr-v2.40.html
> 
> Maybe CKM_TLS_PRF_GENERAL is actually NSS specific?  That's what Google
> searches tell me, anyway.

I can confirm both.

None of the new TLS 1.2 mechanisms added in PKCS #11 v2.40 can be used
for extended master secret derivation. Those mechanisms were all designed
to be just general enough to produce the intended output, so that an
attacker can't abuse the API to extract useful information about the
base secret key (the pre-master secret or the master secret) or the
TLS/SSL session keys.

CKM_TLS_PRF_GENERAL is NSS specific.

Comment 25

[Robert Relyea]

Comment on attachment 8645873 [details] [diff] [review]
bug-1117022.pkcs11.3.patch

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

Looks like you covered all my comments. r+

Comment 26

[Wan-Teh Chang]

Comment on attachment 8645873 [details] [diff] [review]
bug-1117022.pkcs11.3.patch

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

::: lib/softoken/pkcs11c.c
@@ +6150,5 @@
> +        att2 = sftk_FindAttribute(sourceKey,CKA_KEY_TYPE);
> +        if ((att2 == NULL) ||
> +            (*(CK_KEY_TYPE *)att2->attrib.pValue != CKK_GENERIC_SECRET)) {
> +          if (att2) sftk_FreeAttribute(att2);
> +          crv = CKR_KEY_FUNCTION_NOT_PERMITTED;

Please use an indentation level of four spaces in this file.

Comment 27

[Wan-Teh Chang]

Comment on attachment 8645873 [details] [diff] [review]
bug-1117022.pkcs11.3.patch

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

Richard:

The new mechanisms also need to be added to a mechanism info table
in lib/softoken/pkcs11.c, and possibly some other files in NSS.

For an example, please see CKM_TLS_MASTER_KEY_DERIVE and
CKM_TLS12_MASTER_KEY_DERIVE:
http://mxr.mozilla.org/nss/ident?i=CKM_TLS_MASTER_KEY_DERIVE
http://mxr.mozilla.org/nss/ident?i=CKM_TLS12_MASTER_KEY_DERIVE

Comment 28

[Richard Barnes [:rbarnes]]

Attachment: bug-1117022.pkcs11.4.patch r=rrelyea

Addressing final comments from wtc.

EKR, could you verify that the min/max key sizes are good, and if so, land it?

Comment 29

[Richard Barnes [:rbarnes]]

Attachment: tls_prf_test.tar.gz - Test case for the new mechanism

This is a simple, self-contained test of consistency between the new NSS mechanism and a python implementation of the same.  To run, adjust the include paths in the Makefile and run `make test`.

Comment 30

[Richard Barnes [:rbarnes]]

Attachment: bug-1117022.pkcs11.5.patch r=rrelyea

This patch addresses PKCS#11 plumbing issues that EKR discovered and provided fixes for.  It passes the attached test case.

Comment 31

[Martin Thomson [:mt:]]

That test is fine, but what would be good is a test that we could actually land. Are you able to write a gtest for this? The framework is already in place.

Comment 32

[Eric Rescorla (:ekr)]

Comment on attachment 8649072 [details] [diff] [review]
bug-1117022.pkcs11.5.patch r=rrelyea

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

As MT says, please do add a gtest for this.

::: lib/pk11wrap/pk11mech.c
@@ +379,5 @@
>      case CKM_NSS_TLS_MASTER_KEY_DERIVE_DH_SHA256:
>      case CKM_TLS_KEY_AND_MAC_DERIVE:
>      case CKM_NSS_TLS_KEY_AND_MAC_DERIVE_SHA256:
> +    case CKM_NSS_TLS_EXTENDED_MASTER_KEY_DERIVE:
> +    case CKM_NSS_TLS_EXTENDED_MASTER_KEY_DERIVE_DH:

Are these really the only places you need to indicate this? What about
lib/softoken/pkcs11c.c mechanismList?

Comment 33

[Robert Relyea]

Comment on attachment 8649072 [details] [diff] [review]
bug-1117022.pkcs11.5.patch r=rrelyea

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

::: lib/pk11wrap/pk11mech.c
@@ +379,5 @@
>      case CKM_NSS_TLS_MASTER_KEY_DERIVE_DH_SHA256:
>      case CKM_TLS_KEY_AND_MAC_DERIVE:
>      case CKM_NSS_TLS_KEY_AND_MAC_DERIVE_SHA256:
> +    case CKM_NSS_TLS_EXTENDED_MASTER_KEY_DERIVE:
> +    case CKM_NSS_TLS_EXTENDED_MASTER_KEY_DERIVE_DH:

The mechanism list is in pkcs11.c, and it appears to be included in this patch. The implementation is isn pkcs11c.c which is also in this patch.

Since these are derive mechanisms, I don't think there's a lot of places to need to tweak in the pk11wrap.

This patch doesn't have the ssl changes to use the call.

Comment 34

[Richard Barnes [:rbarnes]]

Attachment: bug-1117022.pkcs11.6.patch r=rrelyea

Fixing mt's nit from rietveld.

Comment 35

[Richard Barnes [:rbarnes]]

Attachment: bug-1117022.pkcs11.7.patch r=rrelyea

Fixing a few spacing nits that :mt noticed.

Comment 36

[Eric Rescorla (:ekr)]

Richard's patch #1 landed at:
https://hg.mozilla.org/projects/nss/rev/ad2c76bdd683

Comment 37

[Eric Rescorla (:ekr)]

Unit tests landed at:
https://hg.mozilla.org/projects/nss/rev/98b0d0acc455

Reviewed at:
https://codereview.appspot.com/258340043/

Comment 38

[Wan-Teh Chang]

Attachment: Fix a "variable set but not used" compiler warning

Without this patch, compilation with -Werror enabled fails on Linux:

gcc -o Linux3.13_x86_64_gcc_glibc_PTH_64_DBG.OBJ/pkcs11c.o -c -g -fPIC -DLINUX2_1 -m64 -Wall -pipe -ffunction-sections -fdata-sections -DLINUX -Dlinux -DHAVE_STRERROR -Werror -DXP_UNIX -DSHLIB_SUFFIX=\"so\" -DSHLIB_PREFIX=\"lib\" -DSOFTOKEN_LIB_NAME=\"libsoftokn3.so\" -DSHLIB_VERSION=\"3\" -DDEBUG -UNDEBUG -DDEBUG_wtc -D_REENTRANT -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -I../../../dist/Linux3.13_x86_64_gcc_glibc_PTH_64_DBG.OBJ/include -I../../../dist/public/nss -I../../../dist/private/nss  pkcs11c.c
pkcs11c.c: In function ‘NSC_DeriveKey’:
pkcs11c.c:6145:19: error: variable ‘status’ set but not used [-Werror=unused-but-set-variable]
         SECStatus status;
                   ^

Comment 39

[Wan-Teh Chang]

Attachment: Declare variables at the beginning of a block. Fix comment typos. Move code around.

Regrettably Visual C++ 2010 still doesn't allow C code to declare
variables in the middle of a block.

There are two such problems. For the second one, I found that the
code before the variable declaration is a redundant PMS key length
check, so I simply removed that.

IMPORTANT: I also moved the PMS key length check earlier, to match
the "classic" master secret derivation code. Please let me know
if you deliberately put the PMS key length check there.

Comment 40

[Wan-Teh Chang]

Comment on attachment 8652066 [details] [diff] [review]
Declare variables at the beginning of a block. Fix comment typos. Move code around.

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

::: lib/softoken/pkcs11c.c
@@ -6146,5 @@
>          SECItem pms    = { siBuffer, NULL, 0 };
>          SECItem seed   = { siBuffer, NULL, 0 };
>          SECItem master = { siBuffer, NULL, 0 };
>  
> -        ems_params = (CK_NSS_TLS_EXTENDED_MASTER_KEY_DERIVE_PARAMS*) pMechanism->pParameter;

This line is longer than 80 characters.

Comment 41

[Eric Rescorla (:ekr)]

Committed as:
https://hg.mozilla.org/projects/nss/rev/b858337c4c1b

Reviewed at:
https://codereview.appspot.com/259430043/

Comment 42

[Wan-Teh Chang]

Comment on attachment 8652055 [details] [diff] [review]
Fix a "variable set but not used" compiler warning

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

I'm checking in this patch to fix the Linux gcc 4.8 compilation failure
because now -Werror is enabled for gcc 4.8.

https://hg.mozilla.org/projects/nss/rev/3577e602bf78

::: lib/softoken/pkcs11c.c
@@ +6209,5 @@
>          }
> +        if (status != SECSuccess) {
> +            crv = CKR_FUNCTION_FAILED;
> +            break;
> +        }

I copied this |status| check from the code that handles
"master secret" derivation.

http://mxr.mozilla.org/nss/search?string=TLS_P_hash%28tlsPrfHash%2C%2B%26pms%2C%2B%22master%2Bsecret%22%2C

Comment 43

[Wan-Teh Chang]

Comment on attachment 8652066 [details] [diff] [review]
Declare variables at the beginning of a block. Fix comment typos. Move code around.

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

Martin: this patch also fixes related problems in addition to fixing
the variable declaration.

Comment 44

[Wan-Teh Chang]

Comment on attachment 8652066 [details] [diff] [review]
Declare variables at the beginning of a block. Fix comment typos. Move code around.

https://hg.mozilla.org/projects/nss/rev/3796f6b6077b

Comment 45

[Wan-Teh Chang]

Comment on attachment 8652055 [details] [diff] [review]
Fix a "variable set but not used" compiler warning

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

Martin: please see comment 42 for a description of this patch. Thanks.

Comment 46

[Martin Thomson [:mt:]]

Comment on attachment 8652055 [details] [diff] [review]
Fix a "variable set but not used" compiler warning

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

I reviewed this previously, but since you had asked others, and checked it in, I figured that you were OK.

Comment 47

[Wan-Teh Chang]

Attachment: Declare variables at the beginning of a block in ssl3con.c

This problem was introduced in https://hg.mozilla.org/projects/nss/rev/b858337c4c1b

Comment 48

[Eric Rescorla (:ekr)]

Comment on attachment 8654631 [details] [diff] [review]
Declare variables at the beginning of a block in ssl3con.c

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

Thank you for catching this. LGTM.

Comment 49

[Wan-Teh Chang]

Comment on attachment 8654631 [details] [diff] [review]
Declare variables at the beginning of a block in ssl3con.c

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

Thank you for the quick review! It was Kai's buildbots that caught this :-)

Patch checked in: https://hg.mozilla.org/projects/nss/rev/5fe63c0b9427

Comment 50

[Richard Barnes [:rbarnes]]

Comment on attachment 8652055 [details] [diff] [review]
Fix a "variable set but not used" compiler warning

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

LGTM