969479 - does not fallback from TLS to SSL when using proxy

Status: VERIFIED FIXED

(Core :: Security: PSM, defect)

Description

[Jonh Wendell]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20140203105410

Steps to reproduce:

- Try to access: https://wiki.gnome.org

Without a proxy, a client hello with tls 1.2 is sent, everything is fine. 

- When using a http proxy to reach https sites (a http tunnel is created with CONNECT), the server (or the proxy, or whatever, I really don't know) doesn't accept the client hello with tls 1.2 and the connection is dropped.


Actual results:

Firefox shows me an error: connection interrupted


Expected results:

Chromium and Epiphany (gnome's browser) detects this and open a new connection, this time the client_hello message contains SSL 3.0.

For whatever reason the connection is accepted and the page is loaded correctly.

I think FF should just do this same workaround.

Comment 1

[Patrick McManus [:mcmanus]]

it sounds like you're being mitm'd by your proxy (or something on the path) to force ssl 3.0 and the fallback logic doesn't deal with CONNECT on the fallback path (nsISSLSocketControl::*StartTLS)..

you might be better off not connecting :)

Comment 2

[Jonh Wendell]

the weird thing is that with *most* sites, like, for instance, this bugzilla (https://bugzilla.mozilla.org) the client_hello is sent with tls 1.2 and the connection is well stablished.

https://wiki.gnome.org and a few others I tried seems to have this issue.

(I'm monitoring with tcpdump)

Comment 3

[Alexander Usyskin]

(In reply to Patrick McManus [:mcmanus] from comment #1)
> it sounds like you're being mitm'd by your proxy (or something on the path)
> to force ssl 3.0 and the fallback logic doesn't deal with CONNECT on the
> fallback path (nsISSLSocketControl::*StartTLS)..
> 
> you might be better off not connecting :)

Yes, at least my corp proxy is known for mitm. So sometimes you have to connect :(

Comment 4

[Patrick McManus [:mcmanus]]

david - can you confirm the hypothesis here from comment 1? Thanks!

Comment 6

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

I can't reproduce this with https://wiki.gnome.org, bug https://national.virginmedia.com/service-announcements/status is broken as described. From what I can tell, it is intended that TLS fallback be prevented for STARTTLS connections. However, the check on nsNSSIOLayer.cpp:1008 also happens to be true for when there is a proxy (see nsNSSIOLayer.cpp:2300-2304).

998       // Don't allow STARTTLS connections to fall back on connection resets or
999       // EOF. Also, don't fall back from TLS 1.0 to SSL 3.0 for connection
1000       // resets, because connection resets have too many false positives,
1001       // and we want to maximize how often we send TLS 1.0+ with extensions
1002       // if at all reasonable. Unfortunately, it appears we have to allow
1003       // fallback from TLS 1.2 and TLS 1.1 for connection resets due to bad
1004       // servers and possibly bad intermediaries.
1005     conditional:
1006       if ((err == PR_CONNECT_RESET_ERROR &&
1007            range.max <= SSL_LIBRARY_VERSION_TLS_1_0) ||
1008           socketInfo->GetHasCleartextPhase()) {
1009         return false;
1010       }
1011       break;

...

2300   if (forSTARTTLS || proxyHost) {
2301     if (SECSuccess != SSL_OptionSet(fd, SSL_SECURITY, false)) {
2302       return NS_ERROR_FAILURE;
2303     }
2304     infoObject->SetHasCleartextPhase(true);
2305   }

Comment 7

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

Attachment: patch

Brian, if you have some spare bandwidth for this, this is a TLS intolerance fallback compatibility issue. (Basically, for the check that we're not falling back when using STARTTLS, we need to check that we actually are using STARTTLS.)

Comment 8

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

Comment on attachment 8379196 [details] [diff] [review]
patch

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

Please uplift to Firefox 28.

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +1002,5 @@
>        // servers and possibly bad intermediaries.
>      conditional:
>        if ((err == PR_CONNECT_RESET_ERROR &&
>             range.max <= SSL_LIBRARY_VERSION_TLS_1_0) ||
> +          (socketInfo->GetHasCleartextPhase() &&

Do we even need the hasCleartextPhase attribute at all, for anything, anymore? It seems we should just remove it.

Comment 9

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

Attachment: patch v1.1

Thanks for the quick review.

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #8)
> Please uplift to Firefox 28.

Ok - I'll take care of it.

> ::: security/manager/ssl/src/nsNSSIOLayer.cpp
> Do we even need the hasCleartextPhase attribute at all, for anything,
> anymore? It seems we should just remove it.

I agree, so I removed it and carried over r+.

https://hg.mozilla.org/integration/mozilla-inbound/rev/774001539b7e

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

Not tracking this for 27 as we're not very likely to do a respin there unless a chemspill driver shows up.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/774001539b7e

Comment 12

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

Comment on attachment 8379326 [details] [diff] [review]
patch v1.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): TLS intolerance fallback
User impact if declined: TLS intolerance fallback won't happen through proxies (i.e. some sites just won't be accessible to users using proxies)
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/927e4ae612b6

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/dd02eeb75188

Comment 15

[u279076]

John, can you please verify this is fixed using the following builds?

Firefox 28.0b5 (should be available tomorrow):
ftp://ftp.mozilla.org/pub/firefox/nightly/28.0b5-candidates/build1/ 

Firefox 29.0a2 (as of February 25th):
ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-aurora

Firefox 30.0a1 (as of Febryuary 22nd):
ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-central

Thanks

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/dd02eeb75188

Comment 17

[Jonh Wendell]

Yep, it's fixed, thanks!

Comment 18

[u279076]

(In reply to Jonh Wendell from comment #17)
> Yep, it's fixed, thanks!

Marking this bug verified fixed. Thanks for your help, Jonh.