content script access to canvas.drawWindow

RESOLVED FIXED in Firefox 54

Status

()

Toolkit
WebExtensions: General
P2
normal
RESOLVED FIXED
8 months ago
2 months ago

People

(Reporter: mixedpuppy, Assigned: zombie)

Tracking

(Blocks: 3 bugs)

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [design-decision-approved] triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

8 months ago
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.
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.
Component: WebExtensions: Untriaged → WebExtensions: General
Whiteboard: [design-decision-needed]
Duplicate of this bug: 1310353
(Reporter)

Updated

7 months ago
Blocks: 1311472

Updated

7 months ago
Whiteboard: [design-decision-needed] → [design-decision-needed] triaged
Duplicate of this bug: 1318565

Comment 4

5 months ago
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.
To be discussed at Jan 10 WebExtensions Triage meeting. 

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

Comment 6

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

Comment 7

5 months ago
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.

Updated

5 months ago
webextensions: --- → +
Priority: -- → P2
Whiteboard: [design-decision-needed] triaged → [design-decision-approved] triaged
(Assignee)

Updated

4 months ago
Assignee: nobody → tomica
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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 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
Attachment #8830897 - Flags: review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

4 months ago
mozreview-review
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.
(Assignee)

Comment 15

4 months ago
mozreview-review-reply
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

4 months ago
mozreview-review-reply
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.
(Assignee)

Updated

4 months ago
Attachment #8830898 - Flags: review?(kmaglione+bmo)
(Assignee)

Updated

4 months ago
Attachment #8830897 - Flags: review?(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.  ;)
Attachment #8830897 - Flags: review?(bzbarsky) → review+
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 21

4 months ago
mozreview-review-reply
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?
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.
(Assignee)

Comment 23

4 months ago
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.
Attachment #8830897 - Flags: review+ → review?(bzbarsky)
(Assignee)

Comment 24

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

I was too slow.  ;)
Attachment #8830897 - Flags: review?(bzbarsky)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
Attachment #8830897 - Flags: review?(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
Attachment #8830897 - Flags: review?(bzbarsky) → review+
Comment on attachment 8830898 [details]
Bug 1310318 - Part 2: Test access to canvas drawWindow() with permission

https://reviewboard.mozilla.org/r/107564/#review110070
Attachment #8830898 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Updated

4 months ago
Keywords: checkin-needed
Version: 49 Branch → unspecified
(Assignee)

Comment 29

4 months ago
mozreview-review-reply
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

4 months ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed

Comment 31

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1a1e741ca6d9
https://hg.mozilla.org/mozilla-central/rev/b36ca53f6bed
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Blocks: 1318565
You need to log in before you can comment on or make changes to this bug.