Closed Bug 1403536 Opened 7 years ago Closed 6 years ago

Protect all protocol request methods against unsafe objects

Categories

(DevTools :: Console, defect, P2)

Product:
DevTools
Component:
Console
Type:
defect

Tracking

(firefox57 wontfix, firefox58 fixed, firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed
firefox59 --- fixed

People

(Reporter: Oriol, Assigned: Oriol)

References

Details

Attachments

(2 files, 1 obsolete file)

Bug 1403536 - Protect all protocol request methods against unsafe objects
59 bytes, text/x-review-board-request
ochameau
: review+
Details
beta-patch.patch
51.49 KB, patch
Details | Diff | Splinter Review 
The Object Inspector now uses other methods than onPrototypeAndProperties, and they can throw for various browsers.

For example, run this code in a privileged web console:

    var sandbox = Cu.Sandbox(null);
    Cu.nukeSandbox(sandbox);
    inspect(sandbox);

The browser console shows

getProperty threw an exception: TypeError: can't access dead object
Stack: exports.getProperty@resource://devtools/shared/base-loader.js -> resource://devtools/shared/DevToolsUtils.js:189:20
enumArrayProperties@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/object.js:869:16
PropertyIteratorActor@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/object.js:819:21
onEnumProperties@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/object.js:247:17
onPacket@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:1792:15
receiveMessage@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/transport.js:761:7
Line: 189, column: 20


error occurred while processing 'enumProperties: TypeError: can't access dead object
Stack: enumArrayProperties@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/object.js:874:17
PropertyIteratorActor@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/object.js:819:21
onEnumProperties@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/object.js:247:17
onPacket@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:1792:15
receiveMessage@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/transport.js:761:7
Line: 874, column: 17


onPacket threw an exception: Error: Server did not specify an actor, dropping packet: {"error":"unknownError","message":"error occurred while processing 'enumProperties: TypeError: can't access dead object\nStack: enumArrayProperties@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/object.js:874:17\nPropertyIteratorActor@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/object.js:819:21\nonEnumProperties@resource://devtools/shared/base-loader.js -> resource://devtools/server/actors/object.js:247:17\nonPacket@resource://devtools/shared/base-loader.js -> resource://devtools/server/main.js:1792:15\nreceiveMessage@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/transport.js:761:7\nLine: 874, column: 17"}
Stack: onPacket@resource://devtools/shared/base-loader.js -> resource://devtools/shared/client/main.js:953:9
send/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/transport/transport.js:570:13
exports.makeInfallible/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14
exports.makeInfallible/<@resource://devtools/shared/base-loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:109:14
Line: 953, column: 9
I meant "various objects", not browsers.
This sounds like a great idea, would you be able to take this one?
status-firefox57: --- → fix-optional
Priority: -- → P2
Yes, I think I already have the fix, but this will need various new tests.

Is there a way to create a CPOW from the server tests?
Assignee: nobody → oriol-bugzilla
Status: NEW → ASSIGNED
(In reply to Oriol Brufau [:Oriol] from comment #3)
> Yes, I think I already have the fix, but this will need various new tests.
> 
> Is there a way to create a CPOW from the server tests?

I think it depends on the type of test and what you need to test. For instance, do you need to set up a debugger server or can you test DevToolsUtils directly? I had some difficulty with this in https://bugzilla.mozilla.org/show_bug.cgi?id=1382833#c24 and ended up just adding a browser-chrome test (see this comment also: https://hg.mozilla.org/integration/autoland/rev/020aa7f3789c#l1.52).

There is an example using CPOWs in the dom tests (see below), but it didn't work for my particular case - maybe it will work out better for you and you can use the pattern:

https://dxr.mozilla.org/mozilla-central/source/dom/base/test/chrome/test_cpows.xul
https://dxr.mozilla.org/mozilla-central/source/dom/base/test/chrome/cpows_parent.xul
https://dxr.mozilla.org/mozilla-central/source/dom/base/test/chrome/cpows_child.js
I wanted to do a server unit test, but I don't think I can get a CPOW there. I guess I will do something like browser_console.js as you suggest. These webconsole tests won't be removed with the old console or something like that, right?
(In reply to Oriol Brufau [:Oriol] from comment #5)
> I wanted to do a server unit test, but I don't think I can get a CPOW there.
> I guess I will do something like browser_console.js as you suggest. These
> webconsole tests won't be removed with the old console or something like
> that, right?

We will be porting over the old console mochitests (including browser console tests) - meta bug 1400847. Depending on what you need to add you could even just put new assertions into browser_console.js, it seems like they would fit fine there.
Comment on attachment 8913402 [details]
Bug 1403536 - Protect all protocol request methods against unsafe objects

https://reviewboard.mozilla.org/r/184734/#review189910

I removed most try statements in favor of the new `DevToolsUtils.isSafeDebuggerObject`.

::: devtools/server/actors/object.js:2068
(Diff revision 1)
> +      }
> +      return "<inaccessible>";
> +    } else if (unwrapped.isProxy) {
> +      return "<proxy>";
> +    } else if (obj.class == "DeadObject") {
> -    return "<dead object>";
> +      return "<dead object>";

`<dead object>` and `<cpow>` are from http://searchfox.org/mozilla-central/rev/f54c1723befe6bcc7229f005217d5c681128fcad/js/ipc/JavaScriptLogging.h#112,118. For consistency I also used `<inaccessible>` and `<proxy>`, but `"[object " + obj.class + "]"` could be used too.

::: devtools/server/tests/unit/test_objectgrips-20.js:42
(Diff revision 1)
> +  callback();
> +}
> +
> +async function test_unsafe_grips(principal) {
> +  // Obtaining a CPOW here does not seem possible, so the CPOW test is in
> +  // devtools/client/webconsole/test/browser_console.js

I didn't place all tests in browser_console.js because it runs code in the browser console, but I also needed a non-privileged debuggee.

::: devtools/server/tests/unit/test_objectgrips-20.js:108
(Diff revision 1)
> +
> +          response = await objClient.getPrototype();
> +          check_prototype(response.prototype, !i, data);
> +
> +          response = await objClient.getDisplayString();
> +          check_display_string(response.displayString, !i, data);

I didn't test some request methods because they enforce the grip to have a Promise or a Function class.

::: devtools/server/tests/unit/test_safe-getter.js:40
(Diff revision 1)
> +    obj;
> +  `)));
> +
> +  // After waiving Xrays, the object has 2 wrappers. Both must be removed
> +  // in order to detect that the getter is not safe.
> +  assert(!DevToolsUtils.hasSafeGetter(obj.getOwnPropertyDescriptor("bar")));

While I was at it, I noticed that unsafe getters with two wrappers were considered safe, because they were unwrapped only once. Now fixed.

::: devtools/shared/DevToolsUtils.js:272
(Diff revision 1)
> +
> +  // Dead objects throw when accessing properties.
> +  if (obj.class == "DeadObject") {
> +    return false;
> +  }
> +

If a new unsafe object is discovered, adding it here it should be enough to handle it everywhere.
Comment on attachment 8913402 [details]
Bug 1403536 - Protect all protocol request methods against unsafe objects

https://reviewboard.mozilla.org/r/184736/#review190700

The problem is that cross-origin Window and Location objects do expose a small set of properties. Inspecting them shouldn't be prevented. So this needs a different approach.
Attachment #8913402 - Flags: review?(jimb)
Basically, the changes are

 - Invisible-to-debugger objects now have a "InvisibleToDebugger: class" class (similar to CPOWs). They are still not accessed.
 - Objects belonging to a global which is not subsumed now have a "Restricted" class instead of "Inaccessible", because in some case they may expose properties. A preview is created like for normal objects, using the original class (either "Object" or "Function").
 - Internal method calls are protected with DevToolsUtils.isSafeDebuggerObject against proxy and invisible-to-debugger objects, and with a try statement in case a "Restricted" or a WrappedNative throws.
 - For object stringification, I used "<dead object>" and "<cpow>" from http://searchfox.org/mozilla-central/rev/f54c1723befe6bcc7229f005217d5c681128fcad/js/ipc/JavaScriptLogging.h#112,118. For consistency I also used `<invisibleToDebugger>` and `<proxy>`.
I forgot to say that suggestions for the grip classes or the stringifications are welcome.

Anyways, some people already complained the current way of treating objects that can't be unwrapped is too restrictive. The patch fixes this, so it would be good if it could be reviewed and land before firefox 58 becomes beta.
I added a test for cross-origin Window and Location objects, which expose some properties.
Jim seems occupied with other things. Who else can review this?
Flags: needinfo?(bgrinstead)
Rebased patch.
(In reply to Oriol Brufau [:Oriol] from comment #15)
> Jim seems occupied with other things. Who else can review this?

Alex, would you be able to review this ObjectActor change?
Flags: needinfo?(bgrinstead) → needinfo?(poirot.alex)
Thanks Oriol, that's a quite significant patch which needs quite some time to digest.
I'll get back to you on Monday.
Flags: needinfo?(poirot.alex)
Attachment #8913402 - Flags: review?(jimb) → review?(poirot.alex)
Thanks! Rebased the patch.
(In reply to Oriol Brufau [:Oriol] from comment #12)
> Anyways, some people already complained the current way of treating objects
> that can't be unwrapped is too restrictive.

I tried to see what changes with your patch, but couldn't see many differences.

Comment 0 STR:
    var sandbox = Cu.Sandbox(null);
    Cu.nukeSandbox(sandbox);
    inspect(sandbox);

Works fine on Nightly. No particular error.

Accessing to `gBrowser.mCurrentBrowser.contentWindowAsCPOW` works also fine on Nightly.
(Object inspection doesn't work, objects are reported as with no properties/symbols)

The only differences I see are:
* with Proxies:
  p = new Proxy({}, function () {});
  Displays:
    Proxy { , 2 more… }  // on Nightly
    Proxy { <target>: Object, <handler>: p<() }  // with your patch

* with cross compartment objects:
  On this url: data:text/html,<iframe src="http://techno-barje.fr"></iframe>
  Evaluating this in the web console: document.body.firstChild.contentWindow.title
  Displays:
    Inaccessible // on Nightly
    Restricted http://techno-barje.fr/ // with your patch
  But the object inspection seems to be equivalent.
Comment on attachment 8913402 [details]
Bug 1403536 - Protect all protocol request methods against unsafe objects

https://reviewboard.mozilla.org/r/184736/#review208486

Thanks for this patch, you are contributing to one of the most complex patch of this actor...
It looks good to me, I mostly have comments to simplify the test comprehension.

::: devtools/server/actors/object.js:107
(Diff revision 5)
>      }
>  
> -    // Dead objects also deny access.
> -    if (this.obj.class == "DeadObject") {
> -      g.class = "DeadObject";
> -      return g;
> +    // If the debuggee does not subsume the object's compartment, most properties won't
> +    // be accessible. Cross-orgin Window and Location objects might expose some, though.
> +    // Change the displayed class, but when creating the preview use the original one.
> +    if (unwrapped === null) {

Does isSafeDebuggerObject return false for DeadObject? If yes, that would be worth commenting in isSafeDebuggerObject and unwrap.

::: devtools/server/actors/object.js:2136
(Diff revision 5)
>   * @return String
>   *         The stringification for the object.
>   */
>  function stringify(obj) {
> -  if (obj.class == "DeadObject") {
> -    const error = new Error("Dead object encountered.");
> +  if (!DevToolsUtils.isSafeDebuggerObject(obj)) {
> +    let unwrapped = DevToolsUtils.unwrap(obj);

You may call unwrap only after the `if isCPOW` check as you don't need it for CPOW.

::: devtools/server/actors/object.js:2137
(Diff revision 5)
>   *         The stringification for the object.
>   */
>  function stringify(obj) {
> -  if (obj.class == "DeadObject") {
> -    const error = new Error("Dead object encountered.");
> -    DevToolsUtils.reportException("stringify", error);
> +  if (!DevToolsUtils.isSafeDebuggerObject(obj)) {
> +    let unwrapped = DevToolsUtils.unwrap(obj);
> +    if (DevToolsUtils.isCPOW(obj)) {

It isn't clear that isSafeDebuggerObject will return false for CPOW. Could you clarify that?
Same for `unwrap` it used to return `null`. It seems to return `undefined` now?
(could you document that in these functions?)

::: devtools/server/actors/object.js:2144
(Diff revision 5)
> +    } else if (unwrapped === undefined) {
> +      return "<invisibleToDebugger>";
> +    } else if (unwrapped.isProxy) {
> +      return "<proxy>";
> +    }
> +    return "[object " + obj.class + "]";

Given that isSafeDebuggerObject returns false only when unwrapped is undefined of unwrrapped.isProxy is true, it looks like we can never reach this line.
Should we throw? log a warning?
At the very least you should add a comment.

::: devtools/server/tests/unit/test_objectgrips-21.js:79
(Diff revision 5)
> +    hasOwnPropertyNames: false,
> +
> +    // Whether the object has some own symbol properties.
> +    hasOwnPropertySymbols: false,
> +
> +    // The descriptor obtained when retrieving property "x" or Symbol("x").

You should say a word here or before about how this test works. You should say that you are not inspecting `obj`, but another special object that inherits from `obj`.

::: devtools/server/tests/unit/test_objectgrips-21.js:213
(Diff revision 5)
> +      Services.appShell.createWindowlessBrowser().document.defaultView.location;
> +    \`)`,
> +    hasOwnPropertyNames: true,
> +    hasOwnPropertySymbols: true,
> +    property: descriptor({value: "SecurityError"}),
> +  }];

All nullPrincipalTests's code attributes are like this:
```
  code : "var obj = privileged(\`${JS}\`)",
```
Could you make it so that it becomes:
  code: `${JS}`,
?

::: devtools/server/tests/unit/test_objectgrips-21.js:215
(Diff revision 5)
> +    hasOwnPropertyNames: true,
> +    hasOwnPropertySymbols: true,
> +    property: descriptor({value: "SecurityError"}),
> +  }];
> +
> +  let tests = principal === systemPrincipal ? systemPrincipalTests : nullPrincipalTests;

You should refactor run_test_with_server,
move symtemPrincipalTests and nullPrincipalTests out of this function.

```
async function run_test_with_server(server, callback) {
  ...
  let principals = {
    "test-grips-system-principal": systemPrincipal, systemPrincipalTests,
    "test-grips-null-principal": null, nullPrincipalTests,
  };
  for (let [title, principal, tests] of Object.entries(principals)) {
    ...
    await test_unsafe_grips(principal, tests);
```

::: devtools/server/tests/unit/test_objectgrips-21.js:259
(Diff revision 5)
> +
> +          response = await objClient.getDisplayString();
> +          check_display_string(response.displayString, data, !i);
> +
> +          grip.class = "Function";
> +          objClient = gThreadClient.pauseGrip(grip);

Please add a comment to explain why you are changing the class here.

::: devtools/server/tests/unit/test_objectgrips-21.js:266
(Diff revision 5)
> +          try {
> +            response = await objClient.getParameterNames();
> +            check_parameter_names(response.parameterNames, data, !i);
> +          } catch (err) {
> +            check_function_error(err.error, data, !i);
> +          }

These checks are really hard to follow.
I think it would be clearer if you inline check_parameter_names and check_function_error.
It looks like getParameterNames sometimes throw, but not always. You had to dig into check_function_error to know.

try {
  response = await objClient.getParameterNames();
  ok(data.isFunction, ...);
  strictEqual(response.parameterNames, undefined, "Undefined parameter names.");
} catch (err) {
  ok(!data.isFunction, ...);
  strictEqual(err, "objectNotFunction", "The object is not a function.");
}

::: devtools/server/tests/unit/test_objectgrips-21.js:278
(Diff revision 5)
> +          }
> +
> +          try {
> +            response = await objClient.getScope();
> +            ok(false, "The above failed");
> +            check_definition_site(response, data, !i);

Looks like a leftover.

::: devtools/server/tests/unit/test_objectgrips-21.js:284
(Diff revision 5)
> +          } catch (err) {
> +            check_function_error(err.error, data, !i, "notDebuggee");
> +          }
> +
> +          grip.class = "Promise";
> +          objClient = gThreadClient.pauseGrip(grip);

It isn't clear what you are trying to test by hacking the grip to be a Promise.
Is all these tests really useful?
If yes, you should comment about what you are doing here.

::: devtools/server/tests/unit/test_objectgrips-21.js:349
(Diff revision 5)
> +  if (isUnsafe) {
> +    strictEqual(grip.class, data.class, "The grip has the proper class.");
> +  } else {
> +    strictEqual(grip.class, "Object", "The grip has 'Object' class.");
> +  }
> +  strictEqual("preview" in grip, data.hasPreview || !isUnsafe, "Check preview presence.");

I think it would be clearer to make `hasPreview` be false for each test, rather than checking for `isUnsafe` here. It is quite misleading to see in test defaults that most have `hasPreview` set to true, whereas they don't have preview after all because they are unsafe.

The same comment applies to:
* `check_grip` with `class`, 
* `check_property_name` with `hasOwnPropertyNames`,
* `check_property` with `property`,
* `check_symbol_names` with `hasOwnPropertySymbol`,
* `check_symbol` with `property`,
* `check_prototype` with `protoType`,
* `check_displat_string` with `string`,
* `check_parameters_names` with `isFunction`,
* `check_function_error` with `isFunction`.

::: devtools/shared/DevToolsUtils.js:272
(Diff revision 5)
> +  if (unwrapped === undefined) {
> +    return false;
> +  }
> +
> +  // If the debuggee does not subsume the object's compartment, most properties won't
> +  // be accessible. Cross-orgin Window and Location objects might expose some, though.

s/Cross-orgin/Cross-origin/
Attachment #8913402 - Flags: review?(poirot.alex)
(In reply to Alexandre Poirot [:ochameau] from comment #23)
> Does isSafeDebuggerObject return false for DeadObject? If yes, that would be
> worth commenting in isSafeDebuggerObject and unwrap.

No, because it seems there is no danger of unadvertedly run code when attempting to access the object, so I treated it like objects from a not subsumed principal. But unlike these, it seems dead objects never expose any properties, so it woukdn't hurt if `isSafeDebuggerObject` returned `false`.

> It isn't clear that isSafeDebuggerObject will return false for CPOW. Could
> you clarify that?
> Same for `unwrap` it used to return `null`. It seems to return `undefined`
> now?
> (could you document that in these functions?)

OK. For CPOWs, `isSafeDebuggerObject` returns `false` and `unwrap` returns `undefined` because CPOWs are in an invisible-to-debugger compartment (bug 1403536).


> Given that isSafeDebuggerObject returns false only when unwrapped is
> undefined of unwrrapped.isProxy is true, it looks like we can never reach
> this line.
> Should we throw? log a warning?
> At the very least you should add a comment.

I added this line just in case `isSafeDebuggerObject` is modified to return `false` for some additional kind of object.

> Please add a comment to explain why you are changing the class here.

OK, it's because object-client.js checks that the class of the grip is "Function", and if it's not, the method in object.js is not called. But some tests have a grip with a class that is not "Function" (e.g. it's "Proxy") but the DebuggerObject has a "Function" class because the object is callable (despite not being a Function object). So I change the grip class in order to test the object.js method.

> It isn't clear what you are trying to test by hacking the grip to be a
> Promise.
> Is all these tests really useful?
> If yes, you should comment about what you are doing here.

This is also to avoid the object-client.js checks and test the object.js methods. But unlike "Function", it seems the class of a DebuggerObject is only "Promise" if the object really is a promise, so they are not much useful.

> I think it would be clearer to make `hasPreview` be false for each test,
> rather than checking for `isUnsafe` here. It is quite misleading to see in
> test defaults that most have `hasPreview` set to true, whereas they don't
> have preview after all because they are unsafe.

Yes, makes sense.
(In reply to Alexandre Poirot [:ochameau] from comment #22)
> (In reply to Oriol Brufau [:Oriol] from comment #12)
> > Anyways, some people already complained the current way of treating objects
> > that can't be unwrapped is too restrictive.
> 
> I tried to see what changes with your patch, but couldn't see many
> differences.

The differences are:

 - Some cases that not show errors in the console will stop doing so because a fail is expected.
   This is what happens with

    var sandbox = Cu.Sandbox(null);
    Cu.nukeSandbox(sandbox);
    inspect(sandbox);

 - Less restrictive behavior for objects that are not subsumed. They have a Restricted class instead of Inaccessible, and have a normal preview, because cross-origin Window and Location objects are partially accessible.

 - Various protections against unintended code execution, especially with proxies from an invisible-to-debugger compartment.

 - Ability to use `inspect(Cu)` and `Cu.Sandbox`
(In reply to Alexandre Poirot [:ochameau] from comment #23)
> I think it would be clearer to make `hasPreview` be false for each test,
> rather than checking for `isUnsafe` here. It is quite misleading to see in
> test defaults that most have `hasPreview` set to true, whereas they don't
> have preview after all because they are unsafe.

If hasPreview is true, then there must be a preview. What this does is,
when hasPreview is false, the unsafe object `obj` does not have a preview,
but the safe object `Object.create(obj)` does.

> The same comment applies to:
> * `check_grip` with `class`, 
> * `check_property_name` with `hasOwnPropertyNames`,
> * `check_property` with `property`,
> * `check_symbol_names` with `hasOwnPropertySymbol`,
> * `check_symbol` with `property`,
> * `check_prototype` with `protoType`,
> * `check_displat_string` with `string`,
> * `check_parameters_names` with `isFunction`,
> * `check_function_error` with `isFunction`.

Still the same, e.g. getOwnPropertyNames fails for a dead object, but it
works for a safe object that inherits from a dead object.

I will add a comment explaining how the tests work, hopefully it will become clear.
(In reply to Oriol Brufau [:Oriol] from comment #24)
> (In reply to Alexandre Poirot [:ochameau] from comment #23)
> > Does isSafeDebuggerObject return false for DeadObject? If yes, that would be
> > worth commenting in isSafeDebuggerObject and unwrap.
> 
> No, because it seems there is no danger of unadvertedly run code when
> attempting to access the object, so I treated it like objects from a not
> subsumed principal. But unlike these, it seems dead objects never expose any
> properties, so it woukdn't hurt if `isSafeDebuggerObject` returned `false`.

It may be helpful to say a word about what happens for DeadObject in these methods.
So, they are considered just like a COW.

> > Please add a comment to explain why you are changing the class here.
> 
> OK, it's because object-client.js checks that the class of the grip is
> "Function", and if it's not, the method in object.js is not called. But some
> tests have a grip with a class that is not "Function" (e.g. it's "Proxy")
> but the DebuggerObject has a "Function" class because the object is callable
> (despite not being a Function object). So I change the grip class in order
> to test the object.js method.

This is really hard to follow. I'm afraid this test may be hard to maintain over time.
Could we execute these asserts only if data.isFunction is true?
That would make it easier to follow. I don't see the point of checking that these methods fail when the object is obviously not a function. Same about the Promise checks.
About overriding the class, may be doing the following check would help:
  // Your comment about class overriding
  if (grip.class == "Proxy") {
    grip.class = "Function";
  }

So at the end we would only do:
  if (data.isFunction) {
    // Your comment about class overriding
    if (grip.class == "Proxy") {
      grip.class = "Function";
    }
    objClient = gThreadClient.pauseGrip(grip);

    response = await objClient.getParameterNames();
    strictEqual(response.parameterNames, undefined, "Undefined parameter names.");
  }
And remove everything else, that checks for exceptions. Same for all Promise checks.
Even then, it isn't clear what you are really trying to assert here.
What does it mean for getParameterNames to not throw and have undefined parameterNames?
If that's only to check that DebuggerObject.class is "Function" on the actor side, then comment about that.
If that's it, you could do:
  try {
    response = await objClient.getParameterNames();
    ok(true, "getParameterNames passed. DebuggerObject.class is Function on the object actor");
  } catch(e) {
    ok(false, "getParameterNames failed. DebuggerObject.class may not be Function on the object actor");
  }

(In reply to Oriol Brufau [:Oriol] from comment #25)
> The differences are:

Thanks for the summary!

>  - Less restrictive behavior for objects that are not subsumed. They have a
> Restricted class instead of Inaccessible, and have a normal preview, because
> cross-origin Window and Location objects are partially accessible.

About this one.
I also tried `document.body.firstChild.contentWindow.location`.
It gives:
  Restricted { … }   // with your patch
  Inaccessible {  }  // without
Is that expected?
Comment on attachment 8913402 [details]
Bug 1403536 - Protect all protocol request methods against unsafe objects

https://reviewboard.mozilla.org/r/184736/#review209404

Thanks again.
It looks good, only have comments about the test.
Feel free to r? again if you want me to read it again.

::: devtools/server/tests/unit/test_objectgrips-21.js:226
(Diff revision 6)
> +  for (let data of tests) {
> +    await new Promise(function (resolve) {
> +      gThreadClient.addOneTimeListener("paused", async function (event, packet) {
> +        let [unsafeGrip, inheritsGrip] = packet.frame.arguments;
> +        for (let [i, grip] of [unsafeGrip, inheritsGrip].entries()) {
> +          check_grip(grip, data, !i);

Ok, I think I've been unecessarely confused by this code.
1/ "unsafeGrip" versus "inheritsGrip"
  To ease following I would have called unsafeGrip objGrip. I didn't realized it was the grip for `obj` but another unsafe way to access inherits object...
2/ `for (let [i, grip] of [unsafeGrip, inheritsGrip].entries())` and `!i` is really confusing.
Something like this would make it easier to read:
```
let [unsafeGrip, inheritsGrip] = packet.frame.arguments;
for (let grip of [unsafeGrip, inheritsGrip]) {
  let isUnsafe = grip == unsafeGrip;
  // You may comment right here, that you use `data` object to make assert against `obj`/`objGrip` (isUnsafe = true), but all checks against `inherits` object are hardcoded as you expect all methods to work the same on it in every test.
  check_grip(grip, data, isUnsafe);
  ...
}
```
Then if you rename unsafeGrip to objGrip, you may also rename isUnsafe.
Attachment #8913402 - Flags: review?(poirot.alex) → review+
(In reply to Alexandre Poirot [:ochameau] from comment #28)
> It may be helpful to say a word about what happens for DeadObject in these
> methods.
> So, they are considered just like a COW.

Not like a CPOW. CPOWs belong to an invisible-to-debugger compartment and thus can't be unwrapped.
DeadObjects have no wrapper so unwrap() returns the same object and isSafeDebuggerObject returns true.
I added comments about this.

Also updated the test.

> About this one.
> I also tried `document.body.firstChild.contentWindow.location`.
> It gives:
>   Restricted { … }   // with your patch
>   Inaccessible {  }  // without
> Is that expected?

Yes, the "…" means there is a preview with some non-enumerable properties.
The preview is more useful with Window objects, then it shows the URL.
(Maybe this should also happen for Location objects, but that's off-topic).
Comment on attachment 8913402 [details]
Bug 1403536 - Protect all protocol request methods against unsafe objects

Just to check all your concerns have been fixed.
Attachment #8913402 - Flags: review+ → review?(poirot.alex)
Comment on attachment 8913402 [details]
Bug 1403536 - Protect all protocol request methods against unsafe objects

https://reviewboard.mozilla.org/r/184736/#review209474

Thanks again, it looks great!

::: devtools/server/tests/unit/test_objectgrips-21.js:227
(Diff revision 7)
> +    await new Promise(function (resolve) {
> +      gThreadClient.addOneTimeListener("paused", async function (event, packet) {
> +        let [objGrip, inheritsGrip] = packet.frame.arguments;
> +        for (let grip of [objGrip, inheritsGrip]) {
> +          let isUnsafe = grip === objGrip;
> +          // If `isUnsafe` is `true, the parameters in `data` will be used to assert

s/`true/true/
Attachment #8913402 - Flags: review?(poirot.alex) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/897f029f8ef6
Protect all protocol request methods against unsafe objects r=ochameau
Keywords: checkin-needed
Backout by btara@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3274f02a1615
Backed out 1 changesets for failing ES Linting tests on /builds/worker/checkouts/gecko/devtools/server/tests/unit/test_objectgrips-21.js r=backout on a CLOSED TREE
Sorry, I was sure I checked both files, but maybe I only checked DevToolsUtils.js
Pushed to try to make sure everything is right.
Backed out 1 changesets (bug 1403536) for failing ES Linting tests on /builds/worker/checkouts/gecko/devtools/server/tests/unit/test_objectgrips-21.js

Backed out 1 changesets (bug 1403536) for failing ES Linting tests on /builds/worker/checkouts/gecko/devtools/server/tests/unit/test_objectgrips-21.js

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=3274f02a16154e9ad7c725a714acca494866051e

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=897f029f8ef6ba05cd2ff3c4ee6db06a8fda9f54&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&selectedJob=148572182&filter-searchStr=Linting%20opt%20source-test-mozlint-eslint%20(ES)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9fb71b0692a6
Protect all protocol request methods against unsafe objects r=ochameau
Keywords: checkin-needed
Backout by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fb0b4727d11d
Backed out changeset 9fb71b0692a6 for devtools-chrome failures on browser_console_dead_objects.js r=backout on a CLOSED TREE
OK the problem is that previously dead objects had no preview, so they were displayed as "[object DeadObject]". Now they have an empty preview, so they are displayed as "DeadObject {  }".
Not sure why this test only runs in Windows 7 debug, I was only testing linux so I didn't see this fail.
Flags: needinfo?(oriol-bugzilla)
Oriol do you think we should handle that on the Reps side as well ? I guess "DeadObject" is not really self explanatory. Maybe we could style it in a different way, with a link to a mdn page giving more explanations about what it is.
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #45)
> Oriol do you think we should handle that on the Reps side as well ? I guess
> "DeadObject" is not really self explanatory. Maybe we could style it in a
> different way, with a link to a mdn page giving more explanations about what
> it is.

For DeadObjects it may not be that necessary because they only occur in privileged code.
But as I said in bug 1394559 comment 24,
> Reps could check whether the grip class is "Restricted" to know when to show
> some kind of message. It could explain that the targeted document is not
> allowed to fully access the object, and suggest using "Select an iframe as
> the current targeted document" to fix it.

This would address bug 1404027.
Comment on attachment 8913402 [details]
Bug 1403536 - Protect all protocol request methods against unsafe objects

I don't know much about treeherder, but I think https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab09fd3f7e2a and https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d90d9d1807d5ac9919265e7e04cc97bf00a8f29&selectedJob=149124212 (I forgot win32 the first time) cover the failure in https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9fb71b0692a6f089dcd0b52a5dccd952b375e42c&filter-resultStatus=testfailed, so it should be right now.
Attachment #8913402 - Flags: review+ → review?(poirot.alex)
Comment on attachment 8913402 [details]
Bug 1403536 - Protect all protocol request methods against unsafe objects

https://reviewboard.mozilla.org/r/184736/#review210506

(In reply to Oriol Brufau [:Oriol] from comment #43)
> OK the problem is that previously dead objects had no preview, so they were
> displayed as "[object DeadObject]". Now they have an empty preview, so they
> are displayed as "DeadObject {  }".
> Not sure why this test only runs in Windows 7 debug, I was only testing
> linux so I didn't see this fail.

Hum, that's surprising. It isn't so much related to windows.
browser_console_dead_objects.js is one of these tests that are disabled on e10s.
And it looks like, we only run devtools test without e10s on win32 debug, so only on Windows 7 debug...

So this try doesn't cover it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab09fd3f7e2a&filter-searchStr=mochitest
because you requested win64, while this one: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d90d9d1807d5ac9919265e7e04cc97bf00a8f29&filter-searchStr=mochitest
ran it as you requested win32.
Attachment #8913402 - Flags: review?(poirot.alex) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/eb9f964d829d
Protect all protocol request methods against unsafe objects r=ochameau
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/eb9f964d829d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Blocks: 1406961
Looks like the vast majority of this patch are just test changes. Is this something we should consider backporting to Beta for the benefit of our DevEdition users?
status-firefox57: fix-optional → wontfix
status-firefox58: --- → affected
Flags: needinfo?(oriol-bugzilla)
I thought this was too big to be uplifted, but yes, it can be good to land in 58 because if fixes some wrong behavior from bug 1394559 which could annoy someone. And the protections against unsafe objects can be useful too.
Flags: needinfo?(oriol-bugzilla)
Attached patch beta-patch.patch (obsolete) — — Splinter Review 
This is a rebase of the patch for beta.

Approval Request Comment
[Feature/Bug causing the regression]: this fixes some regressions from bug 1394559 and some old problems.

[User impact if declined]:
 - Inspecting some unsafe objects will show errors in the browser console (this is old bug).
 - Inspecting some rare objects may cause unintended code execution (this is old bug).
 - Inability to inspect Cu.Sandbox (regression from bug 1394559).
 - Too restrictive behavior for cross-origin Window and Location objects (regression from bug 1394559).

[Is this code covered by automated tests?]: Yes.

[Has the fix been verified in Nightly?]: I have manually verified this in nightly.

[Needs manual test from QE? If yes, steps to reproduce]: it's all covered by automatic tests. But if you want to test:

 - Enter this code in a privileged web console. Should not produce any error in the browser console:

    var sandbox = Cu.Sandbox(null);
    Cu.nukeSandbox(sandbox);
    inspect(sandbox);

 - Enter this code in a privileged web console. Should not produce any error in the browser console:

    var sandbox = Cu.Sandbox(Cu.getObjectPrincipal({}), {invisibleToDebugger: true});
    sandbox.eval("var p = Proxy.revocable({},{}); p.revoke(); p.proxy");

 - Enter this code in a privileged web console. You should see its __proto__

    inspect(Cu.Sandbox)

 - Enter this code in a non-privileged web console (e.g. http://example.com/). Should show "Restricted about:blank" instead of "Inaccessible {  }".

    var iframe = document.createElement("iframe");
    iframe.sandbox = "allow-scripts";
    document.documentElement.appendChild(iframe);
    iframe.contentWindow;

[List of other uplifts needed for the feature/fix]: None.

[Is the change risky?]: Not much risky.

[Why is the change risky/not risky?]: It's a big patch but mostly that's because of the tests. Other than this the changes are basically new code to check against unsafe objects and more try statements.

[String changes made/needed]: None.
Attachment #8936309 - Flags: approval-mozilla-beta?
Comment on attachment 8936309 [details] [diff] [review]
beta-patch.patch

Fix the regression of bug 1394559 and include tests. Beta58+.
Attachment #8936309 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Status: RESOLVED → REOPENED
status-firefox58: fixed → affected
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 7 years ago → 6 years ago
Resolution: --- → FIXED
Attached patch beta-patch.patch — — Splinter Review 
A test used spread properties but it seems that eslint does not like this in Firefox 58. I just used Object.assign instead. The try seems OK.
Attachment #8936309 - Attachment is obsolete: true
Flags: needinfo?(oriol-bugzilla)
I suppose there is no need to request approval-mozilla-beta again if I only fixed a small eslint problem in a test, right?
Keywords: checkin-needed
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: