Closed Bug 737307 (CVE-2012-0474) Opened 12 years ago Closed 12 years ago

universal XSS by confusing docshell with short-circuited loads

Categories

(Core :: DOM: Navigation, defect)

11 Branch
x86
Linux
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla14
Tracking Status
firefox11 --- wontfix
firefox12 + verified
firefox13 + verified
firefox14 + verified
firefox-esr10 12+ verified

People

(Reporter: cmcgowen.dev, Assigned: justin.lebar+bug)

References

Details

(Whiteboard: [sg:high][qa!])

Attachments

(3 files, 3 obsolete files)

Attached file 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.
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
Component: Untriaged → Document Navigation
Product: Firefox → Core
QA Contact: untriaged → docshell
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
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/.
This happens if we substitute a hash navigation for the pushstate.
Attachment #607744 - Attachment description: Testcase with hash navigation instead of pushstate → Testcase with hash navigation instead of pushstate (must be run locally)
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.
I was able to leverage this bug as a XSS on maps.google.com through a requested javascript file.
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?
> 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).
> 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.
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Assignee: nobody → justin.lebar+bug
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #607822 - Flags: review?(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... :(
Attachment #607822 - Flags: review?(bzbarsky) → review-
> 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?
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?
Attached patch Patch v2 (obsolete) — Splinter Review
Ugh.
Attachment #607822 - Attachment is obsolete: true
Attachment #607852 - Flags: review?(bzbarsky)
Comment on attachment 607852 [details] [diff] [review]
Patch v2

NS_ERROR_BINDING_ABORTED is better.

And r=me, assuming this fixes the bug.
Attachment #607852 - Flags: review?(bzbarsky) → review+
Do we want to include a test?  Do we want to write a test and check it in later?
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
Attachment #607852 - Flags: approval-mozilla-beta?
Attachment #607852 - Flags: approval-mozilla-aurora?
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.
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
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.
Attachment #607934 - Flags: review?(bzbarsky)
> Do we want to include a test?  Do we want to write a test and check it in later?

Probably the latter....
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....
Attachment #607934 - Flags: review?(bzbarsky) → review+
Attachment #607934 - Flags: approval-mozilla-beta?
Attachment #607934 - Flags: approval-mozilla-aurora?
Attachment #607852 - Flags: approval-mozilla-esr10?
Attachment #607934 - Flags: approval-mozilla-esr10?
[Triage Comment]
Leaving these approvals until landed on m-c and we get some bake time.
https://hg.mozilla.org/mozilla-central/rev/6a2c57fa8edf
https://hg.mozilla.org/mozilla-central/rev/34526f45d863
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
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
Status: RESOLVED → VERIFIED
Comment on attachment 607852 [details] [diff] [review]
Patch v2

[Triage Comment]
Approved for Aurora 13, Beta 12, and ESR given the user impact.
Attachment #607852 - Flags: approval-mozilla-esr10?
Attachment #607852 - Flags: approval-mozilla-esr10+
Attachment #607852 - Flags: approval-mozilla-beta?
Attachment #607852 - Flags: approval-mozilla-beta+
Attachment #607852 - Flags: approval-mozilla-aurora?
Attachment #607852 - Flags: approval-mozilla-aurora+
Attachment #607934 - Flags: approval-mozilla-esr10?
Attachment #607934 - Flags: approval-mozilla-esr10+
Attachment #607934 - Flags: approval-mozilla-beta?
Attachment #607934 - Flags: approval-mozilla-beta+
Attachment #607934 - Flags: approval-mozilla-aurora?
Attachment #607934 - Flags: approval-mozilla-aurora+
Summary: history.pushState during load of foreign site → Attack page can fake location (load a.com as b.com) by confusing docshell with short-circuited loads
Depends on: 739478
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.
Depends on: 739588
Boris, do I have r=you to back this change out on m-c?
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
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.
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.
Summary: Attack page can fake location (load a.com as b.com) by confusing docshell with short-circuited loads → universal XSS by confusing docshell with short-circuited loads
Backed out from inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/15d5cdd96147
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
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.
Could we not change load types on short-circuited loads?  Or is mLoadType looked at after load completion by someone?
(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...
Attached patch Patch v3Splinter Review
The Try build works with INGDirect.
Comment on attachment 610213 [details] [diff] [review]
Patch v3

r=me if you use mozilla:AutoRestore<PRUint32> loadTypeResetter(mLoadType); instead of recreating AutoRestore.  ;)
Attachment #610213 - Flags: review?(bzbarsky) → review+
Does this latest patch also fix the Joomla issue in bug 739588?
Blocks: 687745
(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
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
(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.
Attachment #607852 - Attachment is obsolete: true
Attachment #607934 - Attachment is obsolete: true
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.
Attachment #610213 - Flags: approval-mozilla-esr10?
Attachment #610213 - Flags: approval-mozilla-beta?
Attachment #610213 - Flags: approval-mozilla-aurora?
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.
Attachment #610213 - Flags: approval-mozilla-esr10?
Attachment #610213 - Flags: approval-mozilla-esr10+
Attachment #610213 - Flags: approval-mozilla-beta?
Attachment #610213 - Flags: approval-mozilla-beta+
Attachment #610213 - Flags: approval-mozilla-aurora?
Attachment #610213 - Flags: approval-mozilla-aurora+
Whiteboard: [sg:high] → [sg:high][qa+]
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
(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.
Verified fixed in trunk as well: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:14.0) Gecko/20120416 Firefox/14.0a1.
Status: RESOLVED → VERIFIED
Alias: CVE-2012-0474
Whiteboard: [sg:high][qa+] → [sg:high][qa!]
Group: core-security
rforbes-bugspam-for-setting-that-bounty-flag-20130719
Flags: sec-bounty+
You need to log in before you can comment on or make changes to this bug.