235457 - blocked popup window opened from menu circumvents content restrictions

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Dan M]

A blocked popup window opened from the list of blocked popups (see bug 198846)
is opened with chrome privileges. This means for example that while an attempt
from web content to open a window like so

open("sneaky.html", "_blank", "top=3000,left=3000,height=1,width=1")

will be caught and turned into a visible window, it will however be opened
invisible, just as requested, from the list of blocked popup windows.

Comment 1

[Johnny Stenback (:jst)]

The patch in bug 198846 made Mozilla open the popup from chrome, passing in the
features from the webpage. Not cool. Any thoughs on a fix? I guess what we need
is for the code at the end of nsWindowWatcher::SizeOpenedDocShellItem() do the
security checks even in this case. Would it be as easy as calling open on the
content window in stead of the chrome window? I'd think that would do it...

Comment 2

[Dan M]

I'm a little stymied on this one.

>calling open on the content window
not unless I'm missing something.
  _content.open(uri, "", features);
doesn't work because the JS context used for security comparison still comes
from the chrome window.
  _content.setTimeout(_content.open, 0, uri, "", features);
does indeed find the correct security principal but, heh, that's a timer.

I don't think I can explicitly push an unprivileged context onto the execution
stack from script :) so that leaves sending an undocumented code through the
open feature parameter telling it to be more restrictive. That's kinda icky and
still not quite right (what if the appropriate capabilities *are* enabled in the
context that I really should be using?) And did I mention, icky?

Comment 3

[Johnny Stenback (:jst)]

Hmm. Yeah, I guess the JS stack frame we get from window.open() is native, so we
won't find principals in it, and we'll use the callers.

Maybe we need to make window.open() push its own context onto the stack if
called from chrome? That way we'd never give away chrome priveleges by calling
open() on any window.

Comment 4

[Michiel van Leeuwen (email: mvl+moz@)]

>   _content.setTimeout(_content.open, 0, uri, "", features);
> does indeed find the correct security principal but, heh, that's a timer.

I'm wondering: does the setTimeout make the function being called from _content,
and getting the security stuff from there, or from 'nothing', and so it gets the
most restrictive settings?
If the first, isn't that what we want? if the second, isn't this a workaround?
(yeah i know, workarounds stink, they tend to stay way too long)

Comment 5

[Dan M]

Johnny: For this bug, we need chrome window.open to in fact give away its chrome
privileges. But only under certain circumstances, of course. So it seems that no
kind of general solution would work.

Michiel: _content.setTimeout uses _content's security settings. Which is
beautiful, except that, being called from a timeout, from content, window.open
is no more likely to get past the popup blocker than the original popup window.

Comment 6

[Dan M]

Attachment: implement feature flag to cast away chrome priviliges

I'm embarrassed to suggest this patch. But given all the foregoing discussion
about what won't work, I'm not shamed by it. I believe it solves the bug
without creating any new problems.

Notes: 1) Icky. 2) All windows opened from the list of blocked popups will be
treated as if they're from content without UniversalBrowserWrite privileges,
whether or not that's true. 3) Since cooperation is required from the calling
script, it's possible for someone to write an extension that doesn't restrict
popup windows. (This is a problem only for window.open features such as
position and chrome, not for script running in the window. Script has never
been a problem.)

Comment 7

[Dan M]

That thing I said above, "Script has never been a problem," that's not quite
true. Since the popup is opened from a chrome docshell, restrictions besides
those from window.open itself are also lifted. It's possible for content to use
this loophole to link to chrome:// files, if you can just convince the user to
open the blocked popup from the list.

Seems like we need to open the popup from a genuine content docshell. I think we
should disable bug 198846 if we're unable to solve this problem before 1.7 ships.

Oh and besides not addressing this issue, my first patch in this bug had other
flaws, too.

Comment 8

[Johnny Stenback (:jst)]

Attachment: How about this?

This does 3 things.

1. Makes openinng a window from chrome using a non-chrome window act as if the
new window was opened from the window on which open was called.

2. Makes caps' IsCapabilityEnabled() return true if there's no JS on the stack
only if the subject principal is the system principal (needed to prevent lieing
about what's enabled when called with a non-chrome cx on the stack w/o any JS
running on that stack, as is the case here now).

3. Makes the popup code call open on the content window.

Scary? Yeah, a bit, but it seems to work. Dan, wanna apply and help me test
here?

Comment 9

[Brendan Eich [:brendan]]

Let's get this reviewed and tested tonight.

/be

Comment 10

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 144002 [details] [diff] [review]
How about this?

Johnny asked me to look at the caps changes:

>Index: caps/src/nsScriptSecurityManager.cpp
>-    fp = cx ? JS_FrameIterator(cx, &fp) : nsnull;

Hm, so, if cx is null here, we assign |nsnull| into fp.  So I'm not sure how
your change here actually does anything....

>-    if (!fp)
>+    if (!cx)
>     {
>-        // No script code on stack. Allow execution.
>+        // No context reachable. Allow execution.
>         *result = PR_TRUE;
>         return NS_OK;
>     }
>     *result = PR_FALSE;
>     nsCOMPtr<nsIPrincipal> previousPrincipal;
>-    do
>+    while ((fp = JS_FrameIterator(cx, &fp)) != nsnull)
>     {
>         nsCOMPtr<nsIPrincipal> principal;
>         if (NS_FAILED(GetFramePrincipal(cx, fp, getter_AddRefs(principal))))
>             return NS_ERROR_FAILURE;
>         if (!principal)
>             continue;
>         // If caller has a different principal, stop looking up the stack.
>         if(previousPrincipal)
>         {
>@@ -2025,19 +2024,19 @@ nsScriptSecurityManager::IsCapabilityEna
>             canEnable != nsIPrincipal::ENABLE_WITH_USER_PERMISSION)
>             return NS_OK;
> 
>         // Now see if the capability is enabled.
>         void *annotation = JS_GetFrameAnnotation(cx, fp);
>         rv = principal->IsCapabilityEnabled(capability, annotation, result);
>         if (NS_FAILED(rv)) return rv;
>         if (*result)
>             return NS_OK;
>-    } while ((fp = JS_FrameIterator(cx, &fp)) != nsnull);
>+    }
> 
>     if (!previousPrincipal)
>     {
>         // No principals on the stack, all native code.  Allow
>         // execution if the subject principal is the system principal.
> 
>         return SubjectPrincipalIsSystem(result);
>     }
>

Comment 11

[Christopher Aillon (sabbatical, not receiving bugmail)]

Never mind.  I see what's going on now.  r=me for the caps change.

Comment 12

[Dan M]

Comment on attachment 144002 [details] [diff] [review]
How about this?

This is indeed a little scary. I can't picture what could go wrong, and I've
played with it for half an hour and it seems to work well. I say let's get it
into the beta.

One thing though. The nsGlobalWindow part should read

>+  nsCOMPtr<nsIJSContextStack> stack;
>+
>+  if (IsCallerChrome() && mContext) {
>+    stack = do_GetService(sJSStackContractID);
>+
>+    JSContext *my_cx = NS_REINTERPRET_CAST(JSContext *,
>+                                           mContext->GetNativeContext());
>+
>+    if (stack && my_cx)
>+      stack->Push(my_cx);
>+    else
>+      stack = nsnull;
>+  }
>+
>   rv = OpenInternal(url, name, options, PR_FALSE, nsnull, 0, nsnull, _retval);
> 
>+  if (stack)
>+    stack->Pop(nsnull);

or it'll crash on exit under the right circumstances. Seriously, it's the
braces! Or maybe it's that fourth line. I didn't try both one at a time.

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 144002 [details] [diff] [review]
How about this?

Good catch, danm.  sr=[e-mail].  I say we want this for 1.7 (fixes a
regression, fixes a security bug), so we should get it in tonight for 1.7b.  If
you get no other driver to approve, consider my words here as emergency a=.

/be

Comment 14

[Johnny Stenback (:jst)]

Duh, yeah. That was fixed locally, but apparently after I diffed :-(

Comment 15

[Johnny Stenback (:jst)]

Fix checked in.

Comment 16

[Johnny Stenback (:jst)]

Ugh, this regressed autocomplete, and typing URLs, in the URL bar :-( Backed out.

Comment 17

[Johnny Stenback (:jst)]

So what broke here was calling from C++ into a JS XPCOM component, in that case,
XPConnect finds the safe context, and makes the call there (i.e. not on a chrome
context), and that breaks the IsCapabilityEnabled() method after my change.

This whole mess is rather lame. IMO we should re-land my change, but not before
fixing XPConnect. My thoughts at the moment would be to make XPConnect push a
dummy JS stack frame with system principals onto the cx's stack frame, or
somesuch. This'll require some serious thinking and testing, so no way to do
that for 1.7 any more.

Comment 18

[Dan M]

d'oh. Note to self: test autocomplete every now and then. I did have that
nagging feeling that a general solution would be a problem.

Is the Safe Context used for anything else? Should we find a way to obviate the
Safe Context, it may be possible to finally dump Hidden Window on some platforms
(bug 71895 comment 4). Double scary goodness.

Comment 19

[Jesse Ruderman]

I don't think this bug needs to be security-sensitive, since bug 198846 was
backed  out and now depends on this bug.

Comment 20

[Johnny Stenback (:jst)]

Opening up this bug.

Comment 21

[eric lindvall]

> >Okay, let me see if I've gotten this right.
> >
> >I think I've gathered that there are actually two different issues that
> >were discovered with the pop-up patch:
> >
> >1. There is a potential problem with using both window.open() and
> >   WindowWatcher.openWindow():
> >
> >   The "features" that are passed to openWindow() could have evil in them,
> >   including "chrome=yes", "dialog=yes", or "width=50000000" (because we
> >   are taking the features that were set to open the initial pop-up, and
> >   using them).
> >
> >
> >2. There is a problem with using window.open() from chrome to create a
> >   browser window:
> >
> >   The window that is created has the capabilities of a chrome window,
> >   allowing the pop-up to do things like point to chrome:// urls and other
> >   nasty evilness.
> >
> >
> >So, even if I am using the nsIWindowWatcher, I still have the first
> >problem, right?
> >
>
> Correct. Neither of those problems are really a problem with window.open
> or any of our window opening code though (even if part of the problem
> could be "fixed" by the changes discussed in bug 235457). The real
> problem is that while in chrome, you must never ever trust data you get
> from a webpage, always check, and err on the side of caution, before
> doing anything with that data.

How much checking should I do before sending the features to
nsIWindowWatcher.openWindow()?

Is making sure I only pass features that are defined here
<http://www.mozilla.org/docs/dom/domref/dom_window_ref76.html> enough?

Do I need to guard against "top=3000"?

Comment 22

[Johnny Stenback (:jst)]

This is where this gets nasty, and I strongly discourage you from even trying to
ensure that the arguments are safe in JS. The code that does that lives in the
window watcher, and it's far from trivial. Trying to duplicate that is asking
for bugs, of not now, later. I might have a lead on how to fix this, more later...

Comment 23

[Johnny Stenback (:jst)]

Attachment: This might be acceptable here.

Comment 24

[Johnny Stenback (:jst)]

Comment on attachment 146888 [details] [diff] [review]
This might be acceptable here.

Um, not quite.

Comment 25

[Johnny Stenback (:jst)]

Attachment: More like this, still untested tho

This should make windows opened from chrome through a non-chrome window be
treated as if it was opened from non-vhrome script.

Comment 26

[eric lindvall]

(In reply to comment #22)
> This is where this gets nasty, and I strongly discourage you from even trying to
> ensure that the arguments are safe in JS. The code that does that lives in the
> window watcher, and it's far from trivial. Trying to duplicate that is asking
> for bugs, of not now, later. I might have a lead on how to fix this, more later...

That was what has confused me about all of this.

While I understand that fundamentally chrome shouldn't trust input it gets -- I
don't see how in this instance (of opening a browser pop-up from chrome) how the
code in the chrome can do anything to sanitize the input in a way that won't
lead to the issues you raised.

Really, it seems there needs to be a way for the JS code running in chrome to
say: "The code I'm running here is untrusted".

Comment 27

[eric lindvall]

(In reply to comment #25)
> Created an attachment (id=146890)
> More like this, still untested tho
> 
> This should make windows opened from chrome through a non-chrome window be
> treated as if it was opened from non-vhrome script.

I think there is a more general problem here.

I created a patch for FireFox that added a button to the "Blocked Popups" window
(pageReport.xul) called "Show Pop-up" that would spawn the pop-up, which would
mean it would be a chrome window (the browser window) would be creating another
chrome window (the "Blocked Popups" window), which would then be trying to
create a non-chrome window.

If this doesn't make enough sense, I can post the patch somewhere (I decided
against posting it on bug #176564 after I saw the Mozilla patch was undone).

Comment 28

[Michiel van Leeuwen (email: mvl+moz@)]

I tested the patch, and sofar it works. Autocomplete also sitll works :)
To compile, i had to add a closing ) in the |if| statement in
NS_CALCULATE_CHROME_FLAG_FOR
I also put back the 'this was backed out' patch from bug 198846, and changed
window.open to content.open. Without that change you could still open a 1x1
window. Now, you cant.

Comment 29

[Johnny Stenback (:jst)]

(In reply to comment #28)
> I tested the patch, and sofar it works. Autocomplete also sitll works :)
> To compile, i had to add a closing ) in the |if| statement in
> NS_CALCULATE_CHROME_FLAG_FOR

Yeah, I noticed that later on too, sorry, I was in a rush to catch a flight when
I attached that patch.

Thanks for testing the patch.

Comment 30

[Johnny Stenback (:jst)]

(In reply to comment #27)
> > This should make windows opened from chrome through a non-chrome window be
> > treated as if it was opened from non-vhrome script.
> 
> I think there is a more general problem here.
> 
> I created a patch for FireFox that added a button to the "Blocked Popups" window
> (pageReport.xul) called "Show Pop-up" that would spawn the pop-up, which would
> mean it would be a chrome window (the browser window) would be creating another
> chrome window (the "Blocked Popups" window), which would then be trying to
> create a non-chrome window.

In such a case you would need to make sure you have a non-chrome window
available when you want to open the non-chrome popup from your chrome code.
That's the whole basis of this approach. If that's not enough, then we need a
different approach here, but I fail to see an easy one anywhere around...

> 
> If this doesn't make enough sense, I can post the patch somewhere (I decided
> against posting it on bug #176564 after I saw the Mozilla patch was undone).

Comment 31

[eric lindvall]

(In reply to comment #30)
> In such a case you would need to make sure you have a non-chrome window
> available when you want to open the non-chrome popup from your chrome code.
> That's the whole basis of this approach. If that's not enough, then we need a
> different approach here, but I fail to see an easy one anywhere around...

I guess I may not understand what the solution is.

What would the JS look like that would be able to open a window from chrome,
where the window that was opened would not have a chrome security context?

Comment 32

[Johnny Stenback (:jst)]

"newwin = somechromewin.content.open(<untrusted arguments>)", the "content" part
is the key.

Comment 33

[eric lindvall]

(In reply to comment #32)
> "newwin = somechromewin.content.open(<untrusted arguments>)", the "content" part
> is the key.

But none of this works until someone commits that patch?

Comment 34

[Johnny Stenback (:jst)]

Correct.

Comment 35

[Johnny Stenback (:jst)]

Comment on attachment 146890 [details] [diff] [review]
More like this, still untested tho

This patch, if you add the missing ')' in the NS_CALCULATE_CHROME_FLAG_FOR
macro, appears to solve this w/o bad side effects. Dan, wanna reiview?

Comment 36

[Dan M]

Comment on attachment 146890 [details] [diff] [review]
More like this, still untested tho

SecurityCheckURL will still incorrectly pass the test: chrome URLs can be
opened from the blocked popup menu. To fix that, I think we have to mess with
contexts rather like in the previous patch. Don't we have to use the caller's
script context instead of the opening window's?

@@ -5469,7 +5469,8 @@ GlobalWindowImpl::SecurityCheckURL(const
   if (NS_FAILED(rv))
     return rv;

-  if (NS_FAILED(sSecMan->CheckLoadURIFromScript(cx, uriToLoad))) {
+  if (NS_FAILED(sSecMan->CheckLoadURIFromScript((JSContext
*)mContext->GetNativeContext(),
+						 uriToLoad))) {
     return NS_ERROR_FAILURE;
   }

The patch might be OK with this addition, but I don't have a clear idea of what
that'll break. The URLbar seems OK :)

Comment 37

[Johnny Stenback (:jst)]

Attachment: Make URI security check in window.open() behave too

Ah, good catch, Dan. How about this, this makes a similar change to
GlobalWindowImpl::SecurityCheckURL() which does what your suggested change
does, but only for the same case that the other changes here affect, when
calling window.open() from chrome on a non-chrome window.

Comment 38

[Dan M]

Comment on attachment 149235 [details] [diff] [review]
Make URI security check in window.open() behave too

That's better. Good catch on the GetBaseURI thing. I can't find any misdeeds
from this patch :)

Comment 39

[Johnny Stenback (:jst)]

Fix landed on the trunk.

Comment 40

[Mike Kaply [:mkaply]]

Was this last patch put in aviary?

If so, I'd also like to put it in 1.7.

Comment 41

[Dan M]

Right, this bug is currently fixed on trunk and Aviary, but not Moz1.7. However
the reason this fix is important, bug 198846, is also missing from Moz1.7.

Comment 42

[Johnny Stenback (:jst)]

Plusing for 1.7.x per above comments.

Comment 43

[Mike Kaply [:mkaply]]

Would this fix provide any benefit to 1.7 without bug 198846?

Comment 44

[Michiel van Leeuwen (email: mvl+moz@)]

Not that I know of.

Comment 45

[Mike Kaply [:mkaply]]

This bug is confusing the heck out of me. The bug was opened on pre 1.7
browsers, but this fix has no effect on 1.7?

Comment 46

[Michiel van Leeuwen (email: mvl+moz@)]

before 1.7 was release, a patch for bug 198846 was checked in to show blocked
popups. (before 1.7b iirc). Then this bug was discovered, and the patch for bug
198846 was backed out. (still before 1.7). After 1.7 branched, this bug was
fixed, and bug 198846 was checked in again. So 1.7 doesn't have a use for the
patch here, since it can't open blocked popups.

Comment 47

[OstGote!]

(In reply to comment #46)
> So 1.7 doesn't have a use for the
> patch here, since it can't open blocked popups.

So the blocking1.7.5+ flag is obsolete?