223159 - Script with UniversalBrowserWrite privilige still unable to window.close() a non-JS-created window

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Charles Duffy]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5a) Gecko/20031010 Mozilla Firebird/0.6.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5) Gecko/20031007

The following script gives the error: "Scripts may not close windows that were
not opened by script", if embedded in a page which was not opened via script.
This behaviour is incorrect because UniversalBrowserWrite, according to
http://www.mozilla.org/projects/security/components/jssec.html#privs-list,
provides the privilege "To unconditionally close a browser window", and the
below script fails even when the user grants this privilege as the script requests.

Reproducible: Always

Steps to Reproduce:
Start the JavaScript console. Then, open a the browser window (not via script)
to a page with JavaScript performing the following steps:

1. netscape.security.PrivilegeManager.enablePrivilege("UniversalBrowserWrite");
2. Optionally, validate that other UniversalBrowserWrite privileges are active
(ie. opening a window with its title bar disabled):
  testwindow = window.open("test-that-we're-privileged.htm", "popup", "close=no,
titlebar=no";
3. self.close()
Actual Results:  
The JavaScript console now has an informational note, "Scripts may not close
windows that were not opened by script", and that the parent window (which in
turn opened the titlebar-less child) is still open.

Expected Results:  
Because of the script having the UniversalBrowserWrite privilege, the parent
window should have closed, and no message should have been written to the
JavaScript console.

Comment 1

[timeless]

Attachment: honor UBW

Comment 2

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 148645 [details] [diff] [review]
honor UBW

>Index: mozilla/dom/src/base/nsGlobalWindow.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/dom/src/base/nsGlobalWindow.cpp,v
>retrieving revision 1.662
>diff -up5 -r1.662 mozilla/dom/src/base/nsGlobalWindow.cpp
>--- mozilla/dom/src/base/nsGlobalWindow.cpp
>+++ mozilla/dom/src/base/nsGlobalWindow.cpp
>@@ -3437,10 +3437,15 @@ GlobalWindowImpl::Close()
>     nsCOMPtr<nsIScriptSecurityManager> secMan(
>       do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv));
>     if (NS_SUCCEEDED(rv)) {
>       PRBool inChrome = PR_TRUE;
>       rv = secMan->SubjectPrincipalIsSystem(&inChrome);
>+
>+      if (!inChrome)
>+        rv = sSecMan->IsCapabilityEnabled("UniversalBrowserWrite",
>+                                          &inChrome);

Use braces here.

I don't like using the "inChrome" identifier for this, since we are not
necessarily in chrome here.


>+
>       if (NS_SUCCEEDED(rv) && !inChrome) {
>         PRBool allowClose =

Instead, I would use "allowClose" (move it out, and kill the other variable)

>           nsContentUtils::GetBoolPref("dom.allow_scripts_to_close_windows",
>                                       PR_TRUE);
>         if (!allowClose) {


Do that and r=caillon

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 148645 [details] [diff] [review]
honor UBW

Yeah, what caillon said (maybe just move out the allowClose boolean and use
that in stead of inChrome?). I'll sr a patch with that changed.

Comment 4

[timeless]

Attachment: per caillon+jst

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 148893 [details] [diff] [review]
per caillon+jst

@@ -1400,9 +1400,9 @@
   // First, check if we were called from a privileged chrome script

   NS_ENSURE_TRUE(sSecMan, NS_ERROR_FAILURE);
-  PRBool inChrome;
-  nsresult rv = sSecMan->SubjectPrincipalIsSystem(&inChrome);
-  if (NS_SUCCEEDED(rv) && inChrome) {
+  PRBool allowClose;
+  nsresult rv = sSecMan->SubjectPrincipalIsSystem(&allowClose);
+  if (NS_SUCCEEDED(rv) && allowClose) {

Since you're changing this too, you might as well just make this use
IsCallerChrome() and eliminate rv and allowClose alltogether.

+      PRBool allowClose = PR_TRUE;
+      rv = secMan->SubjectPrincipalIsSystem(&allowClose);

Initialize allowClose to PR_FALSE, just in case. Or better yet, use
IsCallerChrome() here too and initialize allowClose to what IsCallerChrome()
returns.

sr=jst with those changes.

Comment 6

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 148893 [details] [diff] [review]
per caillon+jst

>@@ -3437,10 +3437,15 @@
>     nsCOMPtr<nsIScriptSecurityManager> secMan(
>       do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv));
>     if (NS_SUCCEEDED(rv)) {
>-      PRBool inChrome = PR_TRUE;
>-      rv = secMan->SubjectPrincipalIsSystem(&inChrome);
>-      if (NS_SUCCEEDED(rv) && !inChrome) {
>-        PRBool allowClose =
>+      PRBool allowClose = PR_TRUE;
>+      rv = secMan->SubjectPrincipalIsSystem(&allowClose);
>+
>+      if (!allowClose)
>+        rv = sSecMan->IsCapabilityEnabled("UniversalBrowserWrite",
>+                                          &allowClose);

So before, were SubjectPrincipalIsSystem() to fail, we would not allow closing
the window.  Now, if it fails (and an evil caller touches the out parameter)
you could potentially still allow that by clobbering the rv.  How about also
checking rv above for posterity?  Or or (as in operator|=()) the capability
call's rv.

>+
>+      if (NS_SUCCEEDED(rv) && !allowClose) {
>+        allowClose =
>           nsContentUtils::GetBoolPref("dom.allow_scripts_to_close_windows",
>                                       PR_TRUE);
>         if (!allowClose) {

Comment 7

[Asa Dotzler [:asa]]

Comment on attachment 148893 [details] [diff] [review]
per caillon+jst

a=asa for checkin to 1.8a3

Comment 8

[timeless]

Comment on attachment 148893 [details] [diff] [review]
per caillon+jst

mozilla/dom/src/base/nsGlobalWindow.cpp 	1.688

Comment 9

[timeless]

reporter: please verify that this is fixed.

Comment 10

[Mike Kaply [:mkaply]]

Comment on attachment 148893 [details] [diff] [review]
per caillon+jst

a=mkaply assuming you addressed caillon's last comment.

Comment 11

[Mike Kaply [:mkaply]]

Comment on attachment 148893 [details] [diff] [review]
per caillon+jst

this was not in firefox so it shouldn't go in 1.7.5. Wait for 1.7.5 to ship,
then check it in.