Note: There are a few cases of duplicates in user autocompletion which are being worked on.

DOM property setter override (remote compromise)

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
JavaScript Engine
P1
critical
RESOLVED FIXED
12 years ago
4 years ago

People

(Reporter: Paul Nickerson, Assigned: mrbkap)

Tracking

(Blocks: 1 bug, {testcase, verified1.8.0.4, verified1.8.1})

Trunk
mozilla1.9alpha1
testcase, verified1.8.0.4, verified1.8.1
Points:
---
Bug Flags:
blocking1.7.14 +
blocking-aviary1.0.9 +
blocking1.8.1 +
blocking1.8.0.4 +
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical] [patch])

Attachments

(10 attachments, 3 obsolete attachments)

297 bytes, text/html
Details
299 bytes, text/html
Details
381 bytes, text/html
Details
1.12 KB, patch
jst
: review+
Details | Diff | Splinter Review
2.29 KB, patch
jst
: review+
Details | Diff | Splinter Review
264 bytes, text/html
Details
2.93 KB, patch
jst
: review+
brendan
: superreview+
Details | Diff | Splinter Review
1.24 KB, patch
Details | Diff | Splinter Review
5.26 KB, patch
Details | Diff | Splinter Review
3.38 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

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

12 years ago
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?
Assignee: nobody → jst
Whiteboard: [sg:investigate]
Blocks: 330774
No longer blocks: 330774
Blocks: 330774

Comment 2

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

12 years ago
Created attachment 215478 [details]
testcase1 - arbitrary code execution using Node.__proto__

Comment 4

12 years ago
Created attachment 215479 [details]
testcase2 - arbitrary code execution using Object.prototype

Updated

12 years ago
Whiteboard: [sg:investigate] → [sg:critical]
(Reporter)

Comment 5

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

Updated

12 years ago
Severity: normal → critical
Flags: blocking1.8.0.2?
(Reporter)

Updated

12 years ago
Summary: DOM property setter override (possible vuln?) → DOM property setter override (remote compromise)

Comment 6

12 years ago
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.)
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.
Component: General → JavaScript Engine
Flags: blocking1.8.0.3+
Flags: blocking1.8.0.2?
Flags: blocking-aviary1.0.9+
Product: Firefox → Core
Version: 1.5.0.x Branch → Trunk
(Assignee)

Comment 8

12 years ago
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
Attachment #215694 - Flags: review?(jst)
(Assignee)

Comment 9

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

Comment 10

12 years ago
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.
Assignee: jst → mrbkap
Attachment #215694 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #215736 - Flags: superreview?(jst)
Attachment #215736 - Flags: review?(jst)
Attachment #215694 - Flags: review?(jst)
Comment on attachment 215736 [details] [diff] [review]
Fix, part 1

r+sr=jst
Attachment #215736 - Flags: superreview?(jst)
Attachment #215736 - Flags: superreview+
Attachment #215736 - Flags: review?(jst)
Attachment #215736 - Flags: review+
(Assignee)

Comment 12

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

Updated

12 years ago
Priority: -- → P1
Target Milestone: --- → mozilla1.9alpha

Comment 13

12 years ago
Why didn't the fix for bug 223041 prevent setTimeout from being used in this exploit?
(Assignee)

Comment 14

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

12 years ago
Hmm, doesn't leaving out setTimeout negate the benefit of that fix?  Do you know why setTimeout was left out?
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
(Assignee)

Comment 17

12 years ago
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.
OS: Windows XP → All
Hardware: PC → All
(In reply to comment #16)
> Should the prototype property have JSPROP_READONLY and JSPROP_PERMANENT too? 

I'm all for that!
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
(Assignee)

Comment 20

12 years ago
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.
Attachment #215932 - Flags: review?(jst)
(Reporter)

Comment 21

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

Comment 22

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

Updated

12 years ago
Attachment #215937 - Attachment is obsolete: true
(Reporter)

Comment 23

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

Comment 24

12 years ago
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.
Attachment #215939 - Flags: review?(jst)
(Assignee)

Updated

12 years ago
Whiteboard: [sg:critical] → [sg:critical] [patch]

Updated

12 years ago
Attachment #215932 - Flags: review?(jst) → review+
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.
Attachment #215939 - Flags: review?(jst) → review+
(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
(Assignee)

Updated

12 years ago
Attachment #215932 - Flags: superreview?(bzbarsky)

Comment 27

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

Rename "prin" to "ourPrincipal" and sr=bzbarsky.
Attachment #215932 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 28

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

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

Comment 29

12 years ago
I checked attachment 215932 [details] [diff] [review] into the trunk.
(Assignee)

Comment 30

12 years ago
Created attachment 216133 [details] [diff] [review]
JS_Set(UC)Property nukage, v2

I think this patch uses the correct bits.
Attachment #215939 - Attachment is obsolete: true
Attachment #216133 - Flags: review?(jst)
(Assignee)

Comment 31

12 years ago
Created attachment 216144 [details] [diff] [review]
Followup

Ugh, we _really_ need to figure out how to use nsRefPtr with nsTimeouts...
Attachment #216144 - Flags: superreview?(bzbarsky)
Attachment #216144 - Flags: review?(bzbarsky)

Comment 32

12 years ago
Comment on attachment 216144 [details] [diff] [review]
Followup

Verily.  :(
Attachment #216144 - Flags: superreview?(bzbarsky)
Attachment #216144 - Flags: superreview+
Attachment #216144 - Flags: review?(bzbarsky)
Attachment #216144 - Flags: review+
(Assignee)

Comment 33

12 years ago
Attachment 216144 [details] [diff] has been checked in.
Comment on attachment 216133 [details] [diff] [review]
JS_Set(UC)Property nukage, v2

r=jst fwiw.
Attachment #216133 - Flags: review?(jst) → review+
(Assignee)

Updated

12 years ago
Attachment #216133 - Flags: superreview?(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
Attachment #216133 - Flags: superreview?(brendan) → superreview+
(Assignee)

Comment 36

12 years ago
Final fix checked into trunk. I'll make a rollup patch for the 1.8 branches shortly.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Comment 37

12 years ago
Created attachment 217039 [details] [diff] [review]
Roll-up patch for branches

'cvs up -j' is my friend.
Attachment #217039 - Flags: approval1.8.0.3?
Attachment #217039 - Flags: approval-branch-1.8.1?(jst)
Flags: blocking1.8.1+

Updated

11 years ago
Attachment #217039 - Flags: approval-branch-1.8.1?(jst) → approval-branch-1.8.1+
(Assignee)

Comment 38

11 years ago
Fix checked into the 1.8 branch.
Keywords: fixed1.8.1
Comment on attachment 217039 [details] [diff] [review]
Roll-up patch for branches

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #217039 - Flags: approval1.8.0.3? → approval1.8.0.3+
(Assignee)

Comment 40

11 years ago
Fix checked into the 1.8.0 branch.
Keywords: fixed1.8.0.3

Comment 41

11 years ago
added to the security tests on the qa farm.
Flags: in-testsuite+
verified with Windows and Mac Ffx 1.5.0.4 builds from 20060508
Keywords: fixed1.8.0.4 → verified1.8.0.4
Flags: blocking1.7.14+

Comment 43

11 years ago
Created attachment 225482 [details] [diff] [review]
1.0.x try

Comment 44

11 years ago
ff2b2 verified 1.8 fixed windows/linux
Keywords: fixed1.8.1 → verified1.8.1
Group: security

Updated

10 years ago
Flags: in-testsuite+ → in-testsuite?

Updated

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