Bug 388121 (CVE-2007-3844)

[FIX]about:blank loaded by chrome in particular ways has chrome privileges

VERIFIED FIXED

Status

()

Core
DOM
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: moz_bug_r_a4, Assigned: bz)

Tracking

(4 keywords)

unspecified
fixed1.8.0.15, regression, verified1.8.1.6, verified1.8.1.8
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
blocking1.8.1.6 +
blocking1.8.1.8 +
blocking1.8.0.14 -
blocking1.8.0.next +
wanted1.8.0.x +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:moderate] potentially critical for extensions that use this.)

Attachments

(3 attachments)

(Reporter)

Description

10 years ago
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
(Reporter)

Comment 1

10 years ago
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
(Reporter)

Comment 2

10 years ago
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.
We have the same behavior on trunk, right?
(Reporter)

Comment 4

10 years ago
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?
(Assignee)

Updated

10 years ago
Blocks: 332182
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).
Group: mozillaorgconfidential
Flags: blocking1.8.1.5?
Keywords: regression
Group: mozillaorgconfidential
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.
(Reporter)

Comment 12

10 years ago
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;
}
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.
Attachment #272411 - Flags: superreview?(jst)
Attachment #272411 - Flags: review?(jst)
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
Attachment #272411 - Flags: superreview?(jst)
Attachment #272411 - Flags: superreview+
Attachment #272411 - Flags: review?(jst)
Attachment #272411 - Flags: review+
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?
Attachment #272411 - Flags: approval1.8.1.5?
Attachment #272411 - Flags: approval1.8.0.13?
Created attachment 272432 [details] [diff] [review]
Trunk patch, plus regression tests
Attachment #272432 - Flags: superreview?(jst)
Attachment #272432 - Flags: review?(jst)
(Assignee)

Updated

10 years ago
Assignee: nobody → bzbarsky
Flags: blocking1.9+
OS: Windows XP → All
Hardware: PC → All
Summary: about:blank loaded by chrome in particular ways has chrome privileges → [FIX]about:blank loaded by chrome in particular ways has chrome privileges

Updated

10 years ago
Attachment #272432 - Flags: superreview?(jst)
Attachment #272432 - Flags: superreview+
Attachment #272432 - Flags: review?(jst)
Attachment #272432 - Flags: review+
Checked in trunk fix, except the tests.  I'll wait till after we ship 1.8.1.6 to do that, I guess.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED

Updated

10 years ago
Flags: blocking1.8.1.7+
Flags: blocking1.8.1.6+
Flags: blocking1.8.1.5?
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
Attachment #272411 - Flags: approval1.8.1.7+
Attachment #272411 - Flags: approval1.8.1.6+
Attachment #272411 - Flags: approval1.8.1.5?
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
Status: RESOLVED → VERIFIED
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).
Attachment #272411 - Flags: approval1.8.0.13? → approval1.8.0.13+
Fixed for the 1.8.1 branches.  Looks like 1.8.0 needs to wait on bug 381300.  :(
Keywords: fixed1.8.1.5, fixed1.8.1.6
(Assignee)

Updated

10 years ago
Keywords: fixed1.8.1.5 → fixed1.8.1.7
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.
Keywords: fixed1.8.1.6 → verified1.8.1.6
(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.
Blocks: 381300
Group: security
Alias: CVE-2007-3844
Whiteboard: [sg:moderate] potentially critical for extensions that use this.
Flags: blocking1.8.0.14+
Comment on attachment 272411 [details] [diff] [review]
Branch fix

moving approval to 1.8.0.14 to match regressing bug 381300
Attachment #272411 - Flags: approval1.8.0.13+ → approval1.8.0.14+
verified still fixed in 1.8.1.8
Keywords: fixed1.8.1.8 → verified1.8.1.8
Fix not needed for Thunderbird, but wanted in any future Firefox 1.5.0.x release
Flags: wanted1.8.0.x+
Flags: blocking1.8.0.15+
Flags: blocking1.8.0.14-
Flags: blocking1.8.0.14+
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.
Attachment #272411 - Flags: approval1.8.0.15?
Attachment #272411 - Flags: approval1.8.0.14-
Attachment #272411 - Flags: approval1.8.0.14+
Comment on attachment 272411 [details] [diff] [review]
Branch fix

a=caillon for 1.8.0.15 branch
Attachment #272411 - Flags: approval1.8.0.15? → approval1.8.0.15+
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
Whiteboard: [sg:moderate] potentially critical for extensions that use this. → [needs checkin on 1.8.0 branch][sg:moderate] potentially critical for extensions that use this.
This needs a new 1.8.0 branch patch, as the current one doesn't even come close to applying.
Keywords: checkin-needed
Whiteboard: [needs checkin on 1.8.0 branch][sg:moderate] potentially critical for extensions that use this. → [needs new patch for 1.8.0 branch][sg:moderate] potentially critical for extensions that use this.
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
Keywords: fixed1.8.0.15
Whiteboard: [needs new patch for 1.8.0 branch][sg:moderate] potentially critical for extensions that use this. → [sg:moderate] potentially critical for extensions that use this.
Depends on: 470709
You need to log in before you can comment on or make changes to this bug.