737307 - (CVE-2012-0474) universal XSS by confusing docshell with short-circuited loads

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect)

Description

[Chris McGowen]

Attachment: 9.html

User Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:10.0.2) Gecko/20100101 Firefox/10.0.2
Build ID: 20120215223356

Steps to reproduce:

Use javascript to open new window of location on same domain.
On new window, history.pushState change to same or other location on same domain. 
Change location of window to foreign website.
History.back.
History.forward.
History.back.


Actual results:

Foreign website is loaded with my url. 
Also, the javascript on that page then sends requests to my domain. Private information can be obtained from these requests.


Expected results:

Loading of a different domain should cancel the history change.

Comment 1

[Chris McGowen]

I have tested this on: Firefox 10.0.2 on linux and Firefox 11 on Windows 7

I've also made a test site to view the active code. Further requests on maps.google.com do not go to google but go to my domain: badcoding.net

http://badcoding.net/test9_lmkunuht67129fjigh13/indexOVR.html

Comment 2

[Chris McGowen]

I've added another example in which it retains the SSL identifier of wikipedia.org.

If you switch tabs after this and return later, the name on the SSL identifier changes from wikipedia.org to badcoding.net while still displaying the SSL information from wikipedia.org

When viewing the dropdown of the SSL identifier, it shows:
 You are connected to:
 badcoding.net
 
on "More Information", the website is badcoding.net
on "View Certificate", it is wikipedia.org's certificate.

http://badcoding.net/test9_lmkunuht67129fjigh13/indexSSL.html

Firefox 11.0 on Ubuntu 10.04

Comment 3

[Justin Lebar (not reading bugmail)]

We really think it's at the new URL, too.  I get

Security Error: Content at http://badcoding.net/ may not load data from https://maps.google.com/.

Comment 4

[Justin Lebar (not reading bugmail)]

Attachment: Testcase with hash navigation instead of pushstate (must be run locally)

This happens if we substitute a hash navigation for the pushstate.

Comment 5

[Justin Lebar (not reading bugmail)]

Somehow we're doing a short-circuited load from maps.google.com to the attack page on the final <back>.  A short-circuited load is only supposed to happen between SHEntries which represent the same document (via pushstate or a hash navigation), so this is extremely wrong.

Comment 6

[Chris McGowen]

I was able to leverage this bug as a XSS on maps.google.com through a requested javascript file.

Comment 7

[Justin Lebar (not reading bugmail)]

So I think what's going wrong here is:

We call nsSHistory::AddEntry off nsDocShell::CreateContentViewer, which happens as a result of nsHttpChannel::OnStartRequest.  So the SHEntry for google maps is, in normal operation, created asynchronously after we navigate the window.

I don't have a complete understanding of what's going on here, because it disappears when I look at it in a debugger, but I have the initial sequence down:

 1) We set up two shentries which share a document (via pushstate or a hash navigation, doesn't seem to matter).  Call these SHEntries A and B.

 2) We start loading maps.google.com.

 3) Before we call nsSHistory::AddEntry for maps.google.com, we go back.  Now we're at index 0, corresponding to SHEntry A.

Now the details are a bit hazier, but what I guess happens is

 4) We go forward.  Now we're at index 1, SHEntry B.  This navigation is correctly a short-circuited load.

 5) maps.google.com loads into the current docshell SHEntry, B rather than calling nsSHistory::AddEntry and creating a brand new SHEntry for itself.

The final go(-1) in the test is unnecessary.

The real question is, why is (5) happening?

Comment 8

[Justin Lebar (not reading bugmail)]

> The real question is, why is (5) happening?

Oh, because mLoadType after step (4) is 0x4, LOAD_CMD_HISTORY.

So the back->forward clobbers our memory of the type of the outstanding load.

I'm not sure what's the right solution here, but one thing we could do is cancel outstanding asynchronous loads when we do a short-circuit load.  That would match what we do when you do two asynchronous loads in a row (we cancel the first one).

Comment 9

[Boris Zbarsky [:bzbarsky]]

> but one thing we could do is cancel outstanding asynchronous loads when we do a
> short-circuit load.

Hmm.  http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#navigate says that a preexisting navigation that's not far enough along should be canceled on attempts to navigate at step 6.  The short-circuiting of a load should happen at step 7.

So we should be canceling the outstanding load when the scroll request is made, yes, if it's early enough in the load.  In fact we should be canceling it when the back() call happens, I think.  I'd have to recheck when the back() call enters the "navigate" steps.  I believe that would in fact fix this bug.

Comment 10

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

Do we want to include a test?  It would be basically this exploit.  I guess we could hang on to the test until we roll out the fixed binaries.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 607822 [details] [diff] [review]
Patch v1

See, this is no good.  With this patch, clicking on an anchor to scroll down a page while that page is loading will stop the loading of the page.

That's why the Stop() call is after the short-circuit code right now and why we want to only cancel navigations that are not "far enough" along... :(

Comment 12

[Justin Lebar (not reading bugmail)]

> See, this is no good.  With this patch, clicking on an anchor to scroll down a page while that page 
> is loading will stop the loading of the page.

Oh, goodness...

What if we call Stop() in the short-circuit code if the short-circuit URI doesn't match-except-hash mLoadingURI?

Comment 13

[Boris Zbarsky [:bzbarsky]]

The mLoadingURI thing would fail for pushState, no?

How about we grovel through the loadgroup looking for requests with the LOAD_DOCUMENT_URI flag and if those do not match the channel of the document currently in the docshell, cancel just those requests?

Comment 14

[Justin Lebar (not reading bugmail)]

Attachment: Patch v2

Ugh.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 607852 [details] [diff] [review]
Patch v2

NS_ERROR_BINDING_ABORTED is better.

And r=me, assuming this fixes the bug.

Comment 16

[Justin Lebar (not reading bugmail)]

Do we want to include a test?  Do we want to write a test and check it in later?

Comment 17

[Justin Lebar (not reading bugmail)]

Comment on attachment 607852 [details] [diff] [review]
Patch v2

[Approval Request Comment]
* Regression caused by (bug #): Unknown; this may have been present for a long time.
* User impact if declined: Serious sg:high, can reliably XSS any page.
* Testing completed (on m-c, etc.): Fixes included testcase.  We can allow to bake on m-c, but the clock is ticking for beta.
* Risk to taking this patch (and alternatives if risky): Any docshell change is risky.  This could break some edge case we haven't thought of.
* String changes made by this patch: None

Comment 18

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d69dc0ec681

Also, wrt to risk if declined: Bug 687745 appears to be a duplicate of this bug (albeit with less-reliable testcases).  That bug was filed 6 months ago.  So it's not unlikely others have discovered this issue.

Comment 19

[Justin Lebar (not reading bugmail)]

I thought I'd put my money where my mouth is wrt risk and push to m-i without first pushing to try.  That was perhaps ill-advised.

This patch turns the scriptaculous suite permaorange.  I imagine scriptaculous is doing something dumb, but this does suggest that sites may be affected by this change.

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/d9491b6074a4

Comment 20

[Justin Lebar (not reading bugmail)]

Attachment: Part 2, v1: Fix scriptaculous test.

Scriptaculous simulates a mouse click on an anchor reference and then, without
ever spinning the event loop, eventually goes on to the next test in the line
modified here.

The click is handled asynchronously, so it happens after we do |testIframe.src
= nextTest|, so the anchor load triggered by the click cancels the nextTest
load.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> Do we want to include a test?  Do we want to write a test and check it in later?

Probably the latter....

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 607934 [details] [diff] [review]
Part 2, v1: Fix scriptaculous test.

r=me, I guess, but we should really look into making the event ordering match html5 for this stuff; I believe this change would not be needed then....

Comment 23

[Justin Lebar (not reading bugmail)]

With test change:

https://hg.mozilla.org/integration/mozilla-inbound/rev/34526f45d863
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a2c57fa8edf

Comment 25

[Lukas Blakk [:lsblakk] use ?needinfo]

[Triage Comment]
Leaving these approvals until landed on m-c and we get some bake time.

Comment 26

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/mozilla-central/rev/6a2c57fa8edf
https://hg.mozilla.org/mozilla-central/rev/34526f45d863

Comment 27

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

Verified on nightly trunk with all three attached testcases (two original urls and the local only one). 

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:14.0) Gecko/20120323 Firefox/14.0a1

Comment 28

[Alex Keybl [:akeybl]]

Comment on attachment 607852 [details] [diff] [review]
Patch v2

[Triage Comment]
Approved for Aurora 13, Beta 12, and ESR given the user impact.

Comment 29

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6bf25f8c7291
https://hg.mozilla.org/releases/mozilla-aurora/rev/3760953b2629

Comment 30

[Justin Lebar (not reading bugmail)]

I'm going to back out of Aurora and hold off landing elsewhere until we can get a handle on this regression.  Unclear whether there's anything we can do about it.

Comment 31

[Justin Lebar (not reading bugmail)]

Backed out from Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/662cd575c01a

Comment 32

[Justin Lebar (not reading bugmail)]

Boris, do I have r=you to back this change out on m-c?

Comment 33

[Justin Lebar (not reading bugmail)]

wrt the spec, step 6 of the navigation algorithm [1], which is run synchronously when a link is clicked, says

"If there is a preexisting attempt to navigate the browsing context, and either that attempt has not yet matured (i.e. it has not passed the point of making its Document the active document), or that navigation's resource is not to be fetched using HTTP GET or equivalent, or its resource's absolute URL differs from this attempt's by more than the presence, absence, or value of the <fragment> component, then cancel that preexisting attempt to navigate the browsing context."

So this implies that a fragment navigation cancels any other loads.

Presumably the link's onclick handler gets called before we run the navigation algorithm (because the onclick handler needs to be able to cancel the click event) -- in that case, it sounds to me that we're basically following the spec here.  So we may need to fix the spec, too.  :(

[1] http://www.whatwg.org/specs/web-apps/current-work/#navigate

Comment 34

[Justin Lebar (not reading bugmail)]

Oh...he's saying to cancel the load only if the URLs differ by *more* than the fragment.  So this should not cancel the load.

Comment 35

[Justin Lebar (not reading bugmail)]

So in the face of

 <a href="#foo" onclick="window.location='http://google.com'">

as I understand the spec, we're supposed to run onclick followed by #foo.

If that's the case, making the click synchronous, as we discussed earlier today, isn't going to help.

Comment 36

[Justin Lebar (not reading bugmail)]

Backed out from inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/15d5cdd96147

Comment 37

[Boris Zbarsky [:bzbarsky]]

Just to follow up, comment 35 was correct, and that was a spec bug.  Ian will be fixing that, but we need a different fix for this bug.

Comment 38

[Boris Zbarsky [:bzbarsky]]

Could we not change load types on short-circuited loads?  Or is mLoadType looked at after load completion by someone?

Comment 39

[Justin Lebar (not reading bugmail)]

(In reply to Boris Zbarsky (:bz) from comment #38)
> Could we not change load types on short-circuited loads?  Or is mLoadType
> looked at after load completion by someone?

I'm sure we could do this to solve this bug.  But I'm not sure we could do this without introducing any new bugs.  mLoadType is read in a bunch of places, e.g. AddToSessionHistory.

Now, maybe we could change the load type at the beginning of the short-circuited load and then change it back at the end of the load...

Comment 40

[Justin Lebar (not reading bugmail)]

Attachment: Patch v3

Comment 41

[Justin Lebar (not reading bugmail)]

Try push of patch v3: https://tbpl.mozilla.org/?tree=Try&rev=7bc85395c0dd

Comment 42

[Justin Lebar (not reading bugmail)]

Comment on attachment 610213 [details] [diff] [review]
Patch v3

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

Comment 43

[Ryan VanderMeulen [:RyanVM]]

The Try build works with INGDirect.

Comment 44

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 610213 [details] [diff] [review]
Patch v3

r=me if you use mozilla:AutoRestore<PRUint32> loadTypeResetter(mLoadType); instead of recreating AutoRestore.  ;)

Comment 45

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/612fd2e764fd

Comment 46

[Daniel Veditz [:dveditz]]

Does this latest patch also fix the Joomla issue in bug 739588?

Comment 47

[Ed Morley [:emorley]]

(In reply to Justin Lebar [:jlebar] from comment #45)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/612fd2e764fd

https://hg.mozilla.org/mozilla-central/rev/612fd2e764fd

Comment 48

[Justin Lebar (not reading bugmail)]

(In reply to Daniel Veditz [:dveditz] from comment #46)
> Does this latest patch also fix the Joomla issue in bug 739588?

Apparently so.

The web-facing change in the latest patch is much more tightly-scoped, so it's (hopefully) much less likely to break sites.

Comment 49

[Justin Lebar (not reading bugmail)]

Comment on attachment 610213 [details] [diff] [review]
Patch v3

This patch should be much less web-visible than the previous one.  (I didn't have to make any changes to the test suite to get it to pass, for example.)

Still risky, as always.

Comment 50

[Alex Keybl [:akeybl]]

Comment on attachment 610213 [details] [diff] [review]
Patch v3

[Triage Comment]
Approved for all branches. Please land on m-b before 2:30 PM PT so that this makes it into our fourth beta.

Comment 51

[Justin Lebar (not reading bugmail)]

Landed on beta for FF12: https://hg.mozilla.org/releases/mozilla-beta/rev/f9c358204bce

Comment 52

[Justin Lebar (not reading bugmail)]

Landed on aurora for FF13: https://hg.mozilla.org/releases/mozilla-aurora/rev/ac43257b27fe
Landed on ESR10: https://hg.mozilla.org/releases/mozilla-esr10/rev/51960baf2325

Comment 53

[u279076]

Can someone please clarify the expected result?

I downloaded a local copy of the testcase and opened it in Firefox.

RESULT:
1. Open file:///home/mozilla/Downloads/pushstate-sec.html and click "Click Here"
2. Opens a new tab and redirects to maps.google.com
3. Redirects back to file:///home/mozilla/Downloads/pushstate-sec.html#foo

Comment 54

[Justin Lebar (not reading bugmail)]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #53)
> Can someone please clarify the expected result?
> 
> I downloaded a local copy of the testcase and opened it in Firefox.
> 
> RESULT:
> 1. Open file:///home/mozilla/Downloads/pushstate-sec.html and click "Click
> Here"
> 2. Opens a new tab and redirects to maps.google.com
> 3. Redirects back to file:///home/mozilla/Downloads/pushstate-sec.html#foo

Yes, that's right.

What would be wrong is if it opened maps.google.com but displayed pushstate-sec.html as the URL.

Please try all the relevant testcases, both in this bug and in bug 687745.

Comment 55

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

Verified fixed in trunk as well: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:14.0) Gecko/20120416 Firefox/14.0a1.

Comment 56

[Raymond Forbes[:rforbes]]

rforbes-bugspam-for-setting-that-bounty-flag-20130719