201132 - (liu1) Same origin violation using events

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Mitchell Stoltz (not reading bugmail)]

from [e-mail] - the following testcase appears to be able to
read properties (including document.cookie) from a third-party webpage - a
same-origin violation.

Comment 1

[Mitchell Stoltz (not reading bugmail)]

Attachment: auxiliary file for testcase

Comment 2

[Mitchell Stoltz (not reading bugmail)]

Attachment: The exploit - reads cookie from www.yahoo.com

Comment 3

[Mitchell Stoltz (not reading bugmail)]

Attachment: The exploit

Whoops, disregard the last one

Comment 4

[Mitchell Stoltz (not reading bugmail)]

OK, I give up. The exploit doesn't work when run directly from the attachment
above, but you can try it at
http://liudieyuinchina.vip.sina.com/NETSCAPECONFIDENTIAL-WS1-030408002400/MOZedgeLink.htm

I have reproduced this - it works.

Heikki, jst, any clue what's going on here? Looks like he's setting a timeout,
changing the page location, and then calling an event handler from the timeout,
which reads the newly loaded page.

Comment 5

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Adding John to CC since this might be events code...

Comment 6

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I have a working copy of the exploit locally at
http://green.nscp.aoltw.net/heikki/201132/MOZedgeLink.htm

Comment 7

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Btw, I was unable to see this originally because my popup controls prevented
window.open, so maybe that can protect some people in some cases...

Comment 8

[georgi - hopefully not receiving bugspam]

For me the problem seems that MyLink is saved from the old window and then used
in the new window to access it.
25	MyLink=w.document.links[0];
then
15				MyLink.onclick();
The event handler is executed in the context of the target window, without a
check that it is called from a page in another domain.

Probably there is additionally a race condition also.

Comment 9

[Johnny Stenback (:jst)]

Attachment: Plug this security hole.

This code makes sure that event handlers are compiled using the right
principal, even when event handlers on elements on documents that are no longer
displayed in a window are compiled.

This also eliminates the per-context strong reference to the security manager
in favor of a global one, that'll cut off 4 bytes off of the size of a context,
and also simplify the code a bit. If needed, we can take that change out for
landing on a branch or something, but this is what I want to have checked in on
the trunk.

Comment 10

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I think you want NS_IF_ADDREF in nsJSContext::GetSecurityManager().

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 120013 [details] [diff] [review]
Plug this security hole.

General remarks:

- This is all because nsEventListenerManager.cpp defers compilation until an
event is actually being handled on the object, saving the string value of the
attribute for deferred compilation, right?

- In that case, using the target's object principal instead of the context's
(subject) principal is clearly the right fix -- pls. comment in
nsJSContext::CompileEventHandler saying so.

- But could it not be the wrong thing if some evil content could set the
onclick property of a victim window's document.links[0], or similar?  Do we
restrict access to all event targets using same-origin checks?	I hope we do.

This patch, while it clearly fixes the bug at hand, adds a degree of
"auto-magic" to nsJSContext::CompileEventHandler that may break the "inverse"
case, if such a case is possible.  Such a case (evil onclick setter on victim
target, not good onclick setter on
dangling-therefore-potentially-evil-in-light-of-this-bug target) should be
ruled out with extreme prejudice! ;-)

>@@ -930,25 +932,37 @@ nsJSContext::CompileEventHandler(void *a
>                                  const nsAString& aBody,
>                                  PRBool aShared, void** aHandler)
> {
>+  JSObject *target = (JSObject*)aTarget;
>   JSPrincipals *jsprin = nsnull;
> 
>-  nsCOMPtr<nsIScriptGlobalObject> global;
>-  GetGlobalObject(getter_AddRefs(global));
>-  if (global) {
>-    // XXXbe why the two-step QI? speed up via a new GetGlobalObjectData func?
>-    nsCOMPtr<nsIScriptObjectPrincipal> globalData = do_QueryInterface(global);
>-    if (globalData) {
>-      nsCOMPtr<nsIPrincipal> prin;
>-      if (NS_FAILED(globalData->GetPrincipal(getter_AddRefs(prin))))
>-        return NS_ERROR_FAILURE;
>-      prin->GetJSPrincipals(&jsprin);
>+  if (target && sSecurityManager) {
>+    nsCOMPtr<nsIPrincipal> prin;
>+
>+    nsresult rv =
>+      sSecurityManager->GetObjectPrincipal(mContext, target,
>+                                           getter_AddRefs(prin));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    prin->GetJSPrincipals(&jsprin);
>+  } else {
>+    nsCOMPtr<nsIScriptGlobalObject> global;
>+    GetGlobalObject(getter_AddRefs(global));
>+    if (global) {
>+      // XXXbe why the two-step QI? speed up via a new
>+      // GetGlobalObjectData func?
>+      nsCOMPtr<nsIScriptObjectPrincipal> globalData = do_QueryInterface(global);
>+      if (globalData) {
>+        nsCOMPtr<nsIPrincipal> prin;
>+        if (NS_FAILED(globalData->GetPrincipal(getter_AddRefs(prin))))
>+          return NS_ERROR_FAILURE;
>+        prin->GetJSPrincipals(&jsprin);
>+      }
>     }
>   }

Why not move down and fuse the prin->GetJSPrincipals(&jsprin) calls (and
therefore move up the nsCOMPtr<nsIPrincipal> prin decl)?  You'd need to
null-test prin in case !globalData, but perhaps we should instead insist that
we always get a non-null prin here -- otherwise we're compiling without
principals.

> NS_IMETHODIMP
> nsJSContext::GetSecurityManager(nsIScriptSecurityManager **aInstancePtr)
> {
>-  if (!mSecurityManager) {
>-    nsresult rv = NS_OK;
>-
>-    mSecurityManager = do_GetService(kScriptSecurityManagerContractID, &rv);
>-    NS_ENSURE_SUCCESS(rv, rv);
>-  }
>-
>-  *aInstancePtr = mSecurityManager;
>+  *aInstancePtr = sSecurityManager;
>   NS_ADDREF(*aInstancePtr);

Heikki's got a point, but you've changed the semantics a bit: before, we would
fail if do_GetService failed -- we would not return a null *aInstancePtr and
NS_OK.	Do you want to fail if !sSecurityManager?

/be

Comment 12

[Mitchell Stoltz (not reading bugmail)]

Yes, I think we do want to fail if !sSecurityManager; that seems safer.

jst, can you help me test Brendan's first point about the reverse case?

Comment 13

[Mitchell Stoltz (not reading bugmail)]

Attachment: Patch v2 - addresses review comments

I can't think of any way to set an event handler on an element in a document
from a different host, so I think the reverse case is OK, but I'd like to hear
from jst.

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 120096 [details] [diff] [review]
Patch v2 - addresses review comments

>@@ -930,25 +932,37 @@

Use cvs diff -p -u -- the -p provides winning function or method name context.

>                                  const nsAString& aBody,
>                                  PRBool aShared, void** aHandler)
> {
>+  JSObject *target = (JSObject*)aTarget;
>   JSPrincipals *jsprin = nsnull;
>+  nsCOMPtr<nsIPrincipal> prin;
> 
>-  nsCOMPtr<nsIScriptGlobalObject> global;
>-  GetGlobalObject(getter_AddRefs(global));
>-  if (global) {
>-    // XXXbe why the two-step QI? speed up via a new GetGlobalObjectData func?
>-    nsCOMPtr<nsIScriptObjectPrincipal> globalData = do_QueryInterface(global);
>-    if (globalData) {
>-      nsCOMPtr<nsIPrincipal> prin;
>-      if (NS_FAILED(globalData->GetPrincipal(getter_AddRefs(prin))))
>-        return NS_ERROR_FAILURE;
>-      prin->GetJSPrincipals(&jsprin);
>+  if (target && sSecurityManager) {

Question: if target && !sSecurityManager, should we really use the subject
prin?  Or should we fail hard inside an if (target) { ... } block here, right
away, if (!sSecurityManager)?

>+    // If we're doing lazy compilation of a handler defined as an attribute,
>+    // we want to use the principal of the target object, *not* the principal
>+    // of the context, which may be different.
>+    nsresult rv =
>+      sSecurityManager->GetObjectPrincipal(mContext, target,
>+                                           getter_AddRefs(prin));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  } else {
>+    nsCOMPtr<nsIScriptGlobalObject> global;
>+    GetGlobalObject(getter_AddRefs(global));
>+    if (global) {
>+      // XXXbe why the two-step QI? speed up via a new
>+      // GetGlobalObjectData func?
>+      nsCOMPtr<nsIScriptObjectPrincipal> globalData = do_QueryInterface(global);
>+      if (globalData) {
>+        globalData->GetPrincipal(getter_AddRefs(prin));
>+      }
>     }
>-  }
>+  }
>+  if (!prin)
>+      return NS_ERROR_FAILURE;

This if is not needed here in the (target && sSecurityManager) case, because of
the NS_ENSURE_SUCCESS(rv, rv) -- I say make the else clause that gets the
subject prin fail hard if (!globalData), so you can assert that prin is
non-null here (don't bother asserting -- just dereference null).

/be

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 120096 [details] [diff] [review]
Patch v2 - addresses review comments

>+    NS_ENSURE_SUCCESS(rv, rv);
>+  } else {
>+    nsCOMPtr<nsIScriptGlobalObject> global;
>+    GetGlobalObject(getter_AddRefs(global));
>+    if (global) {
>+      // XXXbe why the two-step QI? speed up via a new
>+      // GetGlobalObjectData func?
>+      nsCOMPtr<nsIScriptObjectPrincipal> globalData = do_QueryInterface(global);
>+      if (globalData) {
>+        globalData->GetPrincipal(getter_AddRefs(prin));
>+      }
>     }
>-  }

Have to fail if (!global), too, with the change I proposed to ensure a non-nul
prin.

/be

Comment 16

[Mitchell Stoltz (not reading bugmail)]

jst said he's getting rid of the (target && !sSecurityManager) clause that gets
the subject principal.

Comment 17

[Johnny Stenback (:jst)]

Attachment: Same thing, but always get the principal from the target, bail if no target.

This should fix all of the above comments, this makes us never use the
principal from the global object in the context where the event handler is
compiled, we now always use the target. This makes a target and a script
security manager required to compile an event handler, w/ the current code,
we'd crash in the JS engine if we didn't get a target, and we should never end
up in this code in the first place if we can't create a script security
manager.

Comment 18

[Mitchell Stoltz (not reading bugmail)]

Comment on attachment 120152 [details] [diff] [review]
Same thing, but always get the principal from the target, bail if no target.

r=mstoltz.

Comment 19

[Brendan Eich [:brendan]]

Comment on attachment 120152 [details] [diff] [review]
Same thing, but always get the principal from the target, bail if no target.

Not so fast, if you please.  I'll mark sr=[e-mail] on substantially
different, new patches.

/be

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 120152 [details] [diff] [review]
Same thing, but always get the principal from the target, bail if no target.

See bug 195010, look for the diff -p identifier for
nsXULElement::CompileEventHandler. I am trying to debloat (both code and
dynamic memory) the XUL content sink from having to create global objects for
each XUL document, just to be able to precompile event handlers and scripts.

It is legal to pass a null "target" scope object into
JS_CompileUCFunctionForPrincipals.  It's also legal to pass a null name, btw --
the two are independent parameters.  You might want the JS compiler to prebind
the function it compiles to a scope that you know you'll use when invoking the
function; or not.  You might want to name the function internally (for
decompilation, pretty-printing) but not find the function object value to that
name in any scope when precompiling (i.e., when passing a null target obj
param); or not.

jst, mstoltz: can we allow a null target parameter here and use a null
principal? The XUL code and the JS engine will arrange to clone any function
objects that are precompiled against a null, or the wrong, scope object.  The
caps code should use the static (parent) link to find the right principals for
the clone, preferring those principals to any precompiled into the cloned
function object's prototype object.

/be

Comment 21

[Brendan Eich [:brendan]]

I wrote

> but not find the function object value to that name in any scope when
> precompiling

s/find/bind/

I agree we should not use the subject principals in
nsJSContext::CompileEventHandler under any circumstances.  I'm proposing that if
a null target is passed in, that method should use null principals when calling
JS_CompileUCFunctionForPrincipals.  Null principals should not give any powers
to scripts having them.

/be

Comment 22

[Mitchell Stoltz (not reading bugmail)]

Unfortunately, that's not the case - null principal is treated as equivalent to
the system principal. While I think that's a bad state of affairs (I have an
action item to make all system scripts require the system principal), it would
be a big, somewhat risky change that I don't want to attempt as part of this
fix. So, for now, no object should have a null principal.

Comment 23

[Brendan Eich [:brendan]]

Ok, but I don't see a practical difference right now between
nsJSContext::CompileEventHandler using a null principal for null target (when
XUL is precompiling) and requiring a non-null target from which to get a
non-null principal.

All precompiled XUL JS comes from chrome: URLs, which get the system principal
through the pile of bloated nsXULPDGlobalObject/nsXULPrototypeDocument code I'm
trying to eliminate in bug 195010 and sequelae.  If null means the same as
system principal, let's save that code and runtime data.

But remember: all precompiled functions are auto-cloned before being invoked
against real chrome windows and other, window-contained event targets.  See
http://lxr.mozilla.org/mozilla/source/dom/src/base/nsJSEnvironment.cpp#1090 and
http://lxr.mozilla.org/mozilla/source/content/xul/content/src/nsXULElement.cpp#1848
(notice the difference between aTarget and scopeObject in the latter, especially
when you apply my patch for bug 195010, which I'm hoping to land today).

See
http://lxr.mozilla.org/mozilla/source/caps/src/nsScriptSecurityManager.cpp#1820
for where the caps code does the right thing with cloned function objects.

/be

Comment 24

[Johnny Stenback (:jst)]

Attachment: Allow for null targets, use null JSPrincipals

Ok, this should allow for that, and I assume you're correct in that
JS_CompileUCFunctionForPrincipals() is safe for a null object, though I did try
that in the debugger, and the JS engine ended up trying to access members on
the object, and crashed. I'm assuming that then only happens in circumstances
where it would never get a null object...

Comment 25

[Mitchell Stoltz (not reading bugmail)]

Comment on attachment 120240 [details] [diff] [review]
Allow for null targets, use null JSPrincipals

r=mstoltz on the latest patch.

Comment 26

[Brendan Eich [:brendan]]

Comment on attachment 120240 [details] [diff] [review]
Allow for null targets, use null JSPrincipals

jst: make sure you have an up-to-date jsapi.c.

This looks ok to me, although nsJSContext::GetSecurityManager now seems to go
out of its way to set its out param before failing, which isn't strictly
necessary (but which doesn't hurt).

/be

Comment 27

[georgi - hopefully not receiving bugspam]

Question about comment 23:

>All precompiled XUL JS comes from chrome: URLs, which get the system principal
>through the pile of bloated nsXULPDGlobalObject/nsXULPrototypeDocument code I'm
>trying to eliminate in bug 195010 and sequelae.  If null means the same as
>system principal, let's save that code and runtime data.

Is this precompiled JS:
a=new Script("alert('a')");a.compile();a.exec()

Can it cause security problems because it is remote?

Comment 28

[Brendan Eich [:brendan]]

georgi: by precompiled XUL JS I mean the scripts and event handlers compiled by
code in content/xul/content/src/nsXULElement.cpp.

Using the Script object from JS precompiles differently, and uses the principals
of the calling script.  See code at and after
http://lxr.mozilla.org/mozilla/source/js/src/jsscript.c#191.

/be

Comment 29

[Johnny Stenback (:jst)]

Fix checked in. Marking FIXED. Feel free to reopen if this needs fixin' on
branches or whatever.

Comment 30

[Johnny Stenback (:jst)]

Oh, and yeah, Brendan, I had an old jsapi.c when I tested (null target)...

Comment 31

[Peter Van der Beken [:peterv]]

We backed this out because of smoketest blocker bug 201889.  I have a tree with
the patch, trying to look into it now.

Comment 32

[Peter Van der Beken [:peterv]]

So I tried adding back some code to use the principal of the global object when
target is null in nsJSContext::CompileEventHandler and that seems to fix the
problems (I test by trying to open the JS Console). It's obviously not the right
fix but it might give others some clue as to what is going on. I need to go home
now, will check back from there.

Comment 33

[Johnny Stenback (:jst)]

Attachment: Make caps deal with Function object w/o principals

This fixes the regression that was caused by the above patch. This patch makes
caps treat a cloned Function object whose script has no principals as brutally
shared chrome.

Comment 34

[Johnny Stenback (:jst)]

Attachment: Get the principals from the scope for all cloned functions

Per feedback from Brendan, this makes caps always get the principals for a
cloned function object from the functions scope since the principals that are
compiled into the function are never reliable, no matter what they are.

Comment 35

[Johnny Stenback (:jst)]

Attachment: Same as above with some assertions and comment changes.

Comment 36

[Brendan Eich [:brendan]]

Comment on attachment 120518 [details] [diff] [review]
Same as above with some assertions and comment changes.

sr=[e-mail] with the comment tweak we spoke about on IRC.

/be

Comment 37

[Mitchell Stoltz (not reading bugmail)]

Comment on attachment 120518 [details] [diff] [review]
Same as above with some assertions and comment changes.

This change looks OK to me - looks like the security-critical cases will behave
correctly. r=mstoltz.

Comment 38

[Johnny Stenback (:jst)]

Checked in both patches. FIXED.

Comment 39

[Jesse Ruderman]

New testcase URL (old one is now 404):
http://liudieyuinchina.vip.sina.com/EdgeLink/EdgeLink-MyPage.htm

Comment 40

[Johnny Stenback (:jst)]

Fix checked in on the 1.3 branch.

Comment 41

[Mozilla_rocks]

Comment on attachment 120096 [details] [diff] [review]
Patch v2 - addresses review comments

removing obsolete review request