Last Comment Bug 92773 - calling custom getter/setter not subject to same-origin check
: calling custom getter/setter not subject to same-origin check
Status: RESOLVED FIXED
[sg:mustfix]
: csectype-sop, js1.5
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: x86 Windows 98
: P2 normal (vote)
: mozilla1.4alpha
Assigned To: Brendan Eich [:brendan]
: ckritzer (gone)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2001-07-29 23:27 PDT by Jesse Ruderman
Modified: 2013-06-09 18:48 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
attacker.html (521 bytes, text/html)
2001-07-29 23:28 PDT, Jesse Ruderman
no flags Details
victim.html - place in http://localhost/ (619 bytes, text/html)
2001-07-29 23:29 PDT, Jesse Ruderman
no flags Details
proposed js/src patch (4.41 KB, patch)
2003-03-03 15:09 PST, Brendan Eich [:brendan]
no flags Details | Diff | Review
proposed caps patch (5.47 KB, patch)
2003-03-03 15:11 PST, Brendan Eich [:brendan]
security-bugs: review+
jst: superreview+
Details | Diff | Review
Timing test #1 (3.40 KB, text/html)
2003-03-04 15:41 PST, Phil Schwartau
no flags Details
Timing test #1 (corrected a typo) (3.40 KB, text/html)
2003-03-04 15:43 PST, Phil Schwartau
no flags Details
JS shell version of Timing test #1 (1.80 KB, text/plain)
2003-03-05 21:14 PST, Phil Schwartau
no flags Details
fixed js/src patch (4.18 KB, patch)
2003-03-06 09:25 PST, Brendan Eich [:brendan]
shaver: review+
Details | Diff | Review

Description Jesse Ruderman 2001-07-29 23:27:54 PDT
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?).
Comment 1 Jesse Ruderman 2001-07-29 23:28:35 PDT
Created attachment 43968 [details]
attacker.html
Comment 2 Jesse Ruderman 2001-07-29 23:29:00 PDT
Created attachment 43969 [details]
victim.html - place in http://localhost/
Comment 3 Mitchell Stoltz (not reading bugmail) 2001-09-04 16:54:22 PDT
Not critical for 0.9.4, moving to 0.9.5.
Comment 4 Mitchell Stoltz (not reading bugmail) 2001-10-04 10:34:39 PDT
time marches on. Retargeting to 0.9.6.
Comment 5 Mitchell Stoltz (not reading bugmail) 2001-10-31 14:45:23 PST
Moving the most time-critical bugs and minor security fixes to 0.9.7
Comment 6 Bradley Baetz (:bbaetz) 2001-11-10 07:26:35 PST
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.
Comment 7 Ben Bucksch (:BenB) 2002-06-03 23:07:23 PDT
Adding "custom" to summary to reflect that only site-defined getters/setters can
be called. Please correct, if I misunderstood something.
Comment 8 Johnny Stenback (:jst, jst@mozilla.com) 2002-06-20 18:09:23 PDT
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.
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2002-09-18 14:10:33 PDT
We need some motion on this bug. JS gurus, please nominate someone to own this.
Comment 10 Brendan Eich [:brendan] 2002-09-18 14:20:59 PDT
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
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2002-10-10 09:25:48 PDT
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.
Comment 12 Brendan Eich [:brendan] 2002-10-10 09:42:11 PDT
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
Comment 13 Mitchell Stoltz (not reading bugmail) 2002-10-31 15:11:14 PST
Brendan, are you working on this issue? If not, please pass it back to me or peterv.
Comment 14 Brendan Eich [:brendan] 2002-11-14 11:15:44 PST
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
Comment 15 Jesse Ruderman 2002-11-16 14:54:55 PST
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?).".
Comment 16 Mitchell Stoltz (not reading bugmail) 2003-02-18 11:51:08 PST
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.
Comment 17 Mitchell Stoltz (not reading bugmail) 2003-02-20 18:33:14 PST
Clearing milestone for now.
Comment 18 Brendan Eich [:brendan] 2003-02-21 11:28:53 PST
I'll take this one back, it requires core JS engine work of some kind.

/be
Comment 19 Mitchell Stoltz (not reading bugmail) 2003-02-24 13:35:28 PST
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.
Comment 20 Brendan Eich [:brendan] 2003-03-03 15:09:10 PST
Created attachment 116149 [details] [diff] [review]
proposed js/src patch

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
Comment 21 Brendan Eich [:brendan] 2003-03-03 15:11:08 PST
Created attachment 116150 [details] [diff] [review]
proposed caps patch

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 22 Brendan Eich [:brendan] 2003-03-03 15:21:33 PST
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
Comment 23 Brendan Eich [:brendan] 2003-03-03 15:31:59 PST
Sorry, Phil wasn't cc'd.  See comment #20.

/be
Comment 24 Brendan Eich [:brendan] 2003-03-03 15:38:39 PST
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 25 Brendan Eich [:brendan] 2003-03-03 15:46:48 PST
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 26 Johnny Stenback (:jst, jst@mozilla.com) 2003-03-03 16:07:47 PST
Comment on attachment 116150 [details] [diff] [review]
proposed caps patch

sr=jst, looks good to me.
Comment 27 Mike Shaver (:shaver -- probably not reading bugmail closely) 2003-03-03 18:57:58 PST
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.)
Comment 28 Mitchell Stoltz (not reading bugmail) 2003-03-04 11:45:09 PST
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?
Comment 29 Brendan Eich [:brendan] 2003-03-04 14:36:01 PST
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
Comment 30 Phil Schwartau 2003-03-04 15:40:05 PST
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.
Comment 31 Phil Schwartau 2003-03-04 15:41:06 PST
Created attachment 116250 [details]
Timing test #1
Comment 32 Phil Schwartau 2003-03-04 15:43:58 PST
Created attachment 116251 [details]
Timing test #1 (corrected a typo)
Comment 33 Phil Schwartau 2003-03-04 16:31:17 PST
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 -
Comment 34 Phil Schwartau 2003-03-05 20:57:02 PST
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 35 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2003-03-05 21:03:00 PST
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.)
Comment 36 Phil Schwartau 2003-03-05 21:14:58 PST
Created attachment 116403 [details]
JS shell version of Timing test #1
Comment 37 Phil Schwartau 2003-03-05 21:39:02 PST
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).
Comment 38 Brendan Eich [:brendan] 2003-03-06 09:25:43 PST
Created attachment 116437 [details] [diff] [review]
fixed js/src 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
Comment 39 Mike Shaver (:shaver -- probably not reading bugmail closely) 2003-03-06 09:37:14 PST
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
Comment 40 Brendan Eich [:brendan] 2003-03-06 09:54:54 PST
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
Comment 41 Brendan Eich [:brendan] 2003-03-06 12:07:06 PST
Fixed, for sure.  I tested using apache running on my localhost.

/be
Comment 42 Phil Schwartau 2003-03-06 18:46:00 PST
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.

Note You need to log in before you can comment on or make changes to this bug.