796073 - [jsdbg2] addDebuggee should not accept non-globals

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[Jim Blandy :jimb]

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.

Comment 1

[Jim Blandy :jimb]

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

Comment 2

[Jim Blandy :jimb]

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

Comment 4

[Jim Blandy :jimb]

Attachment: 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

Comment 5

[Jim Blandy :jimb]

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

Comment 6

[Jim Blandy :jimb]

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

Comment 7

[Jim Blandy :jimb]

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

Comment 8

[Jim Blandy :jimb]

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

Comment 9

[Jim Blandy :jimb]

The tests for the revised patches use the functions added in bug 799272.

Comment 10

[Jim Blandy :jimb]

Attachment: Use Debugger.Object.prototype.makeDebuggeeValue where appropriate, not Debugger.prototype.addDebuggee.

Minor revisions, I think.

Comment 11

[Jim Blandy :jimb]

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

Comment 12

[Jim Blandy :jimb]

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

Will r? when Try looks good.

Comment 13

[Jim Blandy :jimb]

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

Comment 14

[Jim Blandy :jimb]

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.

Comment 15

[Jim Blandy :jimb]

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);

Comment 16

[Jim Blandy :jimb]

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

Comment 17

[Jim Blandy :jimb]

Attachment: Identify debuggees only by Debugger.Object instances and CCWs that can be unwrapped securely.

Revised patch, that innerizes outer windows.

Comment 18

[Luke Wagner [:luke]]

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.

Comment 19

[Luke Wagner [:luke]]

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.

Comment 20

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/567352805e77
https://hg.mozilla.org/integration/mozilla-inbound/rev/19abbe2af7cf

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/567352805e77
https://hg.mozilla.org/mozilla-central/rev/19abbe2af7cf

Comment 22

[Alexandre Poirot [:ochameau]]

Attachment: [jsdbg2] addDebuggee should not accept non-globals

Comment 23

[Alexandre Poirot [:ochameau]]

Comment on attachment 771326 [details] [diff] [review]
[jsdbg2] addDebuggee should not accept non-globals

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