Last Comment Bug 796475 - (CVE-2013-0776) HTTPS can be effectively disabled by attackers on rogue networks using a proxy that returns 407 with embedded script
(CVE-2013-0776)
: HTTPS can be effectively disabled by attackers on rogue networks using a prox...
Status: RESOLVED FIXED
[qa-][adv-main19+][adv-esr1703+]
: csectype-priv-escalation, sec-high
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: 15 Branch
: All Windows 7
: -- major (vote)
: mozilla21
Assigned To: Patrick McManus [:mcmanus] PTO until Sep 6
:
Mentors:
http://lcamtuf.blogspot.com/2013/02/f...
Depends on: 904590
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-01 21:24 PDT by Michal Zalewski
Modified: 2014-07-24 14:37 PDT (History)
23 users (show)
dveditz: sec‑bounty+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
tef+
affected
-
wontfix
+
fixed
+
fixed
fixed
affected
19+
fixed
19+
fixed
fixed


Attachments
patch 0 (12.41 KB, patch)
2012-12-20 09:46 PST, Patrick McManus [:mcmanus] PTO until Sep 6
jduell.mcbugs: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
akeybl: approval‑mozilla‑esr10-
akeybl: approval‑mozilla‑esr17+
lmandel: approval‑mozilla‑b2g18+
abillings: sec‑approval+
Details | Diff | Splinter Review

Description Michal Zalewski 2012-10-01 21:24:17 PDT
There appears to be a relatively nasty bug in your handling of HTTPS. To reproduce, set up a TCP server on, say, localhost:1234, and have it read the HTTP request and respond with:

-- snip! --
HTTP/1.0 407 Burp
Proxy-Authenticate: basic
Connection: close
Content-Type: text/html

<html>
<h1>pwnd</h1>
<script>alert(location.href)</script>
[...some padding...]
-- snip! --

Then, configure localhost:1234 as a proxy for HTTP and HTTPS (on a rogue network, this could be done through auto-discovery).

Next, have the victim navigate to https://www.paypal.com/. A prompt will come up, but doing the most reasonable thing (dismissing it or hitting ESC) will display the HTML content returned by the proxy over plain text, executing it in the context of https://www.paypal.com/.
Comment 1 Benjamin Smedberg [:bsmedberg] 2012-10-02 06:21:20 PDT
Calling this "docshell" for now, although it be actually a network-layer issue.
Comment 3 Boris Zbarsky [:bz] 2012-10-02 08:29:29 PDT
I thought this was a known issue with 407 responses from a proxy.  And that in fact there were lots of proxies that _depend_ on this behavior...

Over to networking, where I know people have looked into this in the past.
Comment 4 Michal Zalewski 2012-10-02 08:46:50 PDT
The problem here is mostly that Firefox actually renders the error page as if coming from the target server. This is specific to Firefox.

Most browsers display a password prompt if they hit 407, which is probably required for some proxies to work and is still somewhat bad phishing-wise, but at least it doesn't compromise the HTTPS origin.
Comment 5 Boris Zbarsky [:bz] 2012-10-02 08:52:09 PDT
Ah, I see.  That's odd, I agree.  Patrick, Jason, did that come up before?
Comment 6 Boris Zbarsky [:bz] 2012-10-02 08:57:34 PDT
So the previous discussion was in bug 599295 and bug 491818 and bug 479880.

Patrick, is there a reason we can't reset the channel principal to a null principal in this situation (failed SSL connect with 407)?
Comment 7 Patrick McManus [:mcmanus] PTO until Sep 6 2012-10-02 10:16:15 PDT
(In reply to Boris Zbarsky (:bz) from comment #6)
> So the previous discussion was in bug 599295 and bug 491818 and bug 479880.
> 
> Patrick, is there a reason we can't reset the channel principal to a null
> principal in this situation (failed SSL connect with 407)?

that's my inclination. I haven't done the due diligence on it.
Comment 8 Michal Zalewski 2012-10-02 10:19:38 PDT
FYI, most other browsers don't display the server-supplied error page at all: if auth fails, they just show something like about:neterror. 

Perhaps that's an even simpler fix.
Comment 9 Boris Zbarsky [:bz] 2012-10-02 10:44:39 PDT
That's a much harder fix in terms of actual code changes, actually.

The question is what the goal is.  The testcase in this bug would still alert the server location and the url bar would show the server location, with the null principal thing.  But the site would not run with that server's permissions (e.g. would not be able to do XHR to that server).  It might still be a serious spoof risk.  :(
Comment 10 Patrick McManus [:mcmanus] PTO until Sep 6 2012-10-02 11:23:56 PDT
we could probably easily suppress the OnDataAvailable calls for the 407 body.. but I do worry there are cases where that information is important for actually authorizing to the proxy.
Comment 11 Michal Zalewski 2012-10-02 11:34:24 PDT
Possibly, although 30x redirects, 5xx codes, 4xx, etc, already don't work in Firefox if returned by the proxy for CONNECT requests, and it's already breaking some stuff (bug 491818).

Displaying the proxy-supplied HTML if the address bar still displays https://www.paypal.com/ is probably a very bad choice either way. You could maybe replace the URL with moz-proxy://.../, but then the behavior of "reload" may be a bit confusing.
Comment 12 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-10-02 12:36:11 PDT
I do not think we should show the proxy's error page at all. If we do show it, then the proxy will be able to spoof any HTTPS website. We cannot rely on the user noticing the lack of a security indicator. (As Limit always said, people tend to never notice that an indicator is missing.) So, IMO, we must create an about: page for this case, regardless.

Also, should we be returning the proxy's response to XHR? I vote that we do not.
Comment 13 Michal Zalewski 2012-10-02 12:41:30 PDT
Returning it via XHR would probably mean potential script injection in half of the HTTPS-enabled "web 2.0" stuff (certainly GMail, Facebook, etc).
Comment 14 Jason Duell [:jduell] (needinfo me) 2012-10-02 17:23:48 PDT
>> most other browsers don't display the server-supplied error page at all: if
>> auth fails, they just show something like about:neterror. 
>
> That's a much harder fix in terms of actual code changes, actually.

Returning a boilerplate error page instead of renderin the HTML reply is what we did for failed 30x replies.  Why is that harder for failed 407s?  I agree with Brian that we probably shouldn't be rendering the reply.

> I do worry there are cases where that information is important for
> actually authorizing to the proxy.

That was also a concern when we stopped rendering proxy responses for 30x responses.  It sucked not to be more proxy-friendly, but in the end it was non-trival to render the HTML safely.
Comment 15 Patrick McManus [:mcmanus] PTO until Sep 6 2012-10-02 17:29:56 PDT
(In reply to Jason Duell (:jduell) from comment #14)

> That was also a concern when we stopped rendering proxy responses for 30x
> responses.  It sucked not to be more proxy-friendly, but in the end it was
> non-trival to render the HTML safely.

so be it!
Comment 16 Boris Zbarsky [:bz] 2012-10-02 18:26:10 PDT
> Why is that harder for failed 407s?

It's not harder than it is for 30x, modulo compat issues.

It's harder than stamping a nullprincipal as the owner, which is literally a single line of code.  ;)
Comment 17 Michal Zalewski 2012-12-03 14:28:49 PST
Yello,

Just a gentle ping at a two-month mark :-)
Comment 18 Josh Aas 2012-12-17 07:56:05 PST
Patrick - can you take this one?
Comment 19 Patrick McManus [:mcmanus] PTO until Sep 6 2012-12-19 18:17:47 PST
gecko actually tries to do the right thing here (showing the proxy connection error page) - it is specifically the path dealing with the cancellation of the auth box that has the bug. If you just fail to authenticate (provide bad credentials a few times) you don't see the message body provided by the server and instead get the error page.. so fixing the cancel bug isn't going to break any new ground in terms of a vector people were relying on to provide instructions, etc..
Comment 20 Patrick McManus [:mcmanus] PTO until Sep 6 2012-12-20 09:46:27 PST
Created attachment 694454 [details] [diff] [review]
patch 0

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

Displays an existing error page when proxy auth dialog is canceled.
Comment 21 Jason Duell [:jduell] (needinfo me) 2012-12-25 09:21:46 PST
Comment on attachment 694454 [details] [diff] [review]
patch 0

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

Looks good to me.  Nice to have test coverage :)

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4156,5 @@
>  {
>      LOG(("nsHttpChannel::OnAuthCancelled [this=%p]", this));
>  
>      if (mTransactionPump) {
> +        

nit: splinter seems to show extra whitespace on blank line

@@ +4166,5 @@
> +        // because we do want to show the content if this is an error from
> +        // the origin server.
> +        if (mProxyAuthPending)
> +            Cancel(NS_ERROR_PROXY_CONNECTION_REFUSED);
> +        

ditto
Comment 22 Patrick McManus [:mcmanus] PTO until Sep 6 2012-12-26 07:02:53 PST
Comment on attachment 694454 [details] [diff] [review]
patch 0

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

The patch indicates the problem clearly - but exploiting the bug requires an active mitm attacker.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

probably

Which older supported branches are affected by this flaw?

this bug has always existed in firefox

If not all supported branches, which bug introduced the flaw?

na

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

no. fairly easy to make with low risk.

How likely is this patch to cause regressions; how much testing does it need?

pretty average level for a bugfix.
Comment 23 Al Billings [:abillings] 2012-12-27 11:55:45 PST
I'd like to sec-approve this but I want to know what the branch story is from Release Management first.
Comment 24 bhavana bajaj [:bajaj] 2012-12-31 11:10:08 PST
(In reply to Al Billings [:abillings] from comment #23)
> I'd like to sec-approve this but I want to know what the branch story is
> from Release Management first.

Al, considering we have gone to build with all our beta's , this will be going wontfix for Fx18.We could target timely landings for future versions .
Comment 25 Al Billings [:abillings] 2012-12-31 15:58:14 PST
(In reply to bhavana bajaj [:bajaj] from comment #24)
> (In reply to Al Billings [:abillings] from comment #23)
> > I'd like to sec-approve this but I want to know what the branch story is
> > from Release Management first.
> 
> Al, considering we have gone to build with all our beta's , this will be
> going wontfix for Fx18.We could target timely landings for future versions .

When do we want to get this in then? Second week of new cycle?
Comment 26 Alex Keybl [:akeybl] 2013-01-07 17:20:11 PST
(In reply to Al Billings [:abillings] from comment #25)
> (In reply to bhavana bajaj [:bajaj] from comment #24)
> > (In reply to Al Billings [:abillings] from comment #23)
> > > I'd like to sec-approve this but I want to know what the branch story is
> > > from Release Management first.
> > 
> > Al, considering we have gone to build with all our beta's , this will be
> > going wontfix for Fx18.We could target timely landings for future versions .
> 
> When do we want to get this in then? Second week of new cycle?

Makes sense to me, as far as risk of regression versus security visibility risk. Let's land in the next couple of weeks, and definitely before Beta 3 goes to build on 1/22.
Comment 27 Patrick McManus [:mcmanus] PTO until Sep 6 2013-01-09 07:28:03 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e17ce2c711c
Comment 28 Ed Morley [:emorley] 2013-01-09 15:05:43 PST
https://hg.mozilla.org/mozilla-central/rev/2e17ce2c711c
Comment 29 Patrick McManus [:mcmanus] PTO until Sep 6 2013-01-22 12:05:45 PST
Comment on attachment 694454 [details] [diff] [review]
patch 0

[Approval Request Comment]
Bug caused by (feature/regressing bug #): original sin
User impact if declined: theoretical MITM spoof of HTTPS by active attacker against only users using HTTP proxies who hit "cancel" on the auth popup
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): other than my hand testing we have very little exposure to the use patters of corporate proxy users.
String or UUID changes made by this patch: none

Personally, I wouldn't backport this and instead take normal channel testing.. it has existed forever, requires an esoteric exploit path (the user has to cancel the popup, they can't simply provide bad credentials), and doesn't have any cited exploits. The change is relatively simple, but its not in an area we have extensive tests with (manual or automated).

those who nominated/approved it for tracking might feel differently.
Comment 30 Patrick McManus [:mcmanus] PTO until Sep 6 2013-01-22 12:07:50 PST
(there is automated testing of this change included in the patch.. I realize my comment 29 might have been confusing on that point.)
Comment 31 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-01-22 13:41:58 PST
Comment on attachment 694454 [details] [diff] [review]
patch 0

I think this should be approved and landed for -aurora and -beta, as the patch itself is very instructive for how to exploit this flaw.

Also, if I am understanding the first few comments correctly, this flaw allows the proxy to render content and execute code with the principal of the victim. That is extremely bad, and this is why I am also nominating for ESR and B2G.
Comment 32 Michal Zalewski 2013-01-22 13:50:00 PST
Yeah, that's the impact. The mitigating factor is that the user needs to dismiss an auth dialog first. The mitigating factor to the mitigating factor is that dismissing the modal dialog is the only way to continue using or even close the browser, IIRC.
Comment 33 Patrick McManus [:mcmanus] PTO until Sep 6 2013-01-22 13:52:34 PST
(In reply to Michal Zalewski from comment #32)
> Yeah, that's the impact. The mitigating factor is that the user needs to
> dismiss an auth dialog first. The mitigating factor to the mitigating factor
> is that dismissing the modal dialog is the only way to continue using or
> even close the browser, IIRC.

if you simply say ok to the dialog (maybe twice) the contents of the 407 are not rendered in 18. Its specifically only if you say cancel.
Comment 34 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-01-22 14:23:08 PST
(In reply to Patrick McManus [:mcmanus] from comment #33)
> if you simply say ok to the dialog (maybe twice) the contents of the 407 are
> not rendered in 18. Its specifically only if you say cancel.

Understood. But, it's too subtle. Especially, if the user isn't even expecting to enter a username/password, then they're going to click cancel. And, more generally, "Cancel" should always be a safe action to take in any security prompt.
Comment 35 Daniel Veditz [:dveditz] 2013-01-22 14:27:59 PST
I agree that confused users are likely to cancel (or hit Escape which amounts to the same thing). 

Cancel on this dialog is -meant- to be a safe action, it's just that it isn't due to this bug. But because it's usually safe we should expect a number of users to do that as their first reaction.
Comment 36 Alex Keybl [:akeybl] 2013-01-22 15:12:29 PST
Comment on attachment 694454 [details] [diff] [review]
patch 0

Approving for all branches except B2G, as that branch is still in flux. We'll ping when ready to uplift there as well.
Comment 38 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-01-23 11:31:39 PST
I don't know what tracking-b2g18: "19+" means exactly. Because this allows a MitM to effectively MitM even an HTTPS site, it is really important to land into B2G 1.0 if at all possible.
Comment 39 Lucas Adamski [:ladamski] 2013-01-23 14:07:51 PST
Agree, a bug of this severity is worth taking even this late in the cycle.
Comment 40 Patrick McManus [:mcmanus] PTO until Sep 6 2013-01-24 07:42:09 PST
 https://hg.mozilla.org/releases/mozilla-b2g18/rev/b5b518e39789
Comment 41 Lukas Blakk [:lsblakk] use ?needinfo 2013-01-30 14:40:31 PST
Landed on mozilla-b2g18 prior to the 1/25 branching to mozilla-b2g18_v1_0_0, updating status-b2g-v1.0.0
Comment 42 Ioana (away) 2013-02-13 07:52:53 PST
Marking as qa- per comment 30.

Please only remove the tag if you want any specific manual testing done around this fix.
Comment 43 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-02-15 16:14:06 PST
Michal, would you mind testing to see if this is fixed on your end? It should now be fixed in Firefox 19 Beta, 20 Aurora, 21 Nightly, and 17.0.3esr.
Comment 44 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-02-20 15:04:52 PST
Here is the Microsoft Research paper from 2009 that talks about these kinds of attacks:

http://research.microsoft.com/pubs/79323/pbp-final-with-update.pdf

and here is the Hacker News story:

http://news.ycombinator.com/item?id=5253136

Note You need to log in before you can comment on or make changes to this bug.