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

RESOLVED FIXED in mozilla1.8beta2

Status

()

Core
DOM
RESOLVED FIXED
13 years ago
6 years ago

People

(Reporter: moz_bug_r_a4, Assigned: jst)

Tracking

({fixed-aviary1.0.3, fixed1.7.7})

Trunk
mozilla1.8beta2
fixed-aviary1.0.3, fixed1.7.7
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.7.7 +
blocking-aviary1.0.3 +
blocking1.8b3 +
blocking-aviary1.5 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:fix] need trunk answers from brendan/jst (Monday))

Attachments

(26 attachments, 7 obsolete attachments)

606 bytes, text/html
Details
1.97 KB, patch
Details | Diff | Splinter Review
706 bytes, text/html
Details
452 bytes, text/html
Details
13.61 KB, patch
Details | Diff | Splinter Review
996 bytes, text/html
Details
980 bytes, text/html
Details
12.10 KB, patch
Details | Diff | Splinter Review
16.42 KB, patch
Details | Diff | Splinter Review
20.46 KB, patch
Details | Diff | Splinter Review
14.95 KB, patch
Details | Diff | Splinter Review
20.31 KB, patch
Details | Diff | Splinter Review
598 bytes, text/html
Details
1.53 KB, patch
jst
: review+
brendan
: approval1.7.7+
Details | Diff | Splinter Review
718 bytes, text/html
Details
975 bytes, text/html
Details
546 bytes, text/html
Details
22.10 KB, patch
Details | Diff | Splinter Review
21.15 KB, patch
jst
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
6.44 KB, patch
dveditz
: review+
brendan
: superreview+
Chase Phillips
: approval-aviary1.0.3+
Chase Phillips
: approval1.7.7+
Chase Phillips
: approval1.8b2+
Details | Diff | Splinter Review
3.27 KB, patch
jst
: review+
shaver
: review+
Chase Phillips
: approval-aviary1.0.3+
Chase Phillips
: approval1.7.7+
Chase Phillips
: approval1.8b2+
Details | Diff | Splinter Review
859 bytes, application/xml
Details
147 bytes, text/html
Details
592 bytes, application/xml
Details
141 bytes, text/html
Details
34.92 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

13 years ago
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:
(Reporter)

Comment 1

13 years ago
Created attachment 179674 [details]
testcase

Updated

13 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Blocks: 256195

Updated

13 years ago
Flags: blocking-aviary1.0.3?

Updated

13 years ago
Blocks: 289187
(Assignee)

Comment 2

13 years ago
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.
(Assignee)

Comment 3

13 years ago
*** Bug 289075 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 4

13 years ago
*** Bug 289078 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 5

13 years ago
*** Bug 289079 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 6

13 years ago
*** Bug 289081 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

13 years ago
Depends on: 289083
(Assignee)

Comment 7

13 years ago
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).
Attachment #179828 - Flags: superreview+
Attachment #179828 - Flags: review+
(Assignee)

Comment 8

13 years ago
Fix landed for 1.0.3
Keywords: fixed-aviary1.0.3
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.

Updated

13 years ago
Flags: blocking-aviary1.0.3? → blocking-aviary1.0.3+

Updated

13 years ago
Component: Security → DOM
Flags: review+
OS: Windows XP → All
Product: Firefox → Core
Hardware: PC → All
Target Milestone: --- → mozilla1.8beta2
Version: unspecified → Trunk
Want this in the 1.7 branch and the trunk, too.

/be
Flags: blocking1.8b2+
Flags: blocking1.7.7+
Attachment #179828 - Flags: approval1.7.7+
Attachment #179828 - Flags: approval-aviary1.0.3+
(Reporter)

Comment 11

13 years ago
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); })();";
(Reporter)

Comment 12

13 years ago
Created attachment 179874 [details]
testcase 2
(Reporter)

Comment 13

13 years ago
Created attachment 179875 [details]
testcase 3

This is for Bug 289083
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
Attachment #179896 - Flags: superreview?(jst)
Attachment #179896 - Flags: review?(caillon)
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
Attachment #179896 - Flags: approval1.7.7+
Attachment #179896 - Flags: approval-aviary1.0.3+
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?
moz_bug_r_a4: please contact me about the Mozilla Security Bug Bounty. You can
use my bugzilla email or security@mozilla.org
Attachment #179896 - Flags: review?(caillon) → review+
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
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
(Assignee)

Comment 20

13 years ago
Comment on attachment 179896 [details] [diff] [review]
keep eval and exec from being called across any trust domain boundary

sr=jst
Attachment #179896 - Flags: superreview?(jst) → superreview+
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
Keywords: fixed1.7.7
Whiteboard: [sg:fix]
Fixed on the trunk, too.

/be
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
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.
Created attachment 179946 [details]
creates preference fuckbill in prefs.js
Created attachment 179947 [details]
tries to install software but crashes
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
brendan,

are you saying that chrome will not be able to call at least netscape.preference
and InstallTrigger.* with full privilege?
I applaud your revisionism, indeed.
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
Status: REOPENED → ASSIGNED

Updated

13 years ago
Depends on: 281988

Comment 30

13 years ago
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.
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

Updated

13 years ago
Blocks: 223041
(Assignee)

Comment 32

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

Comment 36

13 years ago
Created attachment 180023 [details] [diff] [review]
Cleaned up version of the above.

Comment 37

13 years ago
*** Bug 289405 has been marked as a duplicate of this bug. ***
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 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
Attachment #180068 - Attachment is obsolete: true
Created attachment 180091 [details] [diff] [review]
new patch to test

Getting closer.

/be
(Assignee)

Comment 41

13 years ago
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
Attachment #180091 - Attachment is obsolete: true
(Assignee)

Comment 42

13 years ago
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.
Attachment #180102 - Attachment is obsolete: true
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.
(Assignee)

Comment 44

13 years ago
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).
Attachment #180104 - Attachment is obsolete: true
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
Attachment #180111 - Flags: approval-aviary1.0.3+
Created attachment 180116 [details] [diff] [review]
diff -w version of last patch
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.
> 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
(Assignee)

Comment 49

13 years ago
Created attachment 180121 [details] [diff] [review]
Same thing with shaver's suggestion
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!
Assignee: dveditz → jst
Status: ASSIGNED → NEW
Flags: blocking-aviary1.1+
(Reporter)

Comment 51

13 years ago
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());
(Reporter)

Comment 52

13 years ago
Created attachment 180149 [details]
testcase

crash when click on the second link
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

Updated

13 years ago
Attachment #180186 - Flags: review?(jst)
(Assignee)

Comment 54

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

r+sr=jst
Attachment #180186 - Flags: superreview+
Attachment #180186 - Flags: review?(jst)
Attachment #180186 - Flags: review+
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
Attachment #180186 - Flags: approval1.7.7+
Attachment #180186 - Flags: approval-aviary1.0.3+
(Reporter)

Comment 56

13 years ago
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);
(Reporter)

Comment 57

13 years ago
Created attachment 180189 [details]
this creates pref "_test_.289074"
(Reporter)

Comment 58

13 years ago
Created attachment 180190 [details]
this is the other version of Bug 289075
(Reporter)

Comment 59

13 years ago
Created attachment 180192 [details]
DOMLinkAdded Event from document.body
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
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
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
Attachment #180235 - Flags: superreview?(dbaron)
Attachment #180235 - Flags: review?(jst)
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
(Assignee)

Comment 64

13 years ago
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.
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
(Assignee)

Comment 66

13 years ago
(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).
(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

Updated

13 years ago
Attachment #180235 - Flags: superreview?(dbaron)
Attachment #180235 - Flags: review?(jst)
Created attachment 180253 [details] [diff] [review]
remove/prevent __proto__ abusage, v2 (for applying)

diff -w next.

/be
Attachment #180234 - Attachment is obsolete: true
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
Attachment #180235 - Attachment is obsolete: true
Attachment #180254 - Flags: superreview?(dbaron)
Attachment #180254 - Flags: review?(jst)
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 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?
Attachment #180254 - Flags: superreview?(dbaron) → superreview+
(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
(Assignee)

Comment 73

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

sr=jst.
Attachment #180254 - Flags: review?(jst) → review+
(Assignee)

Comment 74

13 years ago
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.
Attachment #180413 - Flags: superreview?(brendan)
Attachment #180413 - Flags: review?(dveditz)
Blocks: 289945
Comment on attachment 180413 [details] [diff] [review]
Fix bugzilla regression

r=dveditz

This fixes the problems reported in bug 289945
Attachment #180413 - Flags: review?(dveditz) → review+

Comment 76

13 years ago
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
Attachment #180413 - Flags: approval1.8b2+
Attachment #180413 - Flags: approval1.7.7+
Attachment #180413 - Flags: approval-aviary1.0.3+
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
Attachment #180413 - Flags: superreview?(brendan) → superreview+
(Assignee)

Comment 78

13 years ago
Latest patch landed on the aviary and 1.7 branches.
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
Attachment #179896 - Attachment is obsolete: true
Attachment #180435 - Flags: superreview?(jst)
Attachment #180435 - Flags: review?(shaver)
(Assignee)

Comment 80

13 years ago
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.
Attachment #180435 - Flags: superreview?(jst)
Attachment #180435 - Flags: superreview+
Attachment #180435 - Flags: review?(shaver)
Attachment #180435 - Flags: review+

Comment 81

13 years ago
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
Attachment #180435 - Flags: approval1.8b2+
Attachment #180435 - Flags: approval1.7.7+
Attachment #180435 - Flags: approval-aviary1.0.3+
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 on attachment 180435 [details] [diff] [review]
keep eval and exec from being called across any trust domain boundary, v2

r+=shaver
Attachment #180435 - Flags: review+
(Assignee)

Updated

13 years ago
Blocks: 289949
(Assignee)

Comment 84

13 years ago
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.
(Reporter)

Comment 85

13 years ago
See Bug 290324, its problem is similar to target.rel attack.
Whiteboard: [sg:fix] → [sg:fix] hold private until April 25,2005

Comment 86

13 years ago
Can we resolve this now?
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

13 years ago
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 .

Updated

13 years ago
Whiteboard: [sg:fix] hold private until April 25,2005 → [sg:fix] hold private until April 25,2005 need trunk answers from brendan/jst (Monday)
(Reporter)

Comment 89

13 years ago
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.
(Reporter)

Comment 90

13 years ago
Created attachment 181541 [details]
xbl file (eval)
(Reporter)

Comment 91

13 years ago
Created attachment 181544 [details]
XBL version of bug 289078 attachment 179677 [details] (eval)

for Firefox 1.0.2, Mozilla 1.7.6
(Reporter)

Comment 92

13 years ago
Created attachment 181545 [details]
xbl file (Script.exec)
(Reporter)

Comment 93

13 years ago
Created attachment 181546 [details]
XBL version of bug 289083 attachment 179682 [details] (Script.exec)

for Firefox 1.0.2, Mozilla 1.7.6

Updated

13 years ago
Flags: blocking1.8b2+ → blocking1.8b3+
Group: security
Whiteboard: [sg:fix] hold private until April 25,2005 need trunk answers from brendan/jst (Monday) → [sg:fix] need trunk answers from brendan/jst (Monday)
Depends on: 292591
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).
Blocks: 256197
No longer blocks: 256195

Comment 95

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

Comment 96

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

Comment 97

12 years ago
marking fixed.  moz_bug_r_a4@yahoo.com  can you verify that the trunk is ok for
all problems related to this bug?
Status: NEW → RESOLVED
Last Resolved: 13 years ago12 years ago
Resolution: --- → FIXED
(Reporter)

Comment 98

12 years ago
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.
(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

(Reporter)

Comment 100

12 years ago
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.

Updated

12 years ago
Flags: testcase+

Updated

11 years ago
Flags: in-testsuite+ → in-testsuite?
You need to log in before you can comment on or make changes to this bug.