Last Comment Bug 388121 - (CVE-2007-3844) [FIX]about:blank loaded by chrome in particular ways has chrome privileges
(CVE-2007-3844)
: [FIX]about:blank loaded by chrome in particular ways has chrome privileges
Status: VERIFIED FIXED
[sg:moderate] potentially critical fo...
: fixed1.8.0.15, regression, verified1.8.1.6, verified1.8.1.8
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on: 470709
Blocks: 332182 CVE-2007-3089
  Show dependency treegraph
 
Reported: 2007-07-13 21:45 PDT by moz_bug_r_a4
Modified: 2009-04-06 00:02 PDT (History)
23 users (show)
bzbarsky: blocking1.9+
mconnor: blocking1.8.1.6+
mconnor: blocking1.8.1.8+
dveditz: blocking1.8.0.14-
dveditz: blocking1.8.0.next+
dveditz: wanted1.8.0.x+
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (450 bytes, text/html)
2007-07-13 21:48 PDT, moz_bug_r_a4
no flags Details
Branch fix (7.79 KB, patch)
2007-07-15 12:38 PDT, Boris Zbarsky [:bz]
jst: review+
jst: superreview+
mconnor: approval1.8.1.6+
mconnor: approval1.8.1.8+
dveditz: approval1.8.0.14-
caillon: approval1.8.0.next+
Details | Diff | Review
Trunk patch, plus regression tests (13.07 KB, patch)
2007-07-15 17:13 PDT, Boris Zbarsky [:bz]
jst: review+
jst: superreview+
Details | Diff | Review

Description moz_bug_r_a4 2007-07-13 21:45:46 PDT
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
Comment 1 moz_bug_r_a4 2007-07-13 21:46:56 PDT
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
Comment 2 moz_bug_r_a4 2007-07-13 21:48:38 PDT
Created attachment 272283 [details]
testcase

This works with Web Developer 1.1.4 on fx2.0.0.5-rc1 and fx2.0.0.5-rc2.
Comment 3 Boris Zbarsky [:bz] 2007-07-13 22:01:41 PDT
We have the same behavior on trunk, right?
Comment 4 moz_bug_r_a4 2007-07-13 22:06:24 PDT
Yes.
Comment 5 Boris Zbarsky [:bz] 2007-07-13 22:09:39 PDT
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?
Comment 6 Jonas Sicking (:sicking) 2007-07-13 22:30:46 PDT
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 :(
Comment 7 Boris Zbarsky [:bz] 2007-07-13 22:53:02 PDT
> 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.
Comment 8 Jonas Sicking (:sicking) 2007-07-13 23:27:38 PDT
> 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?
Comment 9 Boris Zbarsky [:bz] 2007-07-13 23:52:25 PDT
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.
Comment 10 Daniel Veditz [:dveditz] 2007-07-14 22:28:52 PDT
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).
Comment 11 Boris Zbarsky [:bz] 2007-07-15 02:26:46 PDT
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.
Comment 12 moz_bug_r_a4 2007-07-15 06:19:52 PDT
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;
}
Comment 13 Boris Zbarsky [:bz] 2007-07-15 12:38:32 PDT
Created attachment 272411 [details] [diff] [review]
Branch fix

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 14 Johnny Stenback (:jst, jst@mozilla.com) 2007-07-15 16:44:48 PDT
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 15 Boris Zbarsky [:bz] 2007-07-15 16:56:08 PDT
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?
Comment 16 Boris Zbarsky [:bz] 2007-07-15 17:13:31 PDT
Created attachment 272432 [details] [diff] [review]
Trunk patch, plus regression tests
Comment 17 Boris Zbarsky [:bz] 2007-07-17 18:48:53 PDT
Checked in trunk fix, except the tests.  I'll wait till after we ship 1.8.1.6 to do that, I guess.
Comment 18 Mike Connor [:mconnor] 2007-07-22 13:55:14 PDT
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
Comment 19 Carsten Book [:Tomcat] 2007-07-22 14:42:17 PDT
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
Comment 20 Boris Zbarsky [:bz] 2007-07-22 17:38:50 PDT
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...
Comment 21 Boris Zbarsky [:bz] 2007-07-22 17:39:47 PDT
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 22 Boris Zbarsky [:bz] 2007-07-22 18:03:00 PDT
Comment on attachment 272411 [details] [diff] [review]
Branch fix

mconnor says to land this on 1.8.0 as well (irc).
Comment 23 Boris Zbarsky [:bz] 2007-07-22 19:09:47 PDT
Fixed for the 1.8.1 branches.  Looks like 1.8.0 needs to wait on bug 381300.  :(
Comment 25 Mike Shaver (:shaver -- probably not reading bugmail closely) 2007-07-23 00:42:35 PDT
Add fligtar for insight into Facebook toolbar (comment 24).
Comment 26 Carsten Book [:Tomcat] 2007-07-23 05:01:27 PDT
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.
Comment 27 Carsten Book [:Tomcat] 2007-07-23 05:24:48 PDT
(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
Comment 28 Justin Scott [:fligtar] 2007-07-23 23:46:21 PDT
(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.
Comment 29 Shawn Wilsher :sdwilsh 2007-07-24 16:45:09 PDT
(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.
Comment 30 Shawn Wilsher :sdwilsh 2007-07-24 17:08:39 PDT
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 31 Daniel Veditz [:dveditz] 2007-08-07 11:31:39 PDT
Comment on attachment 272411 [details] [diff] [review]
Branch fix

moving approval to 1.8.0.14 to match regressing bug 381300
Comment 32 Daniel Veditz [:dveditz] 2007-10-12 15:39:18 PDT
verified still fixed in 1.8.1.8
Comment 33 Daniel Veditz [:dveditz] 2007-12-03 14:27:04 PST
Fix not needed for Thunderbird, but wanted in any future Firefox 1.5.0.x release
Comment 34 Daniel Veditz [:dveditz] 2007-12-03 15:44:07 PST
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 35 Christopher Aillon (sabbatical, not receiving bugmail) 2008-02-19 08:22:29 PST
Comment on attachment 272411 [details] [diff] [review]
Branch fix

a=caillon for 1.8.0.15 branch
Comment 36 Reed Loden [:reed] (use needinfo?) 2008-02-20 02:26:03 PST
This needs a new 1.8.0 branch patch, as the current one doesn't even come close to applying.
Comment 37 Boris Zbarsky [:bz] 2008-02-20 08:03:39 PST
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....
Comment 38 Reed Loden [:reed] (use needinfo?) 2008-02-21 02:43:25 PST
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

Note You need to log in before you can comment on or make changes to this bug.