658222 - (tls-false-start) Enable SSL False Start for HTTPS/WebSockets/SPDY connections

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

After reading the chromium blog post about False Start (http://blog.chromium.org/2011/05/ssl-falsestart-performance-results.html), I started guessing if we had the same feature.

I figured out we do, but it has been disabled by default for Firefox 4, because not ready for prime time, in Bug 591523.

Currently the chromium blacklist is a good starting point to check the current situation about compatibility: http://codesearch.google.com/codesearch/p#OAMlx_jo-ck/src/net/base/ssl_false_start_blacklist.txt

I'm filing this bug because I can't find a bug filed to reevaluate the decision for a future version, and where to append dependent bugs, where needed.

Comment 1

[Dirkjan Ochtman (:djc)]

Can we do this, please? Looking at today's blogpost, this seems like a no-brainer. Some people seem to think blacklists are never the right solution, but from this blog post it looks like there's very little reason to not use a blacklist in this case.

http://blog.chromium.org/2011/05/ssl-falsestart-performance-results.html

Comment 2

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

Also, from previous discussion with Chromium developers a couple of weeks ago, we would need to handle certain anti-virus software specially, as some products are also incompatible with false start.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Btw, I don't think this is something feasible for FF6 (not enough time to figure out the current situation), most likely could be evaluated for 7 or 8.

Comment 4

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

I spoke with Wan-Teh about the process they used to enable False Start in Chrome. I also investigated the Chromium bug database. Google is still receiving bug reports of incompatible sites.

So far, an incomplete list of products that are known to have current or previous compatibility problems with False Start include (AFAICT): F5, A10, Microsoft TMG, Cisco ASA, ServerIron ADX, ESET, NetNanny, some configurations of Java's SSL server implementation. Most, perhaps all, of these vendors have either released patches or are working on patches. In many cases, new deployments of these products will be shipping with the False Start compatibility fixes applied. This is why Google feels a black list is acceptable. 

Restricting False Start to work only for certs that chain to a built-in root would, according to Wan-Teh, eliminate the known antivirus-/LSP/intercepting-proxy- incompatibilities.

The blacklist is mostly for the server-side products. The blacklist was created by crawling sites (using a list derived from Google's database of known SSL sites) and from bug reports. Google is still getting bug reports and/or requests to be added to the blacklist. Google generally asks the website to update to the latest version of their TLS terminator software and Google generally only adds the websites to the blacklist as a last resort.

Long-term, the safest solution is to define a TLS extension that allows the server to indicate that it supports and wants False Start to be used, and then make False Start off by default. This would be straightforward to specify, andi it would be easy to contribute the implementation of this extension to server-side NSS, OpenSSL, and even the JDK. I will ask the Googlers to participate in the effort of specifying the extension, but I do not know if they want to bother with it. On the other hand, I just met with Google today about another TLS extension and it seems pretty straightforward to push the two of them together at the same time, especially as there is an IETF meeting coming up very soon. I will also contact Microsoft, Opera, and other vendors about it.

Short-term, I think we should implement TLS optimistically like Google has done, until such an extension can be widely adopted. But, I think we might be able to come up with a safer way of enabling it, that will work for the vast majority of websites while greatly reducing the likelihood of running into problems with even unknown incompatible sites. In particular, I would like to investigate fingerprinting of the server's handshake messages and/or HTTP responses to identify extremely-likely-to-be-compatible and likely-to-be-incompatible servers in addition to using the "built-in roots only" policy and/or the blacklist. I will ask Google if they investigated this kind of fingerprinting and report the results here. Note: doing fingerprinting based on a HTTP response from the server would mean that the very first connection to a server would never be a False Start connection. See below.

IMO, it is best to just do it for ((EC)DHE_)RSA_(AES*|RC4-128|3DES)* cipher suites, which is basically what Chrome has done. We specifically need to leave it disabled for False Start on ECDHE_ECDSA and DSA cipher suites specifically as government recommendations/regulations aren't written with this shortcut in mind. In practice, these restrictions would have no significant impact on the users we are aiming for. We may further restrict the implementation to TLS 1.0+-only servers and only to servers that don't trigger our insecure compatibility rollback mode (to SSL 3.0 without extensions).

Restricting False Start to built-in roots only will probably have a negative impact on users using many kinds of anti-virus software. I will revisit this decision after we have defeated other sources of incompatibility.

One problem with False Start is that it enables cipher suite and protocol version rollback attacks. In practice, by limiting the cipher suites and/or versions that are considered acceptable for False Start, and never enabling it on an insecurely rolled back connection, the risks of rollback are very low. We could further mitigate the risk of rollback by requiring the very first connection to a server to be a non-False-Start one, remembering what cipher suite and version it picked, doing False Start only for connections with those same parameters, and resetting this state if/when the parameters ever change. 

Google's testing has, AFAICT, only been done for HTTPS servers. That is why I am focusing this bug specifically on the ability for PSM to allow the application to enable False Start for *HTTPS* connections; IMO, the reward/risk ratio for enabling it for HTTPS, Websockets, and SPDY. Possibly/probably, this means restricting it to port 443 and ports above 1023 (as many test servers run on these high-numbered ports).

I do not know exactly what compatibility assurance strategies we will need to implement in order to turn SPDY on in stable branches. But, I would like to implement as few of them as possible (e.g. just the restriction to built-in roots, plus a hard-coded blacklist), enable it in Nightly ASAP, explicitly disabled in Aurora/Beta/Final releases until we have a high degree of confidence in whatever additional mitigation we implement.

Note also that there are other optimizations to our handshaking that will, AFAICT, have a bigger performance impact and less risk than TLS False Start. I will document this and suggest the prioritization of all these improvements ASAP.

Comment 5

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

> IMO, the reward/risk ratio for enabling it for HTTPS, Websockets, and SPDY

... is high, but the reward/risk ratio for other protocols/ports seems low.

Comment 6

[Adam Langley]

(In reply to comment #4)
> So far, an incomplete list of products that are known to have current or
> previous compatibility problems with False Start include (AFAICT): F5, A10,
> Microsoft TMG, Cisco ASA, ServerIron ADX, ESET, NetNanny, some
> configurations of Java's SSL server implementation.

(In rough order of observed occurrence:)

A10: Firmware update available
Brocade (F5/ServerIron): Firmware update available (Not all products affected.) 
FTMG: Update available
NetNanny: silent fallback in Chrome to allow vendor to update their users.
ESET: No response from vendor. Currently a hard error in Chrome with instructions for disabling HTTPS scanning.
Cisco ASA: No response from vendor
Java SSL: one report that may be erroneous. I haven't checked because I don't have a Java environment setup and the report was recent.

> The blacklist is mostly for the server-side products. The blacklist was
> created by crawling sites (using a list derived from Google's database of
> known SSL sites) and from bug reports.

The blacklist also contains quite a few false positives because of the noise in the scanning. I'm currently removing entries slowly and at random with the aim of nearly eliminating it by the end of the year.

All requests to add entries to the list are rejected now and have been for some time. 

> Long-term, the safest solution is to define a TLS extension that allows the
> server to indicate that it supports and wants False Start to be used, and
> then make False Start off by default.

Although I don't object to this I feel that, by the time any such extension sees significant deployment, the Internet should be False Start safe anyway.

> In particular, I would
> like to investigate fingerprinting of the server's handshake messages and/or
> HTTP responses to identify extremely-likely-to-be-compatible and
> likely-to-be-incompatible servers.

I considered fingerprinting the ServerHello but there aren't that many distinguishing features in a ServerHello and I didn't immediately see any strong correlations.

Also, the long term aim is to make the world a safe place for False Start and workarounds have to meet a high bar given their history of causing pain down the road. Silently falling back removes any pressure to update incompatible implementations.

Firefox can reduce any incompatibility issues by simply waiting as we'll continue to drive this.

> Note also that there are other optimizations to our handshaking that will,
> AFAICT, have a bigger performance impact and less risk than TLS False Start.
> I will document this and suggest the prioritization of all these
> improvements ASAP.

I'd be very curious to know what these are!


Cheers

AGL

Comment 7

[Neil Harris]

(In reply to comment #4)

> Note also that there are other optimizations to our handshaking that will,
> AFAICT, have a bigger performance impact and less risk than TLS False Start.
> I will document this and suggest the prioritization of all these
> improvements ASAP.

Filing these as bugs would be very useful.

Comment 8

[Asa Dotzler [:asa]]

we're not going to track this for aurora.

Comment 9

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

The handshake callback is called before the handshake completes; we need to analyze everything the handshake callback does to see if we would be deciding things (e.g. SPDY connection coalescing) inappropriately early based on the fact that the handshake callback has been called.

Comment 10

[[Baboo]]

No more blacklist-based solutions in Chrome, False Start just for NPNed connections starting with Chrome 20: http://www.imperialviolet.org/2012/04/11/falsestart.html

Comment 11

[Patrick McManus [:mcmanus]]

I have a PSM patch that, in conjunction with 713933, changes prefs to enable false start only in the presence of NPN.

It also ensures that all the handshake phases are complete before approving a connection for spdy coalescing. Its not clear whether or not that is strictly required, but it maintains the safety status quo for now and has almost no impact on real world connections because we don't have the need for new connections that might be coalesced before reading html from the coalescing-target connection and that obviously requires that the session is fully established to have happened.

https://tbpl.mozilla.org/?tree=Try&rev=605f96e2fe2c

Comment 12

[Patrick McManus [:mcmanus]]

Attachment: patch 0

Comment 13

[Patrick McManus [:mcmanus]]

Attachment: pcap with fb

pcap showing false start of a single http transaction with facebook

Comment 14

[Honza Bambas (:mayhemer)]

Comment on attachment 681958 [details] [diff] [review]
patch 0

According bug 713933, security.ssl.false_start.require_forward_secrecy has to be true by default.  Or I've missed explanation why not.

If channelInfo.fullHandshakeComplete is correctly set (what seems to be, however I didn't check on patch from bug 713933) then this patch is otherwise OK.

r=honzab with the pref value=true.

Comment 15

[Patrick McManus [:mcmanus]]

Thanks Honza!

(In reply to Honza Bambas (:mayhemer) from comment #14)
>
> According bug 713933, security.ssl.false_start.require_forward_secrecy has
> to be true by default.  Or I've missed explanation why not.
>

It is intentional that the NSS default and the PSM default don't match. NSS is set to the more conservative value (to require forward secrecy) and PSM (with the necko pref) is intentionally overriding that. As written this would be a difference between chrome and firefox.

Last time I talked about it with bsmith he was largely supportive but not fully decided yet.. but we can certainly get started with it on moz-central in this way.

Comment 16

[Wan-Teh Chang]

Comment on attachment 681958 [details] [diff] [review]
patch 0

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

This patch seems fine. I didn't review the changes to nsNSSIOLayer.cpp carefully.

::: netwerk/base/public/security-prefs.js
@@ +13,5 @@
>  pref("security.ssl.require_safe_negotiation",  false);
>  pref("security.ssl.warn_missing_rfc5746",  1);
> +pref("security.ssl.enable_false_start", true);
> +pref("security.ssl.false_start.require_npn", true);
> +pref("security.ssl.false_start.require_forward_secrecy", false);

The default value of security.ssl.false_start.require_forward_secrecy does not
match the default value of SSL_FALSE_START_REQUIRES_FORWARD_SECRECY in your
patch in bug 681956. Why do you not want to be "secure by default" here?

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +250,1 @@
>      return NS_OK;

I don't understand these two complicated Boolean expressions, especially the
second one. It would be nice to add comments to explain what you are checking.

Comment 17

[Patrick McManus [:mcmanus]]

Thanks Wan-Teh!

(In reply to Wan-Teh Chang from comment #16)
> Comment on attachment 681958 [details] [diff] [review]
> patch 0
> 
> Review of attachment 681958 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> The default value of security.ssl.false_start.require_forward_secrecy does
> not
> match the default value of SSL_FALSE_START_REQUIRES_FORWARD_SECRECY in your
> patch in bug 681956. Why do you not want to be "secure by default" here?
>

see comment 15 - gecko is intentionally loosening this preference while leaving the nss implementation in a more conservative by default state.

I'm ok with loosening it based on the rough argument that the exposure here is a downgrade to another one of the key exchanges that we consider acceptable anyhow. Attacks to force a downgrade to SSL3 already exist in the same space as far as I understand it. 

But its appropriate for gecko to make that determination and for NSS to operate by default requiring the highest level of kea.
 
> ::: security/manager/ssl/src/nsNSSIOLayer.cpp
> @@ +250,1 @@
> >      return NS_OK;
> 
> I don't understand these two complicated Boolean expressions, especially the
> second one. It would be nice to add comments to explain what you are
> checking.

Sure - I'll add some comments. The code is just checking to make sure that the entire traditional handshake is done (including cert verification) before it will run the spdy coalsescing check. The spdy check says "can I carry traffic for host b.foo.com over this established connection to a.foo.com because it has a cert that covers them both and they resolve to the same address".. and the logic here is making sure that we don't say "OK" to that unless a.foo.com is 100% completely setup.. that's something brian requested.

Comment 18

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

Comment on attachment 681958 [details] [diff] [review]
patch 0

(In reply to Patrick McManus [:mcmanus] from comment #17)
> see comment 15 - gecko is intentionally loosening this preference while
> leaving the nss implementation in a more conservative by default state.

This hasn't been decided, such as this. I'm still undecided about it and Wan-Teh seems to disagree. (And note that Wan-Teh is a peer on this module.)

> I'm ok with loosening it based on the rough argument that the exposure here
> is a downgrade to another one of the key exchanges that we consider
> acceptable anyhow. Attacks to force a downgrade to SSL3 already exist in the
> same space as far as I understand it. 

We know there are some sites/networks that cannot tolerate False Start, based on reports from Google. Patrick, what is the observable behavior as perceived by Gecko when this happens? Timeout? Connection reset? Handshake corruption? 

In particular, I am wondering if we can clearly distinguish between TLS intolerance and False Start intolerance. We should be careful to avoid falling back to SSL 3 due to False Start intolerance.

It seems like if we're not going to effectively require the site to opt in to False Start (which is what the current NPN+(EC)DHE requirement in libssl does), then we should be prepared to fall back to no false start if/when we can detect likely intolerance.

If we decide to disable some of libssl's compatibility measures, we should make sure release-drivers are aware of the increased compatibility risk, as we did with similarly risky things like disabling MD5-based certificates and implementing the BEAST attack workaround.

There is a major change in behavior when False Start is enabled: HandshakeCallback() gets called before the integrity of the handshake has been verified. That means, besides looking at the content of this patch, we need to carefully analyze the HandshakeCallback function:

> void HandshakeCallback(PRFileDesc* fd, void* client_data) {

>   // If the handshake completed, then we know the site is TLS tolerant
>   // (if this was a TLS connection).
>   nsSSLIOLayerHelpers::rememberTolerantSite(infoObject);

This seems like something that must happen after the Finished message has been received.

>   int32_t secStatus;
>   if (sslStatus == SSL_SECURITY_STATUS_OFF)
>     secStatus = nsIWebProgressListener::STATE_IS_BROKEN;
>   else
>     secStatus = nsIWebProgressListener::STATE_IS_SECURE
>               | nsIWebProgressListener::STATE_SECURE_HIGH;

These flags control the security indicators in the address bar. What is the expected behavior here? Off the top of my head, it seems like we should wait to do this until the handshake has actually completed. However, there is some risk that something at higher layers may not work correctly if these flags are not set before other events related to application data are received. Though, most of the existing code that looks at these flags are only thinking about *received* data, so it should be OK.

>   PRBool siteSupportsSafeRenego;
>   if (SSL_HandshakeNegotiatedExtension(fd, ssl_renegotiation_info_xtn, &siteSupportsSafeRenego) != SECSuccess
>       || !siteSupportsSafeRenego) {
> 
>     bool wantWarning = (nsSSLIOLayerHelpers::getWarnLevelMissingRFC5746() > 
>
>     if (nsSSLIOLayerHelpers::treatUnsafeNegotiationAsBroken()) {
>       secStatus = nsIWebProgressListener::STATE_IS_BROKEN;
>     }
>   }

First, I wonder why libssl doesn't treat support for the reneogitation extension as a prerequisite for false start? Perhaps we should change it to do so.

But, also, we need to either move this to be after we've received the Finished message, or (better) at least verify that SSL_HandshakeNegotiatedExtension returns the correct value here during a False Start. IIRC, the Googlers experienced some difficulty with some functions like SSL_HandshakeNegotiatedExtension only returning correct values after the handshake was complete.
 
>     if (SSL_GetNextProto(fd, &state, npnbuf, &npnlen, 256) == SECSuccess) {
>       if (state == SSL_NEXT_PROTO_NEGOTIATED)
>         infoObject->SetNegotiatedNPN(reinterpret_cast<char *>(npnbuf), npnlen);
>       else
>         infoObject->SetNegotiatedNPN(nullptr, 0);
>       mozilla::Telemetry::Accumulate(Telemetry::SSL_NPN_TYPE, state);
>     }
>     else
>       infoObject->SetNegotiatedNPN(nullptr, 0);

Similar to the previous comment, we need to make sure this is working. IIRC, Wan-Teh has a libssl patch pending about getting this to work correctly; I am not sure if it has been checked in.

>     SSLChannelInfo channelInfo;
>     if (SSL_GetChannelInfo(fd, &channelInfo, sizeof(channelInfo)) == SECSuccess) {
> 
>       SSLCipherSuiteInfo cipherInfo;
>       if (SSL_GetCipherSuiteInfo(channelInfo.cipherSuite, &cipherInfo,
>                                  sizeof (cipherInfo)) == SECSuccess) {

Same deal as the previous two comments.

>     infoObject->SetHandshakeCompleted();

On one hand, the name clearly indicates that this should be called after the handshake has actually finished. On the other hand, this function contains the telemetry that is supposed to measure the performance benefits of False Start, so *that* part cannot be deferred until the handshake actually finished.

In the libssl bug, I suggested that we add a new callback that gets called after the handshake has really finished. I think that is what we should do, and then use that new callback to address the above issues.

Comment 19

[Patrick McManus [:mcmanus]]

(In reply to Brian Smith (:bsmith) from comment #18)
> Comment on attachment 681958 [details] [diff] [review]
> patch 0
> 
> (In reply to Patrick McManus [:mcmanus] from comment #17)
> > see comment 15 - gecko is intentionally loosening this preference while
> > leaving the nss implementation in a more conservative by default state.
> 
> This hasn't been decided, such as this. I'm still undecided about it and
> Wan-Teh seems to disagree. (And note that Wan-Teh is a peer on this module.)
> 

I apologize if there was any confusion here. My statement was about the relationship between the nss and gecko patches because a couple people noted the different behavior between them. The gecko patch is intentionally loosening the preference - I didn't mean to imply it was policy or a done deal or anything. Just that the patch was coded as intended and why.

Comment 20

[Patrick McManus [:mcmanus]]

(In reply to Brian Smith (:bsmith) from comment #18)

> We know there are some sites/networks that cannot tolerate False Start,
> based on reports from Google. 

I don't know of any sites that use NPN and are intolerant to False Start. That includes my surveys of googlers. It of course is possible my question wasn't clear or posed to the right people.

we can watch our SSL3/TLS1 ratio.

> 
> It seems like if we're not going to effectively require the site to opt in
> to False Start (which is what the current NPN+(EC)DHE requirement in libssl
> does), then we should be prepared to fall back to no false start if/when we
> can detect likely intolerance.

I actually don't see the current requirement in libssl for NPN - I believe I added that. But I do understand that to be chrome's policy. Perhaps something was lost in translation.

>  
> >     if (SSL_GetNextProto(fd, &state, npnbuf, &npnlen, 256) == SECSuccess) {

> 
> Similar to the previous comment, we need to make sure this is working. IIRC,
> Wan-Teh has a libssl patch pending about getting this to work correctly; I
> am not sure if it has been checked in.

This works in my testing. A link to any patches we need to get merged would be appreciated.

> On one hand, the name clearly indicates that this should be called after the
> handshake has actually finished. On the other hand, this function contains
> the telemetry that is supposed to measure the performance benefits of False
> Start, so *that* part cannot be deferred until the handshake actually
> finished.
> 
> In the libssl bug, I suggested that we add a new callback that gets called
> after the handshake has really finished. I think that is what we should do,
> and then use that new callback to address the above issues.

we can move most of it.. the telem on "READY" as well as the NextProto stuff being exceptions needed by the sending side of things.

Comment 21

[Wan-Teh Chang]

(In reply to Brian Smith (:bsmith) from comment #18)
>
> We know there are some sites/networks that cannot tolerate False Start,
> based on reports from Google. Patrick, what is the observable behavior as
> perceived by Gecko when this happens? Timeout? Connection reset? Handshake
> corruption?

Unfortunately a common behavior of a False Start intolerant server is
timeout. This is why in general a client can't detect it quickly and
retry with False Start turned off.

Comment 22

[Patrick McManus [:mcmanus]]

Attachment: patch 1

I've verified that this gives firefox False-Start with twitter and facebook and google (all running NPN), but not with paypal (not running NPN). 

I've also repeated those tests with the require-false-secrecy option set to true and the set is reduced to google.com.

Comment 23

[Patrick McManus [:mcmanus]]

Attachment: patch v2

This version of the patch makes the CanFalseStart() determination one time, generally during the gather handshake logic. Subsequent callsites that need that information access a cached value of what CanFalseStart() returned.

A CanFalseStart callback function is introduced. If set by the application this callback is invoked instead of using the existing algorithm embedded in NSS. If it isn't set the existing algorithm is used.

The HandshakeCallback now only happens when the handshake is fully complete by its traditional definition. Applications looking for a false start indication should use the CanFalseStart callback but they can now be confident the entire handshake exchange has completed when HandshakeCallback is invoked.

No actual application sends (which are what really define false start) happen while asynchronous certificate validation is outstanding.

Comment 24

[Patrick McManus [:mcmanus]]

Attachment: patch v2

[ please disregard comment 23. It does not apply to this bug (it was typed in the wrong tab)]

Set the NSS False Start Pref to true for Mozilla Necko.

Implement the NSS CanFalseStart callback to require NPN (as a signal of a modern TLS stack) and either a forward-secret KeyExchange or (RSA and historical evidence that the host has completed a full handshake with RSA in the recent past)

Comment 25

[Patrick McManus [:mcmanus]]

Attachment: patch v3

Comment 26

[Patrick McManus [:mcmanus]]

Attachment: patch v4

updated to reflect a sidebar conversation where we should limit any false start with RC4 to hosts that have previously completed a handshake using RC4.

Comment 27

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

Comment on attachment 725806 [details] [diff] [review]
patch v4

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

Looks very good to me.

As always, deal with the trailing whitespace.

In general, it looks like all the state-machine-related stuff is right and the interpretation of the prefs is also right, except we should add a TLS 1.0 check.

I want to look at the updated patch again. I will try to respond faster than previously.

::: netwerk/base/public/security-prefs.js
@@ +13,5 @@
>  pref("security.ssl.require_safe_negotiation",  false);
>  pref("security.ssl.warn_missing_rfc5746",  1);
> +pref("security.ssl.enable_false_start", true);
> +pref("security.ssl.false_start.require-npn", true);
> +pref("security.ssl.false_start.require-forward-secrecy", false);

Not sure that the two require-* options are necessary, but I don't mind having them either.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +1226,5 @@
> +    // set a permission manager flag that future transactions can
> +    // use via SetSSLFlags(()
> +
> +    nsCOMPtr<nsIPermissionManager> permMgr =
> +        do_GetService(NS_PERMISSIONMANAGER_CONTRACTID);

I am feeling a little dumb: how does this work with per-window private browsing?

::: netwerk/socket/nsISSLSocketControl.idl
@@ +58,5 @@
> +
> +    readonly attribute uint16_t KEAUsed;
> +    attribute uint16_t KEAExpected;
> +    readonly attribute uint16_t SymmetricCipherUsed;
> +    attribute uint16_t SymmetricCipherExpected;

These should all be marked with [infallible] so that you can use:

  X x = x->GetXXX();

instead of :

  X x;
  nsresult rv = GetXXX(&x);
  NS_ENSURE_SUCCESS(rv, rv);

Please update the calls to these methods to make use of the clearer infallible forms.

@@ +60,5 @@
> +    attribute uint16_t KEAExpected;
> +    readonly attribute uint16_t SymmetricCipherUsed;
> +    attribute uint16_t SymmetricCipherExpected;
> +
> +    const long KEY_EXCHANGE_UNKNOWN  = 0;

NULL doesn't mean "UNKNOWN", it means that there is no cryptographic key exchange done, which is possible with some cipher suites that libssl supports, but that we don't enable by default. IMO, the UNKNOWN constant should not be zero, but instead a separate value from null like -1.

Also, do we really need to name all these constants? Can't we just use the names in sslt.h directly? It seems like the only name we really need to add is UNKNOWN. Otherwise, add a comment that these constants must correspond to SSLKEAType in sslt.h.

@@ +66,5 @@
> +    const long KEY_EXCHANGE_DH       = 2;
> +    const long KEY_EXCHANGE_FORTEZZA = 3;
> +    const long KEY_EXCHANGE_ECDH     = 4;
> +
> +    const long SYMMETRIC_CIPHER_UNKNOWN  = 0;

Same comments apply here. ssl_calg_null means that no encryption is done, which is different than "unknown". Also, I think that using the names from sslt.h makes more sense than minting new names.

::: security/manager/ssl/src/nsNSSCallbacks.cpp
@@ +845,5 @@
> +  if (SSL_GetNextProto(fd, &state, npnbuf, &npnlen, 256) == SECSuccess) {
> +    if (state == SSL_NEXT_PROTO_NEGOTIATED)
> +      infoObject->SetNegotiatedNPN(reinterpret_cast<char *>(npnbuf), npnlen);
> +    else
> +      infoObject->SetNegotiatedNPN(nullptr, 0);

Use braces in the if and else.

@@ +848,5 @@
> +    else
> +      infoObject->SetNegotiatedNPN(nullptr, 0);
> +    mozilla::Telemetry::Accumulate(Telemetry::SSL_NPN_TYPE, state);
> +  }
> +  else

use braces

@@ +854,5 @@
> +}
> +
> +SECStatus
> +CanFalseStartCallback(PRFileDesc* fd, void* client_data,
> +                      PRBool *canFalseStart)

Nit: this looks like it can fit all on one line without going over 80 chars.

@@ +856,5 @@
> +SECStatus
> +CanFalseStartCallback(PRFileDesc* fd, void* client_data,
> +                      PRBool *canFalseStart)
> +{
> +  *canFalseStart = PR_FALSE;

s/PR_FALSE/false/.

Only use NSPR types when interaction with pointer-to-NSPR-type requires it, and never use PR_FALSE or PR_TRUE, except in NSS and NSPR.

@@ +860,5 @@
> +  *canFalseStart = PR_FALSE;
> +
> +  nsNSSShutDownPreventionLock locker;
> +  nsNSSSocketInfo* infoObject = (nsNSSSocketInfo*) fd->higher->secret;
> +  if (!infoObject)

You need to always call PR_SetError() when you return SECFailure, except immediately after a function call that failed with SECFailure (because that function call already called PR_SetError()).

You can use PR_INVALID_STATE_ERROR as the error code.

@@ +869,5 @@
> +  SSLChannelInfo channelInfo;
> +  if (SSL_GetChannelInfo(fd, &channelInfo, sizeof(channelInfo)) != SECSuccess) {
> +    PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("CanFalseStartCallback [%p] failed - "
> +                                      "SSL_GetChannelInfo\n", fd));
> +    return SECSuccess;

If SSL_GetChannelInfo fails, it is probably because of OOM or because of some state machine problem. So, I think we should return SECFailure here.

Nit: SSL_GetChannelInfo should almost never fail, so I don't think the logging will be useful. IMO better to remove it.

@@ +871,5 @@
> +    PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("CanFalseStartCallback [%p] failed - "
> +                                      "SSL_GetChannelInfo\n", fd));
> +    return SECSuccess;
> +  }
> +

We should also check the TLS version to ensure that the TLS version is >= TLS 1.0. This is implicit in the NPN check, but the NPN check is optional.

Please file a follow-up bug blocking the bug to enable TLS 1.1 to fix this to use similar logic that we use for cipher suites to prevent downgrade attacks from TLS 1.1 -> TLS 1.0 when we add TLS 1.1 support.

We should never do false start for SSL 3.0 because some servers negotiate different cipher suites for SSL 3.0 connections than they negotiate for TLS 1.0 and later. (In particular, some servers refuse to use AES cipher suites or ECDHE cipher suites unless the protocol version is TLS 1.0 or later.)

@@ +877,5 @@
> +  if (SSL_GetCipherSuiteInfo(channelInfo.cipherSuite, &cipherInfo,
> +                             sizeof (cipherInfo)) != SECSuccess) {
> +    PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("CanFalseStartCallback [%p] failed - "
> +                                      "SSL_GetCipherSuiteInfo\n", fd));
> +    return SECSuccess;

Same comments as above with SSL_GetChannelInfo. We should return SECFailure and the logging is, IMO, unnecessary.

@@ +891,5 @@
> +  }
> +
> +  // never do false start without at least 80 bits of key material. This should
> +  // be redundant to an NSS precondition
> +  if (cipherInfo.effectiveKeyBits < 80) {

MOZ_NOT_REACHED("NSS is not enforcing the precondition that the effective key size must be >= 80 bits for false start");

@@ +898,5 @@
> +                                      cipherInfo.effectiveKeyBits));
> +    return SECSuccess;
> +  }
> +
> +  // Enforce NPN to do false start if policy requires it

Somewhere, we should have an explanation in the code about why we have this "require NPN" flag, NPN isn't necessary (or helpful) for the security properties for false start. Instead, we're limiting false start to NPN-enabled servers just for compatibility reasons. I think that this is a good place to document that.

In the future, we should try to limit the potential for the attacker to choose the protocol in the same way we limit the attacker's ability to choose the cipher suite. Please add a comment like this, to that affect:

// XXX: An attacker can choose which protocols are advertised in the
// NPN extension. TODO(Bug XXXXXX): We should restrict the ability
// of an attacker leverage this capability by restricting false start
// to the same protocol we previously saw for the server, after the
// first successful connection to the server.

@@ +911,5 @@
> +    }
> +  }
> +
> +  // If we're not using eliptical curve kea then make sure we've seen the
> +  // same kea from this host in the past.

s/past./past, to limit the potential for downgrade attacks./

@@ +914,5 @@
> +  // If we're not using eliptical curve kea then make sure we've seen the
> +  // same kea from this host in the past.
> +  if (cipherInfo.keaType != ssl_kea_ecdh) {
> +    uint16_t expected = 0;
> +    if (NS_FAILED(infoObject->GetKEAExpected(&expected)) ||

In addition to checking that expected == unknown, we should whitelist the KEAs that we want to allow false start for.

In particular, the way things are currently written, you also need to check that expected != ssl_kea_null, because ssl_kea_null == UNKNOWN == 0 is a possible value from libssl (though, we never enable any cipher suites with null KEA currently).

IMO, that whitelist should be:

    ssl_kea_rsa      = 1,
    ssl_kea_dh       = 2,
    ssl_kea_ecdh     = 4,

In particular, if we change the whitelist of algorithms in the future, then this explciit check will allow us to avoid writing code to delete persistent state that remembered that removed algorithms were previously negotiated for the host.

@@ +924,5 @@
> +    }
> +  }
> +
> +  // If we're not using AES then verify that this is the historically expected
> +  // symmetrical cipher for this host.

s/host./host, to limit the potential for downgrade attacks./

@@ +925,5 @@
> +  }
> +
> +  // If we're not using AES then verify that this is the historically expected
> +  // symmetrical cipher for this host.
> +  if(cipherInfo.symCipher != ssl_calg_aes) {

nit: whitespace.

@@ +928,5 @@
> +  // symmetrical cipher for this host.
> +  if(cipherInfo.symCipher != ssl_calg_aes) {
> +    uint16_t expected = 0;
> +    if (NS_FAILED(infoObject->GetSymmetricCipherExpected(&expected)) ||
> +        cipherInfo.symCipher != expected) {

The same concerns apply here. We should explicitly whitelist the algorithms that we want to enable for false start, and in particular we should avoid allowing false start for ssl_calg_null.

IMO, that whitelist should be:

    ssl_calg_rc4      = 1,
    ssl_calg_3des     = 4,
    ssl_calg_aes      = 7,

In particular, if we change the whitelist of algorithms in the future, then this explciit check will allow us to avoid writing code to delete persistent state that remembered that removed algorithms were previously negotiated for the host.

@@ +936,5 @@
> +      return SECSuccess;
> +    }
> +  }
> +
> +  *canFalseStart = PR_TRUE;

s/PR_TRUE/true/

@@ +939,5 @@
> +
> +  *canFalseStart = PR_TRUE;
> +  PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("CanFalseStartCallback [%p] ok\n", fd));
> +  return SECSuccess;
> +}

Is is worth doing telemetry for how often we false start vs. how often we get asked about false start vs. not asked, but that can be done in a follow-up.

@@ +951,5 @@
>    int32_t keyLength;
>    nsresult rv;
>    int32_t encryptBits;
>  
> +  // If the false start stage was skipped catch up with the bookkeeping that

s/skipped catch/skipped, catch/

::: security/manager/ssl/src/nsNSSCallbacks.h
@@ +15,5 @@
>  #include "mozilla/CondVar.h"
>  #include "mozilla/Mutex.h"
>  #include "mozilla/Attributes.h"
>  #include "nsString.h"
> +#include "ssl.h"

this should be sslt.h

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +87,5 @@
> +    mFirstWriteReceived(false),
> +    mKEAUsed(0),
> +    mKEAExpected(0),
> +    mSymmetricCipherUsed(0),
> +    mSymmetricCipherExpected(0),

Use the named UNKNOWN constants you created, because 0 == no encryption which means something different than UNKNOWN.

@@ +236,5 @@
>    }
>  }
>  
>  void
> +nsNSSSocketInfo::NoteConnectionReady()

Please rename this to be more specific: e.g. NoteApplicationDataSent. In particular, the connection isn't (necessarily) ready to receive data and also this method isn't called until we've already sent data, which is slightly different than being just ready to send data.

Comment 28

[Patrick McManus [:mcmanus]]

> ::: netwerk/protocol/http/nsHttpChannel.cpp
> @@ +1226,5 @@
> > +    // set a permission manager flag that future transactions can
> > +    // use via SetSSLFlags(()
> > +
> > +    nsCOMPtr<nsIPermissionManager> permMgr =
> > +        do_GetService(NS_PERMISSIONMANAGER_CONTRACTID);
> 
> [..] how does this work with per-window private
> browsing?


it didn't.. thanks for the catch. I forgot it needs to be managed per-channel now. I added a private browsing check before doing any nsIPermissionManager->Add().. which is slightly suboptimal.

Comment 29

[Patrick McManus [:mcmanus]]

Attachment: patch v5

Comment 30

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

Comment on attachment 736924 [details] [diff] [review]
patch v5

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

It is a little unfortunate that so much of this logic is in nsHttpChannel, because that means that mozTCPSocket will not be able to take advantage of the same logic. But, I wouldn't worry about that now; let's just file a follow-up bug about that. (mozTCPSocket should be able to use NPN too, and that is a similar problem.)

Unfortunately, I got a little burned out staring at this patch, so I doubt I caught everything. The main major issue I see is the interaction with private browsing. Please check with ehsan/mconnor about how important it is for the initial version of this patch to support false start in private browsing mode to meet the requirement that private browsing mode must work as much like the normal browsing mode as possible. If it is not necessary, then please file the follow-up bug and CC me.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +401,5 @@
>      if (!net_IsValidHostName(nsDependentCString(mConnectionInfo->Host())))
>          return NS_ERROR_UNKNOWN_HOST;
>  
>      // Consider opening a TCP connection right away
> +    SetSSLFlags();

I suggest "RetrieveSSLOptions" so that SetupNPN can be renamed to "SetSSLOptions". "SetSSLFlags" sounds like a function that does what SetupNPN does.

@@ +711,5 @@
> +{
> +    if (!IsHTTPS())
> +        return;
> +
> +    nsCOMPtr<nsIPermissionManager> permMgr =

We shouldn't do this in private browsing mode, right?

@@ +722,5 @@
> +        NS_FAILED(baseURI->SetPath(NS_LITERAL_CSTRING("/"))))
> +        return;
> +
> +    uint32_t perm;
> +    nsresult rv = permMgr->TestExactPermission(baseURI, "falsestart-rsa", &perm);

Shouldn't we be using testExactPermissionFromPrincipal? Generally, especially for security-sensitive things, we are supposed to be using principals instead of constructing URLs--especially in order to correctly support B2G apps, which have "extended principals" (which take into account the app-id) on which their permissions are keyed.

@@ +1221,5 @@
> +    // do the same for DH but its rarity doesn't justify the lookup.
> +    // Also do the same for RC4 symmetric ciphers.
> +
> +    if (mCanceled || NS_FAILED(mStatus) || !mSecurityInfo ||
> +        !IsHTTPS() || mPrivateBrowsing)

We should also be using the the criteria used in nsStrictTransportSecurityService::ShouldIgnoreStsHeader so that a cert override (e.g. for a captive portal) doesn't affect the (permanently-)stored values of these flags.

We should generally have the same behaviors during private browsing that we have outside of private browsing. This is an important property of private browsing, to make it (somewhat) difficult to tell whether a user is in private browsing mode. However, I see from bug 557598 and other features like the cert override service that it seems like significant work to properly do this; other features that try to do this generally maintain a separate in-memory database outside of permission manager just for private browsing mode. That sounds a little sucky but I don't know what alternative there is.

Howe important is it to maintain this symmetry between private browsing mode and non-private browsing mode? Is it acceptable to address that in a follow-up bug? Or, if it is not a lot of work, perhaps we can do it in an add-on patch to this bug, based on the technique that nsCertOverrideService and nsStrictTransportSecurityService use.

@@ +1231,5 @@
> +    if (!ssl)
> +        return;
> +
> +    ssl->GetKEAUsed(&kea);
> +    ssl->GetSymmetricCipherUsed(&symcipher);

Either use the infallible versions of these APIs or check the return value. Otherwise, kea and/or symcipher could be uninitialized.

@@ +1251,5 @@
> +        return;
> +
> +    // Allow this to stand for a week
> +    int64_t expireTime = (PR_Now() / PR_USEC_PER_MSEC) +
> +        (86400 * 7 * PR_MSEC_PER_SEC);

Use a named variable like:

static const int64_t SECONDS_PER_DAY = 24 * 60 * 60;

@@ +1255,5 @@
> +        (86400 * 7 * PR_MSEC_PER_SEC);
> +
> +    if (kea == ssl_kea_rsa) {
> +        permMgr->Add(baseURI, "falsestart-rsa", nsIPermissionManager::ALLOW_ACTION,
> +                     nsIPermissionManager::EXPIRE_TIME, expireTime);

Again, we should be using the principal-based forms for these calls.

@@ +1258,5 @@
> +        permMgr->Add(baseURI, "falsestart-rsa", nsIPermissionManager::ALLOW_ACTION,
> +                     nsIPermissionManager::EXPIRE_TIME, expireTime);
> +        LOG(("nsHttpChannel::ProcessSSLInformation [this=%p] "
> +             "falsestart-rsa permission granted for this host\n", this));
> +    }

else we should remove the permission, to handle the case where the server disabled RSA for some security reason.

@@ +1265,5 @@
> +        permMgr->Add(baseURI, "falsestart-rc4", nsIPermissionManager::ALLOW_ACTION,
> +                     nsIPermissionManager::EXPIRE_TIME, expireTime);
> +        LOG(("nsHttpChannel::ProcessSSLInformation [this=%p] "
> +             "falsestart-rc4 permission granted for this host\n", this));
> +    }

Ditto here: else remove the permission.

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ +360,5 @@
>      return rv;
>  }
>  
>  void
>  nsHttpConnection::SetupNPN(uint32_t caps)

The call to this method has a comment "SetupNPN(caps); // only for spdy" but that comment seems to now be out-of-date, at least with respect to this change. Also, it seems like the name should be something like SetSSLParameters (something symmetric with the name of the function that retrieves the preferences and stores their values in mCaps) since it does more than NPN.

@@ +387,5 @@
>                  do_QueryInterface(securityInfo, &rv);
>              if (NS_FAILED(rv))
>                  return;
>  
> +            if (caps & NS_HTTP_ALLOW_RSA_FALSESTART)

Why is this done here, instead of above the "// Setup NPN Negotiation if necessary (only for SPDY)" line? At least with the preferences you implemented, NPN and false start are somewhat orthogonal.

::: netwerk/protocol/http/nsHttpConnectionMgr.h
@@ +106,5 @@
>      // connection manager, nor is the submitter obligated to actually submit a
>      // real transaction for this connectionInfo.
>      nsresult SpeculativeConnect(nsHttpConnectionInfo *,
> +                                nsIInterfaceRequestor *,
> +                                uint32_t caps = 0);

It seems wrong to add this caps parameter to SpeculativeConnect. Instead, shouldn't SpeculativeConnect should calculate the values of the caps itself. Otherwise, all the other callers of SpeculativeConnect will not get the benefits of false start, but I think that would be bad--especially awesomebar/searchbar.

::: security/manager/ssl/src/nsNSSCallbacks.cpp
@@ +860,5 @@
> +CanFalseStartCallback(PRFileDesc* fd, void* client_data, PRBool *canFalseStart)
> +{
> +  *canFalseStart = false;
> +
> +  nsNSSShutDownPreventionLock locker;

Whenever you acquire the shutdown prevention lock, you need to make sure that NSS has not been shut down already. i.e. check infoObject->isAlreadyShutDown() and:

   MOZ_NOT_REACHED("SSL socket used after NSS shut down");
   PORT_SetError(PR_INVALID_STATE_ERROR, 0);
   return SECFailure;

Eventually, we can take as a precondition that SSL callbacks would never get called if NSS was shut down, because this case is a serious bug in Gecko. (Similarly changes need to be made to HandshakeCallback).

@@ +871,5 @@
> +  PreliminaryHandshakeDone(fd);
> +
> +  SSLChannelInfo channelInfo;
> +  if (SSL_GetChannelInfo(fd, &channelInfo, sizeof(channelInfo)) != SECSuccess) {
> +    return SECFailure;

Why return SECFailure here and for SSL_GetCipherSuiteInfo but return SECSuccess in the other cases?

@@ +883,5 @@
> +
> +  if (channelInfo.protocolVersion < SSL_LIBRARY_VERSION_TLS_1_0) {
> +    PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("CanFalseStartCallback [%p] failed - "
> +                                      "SSL Version must be >= TLS1 %x\n", fd,
> +                                      channelInfo.protocolVersion));

When using PR_LOG() with integer values and formats like %x and %d, do we need static_cast<int>() or static_cast<int32_t>() around the values to avoid badness? See bug 277394 comment 9 and adjust the patch accordingly, if necessary. In particular, according to that comment, 16-bit values need "%hx" or "%hd" instead of "%x" or "%d". (Personally, I tend to static_cast<int>(x) every value I use for "%x" and "%d" to avoid this issue, which I find to be extremely confusing.)

@@ +904,5 @@
> +                    "key size must be >= 80 bits for false start");
> +    PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("CanFalseStartCallback [%p] failed - "
> +                                      "key too small %d\n", fd,
> +                                      cipherInfo.effectiveKeyBits));
> +    return SECSuccess;

PR_SetError(PR_INVALID_STATE_ERROR, 0);
return SECFailure;

@@ +951,5 @@
> +
> +    // whitelist the expected key exchange algorithms that are 
> +    // acceptable for false start when seen before.
> +    if ((expected != ssl_kea_rsa) && (expected != ssl_kea_dh) &&
> +        (expected != ssl_kea_ecdh)) {

Nit: please remove the unnecessary parentheses, and fix the trailing whitespace in the comment.

@@ +955,5 @@
> +        (expected != ssl_kea_ecdh)) {
> +      PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("CanFalseStartCallback [%p] failed - "
> +                                        "KEA used, %d, "
> +                                        "is not supported with False Start.\n",
> +                                        fd, expected));

static_cast<int32_t>(expected).

@@ +965,5 @@
> +  // symmetrical cipher for this host, to limit potential for downgrade attacks.
> +  if (cipherInfo.symCipher != ssl_calg_aes) {
> +    int16_t expected = 0;
> +
> +    infoObject->GetSymmetricCipherExpected(&expected);

Use infallible form or check return value.

@@ +994,2 @@
>  void HandshakeCallback(PRFileDesc* fd, void* client_data) {
>    nsNSSShutDownPreventionLock locker;

:( Here's the bad example that you were emulating, I guess. :( If you don't want to fix this instance (which is harder, because you can't return SECfailure from this function), then let's file a follow-up bug.

@@ +1008,4 @@
>    bool isResumedSession = !(infoObject->GetFirstServerHelloReceived());
>  
> +  // If the false start stage was skipped, catch up with the bookkeeping that
> +  // would have been done at that point.

The comment suggests that this function won't get called during the false start case. I suggest "Do the bookkeeping that needs to be done after the server's ServerHello...ServerHelloDone have been processed, but that doesn't need the handshake to be completed."

::: security/manager/ssl/src/nsNSSCallbacks.h
@@ +15,5 @@
>  #include "mozilla/CondVar.h"
>  #include "mozilla/Mutex.h"
>  #include "mozilla/Attributes.h"
>  #include "nsString.h"
> +#include "sslt.h"

This included isn't needed any more.

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +237,5 @@
>    }
>  }
>  
>  void
> +nsNSSSocketInfo::NoteApplicationDataSent()

Please rename this to NoteTimeUntilReady because that is what this is noting; in particular, the socket could become ready without us writing anything to it (e.g. preconnect).

@@ +239,5 @@
>  
>  void
> +nsNSSSocketInfo::NoteApplicationDataSent()
> +{
> +  if (mFirstWriteReceived)

Similarly, rename this to mNotedTimeUntilReady

@@ +240,5 @@
>  void
> +nsNSSSocketInfo::NoteApplicationDataSent()
> +{
> +  if (mFirstWriteReceived)
> +    return;

blank line here.

@@ +247,5 @@
> +  // This will include TCP and proxy tunnel wait time
> +  Telemetry::AccumulateTimeDelta(Telemetry::SSL_TIME_UNTIL_READY,
> +                                 mSocketCreationTimestamp, TimeStamp::Now());
> +  PR_LOG(gPIPNSSLog, PR_LOG_DEBUG,
> +         ("[%p] nsNSSSocketInfo::NoteApplicationDataSent\n", (void*)mFd));

Not necessary to cast to (void*), is it?

Also, log with the new function name.

@@ +276,5 @@
>      mHandshakeCompleted = true;
> +
> +    PR_LOG(gPIPNSSLog, PR_LOG_DEBUG,
> +           ("[%p] nsNSSSocketInfo::SetHandshakeCompleted\n", (void*)mFd));
> +    NoteApplicationDataSent();

This should be done in HandshakeCallback() to be symmetric with CanFalseStartCallback.

@@ +1266,5 @@
>    PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("[%p] wrote %d bytes\n",
>           fd, bytesWritten));
>  
> +  if (bytesWritten > 0)
> +    socketInfo->NoteApplicationDataSent();

This should be done only in CanFalseStartCallback and HandshakeCallback. Otherwise, we will not get an accurate reading in the cases where we have connected a socket but haven't tried to write to it (e.g. speculative connect, or mozTCPSocket where the first operation is a read instead of a write).

Comment 31

[Honza Bambas (:mayhemer)]

(In reply to Brian Smith (:bsmith) from comment #30)
> Comment on attachment 736924 [details] [diff] [review]
> patch v5
> 
> Review of attachment 736924 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It is a little unfortunate that so much of this logic is in nsHttpChannel,
> because that means that mozTCPSocket will not be able to take advantage of
> the same logic. But, I wouldn't worry about that now; let's just file a
> follow-up bug about that. (mozTCPSocket should be able to use NPN too, and
> that is a similar problem.)

Bug 715905?

Comment 32

[Patrick McManus [:mcmanus]]

re: using principals with the permission manager.. this seems like a good idea on balance, but nsipermissionmanager->Test() with a principal with a unknown app id results in an assert.. safe browsing fetches generate principals with unknown app ids (and I'm sure other things do too). I'll file a separate bug about this (done.. 878886), but for now I've just got a workaround to not do the permission manager work with these principals (so they won't be able to use rc4 or rsa)

> private browsing. Please check with ehsan/mconnor about how important it is

I have an email out about this. let's proceed in parallel for the moment.


> ::: netwerk/protocol/http/nsHttpConnectionMgr.h

> Otherwise, all the other callers of SpeculativeConnect will not get the
> benefits of false start, but I think that would be bad--especially
> awesomebar/searchbar.

unfortunately general uses of speculative connect don't really know if they are in private browsing mode.. there is no channel or principal.. we could implement something as a follow on through some kind of nsIInterfaceRequestor trail of tears or just an alternate interface. Technically they can still false start - just not in the presence of RC4 or RSA.. so hopefully at least the searchbar will shortly meet that criteria.

Comment 33

[Patrick McManus [:mcmanus]]

Attachment: Enable TLS False Start (PSM)

Comment 34

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

(In reply to Patrick McManus [:mcmanus] from comment #32)
> re: using principals with the permission manager.. this seems like a good
> idea on balance, but nsipermissionmanager->Test() with a principal with a
> unknown app id results in an assert.. safe browsing fetches generate
> principals with unknown app ids (and I'm sure other things do too). I'll
> file a separate bug about this (done.. 878886), but for now I've just got a
> workaround to not do the permission manager work with these principals (so
> they won't be able to use rc4 or rsa)

> > private browsing. Please check with ehsan/mconnor about how important it is
> 
> I have an email out about this. let's proceed in parallel for the moment.

OK, I agree on both points.

> unfortunately general uses of speculative connect don't really know if they
> are in private browsing mode.. there is no channel or principal.. we could
> implement something as a follow on through some kind of
> nsIInterfaceRequestor trail of tears or just an alternate interface.
> Technically they can still false start - just not in the presence of RC4 or
> RSA.. so hopefully at least the searchbar will shortly meet that criteria.

I don't understand this though. We separate every SSL connection into two buckets: private-browsing-mode or not-private-browsing-mode. All SSL connections share state with connections in one of those buckets or the other and, AFAICT, there is no way to choose a good default for when we cannot determine the private browsing state. And, speculative connect always (AFAICT) within the context of some user interaction in some browser window. In other words, if something is calling speculativeConnect without aCallbacks that help us navigate to an nsILoadContext (which determines whether we are in private browsing mode or not) then that is a bug and we should not do the speculative connection at all (since we don't know which bucket to use for the the shared state for the connection). But, I agree with you that that isn't this bug.

Comment 35

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

Comment on attachment 757548 [details] [diff] [review]
Enable TLS False Start (PSM)

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +1253,5 @@
> +    int16_t kea = ssl->GetKEAUsed();
> +    int16_t symcipher = ssl->GetSymmetricCipherUsed();
> +
> +    if ((kea != ssl_kea_rsa) && (symcipher != ssl_calg_rc4))
> +        return;

This early return should be removed because it would prevent us from executing the permMgr->RemoveFromPrincipal calls.

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ +362,4 @@
>  {
> +    LOG(("nsHttpConnection::SetupSSL %p caps=0x%X\n", this, caps));
> +
> +    if (mSetupSSLCalled)                                /* do only once */

Might as well fix the formatting of this comment while you're touching this line.

@@ +386,5 @@
>  
> +    nsCOMPtr<nsISSLSocketControl> ssl = do_QueryInterface(securityInfo, &rv);
> +    if (NS_FAILED(rv))
> +        return;
> +    

trailing whitespace.

@@ +402,3 @@
>  
> +    nsTArray<nsCString> protocolArray;
> +    

trailing whitespace.

::: netwerk/socket/nsISSLSocketControl.idl
@@ +65,5 @@
> +    [infallible] readonly attribute short SymmetricCipherUsed;
> +    [infallible] attribute short SymmetricCipherExpected;
> +
> +    const short KEY_EXCHANGE_UNKNOWN  = -1;
> +    const short SYMMETRIC_CIPHER_UNKNOWN  = -1;

Single spaces between tokens please.

::: security/manager/ssl/src/nsNSSIOLayer.h
@@ +94,5 @@
>    void SetTLSEnabled(bool enabled) { mTLSEnabled = enabled; }
>  
>    void AddPlaintextBytesRead(uint64_t val) { mPlaintextBytesRead += val; }
> +
> +  bool IsPreliminaryHandshakeDone() { return mPreliminaryHandshakeDone; }

nit: add const

@@ +98,5 @@
> +  bool IsPreliminaryHandshakeDone() { return mPreliminaryHandshakeDone; }
> +  void SetPreliminaryHandshakeDone() { mPreliminaryHandshakeDone = true; }
> +
> +  void SetKEAUsed(PRUint16 kea) { mKEAUsed = kea; }
> +  inline int16_t GetKEAExpected() // infallible in nsISSLSocketControl

Hmm...I had thought that these inline wrappers were created automatically by the IDL compiler.

Comment 36

[Patrick McManus [:mcmanus]]

(In reply to Brian Smith (:bsmith) from comment #35)
> Comment on attachment 757548 [details] [diff] [review]
> 
> > +  void SetKEAUsed(PRUint16 kea) { mKEAUsed = kea; }
> > +  inline int16_t GetKEAExpected() // infallible in nsISSLSocketControl
> 
> Hmm...I had thought that these inline wrappers were created automatically by
> the IDL compiler.

yes.. in this case in nsISSLSocketControl.h, but in the case of CanFalseStartCallback it sees the object as a nsNSSSocketInfo * so it doesn't have the wrapper handy. Introducing extra xpcom didn't seem helpful :)

Comment 37

[Patrick McManus [:mcmanus]]

Attachment: Enable TLS False Start (PSM)

Comment 38

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/99e03f6249b9

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/99e03f6249b9

Comment 40

[Octoploid]

With "ac_add_options --with-system-nss --with-system-nspr" I get:
...
/home/markus/mozilla-central/security/manager/ssl/src/nsNSSIOLayer.cpp:2566:3: error: use of undeclared identifier 'SSL_SetCanFalseStartCallback'; did you mean
      'CanFalseStartCallback'?
  SSL_SetCanFalseStartCallback(sslSock, CanFalseStartCallback, infoObject);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
  CanFalseStartCallback
/home/markus/mozilla-central/security/manager/ssl/src/nsNSSCallbacks.h:26:11: note: 'CanFalseStartCallback' declared here
SECStatus CanFalseStartCallback(PRFileDesc* fd, void* client_data,

          ^
/home/markus/mozilla-central/security/manager/ssl/src/nsNSSIOLayer.cpp:2566:41: error: cannot initialize a parameter of type 'void *' with an lvalue of type
      'SECStatus (PRFileDesc *, void *, PRBool *)'
  SSL_SetCanFalseStartCallback(sslSock, CanFalseStartCallback, infoObject);
                                        ^~~~~~~~~~~~~~~~~~~~~
/home/markus/mozilla-central/security/manager/ssl/src/nsNSSCallbacks.h:26:55: note: passing argument to parameter 'client_data' here
SECStatus CanFalseStartCallback(PRFileDesc* fd, void* client_data,
                                                      ^

Comment 41

[Patrick McManus [:mcmanus]]

octoploid, nightly is out of sync with system-nss for bit so we can get some testing with this patch in firefox while 713933 is tweaked for nss/chromium. see comments 48 and 49 in 713933

Comment 42

[Kai Engert (:KaiE:)]

There's some housekeeping necessary.

First, since your code depends on non-released NSS code, you should push for that release, and get Mozilla 25 properly updated to that release. I started that process in bug 898431.

Second, you didn't follow procedures correctly when you added a patch to the mozilla/security/patches directory. The intention is that you add the *NSS* patches that you have (temporarily) used on top of released NSS, but instead you placed a PSM patch into that directory.

Why am I complaining?

Because nobody else has done the NSS-release-housekeeping for aurora 24 yet, I'm trying to get it done. But the usual rules are "upgrade mozilla-central first, only afterwards upgrade aurora etc.".

I cannot do that, because I don't have the exact patch that would be required to be applied on the NSS 3.15.1 RTM tag, so I'll have to ask for an exception to get the aurora 24 housekeeping approval.

Finally, I wouldn't call this bug 658222 "fixed for 25" until you have picked up a NSS release version that contains the NSS code that you depend on.

I have proposed to tag a NSS 3.15.2 beta release, and update mozilla-central to pick up that NSS beta, thereby overwriting the patched NSS snapshot currently used by mozilla-central.

This will tell us, if all code you rely on has been properly integrated into NSS already.

Thanks

Comment 43

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 769155 [details] [diff] [review]
Enable TLS False Start (PSM)

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +717,5 @@
> +    if (!permMgr)
> +        return;
> +
> +    uint32_t perm;
> +    nsresult rv = permMgr->TestPermissionFromPrincipal(principal,

Shouldn't this be TestExactPermissionFromPrincipal? TestPermissionFromPrincipal will match subdomains - was this intended?

@@ +724,5 @@
> +        LOG(("nsHttpChannel::RetrieveSSLOptions [this=%p] "
> +             "falsestart-rsa permission found\n", this));
> +        mCaps |= NS_HTTP_ALLOW_RSA_FALSESTART;
> +    }
> +    rv = permMgr->TestPermissionFromPrincipal(principal, "falsestart-rc4", &perm);

Ditto.

Comment 44

[Patrick McManus [:mcmanus]]

re comment 43 see 912582. thanks

Comment 45

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

FYI, this was disabled in bug 920248 and will get re-enabled in bug 920248.