1292159 - (CVE-2016-5291) Firefox V48.0 UXSS & Address Bar Spoofing

Status: VERIFIED FIXED

(Core :: Networking: File, defect)

Description

[Yuyang Zhou]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/52.0.2743.82 Safari/537.36

Steps to reproduce:

Firefox Version: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0

1. Download the poc.zip file

2. Put the p.url file into the Path 'C:/'

3. Open the spoof.html, click the 'sumbit' button, wait and reproduce this issue.


spoof.html:

<form action="file://127.0.0.1/c:/p.url"  target="xx" method="post" onsubmit="setTimeout(k, 2020)" id="c">

<input type="submit">a</input>

</form>

<script>

function k(){

window.open("ja		vascript:document.writeln('Spoof Google.com<img src=# onerror=alert(location.href)>!!')","xx");

}

</script>

 You could find more details by watching PoC.mp4 and watch PoC.png


Actual results:

The attacker could spoof and inject javascript code into any web pages. In the PoC, you could find the google.com is spoofed and the same-origin police has been bypassed.



Expected results:

Never change the address bar and navigation utill the page has been full loaded.

Comment 1

[Yuyang Zhou]

Attachment: poc.zip

Comment 2

[Yuyang Zhou]

Attachment: PoC.png

Comment 3

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed on latest Nightly. Seems that we have several odd things happening here.

Comment 4

[Yuyang Zhou]

I'm wondering if there were any follow-up on this issue this week? Thanks.

Comment 5

[Al Billings [:abillings - ex-MoCo]]

Moving this to the proper component so, maybe, people will see it.

Comment 6

[Yuyang Zhou]

Any follow-ups on this issue?

Comment 7

[:Gijs (he/him)]

If you have to convince a user to download a file to a specific path that the website then needs knowledge of to get to a spoof, that feels like it's very hard to exploit. If you can direct the user to download a particular file to a particular location, it feels like that would enable you to do more severe things than spoof the location bar. In other words, I'm not really sure about the severity here.

In fact, does this work if spoof.html is hosted on http? The screenshot shows it on file:///. I would expect it to only work if hosted locally, because the POST to file:/// is presumably not allowed. And if that *is* allowed, that feels much more like a docnav issue to me than something to do with the location bar.

(In reply to Yuyang Zhou from comment #0)
> Never change the address bar and navigation utill the page has been full
> loaded.

As written, I don't believe this helps (and it has other downsides both from a UX and security perspective). AIUI, it is possible for the testcase to work even if google.com loads fully - in fact, if location.href has been updated to reflect google.com, it *is* loaded (at least to some degree - waiting for extra images or stylesheets or whatever won't make a difference here).

Instead, the location bar should be updating when the JS runs, though I'd argue that the JS shouldn't be running against the other window at all, because it shouldn't have the same principal as the file-linked-to-google.com document. That's a Core issue, and it wouldn't go away by just not updating the location bar.

Boris, I would expect that we get notified (like through nsIWebProgressListener) when the JS runs here somehow. As noted, I also think the JS shouldn't be running at all, and the fact that it does is a Core issue that needs to be addressed. Am I missing something?

Comment 8

[:Gijs (he/him)]

Also pulling in Christoph because of the ... interesting... similarities with bug 1278013. Which is fixed for 48. Not sure why it's not helping here.

Comment 9

[Boris Zbarsky [:bzbarsky]]

There is nothing preventing POST to a file:/// URL that I know of.  It just gets turned into a GET, with the postdata dropped on the floor.

This does in fact look like bug 1278013 to me.  And at least on Mac if I try the attached testcase (replacing the .url file with an equivalent .desktop file) I don't see the JS running or the spoof working.

I assume the problem _is_ reproducible on Windows?  If so, I'd really appreciate it if someone who has easy access to a Windows debug build could check what things look like in nsJSProtocolHandler.cpp, in the nsJSThunk::EvaluateScript, around the principal->Subsumes(objectPrincipal) call.  What are principal and objectPrincipal in this case?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Wait.  The patch for bug 1278013 is using "^" and not "&~" to remove the security-inheritance bit?  Which means it might be _adding_ it in this case in some situations?  That could explain what's going on, if so...  :(

Gijs, if you change 

    nsSecurityFlags secFlags = mLoadInfo->GetSecurityFlags() ^
                               nsILoadInfo::SEC_FORCE_INHERIT_PRINCIPAL;

to:

    nsSecurityFlags secFlags = mLoadInfo->GetSecurityFlags() &
                               ~nsILoadInfo::SEC_FORCE_INHERIT_PRINCIPAL;

in nsBaseChannel::Redirect, does that fix the problem?

Comment 11

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #10)
> Wait.  The patch for bug 1278013 is using "^" and not "&~" to remove the
> security-inheritance bit?  Which means it might be _adding_ it in this case
> in some situations?  That could explain what's going on, if so...  :(
> 
> Gijs, if you change 
> 
>     nsSecurityFlags secFlags = mLoadInfo->GetSecurityFlags() ^
>                                nsILoadInfo::SEC_FORCE_INHERIT_PRINCIPAL;
> 
> to:
> 
>     nsSecurityFlags secFlags = mLoadInfo->GetSecurityFlags() &
>                                ~nsILoadInfo::SEC_FORCE_INHERIT_PRINCIPAL;
> 
> in nsBaseChannel::Redirect, does that fix the problem?

With this change, AFAICT the JS no longer runs. It's hard to be 100% sure because of the timeout, which means the testcase feels like it'll be a bit racy, but actually it's 100% reproducible for me on beta, and 0% reproducible after this change, when testing on self-compiled nightly. So I suspect you're right. Please needinfo if it'd be helpful for me to poke at this more (e.g. stacks for how we're hitting this case for this exploit) and I'll try and answer any questions tomorrow if that's helpful.

Comment 12

[Boris Zbarsky [:bzbarsky]]

OK, I thought about it more, and I can reproduce this bug on Mac too.  All I have to do is make sure that p.desktop is in the _parent_ of the directory that houses spoof.html, so that normally the load of p.desktop would _not_ inherit the principal.  The use of xor instead of &~ means the inherit bit gets added in that situation...

Patch coming up.

Comment 13

[Boris Zbarsky [:bzbarsky]]

We should take this on all branches, including 48 and 45esr.  Because once someone looks at the advisory for bug 1278013 this would be the obvious thing to try.  :(

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Be more careful about removing out inherit principal bit on base channel redirects

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Be more careful about removing the inherit principal bit on base channel redirects

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8786924 [details] [diff] [review]
Be more careful about removing the inherit principal bit on base channel redirects

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

Good question.  Probably pretty easily if you know how this codepath is hit and how our file security model works.

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

Not as far as I could avoid.  I did explicitly talk about removing the bit, but I assume anyone who sees fooo & ~bar will know that anyway.

Which older supported branches are affected by this flaw?

45esr, 48, 49, 50, 51.

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

Bug 1278013.

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

This should be a trivial backport.

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

This seems pretty safe to me.  Minimal testing is probably OK.

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8786924 [details] [diff] [review]
Be more careful about removing the inherit principal bit on base channel redirects

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

(In reply to Boris Zbarsky [:bz] from comment #12)
> The use of xor instead of &~ means the inherit bit gets added in that situation...

Arrrh. Headbang!!! Thanks Boris for fixing this up!

Comment 18

[Boris Zbarsky [:bzbarsky]]

Daniel, note that this bug basically allows file:// stuff to violate our security model for file://.  Place a .url/.desktop file in a dir pointing to an arbitrary file, have a file in a _child_ of that dir that links to that .url/.desktop, load that file from the child dir, and suddenly you are same-origin with the arbitrary file the .url/.desktop links to.

So it may be worth taking this in a security update to 48, unless there simply isn't time for one before 49 ships anyway.

Comment 19

[Al Billings [:abillings - ex-MoCo]]

There isn't time for this before 49 ships. We've had our last beta.

As a sec-moderate (unless this is mis-rated), it doesn't need sec-approval+ to land on trunk.

Comment 20

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/987d4f3854dabeb33f7070b7d13392d321067b6f

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8786924 [details] [diff] [review]
Be more careful about removing the inherit principal bit on base channel redirects

Approval Request Comment
[Feature/regressing bug #]: Bug 1278013
[User impact if declined]: Allows files to attack other files on the hard drive
    in ways they shouldn't be able to do, in addition to creating spoofing risks.
[Describe test coverage new/current, TreeHerder]: Test coverage is lacking for
   file:// stuff.
[Risks and why]: I think this is pretty low risk, but since this is a regression
   from what we thought was a low-risk fix...
[String/UUID change made/needed]: None.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/987d4f3854da

Comment 23

[Yuyang Zhou]

Thanks for the timely release of the patch. I'm wondering if this report could be assigned a CVE ID?

Comment 24

[Boris Zbarsky [:bzbarsky]]

I believe we assign CVEs when we get closer to actually shipping the fix...  So far it's just checked into the development trunk.

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Doesn't match the esr45 criteria

Comment 26

[Boris Zbarsky [:bzbarsky]]

Wait, what?  Why not?  What are these criteria?

Comment 27

[Al Billings [:abillings - ex-MoCo]]

(In reply to Boris Zbarsky [:bz] from comment #26)
> Wait, what?  Why not?  What are these criteria?

Generally sec-high or sec-critical unless a special case is made. We don't take moderate or low rated issues on the ESR branch normally for stability reasons.

Comment 28

[Boris Zbarsky [:bzbarsky]]

I see.  So same-origin policy bypass for file:// URIs is not something we would normally take on ESR?

Comment 29

[Al Billings [:abillings - ex-MoCo]]

Dan is the one who rated it sec-moderate so you would need to ask him why it isn't a high if you think it is misrated. Otherwise, policy is that we don't take moderate or low bugs on ESR. We have to walk a line between "all security fixes" and "no changes, keep it stable."

Comment 30

[Daniel Veditz [:dveditz]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #25)
> Doesn't match the esr45 criteria

This was already marked "49+", because on security grounds it does. "sec-moderate" doesn't mean "we never take it on ESR", it means we judge it case-by-case. The download requirement lessens the mass-pwnage possibilities (thus not "high"), but the impact to affected individuals can still be severe.

Looks like it's too late for 49 (RC already shipped, this isn't a re-spin type fix) so I'll move the ESR to match "50+", but we do still want it on ESR just as we took bug 1278013 on ESR.

Comment 31

[Sylvestre Ledru [:Sylvestre]]

Yes, we have to draw a line somewhere for ESR. As Al said, sec-high and sec-moderate.
If you think that the risk is high enough to take it into ESR, we should probably bump the severity.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Well, I can't tell, because our severity ratings are all web-site-centric and don't really address the threat vector being a local file.

In this case, the attack would involve placing a specially crafted .desktop/.url file on the users disk, so maybe it's not that dangerous.  I just can't tell.  Do we consider a file on a network drive reading arbitrary files off your hard disk a sec-moderate or a sec-high?

Comment 33

[Al Billings [:abillings - ex-MoCo]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #31)
> Yes, we have to draw a line somewhere for ESR. As Al said, sec-high and
> sec-moderate.
> If you think that the risk is high enough to take it into ESR, we should
> probably bump the severity.

We do take exceptions to this policy. The dev or someone else just makes a case for it normally. I didn't realize Dan had wanted it. Bumping the severity isn't really a solution as it messes up our rating system, which is not "make this more severe so we'll take it on ESR." The ratings have specific meanings (as much as possible).

Comment 34

[:Gijs (he/him)]

(In reply to Al Billings [:abillings] from comment #33)
> (In reply to Sylvestre Ledru [:sylvestre] from comment #31)
> > Yes, we have to draw a line somewhere for ESR. As Al said, sec-high and
> > sec-moderate.
> > If you think that the risk is high enough to take it into ESR, we should
> > probably bump the severity.
> 
> We do take exceptions to this policy. The dev or someone else just makes a
> case for it normally. I didn't realize Dan had wanted it. Bumping the
> severity isn't really a solution as it messes up our rating system, which is
> not "make this more severe so we'll take it on ESR." The ratings have
> specific meanings (as much as possible).

Additionally, as Dan said in comment #30, we took bug 1278013 on ESR, which was also sec-moderate rated and involved a similar-ish problem. This bug is a direct regression from that bug, and so arguably as a regression that we uplifted to ESR, the fix for that regression should be uplifted as well, particularly because it is a security issue that, as noted in comment #16, is reasonably "obvious" given both the commit here and the previous fix which was already CVE'd and made public as fixed in earlier versions of ESR.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/df0e14b3f743

Comment 36

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8786924 [details] [diff] [review]
Be more careful about removing the inherit principal bit on base channel redirects

As we will ship 50 with the sec fix and several people think we should take it despite its severity, let's do it.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/3ff0c89f3b2685948e9310d8f41fa69176532882

Comment 38

[Ciprian Georgiu, Desktop QA]

I've managed to reproduce this issue on 48.0.2 (2016-08-04) using STR form comment 0.

This is verified fixed on 45.5.0esr (20161031153904), 50.0-build2 (20161104212021) and 51.0a2 (2016-11-8) using Windows 10 x64. Marking here accordingly.

Comment 39

[David Lawrence [:dkl]]

Does this bug need to still be private? Someone inquired in bug 1320701.