Fail the SPDY transaction if it does not meet TLS base requirements.

Fail the SPDY transaction if it does not meet TLS base requirements.

* Generally follows guidelines in https://http2.github.io/http2-spec/#TLSUsage.
* Apply only to SPDY4+ versions
* Fail the stream job if the TLS version for SPDY is too old (<1.2)
* Fail the stream job if the TLS cipher suite is sucky. Note that we're stricter here than the HTTP/2 spec.

Also added while implementing this CL:
* Add SSLConnectionStatus setters.
* Add ability for SSLSocketDataProvider to set SSLConnectionStatus.
* Add modern cipher suite check into net/ssl.

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

[willchan no longer on Chromium, 2014-05-20 02:35:25]

Preliminary question: Adam, how should I restrict the ciphersuites given an
SSLInfo?

[agl, 2014-05-20 02:45:12]

Do you want to restrict the ciphersuites, or validate them after the fact? I
think you want the latter. It would be nice never to offer any ciphersuites that
we aren't going to accept, but we don't know, for a given TLS handshake, whether
the server is going to negotiate SPDY or HTTP/2 and that determines what
ciphersuites are acceptable.

https://codereview.chromium.org/291093002/diff/1/net/spdy/spdy_network_transa...
File net/spdy/spdy_network_transaction_unittest.cc (right):

https://codereview.chromium.org/291093002/diff/1/net/spdy/spdy_network_transa...
net/spdy/spdy_network_transaction_unittest.cc:6624: 
This blank line should probably appear under the comment so that it's clear that
the commented code is grouped together.

https://codereview.chromium.org/291093002/diff/1/net/ssl/ssl_connection_statu...
File net/ssl/ssl_connection_status_flags.h (right):

https://codereview.chromium.org/291093002/diff/1/net/ssl/ssl_connection_statu...
net/ssl/ssl_connection_status_flags.h:79: DCHECK_GT(version, 0);
The minimum should be SSLv3 (and then be >=).

[willchan no longer on Chromium, 2014-05-20 03:58:25]

On Mon, May 19, 2014 at 7:45 PM, <agl@chromium.org> wrote:

> Do you want to restrict the ciphersuites, or validate them after the fact?
> I
> think you want the latter. It would be nice never to offer any
> ciphersuites that
> we aren't going to accept, but we don't know, for a given TLS handshake,
> whether
> the server is going to negotiate SPDY or HTTP/2 and that determines what
> ciphersuites are acceptable.
>

I meant restrict as in fail the HTTP transactions and kill the SPDY
connection if we negotiate a restricted ciphersuite. Because we want to
structure it as a blacklist, not a whitelist, right?

How do I do that with an SSLInfo?


>
>
> https://codereview.chromium.org/291093002/diff/1/net/spdy/
> spdy_network_transaction_unittest.cc
> File net/spdy/spdy_network_transaction_unittest.cc (right):
>
> https://codereview.chromium.org/291093002/diff/1/net/spdy/
> spdy_network_transaction_unittest.cc#newcode6624
> net/spdy/spdy_network_transaction_unittest.cc:6624:
> This blank line should probably appear under the comment so that it's
> clear that the commented code is grouped together.
>
> https://codereview.chromium.org/291093002/diff/1/net/ssl/
> ssl_connection_status_flags.h
> File net/ssl/ssl_connection_status_flags.h (right):
>
> https://codereview.chromium.org/291093002/diff/1/net/ssl/
> ssl_connection_status_flags.h#newcode79
> net/ssl/ssl_connection_status_flags.h:79: DCHECK_GT(version, 0);
> The minimum should be SSLv3 (and then be >=).
>
> https://codereview.chromium.org/291093002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[agl, 2014-05-20 04:02:51]

On Mon, May 19, 2014 at 8:58 PM, William Chan (陈智昌)
<willchan@chromium.org> wrote:
> How do I do that with an SSLInfo?

SSLConnectionStatusToCipherSuite. That's the IANA cipher suite number.
SSLCipherSuiteToStrings knows how to decode them.


Cheers

AGL

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[willchan no longer on Chromium, 2014-05-20 05:25:40]

On Mon, May 19, 2014 at 9:02 PM, Adam Langley <agl@chromium.org> wrote:

> On Mon, May 19, 2014 at 8:58 PM, William Chan (陈智昌)
> <willchan@chromium.org> wrote:
> > How do I do that with an SSLInfo?
>
> SSLConnectionStatusToCipherSuite. That's the IANA cipher suite number.
> SSLCipherSuiteToStrings knows how to decode them.
>

Sorry, I wasn't clear. Do I have to create an array of blacklisted cipher
suites? What's the best way to do this? If I need a blacklist array, can
you tell me everything I should put into it? I was kinda hoping there were
already functions I could call to blacklist large chunks of them.


>
>
> Cheers
>
> AGL
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[agl, 2014-05-20 17:40:26]

On Mon, May 19, 2014 at 10:25 PM, William Chan (陈智昌)
<willchan@chromium.org> wrote:
> Sorry, I wasn't clear. Do I have to create an array of blacklisted cipher
> suites? What's the best way to do this? If I need a blacklist array, can you
> tell me everything I should put into it? I was kinda hoping there were
> already functions I could call to blacklist large chunks of them.

No, we don't have any functions for that. The SSLCipherSuiteToStrings
code contains the only mapping between cipher suite IDs and the
primitives that make them up that we have in the Chromium code. (NSS
and OpenSSL have their own ones, of course, but we shouldn't depend on
them because we're switching if nothing else.)

SSLCipherSuiteToStrings could be tweaked so that you don't have to
string match on the result.


Cheers

AGL

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[willchan no longer on Chromium, 2014-05-21 00:51:46]

+wtc for net/ssl
+jgraettinger for everything else

[Johnny, 2014-05-21 16:27:04]

https://codereview.chromium.org/291093002/diff/40001/net/http/http_stream_fac...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/291093002/diff/40001/net/http/http_stream_fac...
net/http/http_stream_factory_impl_job.cc:1131: "TLS security too low");
Create an ERR mapping for INADEQUATE_SECURITY [1] and use that?

https://code.google.com/p/chromium/codesearch#chromium/src/net/spdy/spdy_prot...

https://codereview.chromium.org/291093002/diff/40001/net/spdy/spdy_network_tr...
File net/spdy/spdy_network_transaction_unittest.cc (right):

https://codereview.chromium.org/291093002/diff/40001/net/spdy/spdy_network_tr...
net/spdy/spdy_network_transaction_unittest.cc:6619: new
SSLSocketDataProvider(ASYNC, OK));
What about factoring the duplicated test bodies out? Possibly subclass
SpdyNetworkTransactionTest (rather than typedef)
SpdyNetworkTransactionNoTLSUsageCheckTest, and add it there? Ditto with
SpdyNetworkTransactionTLSUsageCheckTest.

[willchan no longer on Chromium, 2014-05-21 17:47:15]

https://codereview.chromium.org/291093002/diff/40001/net/http/http_stream_fac...
File net/http/http_stream_factory_impl_job.cc (right):

https://codereview.chromium.org/291093002/diff/40001/net/http/http_stream_fac...
net/http/http_stream_factory_impl_job.cc:1131: "TLS security too low");
On 2014/05/21 16:27:04, Johnny wrote:
> Create an ERR mapping for INADEQUATE_SECURITY [1] and use that?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/spdy/spdy_prot...

Done.

https://codereview.chromium.org/291093002/diff/40001/net/spdy/spdy_network_tr...
File net/spdy/spdy_network_transaction_unittest.cc (right):

https://codereview.chromium.org/291093002/diff/40001/net/spdy/spdy_network_tr...
net/spdy/spdy_network_transaction_unittest.cc:6619: new
SSLSocketDataProvider(ASYNC, OK));
On 2014/05/21 16:27:04, Johnny wrote:
> What about factoring the duplicated test bodies out? Possibly subclass
> SpdyNetworkTransactionTest (rather than typedef)
> SpdyNetworkTransactionNoTLSUsageCheckTest, and add it there? Ditto with
> SpdyNetworkTransactionTLSUsageCheckTest.

Done.

[Johnny, 2014-05-21 18:12:15]

lgtm for non-net/ssl

[wtc, 2014-05-21 21:51:10]

Patch set 7 LGTM.

High-level comments:

The criteria of IsModernTLSCipherSuite needs to be documented clearly. A caller
of this function most likely needs to meet some security requirements, so the
caller needs to know if the criteria of this function meet (or even exceed)
those security requirements.

https://codereview.chromium.org/291093002/diff/100001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/291093002/diff/100001/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:871: if (!IsModernTLSCipherSuite(

The current IsModernTLSCipherSuite code exceeds the requirements of
https://http2.github.io/http2-spec/#TLSUsage. For example,
IsModernTLSCipherSuite disallows AES_128_CBC, but
https://http2.github.io/http2-spec/#TLSUsage allows it.

https://codereview.chromium.org/291093002/diff/100001/net/ssl/ssl_cipher_suit...
File net/ssl/ssl_cipher_suite_names.cc (right):

https://codereview.chromium.org/291093002/diff/100001/net/ssl/ssl_cipher_suit...
net/ssl/ssl_cipher_suite_names.cc:349: struct CipherSuite desired = {0};

Nit: I know you copied this from existing code, but since this is a C++ file, we
don't need 'struct' here.

https://codereview.chromium.org/291093002/diff/100001/net/ssl/ssl_cipher_suit...
net/ssl/ssl_cipher_suite_names.cc:356: CipherSuiteCmp);

Nit: just wondering why this is formatted in a different way from the original
code at lines 286-288. Is this formatted by clang?

https://codereview.chromium.org/291093002/diff/100001/net/ssl/ssl_cipher_suit...
net/ssl/ssl_cipher_suite_names.cc:369: case 10:  // DHE_RSA

Why do you not allow 8 (DHE_DSS)?

https://codereview.chromium.org/291093002/diff/100001/net/ssl/ssl_cipher_suit...
File net/ssl/ssl_cipher_suite_names.h (right):

https://codereview.chromium.org/291093002/diff/100001/net/ssl/ssl_cipher_suit...
net/ssl/ssl_cipher_suite_names.h:54: NET_EXPORT_PRIVATE bool
IsModernTLSCipherSuite(uint16 cipher_suite);

1. Nit: it seems that this function should be renamed "IsSecureTLSCipherSuite".

Update: after I read the .cc file, I knew what you meant by "modern" and
"backwards compatibility can be ignored". So I have a new suggestion. See the
next comment.

Update2: after I saw how this function is used, I wonder if it should be renamed
"IsTLSCipherSuiteAcceptableToHTTP2".

2. IMPORTANT: I am afraid that we need to document the criteria of this function
here. When I read the .cc file, I found that the criteria weren't what I
expected. So this function isn't usable unless one knows exactly what its
criteria are.

https://codereview.chromium.org/291093002/diff/100001/net/ssl/ssl_connection_...
File net/ssl/ssl_connection_status_flags.h (right):

https://codereview.chromium.org/291093002/diff/100001/net/ssl/ssl_connection_...
net/ssl/ssl_connection_status_flags.h:68: int tmp = *connection_status;

Nit: why do we need to use a tmp variable? Does this help the compiler generate
more efficient code?

[willchan no longer on Chromium, 2014-05-21 22:55:22]

https://codereview.chromium.org/291093002/diff/100001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/291093002/diff/100001/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:871: if (!IsModernTLSCipherSuite(
On 2014/05/21 21:51:10, wtc wrote:
> 
> The current IsModernTLSCipherSuite code exceeds the requirements of
> https://http2.github.io/http2-spec/#TLSUsage. For example,
> IsModernTLSCipherSuite disallows AES_128_CBC, but
> https://http2.github.io/http2-spec/#TLSUsage allows it.

Yes, that's because agl@ and I feel like being stricter than HTTP/2 requires.
I've updated the CL description to note this.

https://codereview.chromium.org/291093002/diff/100001/net/ssl/ssl_cipher_suit...
File net/ssl/ssl_cipher_suite_names.cc (right):

https://codereview.chromium.org/291093002/diff/100001/net/ssl/ssl_cipher_suit...
net/ssl/ssl_cipher_suite_names.cc:349: struct CipherSuite desired = {0};
On 2014/05/21 21:51:10, wtc wrote:
> 
> Nit: I know you copied this from existing code, but since this is a C++ file,
we
> don't need 'struct' here.

Done.

https://codereview.chromium.org/291093002/diff/100001/net/ssl/ssl_cipher_suit...
net/ssl/ssl_cipher_suite_names.cc:356: CipherSuiteCmp);
On 2014/05/21 21:51:10, wtc wrote:
> 
> Nit: just wondering why this is formatted in a different way from the original
> code at lines 286-288. Is this formatted by clang?

Yes.

https://codereview.chromium.org/291093002/diff/100001/net/ssl/ssl_cipher_suit...
net/ssl/ssl_cipher_suite_names.cc:369: case 10:  // DHE_RSA
On 2014/05/21 21:51:10, wtc wrote:
> 
> Why do you not allow 8 (DHE_DSS)?

agl@ tells me no one uses DSS.

https://codereview.chromium.org/291093002/diff/100001/net/ssl/ssl_cipher_suit...
File net/ssl/ssl_cipher_suite_names.h (right):

https://codereview.chromium.org/291093002/diff/100001/net/ssl/ssl_cipher_suit...
net/ssl/ssl_cipher_suite_names.h:54: NET_EXPORT_PRIVATE bool
IsModernTLSCipherSuite(uint16 cipher_suite);
I've renamed to IsSecureTLSCipherSuite and added the criteria to the comments.

On 2014/05/21 21:51:10, wtc wrote:
> 
> 1. Nit: it seems that this function should be renamed
"IsSecureTLSCipherSuite".
> 
> Update: after I read the .cc file, I knew what you meant by "modern" and
> "backwards compatibility can be ignored". So I have a new suggestion. See the
> next comment.
> 
> Update2: after I saw how this function is used, I wonder if it should be
renamed
> "IsTLSCipherSuiteAcceptableToHTTP2".
> 
> 2. IMPORTANT: I am afraid that we need to document the criteria of this
function
> here. When I read the .cc file, I found that the criteria weren't what I
> expected. So this function isn't usable unless one knows exactly what its
> criteria are.

[wtc, 2014-05-22 17:57:01]

Patch set 8 LGTM. Thanks.

https://codereview.chromium.org/291093002/diff/100001/net/ssl/ssl_connection_...
File net/ssl/ssl_connection_status_flags.h (right):

https://codereview.chromium.org/291093002/diff/100001/net/ssl/ssl_connection_...
net/ssl/ssl_connection_status_flags.h:68: int tmp = *connection_status;

On 2014/05/21 21:51:10, wtc wrote:
> 
> Nit: why do we need to use a tmp variable? Does this help the compiler
generate
> more efficient code?

I'm still curious about the answer to this question.

[agl, 2014-05-22 18:04:57]

https://codereview.chromium.org/291093002/diff/100001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/291093002/diff/100001/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:871: if (!IsModernTLSCipherSuite(
On 2014/05/21 22:55:22, willchan (traveling 5.23-6.27) wrote:
> Yes, that's because agl@ and I feel like being stricter than HTTP/2 requires.
> I've updated the CL description to note this.

Hopefully we can get HTTP/2 updated to reflect this. Otherwise site operators
won't have a single place to learn what is needed to safely enable the new
HTTP/2 version.

[willchan no longer on Chromium, 2014-05-22 18:26:43]

https://codereview.chromium.org/291093002/diff/100001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/291093002/diff/100001/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:871: if (!IsModernTLSCipherSuite(
On 2014/05/22 18:04:57, agl wrote:
> On 2014/05/21 22:55:22, willchan (traveling 5.23-6.27) wrote:
> > Yes, that's because agl@ and I feel like being stricter than HTTP/2
requires.
> > I've updated the CL description to note this.
> 
> Hopefully we can get HTTP/2 updated to reflect this. Otherwise site operators
> won't have a single place to learn what is needed to safely enable the new
> HTTP/2 version.

I will take this to httpbis.

https://codereview.chromium.org/291093002/diff/100001/net/ssl/ssl_connection_...
File net/ssl/ssl_connection_status_flags.h (right):

https://codereview.chromium.org/291093002/diff/100001/net/ssl/ssl_connection_...
net/ssl/ssl_connection_status_flags.h:68: int tmp = *connection_status;
On 2014/05/22 17:57:02, wtc wrote:
> 
> On 2014/05/21 21:51:10, wtc wrote:
> > 
> > Nit: why do we need to use a tmp variable? Does this help the compiler
> generate
> > more efficient code?
> 
> I'm still curious about the answer to this question.

Oops, I missed this earlier. I've removed the tmp variables.

[willchan no longer on Chromium, 2014-05-22 22:41:03]

The CQ bit was checked by willchan@chromium.org

[commit-bot: I haz the power, 2014-05-22 22:42:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/willchan@chromium.org/291093002/140001

[commit-bot: I haz the power, 2014-05-23 10:31:53]

Change committed as 272467