1453916 - Fix canvas APIs in extension content scripts when resistFingerprinting is enabled

Status: VERIFIED FIXED

(Core :: Graphics: Canvas2D, defect, P3)

Description

[Tim Nguyen :ntim]

+++ This bug was initially created as a clone of Bug #1412961 +++

Per Bug 1412418 (also copied to https://github.com/mozilla-services/screenshots/issues/3691):

When the privacy.resistFingerprinting setting is turned on, Firefox Screenshots is broken (it uploads a blank image).

I assume this is because canvas access is generally blocked. One option to fix Firefox Screenshots in this case would be to look at the principal using the canvas (moz-extension I guess). Or maybe it would be helpful that Screenshots uses canvas.drawWindow, which is an API only given to extensions.

Comment 1

[Tom S [:evilpie]]

Attachment: WIP

This adds [NeedsSubjectPrincipal] to getImageData and passes that IsImageExtractionAllowed. We would need to add this to all other methods that use this function. An alternative approach would be to get the principal from the JSContext parameter in that function manually.

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8974340 [details] [diff] [review]
WIP

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

That looks reasonable to me. Tom what do you think, makes sense?

Comment 3

[Tom Ritter [:tjr]]

Comment on attachment 8974340 [details] [diff] [review]
WIP

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #2)
> Comment on attachment 8974340 [details] [diff] [review]
> WIP
> 
> Review of attachment 8974340 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> That looks reasonable to me. Tom what do you think, makes sense?

I defer to you on the correctness of the code, my only request would be some comments in IsImageExtractionAllowed explaining what is expected to be passed there and when.

However; in Bug 1450398 we discuss how we want Fingerprinting to behave in the future, and different types of exemptions. So while I wouldn't want to stop you, Tom, from fixing something that annoys you - the best fix here would be what was discussed there.

Comment 4

[Tom S [:evilpie]]

Attachment: Allow extensions access to canvas API with resistFingerprinting

Okay, I finished the patch with the approach in the WIP. I also looked at bug 1450398, and I think following this approach with using a nsIPrincipal parameter would also make sense for ShouldResistFingerprinting.

Comment 5

[Boris Zbarsky [:bzbarsky]]

The general idea of adding [NeedsSubjectPrincipal] to this stuff instead of relying on ambient authority is a good one.

The use of ThreadsafeIsCallerChrome() in IsImageExtractionAllowed can go away, because that's now covered by the "is subject principal system" check.

The file:// check should probably be using the principal's URI, not the document's, unless we really want sandboxed file:// stuff to be able to do canvas extraction (seems dubious to me).  Followup for this is ok, since it's a behavior restriction.

The pdf.js check is ... should probably use the principal URI too.  Followup bug?

Comment 6

[Tom S [:evilpie]]

Attachment: v2 - Allow extensions access to canvas API with resistFingerprinting

I just applied bz's comment about removing ThreadsafeIsCallerChrome(). Before landing this I will also work on tests, but I didn't have the time yet.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8974934 [details] [diff] [review]
v2 - Allow extensions access to canvas API with resistFingerprinting

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

I think this looks good, but I am also not a dom peer. Since bz is already on it, I hope he will sign off on it!

Comment 8

[Tom S [:evilpie]]

Comment on attachment 8974934 [details] [diff] [review]
v2 - Allow extensions access to canvas API with resistFingerprinting

Canceling review for now, because I found a problem with how we check if something is a content script. Fix soon. Thanks for the feedback.

Comment 9

[Tom S [:evilpie]]

Attachment: v3 - Allow extensions access to canvas API with resistFingerprinting

Turns out principal->GetAddonId doesn't actually work for the expanded principals that we use for Webextension content-scripts. Instead I moved some code for extracting the AddonPolicy from expanded principals to the BasePrincipal code, based on Kris' suggestion.

This is also includes a test that failed before and now passes.

Comment 10

[Tom S [:evilpie]]

There are a bunch of eslint style errors for the test file that I am going to fix.

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8975586 [details] [diff] [review]
v3 - Allow extensions access to canvas API with resistFingerprinting

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

Thanks

::: caps/BasePrincipal.h
@@ +113,4 @@
>  
>    already_AddRefed<BasePrincipal> CloneStrippingUserContextIdAndFirstPartyDomain();
>  
> +  // Return the AddonPolicy for content-scripts that use ExpandedPrincipals.

Nit: Something like "If this is an add-on content script principal, returns its AddonPolicy. Otherwise returns null."

::: toolkit/components/extensions/test/mochitest/chrome.ini
@@ +16,4 @@
>  # NO NEW TESTS.  mochitest-chrome does not run under e10s, avoid adding new
>  # tests here unless absolutely necessary.
>  
> +[test_chrome_ext_canvas_resistFingerprinting.html]

Any reason not to make this a plain mochitest? Chrome mochitests never run in content processes, which probably doesn't matter much here, but might matter a little.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_canvas_resistFingerprinting.html
@@ +34,5 @@
> +  }
> +
> +  let extensionData = {
> +    manifest: {
> +      permissions: ["*://tracking.example.com/*"],

Why is this necessary?

Comment 12

[Tom S [:evilpie]]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #11)
> Comment on attachment 8975586 [details] [diff] [review]
> v3 - Allow extensions access to canvas API with resistFingerprinting
> 
> Review of attachment 8975586 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks
> 
> ::: caps/BasePrincipal.h
> @@ +113,4 @@
> >  
> >    already_AddRefed<BasePrincipal> CloneStrippingUserContextIdAndFirstPartyDomain();
> >  
> > +  // Return the AddonPolicy for content-scripts that use ExpandedPrincipals.
> 
> Nit: Something like "If this is an add-on content script principal, returns
> its AddonPolicy. Otherwise returns null."
> 
> ::: toolkit/components/extensions/test/mochitest/chrome.ini
> @@ +16,4 @@
> >  # NO NEW TESTS.  mochitest-chrome does not run under e10s, avoid adding new
> >  # tests here unless absolutely necessary.
> >  
> > +[test_chrome_ext_canvas_resistFingerprinting.html]
> 
> Any reason not to make this a plain mochitest? Chrome mochitests never run
> in content processes, which probably doesn't matter much here, but might
> matter a little.
> 
Somehow I assumed I needed a chrome test to set a pref. I now moved this to a normal mochitest.
> :::
> toolkit/components/extensions/test/mochitest/
> test_chrome_ext_canvas_resistFingerprinting.html
> @@ +34,5 @@
> > +  }
> > +
> > +  let extensionData = {
> > +    manifest: {
> > +      permissions: ["*://tracking.example.com/*"],
> 
> Why is this necessary?
Sorry, just some copy and pasted code that I now removed.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8975586 [details] [diff] [review]
v3 - Allow extensions access to canvas API with resistFingerprinting

The commit message should probably say "even with resistFingerprinting turned on" at the end or something like that, to make it clear that it's not being allowed _just_ with resistFingerprinting.

>+  extensions::WebExtensionPolicy* ContentScriptAddonPolicy();

Do we need to return the policy?  All the consumers so far just care whether there is one, not what it is.  Maybe we should have a boolean-returning predicate instead (e.g. called IsWebExtensionContentScript()).

There are various CanvasRenderingContext2D function signatures that are way over 80 chars with this change and should probably get wrapped.

I suspect that the path-taking signature of CanvasRenderingContext2D::IsPointInPath might want resist-fingerprinting logic but doesn't seem to have any right now....  Maybe check with Tom Ritter about it?

>@@ -794,7 +796,8 @@ HTMLCanvasElement::CaptureStream(const O

Can you please file a bug to have this function get a JSContext passed from the bindings instead of using nsContentUtils::GetCurrentJSContext()?

> HTMLCanvasElement::MozGetAsFileImpl(const nsAString& aName,

Same thing; should probably stop using GetCurrentJSContext.

>+  [NeedsSubjectPrincipal] // Only required because above

"Only required because overloads can't have different extended attributes."  Both places.

r=me

Comment 14

[Tom S [:evilpie]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #13)
> Comment on attachment 8975586 [details] [diff] [review]
> v3 - Allow extensions access to canvas API with resistFingerprinting
> 
> The commit message should probably say "even with resistFingerprinting
> turned on" at the end or something like that, to make it clear that it's not
> being allowed _just_ with resistFingerprinting.
> 
> >+  extensions::WebExtensionPolicy* ContentScriptAddonPolicy();
> 
> Do we need to return the policy?  All the consumers so far just care whether
> there is one, not what it is.  Maybe we should have a boolean-returning
> predicate instead (e.g. called IsWebExtensionContentScript()).
> 
I feel like returning the policy can be useful if we want to start checking for specific permissions the content script has.
> There are various CanvasRenderingContext2D function signatures that are way
> over 80 chars with this change and should probably get wrapped.
> 
> I suspect that the path-taking signature of
> CanvasRenderingContext2D::IsPointInPath might want resist-fingerprinting
> logic but doesn't seem to have any right now....  Maybe check with Tom
> Ritter about it?
> 
Might be because that looks at the some path, and not the current path in the canvas, but I will ask.
> >@@ -794,7 +796,8 @@ HTMLCanvasElement::CaptureStream(const O
> 
> Can you please file a bug to have this function get a JSContext passed from
> the bindings instead of using nsContentUtils::GetCurrentJSContext()?
> 
> > HTMLCanvasElement::MozGetAsFileImpl(const nsAString& aName,
> 
> Same thing; should probably stop using GetCurrentJSContext.
> 
> >+  [NeedsSubjectPrincipal] // Only required because above
> 
> "Only required because overloads can't have different extended attributes." 
> Both places.
> 
> r=me

Comment 15

[Tom S [:evilpie]]

> I feel like returning the policy can be useful if we want to start checking for specific permissions the content script has.
Actually we already use the policy to get the addonId in one place.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe91056fdd27
Allow canvas extraction from webextension content-script even with resistFingerprinting turned on. r=kmag,bz

Comment 17

[Kris Maglione [:kmag]]

(In reply to Tom Schuster [:evilpie] from comment #15)
> > I feel like returning the policy can be useful if we want to start checking for specific permissions the content script has.
> 
> Actually we already use the policy to get the addonId in one place.

Agreed. Even if it wasn't used now, I feel like it's likely enough to be useful in other places that it's worth having a convenient getter. Right now, we use it to report the name of the add-on that's causing slow script warnings in the background hang monitor.

Either way, though, returning a raw weak pointer is just as cheap as returning a boolean, so we don't really lose anything.

That said, as far as checking specific permissions, we should just use BasePrincipal::AddonHasPermission. It already knows how to deal with expanded principals, without the caller having to do extra work.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c20d86aa7fc3
Fix bustage. r=me on CLOSED TREE

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5d001049bb7
Fix eslint errors. r=me on CLOSED TREE

Comment 20

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/fe91056fdd27
https://hg.mozilla.org/mozilla-central/rev/c20d86aa7fc3
https://hg.mozilla.org/mozilla-central/rev/c5d001049bb7

Comment 21

[Anca Soncutean, Desktop QA]

I’ve managed to reproduce this issue with the steps mentioned in bug 1412418 on Firefox 58.0a1 (20171027220059) under Windows 10 x64.
The bug is fixed on Firefox 62.0b8 (20180712042337) under Windows 10 x64, macOS 10.13 and Ubuntu 16.04 x64.