[jsdbg2] addDebuggee should not accept non-globals

RESOLVED FIXED in Firefox 19

Status

defect
--
major
RESOLVED FIXED
7 years ago
Last year

People

(Reporter: jimb, Assigned: jimb)

Tracking

({dev-doc-needed})

Trunk
Firefox 19
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

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.
Severity: normal → major
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.
Duplicate of this bug: 740996
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
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.
Attachment #668667 - Flags: review?(jorendorff)
This patch, adding testing functions, is lifted unchanged from bug 794726. Not asking for review until I address the comments there.
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.
Attachment #666795 - Attachment is obsolete: true
Attachment #668670 - Flags: review?(jorendorff)
The tests for the revised patches use the functions added in bug 799272.
Depends on: 799272
Minor revisions, I think.
Attachment #668667 - Attachment is obsolete: true
Attachment #668668 - Attachment is obsolete: true
Attachment #668670 - Attachment is obsolete: true
Attachment #668667 - Flags: review?(jorendorff)
Attachment #668670 - Flags: review?(jorendorff)
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.
Here's what I'm using in the scratchpad to mess with this:

/*
 * This is a JavaScript Scratchpad.
 *
 * Enter some JavaScript, then Right Click or choose from the Execute Menu:
 * 1. Run to evaluate the selected text (Ctrl+R),
 * 2. Inspect to bring up an Object Inspector on the result (Ctrl+I), or,
 * 3. Display to insert the result in a comment after the selection. (Ctrl+L)
 */

var Cc = Components.classes;
var Ci = Components.interfaces;
var windowMediator = Cc["@mozilla.org/appshell/window-mediator;1"]
  .getService(Ci.nsIWindowMediator);
var top = windowMediator.getMostRecentWindow("navigator:browser");
var win = top.getBrowser().selectedBrowser.contentWindow.wrappedJSObject;


var dbg = new Debugger();
dbg.addDebuggee(win);
Indeed, innerizing the window seems to help. New try:
https://tbpl.mozilla.org/?tree=Try&rev=3e6babd17844
Attachment #669665 - Attachment is obsolete: true
Attachment #669663 - Flags: review?(jorendorff)
Attachment #669959 - Flags: review?(jorendorff)
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+
Comment on attachment 771326 [details] [diff] [review]
[jsdbg2] addDebuggee should not accept non-globals

Rebased attachment 669959 [details] [diff] [review] against b2g18.
Keywords: dev-doc-needed
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.