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

RESOLVED FIXED in mozilla1.4alpha



18 years ago
6 years ago


(Reporter: jruderman, Assigned: brendan)


({csectype-sop, js1.5})

Windows 98
csectype-sop, js1.5

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [sg:mustfix])


(6 attachments, 2 obsolete attachments)



18 years ago
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?).


18 years ago
Group: netscapeconfidential?

Comment 1

18 years ago
Posted file attacker.html
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

Comment 10

17 years ago
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?

If you look at the testcase, Jesse is doing 'x ="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

17 years ago
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?

Brendan, are you working on this issue? If not, please pass it back to me or peterv.
Assignee: mstoltz → brendan

Comment 14

17 years ago
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)?

Assignee: brendan → mstoltz

Comment 15

17 years ago
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 → ---

Comment 18

16 years ago
I'll take this one back, it requires core JS engine work of some kind.

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.

Comment 20

16 years ago
Posted 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"

- It should not assert that the id passed in is a jsval denoting the string
- 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).


Comment 21

16 years ago
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.


Comment 22

16 years ago
Comment on attachment 116149 [details] [diff] [review]
proposed js/src patch

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

Attachment #116149 - Flags: review?(shaver)


16 years ago
Attachment #116150 - Flags: superreview?(jst)
Attachment #116150 - Flags: review?(mstoltz)

Comment 23

16 years ago
Sorry, Phil wasn't cc'd.  See comment #20.


Comment 24

16 years ago
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.


Comment 25

16 years ago
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

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 ?:.

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
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
Attachment #116150 - Flags: review?(mstoltz) → review+

Comment 29

16 years ago
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.


Comment 30

16 years ago
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

16 years ago
Posted file Timing test #1 (obsolete) —


16 years ago
Attachment #116250 - Attachment is obsolete: true

Comment 33

16 years ago
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

16 years ago
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.)

Comment 37

16 years ago
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

16 years ago
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.

Attachment #116149 - Attachment is obsolete: true


16 years ago
Attachment #116149 - Flags: review+


16 years ago
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.)

Attachment #116437 - Flags: review?(shaver) → review+

Comment 40

16 years ago
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.


Comment 41

16 years ago
Fixed, for sure.  I tested using apache running on my localhost.

Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 42

16 years ago
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.


16 years ago
Group: security


6 years ago
Keywords: csec-sop
You need to log in before you can comment on or make changes to this bug.