143369 - Bypassing CheckLoadURI via Window.prototype

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[Bradley Baetz (:bbaetz)]

Also see bug 110510 and bug 141717, which is what gave me the motivation to try
this.

I'm going to attach a test case which sets a getter on
Window.prototype.location. Places in the code which use this to go uri-based
security checks get the new value, and so do the wrong thing when calling
checkLoadUriStr. You shouldn't be able to open chrome/file/etc uris like this.

I don't think that this is directly exploitable - the href getter doesn't appear
to run with chrome privs, and most of the security checks are done in c++
anyway. I haven't tested to check if you then have cross-domain access to the
next page (you can open file urls this way too, of course) - I don't think its
possible to get the associated window object from a page, so I don't know how to
test.

If you have the 'middle-click-opens-in-new-tabs' pref set, there is code whcih
would be affected by this. Is is possible to simulate a middle click MouseEvent
which would run that js code?

Even if its not, is there another attribute on window/element/etc which spoofing
either functions on the page, or attributes, would affect?

Bug 141717 gives anyone the ability to stop context menus popping up. Thats a
bad idea, but its not an exploit.

Also see bug 36946, which describes a similar issue regarding this but doesn't
give an exploit (2 years old and still security sensitive??).

jst suggested in bug 141717 that people call the method via __proto__, but do we
have to do that every time we try to get content from the user? Is there a
better way, without having to audit the codebase for every time we get a
property off a window?

Comment 1

[Bradley Baetz (:bbaetz)]

Attachment: demonstration

Right click on this, and open in new tab/window.

Comment 2

[Bradley Baetz (:bbaetz)]

This may be exploitable via a javascript url, except that the context menu for
js urls don't have the option to open in a new tab/window, so I can't test. (Why
not, btw?)

Comment 3

[georgi - hopefully not receiving bugspam]

I think this is quite similar to bug 143420.
There is the following scary code in nsContextMenu.js:
   showOnlyThisFrame : function () {
        window._content.location.href = this.target.ownerDocument.location.href;
    },
One attack is to redefine location.href via getter - almost sure this will work.
Another possibility is to try playing with location.href setter

Comment 4

[Brendan Eich [:brendan]]

Doesn't this seem important to fix for mozilla1.0?  Today is probably the last
day to fix bugs for 1.0rc3.  What's the plan here?

/be

Comment 5

[Johnny Stenback (:jst)]

Attachment: Proposed fix (not heavily tested yet)

This patch makes window.location (i.e. the location property on the window
object instance) shadow the getter/setter on the proto and thus eliminating the
possibility of overriding the location getter/setter on the prototype. This
patch also stops window.location from being overriden with a getter or setter
on the instance object. Same goes for document.location. In addition to those
changes the patch also makes Location.prototype immutable, thus making it
impossible to override property getters and setters on a location object. Oh,
and while I was at it, I made ChromeWindow.prototype immutable too, even if
that shouldn't be needed. We could do the same for Window.prototype, but that
scares me, I don't want to do that this late in the game.

Beat on this, review, superreview n' do whatever you can if you have a
chance...

Comment 6

[Johnny Stenback (:jst)]

Cc:ing jband in case he has some input on the attached patch...

Comment 7

[Brendan Eich [:brendan]]

I'll review soon.

/be

Comment 8

[Brendan Eich [:brendan]]

Attachment: fix JS engine to report o.__proto__ = ... via OBJ_CHECK_ACCESS

jst needs this to cover all cases.  We must call OBJ_CHECK_ACCESS for the set
case implemented by obj_setSlot, which affects __proto__, as well as for gets
of __proto__ and __parent__ (the latter is readonly and permanent) implemented
by obj_getSlot for those two properties.

We therefore need a flag, so I am redefining the value of JSACC_WRITE,
heretofore unused in the engine (*), so that it may be tested independently of
the "access mode type" isolated from OBJ_CHECK_ACCESS's |JSAccessMode mode|
parameter via (mode & JSACC_TYPEMASK).

/be

(*  The OBJ_CHECK_ACCESS hook was called, until now, only for gets of certain
magic properties, and for sets of watchpointed properties, wherefore the
JSACC_WATCH mode.  It might seem better to pass JSACC_WATCH | JSACC_WRITE, but
a watchpoint can nullify the effect of a write to a property, and anyway, we
must keep API compatibility.  So the only JSACC_* value I can change is the one
that's unused, which was waiting for just such a use as this patch makes of it:
JSACC_WRITE.)

Comment 9

[Johnny Stenback (:jst)]

Attachment: Wrong file, please ignore.

AFAIK this blocks all the location[.href] spoofs that we've thought of/seen so
far, both when done on a proto, or on a document or window instance object.
Prove me wrong, please.

Comment 10

[Johnny Stenback (:jst)]

Attachment: Updated patch (diff -w), needs brendan's earlier patch to work...

Comment 11

[John Bandhauer]

Comment on attachment 84502 [details] [diff] [review]
Updated patch (diff -w), needs brendan's earlier patch to work...

sr=jband

Comment 12

[Brendan Eich [:brendan]]

jst's patching this.

/be

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 84486 [details] [diff] [review]
fix JS engine to report o.__proto__ = ... via OBJ_CHECK_ACCESS

sr=jst

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 84502 [details] [diff] [review]
Updated patch (diff -w), needs brendan's earlier patch to work...

r=[e-mail], let's get this into the trunk ASAP.

a=[e-mail] for the 1.0 branch to land by 4am tomorrow -- we need
this for the rc3 build firing off then.

/be

Comment 15

[Mike Shaver (:shaver emeritus)]

Comment on attachment 84486 [details] [diff] [review]
fix JS engine to report o.__proto__ = ... via OBJ_CHECK_ACCESS

r=shaver.

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 84486 [details] [diff] [review]
fix JS engine to report o.__proto__ = ... via OBJ_CHECK_ACCESS

a=[e-mail], he said.

/be

Comment 17

[Johnny Stenback (:jst)]

Fix checked in on the trunk. Will land on the branch later tonight.

Comment 18

[Johnny Stenback (:jst)]

Attachment: Final diff that was checked in on the trunk (minor tweaks from the above)

Comment 19

[Johnny Stenback (:jst)]

Fix checked in on the branch too. Should we clear the security bit on this bug
and open it up to the public at some point?

Comment 20

[Johnny Stenback (:jst)]

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

Comment 21

[bsharma]

Verified on 2002-06-27-trunk and 2002-06-27-branch build on Win2K.

The above demonstration passes and the exception is thrown in the JS console.