Closed Bug 92773 Opened 23 years ago Closed 21 years ago

calling custom getter/setter not subject to same-origin check

Categories

(Core :: Security, defect, P2)

x86
Windows 98
defect

Tracking

()

RESOLVED FIXED
mozilla1.4alpha

People

(Reporter: jruderman, Assigned: brendan)

Details

(Keywords: csectype-sop, js1.5, Whiteboard: [sg:mustfix])

Attachments

(6 files, 2 obsolete files)

If I can get a reference to an object at another domain, I can call its getters 
and setters.  I can't call getters and setters given to it by its prototype, 
though (why not?).
Group: netscapeconfidential?
Attached file attacker.html
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla0.9.4
Not critical for 0.9.4, moving to 0.9.5.
Group: netscapeconfidential?
Target Milestone: mozilla0.9.4 → mozilla0.9.5
time marches on. Retargeting to 0.9.6.
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Moving the most time-critical bugs and minor security fixes to 0.9.7
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Mass change: It appears that several bugs got accidentally opened up as part of
a mass change - see bug 107718 (now fixed). Moving back to security-sensitive
group. These were open for about 10 days.
Group: security?
Target Milestone: mozilla0.9.7 → mozilla1.0.1
Adding "custom" to summary to reflect that only site-defined getters/setters can
be called. Please correct, if I misunderstood something.
Summary: calling getter/setter not subject to same-origin check → calling custom getter/setter not subject to same-origin check
The JS engine never calls any of the object hooks (i.e. no call to getProperty
when accessing a getter defined on a window). Given that it looks like this
won't be fixable w/o some JS engine changes I'm suggestign we ship MachV with
this bug unless some JS engine guru's can whip up a trivial fix for this problem
real soon.
Target Milestone: mozilla1.0.1 → mozilla1.2alpha
Target Milestone: mozilla1.2alpha → mozilla1.3alpha
We need some motion on this bug. JS gurus, please nominate someone to own this.
CC list accessible: true
Accessible to reporter
I don't know what comment #0 means by "I can't call getters and setters given to
it by its prototype, though".  Is there a testcase variation showing this?

A getter or setter is just a function called from the engine.  How is this
attack any different from calling a function defined in a window loading a
document from a different origin?

I don't see a need for JS engine hooks here.  The root of all evil is gaining
access to another origin's objects.  How is that possible?

/be
If you look at the testcase, Jesse is doing 'x =
window.open("http://localhost/...")' to get a reference to the window.

Anyway, it seems illogical that inherited getters and setters should be
inaccessible but locally defined getters and setters should be accessible. We
should be consistent.
I see no prototypes (C.prototype for a constructor function C, __proto__ usage)
in the testcase.  I'm not running a webserver here right now, so I haven't
tested and debugged, but I will soon.  I was hoping someone (Jesse, ideally)
would say more about what's going on, where the prototype-delegated properties
are tested, etc.  What am I missing?

/be
Brendan, are you working on this issue? If not, please pass it back to me or peterv.
Assignee: mstoltz → brendan
Status: ASSIGNED → NEW
Sorry, I missed the bugmail for comment #13.  I'll pass this back to mstoltz and
stand by to help when needed.

Jesse, I still would appreciate an exact statement of the problem in terms of
the testcase -- where does attacker.html demonstrate the statement "I can't call
getters and setters given to it by its prototype"?  And how do you get a
reference to an object at another domain, except via some other hole (not
reported by this bug)?

/be
Assignee: brendan → mstoltz
The problem demonstrated by the testcase is "If I can get a reference to an
object at another domain, I can call its [custom] getters and setters."  For
example, in the testcase, the attacker is able to call getters and setters for
window.foopy and window.document.foopy in a document from another domain.  If
the getters and setters were on objects other than window or document, this hole
wouldn't be useful, because the attacker shouldn't be able to access those objects.

I don't remember what I meant by "I can't call getters and setters given to it
by its prototype, though (why not?).".
When we get a property that has a getter, it's a pure-JS function invocation
that happens completely within the JS engine - no security check is called.
OTOH, when accessing a property of the Window or Document object directly (with
no getter/setter), the JS engine asks the XPC-wrapped-native scriptable helper
(nsDOMClassInfo) to do the get/set, and a security check does happen in that
case. So, it appears that what we need is a callback from the JS engine when
calling a getter/setter on a property of the Window or Document object. What I'm
not sure about is how selective we can be with that check, and what performance
impact it will have.
Clearing milestone for now.
Target Milestone: mozilla1.3alpha → ---
I'll take this one back, it requires core JS engine work of some kind.

/be
Assignee: mstoltz → brendan
Keywords: js1.5
Target Milestone: --- → mozilla1.4alpha
Thanks, Brendan. Do you think you'll be able to address this in the Moz 1.4
timeframe? We're trying to get all known security issues fixed by then.
Attached patch proposed js/src patch (obsolete) — Splinter Review
This patch adds a hook for each user-defined getter or setter function called
via a property access, to the JSRuntime's checkObjectAccess callback configured
via JS_SetCheckObjectAccessCallback.

The caps implementation for that callback,
nsScriptSecurityManager::CheckJSFunctionCallerAccess, just needs to be
generalized so it doesn't insist on being called only for the "caller"
property:

- It should not assert that the id passed in is a jsval denoting the string
"caller".
- It can use the target object's class name, instead of hard-coding "Function".

- nsScriptSecurityManager can lose the static sCallerID interned JS string
value altogether.

Patch to caps with these changes coming up.

This patch also fixes a long-standing bug that broke the old-style top-level
getter/setter declaration syntax:

    getter function magic()	  { return ++_magic; }
    setter function magic(newval) { return _magic = newval; }

by reporting a "redeclaration of function magic" error on the setter.  Note the
spanky use of NXOR.

Phil, could you please run any real-world JS performance tests that use getters
and setters, if you know of any?  I don't think the js_InternalGetOrSet wrapper
will ding performance noticeably, but it would be good to check (if possible).

/be
Note the XXX comment I added.  I think caps code often returns JS_FALSE to the
JS engine, which should mean an error was reported or an exception set pending,
but in too many cases, the caps code neither reports nor throws.  Mitch, could
you please file a followup bug?  Thanks.

/be
Comment on attachment 116149 [details] [diff] [review]
proposed js/src patch

Shaver, I hope you have time to review this.  See comment #20.	Thanks,

/be
Attachment #116149 - Flags: review?(shaver)
Attachment #116150 - Flags: superreview?(jst)
Attachment #116150 - Flags: review?(mstoltz)
Sorry, Phil wasn't cc'd.  See comment #20.

/be
Comment on attachment 116150 [details] [diff] [review]
proposed caps patch

Forgot to say that I added a missing JS request (JS_BeginRequest/JS_EndRequest)
around an otherwise unbracketed call into the JS API.

/be
Comment on attachment 116149 [details] [diff] [review]
proposed js/src patch

Shaver, make me change js_InternalGetOrSet take a JSAccessMode arg and pass it
through.  I opted for the patch's approach only to keep the JSINVOKE_* layer
self-contained, but really, the only data dependency on the new JSINVOKE_GETTER
and JSINVOKE_SETTER flags is the access mode passed to the checkObjectAccess
callback.

This wouldn't cut down on the high number of parameters to js_InternalGetOrSet,
but it would eliminate the invoke-flag-to-access-mode translation via ?:.

/be
Comment on attachment 116150 [details] [diff] [review]
proposed caps patch

sr=jst, looks good to me.
Attachment #116150 - Flags: superreview?(jst) → superreview+
Comment on attachment 116149 [details] [diff] [review]
proposed js/src patch

You know, I was just saying the other day how the JS engine needed more NXOR.
r=shaver, even if I did have to noodle a little over the various cases of attrs
and oldAttrs.

(Tip for nicer reviewing: the -p flag to "cvs diff" will provide
usually-correct function context in the chunk-position lines.  Thus ends the
lesson.)
Attachment #116149 - Flags: review?(shaver) → review+
Comment on attachment 116150 [details] [diff] [review]
proposed caps patch

Looks good to me, r=mstoltz.

Out of curiosity, what does ::JS_BeginRequest(cx) do, and should I be using it
elsewhere?
Attachment #116150 - Flags: review?(mstoltz) → review+
mstoltz: any non-blocking chunk of native code containing JS API calls, and
typically starting with one API call and ending with another, in a
multi-threaded embedding should be bracketed with JS_BeginRequest and
JS_EndRequest.  It follows from this that any JS native method (function,
getter, setter, etc.) that needs to block on some i/o, a non-JS lock or monitor,
etc., should call JS_SuspendRequest before blocking and JS_ResumeRequest after.

This is all weakly documented by the JS API docs, and by a bunch of postings to
the m.jseng group.  I'll be peppering the tree with fixes for missing requests
in the patch for bug 176182.

/be
Status: NEW → ASSIGNED
Re Comment #20: I haven't found a good real-world test for this;
in lieu of that, I will attach a tight-loop timing test below.
Attached file Timing test #1 (obsolete) —
Attachment #116250 - Attachment is obsolete: true
It will take me a while to pull and build the browser with and without
the patch applied. If anyone has these builds handy already, he can try
the timing test above and report the results. If it is too simplistic
a test, please feel free to suggest or attach variations -
I can't seem to build the browser successfully with the latest patch
applied. I do get something that launches, but it's missing a lot of
functionality and can't load any URL. 

If I try to load a URL, there are lots of errors in the JS Console,
such as, "URLBarFocusHandler is not defined", etc. etc.

I had thought it was due to some mistake on my part, but might there
be a problem with the patch itself?
Comment on attachment 116149 [details] [diff] [review]
proposed js/src patch

It looks like this patch was landed on 2003-03-04 but then backed out because
it caused tinderbox test failures: failing to load URLs in the page cycler, and
perhaps something else.  (Just noting that here since it doesn't seem to have
been noted.)
You may want to increase the UBOUND constant in the JS shell test,
because it completes much faster in the shell than in the browser.

I increased it by a factor of 10 and found no appreciable difference
in the timing results with or without the above js/src patch applied. 

For some reason, I consistently got a better time with the patch in.
(e.g. 573 ms with the js/src patch applied, vs. 577 ms without patch).
I finally got my builds working on RH8 and saw the same problems Phil saw, the
ones that caused dougt to back out the js/src patch I checked in the other day
-- sorry about that, I lost my mind -- not only would it not have worked due to
a bug in the js_CheckRedeclaration change, but the js_InternalGetOrSet change
needed the caps patch to be checked in too!  I am filled with shame.

This patch avoids extra GETTER/SETTER JSINVOKE_* flags by passing JSACC_READ or
JSACC_WRITE, which can be passed directly to the rt->checkObjectAccess hook. 
It also fixes the bug in js_CheckRedeclaration, by restoring needed behavior
that allowed a function or variable to be redefined.  In the diff, notice that
the early return for !(attrs & (JSPROP_GETTER | JSPROP_SETTER) is intact; the
magic ~(oldAttrs ^ attrs) & (JSPROP_GETTER | JSPROP_SETTER) equal-to-zero test
follows it, and the comment describes the logic better too.

This patch works for me in the shell and (with the caps patch) in mozilla.

/be
Attachment #116149 - Attachment is obsolete: true
Attachment #116149 - Flags: review+
Attachment #116437 - Flags: review?(shaver)
Comment on attachment 116437 [details] [diff] [review]
fixed js/src patch

(I had to work out the NXOR  logic from first principles again, sigh.)

r=shaver
Attachment #116437 - Flags: review?(shaver) → review+
I wonder why URLBarFocusHandler (among many other JS functions in chrome .js
files) is being redeclared.  I'll have to fire up my debug build, as soon as
it's together.

/be
Fixed, for sure.  I tested using apache running on my localhost.

/be
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
I have tested the js/src checkin on WinNT, Linux RH7, and Mac OSX.
It passes the JS testsuite in both the debug and optimized JS shell
on all three platforms: no test regressions are observed.

I have also run the JS shell timing test above on Linux, and once again
there was no difference in the result before and after the patch.
Group: security
Keywords: csec-sop
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: