net: allow False Start only for >= TLS 1.2 && AEAD && forward-secure && ALPN/NPN.

net: allow False Start only for >= TLS 1.2 && AEAD && forward-secure && ALPN/NPN.

Tighten up the requirements for False Start. At this point, neither
AES-CBC or RC4 are something that we want to use unless we're sure that
the server wants to speak them.

BUG=427721

[agl, 2014-10-17 22:54:12]

agl@chromium.org changed reviewers:
+ rsleevi@chromium.org

[agl, 2014-10-17 22:54:13]

SSL_GetChannelInfo takes the SpecReadLock, which is the same lock that's taken
and released in ssl3_CheckFalseStart. So I think the reentrance is ok.

It's a little unfortunate that the lock gets taken twice, but it's nicer to
tweak Chromium than NSS.

[Ryan Sleevi, 2014-10-17 23:08:53]

You had me confused for a second with reentrance (turns out, we've already got
lock re-entrancy - ssl3_CheckFalseStart asserts ssl_HaveSSL3HandshakeLock, but
then we call SSL_HandshakeNegotiatedExtension which does
ssl_GetSSL3HandshakeLock - surprised that hasn't caused any issues?)

For //net's usage, I think this is fine. However, I'm finding an increasing
number of people using //net without conforming to the API, and my fear is that
someone is going to be using this from multiple threads again despite that being
against API contract (several teams have done it now). This may break them in
some subtle acquire/release way, though unlikely, and to be fair, they're
already broken, but it's now debt we have to bear x_x.

I just mention all this because while this LGTM and is safe for all uses in
//net, you may find someone yelling 6-8 weeks about some TSAN locking or weird
behaviour. In which case, please slap them (or their code). So just a sort of
wary remark of unease, but it's within our API contract and is the simpler
change.

[Ryan Sleevi, 2014-10-17 23:10:38]

Also, can you file a bug here? This should at least appear in crbug so we can
track if there are any perf refressions that come from tightening this up.

[Ryan Sleevi, 2014-10-27 22:15:51]

On 2014/10/17 23:10:38, Ryan Sleevi wrote:
> Also, can you file a bug here? This should at least appear in crbug so we can
> track if there are any perf refressions that come from tightening this up.

Ping?

[agl, 2014-10-27 22:20:10]

On 2014/10/27 22:15:51, Ryan Sleevi wrote:
> Ping?

On hold because tlslite doesn't support AES-GCM thus this would break our False
Start tests :(

[Ryan Sleevi, 2014-10-27 22:25:10]

On 2014/10/27 22:20:10, agl wrote:
> On 2014/10/27 22:15:51, Ryan Sleevi wrote:
> > Ping?
> 
> On hold because tlslite doesn't support AES-GCM thus this would break our
False
> Start tests :(

Since you're only touching the NSS file, what's the BoringSSL behaviour?

Should we add a testing bool that can disable this check for the false start
tests?

[agl, 2014-10-27 22:30:45]

On 2014/10/27 22:25:10, Ryan Sleevi wrote:
> Since you're only touching the NSS file, what's the BoringSSL behaviour?

The corresponding BoringSSL patch is on hold too.

> Should we add a testing bool that can disable this check for the false start
> tests?

I don't really want to plumb in another config bit. The answer might be to make
this change only in BoringSSL and disable the Chrome-level tests for False Start
with BoringSSL. BoringSSL has it's own False Start tests, although we do end up
dropping some test coverage. However, the other options are 1) not to make this
change or 2) to try and get tlslite to support AES-GCM (ugh).

[Ryan Sleevi, 2014-10-27 22:33:22]

On 2014/10/27 22:30:45, agl wrote:
> On 2014/10/27 22:25:10, Ryan Sleevi wrote:
> > Since you're only touching the NSS file, what's the BoringSSL behaviour?
> 
> The corresponding BoringSSL patch is on hold too.
> 
> > Should we add a testing bool that can disable this check for the false start
> > tests?
> 
> I don't really want to plumb in another config bit. The answer might be to
make
> this change only in BoringSSL and disable the Chrome-level tests for False
Start
> with BoringSSL. BoringSSL has it's own False Start tests, although we do end
up
> dropping some test coverage. However, the other options are 1) not to make
this
> change or 2) to try and get tlslite to support AES-GCM (ugh).

I'd like to suggest we go ahead and make this change, even if it means missing
out on test coverage, on all platforms (NSS and BoringSSL).

While not ideal, I'd rather we get the security story right and then worry about
testing than the alternative.

As a poor justification, it's worth noting that we don't have tests for the
equivalent HTTP/2 functionality (
https://code.google.com/p/chromium/codesearch#chromium/src/net/spdy/spdy_sess...
), and we haven't doomed ourselves quite yet.

[agl, 2014-10-27 22:46:54]

On 2014/10/27 22:33:22, Ryan Sleevi wrote:
> I'd like to suggest we go ahead and make this change, even if it means missing
> out on test coverage, on all platforms (NSS and BoringSSL).

Ok. Let me finished up the "disable SSLv3 patch" and I'll update and ping this
CL.

[agl, 2014-10-27 23:34:10]

Ok, now removes the False Start tests too. (Removed rather than disabled because
I suspect that we won't be adding AES-GCM support to tlslite anytime soon and we
can always pull them out of git.)

[agl, 2014-10-27 23:37:19]

Also, bug filed as requested.

[Ryan Sleevi, 2014-10-27 23:39:42]

On 2014/10/27 23:34:10, agl wrote:
> Ok, now removes the False Start tests too. (Removed rather than disabled
because
> I suspect that we won't be adding AES-GCM support to tlslite anytime soon and
we
> can always pull them out of git.)

Is there a change for BoringSSL too? So we can keep them in sync?

I'm sad to lose the two session resumption tests, since those were real bugs. It
should be possible to keep the tests, with a static
SSLClientSocket::EnableInsecureFalseStartForTesting &
SSLClientSocket::AllowInsecureFalseStartForTesting

The ForTesting will cause presubmit to yell if either method is called from a
non-_unittest file, and lets us keep the same coverage.

[agl, 2014-10-28 20:08:46]

On 2014/10/27 23:39:42, Ryan Sleevi wrote:
> I'm sad to lose the two session resumption tests, since those were real bugs.
It
> should be possible to keep the tests, with a static
> SSLClientSocket::EnableInsecureFalseStartForTesting &
> SSLClientSocket::AllowInsecureFalseStartForTesting
> 
> The ForTesting will cause presubmit to yell if either method is called from a
> non-_unittest file, and lets us keep the same coverage.

I don't want to have to plumb more complexity into BoringSSL to enable this.
It'll be, at least, another flag in the SSL* and more functions to set it etc.
Let me see what I think of that after a while.

[Ryan Sleevi, 2014-10-30 01:38:21]

On 2014/10/28 20:08:46, agl wrote:
> On 2014/10/27 23:39:42, Ryan Sleevi wrote:
> > I'm sad to lose the two session resumption tests, since those were real
bugs.
> It
> > should be possible to keep the tests, with a static
> > SSLClientSocket::EnableInsecureFalseStartForTesting &
> > SSLClientSocket::AllowInsecureFalseStartForTesting
> > 
> > The ForTesting will cause presubmit to yell if either method is called from
a
> > non-_unittest file, and lets us keep the same coverage.
> 
> I don't want to have to plumb more complexity into BoringSSL to enable this.
> It'll be, at least, another flag in the SSL* and more functions to set it etc.
> Let me see what I think of that after a while.

I guess I'm confused, because these tests are running on both NSS and BoringSSL
presently. If BoringSSL was already doing this check, wouldn't these tests be
failing?

Or was the plan to add these internal to BoringSSL (e.g. no callback like w/
NSS) after this change?

For BoringSSL, could we just use an OP flag for testing? 

if (SSLClientSocket::AllowInsecureFalseStart())
  options.ConfigureFlag(SSL_MODE_INSECURE_FALSE_START, true)

[The above structure is to potentially allow for whole program optimization to
see AllowInsecureFalseStart is a constant and optimize out]

[agl, 2014-10-30 01:40:13]

On 2014/10/30 01:38:21, Ryan Sleevi wrote:
> On 2014/10/28 20:08:46, agl wrote:
> > On 2014/10/27 23:39:42, Ryan Sleevi wrote:
> > > I'm sad to lose the two session resumption tests, since those were real
> bugs.
> > It
> > > should be possible to keep the tests, with a static
> > > SSLClientSocket::EnableInsecureFalseStartForTesting &
> > > SSLClientSocket::AllowInsecureFalseStartForTesting
> > > 
> > > The ForTesting will cause presubmit to yell if either method is called
from
> a
> > > non-_unittest file, and lets us keep the same coverage.
> > 
> > I don't want to have to plumb more complexity into BoringSSL to enable this.
> > It'll be, at least, another flag in the SSL* and more functions to set it
etc.
> > Let me see what I think of that after a while.
> 
> I guess I'm confused, because these tests are running on both NSS and
BoringSSL
> presently. If BoringSSL was already doing this check, wouldn't these tests be
> failing?
> 
> Or was the plan to add these internal to BoringSSL (e.g. no callback like w/
> NSS) after this change?
> 
> For BoringSSL, could we just use an OP flag for testing? 
> 
> if (SSLClientSocket::AllowInsecureFalseStart())
>   options.ConfigureFlag(SSL_MODE_INSECURE_FALSE_START, true)
> 
> [The above structure is to potentially allow for whole program optimization to
> see AllowInsecureFalseStart is a constant and optimize out]

The BoringSSL change hasn't landed yet
(https://boringssl-review.googlesource.com/#/c/1980/)

I really don't like adding more flags for this but maybe we have to.

[davidben, 2015-02-02 19:09:00]

We should be able to land this now without extra flags or removing tests, maybe
worth an additional SSLClientSocketFalseStartTest.NoAEAD test. (BoringSSL
standalone could probably do with some negative False Start tests of its own...
I think if we send an alert just before kicking off the ExpectFalseStart
behavior, that could do it fairly robustly. Then it's a matter of whether the
peer sent app data before noticing the alert or no.)