Closed Bug 969479 Opened 6 years ago Closed 6 years ago

does not fallback from TLS to SSL when using proxy

Categories

(Core :: Security: PSM, defect)

x86_64
Linux
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla30
Tracking Status
firefox27 --- wontfix
firefox28 + verified
firefox29 + verified
firefox30 + verified
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: jonh.wendell, Assigned: keeler)

References

Details

(Keywords: regression, Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

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.
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
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)
(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)
Duplicate of this bug: 973628
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   }
Flags: needinfo?(dkeeler)
Attached patch patch (obsolete) — Splinter 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
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8379196 - Flags: review?(brian)
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.
Attachment #8379196 - Flags: review?(brian) → review+
Attached patch patch v1.1Splinter Review
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
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.
https://hg.mozilla.org/mozilla-central/rev/774001539b7e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
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+
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):
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
Flags: needinfo?(jonh.wendell)
Whiteboard: [qa-]
QA Contact: mwobensmith
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.
You need to log in before you can comment on or make changes to this bug.