Revision of session hash patch. Needs first review.

Revision of session hash patch. Needs first review.

[ekr-rietveld, 2015-03-04 21:22:33]

Here is a version that I think is ready for a first review.

[ekr-rietveld, 2015-03-04 21:24:12]

Note. This needs Richard Barnes's patch #1.

[mt, 2015-03-04 21:26:19]

LGTM

(not)

[ekr-rietveld, 2015-03-04 21:26:34]

On 2015/03/04 21:26:19, mt wrote:
> LGTM
> 
> (not)

I guess we know it comes up green now.

[mt, 2015-03-04 22:12:14]

I've reviewed the tests and those look good.  I'll have a look at the other
stuff later.  My mind is not yet prepared for the horror.

https://codereview.appspot.com/211900043/diff/1/cmd/tstclnt/tstclnt.c
File cmd/tstclnt/tstclnt.c (right):

https://codereview.appspot.com/211900043/diff/1/cmd/tstclnt/tstclnt.c#newcode136
cmd/tstclnt/tstclnt.c:136: channel.extendedMasterSecretUsed ? "Yes": "No");
Code duplication ftw.  This is the same set of changes made to selfserv.c

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

https://codereview.appspot.com/211900043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:242:
TEST_P(TlsConnectGenericSingleVersion, ConnectExtendedMasterSecret) {
Is the intent to resume on every one of these?  Maybe add a simple connect case
without resumption would be a good idea.

https://codereview.appspot.com/211900043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:244:
server_->EnableExtendedMasterSecret();
Maybe add a function to roll these up together, like with a lot of the others. 
There are only a few places where you call just one.

https://codereview.appspot.com/211900043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:333:
INSTANTIATE_TEST_CASE_P(VersionsStream, TlsConnectGenericSingleVersion,
Since this is just TLS 1.0, probably call it that.

https://codereview.appspot.com/211900043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:341:
SSL_LIBRARY_VERSION_TLS_1_2)));
Why not an array?

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

https://codereview.appspot.com/211900043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_agent.cc:178: << " expected=" << expected <<
std::endl;
Might drop the printout now.  The assert is fine.

[mt, 2015-03-04 22:47:16]

I'm not sure about some of the code shuffling that is going on and the key
derivation is abominable and could almost certainly be made better.

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

https://codereview.appspot.com/211900043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:303:
ConnectExtendedMasterSecretResumeWithout) {
You don't test the two cases where a server that does not support the extended
master secret, but the client attempts to resume a session that had it
previously.  I can see how you might think that that case is simply too
pathological to test?  I think that the two cases should be tested anyway so
that we know what to expect.

https://codereview.appspot.com/211900043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:311: ConnectExpectFail();
I think that it might be a good idea to capture the alert that the server
generates in all the error cases.

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

https://codereview.appspot.com/211900043/diff/1/lib/ssl/ssl3con.c#newcode3568
lib/ssl/ssl3con.c:3568: ssl3_DeriveMasterSecret(sslSocket *ss, PK11SymKey *pms)
Just as I expected, this is horrific.  I didn't count all the branches, the but
the cyclomatic complexity is something like 40.  That means you need 40 test
cases just to test this one function.  I didn't see anywhere like that many so
far, which means that there are lots of holes.

Is there any way that this could be simplified?  Even this would be better:

SECStatus ssl3_DeriveMasterSecret(sslSocket *ss, PK11SymKey *pms) {
  if (ssl3_ExtensionNegotiated(ss, ssl_extended_master_secret_xtn)) {
    return ssl3_DeriveExtendedMasterSecret(ss, pms);
  }

  ...
}

No doubt you would find some duplicated code, in which case, that's easy to fix.

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

https://codereview.appspot.com/211900043/diff/1/lib/ssl/ssl3ecc.c#newcode343
lib/ssl/ssl3ecc.c:343: }
You have moved a lot of these, can you explain why?  Is this necessary for the
change, or just good hygiene?

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

https://codereview.appspot.com/211900043/diff/1/lib/ssl/ssl3ext.c#newcode1160
lib/ssl/ssl3ext.c:1160: + sizeof(ticket.ticket_lifetime_hint);
My eyes!

https://codereview.appspot.com/211900043/diff/1/lib/ssl/ssl3ext.c#newcode2603
lib/ssl/ssl3ext.c:2603: bug 753136? */
SECFailure is fine. 

In fact, might as well use ssl3_DecodeError(ss) right now.  It's not like we
haven't already started sending alerts on decode errors without failing the
handshake in the past.

[ekr-rietveld, 2015-03-04 23:01:33]

MT, WTC, why don't oyu hold on till I try the refactor MT suggests.

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

https://codereview.appspot.com/211900043/diff/1/lib/ssl/ssl3con.c#newcode3568
lib/ssl/ssl3con.c:3568: ssl3_DeriveMasterSecret(sslSocket *ss, PK11SymKey *pms)
On 2015/03/04 22:47:15, mt wrote:
> Just as I expected, this is horrific.  I didn't count all the branches, the
but
> the cyclomatic complexity is something like 40.  That means you need 40 test
> cases just to test this one function.  I didn't see anywhere like that many so
> far, which means that there are lots of holes.
> 
> Is there any way that this could be simplified?  Even this would be better:
> 
> SECStatus ssl3_DeriveMasterSecret(sslSocket *ss, PK11SymKey *pms) {
>   if (ssl3_ExtensionNegotiated(ss, ssl_extended_master_secret_xtn)) {
>     return ssl3_DeriveExtendedMasterSecret(ss, pms);
>   }
> 
>   ...
> }
> 
> No doubt you would find some duplicated code, in which case, that's easy to
fix.

Sure, I can give this a try. At least we can see how it looks.

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

https://codereview.appspot.com/211900043/diff/1/lib/ssl/ssl3ecc.c#newcode343
lib/ssl/ssl3ecc.c:343: }
On 2015/03/04 22:47:15, mt wrote:
> You have moved a lot of these, can you explain why?  Is this necessary for the
> change, or just good hygiene?

Yes, it's necessary. The issue is that the session hashes are updated as a side
effect of the ssl3_AppendHandshake functions
but ssl3_InitPendingCipherSpec computes the MS. This means that you
need to call it after the handshake messages have been marshalled.

[wtc1, 2015-04-07 18:45:21]

Hi Eric,

I forgot to review this CL. Sorry. What's the status of the CL?

[ekr-rietveld, 2015-06-03 18:43:05]

self-review

https://codereview.appspot.com/211900043/diff/1/cmd/tstclnt/tstclnt.c
File cmd/tstclnt/tstclnt.c (right):

https://codereview.appspot.com/211900043/diff/1/cmd/tstclnt/tstclnt.c#newcode136
cmd/tstclnt/tstclnt.c:136: channel.extendedMasterSecretUsed ? "Yes": "No");
On 2015/03/04 22:12:14, mt wrote:
> Code duplication ftw.  This is the same set of changes made to selfserv.c

Acknowledged.

https://codereview.appspot.com/211900043/diff/20001/cmd/selfserv/selfserv.c
File cmd/selfserv/selfserv.c (right):

https://codereview.appspot.com/211900043/diff/20001/cmd/selfserv/selfserv.c#n...
cmd/selfserv/selfserv.c:2192:
"2:A:BC:DEGL:M:NP:RST:V:Ya:bc:d:e:f:g:hi:jk:lmn:op:qrst:uvw:xyz");
Remove S, which we are not using

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

https://codereview.appspot.com/211900043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:18: uint8_t
kCannedClientKeyExchange[] = {
There's no point in having canned since it ties to some RSA key which is now
gone. Let's just do a bogus one which is the same character repeating.

https://codereview.appspot.com/211900043/diff/20001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/tls_connect.cc (right):

https://codereview.appspot.com/211900043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_connect.cc:233: }
Indent.

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

https://codereview.appspot.com/211900043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3772: 
Extra whistepace.

https://codereview.appspot.com/211900043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3791: /* TODO(ekr@rtfm.com): Update when there is a DH
version. */
Remove this TODO.

https://codereview.appspot.com/211900043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3791: /* TODO(ekr@rtfm.com): Update when there is a DH
version. */
Remove this TODO.

https://codereview.appspot.com/211900043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3844: 
Extra whitespace

https://codereview.appspot.com/211900043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:9491: /* TRIPLE BYPASS, get PMS directly from RSA decryption.
We never negotiate EMS when bypass is on, so just assert here.

[ekr-rietveld, 2015-06-03 20:10:43]

MT can you please take a look at this patch.

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

https://codereview.appspot.com/211900043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:18: uint8_t
kCannedClientKeyExchange[] = {
On 2015/06/03 18:43:05, ekr-webrtc wrote:
> There's no point in having canned since it ties to some RSA key which is now
> gone. Let's just do a bogus one which is the same character repeating.

Done.

https://codereview.appspot.com/211900043/diff/20001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/tls_connect.cc (right):

https://codereview.appspot.com/211900043/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_connect.cc:233: }
On 2015/06/03 18:43:05, ekr-webrtc wrote:
> Indent.

Done.

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

https://codereview.appspot.com/211900043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3772: 
On 2015/06/03 18:43:05, ekr-webrtc wrote:
> Extra whistepace.

Done.

https://codereview.appspot.com/211900043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3791: /* TODO(ekr@rtfm.com): Update when there is a DH
version. */
On 2015/06/03 18:43:05, ekr-webrtc wrote:
> Remove this TODO.

Done.

https://codereview.appspot.com/211900043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3844: 
On 2015/06/03 18:43:05, ekr-webrtc wrote:
> Extra whitespace

Done.

https://codereview.appspot.com/211900043/diff/20001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:9491: /* TRIPLE BYPASS, get PMS directly from RSA decryption.
On 2015/06/03 18:43:05, ekr-webrtc wrote:
> We never negotiate EMS when bypass is on, so just assert here.

Done.

[mt, 2015-06-12 00:16:04]

I'm not sure what the base is here, but I think that the diff I reviewed is a
little off.

I don't see any major issues here, but am sad about the code duplication in the
MS derivation piece.

https://codereview.appspot.com/211900043/diff/40001/cmd/selfserv/selfserv.c
File cmd/selfserv/selfserv.c (right):

https://codereview.appspot.com/211900043/diff/40001/cmd/selfserv/selfserv.c#n...
cmd/selfserv/selfserv.c:2207: case 'G': enableExtendedMasterSecret = PR_TRUE;
break;
It might make sense to have this with an on/off switch rather than just an "on"
switch.

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

https://codereview.appspot.com/211900043/diff/40001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:297:
ExpectExtendedMasterSecret(true);
Maybe you can add a single call for these three calls.

https://codereview.appspot.com/211900043/diff/40001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:386:
ConnectNormalResumeWithExtendedMasterSecret) {
Do you have to include a call to ConfigureSessionCache in this function?

https://codereview.appspot.com/211900043/diff/40001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/tls_agent.cc (right):

https://codereview.appspot.com/211900043/diff/40001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_agent.cc:190: << " expected=" << expected <<
std::endl;
The cerr call probably isn't needed now that this is working.

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

https://codereview.appspot.com/211900043/diff/40001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_connect.h:68: void CheckExtendedMasterSecret();
Do you need this function to be protected?

https://codereview.appspot.com/211900043/diff/40001/lib/softoken/pkcs11c.c
File lib/softoken/pkcs11c.c (right):

https://codereview.appspot.com/211900043/diff/40001/lib/softoken/pkcs11c.c#ne...
lib/softoken/pkcs11c.c:5804: CK_OBJECT_HANDLE_PTR phKey)
This function starts at line 5800 and ends at line 7162.  I would like to take
my clean code book and give it to the person that thought that this was OK. 
Forcibly.  It's as though they never even heard of functional decomposition.

This looks like it *might* be doing the right thing, but I don't plan on
spending the effort required to determine whether that it true.  I have other
things to do.

https://codereview.appspot.com/211900043/diff/40001/lib/softoken/pkcs11c.c#ne...
lib/softoken/pkcs11c.c:5939: att2 = sftk_FindAttribute(sourceKey,CKA_KEY_TYPE);
s/,/, /

https://codereview.appspot.com/211900043/diff/40001/lib/softoken/pkcs11c.c#ne...
lib/softoken/pkcs11c.c:5971: if (tls_prf_params-> prfMechanism == CKM_TLS_PRF) {
s/> />/

https://codereview.appspot.com/211900043/diff/40001/lib/softoken/pkcs11c.c#ne...
lib/softoken/pkcs11c.c:5975: switch (tls_prf_params->prfMechanism) {
Fix indent.

https://codereview.appspot.com/211900043/diff/40001/lib/softoken/pkcs11c.c#ne...
lib/softoken/pkcs11c.c:6019: crv = sftk_forceAttribute(key,CKA_DERIVE, 
&cktrue,sizeof(CK_BBOOL));
Check formatting.

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl.h
File lib/ssl/ssl.h (right):

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl.h#newcode194
lib/ssl/ssl.h:194: /* Use draft-ietf-tls-session-hash; disabled by default. */
I don't like having a non-safe default.  Why?

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

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#oldcode...
lib/ssl/ssl3con.c:8875: 
Rebase.

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

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode1
lib/ssl/ssl3con.c:1: 
Unnecessary change?

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3573: 
Extra line?

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3591: +            ssl3_ExtensionNegotiated(ss,
ssl_extended_master_secret_xtn);
Extraneous '+'.

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3750: PRBool isTLS12=
s/2=/2 =/

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3754: unsigned char extended_label[] = "extended master
secret";
Does this need to be on the stack like this?

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3788: }
Take another look at this if statement.

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3799: keyFlags      = CKF_SIGN | CKF_VERIFY;
Const this.

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3817: if (pms != NULL) {
These tests aren't consistent.  if (pms) or if (pms != NULL)

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3822: ssl3_DeriveMasterSecret.
Why not refactor in reality?  I know that copy-and-paste is the NSS way, but
it's a bad way.  I'm seeing a lot of overlap here, all the way to the bottom of
this function.

If bypass mode loses as a result, I'm not going to be sad, if that's the
concern.

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:4878: }
This code now appears in all four branches of this function.  Hoist it right up.

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:5590: rv = SSL3_SendAlert(ss, alert_warning,
no_renegotiation);
Put the (void) back I think.

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:6644: ssl3_ExtensionNegotiated(ss,
ssl_extended_master_secret_xtn)) {
Are you sure that this code path can be hit?  I believe that we check if we sent
an extension before accepting it from the server anyway.

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

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:6658: handshakes.
We should have a bug number for (optionally) disabling resumption for !EMS.  I
can't imagine wanting to land it for a while yet, but nevertheless it would be
good to have a record here.

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:6824: ssl3_BigIntGreaterThanOne(const SECItem* mpint) {
Restoring this function means that you aren't rebased properly...

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:6978: if (dh_p.len < 512/8) {
hmm, it's rebase time.

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:8297: break;
This code is seriously messed up.  I first went up almost 150 lines to find what
this breaks out of.  Then I noticed that it's much closer than that.  I'd almost
prefer a goto statement here.  No, I think that I would.

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:8400: 
ws

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

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3ext.c#newcode264
lib/ssl/ssl3ext.c:264: { ssl_extended_master_secret_xtn,      
&ssl3_HandleExtendedMasterSecretXtn },
I wouldn't bother with alignment.

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:2596: if (ss->opt.bypassPKCS11) {
Did you want to #ifdef this out?  Do you have to?

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:2617: return 0;
I'd move this up and avoid checking maxBytes against extension_length twice. 
BTW, mixing camel case and underscores is not-awesome.

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:2632: return 0;
SECSuccess

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:2636: return 0;
SECSuccess

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/sslsock.c
File lib/ssl/sslsock.c (right):

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/sslsock.c#newcode86
lib/ssl/sslsock.c:86: PR_FALSE    /* enableEtendedMS    */
PR_TRUE and check spelling

[ekr-rietveld, 2015-06-12 13:36:08]

If you'd really like that refactor, we should talk 1:1.

https://codereview.appspot.com/211900043/diff/40001/cmd/selfserv/selfserv.c
File cmd/selfserv/selfserv.c (right):

https://codereview.appspot.com/211900043/diff/40001/cmd/selfserv/selfserv.c#n...
cmd/selfserv/selfserv.c:2207: case 'G': enableExtendedMasterSecret = PR_TRUE;
break;
On 2015/06/12 00:16:02, mt wrote:
> It might make sense to have this with an on/off switch rather than just an
"on"
> switch.

That's how this code rolls.

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

https://codereview.appspot.com/211900043/diff/40001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:297:
ExpectExtendedMasterSecret(true);
On 2015/06/12 00:16:02, mt wrote:
> Maybe you can add a single call for these three calls.

Sure.

https://codereview.appspot.com/211900043/diff/40001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:386:
ConnectNormalResumeWithExtendedMasterSecret) {
On 2015/06/12 00:16:02, mt wrote:
> Do you have to include a call to ConfigureSessionCache in this function?

Will check.

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

https://codereview.appspot.com/211900043/diff/40001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_connect.h:68: void CheckExtendedMasterSecret();
On 2015/06/12 00:16:03, mt wrote:
> Do you need this function to be protected?

As opposed to private you mean?

https://codereview.appspot.com/211900043/diff/40001/lib/softoken/pkcs11c.c
File lib/softoken/pkcs11c.c (right):

https://codereview.appspot.com/211900043/diff/40001/lib/softoken/pkcs11c.c#ne...
lib/softoken/pkcs11c.c:5804: CK_OBJECT_HANDLE_PTR phKey)
On 2015/06/12 00:16:03, mt wrote:
> This function starts at line 5800 and ends at line 7162.  I would like to take
> my clean code book and give it to the person that thought that this was OK. 
> Forcibly.  It's as though they never even heard of functional decomposition.
> 
> This looks like it *might* be doing the right thing, but I don't plan on
> spending the effort required to determine whether that it true.  I have other
> things to do.

Luckily, you can ignore this part of the patch. It's richards and is a Bob
Relyea review.

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl.h
File lib/ssl/ssl.h (right):

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl.h#newcode194
lib/ssl/ssl.h:194: /* Use draft-ietf-tls-session-hash; disabled by default. */
On 2015/06/12 00:16:03, mt wrote:
> I don't like having a non-safe default.  Why?

I'm trying to avoid changing the ABI defaults. Let's take this up with bob.

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

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode1
lib/ssl/ssl3con.c:1: 
On 2015/06/12 00:16:03, mt wrote:
> Unnecessary change?

Acknowledged.

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3591: +            ssl3_ExtensionNegotiated(ss,
ssl_extended_master_secret_xtn);
On 2015/06/12 00:16:04, mt wrote:
> Extraneous '+'.

Merge error.

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3754: unsigned char extended_label[] = "extended master
secret";
On 2015/06/12 00:16:03, mt wrote:
> Does this need to be on the stack like this?

None.

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3788: }
On 2015/06/12 00:16:04, mt wrote:
> Take another look at this if statement.

That's why it has a TODO there. It's waiting for Richard to update his patch and
I wanted to have a marker.

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3817: if (pms != NULL) {
On 2015/06/12 00:16:04, mt wrote:
> These tests aren't consistent.  if (pms) or if (pms != NULL)

Yeah, agreed.

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3822: ssl3_DeriveMasterSecret.
On 2015/06/12 00:16:03, mt wrote:
> Why not refactor in reality?  I know that copy-and-paste is the NSS way, but
> it's a bad way.  I'm seeing a lot of overlap here, all the way to the bottom
of
> this function.
> 
> If bypass mode loses as a result, I'm not going to be sad, if that's the
> concern.

Because ssl3_DeriveMasterSecret is full of very complicated if statements (which
is what made you sad in the first place) and so I'm reluctant to hack and slash
it.

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:5590: rv = SSL3_SendAlert(ss, alert_warning,
no_renegotiation);
On 2015/06/12 00:16:04, mt wrote:
> Put the (void) back I think.

I don't think this is my change.

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:6644: ssl3_ExtensionNegotiated(ss,
ssl_extended_master_secret_xtn)) {
On 2015/06/12 00:16:03, mt wrote:
> Are you sure that this code path can be hit?  I believe that we check if we
sent
> an extension before accepting it from the server anyway.
> 
>
https://dxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl3ext.c...

I think that's correct, but I decided it was far enough removed from the check
that it would be good to have something here. Maybe also add an assert() to
enforce that.

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:6658: handshakes.
On 2015/06/12 00:16:03, mt wrote:
> We should have a bug number for (optionally) disabling resumption for !EMS.  I
> can't imagine wanting to land it for a while yet, but nevertheless it would be
> good to have a record here.

OK.

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:6824: ssl3_BigIntGreaterThanOne(const SECItem* mpint) {
On 2015/06/12 00:16:03, mt wrote:
> Restoring this function means that you aren't rebased properly...

OK.

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:8297: break;
On 2015/06/12 00:16:04, mt wrote:
> This code is seriously messed up.  I first went up almost 150 lines to find
what
> this breaks out of.  Then I noticed that it's much closer than that.  I'd
almost
> prefer a goto statement here.  No, I think that I would.

I don't disagree that this code would be better with gotos, but the fact that
all the surrounding code uses break seems to make that contraindicated.

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

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:2632: return 0;
On 2015/06/12 00:16:04, mt wrote:
> SECSuccess

Acknowledged.

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:2632: return 0;
Fix alignment.

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:2636: return 0;
On 2015/06/12 00:16:04, mt wrote:
> SECSuccess

Acknowledged.

[ekr-rietveld, 2015-06-22 22:41:55]

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

https://codereview.appspot.com/211900043/diff/40001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:297:
ExpectExtendedMasterSecret(true);
On 2015/06/12 13:36:07, ekr-webrtc wrote:
> On 2015/06/12 00:16:02, mt wrote:
> > Maybe you can add a single call for these three calls.
> 
> Sure.

Done.

https://codereview.appspot.com/211900043/diff/40001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/tls_agent.cc (right):

https://codereview.appspot.com/211900043/diff/40001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_agent.cc:190: << " expected=" << expected <<
std::endl;
On 2015/06/12 00:16:02, mt wrote:
> The cerr call probably isn't needed now that this is working.

Done.

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

https://codereview.appspot.com/211900043/diff/40001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/tls_connect.h:68: void CheckExtendedMasterSecret();
Made private.

On 2015/06/12 13:36:07, ekr-webrtc wrote:
> On 2015/06/12 00:16:03, mt wrote:
> > Do you need this function to be protected?
> 
> As opposed to private you mean?

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

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3573: 
On 2015/06/12 00:16:03, mt wrote:
> Extra line?

Done.

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3591: +            ssl3_ExtensionNegotiated(ss,
ssl_extended_master_secret_xtn);
On 2015/06/12 13:36:08, ekr-webrtc wrote:
> On 2015/06/12 00:16:04, mt wrote:
> > Extraneous '+'.
> 
> Merge error.

Done.

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3750: PRBool isTLS12=
On 2015/06/12 00:16:04, mt wrote:
> s/2=/2 =/

Done.

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3750: PRBool isTLS12=
On 2015/06/12 00:16:04, mt wrote:
> s/2=/2 =/

Done.

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3754: unsigned char extended_label[] = "extended master
secret";
On 2015/06/12 13:36:07, ekr-webrtc wrote:
> On 2015/06/12 00:16:03, mt wrote:
> > Does this need to be on the stack like this?
> 
> None.

Done.

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3799: keyFlags      = CKF_SIGN | CKF_VERIFY;
On 2015/06/12 00:16:04, mt wrote:
> Const this.

Done.

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:3817: if (pms != NULL) {
On 2015/06/12 13:36:07, ekr-webrtc wrote:
> On 2015/06/12 00:16:04, mt wrote:
> > These tests aren't consistent.  if (pms) or if (pms != NULL)
> 
> Yeah, agreed.

Done.

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:4878: }
On 2015/06/12 00:16:03, mt wrote:
> This code now appears in all four branches of this function.  Hoist it right
up.

It's actually two different variants and I think it's easier as-is.

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3con.c#newcode...
lib/ssl/ssl3con.c:6658: handshakes.
On 2015/06/12 00:16:03, mt wrote:
> We should have a bug number for (optionally) disabling resumption for !EMS.  I
> can't imagine wanting to land it for a while yet, but nevertheless it would be
> good to have a record here.

Done.

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

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/ssl3ext.c#newcode...
lib/ssl/ssl3ext.c:2617: return 0;
On 2015/06/12 00:16:04, mt wrote:
> I'd move this up and avoid checking maxBytes against extension_length twice. 
> BTW, mixing camel case and underscores is not-awesome.

I don't think you can do that, because before you only check against
append.

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/sslsock.c
File lib/ssl/sslsock.c (right):

https://codereview.appspot.com/211900043/diff/40001/lib/ssl/sslsock.c#newcode86
lib/ssl/sslsock.c:86: PR_FALSE    /* enableEtendedMS    */
On 2015/06/12 00:16:04, mt wrote:
> PR_TRUE and check spelling
See needinfo on bug.