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)
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)
521 bytes,
text/html
|
Details | |
619 bytes,
text/html
|
Details | |
5.47 KB,
patch
|
security-bugs
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
3.40 KB,
text/html
|
Details | |
1.80 KB,
text/plain
|
Details | |
4.18 KB,
patch
|
shaver
:
review+
|
Details | Diff | Splinter Review |
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?).
Reporter | ||
Updated•23 years ago
|
Group: netscapeconfidential?
Reporter | ||
Comment 1•23 years ago
|
||
Reporter | ||
Comment 2•23 years ago
|
||
Updated•23 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla0.9.4
Comment 3•23 years ago
|
||
Not critical for 0.9.4, moving to 0.9.5.
Group: netscapeconfidential?
Target Milestone: mozilla0.9.4 → mozilla0.9.5
Comment 4•23 years ago
|
||
time marches on. Retargeting to 0.9.6.
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Comment 5•23 years ago
|
||
Moving the most time-critical bugs and minor security fixes to 0.9.7
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Comment 6•23 years ago
|
||
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?
Updated•23 years ago
|
Target Milestone: mozilla0.9.7 → mozilla1.0.1
Comment 7•22 years ago
|
||
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
Comment 8•22 years ago
|
||
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.
Updated•22 years ago
|
Target Milestone: mozilla1.0.1 → mozilla1.2alpha
Updated•22 years ago
|
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
Assignee | ||
Comment 10•22 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? /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.
Whiteboard: [sg:mustfix]
Assignee | ||
Comment 12•22 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? /be
Comment 13•22 years ago
|
||
Brendan, are you working on this issue? If not, please pass it back to me or peterv.
Assignee: mstoltz → brendan
Status: ASSIGNED → NEW
Assignee | ||
Comment 14•22 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)? /be
Assignee: brendan → mstoltz
Reporter | ||
Comment 15•22 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?).".
Comment 16•21 years ago
|
||
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.
Assignee | ||
Comment 18•21 years ago
|
||
I'll take this one back, it requires core JS engine work of some kind. /be
Comment 19•21 years ago
|
||
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.
Assignee | ||
Comment 20•21 years ago
|
||
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
Assignee | ||
Comment 21•21 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. /be
Assignee | ||
Comment 22•21 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, /be
Attachment #116149 -
Flags: review?(shaver)
Assignee | ||
Updated•21 years ago
|
Attachment #116150 -
Flags: superreview?(jst)
Attachment #116150 -
Flags: review?(mstoltz)
Assignee | ||
Comment 23•21 years ago
|
||
Sorry, Phil wasn't cc'd. See comment #20. /be
Assignee | ||
Comment 24•21 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. /be
Assignee | ||
Comment 25•21 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 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•21 years ago
|
||
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 28•21 years ago
|
||
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+
Assignee | ||
Comment 29•21 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. /be
Status: NEW → ASSIGNED
Comment 30•21 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•21 years ago
|
||
Comment 32•21 years ago
|
||
Updated•21 years ago
|
Attachment #116250 -
Attachment is obsolete: true
Comment 33•21 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•21 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 36•21 years ago
|
||
Comment 37•21 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).
Assignee | ||
Comment 38•21 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. /be
Attachment #116149 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #116149 -
Flags: review+
Assignee | ||
Updated•21 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.) r=shaver
Attachment #116437 -
Flags: review?(shaver) → review+
Assignee | ||
Comment 40•21 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. /be
Assignee | ||
Comment 41•21 years ago
|
||
Fixed, for sure. I tested using apache running on my localhost. /be
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Comment 42•21 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.
Reporter | ||
Updated•21 years ago
|
Group: security
You need to log in
before you can comment on or make changes to this bug.
Description
•