330773 - DOM property setter override (remote compromise)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Paul Nickerson]

Messing around, I found that running HTMLDocument.__proto__.__defineSetter__("prototype",function(){alert(location.href)}) will cause alert(location.href) to be executed in the context of the current page periodically (for example, when the page is right-clicked or when view page info is clicked). I can't think of a way to exploit this, but it doesn't seem like proper behavior, so I'm just throwing it out there to see if anyone with more experience in this area can make something of it.

Also, I was wondering what has been done with __defineGetter__ overriding DOM properties. Has this type of attack been blocked completely or just specific holes that use it?

Comment 1

[Jesse Ruderman]

If I do

javascript:HTMLDocument.__proto__.__defineSetter__("prototype",function(){try{d.d.d}catch(e){alert(e.stack)}})

on http://www.squarefree.com/start/ and then bring up a context menu or Page Info, I get a bunch of alerts suggesting that things like getElementsByTagName are calling that setter.  jst, any idea why this is happening, why chrome isn't protected from these content-defined setters, and/or how bad this is?

Comment 2

[moz_bug_r_a4]

Thanks for ccing me, Paul.

When the following conditions are satisfied, an attacker can run arbitrary
code.

  1. A caller of the |prototype| setter has chrome privilege.
  2. Content can control the return value of toString() method of a object that
     is passed to the |prototype| setter.

Also, Object.prototype can be used instead of HTMLDocument.__proto__.

Comment 3

[moz_bug_r_a4]

Attachment: testcase1 - arbitrary code execution using Node.__proto__

Comment 4

[moz_bug_r_a4]

Attachment: testcase2 - arbitrary code execution using Object.prototype

Comment 5

[Paul Nickerson]

(In reply to comment #4)
> Created an attachment (id=215479) [edit]
> testcase2 - arbitrary code execution using Object.prototype
> 

Fantastic work, moz_bug_r_a4!!!

Cheers,
Paul

Comment 6

[moz_bug_r_a4]

Attachment: testcase3 - arbitrary code execution

This works on the trunk, fx 1.5.0.x, fx 1.0.x and moz 1.7.x.
(testcase1 and testcase2 don't work on fx 1.0.x and moz 1.7.x.)

Comment 7

[Daniel Veditz [:dveditz]]

We have bits-in-hand for 1.0.8 and 1.5.0.2, so without a patch this goes in the next set of releases. We may reconsider if we get a small, safe patch very quickly so please ask for approval on it.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Hack

I think this is wrong on at least one or two levels, but it fixes the testcases in this bug by bypassing the content-defined setter for the "prototype" property, all XPCNativeWrapper-style

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #0)
> Also, I was wondering what has been done with __defineGetter__ overriding DOM
> properties. Has this type of attack been blocked completely or just specific
> holes that use it?

In general, any code dealing with DOM nodes (or content nodes of any kind, really) that has chrome privileges will be dealing with XPCNativeWrappers, which don't call user-defined getters, and go straight to the source as it were, and use only IDL-defined properties.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix, part 1

This is a partial fix that fixes the testcases in this bug, but jst and I are trying to find a better and more general solution. I'll explain what's going on in a seperate comment.

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 215736 [details] [diff] [review]
Fix, part 1

r+sr=jst

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

I check attachment 215736 [details] [diff] [review] into the trunk, but I'm leaving this bug open for some additional investigation.

What's happening here is that we're trying to define the .prototype property on a class that's being resolved. In attachment 215551 [details]'s case, this happens to be HTMLParagraphElement. As "expected", we find the setter for the .prototype property on object.prototype and call it. An important aside is that Components.utils.lookupMethod returns a function whose |this| is bound (i.e., not dynamic) so that when setTimeout is called as a setter, its |this| is correct. setTimeout binds its timeouts with the principals of whoever is calling it, so it finds the chrome principals on the stack and runs the given code with those principals. The fix that I just checked in makes us use JS_DefineProperty instead of JS_SetProperty, so we define the .prototype property with no getter or setter, and therefore don't run the evil setter.

jst and I want to look into fixing this attack, even with the setter defined.

Comment 13

[Jesse Ruderman]

Why didn't the fix for bug 223041 prevent setTimeout from being used in this exploit?

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #13)
> Why didn't the fix for bug 223041 prevent setTimeout from being used in this
> exploit?

I don't see why it would have -- that bug never actually touched setTimout.

Comment 15

[Jesse Ruderman]

Hmm, doesn't leaving out setTimeout negate the benefit of that fix?  Do you know why setTimeout was left out?

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 215736 [details] [diff] [review]
Fix, part 1

Should the prototype property have JSPROP_READONLY and JSPROP_PERMANENT too?  That would be the case for built-in ECMA-262 classes.  Blame me for any deviation for the DOM level 0 (which I actually forget! someone please remind me).

/be

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

That isn't a bad idea, though I think it's jst's call. I was aiming for compat with the existing code with my patch.

Comment 18

[Johnny Stenback (:jst)]

(In reply to comment #16)
> Should the prototype property have JSPROP_READONLY and JSPROP_PERMANENT too? 

I'm all for that!

Comment 19

[Brendan Eich [:brendan]]

I don't think JSPROP_ENUMERATE was used in the old days or now.  All native class constructors' 'prototype' properties have attributes JSPROP_READONLY | JSPROP_PERMAMENT (ECMA-262 Ed. 3 would say, {DontEnum, ReadOnly, DontDelete}).

/be

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed setTimeout belt-and-braces

Unfortunately for me, my build isn't reproducing the exploit that uses setTimeout right now, but this is something like what I had in mind. It lets content setTimeout on chrome windows (and run in content privs) and it lets same-origin sites do the same, but chrome gets downgraded principals if it tries to setTimeout on a content window.

Comment 21

[Paul Nickerson]

Attachment: testcase 4

make sure to try this testcase with the patches. it has two changes: first, it sets the prototype to null (so it will be reset); second, it uses setInterval instead of setTimeout.

Comment 22

[Paul Nickerson]

Comment on attachment 215937 [details]
testcase 4

btw, setInterval will cause the browser to go into a continuous loop can only be stopped by killing the process

Comment 23

[Paul Nickerson]

Attachment: testcase 4 (update)

sorry, i was missing a quote in the script on the last testcase. btw, is it possible to edit html attachments without having to reupload them?

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: More JS_Set(UC)Property nukage

I think these are the rest of the JS_Set(UC)Property calls that need to be JS_DefineProperty calls instead.

Comment 25

[Johnny Stenback (:jst)]

Comment on attachment 215939 [details] [diff] [review]
More JS_Set(UC)Property nukage

- In nsDOMClassInfo::ResolveConstructor():

+    if (!::JS_DefineUCProperty(cx, obj, ::JS_GetStringChars(str),
+                               ::JS_GetStringLength(str), val, NULL, NULL,
+                               JSPROP_PERMANENT | JSPROP_READONLY)) {

This should have JSPROP_ENUMERATE as well since the old code made the constructors enumerable.

- In nsWindowSH::GlobalResolve():

     if (!::JS_DefineProperty(cx, class_obj, "prototype", v, NULL, NULL,
-                             JSPROP_ENUMERATE)) {
+                             JSPROP_PERMANENT | JSPROP_READONLY)) {

Why is this not enumerable any more, ECMA?

- In nsHTMLDocumentSH::DocumentAllHelperNewResolve():

-      ::JS_SetProperty(cx, helper, "all", &v);
+      if (!::JS_DefineProperty(cx, helper, "all", v, NULL, NULL,
+                               JSPROP_READONLY)) {

JSPROP_ENUMERATE here too, no need to change the current behaviour, at least not in this patch.

r=jst with that looked into.

Comment 26

[Brendan Eich [:brendan]]

(In reply to comment #25)
> (From update of attachment 215939 [details] [diff] [review] [edit])
> - In nsDOMClassInfo::ResolveConstructor():
> 
> +    if (!::JS_DefineUCProperty(cx, obj, ::JS_GetStringChars(str),
> +                               ::JS_GetStringLength(str), val, NULL, NULL,
> +                               JSPROP_PERMANENT | JSPROP_READONLY)) {
> 
> This should have JSPROP_ENUMERATE as well since the old code made the
> constructors enumerable.

Yeah, I made DOM class constructors enumerable, back in Netscape 2.  Not sure why -- to let people find 'em with for-in?  To distinguish them from the core language ctors, which are not enumerable?  Memory lane is closed.

> - In nsWindowSH::GlobalResolve():
> 
>      if (!::JS_DefineProperty(cx, class_obj, "prototype", v, NULL, NULL,
> -                             JSPROP_ENUMERATE)) {
> +                             JSPROP_PERMANENT | JSPROP_READONLY)) {
> 
> Why is this not enumerable any more, ECMA?

Yes, and Netscape 2.  See http://lxr.mozilla.org/classic/.

> - In nsHTMLDocumentSH::DocumentAllHelperNewResolve():
> 
> -      ::JS_SetProperty(cx, helper, "all", &v);
> +      if (!::JS_DefineProperty(cx, helper, "all", v, NULL, NULL,
> +                               JSPROP_READONLY)) {
> 
> JSPROP_ENUMERATE here too, no need to change the current behaviour, at least
> not in this patch.

What does IE do?

/be

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 215932 [details] [diff] [review]
Proposed setTimeout belt-and-braces

Rename "prin" to "ourPrincipal" and sr=bzbarsky.

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #26)
> What does IE do?

It's enumerable in IE. I'll attach a new patch tomorrow.

Comment 29

[Blake Kaplan (:mrbkap) (inactive)]

I checked attachment 215932 [details] [diff] [review] into the trunk.

Comment 30

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: JS_Set(UC)Property nukage, v2

I think this patch uses the correct bits.

Comment 31

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Followup

Ugh, we _really_ need to figure out how to use nsRefPtr with nsTimeouts...

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 216144 [details] [diff] [review]
Followup

Verily.  :(

Comment 33

[Blake Kaplan (:mrbkap) (inactive)]

Attachment 216144 [details] [diff] has been checked in.

Comment 34

[Johnny Stenback (:jst)]

Comment on attachment 216133 [details] [diff] [review]
JS_Set(UC)Property nukage, v2

r=jst fwiw.

Comment 35

[Brendan Eich [:brendan]]

Comment on attachment 216133 [details] [diff] [review]
JS_Set(UC)Property nukage, v2

>+      if (!::JS_DefineProperty(cx, helper, "all", JSVAL_VOID, nsnull, nsnull,
>+                               JSPROP_ENUMERATE | JSPROP_READONLY)) {

READONLY without PERMANENT is not all that useful, since someone could delete document.all and assign a new .all that was different.  But maybe that's what is intended, for backward compatibility (with some pre-Firefox DHTML lib that attempts to emulate document.all in JS?).  If so, however, READONLY is problematic.

So I think you should use just ENUMERATE here.  sr=me with that.

/be

Comment 36

[Blake Kaplan (:mrbkap) (inactive)]

Final fix checked into trunk. I'll make a rollup patch for the 1.8 branches shortly.

Comment 37

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Roll-up patch for branches

'cvs up -j' is my friend.

Comment 38

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the 1.8 branch.

Comment 39

[Daniel Veditz [:dveditz]]

Comment on attachment 217039 [details] [diff] [review]
Roll-up patch for branches

approved for 1.8.0 branch, a=dveditz for drivers

Comment 40

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the 1.8.0 branch.

Comment 41

[Bob Clary [:bc] (inactive)]

added to the security tests on the qa farm.

Comment 42

[Tracy Walker [:tracy]]

verified with Windows and Mac Ffx 1.5.0.4 builds from 20060508

Comment 43

[Alexander Sack]

Attachment: 1.0.x try

Comment 44

[Bob Clary [:bc] (inactive)]

ff2b2 verified 1.8 fixed windows/linux