[jsdbg2] addDebuggee should not accept non-globals

RESOLVED FIXED in Firefox 19

Status

--
major
RESOLVED FIXED
6 years ago
4 months ago

People

(Reporter: jimb, Assigned: jimb)

Tracking

({dev-doc-needed})

Trunk
Firefox 19
dev-doc-needed
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

(Assignee)

Description

6 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

6 years ago
Severity: normal → major
(Assignee)

Comment 1

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

Comment 2

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

Updated

6 years ago
Duplicate of this bug: 740996
(Assignee)

Comment 4

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

Comment 5

6 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

6 years ago
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.
Attachment #668667 - Flags: review?(jorendorff)
(Assignee)

Comment 7

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

Comment 8

6 years ago
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.
Attachment #666795 - Attachment is obsolete: true
Attachment #668670 - Flags: review?(jorendorff)
(Assignee)

Comment 9

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

Comment 10

6 years ago
Created attachment 669663 [details] [diff] [review]
Use Debugger.Object.prototype.makeDebuggeeValue where appropriate, not Debugger.prototype.addDebuggee.

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

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

Comment 12

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

Will r? when Try looks good.
(Assignee)

Comment 13

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

Comment 14

6 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

6 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

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

Comment 17

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

Updated

6 years ago
Attachment #669665 - Attachment is obsolete: true
(Assignee)

Updated

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

Updated

6 years ago
Attachment #669959 - Flags: review?(jorendorff)

Comment 18

6 years ago
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 19

6 years ago
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: 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.

Updated

5 years ago
Keywords: dev-doc-needed

Updated

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