Cross-compartment wrappers for Window and Location now always show up as "Restricted"
Categories
(DevTools :: Console, defect, P2)
Tracking
(Not tracked)
People
(Reporter: bzbarsky, Unassigned)
References
(Regression)
Details
(Keywords: regression)
Attachments
(1 file)
1.95 KB,
patch
|
Details | Diff | Splinter Review |
BUILD: Today's nightly.
STEPS TO REPRODUCE:
- Load http://example.com/
- Open web console.
- Type
var w; window.addEventListener("click", () => w = window.open("http://example.com/"), { once: true })
- Click somewhere in the window (opens a new tab).
- Go back to the original tab.
- 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....
Updated•5 years ago
|
Updated•5 years ago
|
Comment 1•5 years ago
|
||
This also happens with a simpler STR:
- Open webconsole on this page
- 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?
Reporter | ||
Comment 2•5 years ago
|
||
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.
Comment 3•5 years ago
|
||
(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 callingDebuggerObject::unwrap
, which doesUnwrapOneCheckedStatic
on the referent. Do we have a concept of "who is asking" here such that we could useUnwrapOneCheckedDynamic
?
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?
Reporter | ||
Comment 4•5 years ago
|
||
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.
Comment 5•5 years ago
|
||
(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 forCheckedUnwrapDynamic
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.
Comment 6•5 years ago
|
||
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 aDebugger.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.
Reporter | ||
Comment 7•5 years ago
•
|
||
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.
Reporter | ||
Comment 8•5 years ago
|
||
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.
Comment 9•5 years ago
|
||
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.Object
s 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.Object
s 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.
Comment 10•5 years ago
|
||
(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
Comment 11•5 years ago
|
||
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.
Reporter | ||
Comment 12•5 years ago
|
||
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....
Reporter | ||
Updated•5 years ago
|
Comment 13•5 years ago
|
||
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.
Reporter | ||
Comment 14•5 years ago
|
||
Mid-air collided with Jim saying the same thing... I'll poke at this a bit as I can.
Reporter | ||
Comment 15•5 years ago
|
||
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?
Reporter | ||
Comment 16•5 years ago
|
||
This does fix the immediate bug....
Comment 17•5 years ago
|
||
(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..
Updated•3 years ago
|
Updated•2 years ago
|
Description
•