942729 - Re-enable TLS False Start

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement, P2)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: re-enable-false-start.patch

+++ This bug was initially created as a clone of Bug #920248 +++

From bug 920248 comment #19:
> Note that in this patch I changed the default to require perfect forward
> secrecy, and I removed false start support for RC4.
> 
> I verified that we negotiate AES-GCM cipher suites when connecting to
> Twitter, Facebook, CloudFlare, and Google.
> 
> I also modified the telemetry so that we have more precise measurements for
> why we don't false start. So far in my limited testing, I've found that
> requiring forward secrecy and not allowing false start for RC4 have no
> effect on how often we false start. There appear to be two primary reasons:
> (1) The NPN requirement seems to subsume these requirements; that is, sites
> that support NPN seem to negotiate non-RC4 cipher suites with ephemeral key
> exchange already, and (2) The false start callback isn't called in many
> cases, presumably because the Finished message arrived before the
> certificate was successfully verified. Consequently, based on my limited
> testing, it seems like the primary improvements we need to make to improve
> the frequency of false starting are to remove the NPN requirement and to
> make certificate verification faster (i.e. stop doing OCSP requests).
> 
> Let's see if the telemetry we gather on Nightly and Aurora users matches my
> personal measurements.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I forgot to mention that I also removed 3DES from the set of cipher suites for which we false start, and I added Camellia. This is based on my research of the security merits and also on the telemetry that shows that 3DES is basically never negotiated in Nightly:

http://telemetry.mozilla.org/#path=nightly/28/SSL_SYMMETRIC_CIPHER

Comment 2

[Patrick McManus [:mcmanus]]

(In reply to Brian Smith (:briansmith, was :bsmith; Please NEEDINFO? me if you want a response) from comment #0)
it seems like the primary improvements we need to make to improve
> > the frequency of false starting are to remove the NPN requirement and to
> > make certificate verification faster (i.e. stop doing OCSP requests).

I share that expectation

Comment 3

[Patrick McManus [:mcmanus]]

Attachment: remove expected cipher suite mechanism

Comment 4

[Patrick McManus [:mcmanus]]

I think this patch is likely fine - thanks!

I've read it but want to test it out before I stick an r+ on it. I'll do that tomorrow.

I've added a second patch that just deletes a bunch of code now that we don't use need to track rc4 history in the permission manager anymore. I want to actually test that patch before I put the r? flag on it :)

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Patrick McManus [:mcmanus] from comment #4)
> I've added a second patch that just deletes a bunch of code now that we
> don't use need to track rc4 history in the permission manager anymore. I
> want to actually test that patch before I put the r? flag on it :)

Are you sure we should do that now? If the code isn't in the way, I'd rather keep it. We might need it for CBC mode or we may need it if we decide that blocking downgrade to RC4 is still too drastic. I think the telemetry my patch adds will help inform that decision.

Comment 6

[Patrick McManus [:mcmanus]]

(In reply to Brian Smith (:briansmith, was :bsmith; Please NEEDINFO? me if you want a response) from comment #5)
> (In reply to Patrick McManus [:mcmanus] from comment #4)
> > I've added a second patch that just deletes a bunch of code now that we
> > don't use need to track rc4 history in the permission manager anymore. I
> > want to actually test that patch before I put the r? flag on it :)
> 
> Are you sure we should do that now? If the code isn't in the way, I'd rather
> keep it. We might need it for CBC mode or we may need it if we decide that
> blocking downgrade to RC4 is still too drastic. I think the telemetry my
> patch adds will help inform that decision.

I think so - it tracks rc4 specifically with the permission manager which is definitely something I would prefer not to have if we didn't need it.. I'm skeptical of databases :) We have it as a separate patch so it should be easy to add back if it serves a purpose again and I would be cool with that if the need arose.

Comment 7

[Patrick McManus [:mcmanus]]

Comment on attachment 8337599 [details] [diff] [review]
re-enable-false-start.patch

Review of attachment 8337599 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm. I manually verified with facebook to see the right behavior wrt OCSP.

It was really nice to see a false-started tls 1.2 TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 connection!

::: security/manager/ssl/src/nsNSSCallbacks.cpp
@@ +963,4 @@
>    }
>  
> +  // XXX: This assumes that all TLS_DH_* and TLS_ECDH_* cipher suites
> +  // are disabled. 

whitespace nit

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I will check these in after fixing the whitespace nit.

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2f70138f5aa
https://hg.mozilla.org/integration/mozilla-inbound/rev/288b02c2e5d1

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e2f70138f5aa
https://hg.mozilla.org/mozilla-central/rev/288b02c2e5d1

Comment 11

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

FWIW, Apple's MacOSX/Safari criteria are very similar to ours, except that they don't require NPN and they allow RC4. They require AES and they require ephemeral key exchange.

http://opensource.apple.com/source/Security/Security-55471/libsecurity_ssl/lib/sslTransport.c

Comment 13

[u279076]

Brian, does this need any QA testing before we release Firefox 28?

Comment 14

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #13)
> Brian, does this need any QA testing before we release Firefox 28?

No, I don't know of any good way for QA to test it. We (Patrick and I) have tested it manually quite a few times already.