[jsdbg2] addDebuggee should not accept non-globals

RESOLVED FIXED in Firefox 19

Status

defect
--
major
RESOLVED FIXED
7 years ago
11 months ago

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)

Assignee

Description

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

Updated

7 years ago
Severity: normal → major
Assignee

Comment 1

7 years ago
I'd completely forgotten about bug 740996; trying to figure out how that relates.
Assignee

Comment 2

7 years ago
Seems like bug 740996 comment 6 agrees with the approach taken here. Marking that as dup of this.
Assignee

Updated

7 years ago
Duplicate of this bug: 740996
Assignee

Comment 4

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

Comment 5

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

Comment 6

7 years ago
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)
Assignee

Comment 7

7 years ago
This patch, adding testing functions, is lifted unchanged from bug 794726. Not asking for review until I address the comments there.
Assignee

Comment 8

7 years ago
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)
Assignee

Comment 9

7 years ago
The tests for the revised patches use the functions added in bug 799272.
Depends on: 799272
Assignee

Comment 10

7 years ago
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)
Assignee

Comment 11

7 years ago
Tests revised to use functions from bug 799272 instead of testing-only weird stuff.
Assignee

Comment 12

7 years ago
Try: https://tbpl.mozilla.org/?tree=Try&rev=6e1e43bfbbf3

Will r? when Try looks good.
Assignee

Comment 13

7 years ago
This causes a lot of failures in the debugger mochitests. So, clearly something requires attention...
Assignee

Comment 14

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

Comment 15

7 years ago
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);
Assignee

Comment 16

7 years ago
Indeed, innerizing the window seems to help. New try:
https://tbpl.mozilla.org/?tree=Try&rev=3e6babd17844
Assignee

Updated

7 years ago
Attachment #669665 - Attachment is obsolete: true
Assignee

Updated

7 years ago
Attachment #669663 - Flags: review?(jorendorff)
Assignee

Updated

7 years ago
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+
https://hg.mozilla.org/mozilla-central/rev/567352805e77
https://hg.mozilla.org/mozilla-central/rev/19abbe2af7cf
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Comment on attachment 771326 [details] [diff] [review]
[jsdbg2] addDebuggee should not accept non-globals

Rebased attachment 669959 [details] [diff] [review] against b2g18.

Updated

6 years ago
Keywords: dev-doc-needed

Updated

11 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.