Closed Bug 1310318 Opened 8 years ago Closed 7 years ago

content script access to canvas.drawWindow

Categories

(WebExtensions :: General, defect, P2)

Product:
WebExtensions
Component:
General
Type:
defect

Tracking

(firefox54 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed
Blocking Flags:
webextensions +

People

(Reporter: mixedpuppy, Assigned: zombie)

References

(Blocks 1 open bug)

Details

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

Attachments

(2 files)

Bug 1310318 - Part 1: Allow access to canvas drawWindow() with web extensions permission
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1310318 - Part 2: Test access to canvas drawWindow() with permission
59 bytes, text/x-review-board-request
kmag
: review+
Details 
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]
Blocks: 1311472
Whiteboard: [design-decision-needed] → [design-decision-needed] triaged
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#
This one is absolutely needed to port NoScript's ClearClick anti-clickjacking protection, https://noscript.net/faq#clearclick
Blocks: webext-port-noscript
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.
webextensions: --- → +
Priority: -- → P2
Whiteboard: [design-decision-needed] triaged → [design-decision-approved] triaged
Assignee: nobody → tomica
Status: NEW → ASSIGNED
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 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 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 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.
Attachment #8830898 - Flags: review?(kmaglione+bmo)
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 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.
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)
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)
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+
Keywords: checkin-needed
Version: 49 Branch → unspecified
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.
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
https://hg.mozilla.org/mozilla-central/rev/1a1e741ca6d9
https://hg.mozilla.org/mozilla-central/rev/b36ca53f6bed
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Blocks: 1318565
Product: Toolkit → WebExtensions
See Also: → 1680359
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: