344495 - XSS against certain sites that are using scripts that access properties of top, parent or opener

Status: VERIFIED FIXED

(Core :: Security, defect, P2)

Comment 1

[moz_bug_r_a4]

Attachment: testcase 1

This tries to get cookies for login.live.com.

Comment 2

[moz_bug_r_a4]

Attachment: testcase 2

This tries to get cookies for Gmail.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Terrible patch

This might be the ugliest patch ever, but it's really more of a POC than anything. As far as I know, this patch fixes these bugs without breaking anything badly enough (e.g., I could start the browser and go to google maps and gmail). The idea is to make native frames participate in decisions about whether or not property accesses are allowed. Because of the way that XPConnect hooks into the JS engine, we actually need to skip the top frame just before we call the native function, and when we're wrapping the results.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 229320 [details] [diff] [review]
Terrible patch

Missing jsinterp.h diff?

/be

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: With jsinterp.h

Since I don't have a thunderbird build to try out, I'm getting mscott to try this patch for me.

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 229327 [details] [diff] [review]
With jsinterp.h

Redundant Set-Skippable before goto done.

Also wondering how wide the API needs to be.

/be

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 229327 [details] [diff] [review]
With jsinterp.h

Another thought: make JS_FrameIterator check JSFRAME_SKIPPABLE (rename to JSFRAME_DONT_ITERATE?).

/be

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated to comments

This patch addresses brendan's comments. It also fixes a bug where calls from chrome through XPCNativeWrapper were getting denied because the XPCNativeWrapper frame was hiding the chrome frame. I fixed this by making that stack frame not-iterable through the call, which should be OK, since, as far as caps is concerned, we're simply mirroring the principals of whatever we're calling.

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 229682 [details] [diff] [review]
Updated to comments


>+++ js/src/xpconnect/src/XPCNativeWrapper.cpp	18 Jul 2006 17:38:43 -0000
>@@ -376,23 +376,29 @@ XPC_NW_FunctionWrapper(JSContext *cx, JS
>   JSObject *methodToCallObj = ::JS_GetParent(cx, funObj);
>   XPCWrappedNative *wrappedNative =
>     XPCNativeWrapper::GetWrappedNative(cx, obj);
> 
>   if (!::JS_ObjectIsFunction(cx, methodToCallObj) || !wrappedNative) {
>     return ThrowException(NS_ERROR_UNEXPECTED, cx);
>   }
> 
>+  JSStackFrame *fp = cx->fp;
>+  JSBool iterable = JS_GetFrameIterable(cx, fp);
>+  JS_SetFrameIterable(cx, fp, JS_FALSE);
>+
>   jsval v;
>   if (!::JS_CallFunctionValue(cx, wrappedNative->GetFlatJSObject(),
>                               OBJECT_TO_JSVAL(methodToCallObj), argc, argv,
>                               &v)) {
>+    JS_SetFrameIterable(cx, fp, iterable);
>     return JS_FALSE;
>   }
> 
>+  JS_SetFrameIterable(cx, fp, iterable);

Here, unlike the XPCWrappedNative::CallMethod case, I would prefer JSBool ok = ::JS_CallFunctionValue(...) followed by a single unconditional JS_SetFrameIterable call.

Nice patch, r=me with that.

/be

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: With that too

I missed a spot in the previous patch: we needed to not iterate around the call to ccx.CanCallNow(), since that could create a tearoff, which would need to wrap something.

Comment 11

[Brendan Eich [:brendan]]

Dan, this bug's patch adds a needed access-check to watchpoint invocation, which might help stop that firebug exploit.

/be

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 229692 [details] [diff] [review]
With that too

r=me, thanks.

/be

Comment 13

[moz_bug_r_a4]

I've applied the latest patch and tested.

It is still exploitable by using location.href setter instead of
location.assign to load a javascript: url on the target site.  Also, eval can
be used to exploit on the trunk, 2.0 and 1.5.0.5.  Sorry, I should have
attached various test cases (at least the one that uses eval).

Comment 14

[moz_bug_r_a4]

Attachment: testcase 3

This uses location.href setter.

Comment 15

[moz_bug_r_a4]

Attachment: testcase 4

This uses location.href setter with Components.lookupMethod.

Comment 16

[moz_bug_r_a4]

Attachment: testcase 5

This uses eval.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch to test

moz_bug_r_a4, could you test this patch? Feel free to use new Script, etc.

Comment 18

[moz_bug_r_a4]

(In reply to comment #17)
That patch stops all testcases.  And, I've tried to use new Script and new
Function somehow, but nothing has worked.

Comment 19

[moz_bug_r_a4]

It seems that that patch also fixes the privilege escalation bugs.  With that
patch, all testcases in bug 344494, bug 344751 and 345305 fail to exploit.

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated again

I had to go back to checking explicitly for JS_GetFrameSkippable in caps to preserve JS_FrameIterator API compatibility (and thus changed the name back again). I also added a large comment in xpcwrappednative.cpp explaining what most of this craziness is about.

Comment 21

[Johnny Stenback (:jst)]

Comment on attachment 231014 [details] [diff] [review]
Updated again

r=jst

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 231014 [details] [diff] [review]
Updated again

>Index: js/src/jsobj.c

>+        JS_ASSERT(down || !caller);
>+
>+        if (down && caller && down != caller) {

Given the JS_ASSERT, testing |down && caller| is equivalent to testing 

>+            if (!principals ||
>+                !caller->script->principals ||
>+                !principals->subsume(caller->script->principals, principals))

Should we error out if principals->subsume != caller->script->principals->subsume ?

So what does this block of code really check?  What's |caller|?  What's |down| when we're done with the loop?  Some comments would be very nice here.  For example, how do I tell whether the subsume call is getting the args passed in the right order?  (This last part is not helped by the fact that the declaration of |subsume| doesn't say what the args are, so I'd have to look at existing impls to figure out what should be passed to it.  Adding comments about this would be nice too.)

> obj_watch_handler(JSContext *cx, JSObject *obj, jsval id, jsval 

>+    if (!JS_ValueToId(cx, id, &propid) ||
>+        !OBJ_CHECK_ACCESS(cx, obj, propid, JSACC_WATCH, &v, &attrs)) {

Whose access to do what is this checking?  Please document?

>Index: js/src/jsdbgapi.h

>+extern JS_PUBLIC_API(JSBool)
>+JS_GetFrameSkippable(JSContext *cx, JSStackFrame *fp);
>+
>+extern JS_PUBLIC_API(void)
>+JS_SetFrameSkippable(JSContext *cx, JSStackFrame *fp, JSBool skippable);

What do these do?  I realize there is a policy of not documenting things in the JS engine headers, but did you at least put docs for these on the wiki?

In particular, is this just a hook for jseng consumers, who can make "skippable" mean whatever they want, or does jseng actually do something interesting with frames set as skippable?  If the latter, document what, at least generally?

>Index: js/src/xpconnect/src/xpcwrappednative.cpp
> XPCWrappedNative::CallMethod(XPCCallContext& ccx,

>+    JSStackFrame *fp = ccx.GetJSContext()->fp;
>+    PRBool skippable = JS_GetFrameSkippable(ccx, fp);
>+    JS_SetFrameSkippable(ccx, fp, JS_TRUE);
>+
>     nsresult rv = ccx.CanCallNow();
>+
>+    JS_SetFrameSkippable(ccx, fp, skippable);

This needs some documentation love.  Explain why this is being done, point to the code that relies on this happening, whatever it takes to make this magic make sense.

If the big comment below covers this, just say so here, but it's non-obvious why we need to be skippable just around CanCallNow().  Why reset the old state?  Early returns or something?  If so, document.  If not, document what the reason is.

>+    //    of the function itself has no bearing on what the principals that is

s/principals/principal/

>+    // -- In general, callers of GetSubjectPrincipal, when called from
>+    //    C++ code don't care about the currently running XPConnect function.

Missing comma after "code"?

>+    //    Instead, they really want to see who called the XPConnect function.
>+    //    This means that we trust XPConnect functions more than other
>+    //    functions (more than JS engine functions, even).

Doesn't it rather mean that the principal of the XPConnect function is the same as the object principal for security check purposes and that if we're in this code we're calling into C++ so the function that's going to run is C++-implemented, generally speaking?  I suppose this last could mean we trust it, but the real issue is that its principal is silly, right?

>+    // Note that we'd need to set this frame as skippable around this security
>+    // check (and the CanCallNow check above) and while wrapping return
>+    // values, since during this time, this function isn't actually running,

Hmm.. Which one is "this frame" here?  Is it the frame that called into XPConnect?  Or is it a frame that XPConnect has pushed on the frame stack?  I'm hoping its the latter, given what we're doing here; perhaps we should just document that?

>Index: js/src/xpconnect/src/XPCNativeWrapper.cpp
>+  JSStackFrame *fp = cx->fp;
>+  JSBool skippable = JS_GetFrameSkippable(cx, fp);
>+  JS_SetFrameSkippable(cx, fp, JS_TRUE);

Please document why this is the right thing to do.

sr=bzbarsky with the expanded documentation.

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #22)
> Given the JS_ASSERT, testing |down && caller| is equivalent to testing 

Did you mean to say "testing |caller|"?

> Should we error out if principals->subsume !=
> caller->script->principals->subsume ?

I don't think so: subsumes was created exactly to avoid doing this sort of opaque-pointer checking when we get two objects from two different worlds.

> > obj_watch_handler(JSContext *cx, JSObject *obj, jsval id, jsval 
> Whose access to do what is this checking?  Please document?

This was originally brendan's idea, and in trying to document it, I've lost the motivation (despite the fact that it fixes the first testcase without the rest of the patch). If I add a watchpoint to an object, and the function is from my security context, and then some code from another context runs, using my object for whatever purpose, then that code shouldn't need access to the original watch function to modify that object. I'll take it out unless brendan defends it.

Comment 24

[Brendan Eich [:brendan]]

(In reply to comment #23)
Sounds like some other bug seemed to require checking from obj_watch_handler, but we fixed that bug. ;-)

I still think we should strive to walk the stack and compute the greatest lower bound of trust labels active.  I don't particularly trust certain kinds of native methods more than others.  But I need to unjetlag to review this patch carefully.

/be

Comment 25

[moz_bug_r_a4]

The proposed patch causes a regression.  With that patch applied, when
document.open is called, the document's principal is set to the document.open
function's principal instead of the caller's principal.  (document.write does
not regress -- when document.write on a document that has finished loading is
called, the document's principal is set to the caller's principal.)

Please see bug 346659.  The proposed patch does *not* fix bug 346659, and the
regression makes the problem far worse.

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #25)
> The proposed patch causes a regression.  With that patch applied, when
> document.open is called, the document's principal is set to the document.open
> function's principal instead of the caller's principal.

Isn't the regression there simply that we push document.open.call's principals onto the principal stack now (and since document.open is allAccess, that's a Bad Thing (TM))? That's easy to fix, although I don't think that this patch will necessarily fix that bug.

Comment 27

[moz_bug_r_a4]

(In reply to comment #26)

Yeah, there is another critical regression, which I didn't notice, regarding
Function.prototype.(call|apply).  This allows content to escalate privilege.

  <marquee id="m">
  h = location.__lookupSetter__("href");
  m.init.call.call(h, location, "javascript:alert(Components.stack)");

Comment 28

[moz_bug_r_a4]

Attachment: testcase 6 - Arbitrary code execution using Function.prototype.call

This works only with the proposed patch applied.

Comment 29

[moz_bug_r_a4]

Attachment: testcase 7 - Test for document.open's regression

Comment 30

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #27)
> Yeah, there is another critical regression, which I didn't notice, regarding
> Function.prototype.(call|apply).  This allows content to escalate privilege.

Yeah, I realized this yesterday. This patch won't fly in the long run, it'll be whack-a-mole fixing the regressions caused by it. The discussion in bug 346659 will hopefully end up in a better patch for this bug and similar.

Comment 31

[Mike Schroepfer]

We'd like this on the branch if we can to get bug 344494 fixed as well.

Comment 32

[Brendan Eich [:brendan]]

Comment on attachment 231014 [details] [diff] [review]
Updated again

We need a breather, then a sane model on which to implement a non-whack-a-mole fix.  So this bug should not block beta 2.

/be

Comment 33

[Blake Kaplan (:mrbkap) (inactive)]

The patches in this bug cause more harm than good. I have a new strategy that I'll try to put in action tomorrow.

Comment 34

[Daniel Veditz [:dveditz]]

Given the last two comments it doesn't look like this could possibly make 1.8.0.7, especially not safely.

Comment 35

[Mike Schroepfer]

Is this something that we still want for 181?

Comment 36

[Mike Schroepfer]

Pushing to 1.8.1.1

Comment 37

[Daniel Veditz [:dveditz]]

Restoring lost blocking flag

Comment 38

[Blake Kaplan (:mrbkap) (inactive)]

Brendan and I might have found a new alternative that isn't so buggy... Stay tuned!

Comment 39

[Daniel Veditz [:dveditz]]

(In reply to comment #38)
> Brendan and I might have found a new alternative that isn't so buggy... Stay
> tuned!

Still tuned, did the alternative pan out?

Comment 40

[Daniel Veditz [:dveditz]]

Not baked yet, moving to the next release

Comment 41

[chris hofmann]

any updates on what we should do on this one?

Comment 42

[Samuel Sidler (old account; do not CC)]

Blake, any word on the alternative?

Comment 43

[Damon Sicore (:damons)]

Targeting to B1 per conversation with Blake.

Comment 44

[Blake Kaplan (:mrbkap) (inactive)]

This bug as filed is fixed by XOWs. The remaining issues will be covered by other bugs.

Comment 45

[Al Billings [:abillings - ex-MoCo]]

This is verified fixed in Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050704 Minefield/3.0pre.