Last Comment Bug 330773 - DOM property setter override (remote compromise)
: DOM property setter override (remote compromise)
Status: RESOLVED FIXED
[sg:critical] [patch]
: testcase, verified1.8.0.4, verified1.8.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: P1 critical (vote)
: mozilla1.9alpha1
Assigned To: Blake Kaplan (:mrbkap) (please use needinfo!)
:
Mentors:
Depends on:
Blocks: 330774
  Show dependency treegraph
 
Reported: 2006-03-16 19:34 PST by Paul Nickerson
Modified: 2013-03-26 08:02 PDT (History)
9 users (show)
dveditz: blocking1.7.14+
dveditz: blocking‑aviary1.0.9+
dveditz: blocking1.8.1+
dveditz: blocking1.8.0.4+
choller: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase1 - arbitrary code execution using Node.__proto__ (297 bytes, text/html)
2006-03-18 01:18 PST, moz_bug_r_a4
no flags Details
testcase2 - arbitrary code execution using Object.prototype (299 bytes, text/html)
2006-03-18 01:20 PST, moz_bug_r_a4
no flags Details
testcase3 - arbitrary code execution (381 bytes, text/html)
2006-03-18 19:43 PST, moz_bug_r_a4
no flags Details
Hack (1.06 KB, patch)
2006-03-20 12:52 PST, Blake Kaplan (:mrbkap) (please use needinfo!)
no flags Details | Diff | Review
Fix, part 1 (1.12 KB, patch)
2006-03-20 18:08 PST, Blake Kaplan (:mrbkap) (please use needinfo!)
jst: review+
jst: superreview+
Details | Diff | Review
Proposed setTimeout belt-and-braces (2.29 KB, patch)
2006-03-22 14:26 PST, Blake Kaplan (:mrbkap) (please use needinfo!)
jst: review+
bzbarsky: superreview+
Details | Diff | Review
testcase 4 (262 bytes, text/html)
2006-03-22 14:54 PST, Paul Nickerson
no flags Details
testcase 4 (update) (264 bytes, text/html)
2006-03-22 15:00 PST, Paul Nickerson
no flags Details
More JS_Set(UC)Property nukage (2.71 KB, patch)
2006-03-22 15:22 PST, Blake Kaplan (:mrbkap) (please use needinfo!)
jst: review+
Details | Diff | Review
JS_Set(UC)Property nukage, v2 (2.93 KB, patch)
2006-03-24 11:01 PST, Blake Kaplan (:mrbkap) (please use needinfo!)
jst: review+
brendan: superreview+
Details | Diff | Review
Followup (1.24 KB, patch)
2006-03-24 12:52 PST, Blake Kaplan (:mrbkap) (please use needinfo!)
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Review
Roll-up patch for branches (5.26 KB, patch)
2006-04-03 10:27 PDT, Blake Kaplan (:mrbkap) (please use needinfo!)
jst: approval‑branch‑1.8.1+
dveditz: approval1.8.0.4+
Details | Diff | Review
1.0.x try (3.38 KB, patch)
2006-06-13 14:58 PDT, Alexander Sack
no flags Details | Diff | Review

Description Paul Nickerson 2006-03-16 19:34:29 PST
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 2006-03-17 21:16:24 PST
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 2006-03-18 01:16:20 PST
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 2006-03-18 01:18:24 PST
Created attachment 215478 [details]
testcase1 - arbitrary code execution using Node.__proto__
Comment 4 moz_bug_r_a4 2006-03-18 01:20:07 PST
Created attachment 215479 [details]
testcase2 - arbitrary code execution using Object.prototype
Comment 5 Paul Nickerson 2006-03-18 13:51:49 PST
(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 2006-03-18 19:43:43 PST
Created attachment 215551 [details]
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] 2006-03-20 11:28:36 PST
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) (please use needinfo!) 2006-03-20 12:52:02 PST
Created attachment 215694 [details] [diff] [review]
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) (please use needinfo!) 2006-03-20 12:54:58 PST
(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) (please use needinfo!) 2006-03-20 18:08:56 PST
Created attachment 215736 [details] [diff] [review]
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, jst@mozilla.com) 2006-03-20 18:09:14 PST
Comment on attachment 215736 [details] [diff] [review]
Fix, part 1

r+sr=jst
Comment 12 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-03-20 18:25:34 PST
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 2006-03-20 20:49:54 PST
Why didn't the fix for bug 223041 prevent setTimeout from being used in this exploit?
Comment 14 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-03-21 00:18:43 PST
(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 2006-03-21 00:35:20 PST
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] 2006-03-21 08:06:11 PST
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) (please use needinfo!) 2006-03-21 09:41:46 PST
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, jst@mozilla.com) 2006-03-21 16:42:01 PST
(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] 2006-03-21 17:08:05 PST
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) (please use needinfo!) 2006-03-22 14:26:43 PST
Created attachment 215932 [details] [diff] [review]
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 2006-03-22 14:54:10 PST
Created attachment 215937 [details]
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 2006-03-22 14:55:42 PST
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 2006-03-22 15:00:16 PST
Created attachment 215938 [details]
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) (please use needinfo!) 2006-03-22 15:22:44 PST
Created attachment 215939 [details] [diff] [review]
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, jst@mozilla.com) 2006-03-23 17:37:00 PST
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] 2006-03-23 21:32:01 PST
(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 [:bz] (Out June 25-July 6) 2006-03-23 22:08:00 PST
Comment on attachment 215932 [details] [diff] [review]
Proposed setTimeout belt-and-braces

Rename "prin" to "ourPrincipal" and sr=bzbarsky.
Comment 28 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-03-23 22:18:11 PST
(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) (please use needinfo!) 2006-03-24 10:50:03 PST
I checked attachment 215932 [details] [diff] [review] into the trunk.
Comment 30 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-03-24 11:01:02 PST
Created attachment 216133 [details] [diff] [review]
JS_Set(UC)Property nukage, v2

I think this patch uses the correct bits.
Comment 31 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-03-24 12:52:56 PST
Created attachment 216144 [details] [diff] [review]
Followup

Ugh, we _really_ need to figure out how to use nsRefPtr with nsTimeouts...
Comment 32 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-03-24 13:05:47 PST
Comment on attachment 216144 [details] [diff] [review]
Followup

Verily.  :(
Comment 33 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-03-24 13:52:14 PST
Attachment 216144 [details] [diff] has been checked in.
Comment 34 Johnny Stenback (:jst, jst@mozilla.com) 2006-03-28 17:21:53 PST
Comment on attachment 216133 [details] [diff] [review]
JS_Set(UC)Property nukage, v2

r=jst fwiw.
Comment 35 Brendan Eich [:brendan] 2006-03-29 09:22:09 PST
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) (please use needinfo!) 2006-04-03 09:55:27 PDT
Final fix checked into trunk. I'll make a rollup patch for the 1.8 branches shortly.
Comment 37 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-04-03 10:27:42 PDT
Created attachment 217039 [details] [diff] [review]
Roll-up patch for branches

'cvs up -j' is my friend.
Comment 38 Blake Kaplan (:mrbkap) (please use needinfo!) 2006-04-10 13:58:49 PDT
Fix checked into the 1.8 branch.
Comment 39 Daniel Veditz [:dveditz] 2006-04-17 12:06:53 PDT
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) (please use needinfo!) 2006-04-21 14:37:02 PDT
Fix checked into the 1.8.0 branch.
Comment 41 Bob Clary [:bc:] 2006-04-23 18:24:42 PDT
added to the security tests on the qa farm.
Comment 42 Tracy Walker [:tracy] 2006-05-11 09:28:45 PDT
verified with Windows and Mac Ffx 1.5.0.4 builds from 20060508
Comment 43 Alexander Sack 2006-06-13 14:58:15 PDT
Created attachment 225482 [details] [diff] [review]
1.0.x try
Comment 44 Bob Clary [:bc:] 2006-08-22 13:06:14 PDT
ff2b2 verified 1.8 fixed windows/linux

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