User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; es-ES; rv:126.96.36.199) Gecko/2008070208 Firefox/1.5.0.xx Alexa Toolbar;MEGAUPLOAD 1.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; es-ES; rv:188.8.131.52) Gecko/2008070208 Firefox/1.5.0.xx Alexa Toolbar;MEGAUPLOAD 1.0
Steps to Reproduce:
One can visualize the address unresolved and the blank page amended.
Created attachment 335225 [details]
(modified slightly to make the document.written HTML more sane)
ccing some folks who've dealt with error pages in the past and might have bright ideas here.
(In reply to comment #6)
> window.open() followed by modifying the DOM or document.writing into the
> resulting document needs to work.
When this is exactly allowed? For example following code throws error in console "Permission denied to get property Window.document"
a = window.open("http://www.xxx.xyz%")
I've noticed that the bug doesn't appear if I change "http://www.google.com.ar%" in the testcase to some nonexistent domain (e.g. "www.xxx.xyz"). I've compared both cases and the difference is that '%' isn't allowed in host names and nsHttpChannel::Connect() fails immediately, error page starts loading and new SHEntry is put into mLSHE at http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#8096. Ignore different line numbers in my backtrace, I've put a lot of logging into nsDocShell.cpp...
#0 nsDocShell::OnNewURI (this=0xf60f7d80, aURI=0xee49bbe0, aChannel=0xf1a0b090, aLoadType=1, aFireOnLocationChange=1, aAddToGlobalHistory=0)
#1 0x015a1917 in nsDocShell::OnLoadingSite (this=0xf60f7d80, aChannel=0xf1a0b090, aFireOnLocationChange=1, aAddToGlobalHistory=0)
#2 0x015aac47 in nsDocShell::LoadErrorPage (this=0xf60f7d80, aURI=0xee49bbe0, aURL=0x0, aErrorPage=0xffebc294 "neterror", aErrorType=0xffebc158,
aDescription=0xffebc0c4, aCSSClass=0xffebc2e8 "", aFailedChannel=0xf1a0b090) at /opt/moz/TRUNK_new3/mozilla/docshell/base/nsDocShell.cpp:3284
#3 0x015ac579 in nsDocShell::DisplayLoadError (this=0xf60f7d80, aError=2152398878, aURI=0xee49bbe0, aURL=0x0, aFailedChannel=0xf1a0b090)
#4 0x015b88b7 in nsWebShell::EndPageLoad (this=0xf60f7d80, aProgress=0xf60f7d94, channel=0xf1a0b090, aStatus=2152398878)
#5 0x015a2e91 in nsDocShell::OnStateChange (this=0xf60f7d80, aProgress=0xf60f7d94, aRequest=0xf1a0b090, aStateFlags=131088, aStatus=2152398878)
#6 0x015ce4e4 in nsDocLoader::FireOnStateChange (this=0xf60f7d80, aProgress=0xf60f7d94, aRequest=0xf1a0b090, aStateFlags=131088, aStatus=2152398878)
#7 0x015cf12d in nsDocLoader::doStopDocumentLoad (this=0xf60f7d80, request=0xf1a0b090, aStatus=2152398878)
#8 0x015cf323 in nsDocLoader::DocLoaderIsEmpty (this=0xf60f7d80) at /opt/moz/TRUNK_new3/mozilla/uriloader/base/nsDocLoader.cpp:763
#9 0x015cfa0d in nsDocLoader::OnStopRequest (this=0xf60f7d80, aRequest=0xf1a0b090, aCtxt=0x0, aStatus=2152398878)
#10 0x00b8931f in nsLoadGroup::RemoveRequest (this=0xec90f9b0, request=0xf1a0b090, ctxt=0x0, aStatus=2152398878)
#11 0x00c554d8 in nsHttpChannel::AsyncAbort (this=0xf1a0b060, status=2152398878) at /opt/moz/TRUNK_new3/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp:363
#12 0x00c55979 in nsHttpChannel::AsyncOpen (this=0xf1a0b060, listener=0xee637c40, context=0x0)
#13 0x015c864b in nsURILoader::OpenURI (this=0xf62d4ca0, channel=0xf1a0b090, aIsContentPreferred=0, aWindowContext=0xf60f7d98)
#14 0x015987fc in nsDocShell::DoChannelLoad (this=0xf60f7d80, aChannel=0xf1a0b090, aURILoader=0xf62d4ca0, aBypassClassifier=0)
#15 0x0159dc16 in nsDocShell::DoURILoad (this=0xf60f7d80, aURI=0xee49bbe0, aReferrerURI=0xee49b9d0, aSendReferrer=1, aOwner=0xec96ad30, aTypeHint=0x0,
aPostData=0x0, aHeadersData=0x0, aFirstParty=1, aDocShell=0x0, aRequest=0xffebcea4, aIsNewWindowTarget=1, aBypassClassifier=0)
#16 0x015a603f in nsDocShell::InternalLoad (this=0xf60f7d80, aURI=0xee49bbe0, aReferrer=0xee49b9d0, aOwner=0xec96ad30, aFlags=8, aWindowTarget=0xeed28576,
aTypeHint=0x0, aPostData=0x0, aHeadersData=0x0, aLoadType=1, aSHEntry=0x0, aFirstParty=1, aDocShell=0x0, aRequest=0x0)
#17 0x0159e9d7 in nsDocShell::LoadURI (this=0xf60f7d80, aURI=0xee49bbe0, aLoadInfo=0xee531740, aLoadFlags=16384, aFirstParty=1)
#18 0x01607f4f in nsWindowWatcher::OpenWindowJSInternal (this=0xf62841f0, aParent=0xf0118c80, aUrl=0xec92cb08 "http://www.xxx.xyz%", aName=0x0, aFeatures=0x0,
aDialog=0, argv=0x0, aCalledFromJS=1, _retval=0xffebd7b8) at /opt/moz/TRUNK_new3/mozilla/embedding/components/windowwatcher/src/nsWindowWatcher.cpp:909
#19 0x01608466 in nsWindowWatcher::OpenWindowJS (this=0xf62841f0, aParent=0xf0118c80, aUrl=0xec92cb08 "http://www.xxx.xyz%", aName=0x0, aFeatures=0x0, aDialog=0,
argv=0x0, _retval=0xffebd7b8) at /opt/moz/TRUNK_new3/mozilla/embedding/components/windowwatcher/src/nsWindowWatcher.cpp:484
#20 0x012c5342 in nsGlobalWindow::OpenInternal (this=0xf0118c80, aUrl=@0xffebd9cc, aName=@0xffebd938, aOptions=@0xffebd8a4, aDialog=0, aContentModal=0,
aCalledNoScript=0, aDoJSFixups=1, argv=0x0, aExtraArgument=0x0, aCalleePrincipal=0xec96ad30, aJSCallerContext=0xef553580, aReturn=0xffebdc10)
#21 0x012c661f in nsGlobalWindow::Open (this=0xf0118c80, _retval=0xffebdc10) at /opt/moz/TRUNK_new3/mozilla/dom/src/base/nsGlobalWindow.cpp:5059
Then when a.document.write("<title>test</title>") is called, URL isn't rewritten because of mLSHE in condition at http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#5020.
#0 nsDocShell::OnStateChange (this=0xf60f8140, aProgress=0xf60f8154, aRequest=0xec910120, aStateFlags=983041, aStatus=0)
#1 0x015ce4e4 in nsDocLoader::FireOnStateChange (this=0xf60f8140, aProgress=0xf60f8154, aRequest=0xec910120, aStateFlags=983041, aStatus=0)
#2 0x015cfb83 in nsDocLoader::doStartDocumentLoad (this=0xf60f8140) at /opt/moz/TRUNK_new3/mozilla/uriloader/base/nsDocLoader.cpp:796
#3 0x015cfddc in nsDocLoader::OnStartRequest (this=0xf60f8140, request=0xec910120, aCtxt=0x0) at /opt/moz/TRUNK_new3/mozilla/uriloader/base/nsDocLoader.cpp:528
#4 0x00b896b0 in nsLoadGroup::AddRequest (this=0xec90f080, request=0xec910120, ctxt=0x0) at /opt/moz/TRUNK_new3/mozilla/netwerk/base/src/nsLoadGroup.cpp:603
#5 0x01195a23 in nsHTMLDocument::CreateAndAddWyciwygChannel (this=0xec971800) at /opt/moz/TRUNK_new3/mozilla/content/html/document/src/nsHTMLDocument.cpp:3701
#6 0x0119994a in nsHTMLDocument::OpenCommon (this=0xec971800, aContentType=@0xffebd8f4, aReplace=0)
#7 0x01199a49 in nsHTMLDocument::Open (this=0xec971800, aContentType=@0xffebd8f4, aReplace=0, aReturn=0xffebd8ec)
#8 0x011913c5 in nsHTMLDocument::Open (this=0xec971800) at /opt/moz/TRUNK_new3/mozilla/content/html/document/src/nsHTMLDocument.cpp:2335
#9 0x01197b66 in nsHTMLDocument::WriteCommon (this=0xec971800, aText=@0xffebda40, aNewlineTerminate=0)
#10 0x011980f2 in nsHTMLDocument::ScriptWriteCommon (this=0xec971800, aNewlineTerminate=0)
#11 0x011982c9 in nsHTMLDocument::Write (this=0xec971800) at /opt/moz/TRUNK_new3/mozilla/content/html/document/src/nsHTMLDocument.cpp:2563
If I set mLSHE=0 manually in GDB then URL is set correctly. But I'm not sure, how exactly to fix this bug. Should we change the condition? Or should a.document.write() fail with "Permission denied to get property Window.document" because loading of error page is already in progress?
Don't set a timeout. Do your stuff immediately after the open() call. And if you want, don't pass a URI to window.open(). Then write stuff into it. Then set its location.
Need a bit to think about comment 9.
OK, I see. So the key is that the document.write happens in the middle of the error page load, right? At a point after we have mLSHE but before the document the script sees is the error page document.
I guess for normal loads we Embed() the new viewer right after the OnLoadingSite call that sets up mLSHE, so the mLSHE being present in fact corresponds to our document being the new document.
Hrm. So how come mLoadType is LOAD_NORMAL when we get here, because we manually set it in OpenCommon.
Would it work to just Stop() the docshell in OpenCommon()? Or to have PrepareForNewContentModel() kill off a pending error page load?
(In reply to comment #12)
> OK, I see. So the key is that the document.write happens in the middle of the
> error page load, right? At a point after we have mLSHE but before the document
> the script sees is the error page document.
I think so.
> I guess for normal loads we Embed() the new viewer right after the
> OnLoadingSite call that sets up mLSHE, so the mLSHE being present in fact
> corresponds to our document being the new document.
Yes, this is in nsDocShell::CreateContentViewer().
(In reply to comment #13)
> Hrm. So how come mLoadType is LOAD_NORMAL when we get here, because we
> manually set it in OpenCommon.
> Would it work to just Stop() the docshell in OpenCommon()? Or to have
I've tried that and calling Stop() in OpenCommon() doesn't help.
> PrepareForNewContentModel() kill off a pending error page load?
Hmm, maybe, but how to do it correctly? Calling Stop() at this place doesn't work either.
Hrm. I'm not sure I know, if Stop() doesn't work. Maybe mail biesi?
what happend with this bug?
Nothing, so far. Not sure whether Michal ever mailed biesi.
What about making document.open() throw if the docshell is in the middle of an error page load?
(In reply to comment #17)
> Nothing, so far. Not sure whether Michal ever mailed biesi.
I've sent the mail but received no reply :-/
> What about making document.open() throw if the docshell is in the middle of an
> error page load?
OK, throwing error in nsHTMLDocument::OpenCommon() when error page is loading helps. But how to correctly find out that we are loading the page right now? mBusyFlags in docshell isn't set correctly at this point. Is it always when mLSHE is non-null? If yes, then new method in nsIDocShell is needed to check this condition :-/
Hmm. I guess just looking at the load type is not good enough because the error page might be done loading and in that case we shouldn't prevent the write()?
I guess that argument can apply to the loading case too. So maybe we should really figure out why the Stop() approach didn't work.
This bug was made public by the reporter on July 24: http://www.securityfocus.com/archive/1/505242/30/0/threaded
Boris said he'd look at this since Michal is out.
http://es.geocities.com/jplopezy/firefoxspoofing.html is another testcase.
(In reply to comment #23)
> http://es.geocities.com/jplopezy/firefoxspoofing.html is another testcase.
except that just seems to be Jesse's testcase.
(In reply to comment #24)
> (In reply to comment #23)
> > http://es.geocities.com/jplopezy/firefoxspoofing.html is another testcase.
> except that just seems to be Jesse's testcase.
Never mind. I misread.
The basic issue is as diagnosed in comment 9. Stop() doesn't help because it doesn't do anything with mLSHE directly. Error pages never clear mLSHE because they never actually hit EndPageLoad.
I talked this over with biesi, and we think the safest fix is to just have Stop() reset mLSHE for error page loads to null. Building and testing that now.
Created attachment 391176 [details] [diff] [review]
I'd welcome someone figuring out a way to automatically test this....
mmh. it does not work for me! The URL in the test case says "https://bug451898.bugzilla.mozilla.org/attachment.cgi?id=335225" in the tab that is opened. Nor does the test case from geocities work (not that there be much differnce, but still). Maybe it's one of my addons? (Adblock Plus, DownloadHelper, Linkification, Locationbar², NoScript, SkipScreen, TACO, Xmarks)
Lorenz, it would be quite easy for extensions to interfere with this, as well as with a whole bunch of other things.
Comment on attachment 391176 [details] [diff] [review]
Should take this on branches. This should be a very safe patch, in general: by design it only affects cases when stop() is called (or a new load is started, which always calls stop()) while an error page is loading or is showing. In those cases, it makes sure to make the error page load look basically like a normal load that never completed until that point (which is what it really is, from the point of view of the rest of the system).
Comment on attachment 391176 [details] [diff] [review]
Approved for 184.108.40.206 and 220.127.116.11. a=ss
Checking in docshell/base/nsDocShell.cpp;
/cvsroot/mozilla/docshell/base/nsDocShell.cpp,v <-- nsDocShell.cpp
new revision: 1.915; previous revision: 1.914
The URL in the test case shows "https://bug451898.bugzilla.mozilla.org/attachment.cgi?id=335225" in the tab that is opened.
Is this right behavior?
I tried with new profile and the following hourly build
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090730 Minefield/3.6a1pre ID:20090730062258
Alice, that's the right post-patch behavior, yes.
Verified fixed on trunk and 1.9.1 with builds on all platforms like:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090730 Minefield/3.6a1pre (.NET CLR 3.5.30729) ID:20090730041801
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:18.104.22.168pre) Gecko/20090730 Shiretoko/3.5.3pre (.NET CLR 3.5.30729) ID:20090730044055
Verified on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:22.214.171.124) Gecko/2009073022 Firefox/3.0.13 (.NET CLR 3.5.30729)
Using test cases in comment #1 and comment #23. 3.0.11 opens a tab with a fake url. 3.0.13 shows the right url.
Boris, does this bug affect 1.8.1.x, too? That is, is the bug the fake URL appearing in the location bar, the document.write()ing of arbitrary content in the content area, either, or both? In my latest Camino builds off of 1.8.1.x, I see the fake URL in the location bar, but no document.write()-generated page content.
The bug is the combination of the two. I don't recall enough about the security model around about:blank in 1.8.1 to say anything useful about whether the behavior you see is reliable....
If you plan to release things off 1.8.1.x, I'd just backport this patch.
Created attachment 395537 [details] [diff] [review]
Verified for 126.96.36.199 using the two testcases (as in comment 38) and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:188.8.131.52pre) Gecko/2009081305 GranParadiso/3.0.14pre (.NET CLR 3.5.30729).
this bug is still present in the release version of firefox 3.5.3
(In reply to comment #43)
> this bug is still present in the release version of firefox 3.5.3
Looks fixed to me on Mozilla/5.0 (X11; U; Linux i686; en-US; rv:184.108.40.206) Gecko/20090824 Firefox/3.5.3. Can you go to about: and post your build identifier here? What are you using as your testcase?
Created attachment 406014 [details]
This really doesn't seems to me fixed on 220.127.116.11 (Build identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:18.104.22.168) Gecko/20090824 Firefox/3.5.3 (.NET CLR 3.5.30729)).
If the issue is that a fake page carries identity of the current ssl page, then using the test case does exactly that.
> If the issue is that a fake page carries identity of the current ssl page
It's not. The issue was that the fake page had SSL indicators and a URI of the current SSL page's choosing (NOT the URI of the current SSL page).
And in particular, a page generated via document.write had better carry exactly the same SSL and identity indicators as the generating page.
Marking not blocking 1.8.1.next so we can ship a Thunderbird update, don't know if SeaMonkey 1.1.x or Camino still need this fix.
(In reply to comment #48)
> Marking not blocking 1.8.1.next so we can ship a Thunderbird update, don't know
> if SeaMonkey 1.1.x or Camino still need this fix.
If it affects SeaMonkey 1.1.18, we probably would like this in a 1.1.19 that we're planning to ship in sync with the Thunderbird update, but I'll make this SeaMonkey 1.1.19 a "ship what we can get" release that might ship with known vulnerabilities but be better than 1.1.18 at least. The only really secure path for SeaMonkey users is moving to 2.x in any case.
It's unclear whether this affects Camino 1.6.x or not (see comment 39 and comment 40). As I recall, the then-current 1.8.1.x behavior didn't match the then-current (pre-patched) 1.9.x behavior but the current 1.8.1.x behavior (I think the difference was the absence of generated content on the resulting page on 1.8.1.x), and certainly the current 1.8.1.x behavior doesn't match the current (patched) 1.9.x behavior.
Current 1.8.1.x behavior with Jesse's testcase in comment 1:
New window with title "test", http://www.google.com.ar%/ in the location bar, error page site icon, no SSL indicators, and no page content
Patched 1.8.1.x behavior (with patch in comment 41):
New window with title "test", https://bug451898.bugzilla.mozilla.org/attachment.cgi?id=335225 (aka the bug attachment) in the location bar, site icon for the bug attachment, SSL indicators to match the bug attachment, and no page content
This bug was nominated for 1.8.1.x-next, though, so clearly people do believe 1.8.1.x is vulnerable to something, and the patch improves things by clearing the bogus domain and site icon from the location bar (which seems like comment 47), so I'll second KaiRo; we'd also like to see this patch in the forthcoming 1.8.1.next for our Camino 1.6.11 release.
I'll be happy to land it, even, if everyone agrees the 1.8.1 patch does what it needs to do.