Last Comment Bug 289074 - security hole in markLinkVisited (Bug 217195) has not been fixed
: security hole in markLinkVisited (Bug 217195) has not been fixed
Status: RESOLVED FIXED
[sg:fix] need trunk answers from bren...
: fixed-aviary1.0.3, fixed1.7.7
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.8beta2
Assigned To: Johnny Stenback (:jst, jst@mozilla.com)
:
Mentors:
: 289075 289078 289079 289081 289405 (view as bug list)
Depends on: 281988 289083 292591
Blocks: 223041 sbb+ 289187 289945 289949
  Show dependency treegraph
 
Reported: 2005-04-05 00:16 PDT by moz_bug_r_a4
Modified: 2011-08-05 21:32 PDT (History)
22 users (show)
brendan: blocking1.7.7+
brendan: blocking‑aviary1.0.3+
asa: blocking1.8b3+
dveditz: blocking‑aviary1.5+
bob: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (606 bytes, text/html)
2005-04-05 00:21 PDT, moz_bug_r_a4
no flags Details
Register an objectPrincipalFinder with the JS engine to ensure that it finds the right principal when using eval (1.97 KB, patch)
2005-04-06 01:16 PDT, Johnny Stenback (:jst, jst@mozilla.com)
jst: superreview+
dbaron: approval‑aviary1.0.3+
dbaron: approval1.7.7+
Details | Diff | Review
testcase 2 (706 bytes, text/html)
2005-04-06 11:47 PDT, moz_bug_r_a4
no flags Details
testcase 3 (452 bytes, text/html)
2005-04-06 11:52 PDT, moz_bug_r_a4
no flags Details
keep eval and exec from being called across any trust domain boundary (15.18 KB, patch)
2005-04-06 16:33 PDT, Brendan Eich [:brendan]
shaver: review+
jst: superreview+
dveditz: approval‑aviary1.0.3+
dveditz: approval1.7.7+
Details | Diff | Review
diff -w version of patch, with a few comment tweaks from last patch (13.61 KB, patch)
2005-04-06 17:15 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
creates preference fuckbill in prefs.js (996 bytes, text/html)
2005-04-07 06:48 PDT, georgi - hopefully not receiving bugspam
no flags Details
tries to install software but crashes (980 bytes, text/html)
2005-04-07 06:50 PDT, georgi - hopefully not receiving bugspam
no flags Details
Attempt at preventing chrome from seeing shadowed properties (xpconnect diff) (12.10 KB, patch)
2005-04-07 19:49 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Review
Cleaned up version of the above. (16.42 KB, patch)
2005-04-08 00:05 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Review
patch to jst's last patch to handle user-defined getters and setters (9.90 KB, patch)
2005-04-08 10:06 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
new patch to test (14.96 KB, patch)
2005-04-08 12:27 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
Brendan's last patch with logic to call the 'real' getter/setter, and GetProperty() hook, if wanted by the wrapper (19.81 KB, patch)
2005-04-08 13:52 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Review
Brendan's last patch with logic to call the 'real' getter/setter, and Get/SetProperty() hook, if wanted by the wrapper (20.46 KB, patch)
2005-04-08 14:00 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Review
Don't throw exceptions if we're asked to call a setter for a readonly prop (jus assert for now). (20.46 KB, patch)
2005-04-08 14:58 PDT, Johnny Stenback (:jst, jst@mozilla.com)
brendan: approval‑aviary1.0.3+
Details | Diff | Review
diff -w version of last patch (14.95 KB, patch)
2005-04-08 15:27 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
Same thing with shaver's suggestion (20.31 KB, patch)
2005-04-08 16:17 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Review
testcase (598 bytes, text/html)
2005-04-08 23:36 PDT, moz_bug_r_a4
no flags Details
fix crash on callable non-function object getter/setter (1.53 KB, patch)
2005-04-09 10:00 PDT, Brendan Eich [:brendan]
jst: review+
jst: superreview+
brendan: approval‑aviary1.0.3+
brendan: approval1.7.7+
Details | Diff | Review
this creates pref "_test_.289074" (718 bytes, text/html)
2005-04-09 10:51 PDT, moz_bug_r_a4
no flags Details
this is the other version of Bug 289075 (975 bytes, text/html)
2005-04-09 10:53 PDT, moz_bug_r_a4
no flags Details
DOMLinkAdded Event from document.body (546 bytes, text/html)
2005-04-09 10:57 PDT, moz_bug_r_a4
no flags Details
remove __proto__ abusage in chrome and fail such attempts in xpconnect (19.36 KB, patch)
2005-04-09 17:48 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
diff -w version of last patch (18.48 KB, patch)
2005-04-09 17:53 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
remove/prevent __proto__ abusage, v2 (for applying) (22.10 KB, patch)
2005-04-09 22:35 PDT, Brendan Eich [:brendan]
no flags Details | Diff | Review
diff -w version of last patch (for reviewing) (21.15 KB, patch)
2005-04-09 22:38 PDT, Brendan Eich [:brendan]
jst: review+
dbaron: superreview+
Details | Diff | Review
Fix bugzilla regression (6.44 KB, patch)
2005-04-11 14:22 PDT, Johnny Stenback (:jst, jst@mozilla.com)
dveditz: review+
brendan: superreview+
chase: approval‑aviary1.0.3+
chase: approval1.7.7+
chase: approval1.8b2+
Details | Diff | Review
keep eval and exec from being called across any trust domain boundary, v2 (3.27 KB, patch)
2005-04-11 17:55 PDT, Brendan Eich [:brendan]
jst: review+
shaver: review+
jst: superreview+
chase: approval‑aviary1.0.3+
chase: approval1.7.7+
chase: approval1.8b2+
Details | Diff | Review
xbl file (eval) (859 bytes, application/xml)
2005-04-22 10:26 PDT, moz_bug_r_a4
no flags Details
XBL version of bug 289078 attachment 179677 (eval) (147 bytes, text/html)
2005-04-22 10:30 PDT, moz_bug_r_a4
no flags Details
xbl file (Script.exec) (592 bytes, application/xml)
2005-04-22 10:32 PDT, moz_bug_r_a4
no flags Details
XBL version of bug 289083 attachment 179682 (Script.exec) (141 bytes, text/html)
2005-04-22 10:35 PDT, moz_bug_r_a4
no flags Details
trunk version (34.92 KB, patch)
2005-05-18 06:36 PDT, Daniel Veditz [:dveditz]
no flags Details | Diff | Review

Description moz_bug_r_a4 2005-04-05 00:16:54 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.6) Gecko/20050319
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.6) Gecko/20050317 Firefox/1.0.2

See Bug 217195, its fix is not sufficient.

Vulnerable code:
from markLinkVisited in contentAreaUtils.js

var oldHref = linkNode.getAttribute("href");
linkNode.setAttribute("href", "");
linkNode.setAttribute("href", oldHref);

Exploit:
It is possible to execute the second argument, such as the following.

  eval.setAttribute = eval.call;
  eval.setAttribute("href", code);

|eval| can work with XPCNativeWrapper, such as the following.
(|new XPCNativeWrapper(eval, "href").href| works as
|new XPCNativeWrapper(linkNode, "href").href|.)

  linkNode = document.links[0];
  eval.__proto__ = linkNode;

It is possible to pass |eval| as linkNode, such as the following.

  <a href="#"><span>test</span></a>

  document.links[0].firstChild
  .__defineGetter__('parentNode', function() { return eval; });


I have confirmed that the following testcase works in:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.6) Gecko/20050317
Firefox/1.0.2
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.7) Gecko/20050404
Firefox/1.0.3
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050404
Firefox/1.0+


Reproducible: Always

Steps to Reproduce:
Comment 1 moz_bug_r_a4 2005-04-05 00:21:45 PDT
Created attachment 179674 [details]
testcase
Comment 2 Johnny Stenback (:jst, jst@mozilla.com) 2005-04-06 00:47:58 PDT
Turns out that the problem here is that it's possible to trick our code to
execute JS code with chrome priveleges using eval, even if the eval function and
the object passed as the 'this' parameter is from content code. That should not
be the case, the code that is being evaluated should run with the priveleges of
the page where the 'this' param came from. I'm going to dupe the other similar
bugs against this one and push the fix in here.
Comment 3 Johnny Stenback (:jst, jst@mozilla.com) 2005-04-06 00:49:16 PDT
*** Bug 289075 has been marked as a duplicate of this bug. ***
Comment 4 Johnny Stenback (:jst, jst@mozilla.com) 2005-04-06 00:57:50 PDT
*** Bug 289078 has been marked as a duplicate of this bug. ***
Comment 5 Johnny Stenback (:jst, jst@mozilla.com) 2005-04-06 00:58:21 PDT
*** Bug 289079 has been marked as a duplicate of this bug. ***
Comment 6 Johnny Stenback (:jst, jst@mozilla.com) 2005-04-06 00:58:22 PDT
*** Bug 289081 has been marked as a duplicate of this bug. ***
Comment 7 Johnny Stenback (:jst, jst@mozilla.com) 2005-04-06 01:16:13 PDT
Created attachment 179828 [details] [diff] [review]
Register an objectPrincipalFinder with the JS engine to ensure that it finds the right principal when using eval

This fixes this problem by registering an object principal finder which Brendan
already provided in the JS engine, but was never used. This ensures that the JS
engine can find the right principal when calling eval.

r+sr=brendan (in person).
Comment 8 Johnny Stenback (:jst, jst@mozilla.com) 2005-04-06 01:17:48 PDT
Fix landed for 1.0.3
Comment 9 sairuh (rarely reading bugmail) 2005-04-06 10:34:14 PDT
using 2005040605-1.0.3 ffox bits on Mac OS X, the test cases in this bug as well
as those in the dups (289075, 289078, 289079, 289081) no longer cause the
exploit popup to appear.
Comment 10 Brendan Eich [:brendan] 2005-04-06 11:04:25 PDT
Want this in the 1.7 branch and the trunk, too.

/be
Comment 11 moz_bug_r_a4 2005-04-06 11:42:38 PDT
I've tested in Firefox/1.0.3-2005-04-06-06.
The testcases I've sent before does not work anymore, however, I've found that
it is still exploitable, by using function in the code.

  //var code = "alert('Exploit!\\n\\n' + Components.stack);";
  var code = "(function() { alert('Exploit!\\n\\n' + Components.stack); })();";
Comment 12 moz_bug_r_a4 2005-04-06 11:47:22 PDT
Created attachment 179874 [details]
testcase 2
Comment 13 moz_bug_r_a4 2005-04-06 11:52:13 PDT
Created attachment 179875 [details]
testcase 3

This is for Bug 289083
Comment 14 Brendan Eich [:brendan] 2005-04-06 16:33:18 PDT
Created attachment 179896 [details] [diff] [review]
keep eval and exec from being called across any trust domain boundary

Shaver should review the js/src changes.

/be
Comment 15 Daniel Veditz [:dveditz] 2005-04-06 16:50:42 PDT
Comment on attachment 179896 [details] [diff] [review]
keep eval and exec from being called across any trust domain boundary

a=chase/drivers for branches pending reviews
Comment 16 Mike Shaver (:shaver -- probably not reading bugmail closely) 2005-04-06 16:55:24 PDT
Comment on attachment 179896 [details] [diff] [review]
keep eval and exec from being called across any trust domain boundary

Is that really the check we want?  Should a script with UniversalXPConnect be
able to do this?  Should both directions be banned?
Comment 17 Daniel Veditz [:dveditz] 2005-04-06 16:55:39 PDT
moz_bug_r_a4: please contact me about the Mozilla Security Bug Bounty. You can
use my bugzilla email or security@mozilla.org
Comment 18 Brendan Eich [:brendan] 2005-04-06 17:09:48 PDT
shaver: I don't actually want this case to work, for anyone.  If I had my head
screwed on right ten years ago, I would have made eval a reserved identifier, a
true special form -- no function reference extractable to be passed to victims.
 UniversalXPConnect wouldn't have mattered, in that case.  Let's pretend I did
implement eval that way, ok?

/be
Comment 19 Brendan Eich [:brendan] 2005-04-06 17:15:22 PDT
Created attachment 179898 [details] [diff] [review]
diff -w version of patch, with a few comment tweaks from last patch

This is good for reviewing, it shows the common sub-method of
CheckPropertyAccessImpl, ThrowAccessDeniedException, as not being much changed
apart from being subroutined.  Please review-stamp the prior patch, but have a
look here if you can.

/be
Comment 20 Johnny Stenback (:jst, jst@mozilla.com) 2005-04-06 17:23:44 PDT
Comment on attachment 179896 [details] [diff] [review]
keep eval and exec from being called across any trust domain boundary

sr=jst
Comment 21 Brendan Eich [:brendan] 2005-04-06 17:37:35 PDT
I've checked into the AVIARY_1_0_1_20050124_BRANCH and MOZILLA_1_7_BRANCH tags.
 Getting my trunk build in shape with a merged patch.

/be
Comment 22 Brendan Eich [:brendan] 2005-04-06 19:23:54 PDT
Fixed on the trunk, too.

/be
Comment 23 georgi - hopefully not receiving bugspam 2005-04-07 06:46:19 PDT
the kludge which stops testcase 3 (Mozilla/5.0 (X11; U; Linux i686; en-US;
rv:1.7.7) Gecko/20050406 Firefox/1.0.3) doesn't seem enough.

though it stops eval, looks like a lot of other functions like
navigator.preference(a,b) and InstallTrigger.install (crashes) may be called.
Comment 24 georgi - hopefully not receiving bugspam 2005-04-07 06:48:13 PDT
Created attachment 179946 [details]
creates preference fuckbill in prefs.js
Comment 25 georgi - hopefully not receiving bugspam 2005-04-07 06:50:00 PDT
Created attachment 179947 [details]
tries to install software but crashes
Comment 26 Brendan Eich [:brendan] 2005-04-07 08:08:31 PDT
Those are all native function objects.  When called from chrome, they should not
have privileges.  One more "kludge" coming up (because we really don't want to
take the patch for bug 281988 in a hurry).

/be
Comment 27 georgi - hopefully not receiving bugspam 2005-04-07 08:47:17 PDT
brendan,

are you saying that chrome will not be able to call at least netscape.preference
and InstallTrigger.* with full privilege?
Comment 28 Mike Shaver (:shaver -- probably not reading bugmail closely) 2005-04-07 09:41:18 PDT
I applaud your revisionism, indeed.
Comment 29 Brendan Eich [:brendan] 2005-04-07 10:55:37 PDT
I was rather thinking that *content* native function objects should not have
privilege when called from chrome -- only chrome native function objects (that
is, native function objects whose scope chain ends in a chrome window) should
have privilege.

But that is too much: it breaks chrome calls into the content's DOM native
function objects (setAttribute on a link, and many other valid cases).

Fooey.  We need jst's patch for bug 281988, I keep saying this.  Anything else
is putting fingers in holes in the dyke, and there are more holes than fingers.

I'm backing out my kludge.

/be
Comment 30 Scott MacGregor 2005-04-07 12:59:27 PDT
FYI, after updating a tree with this fix, I'm unable to get Thunderbird to
startup due to a bunch of debug assertions saying:

NS_ASSERTION(!(mode & JSACC_WRITE), "unexpected write access check");

nsDebug::Assertion(const char * 0x0116fd80, const char * 0x0116fd68, const char
* 0x0116fd20, int 456) line 109
nsScriptSecurityManager::CheckObjectAccess(JSContext * 0x01d3d3d0, JSObject *
0x02e17768, long 45444500, JSAccessMode JSACC_WRITE, long * 0x0012a3d0) line 456
+ 36 bytes
js_InternalGetOrSet(JSContext * 0x01d3d3d0, JSObject * 0x02e17768, long
45628984, long 48331432, int 8, unsigned int 1, long * 0x0012add8, long *
0x0012add8) line 1450 + 447 bytes
js_SetProperty(JSContext * 0x01d3d3d0, JSObject * 0x02e17768, long 45628984,
long * 0x0012add8) line 2877 + 53 bytes
js_Interpret(JSContext * 0x01d3d3d0, unsigned char * 0x02effffa, long *
0x0012af80) line 3425 + 1631 bytes
js_Invoke(JSContext * 0x01d3d3d0, unsigned int 0, unsigned int 2) line 1334 + 19
bytes
js_InternalInvoke(JSContext * 0x01d3d3d0, JSObject * 0x02e17768, long 48331496,
unsigned int 0, unsigned int 0, long * 0x00000000, long * 0x0012b0b0) line 1411
+ 20 bytes
JS_CallFunctionValue(JSContext * 0x01d3d3d0, JSObject * 0x02e17768, long
48331496, unsigned int 0, long * 0x00000000, long * 0x0012b0b0) line 3804 + 31 bytes
nsXBLProtoImplAnonymousMethod::Execute(nsIContent * 0x02ce7b30) line 334 + 26 bytes
nsXBLPrototypeBinding::BindingAttached(nsIContent * 0x02ce7b30) line 390 + 18 bytes
nsXBLBinding::ExecuteAttachedHandler() line 769
nsElementSH::PostCreate(nsElementSH * const 0x0296fe08,
nsIXPConnectWrappedNative * 0x02edbac8, JSContext * 0x01c012e0, JSObject *
0x02e17768) line 5698

I'll try again now that Brendan backed it out, but just something to be aware of
if we decide to put it back in.
Comment 31 Brendan Eich [:brendan] 2005-04-07 14:21:30 PDT
mscott: sorry about that.  To make up for it, I didn't just remove that bogus
assertion -- I backed the whole patch out. ;-)  It's stupid to stick mandatory
access controls in a bunch of natives; we're going for a systematic, low-level
fix over in bug 281988.

/be
Comment 32 Johnny Stenback (:jst, jst@mozilla.com) 2005-04-07 19:49:33 PDT
Created attachment 180005 [details] [diff] [review]
Attempt at preventing chrome from seeing shadowed properties (xpconnect diff)

This aint done, there are obvious performance problems with this change, but it
shows our current direction. This makes it so that content can't shadow DOM
properties from chrome code (or the patch simply prevents code with different
principals from shadowing from each other, but that'll change). This is WIP,
and needs more eyeballs and more testing, and it must not suck as bad as this
does performance wise. Please have a look and comment ASAP.
Comment 33 Brendan Eich [:brendan] 2005-04-07 20:39:49 PDT
Comment on attachment 180005 [details] [diff] [review]
Attempt at preventing chrome from seeing shadowed properties (xpconnect diff)

>-extern JSBool
>+extern JS_FRIEND_API(JSBool)
> js_InternalGetOrSet(JSContext *cx, JSObject *obj, jsid id, jsval fval,
>                     JSAccessMode mode, uintN argc, jsval *argv, jsval *rval);

Just for sanity, maybe for older MSVC or Borland or who-knows-what
compiler/linkers, you want JS_FRIEND_API around the return type in the
definition of js_InternalGetOrSet over in jsinterp.c, too.

>+#include "nsIScriptSecurityManager.h"

Heh, can do on the branch, trunk'll need some better factoring.

>+
>+static PRBool
>+IsCrossTrustDomainCall(JSContext *cx, JSObject *obj)
>+{

Idea: test cx->fp->scopeChain == wrapper->mMaybeScope->mGlobalJSObject (pass
wrapper as well as obj) and return false early if true.  I'm not sure when
mMaybeScope is valid (it's in a union with mMaybeProto), so if this does not
work out (we'd want an inline helper in xpcprivate.h to hide the ugly details),
you could always use JS_GetParent(cx, obj).

I pointed out this fast path to bz, and he said "why isn't caps using it?!" 
Good question!	I said "a=me for 1.8b2" ;-)

> JS_STATIC_DLL_CALLBACK(JSBool)
>+XPC_WN_Safe_PropertyStub(JSContext *cx, JSObject *obj, jsval idval, jsval *vp)
>+{
>+    if(IsCrossTrustDomainCall(cx, obj))
>+    {
>+        XPCWrappedNative* wrapper =
>+            XPCWrappedNative::GetWrappedNativeOfJSObject(cx, obj);
>+        // XXX: Can't use THROW_AND_RETURN_IF_BAD_WRAPPER here since this
>+        // hook gets calle from what looks like prototype wrappers,

typo: "called"

>+        // and for those JSObjects we don't find a wrapper (when
>+        // called through someobj.__proto__.somefunction.call). Let
>+        // those calls fall through to the stub for now.

(jst said he'd verify that the proto's ops and class are sane and make sense in
light of this patch.)

>+        if(wrapper)
>+        {
>+            if(!wrapper->IsValid())
>+                return Throw(NS_ERROR_XPC_HAS_BEEN_SHUTDOWN, cx);
>+
>+            JSBool foundMember;
>+            if(!GetUnshadowedMemberValue(cx, wrapper, idval, vp, JS_FALSE,

I'm wondering why we don't want to call the getter if idval identifies an
attribute, here -- or rather, I bet this low-level JSClass.getProperty hook is
never called for an attribute, because its JSScopeProperty that might be in the
property cache was found via the JSObjectOps.getProperty hook impl'd just
below, which always calls the getter if an attribute is found when crossing
trust domains and ignoring any overriding JS property.

I'll have to think about this more, but jst: if you can prove the class hook is
called for an attribute, let me know how that happens.

>+                                         &foundMember))
>+                return JS_FALSE;
>+
>+            if(foundMember)
>+                return JS_TRUE;
>+        }
>+    }
>+
>+    return JS_PropertyStub(cx, obj, idval, vp);
>+}
>+
>+JS_STATIC_DLL_CALLBACK(JSBool)
>+XPC_WN_JSOp_SafeGetProperty(JSContext *cx, JSObject *obj, jsid id, jsval *vp)
>+{
>+    if(IsCrossTrustDomainCall(cx, obj))
>+    {
>+        XPCWrappedNative* wrapper =
>+            XPCWrappedNative::GetWrappedNativeOfJSObject(cx, obj);
>+        THROW_AND_RETURN_IF_BAD_WRAPPER(cx, wrapper);
>+
>+        JSBool foundMember;
>+        if(!GetUnshadowedMemberValue(cx, wrapper, ID_TO_VALUE(id), vp,
>+                                     JS_TRUE, &foundMember))
>+            return JS_FALSE;
>+
>+        if(foundMember)
>+            return JS_TRUE;
>+    }
>+
>+    return js_ObjectOps.getProperty(cx, obj, id, vp);
>+}

This looks good.

So the XPCWrappedNative JSObjectOps (all of 'em ;-) just copy the native
js_ObjectOps.lookupProperty hook, which calls JSClass.resolve (with
JSCLASS_NEW_RESOLVE).  That means (memory here, check me) we're lazily
populating WN prototypes from typeinfo.  That says we can end up with property
cache references to JSScopeProperty nodes that have JSPROP_GETTER in their
attributes, which makes me wonder again why callGetter isn't always true.

I say get the perf improvement suggested above in, try to answer some of the
questions we've raised, and get this into the AVIARY_1_0_1_...BRANCH for
respins so we can unleash some testers on it.  Thanks yet again,

/be
Comment 34 Brendan Eich [:brendan] 2005-04-07 20:46:16 PDT
> Idea: test cx->fp->scopeChain == wrapper->mMaybeScope->mGlobalJSObject (pass
> wrapper as well as obj) and return false early if true.  I'm not sure when
> mMaybeScope is valid (it's in a union with mMaybeProto), so if this does not
> work out (we'd want an inline helper in xpcprivate.h to hide the ugly details),
> you could always use JS_GetParent(cx, obj).

So mMaybeScope/mMaybeProto are discriminated by the low bit, a pointer tag bit.
So all we need for the fast-path condition is

    cx->fp->scopeChain == wrapper->GetScope()->GetGlobalJSObject()

I recommend including jscntxt.h and jsinterp.h (already have the latter) and not
messing around with jsdbgapi.h function calls for the left operand of ==.

/be
Comment 35 Brendan Eich [:brendan] 2005-04-07 20:52:46 PDT
So bz's bug 281988 comment 46 raises the point he, jst and then latecoming I
talked about on IRC: gotta do something to prevent content from using
__defineGetter__ to replace 0, 1, etc. on a DOM element that's array-like.

Or let it do that but catch it at the XPC_WN_JSOp_... level.  We must prove that
there's no way a JSObjectOps.lookupProperty result could return a
JSScopeProperty pointer that gets into the property cache, for such a replaced
index getter.  If that happens, and if chrome uses such indexing, it could end
up calling the getter with none of the code in this patch in the path.

/be
Comment 36 Johnny Stenback (:jst, jst@mozilla.com) 2005-04-08 00:05:23 PDT
Created attachment 180023 [details] [diff] [review]
Cleaned up version of the above.
Comment 37 timeless 2005-04-08 08:24:45 PDT
*** Bug 289405 has been marked as a duplicate of this bug. ***
Comment 38 Brendan Eich [:brendan] 2005-04-08 10:06:47 PDT
Created attachment 180068 [details] [diff] [review]
patch to jst's last patch to handle user-defined getters and setters

jst: I #if 0'd your foo local set to the ccx's security manager -- not sure
what that was for.  The main thing here is to finish writing the code in
XPC_WN_Safe_GetterSetterThunkNative that printfs "BLORT!!!\n" here.  I see that
printf on testcase #3 in this bug, as expected.

/be
Comment 39 Brendan Eich [:brendan] 2005-04-08 11:36:53 PDT
Comment on attachment 180068 [details] [diff] [review]
patch to jst's last patch to handle user-defined getters and setters

Almost right, but it's not safe to set mutable properties on the safe_gsobj,
because of __lookupGetter__ and __lookupSetter__.

/be
Comment 40 Brendan Eich [:brendan] 2005-04-08 12:27:08 PDT
Created attachment 180091 [details] [diff] [review]
new patch to test

Getting closer.

/be
Comment 41 Johnny Stenback (:jst, jst@mozilla.com) 2005-04-08 13:52:35 PDT
Created attachment 180102 [details] [diff] [review]
Brendan's last patch with logic to call the 'real' getter/setter, and GetProperty() hook, if wanted by the wrapper
Comment 42 Johnny Stenback (:jst, jst@mozilla.com) 2005-04-08 14:00:14 PDT
Created attachment 180104 [details] [diff] [review]
Brendan's last patch with logic to call the 'real' getter/setter, and Get/SetProperty() hook, if wanted by the wrapper

Same thing, but with code to call into the SetProperty() wrapper hook as well.
Comment 43 Christopher Aillon (sabbatical, not receiving bugmail) 2005-04-08 14:21:14 PDT
Comment on attachment 179896 [details] [diff] [review]
keep eval and exec from being called across any trust domain boundary

a little late, but r=caillon for the caps changes, fwiw.
Comment 44 Johnny Stenback (:jst, jst@mozilla.com) 2005-04-08 14:58:03 PDT
Created attachment 180111 [details] [diff] [review]
Don't throw exceptions if we're asked to call a setter for a readonly prop (jus assert for now).
Comment 45 Brendan Eich [:brendan] 2005-04-08 15:02:33 PDT
Comment on attachment 180111 [details] [diff] [review]
Don't throw exceptions if we're asked to call a setter for a readonly prop (jus assert for now).

I wrote too much of this to review, but I like what jst did!  a=me in advance,
on behalf of drivers for the branch.

/be
Comment 46 Brendan Eich [:brendan] 2005-04-08 15:27:01 PDT
Created attachment 180116 [details] [diff] [review]
diff -w version of last patch
Comment 47 Mike Shaver (:shaver -- probably not reading bugmail closely) 2005-04-08 15:36:07 PDT
Comment on attachment 180111 [details] [diff] [review]
Don't throw exceptions if we're asked to call a setter for a readonly prop (jus assert for now).

Let's fuse the js_InternalGetOrSet call in GetUnshadowedMemberValue like we do
in XPC_WN_Safe_GetterSetterThunkNative.  (And tweak the assertion
appropriately, natch.)

Also, would it be worthwhile to stick id and unsafe_gsobj in properties named
by tinyids, or perhaps in private slots, to speed access?

r=shaver, though; I can't find anything that looks wrong to me, in there.
Comment 48 Brendan Eich [:brendan] 2005-04-08 15:40:16 PDT
> Also, would it be worthwhile to stick id and unsafe_gsobj in properties named
> by tinyids, or perhaps in private slots, to speed access?

Tinyids don't speed access, they allow a shared (class) getter or setter to
handle many similar special cases of getting or setting.  Anyway, we're not
changing the Function object's class for this.

Private slots would be cool, but it's not worth the trouble here.

/be
Comment 49 Johnny Stenback (:jst, jst@mozilla.com) 2005-04-08 16:17:15 PDT
Created attachment 180121 [details] [diff] [review]
Same thing with shaver's suggestion
Comment 50 Daniel Veditz [:dveditz] 2005-04-08 20:31:16 PDT
The 2005-04-08-18 build is working great. Extensions that were originally broken
by the uber-patch in bug 281988 now work again (Chatzilla and AdBlock, mainly),
and DOMInspector works and can see all JS properties except any that explicitly
try to hide a native property. Blocks the exploit testcases.

Whoo hoo!
Comment 51 moz_bug_r_a4 2005-04-08 23:31:43 PDT
I've tested in Firefox/1.0.3 2005-04-08-18 on Win XP.

The following code causes crash.

  document.body.__defineGetter__('localName', new Script());

But, the following code doesn't.

  document.body.__proto__.__defineGetter__('localName', new Script());
Comment 52 moz_bug_r_a4 2005-04-08 23:36:59 PDT
Created attachment 180149 [details]
testcase

crash when click on the second link
Comment 53 Brendan Eich [:brendan] 2005-04-09 10:00:26 PDT
Created attachment 180186 [details] [diff] [review]
fix crash on callable non-function object getter/setter

Trivial.  We don't need unsafe_gsfun, it was just for propagating the name of
the user-defined getter or setter to the thunk, but that's not necessary.  It's
safe and simpler to make the thunk anonymous.

/be
Comment 54 Johnny Stenback (:jst, jst@mozilla.com) 2005-04-09 10:03:54 PDT
Comment on attachment 180186 [details] [diff] [review]
fix crash on callable non-function object getter/setter

r+sr=jst
Comment 55 Brendan Eich [:brendan] 2005-04-09 10:26:55 PDT
Comment on attachment 180186 [details] [diff] [review]
fix crash on callable non-function object getter/setter

I checked this into the 1.0.3 and 1.7 branches.

/be
Comment 56 moz_bug_r_a4 2005-04-09 10:47:28 PDT
I've tested in Firefox/1.0.3 2005-04-09-06 on Win XP.

It is still unsafe to use |__proto__| in Chrome JS. For instance,
|content.getSelection| is the real native function, but,
|content.__proto__.getSelection| can be the function created by a web page.

Thus, the vulnerability represented in comment 23 is still alive. And it seems
that the fix for comment 11 has been reverted, thus, Bug 289075 has become
exploitable again.

And, if there is no real getter/setter, the user-defined getter/setter will be used.
(Is this behavior benefited by unsafe_gsfun? if so, already fixed.)
Thus, the following code can exploit.

  // Body doesn't have |rel| property
  document.body.__defineGetter__("rel", function() {
    return { toString : new Script(code) };
  });

  var event = document.createEvent("Events");
  event.initEvent("DOMLinkAdded", true, true);
  document.body.dispatchEvent(event);
Comment 57 moz_bug_r_a4 2005-04-09 10:51:44 PDT
Created attachment 180189 [details]
this creates pref "_test_.289074"
Comment 58 moz_bug_r_a4 2005-04-09 10:53:51 PDT
Created attachment 180190 [details]
this is the other version of Bug 289075
Comment 59 moz_bug_r_a4 2005-04-09 10:57:59 PDT
Created attachment 180192 [details]
DOMLinkAdded Event from document.body
Comment 60 Brendan Eich [:brendan] 2005-04-09 12:14:55 PDT
Thanks moz_bug_r_a4, you deserve more bug bounties :-).

jst and I almost saw the completely undefended state of XPConnect wrapped native
prototype objects, as noted in this comment in our recent patch:

    // XXXbe and XXXjst
    // Can't use THROW_AND_RETURN_IF_BAD_WRAPPER here since this
    // hook gets called from what looks like prototype wrappers,
    // and for those JSObjects we don't find a wrapper (when
    // called through someobj.__proto__.somefunction.call). Let
    // those calls fall through to the stub for now.

We suck for not figuring this out fully and fixing it, but we'll do that now.

The other missing defense, which dbaron fully foresaw days ago, is chrome using
properties not pre-defined by XPConnect at all (via XPCOM interface reflection
or JS interface implementation followed by successful QueryInterface).  That's
the target.rel exploit, in part.

The final part of the target.rel attack is that chrome trusts content-generated
events, and jst has a patch to fix that, but we think it's 1.8b2 material only
right now.

/be
Comment 61 Brendan Eich [:brendan] 2005-04-09 17:48:39 PDT
Created attachment 180234 [details] [diff] [review]
remove __proto__ abusage in chrome and fail such attempts in xpconnect

This is for applying.  Diff -w version next, for reviewing.

/be
Comment 62 Brendan Eich [:brendan] 2005-04-09 17:53:52 PDT
Created attachment 180235 [details] [diff] [review]
diff -w version of last patch

Besides ripping out the vulnerable *.__proto__.*.call(...) pattern from Firefox
chrome JS, this breaks any such usage at the XPConnect level by failing chrome
gets and sets on content wrapped native prototypes with XPC_BAD_OP_ON_WN_PROTO.


We still need to do something about chrome gets and sets of content properties
that do not shadow an xpconnect member.  And about chrome trusting
untrustworthy content-generated events.

/be
Comment 63 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-04-09 17:57:24 PDT
Comment on attachment 180235 [details] [diff] [review]
diff -w version of last patch

>Index: browser/base/content/browser.js

This also needs to happen in:
  xpfe/communicator/resources/content/nsContextMenu.js
  mail/base/content/nsContextMenu.js
Comment 64 Johnny Stenback (:jst, jst@mozilla.com) 2005-04-09 18:07:56 PDT
Comment on attachment 180235 [details] [diff] [review]
diff -w version of last patch

+class XPCWrappedNativeOrProto
+{
[...]
+    static XPCWrappedNativeOrProto
+    GetWrappedNativeOfJSObject(JSContext *cx, JSObject *obj,
+				JSObject *funobj = nsnull)
+    {
+	 XPCWrappedNative* wrapper =
+	     XPCWrappedNative::GetWrappedNativeOfJSObject(cx, obj, funobj);
+
+	 // Inline and specialize THROW_AND_RETURN_IF_BAD_WRAPPER(cx, wrapper);
+	 if(!wrapper)
+	     Throw(NS_ERROR_XPC_BAD_OP_ON_WN_PROTO, cx);
+	 if(!wrapper->IsValid())

That doesn't look right. Missing return?

The rest looks good at first glance, but I need to read it again before
stamping my r= on it.
Comment 65 Brendan Eich [:brendan] 2005-04-09 18:30:08 PDT
jst: thanks, forgot an else (wanted to let the common return wrapper; at the
bottom handle the null case).  Fixed.

I wonder if I should also amend the patch to avoid thunking when chrome script
defines a getter or a setter.  That counts on the misnomer
IsCrossTrustDomainCall really meaning IsChromeToContentCall (or GetOrSet). 
Thoughts?

/be
Comment 66 Johnny Stenback (:jst, jst@mozilla.com) 2005-04-09 21:01:07 PDT
(In reply to comment #65)
> I wonder if I should also amend the patch to avoid thunking when chrome script
> defines a getter or a setter.  That counts on the misnomer
> IsCrossTrustDomainCall really meaning IsChromeToContentCall (or GetOrSet). 
> Thoughts?

Seems reasonable. I don't see a point in trying to protect antything in that
case, and we'd save ourselves a bit on performance when calling such
getters/setters (but loose a bit when defining, I guess).
Comment 67 Brendan Eich [:brendan] 2005-04-09 22:15:36 PDT
(In reply to comment #63)
> (From update of attachment 180235 [details] [diff] [review] [edit])
> >Index: browser/base/content/browser.js
> 
> This also needs to happen in:
>   xpfe/communicator/resources/content/nsContextMenu.js
>   mail/base/content/nsContextMenu.js

Someone else will have to attach a patch for these -- I don't have the trees for
1.7 branch SeaMonkey and aviary 1.0.1 branch Thunderbird pulled.

Following up on jst's comment, new patch in a minute.

/be
Comment 68 Brendan Eich [:brendan] 2005-04-09 22:35:04 PDT
Created attachment 180253 [details] [diff] [review]
remove/prevent __proto__ abusage, v2 (for applying)

diff -w next.

/be
Comment 69 Brendan Eich [:brendan] 2005-04-09 22:38:04 PDT
Created attachment 180254 [details] [diff] [review]
diff -w version of last patch (for reviewing)

Interdiff'ing may help.  Main change is to avoid wrapping a script-defined
getter or setter with the safe thunk if the caller is chrome/native.  This
happens, e.g. when defining _content at startup.

/be
Comment 70 Brendan Eich [:brendan] 2005-04-09 22:45:20 PDT
Comment on attachment 180254 [details] [diff] [review]
diff -w version of last patch (for reviewing)

Pretend this has the following comment at the top of IsCrossTrustDomainCall:

    // Provided script is active on cx, a scope chain that's the same as the
    // global object in the wrappers scope means we're not in a cross-trust-
    // domain call.

/be
Comment 71 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2005-04-09 23:03:27 PDT
Comment on attachment 180254 [details] [diff] [review]
diff -w version of last patch (for reviewing)

>-        if(!wrapper->IsValid())
>-            return Throw(NS_ERROR_XPC_HAS_BEEN_SHUTDOWN, cx);
>-
>         JSBool foundMember;
>-        if(!GetOrSetUnshadowedMemberValue(cx, wrapper, idval, 0, nsnull, vp,
>-                                          &foundMember))
>+        if(!GetOrSetUnshadowedMemberValue(cx, wrapper_or_proto, idval,
>+                                          0, nsnull, vp, &foundMember))
>             return JS_FALSE;

Should the IsValid check move into GetOrSetUnshadowedMemberValue, or is it
unneeded?
Comment 72 Brendan Eich [:brendan] 2005-04-09 23:07:18 PDT
(In reply to comment #71)
> (From update of attachment 180254 [details] [diff] [review] [edit])
> >-        if(!wrapper->IsValid())
> >-            return Throw(NS_ERROR_XPC_HAS_BEEN_SHUTDOWN, cx);
> >-
> >         JSBool foundMember;
> >-        if(!GetOrSetUnshadowedMemberValue(cx, wrapper, idval, 0, nsnull, vp,
> >-                                          &foundMember))
> >+        if(!GetOrSetUnshadowedMemberValue(cx, wrapper_or_proto, idval,
> >+                                          0, nsnull, vp, &foundMember))
> >             return JS_FALSE;
> 
> Should the IsValid check move into GetOrSetUnshadowedMemberValue, or is it
> unneeded?

It's in

+    XPCWrappedNativeOrProto wrapper_or_proto =
+        XPCWrappedNativeOrProto::GetWrappedNativeOfJSObject(cx, obj);

just above the context you showed.

/be
Comment 73 Johnny Stenback (:jst, jst@mozilla.com) 2005-04-10 00:13:27 PDT
Comment on attachment 180254 [details] [diff] [review]
diff -w version of last patch (for reviewing)

sr=jst.
Comment 74 Johnny Stenback (:jst, jst@mozilla.com) 2005-04-11 14:22:27 PDT
Created attachment 180413 [details] [diff] [review]
Fix bugzilla regression

Our IsCrossTrustDomainCall() check was busted and made us think that we were
dealing with chrome calling content when content was calling itself. Silly
mistake. While I was at it I renamed the function to IsSystemCallingContent()
to keep things a bit more sane.
Comment 75 Daniel Veditz [:dveditz] 2005-04-11 14:45:19 PDT
Comment on attachment 180413 [details] [diff] [review]
Fix bugzilla regression

r=dveditz

This fixes the problems reported in bug 289945
Comment 76 Chase Phillips 2005-04-11 14:50:30 PDT
Comment on attachment 180413 [details] [diff] [review]
Fix bugzilla regression

a=chase/drivers for the trunk and aviary1.0.1 and 1.7 branches pending sr from
brendan
Comment 77 Brendan Eich [:brendan] 2005-04-11 14:52:21 PDT
Comment on attachment 180413 [details] [diff] [review]
Fix bugzilla regression

Duh, shoulda seen that when I was hacking.  Good to fix the name too.  sr=me!

/be
Comment 78 Johnny Stenback (:jst, jst@mozilla.com) 2005-04-11 15:13:51 PDT
Latest patch landed on the aviary and 1.7 branches.
Comment 79 Brendan Eich [:brendan] 2005-04-11 17:55:01 PDT
Created attachment 180435 [details] [diff] [review]
keep eval and exec from being called across any trust domain boundary, v2

This is much better than the kludge it obsoletes.  It's still a pair of special
cases, but that's what we want for defense in depth: checks that seem redundant
but will save us if someone finds a way to sick eval or a Script object
somewhere that chrome will call.

/be
Comment 80 Johnny Stenback (:jst, jst@mozilla.com) 2005-04-11 18:21:36 PDT
Comment on attachment 180435 [details] [diff] [review]
keep eval and exec from being called across any trust domain boundary, v2

r+sr=jst, others feel free to review as well.
Comment 81 Chase Phillips 2005-04-11 18:25:48 PDT
Comment on attachment 180435 [details] [diff] [review]
keep eval and exec from being called across any trust domain boundary, v2

a=chase/drivers for trunk, aviary1.0.1 and 1.7 branches
Comment 82 Brendan Eich [:brendan] 2005-04-11 18:37:03 PDT
Comment on attachment 180435 [details] [diff] [review]
keep eval and exec from being called across any trust domain boundary, v2

I checked this into both branches and the trunk.

Is there anything left to patch in this bug or its spin-off bug?

/be
Comment 83 Mike Shaver (:shaver -- probably not reading bugmail closely) 2005-04-11 19:08:13 PDT
Comment on attachment 180435 [details] [diff] [review]
keep eval and exec from being called across any trust domain boundary, v2

r+=shaver
Comment 84 Johnny Stenback (:jst, jst@mozilla.com) 2005-04-12 17:36:50 PDT
For the record, the patches in this bug caused regression bug 289949, sporadic
crashes (thought to be related to the linkification extension). The problem was
missing mark management code when cloning xpconnect wrapped function objects.
See the bug and patch for details.
Comment 85 moz_bug_r_a4 2005-04-14 09:09:06 PDT
See Bug 290324, its problem is similar to target.rel attack.
Comment 86 Asa Dotzler [:asa] 2005-04-17 11:33:37 PDT
Can we resolve this now?
Comment 87 Brendan Eich [:brendan] 2005-04-17 11:45:53 PDT
Asa: we haven't landed needed fixes from the branches on the trunk, and we have
not landed one patch on the branches that I've been developing in bug 290324. 
The approach used with this bug, bug 290324, and bug 289083 needs to be
reevaluated before porting the several branch patches to the trunk.

I'd like to use bug 281988 to do that reevaluation and likely re-implementation.
 It needs to get done for 1.8b2.  This bug and bug 289083 can be closed as far
as I am concerned, but it would be best to do that *after* we've fixed the trunk.

/be
Comment 88 Lloyd D Budd 2005-04-21 00:25:57 PDT
You may want to consider flagging patch :

remove/prevent __proto__ abusage, v2 (for applying)
patch
2005-04-09 22:35 PDT
22.10 KB
https://bugzilla.mozilla.org/attachment.cgi?id=180253

with
approval-aviary1.0.3+
as it seems it is in FF 1.0.3 .
Comment 89 moz_bug_r_a4 2005-04-22 10:24:52 PDT
Is it too late? 

MFSA 2005-41 (http://www.mozilla.org/security/announce/mfsa2005-41.html) says:

  Workaround: Disable Javascript

But, Disabling JavaScript is not a workaround for this bug and bug 289083.
A page using XBL can exploit even though JavaScript is disabled.
Comment 90 moz_bug_r_a4 2005-04-22 10:26:56 PDT
Created attachment 181541 [details]
xbl file (eval)
Comment 91 moz_bug_r_a4 2005-04-22 10:30:33 PDT
Created attachment 181544 [details]
XBL version of bug 289078 attachment 179677 [details] (eval)

for Firefox 1.0.2, Mozilla 1.7.6
Comment 92 moz_bug_r_a4 2005-04-22 10:32:28 PDT
Created attachment 181545 [details]
xbl file (Script.exec)
Comment 93 moz_bug_r_a4 2005-04-22 10:35:09 PDT
Created attachment 181546 [details]
XBL version of bug 289083 attachment 179682 [details] (Script.exec)

for Firefox 1.0.2, Mozilla 1.7.6
Comment 94 Daniel Veditz [:dveditz] 2005-05-18 06:36:44 PDT
Created attachment 183907 [details] [diff] [review]
trunk version

This is a port of these patches to the trunk (including various branch
regression fixes) in case we run out of time and want to land this temporarily
instead of waiting for bug 281988 (which is nearly there anyway).
Comment 95 Caleb 2005-06-02 12:55:15 PDT
This one was fixed by bug 281988. I cannot reproduce any of the testcases.
Comment 96 Asa Dotzler [:asa] 2005-06-14 11:58:49 PDT
JST, can you resolve this if it is fixed on the trunk? Thanks.
Comment 97 chris hofmann 2005-06-21 11:58:23 PDT
marking fixed.  moz_bug_r_a4@yahoo.com  can you verify that the trunk is ok for
all problems related to this bug?
Comment 98 moz_bug_r_a4 2005-06-22 09:55:11 PDT
All of the testcases (except for Bug 290324) can no longer exploit on the trunk.
Is Bug 290324 included in "all problems related to this bug"?

see Bug 290324 comment 37.
Comment 99 Brendan Eich [:brendan] 2005-06-22 10:58:17 PDT
(In reply to comment #98)
> All of the testcases (except for Bug 290324) can no longer exploit on the trunk.
> Is Bug 290324 included in "all problems related to this bug"?
> 
> see Bug 290324 comment 37.

Please see Bug 290324 comment 39.

/be

Comment 100 moz_bug_r_a4 2005-06-22 22:00:58 PDT
I've tested on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2)
Gecko/20050622 Firefox/1.0+

The trunk is ok for all problems related to this bug.

Note You need to log in before you can comment on or make changes to this bug.