Closed Bug 1453916 Opened 2 years ago Closed Last year

Fix canvas APIs in extension content scripts when resistFingerprinting is enabled

Categories

(Core :: Canvas: 2D, defect, P3)

defect

Tracking

()

VERIFIED FIXED
mozilla62
Tracking Status
firefox62 --- verified

People

(Reporter: ntim, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fingerprinting][fingerprinting-breakage][gfx-noted])

Attachments

(1 file, 3 obsolete files)

+++ 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.
Whiteboard: [fingerprinting][fingerprinting-breakage] → [fingerprinting][fingerprinting-breakage][gfx-noted]
Attached patch WIP (obsolete) — Splinter Review
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.
Assignee: nobody → evilpies
Attachment #8974340 - Flags: feedback?(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?
Attachment #8974340 - Flags: feedback?(tom)
Attachment #8974340 - Flags: feedback?(ckerschb)
Attachment #8974340 - Flags: feedback+
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.
Attachment #8974340 - Flags: feedback?(tom) → feedback+
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.
Attachment #8974340 - Attachment is obsolete: true
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?
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.
Attachment #8974669 - Attachment is obsolete: true
Attachment #8974934 - Flags: review?(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!
Attachment #8974934 - Flags: review?(ckerschb)
Attachment #8974934 - Flags: review?(bzbarsky)
Attachment #8974934 - Flags: feedback+
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.
Attachment #8974934 - Flags: review?(bzbarsky)
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.
Attachment #8975586 - Flags: review?(kmaglione+bmo)
Attachment #8975586 - Flags: review?(bzbarsky)
Attachment #8974934 - Attachment is obsolete: true
Attachment #8975586 - Flags: review?(bzbarsky)
There are a bunch of eslint style errors for the test file that I am going to fix.
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?
Attachment #8975586 - Flags: review?(kmaglione+bmo) → review+
(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 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
Attachment #8975586 - Flags: review?(bzbarsky) → review+
(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
> 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.
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe91056fdd27
Allow canvas extraction from webextension content-script even with resistFingerprinting turned on. r=kmag,bz
(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.
See Also: → 1466326
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.