1.05 KB, text/plain
1.05 KB, text/html
2.53 KB, patch
|Details | Diff | Splinter Review|
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
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/.
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.
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.
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.
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.
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... :(
> 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?
Created attachment 607852 [details] [diff] [review] Patch v2 Ugh.
Comment on attachment 607852 [details] [diff] [review] Patch v2 NS_ERROR_BINDING_ABORTED is better. And r=me, assuming this fixes the bug.
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
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
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.
> 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....
[Triage Comment] Leaving these approvals until landed on m-c and we get some bake time.
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 on attachment 607852 [details] [diff] [review] Patch v2 [Triage Comment] Approved for Aurora 13, Beta 12, and ESR given the user impact.
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.
Backed out from Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/662cd575c01a
Boris, do I have r=you to back this change out on m-c?
wrt the spec, step 6 of the navigation algorithm , 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. :(  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.
Backed out from inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/15d5cdd96147
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...
Try push of patch v3: https://tbpl.mozilla.org/?tree=Try&rev=7bc85395c0dd
Comment on attachment 610213 [details] [diff] [review] Patch v3 https://tbpl.mozilla.org/?tree=Try&rev=7bc85395c0dd
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. ;)
Does this latest patch also fix the Joomla issue in bug 739588?
(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
(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 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 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.
Landed on beta for FF12: https://hg.mozilla.org/releases/mozilla-beta/rev/f9c358204bce
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
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.