User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.0; fr; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2 (.NET CLR 3.5.30729) Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; fr; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2 (.NET CLR 3.5.30729) This vulnerability is similare to http://www.mozilla.org/security/announce/2009/mfsa2009-44.html The problem originated is from the creation of a link with javascript (window.open & document.location) . When you enter an incorrect address seems that the firefox can't translate to address web and display it in blank page. This blank page can be changed with javascript with making a spoofing attak. Reproducible: Always Steps to Reproduce: Steps to Reproduce : Step 1=> Create an html file with a special javascript or open http://www.seeon.fr/blog/public/spoofurl.html Step 2=>Open the page Actual Results: One can visualize the address unresolved and the fake page. similary to http://www.mozilla.org/security/announce/2009/mfsa2009-44.html the problem is that with javascript window can be changed by altering the content and halting the loading of the page. Function Javascript used : window.open window.stop document.write document.location

This is public on his blog, not sure making it a private bug makes sense. http://www.seeon.fr/blog/index.php?post/2009/09/02/Mozilla-Firefox-3.*.*-URL-SPOOFING

Meant to confirm the bug. The difference between this case and bug 451898 appears to be the addition of setting document.location after writing to the error page.

this POC demonstrates that it is possible to spoof the URL without the character that generates the invalid url is visible

OK, so the key is that setting location to something that triggers an error page load immediately changes the docshell's URI. Then calling stop() stops the error page load, so we never show the error page. But we've already changed the URI. The document.write is only relevant in that it lets us inject content of our choosing. A much simpler POC: javascript:window.location = 'https://www.google.com%';window.stop();void(0); Run this on any page of your choice. This bug page is a good testcase. I think we really need to change the behavior where error pages eagerly set the URI. Christian, thoughts?

I suppose that should be fine. I kinda think that several issues could be avoided if we could make the error page load synchronous and not trigger any events, but that's probably not really possible...

Blocking 1.9.2+.

Will try to catch this next time once we figure out how to fix it for 1.9.2

Honza, if you want to take a crack at this just let me know. Otherwise the plan is that I'll try to do it this next week.

bz, Honza says you guys talked about this and that you were going to fix this. Please work on this asap, we're getting close to 1.9.2 rc...

Now sure you will like this patch, but I simply disallow stop of the error page load. When call to window.stop() is removed from the test case, we don't get the bug. Only problem I see is that mLSHE is wiped out but we are still loading the error page. Is there a strong reason not to get progress from the error page load, to get EndPageLoad?

Comment on attachment 410242 [details] [diff] [review] v1 This makes the error page load also not stop if the user is trying to load something else (e.g. clicked the home button), which means the two loads can race and the error page can stomp on the thing the user is trying to load. That's why I didn't do it that way in the first place...

Another insane patch. This prevents stop of an error page load only when not called from chrome or system; i.e. it will not allow scripts stop the error page load.

Comment on attachment 410531 [details] [diff] [review] v2 Subject principal isn't system during a location set....

And more importantly, we can't allow the racing-load situation, period, ever. If it happens, bad things happen.

The way I want to try to go now: If I change the location assignment from "google.com%20%20 etc" to a valid address ("https://www.google.com/" for example) then the resulting URL in the address bar is the URL of the page trying to spoof (of the opener). To have this behavior also for cases while we are loading error page, we solve this bug.

Right. In other words, the error page load needs to not set the uri to the error page uri until the error page document viewer actually embeds itself. Or something.

Delayed current uri assignment and history entry creation. A bit tricky but should be ok.

Yeah, that's more like what I was thinking! Thank you! A few comments offhand, without having dug into this in detail yet: 1) Stop() should probably clear out at least mFailedURI and mFailedChannel, right? 2) The comment seems to describe the exploit in detail. Is that desirable? 3) Might it make more sense to put this call next to the "normal" OnLoadingSite call in CreateContentViewer? Presumably right before it...

As you suggest.

Comment on attachment 410752 [details] [diff] [review] v3.1 >+++ b/docshell/base/nsDocShell.cpp > nsDocShell::Stop(PRUint32 aStopFlags) > if (mLoadType == LOAD_ERROR_PAGE && mLSHE) { ... >+ mFailedChannel = nsnull; >+ mFailedURI = nsnull; I think we want to do that even if !mLSHE. >+ PRUint32 currentLoadType = mLoadType; That's guaranteed to be LOAD_ERROR_PAGE. Why have the variable at all? >+ mFailedChannel = nsnull; >+ mFailedURI = nsnull; I would prefer we null those (by swapping into locals, probably) before the OnNewURI calls and such: those can trigger js execution which might start new loads and set a new mFailedChannel/mFailedURI... >+++ b/docshell/base/nsDocShell.h >+ // XXX Update the comment correctly Yes, please do. >+++ b/security/manager/ssl/tests/mochitest/bugs/test_bug514232.html This should probably go in docshell mochitests instead, no? And shouldn't get checked in until we open up the bug? r=bzbarsky with those issues addressed. Thanks for picking this up!

(In reply to comment #23) > (From update of attachment 410752 [details] [diff] [review]) > >+ PRUint32 currentLoadType = mLoadType; > > That's guaranteed to be LOAD_ERROR_PAGE. Why have the variable at all? > I just tough about a future-compatibility when some other flags would be specified during load (not just the LOAD_ERROR_PAGE constant). Don't you think this is safer?

In general, yes, but right now you have an mLoadType check right above this line...

(In reply to comment #23) > r=bzbarsky with those issues addressed. Please attach a patch incorporating these changes when you request branch approval (so downstream distros don't take an incomplete patch).

r=bzbarsky introduced changes from comment 23. will be landed w/o the test until the bug opens.

Please replace this: >+ // for what these objects are needed. With: // for which these objects are needed.

Comment on attachment 411744 [details] [diff] [review] v3.2 [Checkin comment 29] http://hg.mozilla.org/mozilla-central/rev/59ca9e3e4ef9 Fixed according to comment 28

Honza: Can you get this on 1.9.2 today as well?

Here it is, built+tested on 1.9.2.

Comment on attachment 411792 [details] [diff] [review] v3.3 (w/o a test) for 1.9.2 [Checkin 1.9.2 comment 33] This is a blocker, so no need for explicit approval. Please land whenever possible.

Comment on attachment 411792 [details] [diff] [review] v3.3 (w/o a test) for 1.9.2 [Checkin 1.9.2 comment 33] http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8467ea52b569

Unexpected TXul and DHTML talos wins? http://groups.google.com/group/mozilla.dev.tree-management/browse_thread/thread/840c1cf7f812d450#

Unexpected is right. If we're hitting this code during Txul/Tdhtml, something is very wrong.

Comment on attachment 411744 [details] [diff] [review] v3.2 [Checkin comment 29] If this patch requires changes for 1.9.1 or 1.9.0 please attach "as-checked-in" patches. Approved for 1.9.1.6 and 1.9.0.16, a=dveditz for release-drivers

Honza: Any update on getting this landed on 1.9.1 and 1.9.0?

Samuel, going to do it now.

This caused a Tp4 increase on Linux, can't remember if that was known or understood or not.

Checking in docshell/base/nsDocShell.cpp; /cvsroot/mozilla/docshell/base/nsDocShell.cpp,v <-- nsDocShell.cpp new revision: 1.916; previous revision: 1.915 done Checking in docshell/test/Makefile.in; /cvsroot/mozilla/docshell/test/Makefile.in,v <-- Makefile.in new revision: 1.16; previous revision: 1.15 done RCS file: /cvsroot/mozilla/docshell/test/bug529119-window.html,v done Checking in docshell/test/bug529119-window.html; /cvsroot/mozilla/docshell/test/bug529119-window.html,v <-- bug529119-window.html initial revision: 1.1 done RCS file: /cvsroot/mozilla/docshell/test/test_bug529119-1.html,v done Checking in docshell/test/test_bug529119-1.html; /cvsroot/mozilla/docshell/test/test_bug529119-1.html,v <-- test_bug529119-1.html initial revision: 1.1 done RCS file: /cvsroot/mozilla/docshell/test/test_bug529119-2.html,v done Checking in docshell/test/test_bug529119-2.html; /cvsroot/mozilla/docshell/test/test_bug529119-2.html,v <-- test_bug529119-2.html initial revision: 1.1 done

And missing header file changes, fixed build bustage: Checking in docshell/base/nsDocShell.h; /cvsroot/mozilla/docshell/base/nsDocShell.h,v <-- nsDocShell.h new revision: 1.228; previous revision: 1.227 done

Comment on attachment 413088 [details] [diff] [review] as landed on 1.9.0 [Checkin comment 41] Backed out because of build bustage.

This is adjusted for 1.9.0 and includes patch (fix+test) for regression bug 529119 for which I actually ask aproval.

Comment on attachment 413157 [details] [diff] [review] v3.3 for 1.9.0 [Checkin 1.9.0 comment 46] Approved for 1.9.0.16, a=dveditz for release-drivers When you land this please add the fixed1.9.0.16 keyword to bug 529119 as well

Comment on attachment 413157 [details] [diff] [review] v3.3 for 1.9.0 [Checkin 1.9.0 comment 46] Checking in docshell/base/nsDocShell.cpp; /cvsroot/mozilla/docshell/base/nsDocShell.cpp,v <-- nsDocShell.cpp new revision: 1.918; previous revision: 1.917 done Checking in docshell/base/nsDocShell.h; /cvsroot/mozilla/docshell/base/nsDocShell.h,v <-- nsDocShell.h new revision: 1.230; previous revision: 1.229 done Checking in docshell/test/Makefile.in; /cvsroot/mozilla/docshell/test/Makefile.in,v <-- Makefile.in new revision: 1.18; previous revision: 1.17 done Checking in docshell/test/bug529119-window.html; /cvsroot/mozilla/docshell/test/bug529119-window.html,v <-- bug529119-window.html new revision: 1.3; previous revision: 1.2 done Checking in docshell/test/test_bug529119-1.html; /cvsroot/mozilla/docshell/test/test_bug529119-1.html,v <-- test_bug529119-1.html new revision: 1.3; previous revision: 1.2 done Checking in docshell/test/test_bug529119-2.html; /cvsroot/mozilla/docshell/test/test_bug529119-2.html,v <-- test_bug529119-2.html new revision: 1.3; previous revision: 1.2 done

Verified using attached manual testcases with 1.9.1.6 (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.6pre) Gecko/20091119 Shiretoko/3.5.6pre (.NET CLR 3.5.30729)) and 1.9.0.16 (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.16pre) Gecko/2009111921 GranParadiso/3.0.16pre (.NET CLR 3.5.30729)).