1310318 - content script access to canvas.drawWindow

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P2)

Description

[Shane Caraveo (:mixedpuppy)]

For capturing a video of the current page.  May just need to verify whether this can be done.


> I discovered that the only way
> for an add-on to inject a script with that level of privilege is by
> using the framescript (nsIFrameScriptLoader.loadFrameScript) method.
> Other ways to inject script (sdk/page-mod,sdk/content/worker) do not
> provide this.

Comment 1

[Kris Maglione [:kmag]]

This should probably require the "<all_urls>" permission, like `tabs.captureVisibleTab` does. And we should probably at least make some attempt to check that no content from DANGEROUS_TO_LOAD URLs have been loaded into the docShell tree (which we don't currently do for `captureVisibleTab`). But the behavior is pretty much equivalent, given that `captureVisibleTab` is implemented using `drawWindow`, so I can't think of a particularly good reason we shouldn't allow it.

Comment 4

[Matt McLeod]

Would it be reasonable/acceptable to modify tabs.captureVisibleTab() to accept an (optional) capture area? It'd be a non-standard extension so I'm not sure if it'd be allowable. But it would be one way to solve the problem.

Comment 5

[Caitlin Neiman [:caitmuenster]]

To be discussed at Jan 10 WebExtensions Triage meeting. 

Agenda: https://docs.google.com/document/d/18K97o1juaHSeYEkes1iMz8AayjuVkUuIK844ErGaa-c/edit#

Comment 6

[Giorgio Maone [:ma1]]

This one is absolutely needed to port NoScript's ClearClick anti-clickjacking protection, https://noscript.net/faq#clearclick

Comment 7

[Michel Gutierrez]

This is also required for Video DownloadHelper screen capture: https://www.downloadhelper.net/howto#how-do-i-use-the-screen-capture- . It is important to note that in this case, screenshots are taken at a fast rate (typically 24/seconds), so a solution where tabs.captureVisibleTab() would be called at every frame might not be suitable performance-wise.

Comment 8

[Tomislav Jovanovic :zombie]

Attachment: Bug 1310318 - Part 1: Allow access to canvas drawWindow() with web extensions permission

Review commit: https://reviewboard.mozilla.org/r/107562/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107562/

Comment 9

[Tomislav Jovanovic :zombie]

Attachment: Bug 1310318 - Part 2: Test access to canvas drawWindow() with permission

Review commit: https://reviewboard.mozilla.org/r/107564/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107564/

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8830897 [details]
Bug 1310318 - Part 1: Allow access to canvas drawWindow() with web extensions permission

https://reviewboard.mozilla.org/r/107562/#review108984

::: dom/canvas/CanvasRenderingContext2D.cpp:5220
(Diff revision 1)
>    // We can't allow web apps to call this until we fix at least the
>    // following potential security issues:
>    // -- rendering cross-domain IFRAMEs and then extracting the results
>    // -- rendering the user's theme and then extracting the results
>    // -- rendering native anonymous content (e.g., file input paths;
>    // scrollbars should be allowed)
>    if (!nsContentUtils::IsCallerChrome()) {

I don't think this check is necessary anymore. It should be covered by the WebIDL annotations.

::: dom/canvas/CanvasRenderingContext2D.cpp:5227
(Diff revision 1)
> +    // But we do allow Web Extensions (with a permission) to draw content
> +    // from non-dangerous URIs.
> +
> +    BasePrincipal *p = BasePrincipal::Cast(nsContentUtils::SubjectPrincipal());
> +    bool hasPermission = p->AddonHasPermission(NS_LITERAL_STRING("<all_urls>"));
> +
> +    bool isDangerous;
> +    nsIURI *uri = aWindow.AsInner()->GetDocumentURI();
> +    nsresult rv = NS_URIChainHasFlags(uri,
> +                                      nsIProtocolHandler::URI_DANGEROUS_TO_LOAD,
> +                                      &isDangerous);
> +
> +    if (!hasPermission || NS_FAILED(rv) || isDangerous) {

This check isn't very useful. An add-on shouldn't be able to get access to a DANGEROUS_TO_LOAD window in any case, and if it did somehow manage it, it could just draw the parent frame instead.

The WebIDL annotations should take care of all of the permissions checks we need to do here.

::: dom/canvas/CanvasUtils.cpp:138
(Diff revision 1)
> +  // Otherwise, we need to check the principal
> +  JSCompartment *c = js::GetContextCompartment(aCx);
> +  nsIPrincipal *p = nsJSPrincipals::get(JS_GetCompartmentPrincipals(c));
> +
> +  // And only allow if it is an addon that has the permission
> +  if (BasePrincipal::Cast(p)->AddonHasPermission(NS_LITERAL_STRING("<all_urls>")))
> +    return true;

I think it might be worth adding a nsContentUtils helper for this. Something like `CallerHasAddonPermission`.

::: dom/canvas/CanvasUtils.cpp:139
(Diff revision 1)
> +  JSCompartment *c = js::GetContextCompartment(aCx);
> +  nsIPrincipal *p = nsJSPrincipals::get(JS_GetCompartmentPrincipals(c));

Nit: `*` should be attached to the type name rather than the variable name.

::: dom/canvas/CanvasUtils.cpp:146
(Diff revision 1)
> +
> +  // And only allow if it is an addon that has the permission
> +  if (BasePrincipal::Cast(p)->AddonHasPermission(NS_LITERAL_STRING("<all_urls>")))
> +    return true;
> +
> +  return false;

Nit: `return AddonHasPermission(...)`

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8830897 [details]
Bug 1310318 - Part 1: Allow access to canvas drawWindow() with web extensions permission

https://reviewboard.mozilla.org/r/107562/#review109032

r=me on the idl bits

Comment 12

[Tomislav Jovanovic :zombie]

Comment on attachment 8830897 [details]
Bug 1310318 - Part 1: Allow access to canvas drawWindow() with web extensions permission

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107562/diff/1-2/

Comment 13

[Tomislav Jovanovic :zombie]

Comment on attachment 8830898 [details]
Bug 1310318 - Part 2: Test access to canvas drawWindow() with permission

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107564/diff/1-2/

Comment 14

[Tomislav Jovanovic :zombie]

Comment on attachment 8830897 [details]
Bug 1310318 - Part 1: Allow access to canvas drawWindow() with web extensions permission

https://reviewboard.mozilla.org/r/107562/#review109070

::: dom/canvas/CanvasRenderingContext2D.cpp
(Diff revision 2)
>    bool discardContent = GlobalAlpha() == 1.0f
>      && (op == CompositionOp::OP_OVER || op == CompositionOp::OP_SOURCE);
>    const gfx::Rect drawRect(aX, aY, aW, aH);
>    EnsureTarget(discardContent ? &drawRect : nullptr);
>  
> -  // We can't allow web apps to call this until we fix at least the

Per Kris's comment, this check predates the `ChromeOnly` flag, and now that we wanna allow non-chrome code to access this, it's in the way.

Comment 15

[Tomislav Jovanovic :zombie]

Comment on attachment 8830897 [details]
Bug 1310318 - Part 1: Allow access to canvas drawWindow() with web extensions permission

https://reviewboard.mozilla.org/r/107562/#review108984

> I think it might be worth adding a nsContentUtils helper for this. Something like `CallerHasAddonPermission`.

I decided against.  In this case we are not checking the "caller", but an explicitly passed JSContext.  After getting the principal, the common code that's left is a one-liner, which doesn't seem worth making into a method (used exactly twice).  But if Boris thinks it is, I'd be happy to do it.

Comment 16

[Kris Maglione [:kmag]]

Comment on attachment 8830897 [details]
Bug 1310318 - Part 1: Allow access to canvas drawWindow() with web extensions permission

https://reviewboard.mozilla.org/r/107562/#review108984

> I decided against.  In this case we are not checking the "caller", but an explicitly passed JSContext.  After getting the principal, the common code that's left is a one-liner, which doesn't seem worth making into a method (used exactly twice).  But if Boris thinks it is, I'd be happy to do it.

It would be the same as the IsSystemCaller check, which also takes a context. And it should probably just always return true for system callers.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8830897 [details]
Bug 1310318 - Part 1: Allow access to canvas drawWindow() with web extensions permission

https://reviewboard.mozilla.org/r/107562/#review109176

::: dom/canvas/CanvasUtils.cpp:136
(Diff revision 2)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  // Chrome code gets access by default
> +  if (nsContentUtils::IsSystemCaller(aCx))
> +    return true;

Curlies around the body, please.

::: dom/canvas/CanvasUtils.cpp:140
(Diff revision 2)
> +  if (nsContentUtils::IsSystemCaller(aCx))
> +    return true;
> +
> +  // Otherwise, we need to check the principal
> +  JSCompartment* c = js::GetContextCompartment(aCx);
> +  nsIPrincipal* p = nsJSPrincipals::get(JS_GetCompartmentPrincipals(c));

I think you might as well just do:

    nsIPrincipal* p = nsContentUtils::SubjectPrincipal();
    
and skip the JSContext bits.  Though really, we should factor out the "get the JSContext" bits out of SubjectPrincipal and have a version that takes a JSContext (called from the current SubjectPrincipal and also from here).  Please file a followup bug on that and cc me?  Bonus points for fixing that followup bug, of course.  ;)

Comment 18

[Boris Zbarsky [:bzbarsky]]

Or yes, you could do something like CallerHasPermission(JSContext*) in nsContentUtils, which extracts the principal and then does the checks (effectively sinking the "is system" check into there).  Just noticed that Kris had suggested that in the review.

Comment 19

[Tomislav Jovanovic :zombie]

Comment on attachment 8830897 [details]
Bug 1310318 - Part 1: Allow access to canvas drawWindow() with web extensions permission

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107562/diff/2-3/

Comment 20

[Tomislav Jovanovic :zombie]

Comment on attachment 8830898 [details]
Bug 1310318 - Part 2: Test access to canvas drawWindow() with permission

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107564/diff/2-3/

Comment 21

[Tomislav Jovanovic :zombie]

Comment on attachment 8830897 [details]
Bug 1310318 - Part 1: Allow access to canvas drawWindow() with web extensions permission

https://reviewboard.mozilla.org/r/107562/#review109176

> I think you might as well just do:
> 
>     nsIPrincipal* p = nsContentUtils::SubjectPrincipal();
>     
> and skip the JSContext bits.  Though really, we should factor out the "get the JSContext" bits out of SubjectPrincipal and have a version that takes a JSContext (called from the current SubjectPrincipal and also from here).  Please file a followup bug on that and cc me?  Bonus points for fixing that followup bug, of course.  ;)

I went with `CallerHasPermission`, so should I still file this refactoring bug?

Comment 22

[Boris Zbarsky [:bzbarsky]]

Please do NOT add new IsCallerChrome() callers.  We're trying to remove that API.

Likewise, imo, for SubjectPrincipal().  So yes, we should still refactor to actually get things properly of the JSContext you already have.

Comment 23

[Tomislav Jovanovic :zombie]

Comment on attachment 8830897 [details]
Bug 1310318 - Part 1: Allow access to canvas drawWindow() with web extensions permission

I updated Part 1, and went with CallerHasPermission() helper.  Boris, can you please look over this once more?  I'm not really confident enough to land C++ patches without final approval.

Comment 24

[Tomislav Jovanovic :zombie]

Comment on attachment 8830897 [details]
Bug 1310318 - Part 1: Allow access to canvas drawWindow() with web extensions permission

I was too slow.  ;)

Comment 25

[Tomislav Jovanovic :zombie]

Comment on attachment 8830897 [details]
Bug 1310318 - Part 1: Allow access to canvas drawWindow() with web extensions permission

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107562/diff/3-4/

Comment 26

[Tomislav Jovanovic :zombie]

Comment on attachment 8830898 [details]
Bug 1310318 - Part 2: Test access to canvas drawWindow() with permission

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107564/diff/3-4/

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8830897 [details]
Bug 1310318 - Part 1: Allow access to canvas drawWindow() with web extensions permission

https://reviewboard.mozilla.org/r/107562/#review110068

r=me

Comment 28

[Kris Maglione [:kmag]]

Comment on attachment 8830898 [details]
Bug 1310318 - Part 2: Test access to canvas drawWindow() with permission

https://reviewboard.mozilla.org/r/107564/#review110070

Comment 29

[Tomislav Jovanovic :zombie]

Comment on attachment 8830897 [details]
Bug 1310318 - Part 1: Allow access to canvas drawWindow() with web extensions permission

https://reviewboard.mozilla.org/r/107562/#review109176

> I went with `CallerHasPermission`, so should I still file this refactoring bug?

Filed bug 1335890.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1a1e741ca6d9
Part 1: Allow access to canvas drawWindow() with web extensions permission r=bz
https://hg.mozilla.org/integration/autoland/rev/b36ca53f6bed
Part 2: Test access to canvas drawWindow() with permission r=kmag

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1a1e741ca6d9
https://hg.mozilla.org/mozilla-central/rev/b36ca53f6bed