Closed Bug 1412961 Opened 7 years ago Closed 6 years ago

Fix canvas APIs in extension documents when resistFingerprinting is enabled

Categories

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

Product:
Core
Component:
Graphics: Canvas2D
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: ianbicking, Assigned: ntim)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file)

Bug 1412961 - Make extension principals bypass canvas permission checks.
59 bytes, text/x-review-board-request
ckerschb
: review+
johannh
: review+
Details 
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.
I agree, we should probably ignore the canvas permission for WebExtension principals and/or for that API.

Maybe this should live in a WebExtensions component then? I don't think it's high-priority in any case right now, since the resistFingerprinting pref is not exposed to users.
Blocks: 967895
Priority: -- → P3
Whiteboard: [fingerprinting]
Whiteboard: [fingerprinting] → [fingerprinting][fingerprinting-breakage]
Summary: privacy.resistFingerprinting breaks Screenshots → privacy.resistFingerprinting breaks extensions that use extension-based canvas APIs (like Screenshots)
This is breaking SwitchyOmega on Firefox 58.0. [1] The SwitchyOmega addon is using canvas to draw the icon for browserAction. It only draws and access the canvas on the extension background page, but not websites. The users seem quite confused because there are no prompts to grant the permission. Any workarounds or solutions?

Is it possible to white-list all canvas on moz-extension:// origins? Or, can we add a WebExtension permission to grant this? For example, a permission named "canvasData" and a warning like "This extension may take screenshots from webpages, which can lead to fingerprinting."

By the way, is it possible to feature-detect canvas data being disabled? SwitchyOmega already gracefully falls back to a static image on other browsers that throws when trying to access canvas data, but a silent failure with a blank image is quite hard to deal with.

[1] https://github.com/FelisCatus/SwitchyOmega/issues/1357
(In reply to catusx from comment #2)
> This is breaking SwitchyOmega on Firefox 58.0. [1] The SwitchyOmega addon is
> using canvas to draw the icon for browserAction. It only draws and access
> the canvas on the extension background page, but not websites. The users
> seem quite confused because there are no prompts to grant the permission.
> Any workarounds or solutions?
> 
> Is it possible to white-list all canvas on moz-extension:// origins? Or, can
> we add a WebExtension permission to grant this? For example, a permission
> named "canvasData" and a warning like "This extension may take screenshots
> from webpages, which can lead to fingerprinting."

The correct fix for this, which we will implement 'in the future' is WebExtensions will just be able to get canvas data, no permission required. It's on the list, but I don't have an ETA. (Patches always welcome of course.)

> By the way, is it possible to feature-detect canvas data being disabled?
> SwitchyOmega already gracefully falls back to a static image on other
> browsers that throws when trying to access canvas data, but a silent failure
> with a blank image is quite hard to deal with.

In Bug 1429519 we'll add a way to determine if the browser will show a permission prompt. Currently, you can infer that a permission prompt _was_[0] shown by reading the canvas data and observing it is the fixed-size white image.

Maybe that's enough of a signal to trigger your work-around?

[0] Although you're saying in the Web Extension use case, the prompt isn't even shown?
(In reply to Tom Ritter [:tjr] from comment #3)

> The correct fix for this, which we will implement 'in the future' is
> WebExtensions will just be able to get canvas data, no permission required.
> It's on the list, but I don't have an ETA. (Patches always welcome of
> course.)
> 

Got it, thanks for the clarification!

> 
> In Bug 1429519 we'll add a way to determine if the browser will show a
> permission prompt. Currently, you can infer that a permission prompt
> _was_[0] shown by reading the canvas data and observing it is the fixed-size
> white image.
> 
> Maybe that's enough of a signal to trigger your work-around?

Yes, I think I can work with that for now.

> 
> [0] Although you're saying in the Web Extension use case, the prompt isn't
> even shown?

No, in this case it's not even shown. I think this is due to everything happening on the background page, which is not a tab or a window so there is no place to show the prompt. From what I observed, a white image is immediately returned without any interactions. Is this intended or considered a bug?
(In reply to catusx from comment #4)
> > [0] Although you're saying in the Web Extension use case, the prompt isn't
> > even shown?
> 
> No, in this case it's not even shown. I think this is due to everything
> happening on the background page, which is not a tab or a window so there is
> no place to show the prompt. From what I observed, a white image is
> immediately returned without any interactions. Is this intended or
> considered a bug?

No, that's intentional, we return a blank image immediately; otherwise we would have to block a synchronous API and that would break more things.  Once the permission were to be granted, calling the API again would return the correct image.
(In reply to Tom Ritter [:tjr] from comment #5)

> No, that's intentional, we return a blank image immediately; otherwise we
> would have to block a synchronous API and that would break more things. 
> Once the permission were to be granted, calling the API again would return
> the correct image.

Yes, I get it. But shall we expecting the permission prompt to be shown for WebExtension background pages or not? Or more generally, if any API that triggers a permission prompt is called on a WebExtension background pages, should the browser ask the user for permission?
(In reply to catusx from comment #6)
> (In reply to Tom Ritter [:tjr] from comment #5)
> 
> > No, that's intentional, we return a blank image immediately; otherwise we
> > would have to block a synchronous API and that would break more things. 
> > Once the permission were to be granted, calling the API again would return
> > the correct image.
> 
> Yes, I get it. But shall we expecting the permission prompt to be shown for
> WebExtension background pages or not? 

When this bug is fixed, WebExtensions will be able to get canvas data without a permission prompt. If you call the permissions.query API (once Bug 1429519 is implemented) it will return "granted".

> Or more generally, if any API that
> triggers a permission prompt is called on a WebExtension background pages,
> should the browser ask the user for permission?

I'm afraid I don't know the answer to this question, but flagging someone who probably does.
Flags: needinfo?(jhofmann)
(In reply to Tom Ritter [:tjr] from comment #7)
> (In reply to catusx from comment #6)
> > Or more generally, if any API that
> > triggers a permission prompt is called on a WebExtension background pages,
> > should the browser ask the user for permission?
> 
> I'm afraid I don't know the answer to this question, but flagging someone
> who probably does.

WebExtensions are using a separate permission system, so any web permission requested by a WebExtension background page should be blocked unless it has been ported and shows up in one of these lists:

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/permissions#API_permissions
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/optional_permissions

(Or unless the specific API, like for canvas, has decided to allow WebExtension principals)
Flags: needinfo?(jhofmann)
Comment on attachment 8961573 [details]
Bug 1412961 - Make extension principals bypass canvas permission checks.

https://reviewboard.mozilla.org/r/230440/#review236030

I think this should work fine, but I'll defer to Jeff and Tom for approval...

::: dom/canvas/CanvasUtils.cpp:60
(Diff revision 1)
>          return false;
>      }
>  
>      // Documents with system principal can always extract canvas data.
>      nsPIDOMWindowOuter *win = aDocument->GetWindow();
> -    nsCOMPtr<nsIScriptObjectPrincipal> sop(do_QueryInterface(win));
> +    nsIPrincipal *principal = nsContentUtils::SubjectPrincipal(aCx);

Note that this asserts that we're on the main thread. I don't think canvas is available for workers, but why can't we keep getting the principal from the SOP?

::: dom/canvas/CanvasUtils.cpp:71
(Diff revision 1)
>      if (nsContentUtils::ThreadsafeIsCallerChrome()) {
>          return true;
>      }
>  
> +    // Allow extension principals
> +    nsString addonId;

nit: nsAutoString

::: dom/canvas/CanvasUtils.cpp:72
(Diff revision 1)
>          return true;
>      }
>  
> +    // Allow extension principals
> +    nsString addonId;
> +    principal->GetAddonId(addonId);

Might be good to use an NS_WARN_IF here?

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Use_the_nice_macros
Attachment #8961573 - Flags: review?(jhofmann)
Attachment #8961573 - Flags: review?(tom)
Attachment #8961573 - Flags: review?(jmuizelaar)
Thanks for working on this!
Comment on attachment 8961573 [details]
Bug 1412961 - Make extension principals bypass canvas permission checks.

https://reviewboard.mozilla.org/r/230440/#review236094

Drive by review as reuqested by tjr. Overall that looks good except the potential duplicate check I pointed out underneath.

::: dom/canvas/CanvasUtils.cpp:61
(Diff revision 1)
>      }
>  
>      // Documents with system principal can always extract canvas data.
>      nsPIDOMWindowOuter *win = aDocument->GetWindow();
> -    nsCOMPtr<nsIScriptObjectPrincipal> sop(do_QueryInterface(win));
> -    if (sop && nsContentUtils::IsSystemPrincipal(sop->GetPrincipal())) {
> +    nsIPrincipal *principal = nsContentUtils::SubjectPrincipal(aCx);
> +    if (nsContentUtils::IsSystemPrincipal(principal)) {

I don't fully understand what we are checking here. As far as I understand it:

if (nsContentUtils::ThreadsafeIsCallerChrome()) {
is the same as

nsIPrincipal *principal = nsContentUtils::SubjectPrincipal(aCx);
if (nsContentUtils::IsSystemPrincipal(principal)) 

I suppose we could remove one of those calls, no?
Comment on attachment 8961573 [details]
Bug 1412961 - Make extension principals bypass canvas permission checks.

I'm giving f+ because I support this patch, but don't understand the principal system enough to give it a r+ (so I asked Christoph to double check it).
Attachment #8961573 - Flags: review?(tom) → feedback+
Comment on attachment 8961573 [details]
Bug 1412961 - Make extension principals bypass canvas permission checks.

https://reviewboard.mozilla.org/r/230440/#review236618

This seems ok, but I'm not a good person to review the checks.
Attachment #8961573 - Flags: review?(jmuizelaar)
With all stakeholders agreeing that they are okay with this I think it's fine to have ckerschbaumer (and optionally me) review the patch.
Assignee: nobody → ntim.bugs
Component: Security → Canvas: 2D
Product: Firefox → Core
Comment on attachment 8961573 [details]
Bug 1412961 - Make extension principals bypass canvas permission checks.

https://reviewboard.mozilla.org/r/230440/#review236712

Thanks for working on this patch, please address my concern and flag me again. Next review will be super fast!

::: dom/canvas/CanvasUtils.cpp:76
(Diff revision 3)
> +    nsAutoString addonId;
> +    NS_WARN_IF(NS_FAILED(sop->GetPrincipal()->GetAddonId(addonId)));
> +    if (!addonId.IsEmpty()) {
> +        return true;
> +    }
> +

Sorry for poking on this again, but sop might be null at this point which would cause a browser crash.

A few lines above we also need the principal [1]. Since that code is already there we shouldn't change the prerequisities within this patch. What we can do however is, assing the principal to a variable, then perform that IsSystemPrincipal check there and right after perform the addonId check within this patch.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/canvas/CanvasUtils.cpp#61
Attachment #8961573 - Flags: review?(ckerschb)
Comment on attachment 8961573 [details]
Bug 1412961 - Make extension principals bypass canvas permission checks.

https://reviewboard.mozilla.org/r/230440/#review236720

Yup, looks good, but please share the principal in a variable.

We should probably defer the duplicate system principal check to another bug...
Attachment #8961573 - Flags: review?(jhofmann)
Comment on attachment 8961573 [details]
Bug 1412961 - Make extension principals bypass canvas permission checks.

https://reviewboard.mozilla.org/r/230440/#review236924

Please make sure to address my comments before landing.

::: dom/canvas/CanvasUtils.cpp:61
(Diff revision 4)
>  
> -    // Documents with system principal can always extract canvas data.
>      nsPIDOMWindowOuter *win = aDocument->GetWindow();
>      nsCOMPtr<nsIScriptObjectPrincipal> sop(do_QueryInterface(win));
> -    if (sop && nsContentUtils::IsSystemPrincipal(sop->GetPrincipal())) {
> +    
> +    if (sop) {

nit: trailing whitespace.

::: dom/canvas/CanvasUtils.cpp:68
(Diff revision 4)
> +        nsIPrincipal *principal = sop->GetPrincipal();
> +        if (nsContentUtils::IsSystemPrincipal(principal)) {
> -        return true;
> +            return true;
> -    }
> +        }
> +    
> +        // Allow extension principals

nit: trailing whitespace.

::: dom/canvas/CanvasUtils.cpp:70
(Diff revision 4)
> -        return true;
> +            return true;
> -    }
> +        }
> +    
> +        // Allow extension principals
> +        nsAutoString addonId;
> +        NS_WARN_IF(NS_FAILED(principal->GetAddonId(addonId)));

please note that it's not guaranteed that sop->GetPrincipal() actually returns a non null principal.

That's not a problem for nsContentUtils::IsSystemPrincipal which performs the null check but is a problem if you access 'principal' directly.

Please add a null check there as well.
Attachment #8961573 - Flags: review?(ckerschb) → review+
Comment on attachment 8961573 [details]
Bug 1412961 - Make extension principals bypass canvas permission checks.

https://reviewboard.mozilla.org/r/230440/#review236978

Looks good to me, but see comment.

Thank you!

::: dom/canvas/CanvasUtils.cpp:71
(Diff revision 5)
> +        }
> +
> +        if (principal) {
> +            // Allow extension principals
> +            nsAutoString addonId;
> +            NS_WARN_IF(NS_FAILED(principal->GetAddonId(addonId)));

This needs to check its return value (https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/xpcom/base/nsDebug.h#40), so you could either put this in the if condition or add an "Unused <<" in front of it (it doesn't really matter in practice because all implementations of this return NS_OK and the string will still be empty if it fails)...
Attachment #8961573 - Flags: review?(jhofmann) → review+
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5c9399f5449f
Make extension principals bypass canvas permission checks. r=ckerschb,johannh
https://hg.mozilla.org/mozilla-central/rev/5c9399f5449f
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
This is still broken in today's Nightly: https://screenshots.firefox.com/MuIyjAo6CXedH122/www.mozilla.org
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to François Marier [:francois] from comment #29)
> This is still broken in today's Nightly:
> https://screenshots.firefox.com/MuIyjAo6CXedH122/www.mozilla.org

Hum, probably because I'm checking against the document's principal, which isn't an extension principal.

I should probably check against the subject principal?
status-firefox61: fixed → affected
Target Milestone: mozilla61 → ---
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
status-firefox61: affected → fixed
Resolution: --- → FIXED
Summary: privacy.resistFingerprinting breaks extensions that use extension-based canvas APIs (like Screenshots) → Fix canvas APIs in extension documents when resistFingerprinting is enabled
Target Milestone: --- → mozilla61
Blocks: 1453916
Why this fix wasn't uplifted to 60 to have it in new ESR?
(In reply to juraj.masiar from comment #32)
> Why this fix wasn't uplifted to 60 to have it in new ESR?

Officially: because Resist Fingerprinting is a feature for Tor, so there is no reason to uplift things into ESR that Tor doesn't need, since they use the ESRs. Enabling RFP in FF is a 'at your own risk' type of thing that we don't officially support.

Unofficially: no one asked for it. If the risk is low, we're early in a release cycle, and it rebases easily, we can usually put things like this into ESR. This is done at the mercy and with the grace of the release managers, especially since, as I said, there is no support for this feature.
Thank you for the info.
The Tor browser is the main reason I'm here now - I've been checking the brand new version based on the Firefox 60 and I found out this old bug.
I'm add-on developer and this was reported to me. I was somehow expecting it to be fixed in Tor since it's enabled there by default and it breaks all image manipulation for all add-ons.

At least it can be fixed by disabling the config.
Anyway, never mind, I will just tell users to disable fingerprinting. There are more important bugs to fix.
(In reply to juraj.masiar from comment #34)
> Thank you for the info.
> The Tor browser is the main reason I'm here now - I've been checking the
> brand new version based on the Firefox 60 and I found out this old bug.
> I'm add-on developer and this was reported to me. I was somehow expecting it
> to be fixed in Tor since it's enabled there by default and it breaks all
> image manipulation for all add-ons.
> 
> At least it can be fixed by disabling the config.
> Anyway, never mind, I will just tell users to disable fingerprinting. There
> are more important bugs to fix.

Tor Browser doesn't encourage users to install extensions, and having Tor Browser users disable fingerprinting would be disastrous for one's anonymity.

If you're running into issues in Tor Browser with this, I would ask the Tor Browser team if they would consider uplifting this patch into their tree (or requesting an uplift into ESR).

Recommending users disable fingerprinting in Tor Browser is a very wrong thing to do; but this is not the place to discuss it. https://trac.torproject.org/projects/tor/newticket
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: