Closed Bug 1414674 Opened 7 years ago Closed 6 years ago

Element.animate gives error when run in WebExtensions content script ("Permission denied to access property Symbol.iterator")

Categories

(Core :: DOM: Animation, defect, P3)

Product:
Core
Component:
DOM: Animation
Version:
58 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: daniel.rb.lobo, Assigned: arai)

Details

Attachments

(2 files, 1 obsolete file)

Animate Permission Denied - WebExtension.zip
1.67 KB, application/x-zip-compressed
Details
Do not enter the compartment of the target document when calling KeyframeEffect and KeyframeEffectReadOnly constructor via Xray.
19.07 KB, patch
birtles
: review+
bzbarsky
: review+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20171104220420

Steps to reproduce:

I created a WebExtension example (attached zip file) that injects a content script on page "complete". The content script changes the body's border to 2px red and then tries to run an opacity animation using this code:

let elem = document.getElementsByTagName("body")[0];
elem.style.border = "2px solid red";
elem.animate({ opacity: [0, 1] }, 2000);

If you run the extension and load or refresh a page, Element.animate gives this error if you run the add-on debugger with "Pause on uncaught exceptions" active:

"Error: Permission denied to access property Symbol.iterator"

Without the debugger you can't see the error, but the execution flow stops at "animate". It's exactly the same if you run with an empty object as argument:

elem.animate({}, 2000);

"Animate" works in the add-on settings page, however, and it is supported since Firefox 48. I'm not loading an iframe or anything related (seen here: https://developer.mozilla.org/docs/Web/JavaScript/Reference/Errors/Property_access_denied).


Actual results:

Element.animate fails to run on a content script and throws exception "Permission denied to access property Symbol.iterator".


Expected results:

Animate should run.
This happens on 56.0.2 (64-bit) as well.
Component: Untriaged → WebExtensions: General
Product: Firefox → Toolkit
This happens because Element::Animate wraps the keyframes object for the window that owns the element (which doesn't have permission to access it) before parsing it.

I'm not really sure why it does that. As far as I can see, the keyframes should be parsed and initialized correctly without changing the context compartment or wrapping the initial object (which is how we usually handle parsing WebIDL parameters).

As a workaround, you can clone the keyframes object into the content page compartment using something like `cloneInto({ opacity }, window)`
Status: UNCONFIRMED → NEW
Component: WebExtensions: General → DOM: Animation
Ever confirmed: true
Flags: needinfo?(bzbarsky)
Product: Toolkit → Core
> I'm not really sure why it does that.

See bug 1096773 comment 15.  

But to summarize, animate() just calls the KeyframeEffect constructor.  If you just directly call |new contentWindow.KeyframeEffect| via Xrays, you will get the same failure mode: it's declared with an "object" argument in the IDL, but constructors are always invoked on the C++ side in the compartment of the thing being constructed, so "keyframes" will be an opaque wrapper...  We made animate() have the same behavior as direct invocation of the KeyframeEffect constructor, for consistency.

As for _why_ constructors are invoked in the compartment of the thing being constructed... there's some long backstory there including bug 778152 (looking for things on the wrong globals), but the real reason we still have the behavior is that the other option would be a footgun when the constructor allocated any members via JSAPI: it would get the wrong compartment for them unless it manually entered it.

This is all pretty horrible for this case, of course.  We used to have a carveout where we didn't enter the content compartment for the Promise constructor; we could consider expanding that out to some other cases, as long as we audit them _really_ carefully.  And the KeyframeEffect constructor will be great fun to audit.
Flags: needinfo?(bzbarsky)
so, what we need to do is, to operate on the passed keyframes object (and maybe options object) on those objects' compartment
inside KeyframeUtils::GetKeyframesFromObject [1],
if Element::Animate [2] enters from the compartment?
or maybe operate on them with the compartment where the Element::Animate enters from?

[1] https://searchfox.org/mozilla-central/rev/f80722d4f3bfb722c5ec53880c4a7efb71285676/dom/animation/KeyframeUtils.cpp#429
> /* static */ nsTArray<Keyframe>
> KeyframeUtils::GetKeyframesFromObject(JSContext* aCx,
>                                       nsIDocument* aDocument,
>                                       JS::Handle<JSObject*> aFrames,
>                                       ErrorResult& aRv)
> {
> ...
>   if (!iter.init(objectValue, JS::ForOfIterator::AllowNonIterable)) {

[2] https://searchfox.org/mozilla-central/rev/4cd7d094b812eebd2e879978e56c818fa42435e5/dom/base/Element.cpp#3346-3353
> /* static */ already_AddRefed<Animation>
> Element::Animate(const Nullable<ElementOrCSSPseudoElement>& aTarget,
>                  JSContext* aContext,
>                  JS::Handle<JSObject*> aFrames,
>                  const UnrestrictedDoubleOrKeyframeAnimationOptions& aOptions,
>                  ErrorResult& aError)
> {
> ...
>   Maybe<JSAutoCompartment> ac;
>   if (js::GetContextCompartment(aContext) !=
>       js::GetObjectCompartment(ownerGlobal->GetGlobalJSObject())) {
>     ac.emplace(aContext, ownerGlobal->GetGlobalJSObject());
>     if (!JS_WrapObject(aContext, &frames)) {
>       return nullptr;
>     }
>   }
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #3)
> We used to have a
> carveout where we didn't enter the content compartment for the Promise
> constructor; we could consider expanding that out to some other cases, as
> long as we audit them _really_ carefully.  And the KeyframeEffect
> constructor will be great fun to audit.

now I think I understand.
So, currently we're entering the content compartment,
I was about to re-enter enclosing compartment where necessary,
but the plan was to just stop entering content compartment and
check if there's nothing accidentally done in the original compartment, that was supposed to be done in content compartment, right?
> but the plan was to just stop entering content compartment and check if there's
> nothing accidentally done in the original compartment

That is at least _a_ possible plan, yes.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #6)
> > but the plan was to just stop entering content compartment and check if there's
> > nothing accidentally done in the original compartment
> 
> That is at least _a_ possible plan, yes.

Thanks :)

here's try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ac80535aa157bfc011e2bfb41f3afa8be42ffd0
* Added AnimationUtils::GetDocumentFromGlobal that returns document from global object,
   instead of from context (AnimationUtils::GetCurrentRealmDocument)

 * Added aOwnerGlobalObject parameter to KeyframeEffect::Constructor and ConstructKeyframeEffect
   to pass the global object of the owner document of the element
   to retrieve the document from it with AnimationUtils::GetDocumentFromGlobal

 * In KeyframeEffectReadOnly::ConstructKeyframeEffect,
   retrieve document from passed global object(aOwnerGlobalObject) if it's non-null
   (otherwise same as the previous behavior)
   if aOwnerGlobalObject is provided, current compartment is supposed to be
   the triggering compartment, instead of the target document's compartment

 * Element::Animate
   Now it doesn't enter the target document's compartment when constructing
   key frames.
   instead, it passes target document's global object

   then, now it enters the target document's compartment when constructing
   Animation (just to keep the previous behavior)

 * Fixed dom/animation/test/chrome/file_animate_xrays.html
   properly override constructors to throw error when they're called

 * Fixed dom/animation/test/chrome/test_animate_xrays.html
   now animate via X-ray is allowed and they're visible to caller
   it now checks if it returns animation and key frames are accessible

 * Added browser_ext_contentscript_animate.js
     Test that does the same thing as the example in comment #0.
     Triggers animate via X-ray, and checks if it returns animation and
     key frames are accessible
Attachment #8949711 - Flags: review?(bzbarsky)
I'm sorry for the lag here.

So with this patch, Element.animate will now have different behavior from new KeyframeEffect, afaict.  We should probably fix the latter too. I would think...
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #9)
> So with this patch, Element.animate will now have different behavior from
> new KeyframeEffect, afaict.  We should probably fix the latter too. I would
> think...

thanks, I'll look into that part too.
So, in `new KeyframeEffect` case, generated binding enters the callee function's compartment.

> static bool
> _constructor(JSContext* cx, unsigned argc, JS::Value* vp)
> {
>   JS::CallArgs args = JS::CallArgsFromVp(argc, vp);
>   JS::Rooted<JSObject*> obj(cx, &args.callee());
> ...
>   unsigned argcount = std::min(args.length(), 3u);
>   switch (argcount) {
> ...
>     case 3: {
>       bool objIsXray = xpc::WrapperFactory::IsXrayWrapper(obj);
> ...
>       Maybe<JSAutoCompartment> ac;
>       if (objIsXray) {
>         obj = js::CheckedUnwrap(obj);
>         if (!obj) {
>           return false;
>         }
>         ac.emplace(cx, obj);
>         if (!JS_WrapObject(cx, &desiredProto)) {
>           return false;
>         }
>         if (!JS_WrapObject(cx, &arg1)) {
>           return false;
>         }
>       }
>       binding_detail::FastErrorResult rv;
>       auto result(StrongOrRawPtr<mozilla::dom::KeyframeEffect>(mozilla::dom::KeyframeEffect::Constructor(global, Constify(arg0), arg1, Constify(arg2), rv)));

in that case, the possible solution would be,
  * add some option to binding generator to stop entering the callee function's compartment,
    and use it in KeyframeEffect.webidl
  * add some option to pass caller's compartment (or something) to binding generator,
    and use it in KeyframeEffect.webidl,
    and re-enter the caller's compartment inside KeyframeEffect ctor (similar to the plan in comment #4)
Comment on attachment 8949711 [details] [diff] [review]
Do not enter the owner global's compartment when creating KeyframeEffect.

apparently I didn't have to add extra parameter,
that was completely redundant.

I'll fix it as well.
Attachment #8949711 - Flags: review?(bzbarsky)
Added RunConstructorInCallerCompartment extended attribute to webidl that suppress entering the callee compartment.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8afc162343e95da3b73cc612d6ec75d9e606c909
* Added AnimationUtils::GetDocumentFromGlobal that returns document from global object,
   instead of from context (AnimationUtils::GetCurrentRealmDocument)

 * Added RunConstructorInCallerCompartment extended attribute to webidl
   if it's specified, the constructor is called on caller's compartment.

 * KeyframeEffectReadOnly::ConstructKeyframeEffect now retrieves the
   document from the global object, instead of current context

   it now expects it's executing on caller's compartment.

 * Fixed dom/animation/test/chrome/file_animate_xrays.html
   properly override constructors to throw error when they're called

 * Fixed dom/animation/test/chrome/test_animate_xrays.html
   now animate via X-ray is allowed and they're visible to caller
   it now checks if it returns animation and key frames are accessible

 * Added browser_ext_contentscript_animate.js
     Test that does the same thing as the example in comment #0.
     Triggers Element.animate via X-ray, and checks if it returns animation
     and key frames are accessible
     also calls KeyframeEffect constructor via X-ray and checks the same thing
Attachment #8949711 - Attachment is obsolete: true
Attachment #8950062 - Flags: review?(bzbarsky)
Comment on attachment 8950062 [details] [diff] [review]
Do not enter the compartment of the target document when calling KeyframeEffect and KeyframeEffectReadOnly constructor via Xray.

>Bug 1414674 - Do not enter the compartment of the target document when calling KeyframeEffect and KeyframeEffectReadOnly constructor via Xray.

s/target document/target window/

>+++ b/browser/components/extensions/test/browser/browser_ext_contentscript_animate.js

I assume that this test fails without the code changes, right?

>+++ b/dom/animation/AnimationUtils.h
>   /**
>+   * Get the document from the global object.

This should make it clear that null can be returned.  And perhaps document why one would use this.

>+++ b/dom/animation/KeyframeEffectReadOnly.cpp
>-  nsIDocument* doc = AnimationUtils::GetCurrentRealmDocument(aGlobal.Context());
>+  nsIDocument* doc = AnimationUtils::GetDocumentFromGlobal(aGlobal.Get());

This needs to be _very_ clearly documented, because it does not match the spec (which very clearly says to use the current realm).

So we need to explain that in all non-Xray cases aGlobal matches our current Realm, hence getting a document from it matches the spec behavior.  And in the Xray case we want to create out new objects using the document of the target global.

Something in the commit message should probably describe the auditing that has been done to ensure this function running in the caller compartment is OK.

For example, KeyframeEffectParamsFromUnion uses aCallerType, which comes from aGlobal.CallerType() in ConstructKeyframeEffect.  We are now changing this, I think: I expect it used to be non-system when called over Xrays and will now be system.  You should probably check with Brian to make sure the behavior change is OK.

The other change, the one we mostly want, is in GetKeyframesFromObject.  This needs to be able to touch the passed-in JSObject, and doesn't do any gcthing allocation, I assume.  But it _does_ do nsDocument::IsWebAnimationsEnabled() checks and we're changing the behavior for those (used to test non-system and will now test system).  Again, please check with Brian.

>+++ b/dom/base/Element.cpp
>+  JS::Rooted<JSObject*> ownerGlobalObject(aContext);

This is unused.

>+++ b/dom/bindings/Codegen.py
>+        runConstructorInCallerCompartment = descriptor.interface.getExtendedAttribute('RunConstructorInCallerCompartment')

This is clearly not going to fit in 80 chars no matter what you do, but maybe just wrap the part after the '=' to the next line (in parens as needed)?


>+++ b/dom/webidl/KeyframeEffect.webidl
>+ RunConstructorInCallerCompartment,

Both instances of this should document why we want to run in the caller compartment: because we need to do custom processing on the "object" argument and hence need to be able to access it.
Attachment #8950062 - Flags: review?(bzbarsky)
Attachment #8950062 - Flags: review?(bbirtles)
Attachment #8950062 - Flags: review+
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #15)
> For example, KeyframeEffectParamsFromUnion uses aCallerType, which comes
> from aGlobal.CallerType() in ConstructKeyframeEffect.  We are now changing
> this, I think: I expect it used to be non-system when called over Xrays and
> will now be system.  You should probably check with Brian to make sure the
> behavior change is OK.

KeyframeEffectParamsFromUnion uses aCallerType to determine if we can enable composite modes or not. That's a somewhat niche feature so I'm not too concerned if we end up disabling that in the x-ray case (by using the global from the target realm instead of the current realm) -- in fact that's probably reasonable. I can't see any particular security issues resulting from it applying in the opposite direction either so I think this particular change is ok.
 
> The other change, the one we mostly want, is in GetKeyframesFromObject. 
> This needs to be able to touch the passed-in JSObject, and doesn't do any
> gcthing allocation, I assume.  But it _does_ do
> nsDocument::IsWebAnimationsEnabled() checks and we're changing the behavior
> for those (used to test non-system and will now test system).  Again, please
> check with Brian.

This too is only being used to enable/disable composite modes so, as above, I'm not too concerned about it. Ultimately we plan to ship that (once we've spec'ed how addition works for all the different property types) and then this check will go away so any difference here is hopefully only temporary.
Comment on attachment 8950062 [details] [diff] [review]
Do not enter the compartment of the target document when calling KeyframeEffect and KeyframeEffectReadOnly constructor via Xray.

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

::: browser/components/extensions/test/browser/browser_ext_contentscript_animate.js
@@ +20,5 @@
> +        let elem = document.getElementsByTagName("body")[0];
> +        elem.style.border = "2px solid red";
> +
> +        let anim = elem.animate({opacity: [1, 0]}, 2000);
> +        let frames = anim.effect.getKeyframes();

This API (the `effect` member of Animation) is currently pref-ed off for content in beta/release channels. I assume that affects content scripts in extensions too? If so we'll need suitable annotations to make sure dom.animations-api.core.enabled is enabled when running this test. (Fortunately bug 1328830 just landed so we can now use pref annotations in mochitest.ini, but only on a per-directory basis.)
Attachment #8950062 - Flags: review?(bbirtles) → review+
> so I'm not too concerned if we end up disabling that in the x-ray case 

We're _enabling_ it in the xray case, afaict.  It's disabled right now (also afaict).
Thank you for reviewing :D

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #15)
> >+++ b/browser/components/extensions/test/browser/browser_ext_contentscript_animate.js
> 
> I assume that this test fails without the code changes, right?

Yes, it fails with `Permission denied to access property Symbol.iterator"`
regarding comment #18,
is it okay to use Web Animation in content process where it can be disabled depending on the pref value,
in Xray case?
(I'll also check locally what happens in that case, once the build finishes)
Flags: needinfo?(bbirtles)
(In reply to Tooru Fujisawa [:arai] from comment #20)
> regarding comment #18,
> is it okay to use Web Animation in content process where it can be disabled
> depending on the pref value,
> in Xray case?
> (I'll also check locally what happens in that case, once the build finishes)

Yes, I think that's fine.
Flags: needinfo?(bbirtles)
> For example, KeyframeEffectParamsFromUnion uses aCallerType, which comes from
> aGlobal.CallerType() in ConstructKeyframeEffect.  We are now changing this,
> I think: I expect it used to be non-system when called over Xrays and will
> now be system.

aGlobal is the target window global, and aGlobal.CallerType() is NonSystem if the
target window is web content, regardless of the current compartment.
So, the patch doesn't change this part.


I just confirmed that what happens if WebExtensions triggers animation with iterationComposite option,
where dom.animations-api.core.enabled==false,
and the option is just ignored, because IsWebAnimationsEnabled is false.
Pushed by arai_a@mac.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3f16a179c93
Do not enter the compartment of the target window when calling KeyframeEffect and KeyframeEffectReadOnly constructor via Xray. r=bz,birtles
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3f16a179c93d9b75dd4581280967fa6ef364cc3
Bug 1414674 - Do not enter the compartment of the target window when calling KeyframeEffect and KeyframeEffectReadOnly constructor via Xray. r=bz,birtles
> aGlobal is the target window global, and aGlobal.CallerType() is NonSystem if the
> target window is web content, regardless of the current compartment.

That's not correct.  aGlobal.CallerType() is implemented like so:

  GlobalObject::CallerType() const
  {
    return nsContentUtils::ThreadsafeIsSystemCaller(mCx) ?
      dom::CallerType::System : dom::CallerType::NonSystem;
  }

so is very much just checking the current compartment in the non-worker case.

> and the option is just ignored, because IsWebAnimationsEnabled is false.

Sure, because webextension compartments are CallerType::NonSystem.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #25)
> > aGlobal is the target window global, and aGlobal.CallerType() is NonSystem if the
> > target window is web content, regardless of the current compartment.
> 
> That's not correct.  aGlobal.CallerType() is implemented like so:
> 
>   GlobalObject::CallerType() const
>   {
>     return nsContentUtils::ThreadsafeIsSystemCaller(mCx) ?
>       dom::CallerType::System : dom::CallerType::NonSystem;
>   }

eh...
looks like I'm completely misunderstanding about dom::GlobalObject.
okay, I'll check again and fix the commit message
so, I think I should check the case that browser chrome code triggers animation on web content,
instead of the case that WebExtensions triggers.
https://hg.mozilla.org/mozilla-central/rev/c3f16a179c93
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Evaluated the following in Scratchpad with browser environment, with e10s disabled, and dom.animations-api.core.enabled=false, and iterationComposite is reflected to the animation:

var w = Services.wm.getMostRecentWindow("navigator:browser").document.getElementById("content").contentWindow;
var d = w.document;
let MS_PER_SEC = 1000;
const anim = d.body.animate(
  {
    marginLeft: ['0px', '100px']
  },
  {
    duration: 1 * MS_PER_SEC,
    easing: 'linear',
    iterations: 10,
    iterationComposite: 'accumulate',
  }
);

So, for this case, it works just like Web Animations API is enabled.
Status: REOPENED → ASSIGNED
then, now it matches to other cases:

// setting .iterationComposite property works.
{
  let w = Services.wm.getMostRecentWindow("navigator:browser").document.getElementById("content").contentWindow;
  let d = w.document;
  let effect = new w.KeyframeEffect(d.body, [
    {marginLeft: '0px', offset: 0},
    {marginLeft: '100px', offset: 1}
  ], { duration: 1000, fill: "forwards", iterations: 10 });
  effect.iterationComposite = 'accumulate';
  let animation = new w.Animation(effect, document.timeline);
  animation.play();
}

// .effect property returns actual effect
{
  let w = Services.wm.getMostRecentWindow("navigator:browser").document.getElementById("content").contentWindow;
  let d = w.document;
  let anim = d.body.animate(
    {
      marginLeft: ['0px', '100px']
    },
    {
      duration: 1000,
      easing: 'linear',
      iterations: 10,
      iterationComposite: 'accumulate',
    }
  );
  anim.effect;
}
here's the updated commit message, does it look okay?

==== begin ====

KeyframeEffect and KeyframeEffectReadOnly constructors can run in the caller
compartment, which is okay because the current compartment is used in the
following places and all of them are safe:

1. GlobalObject::CallerType(), that is ultimately passed to
   nsDocument::IsWebAnimationsEnabled in KeyframeEffectParamsFromUnion,
   to decide whether reflecting mIterationComposite/mComposite to
   KeyframeEffectParams.

   GlobalObject::CallerType() can now be different than the target window's one,
   if the caller is system principles and the target is web content, and in that
   case nsDocument::IsWebAnimationsEnabled there always returns true while
   Web Animations can be disabled on web content.

   Reflecting those properties is okay, since it just changes the animation
   behavior, and this is disabled by default because of the spec issue.

2. GlobalObject::Context(), that is ultimately passed to
   KeyframeUtils::GetKeyframesFromObject and used while extracting information
   from passed-in keyframe object, with iterable/iterator protocols.

   Performing that operation in the caller side is okay, since the same thing
   can be done on caller, and the operation doesn't perform any GCThing
   allocation on the target window global.

==== end ====
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bbirtles)
Thank you for looking into this. bz is 100x more knowledgeable about this than I so I'll defer to him for commenting about the specifics but the behavior described in comment 30 seems like the desired behavior and comment 31 suggests it is consistent between using Element.animate and using the constructor directly.

A couple of minor nits on the proposed commit message:

* Perhaps comment 32 is missing the first sentence of the commit message but normally the commit message should describe what is changing
* I'm not sure of the correct terminology, but I think we want s/is system principles/has the system principal/
* s/because of the spec issue/until remaining spec issues are resolved/
Flags: needinfo?(bbirtles)
"to decide whether reflecting mIterationComposite/mComposite" should probably be "to decide whether to copy mIterationComposite/mComposite".

"if the caller is system principles" should be "if the caller has the system principal".

"Reflecting those properties is okay" should probably be "honoring the mIterationComposite/mComposite properties is OK".

Apart from that and Brian's concerns, the commit message looks good.  The actual behavior we are ending up with seems fine to me as well.
Flags: needinfo?(bzbarsky)
https://hg.mozilla.org/integration/mozilla-inbound/rev/184418c5611982b3aabfeb762c7db7e045dd8676
Bug 1414674 - Do not enter the compartment of the target window when calling KeyframeEffect and KeyframeEffectReadOnly constructor via Xray. r=bz,birtles
Status: ASSIGNED → RESOLVED
Closed: 6 years ago → 6 years ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: