window.open("about:blank"); content.location = "about:blank"; about:blank loaded by chrome in these ways has chrome privileges. This behavior could cause security issues in certain extensions that are thinking that about:blank does not have chrome privileges. Imagine an extension that does: 1. Collect urls from content. 2. Load about:blank. (window.open("about:blank") or content.location = "about:blank") 3. Generate links with the urls and insert those into the about:blank document. When an user clicks a javascript: link in the generated page, the script run with chrome privileges. I'm not sure whether this should be fixed or not. If not, we need to advertise the potential problem. (There is an affected extension on AMO.) Test: Evaluate the following code in the error console. w=open("about:blank");alert(1);u="javascript:alert(Components.stack);";w.document.body.innerHTML=u.link(u);w.focus();1 or top.opener.content.location="about:blank";alert(1);u="javascript:alert(Components.stack);";(w=top.opener.content).document.body.innerHTML=u.link(u);w.focus();1

Web Developer is affected when "Open generated pages in tabs instead of windows" option is not checked. (it's checked by default.) Web Developer https://addons.mozilla.org/en-US/firefox/addon/60

This works with Web Developer 1.1.4 on fx2.0.0.5-rc1 and fx2.0.0.5-rc2.

We have the same behavior on trunk, right?

Yes.

Note that subframes of chrome only use the parent principal if they're also chrome docshells. I suppose we could put equivalent checks in the two codepaths in question; it's pretty straightforward to do, I think. The question is what behavior we want. On a side note... could we just ignore an "about:blank" argument passed to window.open? That would also fix this, right? Or would that change the way load events are fired too much for us to be happy?

What do you mean by "if they're also chrome docshells"? Is that anytime you're not explicitly does <browser type="content-primary">? Why would ignoring "about:blank" help? Does the implicit about:blank document opened for new windows not inherit the parent principal? Ideally <browser> should default to low-privilege, and only get chrome-level access when explicitly requested, but that might be a hard change to do :(

> Is that anytime you're not explicitly does <browser type="content-primary">? Basically. Any time type is not "content" and doesn't start with "content-". > Why would ignoring "about:blank" help? You're right. It wouldn't.... The implic doc of course inherits the opener principal. Perhaps we should explicitly block system principals for about:blank in content-type docshells, That would make a lot of sense to me.

> Perhaps we should explicitly block system principals for about:blank in > content-type docshells, That would make a lot of sense to me. You mean in chrome docshells?

No, content docshells. The ones you'd get from window.open(""). Not sure what to do about content.location; in general we should be discouraging that, because if you do that with a javascript: URI you're in deep trouble. Plus we have tabbrowser APIs for it that do the right thing.

This _is_ a regression, just to be clear; from bug 381300 maybe? We need to figure out which so we can make an informed backout vs. fix decision (in the short-run). Can we live with the bug between 2.0.0.5 and 2.0.0.6? at the moment I can't think of anywhere default Firefox is affected. I don't understand why a new window behaves differently from a new tab (but it does, as easily confirmed with Web Developer toolbar).

This is indeed a regression from bug 381300 (aka bug 332182). Hence the dep. Not having read the source of Web Developer, I would assume they open a new tab via our tabbrowser APIs, which means that about:blank load comes via the nsFrameLoader and gets the check mentioned in comment 5. I should be able to come up with a fix in the next 10 hours or so that doesn't affect anything but about:blank loaded from chrome, I think.

This is the code of Web Developer that makes the difference between new window and new tab. In images.js: function webdeveloper_findBrokenImages() { ... var generatedDocument = webdeveloper_generateDocument(""); ... In webdeveloper.js: // Generates a document in a new tab or window function webdeveloper_generateDocument(url) { var generatedPage = null; var request = new XMLHttpRequest(); // If the open tabs preference is set to true if(webdeveloper_getBooleanPreference("webdeveloper.open.tabs", true)) { getBrowser().selectedTab = getBrowser().addTab(url); generatedPage = window; } else { generatedPage = window.open(url); } // This must be done to make generated content render request.open("get", "about:blank", false); request.send(null); return generatedPage.content.document; }

The three hunks of this patch do the following: 1) Never allow chrome-privileged data:, javascript:, or about:blank loads in content docshells. Switch them to inheriting principals instead (which is a no-op for about:blank). This behavior is now consistent across all "normal" ways of loading, whereas before window.location allowed chrome javascript: while the nsIWebNavigation APIs, tabbrowser, and setting "src" on <browser>s did not. It's still possible to do such loads via manual invocation of nsILinkHandler, but that's not a scriptable API, and doesn't take a principal pointer anyway (it takes a node). This fixes the window.location aspect of this bug. 2) Don't propagate a system principal as the opener principal to new content windows. This fixes the window.open() aspect of this bug. Note that we need both, because CreateAboutBlankContentViewer doesn't actually do a load. 3) Remove now-redundant code in nsFrameLoader. The only risk here, imo is that this does change the behavior of data: and javascript: URIs loaded from chrome in content windows via window.location. If we want I can try to avoid changing that, but the code would be more complex, and I don't think we want to allow it anyway. The former is certainly not safe.

Comment on attachment 272411 [details] [diff] [review] Branch fix This looks right to me, and I agree that it doesn't make sense to allow loading javascript: or data: URIs with chrome privs in content windows. r+sr=jst

Comment on attachment 272411 [details] [diff] [review] Branch fix Requesting approval. Risk analysis: The main behavior change from 1.8.1.4 with this patch is that setting window.location to a javascript: or data: URI on a content window no longer uses the system principal for that load. I would say that doing that in unsafe to start with (certainly so for javascript: if there is something loaded in that window). If we feel that we really don't want to make this behavior change, I would be happy to modify this patch to only change behavior for about:blank. The change would be to check for the about:blank URI in the docshell and windowwatcher hunks, and not checking in the frameloader hunk. I don't really have a good feel for how this would affect extension compat... can we grep extensions on AMO for javascript: and data: or something to see what things look like?

Checked in trunk fix, except the tests. I'll wait till after we ship 1.8.1.6 to do that, I guess.

Comment on attachment 272411 [details] [diff] [review] Branch fix a=mconnor on behalf of drivers, please land on GECKO181_20070712_RELBRANCH for 2.0.0.6 and MOZILLA_1_8_BRANCH for 2.0.0.7

verified fixed for trunk tested with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a7pre) Gecko/2007072204 Minefield/3.0a7pre ID:2007072204 i get on the testcase with the web developer addon a Error: uncaught exception: Permission denied to get property XPCComponents.stack error in the Error Console

Mike, is there a reason you didn't also approve this for 1.8.0.13? dveditz has been doing both for patches that apply to both, in case someone (Thunderbird comes to mind) wants to do a release off that branch...

Waiting on an answer on comment 20 before checking in. Also, I finally have data on extensions doing this stuff (only took a few days), and I'm looking at it. At first blush, it doesn't look great no matter how you slice it. :(

Comment on attachment 272411 [details] [diff] [review] Branch fix mconnor says to land this on 1.8.0 as well (irc).

Fixed for the 1.8.1 branches. Looks like 1.8.0 needs to wait on bug 381300. :(

Add fligtar for insight into Facebook toolbar (comment 24).

verified fixed 1.8.1.6 using Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.6) Gecko/20070723 Firefox/2.0.0.6 ID:2007072301 and the testcase from this bug with Web Developer. No about:blank is loaded.

(In reply to comment #26) > verified fixed 1.8.1.6 using Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; > rv:1.8.1.6) Gecko/20070723 Firefox/2.0.0.6 ID:2007072301 and the testcase from > this bug with Web Developer. No about:blank is loaded. > and the Error Message in the console was Error: uncaught exception: Permission denied to get property UnnamedClass.stack

(In reply to comment #25) > Add fligtar for insight into Facebook toolbar (comment 24). > I emailed the author of the Facebook Toolbar early this morning; no response as of yet.

(In reply to comment #24) > sdwilsh, is that happening on a content window? If so, will running with > non-chrome privileges break that code? If yes, you guys might want to fix > it... Yes. Don't know (hope not). The first file I have a fix for, the second file I haven't looked at yet.

Ok, do issue 2 doesn't seem to need chrome privs, but it doesn't work on Firefox 3 anyway, so I'll be patching that code soon enough.

Comment on attachment 272411 [details] [diff] [review] Branch fix moving approval to 1.8.0.14 to match regressing bug 381300

verified still fixed in 1.8.1.8

Fix not needed for Thunderbird, but wanted in any future Firefox 1.5.0.x release

Comment on attachment 272411 [details] [diff] [review] Branch fix 1.8.0.14 is focused on Thunderbird's 1.5-EOL release, don't need this patch.

Comment on attachment 272411 [details] [diff] [review] Branch fix a=caillon for 1.8.0.15 branch

This needs a new 1.8.0 branch patch, as the current one doesn't even come close to applying.

Oh. Well, that's because this needs to be applied on top of bug 381300 (without which this bug is not even present). Of course that bug still has its "approval1.8.0.14?" set....

MOZILLA_1_8_0_BRANCH: Checking in docshell/base/nsDocShell.cpp; /cvsroot/mozilla/docshell/base/nsDocShell.cpp,v <-- nsDocShell.cpp new revision: 1.719.2.21.2.15; previous revision: 1.719.2.21.2.14 done Checking in embedding/components/windowwatcher/src/nsWindowWatcher.cpp; /cvsroot/mozilla/embedding/components/windowwatcher/src/nsWindowWatcher.cpp,v <-- nsWindowWatcher.cpp new revision: 1.100.2.5.2.5; previous revision: 1.100.2.5.2.4 done Checking in content/base/src/nsFrameLoader.cpp; /cvsroot/mozilla/content/base/src/nsFrameLoader.cpp,v <-- nsFrameLoader.cpp new revision: 1.53.6.1.4.3; previous revision: 1.53.6.1.4.2 done