Bug 737307 (CVE-2012-0474)

universal XSS by confusing docshell with short-circuited loads

VERIFIED FIXED in Firefox 12

Status

()

Core
Document Navigation
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: Chris McGowen, Assigned: Justin Lebar (not reading bugmail))

Tracking

11 Branch
mozilla14
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox11 wontfix, firefox12+ verified, firefox13+ verified, firefox14+ verified, firefox-esr1012+ verified)

Details

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

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 607410 [details]
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.
(Reporter)

Comment 1

5 years ago
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
(Reporter)

Comment 2

5 years ago
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
(Assignee)

Comment 3

5 years ago
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/.
(Assignee)

Comment 4

5 years ago
Created attachment 607744 [details]
Testcase with hash navigation instead of pushstate (must be run locally)

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

Updated

5 years ago
Attachment #607744 - Attachment description: Testcase with hash navigation instead of pushstate → Testcase with hash navigation instead of pushstate (must be run locally)
(Assignee)

Comment 5

5 years ago
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.
(Reporter)

Comment 6

5 years ago
I was able to leverage this bug as a XSS on maps.google.com through a requested javascript file.
(Assignee)

Comment 7

5 years ago
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?
(Assignee)

Comment 8

5 years ago
> 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.
(Assignee)

Comment 10

5 years ago
Created attachment 607822 [details] [diff] [review]
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.
Assignee: nobody → justin.lebar+bug
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #607822 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
status-firefox11: --- → affected
status-firefox14: --- → affected
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-
(Assignee)

Comment 12

5 years ago
> 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?
(Assignee)

Comment 14

5 years ago
Created attachment 607852 [details] [diff] [review]
Patch v2

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+
(Assignee)

Comment 16

5 years ago
Do we want to include a test?  Do we want to write a test and check it in later?
(Assignee)

Comment 17

5 years ago
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?
(Assignee)

Comment 18

5 years ago
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.
(Assignee)

Comment 19

5 years ago
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
(Assignee)

Comment 20

5 years ago
Created attachment 607934 [details] [diff] [review]
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.
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+
(Assignee)

Comment 23

5 years ago
With test change:

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

Updated

5 years ago
Attachment #607934 - Flags: approval-mozilla-beta?
Attachment #607934 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

5 years ago
Attachment #607852 - Flags: approval-mozilla-esr10?
(Assignee)

Updated

5 years ago
Attachment #607934 - Flags: approval-mozilla-esr10?
status-firefox-esr10: --- → affected
status-firefox12: --- → affected
status-firefox13: --- → affected
tracking-firefox-esr10: --- → ?
tracking-firefox12: --- → +
tracking-firefox13: --- → +
tracking-firefox14: --- → +
Whiteboard: [sg:high]
[Triage Comment]
Leaving these approvals until landed on m-c and we get some bake time.
(Assignee)

Comment 26

5 years ago
https://hg.mozilla.org/mozilla-central/rev/6a2c57fa8edf
https://hg.mozilla.org/mozilla-central/rev/34526f45d863
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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
status-firefox11: affected → wontfix
tracking-firefox-esr10: ? → 12+
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+

Updated

5 years ago
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+
(Assignee)

Updated

5 years ago
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
(Assignee)

Comment 29

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/6bf25f8c7291
https://hg.mozilla.org/releases/mozilla-aurora/rev/3760953b2629
Depends on: 739478
(Assignee)

Comment 30

5 years ago
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.
(Assignee)

Comment 31

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

Updated

5 years ago
Depends on: 739588
(Assignee)

Comment 32

5 years ago
Boris, do I have r=you to back this change out on m-c?
(Assignee)

Comment 33

5 years ago
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
(Assignee)

Comment 34

5 years ago
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.
(Assignee)

Comment 35

5 years ago
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
(Assignee)

Comment 36

5 years ago
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?
(Assignee)

Comment 39

5 years ago
(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...
(Assignee)

Comment 40

5 years ago
Created attachment 610213 [details] [diff] [review]
Patch v3
(Assignee)

Comment 41

5 years ago
Try push of patch v3: https://tbpl.mozilla.org/?tree=Try&rev=7bc85395c0dd
(Assignee)

Comment 42

5 years ago
Comment on attachment 610213 [details] [diff] [review]
Patch v3

https://tbpl.mozilla.org/?tree=Try&rev=7bc85395c0dd
Attachment #610213 - Flags: review?(bzbarsky)
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+
(Assignee)

Comment 45

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/612fd2e764fd
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
Last Resolved: 5 years ago5 years ago
status-firefox14: affected → fixed
Resolution: --- → FIXED
(Assignee)

Comment 48

5 years ago
(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.
(Assignee)

Updated

5 years ago
Attachment #607852 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #607934 - Attachment is obsolete: true
(Assignee)

Comment 49

5 years ago
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+
(Assignee)

Comment 51

5 years ago
Landed on beta for FF12: https://hg.mozilla.org/releases/mozilla-beta/rev/f9c358204bce
status-firefox12: affected → fixed
(Assignee)

Comment 52

5 years ago
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
status-firefox-esr10: affected → fixed
status-firefox13: affected → fixed
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
(Assignee)

Comment 54

5 years ago
(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

Updated

5 years ago
status-firefox12: fixed → verified

Updated

5 years ago
status-firefox13: fixed → verified

Updated

5 years ago
status-firefox-esr10: fixed → verified

Updated

5 years ago
status-firefox14: fixed → verified

Updated

5 years ago
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.