Updated and rebased

Updated and rebased

[ekr-rietveld, 2015-08-19 12:35:43]

Please review.

[ekr-rietveld, 2015-08-19 12:37:42]

MT: Sorry about the new issue, but once I rebased it was really hard to follow
the changes. I did upload to the old bug as well. I know this means you'll have
to do a new review.

WTC: FYI.

[mt, 2015-08-19 16:49:47]

I think that the tests could be tweaked a little here.

https://codereview.appspot.com/259430043/diff/1/external_tests/ssl_gtest/ssl_...
File external_tests/ssl_gtest/ssl_loopback_unittest.cc (right):

https://codereview.appspot.com/259430043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:382: TEST_P(TlsConnectStream,
ConnectStaticRSABogusPMSVersionIgnore) {
Does this need to be stream only?

https://codereview.appspot.com/259430043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:391: TEST_P(TlsConnectStream,
ConnectStaticRSABogusCKE) {
Add a comment here that this is functionally no different to the one two above
(and perhaps reorder them so that the two are together).

https://codereview.appspot.com/259430043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:527: // These test is stream
so we can catch the bad_record_mac alert.
s/These/This

https://codereview.appspot.com/259430043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:531:
TlsInspectorReplaceHandshakeMessage* i1 =
"il" is a poor name.

https://codereview.appspot.com/259430043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:589:
ExpectExtendedMasterSecret(true);
Why not just call EnableExtendedMasterSecret() here?

https://codereview.appspot.com/259430043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:615: ConnectExpectFail();
Does this generate a particular alert?

https://codereview.appspot.com/259430043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:626: Connect();
I think that this needs to have calls to ConfigureSessionCache() or it will
probably not be testing what you think it does.

https://codereview.appspot.com/259430043/diff/1/external_tests/ssl_gtest/tls_...
File external_tests/ssl_gtest/tls_agent.cc (right):

https://codereview.appspot.com/259430043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_agent.cc:431: PR_FALSE);
I really wonder why someone would ever want this feature, but it's good that it
is being tested.

https://codereview.appspot.com/259430043/diff/1/lib/ssl/ssl3ext.c
File lib/ssl/ssl3ext.c (right):

https://codereview.appspot.com/259430043/diff/1/lib/ssl/ssl3ext.c#newcode2652
lib/ssl/ssl3ext.c:2652: return SECFailure;
+  PORT_SetError(PR_NOT_IMPLEMENTED_ERROR);

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

https://codereview.appspot.com/259430043/diff/1/lib/util/pkcs11n.h#newcode325
lib/util/pkcs11n.h:325: * Parameter for with the TLS extended master secret key
derivation mechanisms:
s/for with/for

[ekr-rietveld, 2015-08-19 18:06:00]

PTAL

https://codereview.appspot.com/259430043/diff/1/external_tests/ssl_gtest/ssl_...
File external_tests/ssl_gtest/ssl_loopback_unittest.cc (right):

https://codereview.appspot.com/259430043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:382: TEST_P(TlsConnectStream,
ConnectStaticRSABogusPMSVersionIgnore) {
On 2015/08/19 16:49:47, mt wrote:
> Does this need to be stream only?

No. Done.

https://codereview.appspot.com/259430043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:391: TEST_P(TlsConnectStream,
ConnectStaticRSABogusCKE) {
On 2015/08/19 16:49:47, mt wrote:
> Add a comment here that this is functionally no different to the one two above
> (and perhaps reorder them so that the two are together).

Added some comments and reordered.

https://codereview.appspot.com/259430043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:527: // These test is stream
so we can catch the bad_record_mac alert.
On 2015/08/19 16:49:47, mt wrote:
> s/These/This

Done.

https://codereview.appspot.com/259430043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:531:
TlsInspectorReplaceHandshakeMessage* i1 =
On 2015/08/19 16:49:47, mt wrote:
> "il" is a poor name.

Done.

https://codereview.appspot.com/259430043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:589:
ExpectExtendedMasterSecret(true);
On 2015/08/19 16:49:47, mt wrote:
> Why not just call EnableExtendedMasterSecret() here?

Done.

https://codereview.appspot.com/259430043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:615: ConnectExpectFail();
On 2015/08/19 16:49:47, mt wrote:
> Does this generate a particular alert?

Yes.

https://codereview.appspot.com/259430043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:626: Connect();
On 2015/08/19 16:49:47, mt wrote:
> I think that this needs to have calls to ConfigureSessionCache() or it will
> probably not be testing what you think it does.

Done.

https://codereview.appspot.com/259430043/diff/1/lib/ssl/ssl3ext.c
File lib/ssl/ssl3ext.c (right):

https://codereview.appspot.com/259430043/diff/1/lib/ssl/ssl3ext.c#newcode2652
lib/ssl/ssl3ext.c:2652: return SECFailure;
On 2015/08/19 16:49:47, mt wrote:
> +  PORT_SetError(PR_NOT_IMPLEMENTED_ERROR);

Thanks. Also PORT_Assert(0);

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

https://codereview.appspot.com/259430043/diff/1/lib/util/pkcs11n.h#newcode325
lib/util/pkcs11n.h:325: * Parameter for with the TLS extended master secret key
derivation mechanisms:
On 2015/08/19 16:49:47, mt wrote:
> s/for with/for


Note: this is for Richard.

[mt, 2015-08-19 18:14:17]

LGTM

[ekr-rietveld, 2015-08-19 18:20:29]

Richard. Please look at MT's previous review and fix your piece.

[wtc1, 2015-08-20 00:18:03]

Review comments on patch set 2:

I only reviewed the master secret and extended master
secret derivation code, including the faux MS generation
code.

I cannot assess whether the algorithm you came up with is
good. I verified that the only difference from the original
code is that you now generate the faux MS before the
(extended) master secret derivation. Is that correct?

I found several places where the comments don't match
the code. Those should be fixed. I marked them with
"IMPORTANT".

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3599: ssl3_GenerateFauxMS(sslSocket *ss, PK11SymKey *pms,
ssl3CipherSpec *pwSpec,

It would be useful to document what it means for this function to
be called with a null or a non-null |pms|.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3608: PK11SlotInfo *slot;

I highly recommend you make |fpms| and |slot| local variables inside
the two if blocks. I.e., only leave |fms| at the function scope.
This way you don't need to set |slot| and |fpms| to null after
freeing them.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3612: * in RFC 5246  Sec. 7.4.7.1. Specifically, between this
function

I assume we cannot follow the first algorithm in RFC 5246  Sec. 7.4.7.1
because it is very cumbersome to do the last step:

    pre_master_secret = ClientHello.client_version || M[2..47]

in PKCS #11?

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3617: *    faux PMS value. If that fails, generate an error.

IMPORTANT: this doesn't match the code. The code always generates a
random faux PMS value, whether the RSA key exchange block is decrypted
correctly or not.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3623: *    Set the MS = F

This is a confusing place to document this algorithm, because the
algorithm is not entirely implemented in this function. But I can't
suggest a better place. So at least we should point out that this
algorithm is implemented across multiple functions.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3630: /* Step 1. Generate a faux master secret in the same
slot as

IMPORTANT: "Step 1" is confusing because it seems to refer to the step 1
on line 3615.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3632: * failure. */

IMPORTANT: This is also used in case of client hello version check failure.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3646: /* Step 2. No PMS or failure on the same slot, move to
internal slot. */

1. IMPORTANT: "Step 2" is confusing because it seems to refer to the
step 2 on line 3616.

2. Change "move to internal slot" to "try again on the internal slot".
This is because "move to" may be misinterpreted as moving a key to the
internal slot.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3779: PORT_Assert(fms);

IMPORTANT: this assertion should be after the if (rv != SECSuccess) check,
right?

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3814: }

IMPORTANT: delete these four lines (3811-3814) because
pwSpec->master_secret cannot be NULL at this point in the new code.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3850: tls_DeriveExtendedMasterSecret(sslSocket *ss, PK11SymKey
*pms)

Nit: tls_DeriveExtendedMasterSecret has a lot of code duplication with
ssl3_DeriveMasterSecret. They only differ in the first half, where they
set up the PKCS #11 mechanisms and the mechanism parameters. After that,
the second half (starting from the comment "Generate a faux MS for use in
case of failur") is the same.

It's possible that you didn't realize this. I suggest we just add a TODO
comment and refrain from making such a structural change right now.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3860: PRBool    isDH = (PRBool)
((ss->ssl3.hs.kea_def->exchKeyType == kt_dh) ||

Nit: in this function you should get rid of all the unnecessary
alignment of variable declarations or assignment operators (=).

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3889: key_derive    = CKM_NSS_TLS_KEY_AND_MAC_DERIVE_SHA256;

IMPORTANT: this seems like a merge error. This should be

    key_derive    = CKM_TLS12_KEY_AND_MAC_DERIVE;

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3896: 

Nit: delete a blank line.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3914: PORT_Assert(fms);

IMPORTANT: this assertion should be after the if (rv != SECSuccess) check,
right?

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3919: /* At this point we have something in the fms slot which
we in

1. Nit: "the fms slot" seems wrong because "fms" is not a PK11SlotInfo.

2. This comment should also be added to ssl3_DeriveMasterSecret.

[ekr-rietveld, 2015-08-20 00:44:14]

Thank you for the thorough review. I will fix the indicated issues. I have some
questions for you below.

Would you like to see another version of this patch before I land it?

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3608: PK11SlotInfo *slot;
On 2015/08/20 00:18:03, wtc1 wrote:
> 
> I highly recommend you make |fpms| and |slot| local variables inside
> the two if blocks. I.e., only leave |fms| at the function scope.
> This way you don't need to set |slot| and |fpms| to null after
> freeing them.

Willdo.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3612: * in RFC 5246  Sec. 7.4.7.1. Specifically, between this
function
On 2015/08/20 00:18:03, wtc1 wrote:
> 
> I assume we cannot follow the first algorithm in RFC 5246  Sec. 7.4.7.1
> because it is very cumbersome to do the last step:
> 
>     pre_master_secret = ClientHello.client_version || M[2..47]
> 
> in PKCS #11?

That is correct.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3617: *    faux PMS value. If that fails, generate an error.
On 2015/08/20 00:18:02, wtc1 wrote:
> 
> IMPORTANT: this doesn't match the code. The code always generates a
> random faux PMS value, whether the RSA key exchange block is decrypted
> correctly or not.

This may just be a comment problem. Steps 1 and 2 occur outside this function
in, for instance in
https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl3con.c...

Step 3 is the start of this function.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3623: *    Set the MS = F
On 2015/08/20 00:18:03, wtc1 wrote:
> 
> This is a confusing place to document this algorithm, because the
> algorithm is not entirely implemented in this function. But I can't
> suggest a better place. So at least we should point out that this
> algorithm is implemented across multiple functions.

I can do that.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3630: /* Step 1. Generate a faux master secret in the same
slot as
On 2015/08/20 00:18:03, wtc1 wrote:
> 
> IMPORTANT: "Step 1" is confusing because it seems to refer to the step 1
> on line 3615.

Will fix.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3632: * failure. */
On 2015/08/20 00:18:02, wtc1 wrote:
> 
> IMPORTANT: This is also used in case of client hello version check failure.

Thanks. Will fix.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3646: /* Step 2. No PMS or failure on the same slot, move to
internal slot. */
On 2015/08/20 00:18:02, wtc1 wrote:
> 
> 1. IMPORTANT: "Step 2" is confusing because it seems to refer to the
> step 2 on line 3616.
> 
> 2. Change "move to internal slot" to "try again on the internal slot".
> This is because "move to" may be misinterpreted as moving a key to the
> internal slot.

Agreed. Will fix.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3779: PORT_Assert(fms);
On 2015/08/20 00:18:03, wtc1 wrote:
> 
> IMPORTANT: this assertion should be after the if (rv != SECSuccess) check,
> right?

There are two things we can be asserting here:

1. That the function succeeded (which it always should)
2. That if it succeeded, fms is non-null

Presently, we are more-or-less asserting #1. The suggested change would assert
#2. Perhaps we should have two asserts, one for rv == SECSuccess and one for fms
is non-null.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3814: }
On 2015/08/20 00:18:03, wtc1 wrote:
> 
> IMPORTANT: delete these four lines (3811-3814) because
> pwSpec->master_secret cannot be NULL at this point in the new code.

Good catch. Will fix.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3850: tls_DeriveExtendedMasterSecret(sslSocket *ss, PK11SymKey
*pms)
On 2015/08/20 00:18:03, wtc1 wrote:
> 
> Nit: tls_DeriveExtendedMasterSecret has a lot of code duplication with
> ssl3_DeriveMasterSecret. They only differ in the first half, where they
> set up the PKCS #11 mechanisms and the mechanism parameters. After that,
> the second half (starting from the comment "Generate a faux MS for use in
> case of failur") is the same.
> 
> It's possible that you didn't realize this. I suggest we just add a TODO
> comment and refrain from making such a structural change right now.

I will make a note. I will add a TODO. IIRC there are small differences later as
well, but I will check and make the TODO rigt.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3860: PRBool    isDH = (PRBool)
((ss->ssl3.hs.kea_def->exchKeyType == kt_dh) ||
On 2015/08/20 00:18:02, wtc1 wrote:
> 
> Nit: in this function you should get rid of all the unnecessary
> alignment of variable declarations or assignment operators (=).

Willdo.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3889: key_derive    = CKM_NSS_TLS_KEY_AND_MAC_DERIVE_SHA256;
On 2015/08/20 00:18:03, wtc1 wrote:
> 
> IMPORTANT: this seems like a merge error. This should be
> 
>     key_derive    = CKM_TLS12_KEY_AND_MAC_DERIVE;

Thanks for catching this. This is a collision between this patch and bug 951455.
In the current code in dxr, we are still using the old mechanism, see:

https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl3con.c...

In the NSS tree, this has changed to the new mechanisms:
http://hg.mozilla.org/projects/nss/file/2df598ffa0a9/lib/ssl/ssl3con.c#l3624

I will fix.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3914: PORT_Assert(fms);
On 2015/08/20 00:18:02, wtc1 wrote:
> 
> IMPORTANT: this assertion should be after the if (rv != SECSuccess) check,
> right?


See above.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3919: /* At this point we have something in the fms slot which
we in
On 2015/08/20 00:18:03, wtc1 wrote:
> 
> 1. Nit: "the fms slot" seems wrong because "fms" is not a PK11SlotInfo.
> 
> 2. This comment should also be added to ssl3_DeriveMasterSecret.

Ack. Will do.

[wtc1, 2015-08-20 02:10:33]

On 2015/08/20 00:44:14, ekr-webrtc wrote:
> 
> Would you like to see another version of this patch before I land it?

If it's not too much trouble, please upload a new version
of this patch before you land it. You don't need to wait
for my review. I can review it after you land it.

Unfortunately I realized that I misunderstood what it
means for ssl3_DeriveMasterSecret to be called with
pms=NULL. I thought it meant the PKCS #1 RSA decryption
failed. But the link you provided

https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl3con.c...

shows that in that case, we have already generated a
faux PSM and passed that to ssl3_DeriveMasterSecret.
So I should review this code again.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3617: *    faux PMS value. If that fails, generate an error.

I see. I misunderstood what it means to call ssl3_GenerateFauxMS
with pms=NULL. That means I am not sure if I understand the master
secret derivation logic :-(

It seems that we may have pms=NULL under two conditions:
1. The master secret has been derived in PKCS #11 Bypass mode.
2. We are resuming a session, so we don't need to derive the
master secret.

I will need to read this code again with the new understanding :-(

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3779: PORT_Assert(fms);
On 2015/08/20 00:44:14, ekr-webrtc wrote:
> Perhaps we should have two asserts, one for rv == SECSuccess and one for fms
> is non-null.

Assertions should be used to detect bugs. In theory, ssl3_GenerateFauxMS
can fail if we run out of memory or some hardware crypto device failed.
So I would not assert that ssl3_GenerateFauxMS always succeeds. But it
makes sense to assert that if it succeeded, fms is non-null.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3850: tls_DeriveExtendedMasterSecret(sslSocket *ss, PK11SymKey
*pms)

On 2015/08/20 00:44:14, ekr-webrtc wrote:
> IIRC there are small differences later as
> well, but I will check and make the TODO rigt.

The only difference I spotted in the second half of these two functions
is the PKCS#11 bypass code at the end. I believe that code is also
correct for extended master secret.

[ekr-rietveld, 2015-08-20 03:36:16]

On 2015/08/20 02:10:33, wtc1 wrote:
> On 2015/08/20 00:44:14, ekr-webrtc wrote:
> > 
> > Would you like to see another version of this patch before I land it?
> 
> If it's not too much trouble, please upload a new version
> of this patch before you land it. You don't need to wait
> for my review. I can review it after you land it.

Thanks. I am happy to upload a new version.


> Unfortunately I realized that I misunderstood what it
> means for ssl3_DeriveMasterSecret to be called with
> pms=NULL. I thought it meant the PKCS #1 RSA decryption
> failed. But the link you provided
> 
>
https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl3con.c...
> 
> shows that in that case, we have already generated a
> faux PSM and passed that to ssl3_DeriveMasterSecret.
> So I should review this code again.
> 
> https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c
> File lib/ssl/ssl3con.c (right):
> 
>
https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
> lib/ssl/ssl3con.c:3617: *    faux PMS value. If that fails, generate an error.
> 
> I see. I misunderstood what it means to call ssl3_GenerateFauxMS
> with pms=NULL. That means I am not sure if I understand the master
> secret derivation logic :-(
> 
> It seems that we may have pms=NULL under two conditions:
> 1. The master secret has been derived in PKCS #11 Bypass mode.

I have not studied this case.

> 2. We are resuming a session, so we don't need to derive the
> master secret.

This appears to be correct.


> I will need to read this code again with the new understanding :-(

Thanks. I really appreciate it.


>
https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
> lib/ssl/ssl3con.c:3779: PORT_Assert(fms);
> On 2015/08/20 00:44:14, ekr-webrtc wrote:
> > Perhaps we should have two asserts, one for rv == SECSuccess and one for fms
> > is non-null.
> 
> Assertions should be used to detect bugs. In theory, ssl3_GenerateFauxMS
> can fail if we run out of memory or some hardware crypto device failed.
> So I would not assert that ssl3_GenerateFauxMS always succeeds. But it
> makes sense to assert that if it succeeded, fms is non-null.
>

This seems reasonable. I will do as you suggest.


>
https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
> lib/ssl/ssl3con.c:3850: tls_DeriveExtendedMasterSecret(sslSocket *ss,
PK11SymKey
> *pms)
> 
> On 2015/08/20 00:44:14, ekr-webrtc wrote:
> > IIRC there are small differences later as
> > well, but I will check and make the TODO rigt.
> 
> The only difference I spotted in the second half of these two functions
> is the PKCS#11 bypass code at the end. I believe that code is also
> correct for extended master secret.

I will take a look and provide an appropriate TODO as you suggest.

[ekr-rietveld, 2015-08-20 04:40:52]

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3617: *    faux PMS value. If that fails, generate an error.
On 2015/08/20 02:10:33, wtc1 wrote:
> 
> I see. I misunderstood what it means to call ssl3_GenerateFauxMS
> with pms=NULL. That means I am not sure if I understand the master
> secret derivation logic :-(
> 
> It seems that we may have pms=NULL under two conditions:
> 1. The master secret has been derived in PKCS #11 Bypass mode.

I wonder if this is in fact correct. Here is the code that processes
the RSA PMS in Bypass mode:

https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl3con.c...

As you can see, it eventually calls ssl3_MasterKeyDeriveBypass
https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl3con.c...
which fills in pwSpec->msItem
https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/derive.c#516

Then prior to calling ssl3_DeriveMasterSecret, we check that
pwSpec->msItem is empty at:
https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl3con.c#

I am still studying the code, so I am not yet certain, but I thought this might
be useful to you.

> 2. We are resuming a session, so we don't need to derive the
> master secret.
> 
> I will need to read this code again with the new understanding :-(

[ekr-rietveld, 2015-08-20 17:55:08]

MT, PTAL.

WTC, I updated per your comments. There is one question for you below.

All, please ignore the new ssl_prf_unittest.cc, which is actually being reviewed
on a different issue.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3599: ssl3_GenerateFauxMS(sslSocket *ss, PK11SymKey *pms,
ssl3CipherSpec *pwSpec,
On 2015/08/20 00:18:02, wtc1 wrote:
> 
> It would be useful to document what it means for this function to
> be called with a null or a non-null |pms|.

Done.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3608: PK11SlotInfo *slot;
On 2015/08/20 00:44:14, ekr-webrtc wrote:
> On 2015/08/20 00:18:03, wtc1 wrote:
> > 
> > I highly recommend you make |fpms| and |slot| local variables inside
> > the two if blocks. I.e., only leave |fms| at the function scope.
> > This way you don't need to set |slot| and |fpms| to null after
> > freeing them.
> 
> Willdo.

Re-reading this: we don't presently set them to null after freeing them. I've
moved these to block scope, but I'm concerned that this comment indicates a
logic error that I missed. Can you elaborate.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3623: *    Set the MS = F
On 2015/08/20 00:18:03, wtc1 wrote:
> 
> This is a confusing place to document this algorithm, because the
> algorithm is not entirely implemented in this function. But I can't
> suggest a better place. So at least we should point out that this
> algorithm is implemented across multiple functions.

Done.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3632: * failure. */
On 2015/08/20 00:44:14, ekr-webrtc wrote:
> On 2015/08/20 00:18:02, wtc1 wrote:
> > 
> > IMPORTANT: This is also used in case of client hello version check failure.
> 
> Thanks. Will fix.

I just removed this since it's explained better above.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3646: /* Step 2. No PMS or failure on the same slot, move to
internal slot. */
On 2015/08/20 00:18:02, wtc1 wrote:
> 
> 1. IMPORTANT: "Step 2" is confusing because it seems to refer to the
> step 2 on line 3616.
> 
> 2. Change "move to internal slot" to "try again on the internal slot".
> This is because "move to" may be misinterpreted as moving a key to the
> internal slot.

Done.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3779: PORT_Assert(fms);
On 2015/08/20 02:10:33, wtc1 wrote:
> On 2015/08/20 00:44:14, ekr-webrtc wrote:
> > Perhaps we should have two asserts, one for rv == SECSuccess and one for fms
> > is non-null.
> 
> Assertions should be used to detect bugs. In theory, ssl3_GenerateFauxMS
> can fail if we run out of memory or some hardware crypto device failed.
> So I would not assert that ssl3_GenerateFauxMS always succeeds. But it
> makes sense to assert that if it succeeded, fms is non-null.
> 

Done.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3814: }
On 2015/08/20 00:18:03, wtc1 wrote:
> 
> IMPORTANT: delete these four lines (3811-3814) because
> pwSpec->master_secret cannot be NULL at this point in the new code.

Done.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3850: tls_DeriveExtendedMasterSecret(sslSocket *ss, PK11SymKey
*pms)
On 2015/08/20 02:10:33, wtc1 wrote:
> 
> On 2015/08/20 00:44:14, ekr-webrtc wrote:
> > IIRC there are small differences later as
> > well, but I will check and make the TODO rigt.
> 
> The only difference I spotted in the second half of these two functions
> is the PKCS#11 bypass code at the end. I believe that code is also
> correct for extended master secret.

Yes, this seems to be correct. I will add a TODO comment and file a bug.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3860: PRBool    isDH = (PRBool)
((ss->ssl3.hs.kea_def->exchKeyType == kt_dh) ||
On 2015/08/20 00:18:02, wtc1 wrote:
> 
> Nit: in this function you should get rid of all the unnecessary
> alignment of variable declarations or assignment operators (=).

Done.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3896: 
On 2015/08/20 00:18:02, wtc1 wrote:
> 
> Nit: delete a blank line.

Done.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3914: PORT_Assert(fms);
On 2015/08/20 00:18:02, wtc1 wrote:
> 
> IMPORTANT: this assertion should be after the if (rv != SECSuccess) check,
> right?

Done.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3919: /* At this point we have something in the fms slot which
we in
On 2015/08/20 00:18:03, wtc1 wrote:
> 
> 1. Nit: "the fms slot" seems wrong because "fms" is not a PK11SlotInfo.
> 
> 2. This comment should also be added to ssl3_DeriveMasterSecret.

Done.

[wtc1, 2015-08-20 18:18:22]

Review comments on patch set 2:

Eric, I didn't review your new patch sets, but I reviewed
ssl3con.c in patch set 2 for 1.5 hours, and I believe I
fully understand the server-side RSA premaster secret and
master secret logic now.

I found one problem with your change:

  If the PKCS #1 RSA decryption fails, with your change,
  we will generate two faux premaster secrets.

This is at least wasteful, but I wonder if it also
introduces a timing difference.

I also believe we can implement your variant of the
second algorithm of RFC 5246 Section 7.4.7.1 properly.
This requires generating a faux MS in
ssl3_HandleRSAClientKeyExchange (instead of in
ssl3_DeriveMasterSecret), and passing the faux MS to
ssl3_InitPendingCipherSpec and ssl3_DeriveMasterSecret.

It may be better to let the current CL settle, and
make this change separately.

We should strike while the iron is hot. I will devote
some time to this next week.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:2186: * For the bypass case,  pms is NULL.

IMPORTANT: the comments about a null |pms| on lines 2183 and 2186 should
be merged and updated.

I think it should read something like this:

  This code is common to the bypass and PKCS11 execution paths.
  For the bypass case,  pms is NULL. If the old master secret is
  reused, pms is NULL and the master secret is already in either
  pwSpec->msItem.len (the bypass case) or pwSpec->master_secret.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:2204: if (pms || (!pwSpec->msItem.len &&
!pwSpec->master_secret)) {

When we understand the code, we should add comments here to document
exactly what these two conditions mean.

I believe we can just test
    if (!pwSpec->msItem.len && !pwSpec->master_secret) {
here.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3608: PK11SlotInfo *slot;

On 2015/08/20 17:55:07, ekr-webrtc wrote:
> 
> Re-reading this: we don't presently set them to null after freeing them. I've
> moved these to block scope, but I'm concerned that this comment indicates a
> logic error that I missed. Can you elaborate.

Don't worry, there is no logic error. The reason I commented on
this is that either suggestion (setting the pointer to null after
freeing, or declaring the pointers as local variables inside the
if blocks) would make it easier to verify there is no logic error.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3617: *    faux PMS value. If that fails, generate an error.

I traced the PKCS #11 bypass code. I found a comment that described the
code, and I verified the comment is accurate:

https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl3con.c...

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:6837: /* NULL value for PMS signifies re-use of the old MS */

This comment needs to be updated. It is true that we pass a NULL PMS
when we reuse the old MS, but what really signifies the old MS reuse
is a non-null pwSpec->master_secret (set on line 6784 or line 6808)
or a nonzero pwSpec->msItem.len (set on line 6800).

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:8544: /* NULL value for PMS signifies re-use of the old MS */

Same here: this comment needs to be updated. What really signifies
old MS reuse is a non-null pwSpec->master_secret or a nonzero
pwSpec->msItem.len, set on lines 8445, 8458, or 8468.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:9742: ssl3_HandleRSAClientKeyExchange(sslSocket *ss,

This function seems like a good place to document your variant of the
second algorithm in RFC 5246 Section 7.4.7.1 because this is where
the entire process starts. The downside is that the action is all in
the functions called by this function:

   ssl3_HandleRSAClientKeyExchange
   ssl3_InitPendingCipherSpec
   ssl3_DeriveMasterSecret or tls_DeriveExtendedMasterSecret
   ssl3_GenerateFauxMS
   
No matter where we decide to document the process, I think it is useful
to point out that the process spans across these functions.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:9820: rv = ssl3_MasterKeyDeriveBypass(pwSpec, cr, sr,
&pmsItem, isTLS,

IMPORTANT: we should rename ssl3_MasterKeyDeriveBypass to
ssl3_DeriveMasterSecretBypass. I searched for "MasterSecret" and
couldn't find this function :-(

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:9827: rv = ssl3_InitPendingCipherSpec(ss,  NULL);

Note that we also pass pms=NULL to ssl3_InitPendingCipherSpec in the
bypass mode. In this case we have a nonzero pwSpec->msItem.len.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:9841: CKM_SSL3_MASTER_KEY_DERIVE, CKA_DERIVE, 0);

IMPORTANT: I think I finally understood why you can't do algorithm 2 of
RFC 5246 Section 7.4.7.1!

Ideally, we should have a PKCS #11 mechanism to unwrap an RSA premaster
secret and extract the embedded client_version. This would also eliminate
the need to have an RSA and a DH variant of master secret derivation.

Perhaps such an unwrap mechanism is considered dangerous because it
would output two bytes of the unwrapped secret key material. But I
think the master key derivation mechanism has the same vulnerability.

I think this is why you need to generate a faux MS, rather than just
a faux PMS, right? This is more expensive...

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:9851: pms = ssl3_GenerateRSAPMS(ss, ss->ssl3.prSpec, slot);

IMPORTANT: If the RSA PMS unwrap on line 9840 fails, we generate a faux
PMS here and pass it to ssl3_InitPendingCipherSpec. Therefore, in this
case we will end up generating two faux PMSs with your change! This seems
bad.

[ekr-rietveld, 2015-08-20 18:46:04]

You are correct that if there is a bogus EPMS, we will generate two Faux MSs. I
will respond separately in e-mail with an analysis of the impact of this.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:2186: * For the bypass case,  pms is NULL.
On 2015/08/20 18:18:22, wtc1 wrote:
> 
> IMPORTANT: the comments about a null |pms| on lines 2183 and 2186 should
> be merged and updated.
> 
> I think it should read something like this:
> 
>   This code is common to the bypass and PKCS11 execution paths.
>   For the bypass case,  pms is NULL. If the old master secret is
>   reused, pms is NULL and the master secret is already in either
>   pwSpec->msItem.len (the bypass case) or pwSpec->master_secret.

This sounds right. I will change.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:6837: /* NULL value for PMS signifies re-use of the old MS */
On 2015/08/20 18:18:22, wtc1 wrote:
> 
> This comment needs to be updated. It is true that we pass a NULL PMS
> when we reuse the old MS, but what really signifies the old MS reuse
> is a non-null pwSpec->master_secret (set on line 6784 or line 6808)
> or a nonzero pwSpec->msItem.len (set on line 6800).

Thanks. I will try to update.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:9742: ssl3_HandleRSAClientKeyExchange(sslSocket *ss,
On 2015/08/20 18:18:22, wtc1 wrote:
> 
> This function seems like a good place to document your variant of the
> second algorithm in RFC 5246 Section 7.4.7.1 because this is where
> the entire process starts. The downside is that the action is all in
> the functions called by this function:
> 
>    ssl3_HandleRSAClientKeyExchange
>    ssl3_InitPendingCipherSpec
>    ssl3_DeriveMasterSecret or tls_DeriveExtendedMasterSecret
>    ssl3_GenerateFauxMS
>    
> No matter where we decide to document the process, I think it is useful
> to point out that the process spans across these functions.

OK. I can move the comment.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:9820: rv = ssl3_MasterKeyDeriveBypass(pwSpec, cr, sr,
&pmsItem, isTLS,
On 2015/08/20 18:18:22, wtc1 wrote:
> 
> IMPORTANT: we should rename ssl3_MasterKeyDeriveBypass to
> ssl3_DeriveMasterSecretBypass. I searched for "MasterSecret" and
> couldn't find this function :-(

I can do this.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:9841: CKM_SSL3_MASTER_KEY_DERIVE, CKA_DERIVE, 0);
On 2015/08/20 18:18:22, wtc1 wrote:
> 
> IMPORTANT: I think I finally understood why you can't do algorithm 2 of
> RFC 5246 Section 7.4.7.1!
> 
> Ideally, we should have a PKCS #11 mechanism to unwrap an RSA premaster
> secret and extract the embedded client_version. This would also eliminate
> the need to have an RSA and a DH variant of master secret derivation.

Yes, that seems like it would be ideal.


> Perhaps such an unwrap mechanism is considered dangerous because it
> would output two bytes of the unwrapped secret key material. But I
> think the master key derivation mechanism has the same vulnerability.

I tend to agree. I was just trying to avoid excessive changes to the
PKCS#11 layer and to the structure of this code.

 
> I think this is why you need to generate a faux MS, rather than just
> a faux PMS, right? This is more expensive...

Yes, that seems correct to me.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:9851: pms = ssl3_GenerateRSAPMS(ss, ss->ssl3.prSpec, slot);
On 2015/08/20 18:18:22, wtc1 wrote:
> 
> IMPORTANT: If the RSA PMS unwrap on line 9840 fails, we generate a faux
> PMS here and pass it to ssl3_InitPendingCipherSpec. Therefore, in this
> case we will end up generating two faux PMSs with your change! This seems
> bad.

You are correct that this is the impact of my change.

[ekr-rietveld, 2015-08-20 19:24:12]

After some further thought, I think it would be best to refactor the code to
implement the entire 5246 algorithm before we land it. I will upload a new patch
with that change when I have it.

[ekr-rietveld, 2015-08-25 18:04:33]

Self-review. Mostly nits except for the questions about the msitem tests.

https://codereview.appspot.com/259430043/diff/80001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/ssl_loopback_unittest.cc (right):

https://codereview.appspot.com/259430043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:627:
EXPECT_EQ(kTlsAlertHandshakeFailure, alert_recorder->description());
Check alert level.

https://codereview.appspot.com/259430043/diff/80001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/tls_connect.h (right):

https://codereview.appspot.com/259430043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_connect.h:76: void
CheckResumption(SessionResumptionMode expected);
Verify that these need to be protected

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (left):

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#oldcode...
lib/ssl/ssl3con.c:4591: 
Double check that these should go away

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#oldcode...
lib/ssl/ssl3con.c:4613: }
And this

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#oldcode...
lib/ssl/ssl3con.c:4741: }
And these.

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3596: **
Add a comment about |msp|

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3603: CK_MECHANISM_TYPE master_derive,
Alignment with (

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3605: CK_VERSION* pms_version,
* is aligned with pms_version

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3764: key_derive = CKM_TLS12_KEY_AND_MAC_DERIVE;
Spaces, not tabs here and below.

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3784: /* Wrapper method to compute the master secret and and
Extra and

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3787: ** Called from: ssl3_ComputeMasterSecret
s/://

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3810: ** Called from ssl3_InitPendingCipherSpec.   prSpec is
pwSpec.
REflow

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3840: }
spaces, not tabs.

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3855: 
Extra space.

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:6628: *    extension, the client MUST abort the handshake.
This is the wrong text.

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:9821: PK11_FreeSymKey(currentPms);
Spaces not tabs.

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3ext.c
File lib/ssl/ssl3ext.c (right):

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:1296: 
Extra whitespace.

[ekr-rietveld, 2015-08-25 18:19:44]

MT, I need to double-check the removal of some of the msitem checks (which I
believe stem from WTC's review). Otherwise, this is ready for final review by
you. PTAL.

https://codereview.appspot.com/259430043/diff/80001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/tls_connect.h (right):

https://codereview.appspot.com/259430043/diff/80001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_connect.h:76: void
CheckResumption(SessionResumptionMode expected);
On 2015/08/25 18:04:32, ekr-webrtc wrote:
> Verify that these need to be protected

Done.

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3596: **
On 2015/08/25 18:04:32, ekr-webrtc wrote:
> Add a comment about |msp|

Done.

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3603: CK_MECHANISM_TYPE master_derive,
On 2015/08/25 18:04:32, ekr-webrtc wrote:
> Alignment with (

Done.

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3605: CK_VERSION* pms_version,
On 2015/08/25 18:04:32, ekr-webrtc wrote:
> * is aligned with pms_version

Done.

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3764: key_derive = CKM_TLS12_KEY_AND_MAC_DERIVE;
On 2015/08/25 18:04:32, ekr-webrtc wrote:
> Spaces, not tabs here and below.

Done.

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3784: /* Wrapper method to compute the master secret and and
On 2015/08/25 18:04:32, ekr-webrtc wrote:
> Extra and

Done.

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3787: ** Called from: ssl3_ComputeMasterSecret
On 2015/08/25 18:04:32, ekr-webrtc wrote:
> s/://

Done.

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3810: ** Called from ssl3_InitPendingCipherSpec.   prSpec is
pwSpec.
On 2015/08/25 18:04:32, ekr-webrtc wrote:
> REflow

Done.

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3840: }
On 2015/08/25 18:04:32, ekr-webrtc wrote:
> spaces, not tabs.

Done.

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3855: 
On 2015/08/25 18:04:32, ekr-webrtc wrote:
> Extra space.

Leave it

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:6628: *    extension, the client MUST abort the handshake.
On 2015/08/25 18:04:32, ekr-webrtc wrote:
> This is the wrong text.

Done.

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:9821: PK11_FreeSymKey(currentPms);
On 2015/08/25 18:04:32, ekr-webrtc wrote:
> Spaces not tabs.

Done.

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3ext.c
File lib/ssl/ssl3ext.c (right):

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:1296: 
On 2015/08/25 18:04:33, ekr-webrtc wrote:
> Extra whitespace.

Done.

[ekr-rietveld, 2015-08-25 18:27:44]

MT. I just realized that I did not incorporate WTC's editorial-type comments
from
"Review comments on patch set 2". I will do those now.

[ekr-rietveld, 2015-08-25 18:38:53]

Done in an outstanding patch

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:2186: * For the bypass case,  pms is NULL.
On 2015/08/20 18:46:04, ekr-webrtc wrote:
> On 2015/08/20 18:18:22, wtc1 wrote:
> > 
> > IMPORTANT: the comments about a null |pms| on lines 2183 and 2186 should
> > be merged and updated.
> > 
> > I think it should read something like this:
> > 
> >   This code is common to the bypass and PKCS11 execution paths.
> >   For the bypass case,  pms is NULL. If the old master secret is
> >   reused, pms is NULL and the master secret is already in either
> >   pwSpec->msItem.len (the bypass case) or pwSpec->master_secret.
> 
> This sounds right. I will change.

Done.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:2204: if (pms || (!pwSpec->msItem.len &&
!pwSpec->master_secret)) {
On 2015/08/20 18:18:22, wtc1 wrote:
> 
> When we understand the code, we should add comments here to document
> exactly what these two conditions mean.
> 
> I believe we can just test
>     if (!pwSpec->msItem.len && !pwSpec->master_secret) {
> here.

I am leaving this here for now.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:8544: /* NULL value for PMS signifies re-use of the old MS */
On 2015/08/20 18:18:22, wtc1 wrote:
> 
> Same here: this comment needs to be updated. What really signifies
> old MS reuse is a non-null pwSpec->master_secret or a nonzero
> pwSpec->msItem.len, set on lines 8445, 8458, or 8468.

Done.

https://codereview.appspot.com/259430043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:9820: rv = ssl3_MasterKeyDeriveBypass(pwSpec, cr, sr,
&pmsItem, isTLS,
On 2015/08/20 18:46:04, ekr-webrtc wrote:
> On 2015/08/20 18:18:22, wtc1 wrote:
> > 
> > IMPORTANT: we should rename ssl3_MasterKeyDeriveBypass to
> > ssl3_DeriveMasterSecretBypass. I searched for "MasterSecret" and
> > couldn't find this function :-(
> 
> I can do this.

Done.

[mt, 2015-08-25 18:41:51]

OK, well, I realize that I was jumping the gun somewhat, but I hardly thing that
Wan-Teh's comments will interfere with this.

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3708: master_params_len =
sizeof(CK_SSL3_MASTER_KEY_DERIVE_PARAMS);
On this branch, the value of master_params.prfHashMechanism is uninitialized. 
Even though it isn't read (I think), we should probably set it to something. 
Except that after looking I see that there is no "undefined" value there.  Maybe
just set it to SHA256 unconditionally.

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3795: PORT_Assert( ss->ssl3.prSpec == ss->ssl3.pwSpec);
Extra space after first paren.

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:4716: }
This is good.

(Damn I wish that we could delete all the SSLv2 and SSLv3 code.  That and export
suites...)

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:4849: if (!spec->master_secret) {
This is good.

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:6116: rv = ssl3_InitPendingCipherSpec(ss,  pms);
extra whitespace after comma

(You touch it, you own it.)

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:6117: PK11_FreeSymKey(pms); pms = NULL;
new line after semicolon

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:6208: rv = ssl3_InitPendingCipherSpec(ss,  pms);
extra whitespace after comma

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:6209: PK11_FreeSymKey(pms); pms = NULL;
new line after semicolon

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:9745: #define fauxPms    tmpPms[0]
Do you want to #undef these at the bottom?

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:9788: if (fauxPms == NULL) {
juberti: untabify

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:9800: CKM_SSL3_MASTER_KEY_DERIVE, CKA_DERIVE, 0);
align

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:9801: /* Temporarily use the PMS if the unwrap fails */
Fix comment:  /* Use the faux PMS if unwrapping the real PMS fails. */

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:9806: * then use the fms in place of the PMS. Note, that this
Fix comment: ... we then use the faux PMS in place of the PMS.
Also, remove comma after "Note"

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:9821: PK11_FreeSymKey(currentPms);
On 2015/08/25 18:19:43, ekr-webrtc wrote:
> On 2015/08/25 18:04:32, ekr-webrtc wrote:
> > Spaces not tabs.
> 
> Done.

I'm seeing tabs still.  Also, an extra space after the comma.

[ekr-rietveld, 2015-08-25 19:20:53]

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3708: master_params_len =
sizeof(CK_SSL3_MASTER_KEY_DERIVE_PARAMS);
On 2015/08/25 18:41:51, mt wrote:
> On this branch, the value of master_params.prfHashMechanism is uninitialized. 
> Even though it isn't read (I think), we should probably set it to something. 
> Except that after looking I see that there is no "undefined" value there. 
Maybe
> just set it to SHA256 unconditionally.

This is the way the previous code was (see line 3653). I suggest a comment
instead.

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3795: PORT_Assert( ss->ssl3.prSpec == ss->ssl3.pwSpec);
On 2015/08/25 18:41:50, mt wrote:
> Extra space after first paren.

Done.

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:6116: rv = ssl3_InitPendingCipherSpec(ss,  pms);
On 2015/08/25 18:41:50, mt wrote:
> extra whitespace after comma
> 
> (You touch it, you own it.)

Done.

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:6117: PK11_FreeSymKey(pms); pms = NULL;
On 2015/08/25 18:41:50, mt wrote:
> new line after semicolon

Done.

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:6208: rv = ssl3_InitPendingCipherSpec(ss,  pms);
On 2015/08/25 18:41:51, mt wrote:
> extra whitespace after comma

Done.

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:6209: PK11_FreeSymKey(pms); pms = NULL;
On 2015/08/25 18:41:50, mt wrote:
> new line after semicolon

Done.

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:9745: #define fauxPms    tmpPms[0]
On 2015/08/25 18:41:50, mt wrote:
> Do you want to #undef these at the bottom?

Done.

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:9788: if (fauxPms == NULL) {
On 2015/08/25 18:41:51, mt wrote:
> juberti: untabify

Done.

https://codereview.appspot.com/259430043/diff/80001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:9806: * then use the fms in place of the PMS. Note, that this
On 2015/08/25 18:41:51, mt wrote:
> Fix comment: ... we then use the faux PMS in place of the PMS.
> Also, remove comma after "Note"

Done.

[ekr-rietveld, 2015-08-25 19:32:31]

Updated.

[ekr-rietveld, 2015-08-25 20:28:58]

MT, I think we are ready to land. LGTY?

[ekr-rietveld, 2015-08-25 20:32:43]

On 2015/08/25 20:28:58, ekr-webrtc wrote:
> MT, I think we are ready to land. LGTY?

Wait, it didn't show up...

[ekr-rietveld, 2015-08-25 20:36:05]

OK, for real

[mt, 2015-08-25 20:43:38]

LGTM