does not fallback from TLS to SSL when using proxy

VERIFIED FIXED in Firefox 28, Firefox OS v1.3



Security: PSM
4 years ago
4 years ago


(Reporter: Jonh Wendell, Assigned: keeler)




Firefox Tracking Flags

(firefox27 wontfix, firefox28+ verified, firefox29+ verified, firefox30+ verified, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)


(Whiteboard: [qa-])


(1 attachment, 1 obsolete attachment)



4 years ago
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:

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.
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 :)
Component: Untriaged → Security: PSM
Product: Firefox → Core

Comment 2

4 years ago
the weird thing is that with *most* sites, like, for instance, this bugzilla ( the client_hello is sent with tls 1.2 and the connection is well stablished. and a few others I tried seems to have this issue.

(I'm monitoring with tcpdump)

Comment 3

4 years ago
(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 :(
david - can you confirm the hypothesis here from comment 1? Thanks!
Flags: needinfo?(dkeeler)


4 years ago
Duplicate of this bug: 973628
I can't reproduce this with, bug 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   }


4 years ago
Flags: needinfo?(dkeeler)
Created attachment 8379196 [details] [diff] [review]

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.)
Assignee: nobody → dkeeler
Ever confirmed: true
Attachment #8379196 - Flags: review?(brian)
Comment on attachment 8379196 [details] [diff] [review]

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.
Attachment #8379196 - Flags: review?(brian) → review+
status-firefox27: --- → affected
status-firefox28: --- → affected
status-firefox29: --- → affected
status-firefox30: --- → affected
tracking-firefox27: --- → ?
tracking-firefox28: --- → ?
tracking-firefox29: --- → ?
tracking-firefox30: --- → ?
Created attachment 8379326 [details] [diff] [review]
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+.
Attachment #8379196 - Attachment is obsolete: true
Attachment #8379326 - Flags: review+
Not tracking this for 27 as we're not very likely to do a respin there unless a chemspill driver shows up.
tracking-firefox28: ? → +
tracking-firefox29: ? → +
tracking-firefox30: ? → +
Last Resolved: 4 years ago
status-firefox30: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
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
Attachment #8379326 - Flags: approval-mozilla-beta?
Attachment #8379326 - Flags: approval-mozilla-aurora?
Attachment #8379326 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox27: affected → wontfix
status-firefox29: affected → fixed
tracking-firefox27: ? → ---
Attachment #8379326 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
John, can you please verify this is fixed using the following builds?

Firefox 28.0b5 (should be available tomorrow): 

Firefox 29.0a2 (as of February 25th):

Firefox 30.0a1 (as of Febryuary 22nd):

Flags: needinfo?(jonh.wendell)
Whiteboard: [qa-]
Keywords: regression
QA Contact: mwobensmith
status-b2g-v1.3: --- → fixed

Comment 17

4 years ago
Yep, it's fixed, thanks!
Flags: needinfo?(jonh.wendell)
(In reply to Jonh Wendell from comment #17)
> Yep, it's fixed, thanks!

Marking this bug verified fixed. Thanks for your help, Jonh.
status-firefox28: fixed → verified
status-firefox29: fixed → verified
status-firefox30: fixed → verified
status-b2g-v1.3T: --- → fixed
status-b2g-v1.4: --- → fixed
You need to log in before you can comment on or make changes to this bug.