Bypassing CheckLoadURI via Window.prototype

VERIFIED FIXED in mozilla1.0

Status

()

Core
Security
--
minor
VERIFIED FIXED
16 years ago
15 years ago

People

(Reporter: bbaetz, Assigned: jst)

Tracking

Trunk
mozilla1.0
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [FIXED ON TRUNK][ADT2 RTM])

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

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

Comment 1

16 years ago
Created attachment 83000 [details]
demonstration

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

Comment 2

16 years ago
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?)
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
Keywords: nsbeta1+
Whiteboard: [ADT2 RTM]
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
Status: NEW → ASSIGNED
(Assignee)

Comment 5

16 years ago
Created attachment 84386 [details] [diff] [review]
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...
(Assignee)

Comment 6

16 years ago
Cc:ing jband in case he has some input on the attached patch...
I'll review soon.

/be
Blocks: 143200
Created attachment 84486 [details] [diff] [review]
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.)
(Assignee)

Comment 9

16 years ago
Created attachment 84500 [details] [diff] [review]
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.
(Assignee)

Updated

16 years ago
Attachment #84500 - Attachment description: Updated patch (diff -w), needs brandan's earlier patch to work... → Wrong file, please ignore.
Attachment #84500 - Attachment is obsolete: true
(Assignee)

Comment 10

16 years ago
Created attachment 84502 [details] [diff] [review]
Updated patch (diff -w), needs brendan's earlier patch to work...
(Assignee)

Updated

16 years ago
Attachment #84502 - Attachment description: Updated patch (diff -w), needs brandan's earlier patch to work... → Updated patch (diff -w), needs brendan's earlier patch to work...

Comment 11

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

sr=jband
Attachment #84502 - Flags: superreview+
jst's patching this.

/be
Assignee: mstoltz → jst
Status: ASSIGNED → NEW
(Assignee)

Comment 13

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

sr=jst
Attachment #84486 - Flags: superreview+
Comment on attachment 84502 [details] [diff] [review]
Updated patch (diff -w), needs brendan's earlier patch to work...

r=brendan@mozilla.org, let's get this into the trunk ASAP.

a=chofmann@netscape.com for the 1.0 branch to land by 4am tomorrow -- we need
this for the rc3 build firing off then.

/be
Attachment #84502 - Flags: review+
Attachment #84502 - Flags: approval+
Comment on attachment 84486 [details] [diff] [review]
fix JS engine to report o.__proto__ = ... via OBJ_CHECK_ACCESS

r=shaver.
Attachment #84486 - Flags: review+
Comment on attachment 84486 [details] [diff] [review]
fix JS engine to report o.__proto__ = ... via OBJ_CHECK_ACCESS

a=chofmann@netscape.com, he said.

/be
Attachment #84486 - Flags: approval+
(Assignee)

Comment 17

16 years ago
Fix checked in on the trunk. Will land on the branch later tonight.
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: PC → All
Target Milestone: --- → mozilla1.0
(Assignee)

Updated

16 years ago
Whiteboard: [ADT2 RTM] → [FIXED ON TRUNK][ADT2 RTM]
(Assignee)

Comment 18

16 years ago
Created attachment 84558 [details] [diff] [review]
Final diff that was checked in on the trunk (minor tweaks from the above)
(Assignee)

Comment 19

16 years ago
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?
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Keywords: fixed1.0.0
Resolution: --- → FIXED
(Assignee)

Comment 20

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

Comment 21

16 years ago
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.
Status: RESOLVED → VERIFIED
Keywords: fixed1.0.0 → verified1.0.1
Group: security?
You need to log in before you can comment on or make changes to this bug.