Closed Bug 1480678 Opened 6 years ago Closed 6 years ago

Remove remaining JSAutoRealmAllowCCW uses in dom/

Categories

(Core :: DOM: Core & HTML, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(13 files)

1.22 KB, patch
kmag
: review+
Details | Diff | Splinter Review
1.79 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
11.45 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.58 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
4.10 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.38 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
15.30 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.08 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
1.52 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
3.23 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.32 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.98 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.65 KB, patch
kmag
: review+
Details | Diff | Splinter Review
      No description provided.
When I did the initial audit I thought these two could be on CCWs but actually they can't.
Attachment #8997294 - Flags: review?(mrbkap)
Is there a nicer way to get the JS global in _getwindowobject? The OuterWindow methods I can find are fallible so this seemed like the safest option.
Attachment #8997343 - Flags: review?(bzbarsky)
I just based the IDL bits on the |stack| attribute. It works but maybe there's a more direct way to do it?
Attachment #8997404 - Flags: review?(bzbarsky)
Looking at the callers of this code, I think we just pass an unwrapped object here:

https://searchfox.org/mozilla-central/rev/e52cd92858800a69b74cb97d26d9bdb960d611ca/dom/base/nsContentPermissionHelper.cpp#817-830

I couldn't find any JS callers that pass a |choices| argument but I didn't look too hard.
Attachment #8997406 - Flags: review?(mrbkap)
Attachment #8997406 - Flags: review?(mrbkap) → review+
Attachment #8997294 - Flags: review?(mrbkap) → review+
Priority: -- → P2
Comment on attachment 8997343 [details] [diff] [review]
Part 3 - Store a global in nsJSObjWrapper and use it for realm entering

r=me
Attachment #8997343 - Flags: review?(bzbarsky) → review+
Comment on attachment 8997401 [details] [diff] [review]
Part 4 - Use current global as conversion scope in the Xray case when wrapping the return value in generated bindings

r=me. For what it's worth, it might be adding a comment here pointing to the "When using a slot on the Xray expando..." comment above, which explains exactly why the current global is correct here.
Attachment #8997401 - Flags: review?(bzbarsky) → review+
Comment on attachment 8997404 [details] [diff] [review]
Part 5 - Use nsScriptErrorWithStack's stack global in ConsoleListener::Observe

>+    [noscript] readonly attribute jsval stackGlobal;

Could use documentation.

r=me
Attachment #8997404 - Flags: review?(bzbarsky) → review+
Attachment #8997290 - Flags: review?(kmaglione+bmo) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff61f1adfb8a
part 1 - Remove unnecessary realm entering and wrapping code in ModuleGetter. r=kmag
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ce11c1c3459
part 2 - Use JSAutoRealm instead of JSAutoRealmAllowCCW in dom/xbl. r=mrbkap
https://hg.mozilla.org/integration/mozilla-inbound/rev/5eadb66c18cb
part 3 - Store a global in nsJSObjWrapper and use it for realm entering. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/eeb0c73d5b18
part 4 - Use current global as conversion scope in the Xray case when wrapping the return value in generated bindings. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c9ea4b33221
part 5 - Use nsScriptErrorWithStack's stack global in ConsoleListener::Observe. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fa208b3f007
part 6 - Use CheckedUnwrap in nsContentPermissionRequestProxy::Allow. r=mrbkap
Status: NEW → ASSIGNED
Keywords: leave-open
This is falling into place quite nicely.
Attachment #8997803 - Flags: review?(bzbarsky)
I audited the callers and this function already does GetDOMClass(aObject)->mGetProto(aCx) elsewhere.
Attachment #8997806 - Flags: review?(mrbkap)
At this point there are only a handful of JSAutoRealmAllowCCW uses left in dom/:

(1) Two in dom/base/IntlUtils.cpp:

https://searchfox.org/mozilla-central/rev/3fdc491e118c5cdfbaf6e2d52f3466d2b27ad1de/dom/base/IntlUtils.cpp#91,138

(2) TextEncoder::Encode:

https://searchfox.org/mozilla-central/rev/23c8a3a9f316f880e7e7fc0b52868f37a7b6d521/dom/encoding/TextEncoder.cpp#34

The bindings code calling this passes the Xray target in the Xray case, so I think JSAutoRealm is fine?

(3) A few places that use the mozilla::dom::{TypedArray,ArrayBuffer} wrappers:

* TCPSocket::Send: https://searchfox.org/mozilla-central/rev/3fdc491e118c5cdfbaf6e2d52f3466d2b27ad1de/dom/network/TCPSocket.cpp#861-862

* WriteStructuredCloneImageData: https://searchfox.org/mozilla-central/rev/23c8a3a9f316f880e7e7fc0b52868f37a7b6d521/dom/bindings/StructuredClone.cpp#48-50

* AudioContext::DecodeAudioData: https://searchfox.org/mozilla-central/rev/3fdc491e118c5cdfbaf6e2d52f3466d2b27ad1de/dom/media/webaudio/AudioContext.cpp#593

From what I can see these classes can store a cross-compartment wrapper if SpiderMonkeyInterfaceObjectStorage::WrapIntoNewCompartment is used. Should we unwrap or use mImplObj instead of mWrappedObj here?
Flags: needinfo?(bzbarsky)
Attachment #8997806 - Flags: review?(mrbkap) → review+
Attachment #8997808 - Flags: review?(mrbkap) → review+
Comment on attachment 8997803 [details] [diff] [review]
Part 7 - Use CallbackObject's callback global for realm entering in more places

>+  // use this global instead (must be same-compartment with mScriptedListener

Please assert that in the EventListenerInfo constructor.
Attachment #8997803 - Flags: review?(bzbarsky) → review+
> (1) Two in dom/base/IntlUtils.cpp:

This one is worth checking with gandalf on.  It might be OK to MOZ_RELEASE_ASSERT that the returned object is not a CCW (for both APIs), but it's not clear to me whether it is.

> (2) TextEncoder::Encode:

This one can have a cross-compartment wrapper, like so:

  <iframe></iframe>
  <script>
     var enc = new TextEncoder();
     frames[0].TextEncoder.prototype.encode.call(enc, "foo")
  </script>
  
The worst part is that the spec is somewhere between "doesn't specify" and "wrong" in terms of the behavior of this situation.  Per a literal reading of the current spec, the return value of that call should be a Uint8Array in the frames[0] global.  That's what Firefox and Chrome do, but not what Safari does; Safari uses the global of the TextEncoder object.  Edge does not support this API at all.  Such a lovely mess.  I filed https://github.com/whatwg/encoding/issues/150 on the spec end.  In other cases like this people have pushed for always having the behavior Safari has here, while I've pushed for having factory methods like this use the current global....

In terms of our implementation, the aObj in here is the "scope object" that we pass in for cases that test true for "needScopeObject", and in practice that only happens if we have a typed array return type _and_ a non-wrapper-cached interface.  That's precisely two APIs: this one and FileReaderSync::ReadAsArrayBuffer, which ignores the argument (adding to our consistency awesome).

Preserving our web-facing behavior here is easy: just stop using aObj.  But that will change the Xray behavior: now Xrays will get a typed array in their own global instead of the target global.

Switching our web-facing behavior to match Safari is also easy: Just always set aObj to the unwrapped thing, even in the non-Xray case.  Then aObj represents the "relevant global" in spec terms, aCx represents the "current global" in spec terms, and the callee can implement the desired behavior by either using the current global of aCx or entering the global of aObj.

If we want to preserve _both_ behaviors, we'd probably need to do something like "unwrap the obj in the Xray case, else pass in the current global of cx" or some such in the binding code.  That might be the expedient thing for now.  Let me know if you want me to write this patch, since it may need not-completely trivial codegen changes.

> TCPSocket::Send

This only needs the JSContext to call ArrayBufferInputStream::SetData.  But the only thing ArrayBufferInputStream::SetData uses aCx for is rooting.  I think we should make it use RootingCx(), remove the [implicit_jscontext] annotation in nsIArrayBufferInputStream.idl, and remove this compartment-entering bit altogether.

> WriteStructuredCloneImageData

I think it would make a lot more sense to me to wrap the object we have into the cx compartment than to enter the compartment of the object.  If we're writing the structured clone in some compartment that shouldn't have access to the underlying data of the ImageData, we shouldn't be giving that access here.

> AudioContext::DecodeAudioData

For this one, the only thing it uses the JSContext that it enters a realm on is to call JS_StealArrayBufferContents.  That function does require a same-compartment JSContext.

In practice, in this function, aBuffer.Obj() will never be a cross-compartment wrapper.  That can only happen when we're calling a WebIDL constructor, and this is not a constructor.

But another option here would be to explicitly call CheckedUnwrap() on aBuffer.Obj().  And if that returns null (it shouldn't, per above!) fail out.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #17)
> > (1) Two in dom/base/IntlUtils.cpp:
> 
> This one is worth checking with gandalf on.  It might be OK to
> MOZ_RELEASE_ASSERT that the returned object is not a CCW (for both APIs),
> but it's not clear to me whether it is.

Hm. So, this code is calling an XPCWrappedJS method on a JavaScript service, which means it can return just about anything, including a CCW. I don't think it's reasonable to assert that it isn't. It's way too easy to screw that up on the JS side.

I suppose the thing to do here is just to do UncheckedUnwrap and enter the compartment of that.

That said... it's kind of weird to enter the compartment of the source object before initializing the result dictionary. That works well enough, given that neither of those dictionaries hold JS values... but it seems like kind of a footgun.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #17)
> If we want to preserve _both_ behaviors, we'd probably need to do something
> like "unwrap the obj in the Xray case, else pass in the current global of
> cx" or some such in the binding code.  That might be the expedient thing for
> now.  Let me know if you want me to write this patch, since it may need
> not-completely trivial codegen changes.

What if we change the code to do something like this:

// Use the current global or, in the Xray case, use the Xray's
// target realm.
Maybe<JSAutoRealm> ar;
if (js::GetContextCompartment(aCx) != js::GetObjectCompartment(aObj)) {
    ar.emplace(aObj);
}

If it's more complicated than that, I'd appreciate if you could write the patch for this :)
Flags: needinfo?(bzbarsky)
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f30b1a69497
part 7 - Use CallbackObject's callback global for realm entering in more places. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/883d78be26bf
part 8 - Use JSAutoRealm instead of JSAutoRealmAllowCCW in nsObjectLoadingContent::SetupProtoChain. r=mrbkap
https://hg.mozilla.org/integration/mozilla-inbound/rev/f865653db6dc
part 9 - Assert IDBWrapperCache::mScriptOwner is a global and use JSAutoRealm instead of JSAutoRealmAllowCCW. r=mrbkap
After thinking about it more, maybe JS_WrapValue is the most natural/safe thing to do here. We might create extra wrappers but this function is already pretty slow anyway (entering JS via XPCWrappedJS etc).
Attachment #8998182 - Flags: review?(kmaglione+bmo)
Comment on attachment 8998182 [details] [diff] [review]
Part 13 - Use JS_WrapValue in IntlUtils::{GetDisplayNames,GetLocaleInfo} instead of switching realms

Review of attachment 8998182 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/IntlUtils.cpp
@@ +81,5 @@
>      aError.Throw(rv);
>      return;
>    }
>  
> +  if (!retVal.isObject() || !JS_WrapValue(cx, &retVal)) {

Unfortunately, this probably isn't going to work. The IntlUtils getter is exposed to both chrome and XBL scopes. I have no idea if it's actually *used* in XBL scopes, but if it is, the context we wrap the retVal object into won't be able to access its properties, so we won't be able to populate the return dictionary.
Attachment #8998182 - Flags: review?(kmaglione+bmo) → review-
(In reply to Kris Maglione [:kmag] from comment #26)
> Unfortunately, this probably isn't going to work. The IntlUtils getter is
> exposed to both chrome and XBL scopes. I have no idea if it's actually
> *used* in XBL scopes, but if it is, the context we wrap the retVal object
> into won't be able to access its properties, so we won't be able to populate
> the return dictionary.

This runs in the privileged junk scope [0], is that still a concern then?

[0] https://searchfox.org/mozilla-central/rev/51268dcbdff0f6f4a5cff7986df0f616efc5bcfd/dom/base/IntlUtils.cpp#55-58
Flags: needinfo?(kmaglione+bmo)
Comment on attachment 8998182 [details] [diff] [review]
Part 13 - Use JS_WrapValue in IntlUtils::{GetDisplayNames,GetLocaleInfo} instead of switching realms

(In reply to Jan de Mooij [:jandem] from comment #27)
> (In reply to Kris Maglione [:kmag] from comment #26)
> > Unfortunately, this probably isn't going to work. The IntlUtils getter is
> > exposed to both chrome and XBL scopes. I have no idea if it's actually
> > *used* in XBL scopes, but if it is, the context we wrap the retVal object
> > into won't be able to access its properties, so we won't be able to populate
> > the return dictionary.
> 
> This runs in the privileged junk scope [0], is that still a concern then?
> 
> [0]
> https://searchfox.org/mozilla-central/rev/
> 51268dcbdff0f6f4a5cff7986df0f616efc5bcfd/dom/base/IntlUtils.cpp#55-58

Hm. That comment doesn't really make sense. Passing arrays or plain objects with simple properties from an XBL scope to a chrome scope should work, as long as they're wrapped. But just creating the objects in a privileged scope certainly makes more sense.

In any case, you're right, that shouldn't be a concern as long as the caller is already in the privileged junk scope. I missed that part. Although I'll repeat my earlier comment that this is all pretty dodgy...
Flags: needinfo?(kmaglione+bmo)
Attachment #8998182 - Flags: review- → review+
> What if we change the code to do something like this:

A goal here is to make sure callees don't need to think about this, so we want all the complexity in the binding.  Please file a bug on me for this bit and I will fix.
Flags: needinfo?(bzbarsky)
Comment on attachment 8998179 [details] [diff] [review]
Part 10 - Remove unnecessary realm entering in TCPSocket::Send

r=me
Attachment #8998179 - Flags: review?(bzbarsky) → review+
Comment on attachment 8998180 [details] [diff] [review]
Part 11 - Wrap the typed array in the current compartment instead of entering its realm in WriteStructuredCloneImageData

Lovely.  r=me
Attachment #8998180 - Flags: review?(bzbarsky) → review+
Comment on attachment 8998181 [details] [diff] [review]
Part 12 - Do a CheckedUnwrap in AudioContext::DecodeAudioData to ensure we don't have a CCW

>+    js::ReportAccessDenied(cx);
>+    return nullptr;

That's not right.  This is going to report the exception on our local AutoJSAPI, which will then report the exception (no chance for the page to catch it) and clear it on cx before we unwind.  Then we will return null and no exception to a caller that will crash with a null-deref if we do that.

What we should do instead is:

  aRv.Throw(NS_ERROR_DOM_SECURITY_ERR);
  return nullptr;

r=me with that fixed.
Attachment #8998181 - Flags: review?(bzbarsky) → review+
Depends on: 1481927
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #32)
> That's not right.  This is going to report the exception on our local
> AutoJSAPI

So much for pretending to be a DOM peer ;) Thanks for catching that.
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f189ec6420b6
part 10 - Remove unnecessary realm entering in TCPSocket::Send. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/27e04584ae0f
part 11 - Wrap the typed array in the current compartment instead of entering its realm in WriteStructuredCloneImageData. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/81cb351bae89
part 12 - Do a CheckedUnwrap in AudioContext::DecodeAudioData to ensure we don't have a CCW. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/23b8939ad87e
part 13 - Use JS_WrapValue in IntlUtils::{GetDisplayNames,GetLocaleInfo} instead of switching realms. r=kmag
Bug 1481927 is fixed on inbound (thanks Boris!) so I'm just going to assume that will stick and close this.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Keywords: leave-open
Target Milestone: --- → mozilla63
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: