Note: There are a few cases of duplicates in user autocompletion which are being worked on.
Bug 201132 (liu1)

Same origin violation using events

RESOLVED FIXED in mozilla1.4beta

Status

()

Core
Security
--
critical
RESOLVED FIXED
15 years ago
14 years ago

People

(Reporter: Mitchell Stoltz (not reading bugmail), Assigned: Mitchell Stoltz (not reading bugmail))

Tracking

Trunk
mozilla1.4beta
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 6 obsolete attachments)

193 bytes, text/html
Details
728 bytes, text/html
Details
5.42 KB, patch
Mitchell Stoltz (not reading bugmail)
: review+
brendan
: superreview+
Details | Diff | Splinter Review
2.98 KB, patch
Mitchell Stoltz (not reading bugmail)
: review+
brendan
: superreview+
Details | Diff | Splinter Review
from liudieyuinchina@yahoo.com.cn - the following testcase appears to be able to
read properties (including document.cookie) from a third-party webpage - a
same-origin violation.
(Assignee)

Comment 1

15 years ago
Created attachment 119757 [details]
auxiliary file for testcase
(Assignee)

Comment 2

15 years ago
Created attachment 119758 [details]
The exploit - reads cookie from www.yahoo.com
(Assignee)

Comment 3

15 years ago
Created attachment 119759 [details]
The exploit

Whoops, disregard the last one
Attachment #119758 - Attachment is obsolete: true
(Assignee)

Comment 4

15 years ago
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.
Severity: normal → critical
Status: NEW → ASSIGNED
Keywords: nsbeta1
OS: Windows 2000 → All
Hardware: PC → All
Target Milestone: --- → mozilla1.4beta
Adding John to CC since this might be events code...
I have a working copy of the exploit locally at
http://green.nscp.aoltw.net/heikki/201132/MOZedgeLink.htm
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...
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.

Created attachment 120013 [details] [diff] [review]
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.

Updated

15 years ago
Attachment #120013 - Flags: superreview?(brendan)
Attachment #120013 - Flags: review?(mstoltz)
I think you want NS_IF_ADDREF in nsJSContext::GetSecurityManager().
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
(Assignee)

Comment 12

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

Comment 13

15 years ago
Created attachment 120096 [details] [diff] [review]
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.
Attachment #120013 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #120096 - Flags: superreview?(brendan)
Attachment #120096 - Flags: review?(heikki)
Attachment #120096 - Flags: review?(heikki) → review+
(Assignee)

Updated

15 years ago
Alias: liu1
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 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
(Assignee)

Updated

15 years ago
Flags: blocking1.4b?
(Assignee)

Comment 16

15 years ago
jst said he's getting rid of the (target && !sSecurityManager) clause that gets
the subject principal.
Created attachment 120152 [details] [diff] [review]
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.
Attachment #120096 - Attachment is obsolete: true
(Assignee)

Comment 18

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

r=mstoltz.
Attachment #120152 - Flags: superreview?(brendan)
Attachment #120152 - Flags: review+
Attachment #120152 - Flags: superreview?(brendan) → superreview+
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=brendan@mozilla.org on substantially
different, new patches.

/be
Attachment #120152 - Flags: superreview+ → superreview?(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
Attachment #120152 - Flags: superreview?(brendan) → superreview-
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
(Assignee)

Comment 22

15 years ago
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.
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
Created attachment 120240 [details] [diff] [review]
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...
(Assignee)

Comment 25

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

r=mstoltz on the latest patch.
Attachment #120240 - Flags: review+
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
Attachment #120240 - Flags: superreview+
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?
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
Fix checked in. Marking FIXED. Feel free to reopen if this needs fixin' on
branches or whatever.
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Oh, and yeah, Brendan, I had an old jsapi.c when I tested (null target)...
Blocks: 201889
We backed this out because of smoketest blocker bug 201889.  I have a tree with
the patch, trying to look into it now.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Created attachment 120508 [details] [diff] [review]
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.
Attachment #120152 - Attachment is obsolete: true
Created attachment 120511 [details] [diff] [review]
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.
Attachment #120508 - Attachment is obsolete: true
Created attachment 120518 [details] [diff] [review]
Same as above with some assertions and comment changes.
Attachment #120511 - Attachment is obsolete: true
Comment on attachment 120518 [details] [diff] [review]
Same as above with some assertions and comment changes.

sr=brendan@mozilla.org with the comment tweak we spoke about on IRC.

/be
Attachment #120518 - Flags: superreview+
(Assignee)

Comment 37

15 years ago
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.
Attachment #120518 - Flags: review+
Checked in both patches. FIXED.
Status: REOPENED → RESOLVED
Last Resolved: 15 years ago15 years ago
Resolution: --- → FIXED

Comment 39

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

Updated

15 years ago
Group: security
Fix checked in on the 1.3 branch.

Updated

14 years ago
Flags: blocking1.4b?
(Assignee)

Updated

14 years ago
Attachment #120013 - Flags: superreview?(brendan)
Attachment #120013 - Flags: superreview-
Attachment #120013 - Flags: review?(mstoltz)
Attachment #120013 - Flags: review-

Comment 41

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

removing obsolete review request
Attachment #120096 - Flags: superreview?(brendan)
You need to log in before you can comment on or make changes to this bug.