Support new ChannelID extension.

Support new ChannelID extension.

This change switches over to use the new-style ChannelID extension.

BUG=305951
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235826

[agl, 2013-11-12 22:20:51]

ping.

[wtc, 2013-11-14 00:50:56]

Patch set 2 LGTM.

IMPORTANT: the handshake hash ends at the client's Finished message and does not
include the server's NewSessionTicket and Finished messages. Is this
intentional?

https://codereview.chromium.org/27589002/diff/3001/net/third_party/nss/ssl/ss...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/27589002/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:7581: 

Can you assert ss->sec.ci.sid->cached == never_cached?

https://codereview.chromium.org/27589002/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:7583: /* the cwSpec and zero arguments are
only used for SSLv3, but we know

The cwSpec argument is also used by ssl3_ComputeHandshakeHashes to get the
protocol version (spec->version) and to verify we have the master secret
available, even though the master secret is only used for SSLv3. So this comment
is a little inaccurate.

Are you trying to explain why it doesn't matter whether we're using
ss->ssl3.cwSpec or ss->ssl3.crSpec? At this point, we haven't received the
server's ChangeCipherSpec, so ss->ssl3.crSpec is still using the null cipher.

https://codereview.chromium.org/27589002/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:7586: ssl3_ComputeHandshakeHashes(ss,
ss->ssl3.cwSpec, &hashes, 0);

Check the return value?

https://codereview.chromium.org/27589002/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:7592: goto loser;

This should be simply "return SECFailure" if you move the
ssl_ReleaseXmitBufLock(ss) call up, as I suggest below.

https://codereview.chromium.org/27589002/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:7597:
ssl_ReleaseXmitBufLock(ss);		/*******************************/

We should call ssl_ReleaseXmitBufLock(ss) before recording the full handshake
hash.

https://codereview.chromium.org/27589002/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:10624: size_t j;

Nit: this variable should be named signed_data_len if that's not too long.

https://codereview.chromium.org/27589002/diff/3001/net/third_party/nss/ssl/ss...
File net/third_party/nss/ssl/sslimpl.h (right):

https://codereview.chromium.org/27589002/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/sslimpl.h:703: * handshake. This is either a SHA-1/MD5
combination (for TLS <

IMPORTANT: this hash does not include the final handshake message flow from the
server (optional NewSessionTicket, and Finished). So this comment is confusing.

https://codereview.chromium.org/27589002/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/sslimpl.h:704: * 1.2) or a SHA-2 hash (for TLS 1.2).
This is recorded and used

To be future proof, we should say this is the hash of the TLS PRF for TLS 1.2.

https://codereview.chromium.org/27589002/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/sslimpl.h:708: SECItem           lastHandshakeHash;

Nit: "last" can also be interpreted to mean the initial handshake (as opposed to
a subsequent renegotiation), or even the previous handshake. Perhaps we should
name this "fullHandshakeHash" or "originalHandshakeHash"?

[agl, 2013-11-14 18:50:42]

The hash of the original handshake needs to cover the server's certificate,
primarily. The NewSessionTicket and Finished are not important.

https://codereview.chromium.org/27589002/diff/3001/net/third_party/nss/ssl/ss...
File net/third_party/nss/ssl/ssl3con.c (right):

https://codereview.chromium.org/27589002/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:7581: 
On 2013/11/14 00:50:56, wtc wrote:
> 
> Can you assert ss->sec.ci.sid->cached == never_cached?

Done.

https://codereview.chromium.org/27589002/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:7583: /* the cwSpec and zero arguments are
only used for SSLv3, but we know
On 2013/11/14 00:50:56, wtc wrote:
> The cwSpec argument is also used by ssl3_ComputeHandshakeHashes to get the
> protocol version (spec->version) and to verify we have the master secret
> available, even though the master secret is only used for SSLv3. So this
comment
> is a little inaccurate.

This comment is clearly confusing. I've deleted it.

https://codereview.chromium.org/27589002/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:7586: ssl3_ComputeHandshakeHashes(ss,
ss->ssl3.cwSpec, &hashes, 0);
On 2013/11/14 00:50:56, wtc wrote:
> 
> Check the return value?

Done.

https://codereview.chromium.org/27589002/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:7592: goto loser;
On 2013/11/14 00:50:56, wtc wrote:
> 
> This should be simply "return SECFailure" if you move the
> ssl_ReleaseXmitBufLock(ss) call up, as I suggest below.

Done.

https://codereview.chromium.org/27589002/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:7597:
ssl_ReleaseXmitBufLock(ss);		/*******************************/
On 2013/11/14 00:50:56, wtc wrote:
> 
> We should call ssl_ReleaseXmitBufLock(ss) before recording the full handshake
> hash.

Done.

https://codereview.chromium.org/27589002/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl3con.c:10624: size_t j;
On 2013/11/14 00:50:56, wtc wrote:
> 
> Nit: this variable should be named signed_data_len if that's not too long.

Done.

https://codereview.chromium.org/27589002/diff/3001/net/third_party/nss/ssl/ss...
File net/third_party/nss/ssl/sslimpl.h (right):

https://codereview.chromium.org/27589002/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/sslimpl.h:703: * handshake. This is either a SHA-1/MD5
combination (for TLS <
On 2013/11/14 00:50:56, wtc wrote:
> 
> IMPORTANT: this hash does not include the final handshake message flow from
the
> server (optional NewSessionTicket, and Finished). So this comment is
confusing.

Done.

https://codereview.chromium.org/27589002/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/sslimpl.h:704: * 1.2) or a SHA-2 hash (for TLS 1.2).
This is recorded and used
On 2013/11/14 00:50:56, wtc wrote:
> 
> To be future proof, we should say this is the hash of the TLS PRF for TLS 1.2.

Done.

https://codereview.chromium.org/27589002/diff/3001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/sslimpl.h:708: SECItem           lastHandshakeHash;
On 2013/11/14 00:50:56, wtc wrote:
> 
> Nit: "last" can also be interpreted to mean the initial handshake (as opposed
to
> a subsequent renegotiation), or even the previous handshake. Perhaps we should
> name this "fullHandshakeHash" or "originalHandshakeHash"?

Went with originalHandshakeHash.

[wtc, 2013-11-14 20:25:00]

Patch set 3 LGTM.

I was surprised that the originalHandshakeHash does not include the server's
final message flow, and was wondering if other people may also miss this point.

But I checked the "tls-unique" channel binding type and found it's defined in
the same way ("The first TLS Finished message sent"). So this should be as good
as tls-unique.

https://codereview.chromium.org/27589002/diff/83001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3ext.c (right):

https://codereview.chromium.org/27589002/diff/83001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3ext.c:821: return 0;

We probably should add a PORT_Assert(0) here because reaching this point would
indicate an NSS bug.

[agl, 2013-11-15 18:48:26]

https://codereview.chromium.org/27589002/diff/83001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3ext.c (right):

https://codereview.chromium.org/27589002/diff/83001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3ext.c:821: return 0;
On 2013/11/14 20:25:00, wtc wrote:
> 
> We probably should add a PORT_Assert(0) here because reaching this point would
> indicate an NSS bug.

Done.

[commit-bot: I haz the power, 2013-11-15 18:57:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/27589002/263001

[commit-bot: I haz the power, 2013-11-15 20:04:47]

Retried try job too often on mac_rel for step(s) app_list_unittests,
cacheinvalidation_unittests, cc_unittests, check_deps, chromedriver2_unittests,
components_unittests, content_browsertests, content_unittests, crypto_unittests,
google_apis_unittests, gpu_unittests, ipc_tests, jingle_unittests,
media_unittests, message_center_unittests, nacl_integration, ppapi_unittests,
printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests,
sync_unit_tests, telemetry_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-11-18 19:16:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/27589002/263001

[commit-bot: I haz the power, 2013-11-18 22:00:05]

Change committed as 235826