BUILD: Today's nightly.

STEPS TO REPRODUCE:

1. Load http://example.com/
2. Open web console.
3. Type var w; window.addEventListener("click", () => w = window.open("http://example.com/"), { once: true })
4. Click somewhere in the window (opens a new tab).
5. Go back to the original tab.
6. Type w in the console.

EXPECTED RESULTS: Shows "Window http://example.com/"

ACTUAL RESULTS: Shows "Restricted http://example.com/"

This is likely a regression from 1471496. Cross-compartment wrappers to Window and Location objects are always CrossOriginObjectWrapper, and whether they can be unwrapped depends on who is asking, because document.domain is involved. See the difference between CheckedUnwrapDynamic and CheckedUnwrapStatic.

It looks like the relevant code is at https://searchfox.org/mozilla-central/rev/153172de0c5bfca31ef861bd8fc0995f44cada6a/devtools/server/actors/object.js#95,117-122 and calls into DevToolsUtils.unwrap, which ends up calling DebuggerObject::unwrap, which does UnwrapOneCheckedStatic on the referent. Do we have a concept of "who is asking" here such that we could use UnwrapOneCheckedDynamic?

The current UnwrapOneCheckedStatic was added in https://hg.mozilla.org/mozilla-central/rev/4325cfacfc49 with a comment about how it's the safe thing for now but we should really figure out what should happen here....

This also happens with a simpler STR:

1. Open webconsole on this page
2. Evaluate new Set([{a: 1}])

-> It prints Set [ Restricted ]

This is an important regression that we should fix as soon as possible.
Jim, is there anything I can do to help?

This also happens with a simpler STR:

That's a different bug, and a very recent regression (from 5 days ago). I filed bug 1558504 on that.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #0)

It looks like the relevant code is at https://searchfox.org/mozilla-central/rev/153172de0c5bfca31ef861bd8fc0995f44cada6a/devtools/server/actors/object.js#95,117-122 and calls into DevToolsUtils.unwrap, which ends up calling DebuggerObject::unwrap, which does UnwrapOneCheckedStatic on the referent. Do we have a concept of "who is asking" here such that we could use UnwrapOneCheckedDynamic?

When js::DebuggerObject::unwrap gets called, cx is going to be in the debugger's compartment. That will have chrome privileges, so it should be permitted to unwrap. Does that seem like the right thing for using UnwrapOneCheckedDynamic to consult?

Hmm. So the comment at https://searchfox.org/mozilla-central/source/devtools/shared/DevToolsUtils.js#194-198 seems to think that the unwrap will be done with the permissions of the debuggee.

Looking into how this code used to work before bug 1521906, it looks like it did an UnwrapOneChecked on referent. At that point the unwrap was done with the permissions of referent (or rather the compartment referent lives in), which did not necessarily match those of cx, as far as I can tell. That would match the comment I link to above.

So no, using the compartment of cx here doesn't seem right. What should probably happen instead is that DebuggerObject needs to store the global that its referent came from, so it can enter that global for CheckedUnwrapDynamic purposes as needed... Jan, do you have any better ideas?

That means we need to find the last place where we know what global is being used and plumb it down to Debugger::wrapDebuggeeObject, as far as I can tell.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #4)

What should probably happen instead is that DebuggerObject needs to store the global that its referent came from, so it can enter that global for CheckedUnwrapDynamic purposes as needed... Jan, do you have any better ideas?

Note that in EnterDebuggeeObjectRealm we currently use referent->maybeCCWRealm(). This predates Compartment::firstGlobal. Also, CCW allocation enters the compartment's first global.

I think using referent->compartment()->firstGlobal() would make sense? We'd want to use that also in EnterDebuggeeObjectRealm, maybe it can be factored out in a helper function.

Let me know if you want me to do any of the work here.

Hmm. So the comment at https://searchfox.org/mozilla-central/source/devtools/shared/DevToolsUtils.js#194-198 seems to think that the unwrap will be done with the permissions of the debuggee.

That comment is incorrect. Debugger.Object.prototype.unwrap is intended to support debugger inspection of objects that the debuggee cannot access. The documentation for Debugger.Object.prototype.unwrap says:

If the referent is a wrapper that this Debugger.Object's compartment is permitted to unwrap, return a Debugger.Object instance referring to the wrapped object.

Isn't the original bug here caused by the Debugger failing to unwrap something it ought to be able to unwrap? Perhaps the use of UnwrapOneCheckedStatic back in March was not correct. (There is no jit-test for D.O.p.unwrap-ing between different privilege levels so if this changed behavior, we would not have caught it. The shell does have the functionality needed to write such a test...)

Adding more metadata to a Debugger.Object seems wrong. JS itself does not have metadata attached to object references, and the Debugger API should be using mechanisms parallel to what real JS uses to make its decisions.

That comment is incorrect.

OK, would be good to fix it. And then really carefully audit all the callsites, I guess?

Isn't the original bug here caused by the Debugger failing to unwrap something it ought to be able to unwrap?

Yes. But note that in the case in question in comment 0 the debuggee can unwrap it.

Perhaps the use of UnwrapOneCheckedStatic back in March was not correct.

Entirely possible; like I said in comment 0 its use was temporary pending figuring out what the actual behavior should be.

and the Debugger API should be using mechanisms parallel to what real JS uses to make its decisions.

The mechanism JS uses for doing checked unwrap in full generality at this point is that it has a concept of "who is asking?" represented by the current Realm of the JSContext. But more importantly, the real question needs to be "why are we doing the unwrap and what do we plan to do with the result?" Arbitrary escape of unwrapped values is a likely security issue and needs to be considered very carefully.

In terms of what the behavior here used to be it depends on what referent could be in DebuggerObject::unwrap. Based on comments around that code it seems like referent can be a CCW; can I trust those comments? If so, which compartment is it a CCW from/to? For example, is referent always in the compartment of the DebuggerObject (and hence the compartment of the JSContext coming into DebuggerObject::unwrap? Or can it be in a different compartment? The answer to this determines what the old behavior, using UnwrapOneChecked, was since that used the security policy if the CCW in question when doing the unwrap.

I am here assuming that when we land in DebuggerObject::unwrap both cx and object are in the debugger compartment, not the debuggee compartment, right?

There is no jit-test for D.O.p.unwrap-ing between different privilege levels

The unwrap in comment 0 is not even between different privilege levels, but is on a very specific kind of cross-compartment proxy that does not exist in SpiderMonkey proper.

I think using referent->compartment()->firstGlobal() would make sense?

I think that would give us something closer to the behavior we used to have, but first I'd like to understand what compartment referent is in.... I wish there were documentation for this stuff.

OK, would be good to fix it. And then really carefully audit all the callsites, I guess?

I'm unhappy that the folks who wrote those comments were unclear about what D.O.p.unwrap does. They are accomplished developers, so if they're confused, others are likely to be confused as well. The documentation was correct, but I guess the intended behavior is counterintuitive enough that more aggressive warnings are necessary.

Arbitrary escape of unwrapped values is a likely security issue and needs to be considered very carefully.

Fortunately, 'arbitrary escape' is not on the table here, as far as I can see. D.O.p.unwrap gives debugger code a D.O for the result of the unwrap. Any time a D.O is passed to a debuggee, it is rewrapped for the debuggee's compartment. If it were not, the cross-compartment checks would detect this.

In terms of what the behavior here used to be it depends on what referent could be in DebuggerObject::unwrap. Based on comments around that code it seems like referent can be a CCW; can I trust those comments?

Yes. A Debugger.Object's role is to represent an object as content would see it. Hence, there can be separate Debugger.Objects for a debuggee object and a cross-compartment wrapper to that object, with operations on each behaving accordingly.

If so, which compartment is it a CCW from/to? For example, is referent always in the compartment of the DebuggerObject (and hence the compartment of the JSContext coming into DebuggerObject::unwrap? Or can it be in a different compartment? The answer to this determines what the old behavior, using UnwrapOneChecked, was since that used the security policy if the CCW in question when doing the unwrap.

A single Debugger may produce Debugger.Objects whose referents are in any number of different compartments; it depends on how the D.O was produced.

I think we need some live discussion to sort this out.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #8)

I think that would give us something closer to the behavior we used to have, but first I'd like to understand what compartment referent is in.... I wish there were documentation for this stuff.

There is documentation for the Debugger API in js/src/doc/Debugger. It goes into some detail about compartments and wrappers.

https://searchfox.org/mozilla-central/source/js/src/doc/Debugger/Debugger.Object.md

OK, would be good to fix it. And then really carefully audit all the callsites, I guess?

The use in DevToolsUtils.unwrap is the only callsite of D.O.p.unwrap. That function is called from three places:

• ObjectActor.form, which uses it to choose a class string to include in the protocol's object form, and to detect proxies (the unwrapped value is not used further).
• ObjectActor.proxySlots similarly uses it to properly detect proxies.
• stringify in actors/object/stringifiers.js similarly.

This suggests we could replace unwrap with something substantially less powerful.

I think we need some live discussion to sort this out.

I'm happy to do that. Should I just pick a time?

https://searchfox.org/mozilla-central/source/js/src/doc/Debugger/Debugger.Object.md

Thank you, that's helpful.

I'm unhappy that the folks who wrote those comments were unclear about what D.O.p.unwrap does

This may be worth discussing live, because I'm not sure they were wrong about what it actually did, unless I completely misunderstand which compartments things live in here: the code used to UnwrapOneChecked, which would have done security checks based on the compartment of "referent" and the thing it's a CCW for, not based on the compartment of the DebuggerObject... I think a sort of equivalent in today's code would be to EnterDebuggeeObjectRealm and then do a dynamic checked unwrap, like Jan suggests above. It's not perfect in that it can give "wrong" results on pages that set document.domain, but it's the best we can do if we don't store more state inside the DebuggerObject, I think.

The use in DevToolsUtils.unwrap is the only callsite of D.O.p.unwrap. That function is called from three places:

Thank you for looking into that! It does seem like what we really want to be able to do is query several boolean predicates on the target of the CCW, if we have an unwrappable CCW....

Just had a live conversation with Boris. It seems like the D.O.p.unwrap API is hard to understand, misused in the current code, too powerful for what we actually need, and creates difficult-to-answer questions in a CheckedUnwrapDynamic world. So the best solution would be to replace D.O.p.unwrap (and DevToolsUtils.unwrap) with more specific APIs that do what we actually want.

Mid-air collided with Jim saying the same thing... I'll poke at this a bit as I can.

OK, I'm likely not going to get a chance to sort this out at this point.

Jan, you offered to help with this a while back. Are you still up for that?

This does fix the immediate bug....

(In reply to Boris Zbarsky [:bzbarsky] from comment #15)

Jan, you offered to help with this a while back. Are you still up for that?

Sorry I'm pretty swamped right now :/ If someone wants to take this I'm happy to help or offer feedback..