Use Debugger.Object.prototype.makeDebuggeeValue where appropriate, not Debugger.prototype.addDebuggee.
9.33 KB, patch
|Details | Diff | Splinter Review|
15.96 KB, patch
|Details | Diff | Splinter Review|
17.41 KB, patch
|Details | Diff | Splinter Review|
At present, Debugger.prototype.addDebuggee accepts any sort of object as an argument, not just a global object. When given a non-global, addDebuggee applies JSObject::global and adds that. This behavior undermines the capability-based security model we've been hoping will make Debugger acceptable for use in content some day. Our argument has been that Debugger should reveal only that activity that takes place in the scope of globals it can reach and add as debuggees (with notable exceptions for onNewGlobalObject and findAllGlobals, which are for use by privileged debuggers only, like chrome debuggers). But if addDebuggee applies JSObject::global, that means that the debugger need only obtain any reference to any object in a compartment in order to add its global as a debuggee, a substantially weaker condition.
I'd completely forgotten about bug 740996; trying to figure out how that relates.
Seems like bug 740996 comment 6 agrees with the approach taken here. Marking that as dup of this.
Created attachment 666795 [details] [diff] [review] Identify debuggees only by Debugger.Object instances and securely transparent CCWs. Work in progress. Causes the following tests to fail: -m -n -a /home/jimb/moz/dbg/js/src/jit-test/tests/debug/Debugger-debuggees-07.js -m -n -a /home/jimb/moz/dbg/js/src/jit-test/tests/debug/Debugger-debuggees-08.js -d -m -n -a /home/jimb/moz/dbg/js/src/jit-test/tests/debug/Debugger-debuggees-09.js -m -n -a /home/jimb/moz/dbg/js/src/jit-test/tests/debug/Debugger-findScripts-02.js -m -n -a /home/jimb/moz/dbg/js/src/jit-test/tests/debug/Debugger-findScripts-05.js -m -n -a /home/jimb/moz/dbg/js/src/jit-test/tests/debug/Debugger-findScripts-06.js -m -n -a /home/jimb/moz/dbg/js/src/jit-test/tests/debug/Debugger-findScripts-07.js -m -n -a /home/jimb/moz/dbg/js/src/jit-test/tests/debug/Debugger-findScripts-08.js -m -n -a /home/jimb/moz/dbg/js/src/jit-test/tests/debug/Debugger-findScripts-09.js -m -n -a /home/jimb/moz/dbg/js/src/jit-test/tests/debug/Debugger-findScripts-11.js -m -n -a /home/jimb/moz/dbg/js/src/jit-test/tests/debug/Debugger-findScripts-12.js -m -n -a /home/jimb/moz/dbg/js/src/jit-test/tests/debug/Debugger-findScripts-14.js -m -n -a /home/jimb/moz/dbg/js/src/jit-test/tests/debug/Frame-onPop-11.js -m -n -a /home/jimb/moz/dbg/js/src/jit-test/tests/debug/Object-preventExtensions-01.js -m -n -a /home/jimb/moz/dbg/js/src/jit-test/tests/debug/onEnterFrame-04.js
Assignee: nobody → jimb
Status: NEW → ASSIGNED
More legibly: -m -n -a js/src/jit-test/tests/debug/Debugger-debuggees-07.js -m -n -a js/src/jit-test/tests/debug/Debugger-debuggees-08.js -d -m -n -a js/src/jit-test/tests/debug/Debugger-debuggees-09.js -m -n -a js/src/jit-test/tests/debug/Debugger-findScripts-02.js -m -n -a js/src/jit-test/tests/debug/Debugger-findScripts-05.js -m -n -a js/src/jit-test/tests/debug/Debugger-findScripts-06.js -m -n -a js/src/jit-test/tests/debug/Debugger-findScripts-07.js -m -n -a js/src/jit-test/tests/debug/Debugger-findScripts-08.js -m -n -a js/src/jit-test/tests/debug/Debugger-findScripts-09.js -m -n -a js/src/jit-test/tests/debug/Debugger-findScripts-11.js -m -n -a js/src/jit-test/tests/debug/Debugger-findScripts-12.js -m -n -a js/src/jit-test/tests/debug/Debugger-findScripts-14.js -m -n -a js/src/jit-test/tests/debug/Frame-onPop-11.js -m -n -a js/src/jit-test/tests/debug/Object-preventExtensions-01.js -m -n -a js/src/jit-test/tests/debug/onEnterFrame-04.js
Created attachment 668667 [details] [diff] [review] Use Debugger.Object.prototype.makeDebuggeeValue where appropriate, not Debugger.prototype.addDebuggee. Many tests use Debugger.prototype.addDebuggee to produce Debugger.Object instances referring to random objects. Now we have Debugger.Object.prototype.makeDebuggeeValue to do that, which is more careful about wrappers. This patch changes all the existing tests that abuse addDebuggee in this way (well, they didn't have any choice when they were written) to use makeDebuggeeValue instead, in preparation for tightening up addDebuggee to only accept global objects.
Created attachment 668668 [details] [diff] [review] Add testing-only functions that expose Debugger.Object wrapper structure. This patch, adding testing functions, is lifted unchanged from bug 794726. Not asking for review until I address the comments there.
Created attachment 668670 [details] [diff] [review] Identify debuggees only by Debugger.Object instances and securely transparent CCWs. This patch affects behavior that a lot of debugger tests rely on; I've tried to update the tests without losing coverage of behavior that is still supported: - A prior patch in this series removes uses of addDebuggee to generate Debugger.Object instances referring to random objects, using makeDebuggeeValue instead. - The test debug/Debugger-debuggees-07.js is deleted, because it's testing for the very behavior we're removing. Other tests are trimmed to remove usage that is no longer supported. - The new test debug/Debugger-debuggees-17.js checks that we reject objects that don't designate debuggees. The existing test Debugger-debuggees-06.js checks that non-object values are properly rejected. - The new test debug/Debugger-debuggees-18.js checks that globals are correctly identified regardless of how we designate them.
Created attachment 669663 [details] [diff] [review] Use Debugger.Object.prototype.makeDebuggeeValue where appropriate, not Debugger.prototype.addDebuggee. Minor revisions, I think.
Created attachment 669665 [details] [diff] [review] Identify debuggees only by Debugger.Object instances and CCWs that can be unwrapped securely. Tests revised to use functions from bug 799272 instead of testing-only weird stuff.
Try: https://tbpl.mozilla.org/?tree=Try&rev=6e1e43bfbbf3 Will r? when Try looks good.
This causes a lot of failures in the debugger mochitests. So, clearly something requires attention...
Hmm, I wonder if we've been relying on the "any object" behavior of addDebuggee every time we add a window as a global. After all, you're getting an outer window object, not a global.
Indeed, innerizing the window seems to help. New try: https://tbpl.mozilla.org/?tree=Try&rev=3e6babd17844
Created attachment 669959 [details] [diff] [review] Identify debuggees only by Debugger.Object instances and CCWs that can be unwrapped securely. Revised patch, that innerizes outer windows.
Comment on attachment 669663 [details] [diff] [review] Use Debugger.Object.prototype.makeDebuggeeValue where appropriate, not Debugger.prototype.addDebuggee. Stealing at jimb's request. Good to see this separation of two fundamentally different operations.
Attachment #669663 - Flags: review?(jorendorff) → review+
Comment on attachment 669959 [details] [diff] [review] Identify debuggees only by Debugger.Object instances and CCWs that can be unwrapped securely. Again, this seems like a sensible restriction.
Attachment #669959 - Flags: review?(jorendorff) → review+
Target Milestone: --- → Firefox 19
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Created attachment 771326 [details] [diff] [review] [jsdbg2] addDebuggee should not accept non-globals
Comment on attachment 771326 [details] [diff] [review] [jsdbg2] addDebuggee should not accept non-globals Rebased attachment 669959 [details] [diff] [review] against b2g18.
You need to log in before you can comment on or make changes to this bug.