Open Bug 1248631 Opened 6 years ago Updated 9 months ago

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

Categories

(DevTools :: Debugger, task, P3)

task

Tracking

(Not tracked)

People

(Reporter: bzbarsky, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.
Attached file Testcase
Attached file 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".
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...
What on earth is 'safe' supposed to mean in this context?
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?
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.
(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.
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.
(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.
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.
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.
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.
Priority: -- → P1
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...
(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?
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.
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.
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...
Somebody either needs to take this or change this to P2. Is anyone willing to work on this right now?
(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.
Product: Firefox → DevTools
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
Priority: P1 → P3
Blocks: dbg-api
Type: defect → task
You need to log in before you can comment on or make changes to this bug.