289074 - security hole in markLinkVisited (Bug 217195) has not been fixed

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[moz_bug_r_a4]

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]

Attachment: testcase

Comment 2

[Johnny Stenback (:jst)]

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)]

*** Bug 289075 has been marked as a duplicate of this bug. ***

Comment 4

[Johnny Stenback (:jst)]

*** Bug 289078 has been marked as a duplicate of this bug. ***

Comment 5

[Johnny Stenback (:jst)]

*** Bug 289079 has been marked as a duplicate of this bug. ***

Comment 6

[Johnny Stenback (:jst)]

*** Bug 289081 has been marked as a duplicate of this bug. ***

Comment 7

[Johnny Stenback (:jst)]

Attachment: 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)]

Fix landed for 1.0.3

Comment 9

[sairuh (rarely reading bugmail)]

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]]

Want this in the 1.7 branch and the trunk, too.

/be

Comment 11

[moz_bug_r_a4]

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]

Attachment: testcase 2

Comment 13

[moz_bug_r_a4]

Attachment: testcase 3

This is for Bug 289083

Comment 14

[Brendan Eich [:brendan]]

Attachment: 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]]

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)]

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]]

moz_bug_r_a4: please contact me about the Mozilla Security Bug Bounty. You can
use my bugzilla email or [e-mail]

Comment 18

[Brendan Eich [:brendan]]

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]]

Attachment: 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)]

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]]

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]]

Fixed on the trunk, too.

/be

Comment 23

[georgi - hopefully not receiving bugspam]

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]

Attachment: creates preference fuckbill in prefs.js

Comment 25

[georgi - hopefully not receiving bugspam]

Attachment: tries to install software but crashes

Comment 26

[Brendan Eich [:brendan]]

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]

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)]

I applaud your revisionism, indeed.

Comment 29

[Brendan Eich [:brendan]]

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]

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]]

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)]

Attachment: 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]]

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]]

> 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]]

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)]

Attachment: Cleaned up version of the above.

Comment 37

[timeless]

*** Bug 289405 has been marked as a duplicate of this bug. ***

Comment 38

[Brendan Eich [:brendan]]

Attachment: 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]]

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]]

Attachment: new patch to test

Getting closer.

/be

Comment 41

[Johnny Stenback (:jst)]

Attachment: 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)]

Attachment: 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)]

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)]

Attachment: 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]]

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]]

Attachment: diff -w version of last patch

Comment 47

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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]]

> 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)]

Attachment: Same thing with shaver's suggestion

Comment 50

[Daniel Veditz [:dveditz]]

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]

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]

Attachment: testcase

crash when click on the second link

Comment 53

[Brendan Eich [:brendan]]

Attachment: 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)]

Comment on attachment 180186 [details] [diff] [review]
fix crash on callable non-function object getter/setter

r+sr=jst

Comment 55

[Brendan Eich [:brendan]]

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]

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]

Attachment: this creates pref "_test_.289074"

Comment 58

[moz_bug_r_a4]

Attachment: this is the other version of Bug 289075

Comment 59

[moz_bug_r_a4]

Attachment: DOMLinkAdded Event from document.body

Comment 60

[Brendan Eich [:brendan]]

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]]

Attachment: 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]]

Attachment: 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-4, no longer working on Mozilla)]

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)]

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]]

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)]

(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]]

(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]]

Attachment: remove/prevent __proto__ abusage, v2 (for applying)

diff -w next.

/be

Comment 69

[Brendan Eich [:brendan]]

Attachment: 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]]

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-4, no longer working on Mozilla)]

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]]

(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)]

Comment on attachment 180254 [details] [diff] [review]
diff -w version of last patch (for reviewing)

sr=jst.

Comment 74

[Johnny Stenback (:jst)]

Attachment: 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]]

Comment on attachment 180413 [details] [diff] [review]
Fix bugzilla regression

r=dveditz

This fixes the problems reported in bug 289945

Comment 76

[Chase Phillips]

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]]

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)]

Latest patch landed on the aviary and 1.7 branches.

Comment 79

[Brendan Eich [:brendan]]

Attachment: 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)]

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]

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]]

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)]

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)]

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]

See Bug 290324, its problem is similar to target.rel attack.

Comment 86

[Asa Dotzler [:asa]]

Can we resolve this now?

Comment 87

[Brendan Eich [:brendan]]

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]

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]

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]

Attachment: xbl file (eval)

Comment 91

[moz_bug_r_a4]

Attachment: XBL version of bug 289078 attachment 179677 (eval)

for Firefox 1.0.2, Mozilla 1.7.6

Comment 92

[moz_bug_r_a4]

Attachment: xbl file (Script.exec)

Comment 93

[moz_bug_r_a4]

Attachment: XBL version of bug 289083 attachment 179682 (Script.exec)

for Firefox 1.0.2, Mozilla 1.7.6

Comment 94

[Daniel Veditz [:dveditz]]

Attachment: 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]

This one was fixed by bug 281988. I cannot reproduce any of the testcases.

Comment 96

[Asa Dotzler [:asa]]

JST, can you resolve this if it is fixed on the trunk? Thanks.

Comment 97

[chris hofmann]

marking fixed.  [e-mail]  can you verify that the trunk is ok for
all problems related to this bug?

Comment 98

[moz_bug_r_a4]

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]]

(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]

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.