1248631 - The definition of "safe getter" in DevToolsUtils.hasSafeGetter broken

Status: NEW

(DevTools :: Debugger, task, P3)

Description

[Boris Zbarsky [:bzbarsky]]

DevToolsUtils.hasSafeGetter defines a safe getter like so:

  fn && fn.callable && fn.class == "Function" && fn.script === undefined

but this is absolutely no guarantee of safety.  For a simple example:

1)  Load the attached testcase.
2)  Open the debugger.
3)  Breakpoint on line 7 (the "x.click()" line).
4)  Click the twistie next to "x: <div>" in the "Function scope" section on the
    right (under "Variables").

EXPECTED RESULTS: No change in the web page.

ACTUAL RESULTS: The text disappears.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Another testcase

This one works on tip because we started self-hosting Function.prototype.bind, but on release (and anything before Firefox 46) following the steps from comment 0 on this testcase happily alerts "HELLO".

Comment 3

[Boris Zbarsky [:bzbarsky]]

It might be best to restrict the definition of safe getter to just DOM getters.  And even then, some of them might have side-effects...

Comment 4

[Bobby Holley (:bholley)]

What on earth is 'safe' supposed to mean in this context?

Comment 5

[Bobby Holley (:bholley)]

Oh, from the comment:

> Determines if a descriptor has a getter which doesn't call into JavaScript.

That's totally not enough to determine that the function is safe to call, because it still doesn't imply that the getter is the one you expect. For example, you can work a lot of mischief with .bind-ing native functions to other native objects.

Doesn't the JS debugger API provide a mechanism for carefully inspecting untrusted objects for cases just like this?

Comment 6

[Boris Zbarsky [:bzbarsky]]

I think the idea here is to identify getters which it's safe to call (in the sense of not having page-observable side-effects) and present those properties in the debugger UI as values (the return value of the getter), not as getter functions.  Basically, try to provide better UX when we can.  As a simple example, show an actual node for the .parentNode of a DOM node.

Note that bind() being self-hosted means that bound functions are automatically considered "unsafe" on tip (see comment 2), so we sort of luck out there.  But that's a dependency on implementation details of bind() that we shouldn't rely on, of course.

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Boris Zbarsky [:bz] from comment #6)
> I think the idea here is to identify getters which it's safe to call (in the
> sense of not having page-observable side-effects) and present those
> properties in the debugger UI as values (the return value of the getter),
> not as getter functions.  Basically, try to provide better UX when we can. 
> As a simple example, show an actual node for the .parentNode of a DOM node.

Yes, this is the goal.

Comment 8

[Boris Zbarsky [:bzbarsky]]

In that case, I really do think you should restrict this to things that have a jitinfo and whose jitinfo OpType is Getter.  Also possibly to the ones whose AliasSet is AliasNone or AliasDOMSets, though if you're OK with side-effects like flushing layout then possibly any Getter-type jitinfo is OK.

Comment 9

[Eddy Bruel [:ejpbruel]]

(In reply to Boris Zbarsky [:bz] from comment #8)
> In that case, I really do think you should restrict this to things that have
> a jitinfo and whose jitinfo OpType is Getter.  Also possibly to the ones
> whose AliasSet is AliasNone or AliasDOMSets, though if you're OK with
> side-effects like flushing layout then possibly any Getter-type jitinfo is
> OK.

I'm fine with that. I would rather be too restrictive than have unexpected side effects.

Comment 10

[Jim Blandy :jimb]

The original motivation for this is that it wasn't even possible to show the x and y coordinates of a mouse click event if we refused to call getters altogether (which was the original behavior). So that "safe getter" code should probably have always had a bug like this filed against it, from the moment it was landed.

I'm a little uncomfortable exposing such moz-specific things as jitinfo OpType via Debugger; we otherwise expose very little implementation-specific detail.

If Debugger changes are necessary, perhaps the best thing would be for Debugger.Object.prototype.getOwnPropertyDescriptor to add a "noSideEffects" property to the returned descriptor when "get" is present and Debugger can determine that it's safe to call.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Note that the x/y of a MouseEvent (well, clientX/screenX/etc) are NOT marked AliasNone or AliasDOMSets  They _could_ be, I think, but doing that involves manual annotations which no one has added here.

Comment 12

[Eddy Bruel [:ejpbruel]]

This is one of those things that doesn't lead to problems for users most of the time. However, when things do go wrong, they go spectacularly wrong: calling a getter that is not safe to be called can cause client code with observable side-effects to run while the debugger is paused.

I'm marking this bug as P1 because making the debugger behave correctly should be our biggest priority after (service) worker debugging.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Note that my biggest worry here is actually malicious debuggees: sites that want to prevent you inspecting/debugging them and deliberately nuke the world if you try to...

Comment 14

[Jim Blandy :jimb]

(In reply to Boris Zbarsky [:bz] from comment #13)
> Note that my biggest worry here is actually malicious debuggees: sites that
> want to prevent you inspecting/debugging them and deliberately nuke the
> world if you try to...

It is 100% within Debugger's charter to provide ways for its clients to inspect arbitrary objects without causing debuggee code to run. In fact, it's a bug (perhaps a design flaw; perhaps just an ordinary implementation bug) if any action via the Debugger API not documented to run the debuggee does so.

The two questions here are:

- Is it possible to recognize a large enough set of safe getters without admitting any false positives to give access to values developers need to see? (Mouse event positions serve as a "charismatic species" here.)

- If so, how should we expose that predicate via the Debugger API?

Boris, it sounds like you're saying that JSJitInfo::AliasSet is one way to answer the first question. Is that right? Does my suggestion in the bottom sentence of my comment 10 seem like a reasonably non-implementation-specific way to expose that?

Comment 15

[Boris Zbarsky [:bzbarsky]]

JSJitInfo::AliasSet certainly provides a way to recognize getters that are guaranteed to be safe in the sense of having no side-effects apart from possibly throwing exceptions, which I expect the debugger catches.  A problem is that a sufficiently-restrictive alias set is opt-in on the part of the IDL author and that things that _could_ opt in may not have (e.g. mouse event positions have not; I haven't checked whether they could).  Adding these opt-in annotations involves some manual analysis which can be a bit tricky...

Which is all deeply unsatisfying, of course.  :(

In terms of debugger API, comment 10 sounds reasonable to me.  With the obvious caveat that I don't know much about the debugger API, of course.  But it would certainly allow us to transparently change the underlying determination of "noSideEffects" if we want to do so.

Comment 16

[Jim Blandy :jimb]

Okay, so the next steps in that course would be:

- Specify, implement, and test the new Debugger noSideEffects descriptor property.
- Change DevToolsUtils.hasSafeGetter to use it, or just ditch the indirection: delete hasSafeGetter
  entirely and make its callers check the noSideEffects property directly.
- Add necessary annotations in WebIDL to maintain the devtools' present level of service.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Sounds good.  Or we can think a bit about whether we're OK just exposing all IDL getters, or perhaps all getters on some interfaces plus annotated getters in general or some such...  The definition of "no side effects" that the IDL getters use is possibly a bit more stringent than the one debugger cares about, in that debugger wants it to not be obviously observable that the getter was called, while the IDL annotations want it to not be observable that a getter _wasn't_ called...

Comment 18

[James Long (:jlongster)]

Somebody either needs to take this or change this to P2. Is anyone willing to work on this right now?

Comment 19

[Eddy Bruel [:ejpbruel]]

(In reply to James Long (:jlongster) from comment #18)
> Somebody either needs to take this or change this to P2. Is anyone willing
> to work on this right now?

Please see my comment in bug 1245819. We should not be downgrading priorities because we don't have enough resources to work on them. Priorities reflect how badly something needs to be fixed, not whether someone is actually working on them. If we have more P1 bugs than we can chew off, that's a signal we need to assign more resources. We should not try to cover that up by removing P1 bugs.

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

Moving to p3 because no activity for at least 24 weeks.
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information