Mike Perry Reporter
	Description • 10 years ago

The HTML5 canvas is a huge fingerprinting source. Content can render all sorts of material to an invisible canvas element in a way that varies by OS and by hardware. Targets to render include but not are not limited to fonts, SVG widgets, and WebGL (http://www.w2spconf.com/2012/papers/w2sp12-final4.pdf). This rendered image data can then be extracted and hashed, to produce a single, potentially unique identifier to track users without any actual identifier persistence on the machine.

In Tor Browser, we have opted to have the canvas return white image data until the user has accepted a doorhanger UI that flips a site permission to either enable or permanently block canvas access from that site.

This patch is against Firefox 24ESR.

	Arthur Edelstein [:arthur]
	Comment 1 • 9 years ago

For reference, here's a link that tracks the latest version of the Tor Browser patch:
https://torpat.ch/6253

	Anthony Miyaguchi [:amiyaguchi]
	Comment 2 • 8 years ago

I've attempted to rebase this patch on top of inbound, but there are a few things that prevent me from applying it cleanly.

The first is the lack of an API to access the first-party URI, implemented in the Tor Browser at https://torpat.ch/5742 adding `GetFirstPartyURI` and `GetFirstPartyIsolationURI` among others. I am wondering if the work on bug 1260931 is relevant to this problem and whether it will solve the missing references from the third party service. If not, what kinds of objections would there be to adding new entries to mozIThirdPartyUtil?

There is also an addition of an off-screen canvas that shares the ToBlob helper which extracts the canvas data. I am retaining the current behavior of the off-screen canvas. I am unsure about the interaction between the prerendering canvas and the visible one and is something I'd like to look into once this patch is applied and tested.

	Dave Huseby [:huseby]
	Comment 3 • 8 years ago

Before you get too far, I need to explain what we're doing about the isolation stuff. We're re-designing the isolation from the Tor's getFirstPartyURI and getFirstPartyIsolationURI to origin attributes.

I'm currently working on https://bgz.la/1260931 which is adding an origin attribute called firstPartyDomain that will contain the value that the getFirstParyURI/getFirstPartyIsolationURI returned.

Looking at the patch for https://bgz.la/967895, the correct fix is to get the origin attributes from the window's principal. Near the top of the function IsImageExtractionAllowed the tor patch gets the window from the document and then checks to see if the window's principal is a system principal. You will be able to use the same principal to get the firstPartyDomain origin attribute value instead of calling the ThirdPartyUtil::GetFirstPartyURI function.

That's the correct way to refactor this and all other isolation patches.

	Anthony Miyaguchi [:amiyaguchi]
	Comment 4 • 8 years ago

This is a work in progress patch until origin attributes supports FirstPartyUri. This does not run with expected behavior, but it will compile if the patch from https://bugzil.la/1260931 is applied beforehand.

	Jonathan Hao (inactive) [:jhao]
	Comment 6 • 7 years ago

This is only WIP, and mostly the same with Anthony's patch.

One thing that Anthony didn't do is blocking OffscreenCanvas.toBlob [1], which seems to be fingerprintable.  I'm not sure whether we should prompt the user or just block that API.

I'll discuss this with Tor people in person this week.

[1]: https://developer.mozilla.org/en-US/docs/Web/API/OffscreenCanvas/toBlob

	Arthur Edelstein [:arthur]
	Comment 7 • 7 years ago

(In reply to Jonathan Hao [:jhao] from comment #6)
> This is only WIP, and mostly the same with Anthony's patch.
> 
> One thing that Anthony didn't do is blocking OffscreenCanvas.toBlob [1],
> which seems to be fingerprintable.  I'm not sure whether we should prompt
> the user or just block that API.
> 
> I'll discuss this with Tor people in person this week.
> 
> [1]: https://developer.mozilla.org/en-US/docs/Web/API/OffscreenCanvas/toBlob

Hi Jonathan -- thanks for noticing this issue. Ideally we should attempt to prompt the user, I think. However, at Tor Project we haven't figured out how to do that yet (see https://trac.torproject.org/18599). At the moment we always return a placeholder for OffscreenCanvas::ToBlob (see our latest patch at https://torpat.ch/6253). I'd be happy to discuss further here or at our meeting.

	Jonathan Hao (inactive) [:jhao]
	Comment 8 • 7 years ago

I guess we could probably get the principal and hence the origin attributes from offscreenCanvas.GetGlobalObject().PrincipalOrNull(), but I'm not sure.

By the way, in IsImageExtractionAllowed(), have you considered just using the firstPartyDomain of the document's nodePrincipal instead of getting the URI from the top-level document?

	Arthur Edelstein [:arthur]
	Comment 9 • 7 years ago

(In reply to Jonathan Hao [:jhao] from comment #8)
> I guess we could probably get the principal and hence the origin attributes
> from offscreenCanvas.GetGlobalObject().PrincipalOrNull(), but I'm not sure.
> 
> By the way, in IsImageExtractionAllowed(), have you considered just using
> the firstPartyDomain of the document's nodePrincipal instead of getting the
> URI from the top-level document?

I hadn't, but that sounds like a good idea to me.

	Jonathan Hao (inactive) [:jhao]
	Comment 11 • 7 years ago

(In reply to Arthur Edelstein [:arthuredelstein] from comment #7)
> (In reply to Jonathan Hao [:jhao] from comment #6)
> > This is only WIP, and mostly the same with Anthony's patch.
> > 
> > One thing that Anthony didn't do is blocking OffscreenCanvas.toBlob [1],
> > which seems to be fingerprintable.  I'm not sure whether we should prompt
> > the user or just block that API.
> > 
> > I'll discuss this with Tor people in person this week.
> > 
> > [1]: https://developer.mozilla.org/en-US/docs/Web/API/OffscreenCanvas/toBlob
> 
> Hi Jonathan -- thanks for noticing this issue. Ideally we should attempt to
> prompt the user, I think. However, at Tor Project we haven't figured out how
> to do that yet (see https://trac.torproject.org/18599). At the moment we
> always return a placeholder for OffscreenCanvas::ToBlob (see our latest
> patch at https://torpat.ch/6253). I'd be happy to discuss further here or at
> our meeting.

I rebased Tor browser's latest patch but it doesn't work on Firefox.
The CanvasPermissionPromptHelper can only observe the notification from Gecko if e10s is turned off.

	Mark Smith [:mcs]
	Comment 13 • 7 years ago

(In reply to Jonathan Hao [:jhao] from comment #11)
> I rebased Tor browser's latest patch but it doesn't work on Firefox.
> The CanvasPermissionPromptHelper can only observe the notification from
> Gecko if e10s is turned off.

The e10s issue was recently fixed in Tor Browser by the following "fixup" patch:
 https://gitweb.torproject.org/tor-browser.git/commit/?id=1182bee9789a632f374f0a6d3ed90f21af1e37c5
The Tor bug report is https://trac.torproject.org/projects/tor/ticket/21778

	Jonathan Hao (inactive) [:jhao]
	Comment 15 • 7 years ago

(In reply to Mark Smith (:mcs) from comment #13)
> (In reply to Jonathan Hao [:jhao] from comment #11)
> > I rebased Tor browser's latest patch but it doesn't work on Firefox.
> > The CanvasPermissionPromptHelper can only observe the notification from
> > Gecko if e10s is turned off.
> 
> The e10s issue was recently fixed in Tor Browser by the following "fixup"
> patch:
>  https://gitweb.torproject.org/tor-browser.git/commit/
> ?id=1182bee9789a632f374f0a6d3ed90f21af1e37c5
> The Tor bug report is https://trac.torproject.org/projects/tor/ticket/21778

Thanks, Mark!

	Jonathan Hao (inactive) [:jhao]
	Comment 17 • 7 years ago

Hello Markus,

Could you help review this patch when you have time?

	Markus Stange [:mstange]
	Comment 19 • 7 years ago

The different parts of this patch will need to be reviewed and approved by different people. Please split it into smaller patches so that it's easier to read and reviewers clearly know which part they need to look at. For example, there should be a separate patch for each item in the list under "Also includes:", and probably even a separate bugzilla bug for each of them.

I can review the ImageEncoder::ExtractData part but not much else.

	Markus Stange [:mstange]
	Comment 20 • 7 years ago
mozreview-review

Comment on attachment 8849369 [details]
Bug 967895 - Prompt (w/ Site Permission) before allowing content to extract canvas data (Tor 6253)

https://reviewboard.mozilla.org/r/122156/#review141774

	Jonathan Hao (inactive) [:jhao]
	Comment 24 • 7 years ago

Thanks, Markus.  I've split it into three patches.

Panos, could you review the browser part?

George, could you review the Canvas 2D part?

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 25 • 7 years ago

Jonathan, is the browser part really just adding the 3 new icon files? Has the actual permission prompt landed already in another bug?

	Jonathan Hao (inactive) [:jhao]
	Comment 27 • 7 years ago

(In reply to Panos Astithas [:past] (please needinfo?) from comment #25)
> Jonathan, is the browser part really just adding the 3 new icon files? Has
> the actual permission prompt landed already in another bug?

Sorry, Panos.  Now this patch is correct.

Thanks.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 28 • 7 years ago
mozreview-review

Comment on attachment 8849369 [details]
Bug 967895 - Prompt (w/ Site Permission) before allowing content to extract canvas data (Tor 6253)

https://reviewboard.mozilla.org/r/122156/#review143110

I don't think I'll have the time this week to look at this, maybe Paolo can.

	Panos Astithas (he/him) [:past] (please ni?)
	Comment 29 • 7 years ago

Comment on attachment 8849369 [details]
Bug 967895 - Prompt (w/ Site Permission) before allowing content to extract canvas data (Tor 6253)

Apparently I don't even have the time to make MozReview work.

	:Paolo Amadini
	Comment 30 • 7 years ago

Comment on attachment 8849369 [details]
Bug 967895 - Prompt (w/ Site Permission) before allowing content to extract canvas data (Tor 6253)

The icon should just be a new path along the other existing site permission icons.

Redirecting to Johann for the rest. Also the permission name cannot be changed once decided, make sure you really want "canvas/extractData".

	Johann Hofmann [:johannh]
	Comment 31 • 7 years ago
mozreview-review

Comment on attachment 8849369 [details]
Bug 967895 - Prompt (w/ Site Permission) before allowing content to extract canvas data (Tor 6253)

https://reviewboard.mozilla.org/r/122156/#review148348

I feel like these are enough comments to clear review.

In general, you will want to avoid using Services.perms directly and instead turn this into a site permission managed through SitePermissions.jsm. This will display the permission in the identity icon, the identity popup and the page info screen and allow the user to revert their decision easily. An example of how to add a site permission can be seen in bug 1224137. I don't require you to do that in this bug, though.

::: browser/base/content/browser.js:1388
(Diff revision 8)
>      Services.obs.addObserver(gXPInstallObserver, "addon-install-complete");
>      window.messageManager.addMessageListener("Browser:URIFixup", gKeywordURIFixup);
>  
>      BrowserOffline.init();
>      IndexedDBPromptHelper.init();
> +    CanvasPermissionPromptHelper.init();

This needs a check for the resistFingerprinting pref to avoid initializing when it's not needed.

::: browser/base/content/browser.js:6460
(Diff revision 8)
> +var CanvasPermissionPromptHelper = {
> +  _permissionsPrompt: "canvas-permissions-prompt",
> +  _notificationIcon: "canvas-notification-icon",
> +
> +  init:
> +  function CanvasPermissionPromptHelper_init() {

Please change all functions in here to use shorthand method definitions:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Method_definitions

::: browser/base/content/browser.js:6465
(Diff revision 8)
> +  function CanvasPermissionPromptHelper_init() {
> +    if (document.styleSheets && (document.styleSheets.length > 0)) try {
> +      let ruleText = "panel[popupid=canvas-permissions-prompt] description { white-space: pre-wrap; }";
> +      let sheet = document.styleSheets[0];
> +      sheet.insertRule(ruleText, sheet.cssRules.length);
> +    } catch (e) {}

I think it's reasonable to set this in https://searchfox.org/mozilla-central/source/toolkit/themes/shared/popupnotification.inc.css

for .popup-notification-description

::: browser/base/content/browser.js:6480
(Diff revision 8)
> +  // aSubject is an nsIBrowser (e10s) or an nsIDOMWindow (non-e10s).
> +  // aData is an URL string.
> +  observe:
> +  function CanvasPermissionPromptHelper_observe(aSubject, aTopic, aData) {
> +    if (aTopic != this._permissionsPrompt) {
> +      throw new Error("Unexpected topic");

You should probably just return here.

::: browser/base/content/browser.js:6490
(Diff revision 8)
> +      browser = aSubject.QueryInterface(Ci.nsIBrowser);
> +    } catch (e) {}
> +
> +    if (!browser) {
> +      try {
> +        let contentWindow = aSubject.QueryInterface(Ci.nsIDOMWindow);

If there's no way around having both a content window and a browser as subject, I'd suggest something like

if (aSubject instanceof Ci.nsIDOMWindow) {
  let contentWindow = QI(...
  browser = ...
else {
  browser = QI(...)
}

instead of the whole try-catch-empty

::: browser/base/content/browser.js:6500
(Diff revision 8)
> +        throw new Error("No browser");
> +      }
> +    }
> +
> +    if (!aData) {
> +      throw new Error("Missing URL");

Why are you throwing this error? What is handling it?

::: browser/base/content/browser.js:6513
(Diff revision 8)
> +
> +    // If canvas prompt is already displayed, just return.  This is OK (and
> +    // more efficient) since this permission is associated with the top
> +    // browser's URL.
> +    if (PopupNotifications.getNotification(aTopic, browser))
> +      return;

I believe that the PopupNotification API does this for you already

::: browser/base/content/browser.js:6515
(Diff revision 8)
> +    // more efficient) since this permission is associated with the top
> +    // browser's URL.
> +    if (PopupNotifications.getNotification(aTopic, browser))
> +      return;
> +
> +    let brandBundle = Services.strings.createBundle("chrome://branding/locale/brand.properties");

This should use document.getElementById("bundle_brand").getString instead, see other examples in this file.

::: browser/base/content/browser.js:6517
(Diff revision 8)
> +    if (PopupNotifications.getNotification(aTopic, browser))
> +      return;
> +
> +    let brandBundle = Services.strings.createBundle("chrome://branding/locale/brand.properties");
> +    let productName = brandBundle.GetStringFromName("brandShortName");
> +    var message = getLocalizedString("canvas.siteprompt",

let message

::: browser/base/content/browser.js:6520
(Diff revision 8)
> +    let brandBundle = Services.strings.createBundle("chrome://branding/locale/brand.properties");
> +    let productName = brandBundle.GetStringFromName("brandShortName");
> +    var message = getLocalizedString("canvas.siteprompt",
> +                                     [ uri.asciiHost, productName ]);
> +
> +    var mainAction = {

Please make the "Allow" action the main action.

Since you originally wrote this patch the design of notification doorhangers has changed. It now displays the main action next to the first secondary action, so the user can see both choices. By our UX convention, the main action is always the positive/allow choice, while the secondary action is the negative/deny choice. Not breaking this pattern is important to avoid messing with user intuition.

::: browser/base/content/browser.js:6520
(Diff revision 8)
> +    let brandBundle = Services.strings.createBundle("chrome://branding/locale/brand.properties");
> +    let productName = brandBundle.GetStringFromName("brandShortName");
> +    var message = getLocalizedString("canvas.siteprompt",
> +                                     [ uri.asciiHost, productName ]);
> +
> +    var mainAction = {

Nit: use let

::: browser/base/content/browser.js:6528
(Diff revision 8)
> +      callback() {
> +        return null;
> +      }
> +    };
> +
> +    var secondaryActions = [

Nit: use let

::: browser/base/content/browser.js:6545
(Diff revision 8)
> +            setCanvasPermission(uri, Ci.nsIPermissionManager.ALLOW_ACTION);
> +        }
> +      }
> +    ];
> +
> +    function getLocalizedString(aID, aParams) {

I don't really see how this abstraction adds any value, it's being called with params only once and gNavigatorBundle.getString isn't much longer than getLocalizedString...

::: browser/base/content/browser.js:6554
(Diff revision 8)
> +      return gNavigatorBundle.getString(aID);
> +    }
> +
> +    function setCanvasPermission(aURI, aPerm) {
> +      Services.perms.add(aURI, "canvas/extractData", aPerm,
> +                         Ci.nsIPermissionManager.EXPIRE_NEVER);

Note that permanently saving these permissions in private mode violates the principle that no website data is persisted in private browsing.

Our general pattern for permission prompts is to add a checkbox in the popup options that allows the user to specify if they want to permanently store the permission. It's hidden in PBM, see https://searchfox.org/mozilla-central/rev/b318c7dca7392bd16c0b11929f55b1be133f0b31/browser/modules/PermissionUI.jsm#623

The PopupNotification.show docs have more info on how to use the checkbox:
https://searchfox.org/mozilla-central/rev/b318c7dca7392bd16c0b11929f55b1be133f0b31/toolkit/modules/PopupNotifications.jsm#317

::: browser/base/content/browser.xul:748
(Diff revision 8)
>                    <image id="geo-notification-icon" class="notification-anchor-icon geo-icon" role="button"
>                           tooltiptext="&urlbar.geolocationNotificationAnchor.tooltip;"/>
>                    <image id="addons-notification-icon" class="notification-anchor-icon install-icon" role="button"
>                           tooltiptext="&urlbar.addonsNotificationAnchor.tooltip;"/>
> +                  <image id="canvas-notification-icon" class="notification-anchor-icon" role="button"
> +                         tooltiptext=""/>

You should add a tooltiptext.

::: browser/themes/linux/browser.css:1321
(Diff revision 8)
> +  list-style-image: url(chrome://browser/skin/canvas-popup.svg);
> +}
> +
> +#canvas-notification-icon {
> +  list-style-image: url(chrome://browser/skin/canvas-popup.svg);
> +}

These two rules should be consolidated into a single rule and moved to https://searchfox.org/mozilla-central/source/browser/themes/shared/notification-icons.inc.css

::: browser/themes/linux/canvas-popup.svg:1
(Diff revision 8)
> +<?xml version="1.0" encoding="UTF-8"?>

This icon looks out of place and doesn't adjust well to dark themes. Below a few ideas how to solve that. The icon should also be in browser/themes/shared

::: browser/themes/linux/canvas-popup.svg:8
(Diff revision 8)
> +
> + <metadata>image/svg+xmlOpen Clip Art Librarypalette2009-02-17T21:15:25http://openclipart.org/detail/21620/palette-by-benbenclip artclipartcolorcoloriconiconimagemediapaintpaintpalettepalettepngpublic domainsvg</metadata>
> + <g>
> +  <title>Layer 1</title>
> +  <g id="layer1">
> +   <rect fill="#000000" stroke-width="2" stroke-miterlimit="4" stroke-dashoffset="0" ry="23.587006" rx="26.785715" y="0" x="0" height="200" width="200" id="rect6786"/>

Remove this background rect

::: browser/themes/linux/canvas-popup.svg:10
(Diff revision 8)
> + <g>
> +  <title>Layer 1</title>
> +  <g id="layer1">
> +   <rect fill="#000000" stroke-width="2" stroke-miterlimit="4" stroke-dashoffset="0" ry="23.587006" rx="26.785715" y="0" x="0" height="200" width="200" id="rect6786"/>
> +   <g transform="matrix(4.65116, 0, 0, 4.65116, -1717.85, -314.201)" id="g6820">
> +    <path fill="#ffffff" stroke-width="1.924773" stroke-miterlimit="4" stroke-dasharray="3.84954572, 3.84954572" stroke-dashoffset="3.657068" d="m388.618042,70.128563c-4.746887,0 -18.367279,0.849472 -18.34375,12.3125c0.001312,0.644249 1.226776,2.886879 2.25,3.125c7.5755,1.762962 7.986664,7.834511 7.53125,12.625c-0.03479,0.362747 0.589233,2.891006 1.125,3.53125c4.344635,5.191872 5.822723,7.468742 11.6875,7.46875c11.729614,0 18.3125,-5.93206 18.3125,-17.8125c0,-11.880463 -10.832916,-21.25 -22.5625,-21.25zm-11.875,8.1875c0.986481,0.058029 2.139893,0.464394 3.21875,1.1875c2.157715,1.44622 3.225708,3.668251 2.375,4.9375c-0.850739,1.269257 -3.311005,1.10247 -5.46875,-0.34375c-2.157745,-1.446228 -3.194458,-3.637001 -2.34375,-4.90625c0.425354,-0.634628 1.232269,-0.933029 2.21875,-0.875z" id="path6822"/>

Make this fill="context-fill" and give it fill rules like https://searchfox.org/mozilla-central/rev/b318c7dca7392bd16c0b11929f55b1be133f0b31/browser/themes/shared/identity-block/identity-block.inc.css#59 and https://searchfox.org/mozilla-central/rev/b318c7dca7392bd16c0b11929f55b1be133f0b31/browser/themes/shared/notification-icons.inc.css#29

	Jonathan Hao (inactive) [:jhao]
	Comment 32 • 7 years ago

Hi Ethan,

Could you review the patches related to Canvas or recommend someone who can?

Thanks.

	Ethan Lin[:ethlin]
	Comment 33 • 7 years ago

(In reply to Jonathan Hao [:jhao] from comment #32)
> Hi Ethan,
> 
> Could you review the patches related to Canvas or recommend someone who can?
> 
> Thanks.

I think you can send review request to :jrmuizel or :nical for the patches related to Canvas.

	Jonathan Hao (inactive) [:jhao]
	Comment 37 • 7 years ago

Hi Johann, thanks for your review.  Please take a look again, thanks.

	Jeff Muizelaar [:jrmuizel]
	Comment 38 • 7 years ago

Sorry for the delay in review...

Does the Tor browser expose identifying information through canvas? It seems like as long as gpu accelerated canvas is disabled the results should only depend on the OS and version of Firefox. Is there more information that we're leaking?

	Arthur Edelstein [:arthur]
	Comment 39 • 7 years ago

(In reply to Jeff Muizelaar [:jrmuizel] from comment #38)
> Sorry for the delay in review...
> 
> Does the Tor browser expose identifying information through canvas? It seems
> like as long as gpu accelerated canvas is disabled the results should only
> depend on the OS and version of Firefox. Is there more information that
> we're leaking?

That's a very good question. Installed fonts would be an additional source of entropy leaked by canvas rendering, but I believe our font whitelisting/bundling mitigates this part of the problem in Tor Browser (and would when uplifted to Firefox). So I suppose the remaining questions are

* Can we fully disable all hardware-based or hardware-influenced rendering?
* Are there no software-only rendering differences across versions of a single OS (e.g., Windows 7 vs Windows 10 or Ubuntu vs Fedora)?
* Can we emulate a glCanvas without using hardware?

I don't know the canvas codebase well enough to answer these questions. Jeff, do you have any opinion on these? My inclination would be to stick with the canvas prompt unless we are highly confident that the answer to all these questions is "no".

	Jeff Muizelaar [:jrmuizel]
	Comment 40 • 7 years ago

(In reply to Arthur Edelstein [:arthuredelstein] from comment #39)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #38)
> > Sorry for the delay in review...
> > 
> > Does the Tor browser expose identifying information through canvas? It seems
> > like as long as gpu accelerated canvas is disabled the results should only
> > depend on the OS and version of Firefox. Is there more information that
> > we're leaking?
> 
> That's a very good question. Installed fonts would be an additional source
> of entropy leaked by canvas rendering, but I believe our font
> whitelisting/bundling mitigates this part of the problem in Tor Browser (and
> would when uplifted to Firefox). So I suppose the remaining questions are
> 
> * Can we fully disable all hardware-based or hardware-influenced rendering?

This should be possible, although there could be bugs that expose differences based on cpu features

> * Are there no software-only rendering differences across versions of a
> single OS (e.g., Windows 7 vs Windows 10 or Ubuntu vs Fedora)?

Thinking about this more, it's probably true that they way text is drawn may very between versions of the OS. This would be detectable through canvas.

> * Can we emulate a glCanvas without using hardware?

Do you mean WebGL? It is possible to emulate this WebGL without using hardware. We do this with the system software implementation on the different OSes. It's possible that this might leak information about the OS version.

> 
> I don't know the canvas codebase well enough to answer these questions.
> Jeff, do you have any opinion on these? My inclination would be to stick
> with the canvas prompt unless we are highly confident that the answer to all
> these questions is "no".

So the answers suggest that we might be ok, but my level of confidence is not high enough. So I agree, it probably makes sense to push forward with the prompt.

	Arthur Edelstein [:arthur]
	Comment 41 • 7 years ago

Thanks, Jeff! (And yes, I meant WebGL.) Bug 1041818 could be the place where we fully mitigate canvas fingerprinting in the future, so no prompt will be needed. But in the meantime I think this prompt approach is a good solution.

	Johann Hofmann [:johannh]
	Comment 42 • 7 years ago
mozreview-review

Comment on attachment 8849369 [details]
Bug 967895 - Prompt (w/ Site Permission) before allowing content to extract canvas data (Tor 6253)

https://reviewboard.mozilla.org/r/122156/#review159050

Sorry for the delay. We're getting close. :)

::: browser/base/content/browser.js:6448
(Diff revision 9)
> +      browser = gBrowser.getBrowserForContentWindow(contentWindow);
> +    } else {
> +      browser = aSubject.QueryInterface(Ci.nsIBrowser);
> +    }
> +
> +    let uri = makeURI(aData);

Please use Services.io.newUri instead. makeURI is just around for compatibility. I filed bug 1379143 to get rid of it completely.

::: browser/base/content/browser.js:6454
(Diff revision 9)
> +    if (gBrowser.selectedBrowser !== browser) {
> +      // Must belong to some other window.
> +      return;
> +    }
> +
> +    let brandShortName = document.getElementById("bundle_brand").getString("brandShortName");

Nit: Would be more readable if you split this up and make a brandBundle variable.

::: browser/base/content/browser.js:6466
(Diff revision 9)
> +        setCanvasPermission(uri, Ci.nsIPermissionManager.ALLOW_ACTION,
> +                            state && state.checkboxChecked);
> +      }
> +    };
> +
> +    let secondaryActions = [

You don't need two secondary actions if you have the checkbox. You want a structure similar to e.g. the geolocation prompt. You can just take the "never" action and call it "dontAllow" instead and throw away the notNow action.

::: browser/locales/en-US/chrome/browser/browser.dtd:212
(Diff revision 9)
>  <!ENTITY urlbar.viewSiteInfo.label                      "View site information">
>  
>  <!ENTITY urlbar.defaultNotificationAnchor.tooltip         "Open message panel">
>  <!ENTITY urlbar.geolocationNotificationAnchor.tooltip     "Open location request panel">
>  <!ENTITY urlbar.addonsNotificationAnchor.tooltip          "Open add-on installation message panel">
> +<!ENTITY urlbar.canvasNotificationAncher.tooltip          "Manage canvas extraction permission">

typo: canvasNotificationAnchor

::: browser/locales/en-US/chrome/browser/browser.properties:453
(Diff revision 9)
> +# Canvas permission prompt
> +# LOCALIZATION NOTE (canvas.siteprompt): %1$S is hostname, %2$S is brandShortName (e.g. Firefox)
> +canvas.siteprompt=This website (%1$S) attempted to extract HTML5 canvas image data, which may be used to uniquely identify your computer.\n\nShould %2$S allow this website to extract HTML5 canvas image data?
> +canvas.notNow=Not Now
> +canvas.notNowAccessKey=N
> +canvas.allow=Allow in the Future

I think "Allow in the Future" isn't really fitting to our copy, looking at https://searchfox.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.properties#517 you could either use a descriptive string such as "Allow Image Data Access" (which is quite long) or just say "Allow" (which I would recommend in this case).

::: browser/locales/en-US/chrome/browser/browser.properties:457
(Diff revision 9)
> +canvas.notNowAccessKey=N
> +canvas.allow=Allow in the Future
> +canvas.allowAccessKey=A
> +canvas.never=Never for This Site
> +canvas.neverAccessKey=e
> +canvas.remember=Remember this decision across sessions

Again looking at https://searchfox.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.properties#517, we usually just say "Remember this decision".

::: browser/themes/linux/canvas-popup.svg:1
(Diff revision 9)
> +<?xml version="1.0" encoding="UTF-8"?>

You don't need this declaration.

::: browser/themes/linux/canvas-popup.svg:4
(Diff revision 9)
> +<?xml version="1.0" encoding="UTF-8"?>
> +<svg width="200" height="200" xmlns="http://www.w3.org/2000/svg">
> +
> + <metadata>image/svg+xmlOpen Clip Art Librarypalette2009-02-17T21:15:25http://openclipart.org/detail/21620/palette-by-benbenclip artclipartcolorcoloriconiconimagemediapaintpaintpalettepalettepngpublic domainsvg</metadata>

You can remove the credit I think

::: browser/themes/linux/canvas-popup.svg:5
(Diff revision 9)
> +<?xml version="1.0" encoding="UTF-8"?>
> +<svg width="200" height="200" xmlns="http://www.w3.org/2000/svg">
> +
> + <metadata>image/svg+xmlOpen Clip Art Librarypalette2009-02-17T21:15:25http://openclipart.org/detail/21620/palette-by-benbenclip artclipartcolorcoloriconiconimagemediapaintpaintpalettepalettepngpublic domainsvg</metadata>
> + <g>

Try removing this group.

::: browser/themes/linux/canvas-popup.svg:6
(Diff revision 9)
> +<?xml version="1.0" encoding="UTF-8"?>
> +<svg width="200" height="200" xmlns="http://www.w3.org/2000/svg">
> +
> + <metadata>image/svg+xmlOpen Clip Art Librarypalette2009-02-17T21:15:25http://openclipart.org/detail/21620/palette-by-benbenclip artclipartcolorcoloriconiconimagemediapaintpaintpalettepalettepngpublic domainsvg</metadata>
> + <g>
> +  <title>Layer 1</title>

Remove the title.

::: browser/themes/linux/canvas-popup.svg:7
(Diff revision 9)
> +<svg width="200" height="200" xmlns="http://www.w3.org/2000/svg">
> +
> + <metadata>image/svg+xmlOpen Clip Art Librarypalette2009-02-17T21:15:25http://openclipart.org/detail/21620/palette-by-benbenclip artclipartcolorcoloriconiconimagemediapaintpaintpalettepalettepngpublic domainsvg</metadata>
> + <g>
> +  <title>Layer 1</title>
> +  <g id="layer1">

Try removing this group as well.

::: browser/themes/linux/canvas-popup.svg:14
(Diff revision 9)
> +    <path fill="context-fill" stroke-width="1.924773" stroke-miterlimit="4" stroke-dasharray="3.84954572, 3.84954572" stroke-dashoffset="3.657068" d="m388.618042,70.128563c-4.746887,0 -18.367279,0.849472 -18.34375,12.3125c0.001312,0.644249 1.226776,2.886879 2.25,3.125c7.5755,1.762962 7.986664,7.834511 7.53125,12.625c-0.03479,0.362747 0.589233,2.891006 1.125,3.53125c4.344635,5.191872 5.822723,7.468742 11.6875,7.46875c11.729614,0 18.3125,-5.93206 18.3125,-17.8125c0,-11.880463 -10.832916,-21.25 -22.5625,-21.25zm-11.875,8.1875c0.986481,0.058029 2.139893,0.464394 3.21875,1.1875c2.157715,1.44622 3.225708,3.668251 2.375,4.9375c-0.850739,1.269257 -3.311005,1.10247 -5.46875,-0.34375c-2.157745,-1.446228 -3.194458,-3.637001 -2.34375,-4.90625c0.425354,-0.634628 1.232269,-0.933029 2.21875,-0.875z" id="path6822"/>
> +    <path fill="#ff7a00" stroke-width="1.924773" stroke-miterlimit="4" stroke-dasharray="3.84954572, 3.84954572" stroke-dashoffset="3.657068" id="path6824" d="m392.586761,76.615746a3.5,3.5 0 1 1 -7,0a3.5,3.5 0 1 1 7,0z"/>
> +    <path fill="#95d300" stroke-width="1.924773" stroke-miterlimit="4" stroke-dasharray="3.84954572, 3.84954572" stroke-dashoffset="3.657068" id="path6826" d="m402.524292,81.553246a3.5,3.5 0 1 1 -7,0a3.5,3.5 0 1 1 7,0z"/>
> +    <path fill="#00a6e4" stroke-width="1.924773" stroke-miterlimit="4" stroke-dasharray="3.84954572, 3.84954572" stroke-dashoffset="3.657068" id="path6828" d="m408.461792,90.615746a3.5,3.5 0 1 1 -7,0a3.5,3.5 0 1 1 7,0z"/>
> +    <path fill="#f9de00" stroke-width="1.924773" stroke-miterlimit="4" stroke-dasharray="3.84954572, 3.84954572" stroke-dashoffset="3.657068" id="path6830" d="m403.643463,99.60791a3.5,3.5 0 1 1 -7,0a3.5,3.5 0 1 1 7,0z"/>
> +    <path fill="#e600ad" stroke-width="1.924773" stroke-miterlimit="4" stroke-dasharray="3.84954572, 3.84954572" stroke-dashoffset="3.657068" id="path6832" d="m393.518463,100.60791a3.5,3.5 0 1 1 -7,0a3.5,3.5 0 1 1 7,0z"/>

Remove the ids from all elements please.

::: browser/themes/linux/jar.mn:110
(Diff revision 9)
>    skin/classic/browser/syncProgress-horizontalbar.png
>    skin/classic/browser/syncProgress-horizontalbar@2x.png
>  #ifdef E10S_TESTING_ONLY
>    skin/classic/browser/e10s-64@2x.png (../shared/e10s-64@2x.png)
>  #endif
> +  skin/classic/browser/canvas-popup.svg

You might not have seen my last comment, please move this file into browser/themes/shared/

::: toolkit/themes/shared/popupnotification.inc.css:131
(Diff revision 9)
>    list-style-image: url(chrome://global/skin/icons/menubutton-dropmarker.svg);
>    -moz-context-properties: fill;
>    fill: currentColor;
>  }
> +
> +.popup-notification-description[popupid=canvas-permissions-prompt] description {

That selector isn't matching correctly, it should work if you remove the "description" child selector.

::: toolkit/themes/shared/popupnotification.inc.css:133
(Diff revision 9)
>    fill: currentColor;
>  }
> +
> +.popup-notification-description[popupid=canvas-permissions-prompt] description {
> +  white-space: pre-wrap;
> +}

On second thought this rule should probably live in browser/base/content/browser.css, sorry for that.

	Jeff Muizelaar [:jrmuizel]
	Comment 43 • 7 years ago
mozreview-review

Comment on attachment 8867077 [details]
Bug 967895 - Add an option to return a placeholder when extracting image data. (Tor 6253)

https://reviewboard.mozilla.org/r/138686/#review164984

	Chung-Sheng Fu [:cfu] Assignee
	Comment 48 • 7 years ago

I fixed the issues in jhao's patches.  Please take a look again.

I also added a test that the canvas data should be the same as a canvas filled with white color (the placeholder) before the permission is granted.

Thank you very much.

	Ethan Tseng [:ethan]
	Comment 49 • 7 years ago

Per CS, the current patches did not apply the new UI designed in bug 1382111 yet.
Please hold off the review process.

	Chung-Sheng Fu [:cfu] Assignee
	Comment 50 • 7 years ago

Comment on attachment 8901645 [details]
Bug 967895 - Prompt (w/ Site Permission) before allowing content to extract canvas data (Tor 6253).

Sorry I have to cancel the review request of attachment 8901645 [details] for a while because the UI design is not up-to-date.

	Jeff Muizelaar [:jrmuizel]
	Comment 51 • 7 years ago

How has https://reviewboard.mozilla.org/r/173060/diff/1#index_header changed compared to https://reviewboard.mozilla.org/r/138686/diff/2/?

	Chung-Sheng Fu [:cfu] Assignee
	Comment 52 • 7 years ago

(In reply to Jeff Muizelaar [:jrmuizel] from comment #51)
> How has https://reviewboard.mozilla.org/r/173060/diff/1#index_header changed
> compared to https://reviewboard.mozilla.org/r/138686/diff/2/?

They are exactly the same.  Here is the diff result

    cfu-40226:gecko cfu$ diff <(git show HEAD) <(git show 10788750a9bbd877130ed97a8c9643d7cc3ff7b7)
    1c1
    < commit dfbb5bc81f015221cda743ac567a3bcf070d36d0
    ---
    > commit 10788750a9bbd877130ed97a8c9643d7cc3ff7b7
    3c3
    < Date:   Fri Sep 1 11:37:02 2017 +0800
    ---
    > Date:   Tue Aug 22 11:12:49 2017 +0800
    5c5
    <     jhao's original patch
    ---
    >     Bug 967895 - Add an option to return a placeholder when extracting image data. (Tor 6253)
    7c7
    <     MozReview-Commit-ID: 2hMuCbzPgUA
    ---
    >     MozReview-Commit-ID: 1zTOUL7CZ1E
    10c10
    < index 40b51881bbb1..c3f99cb75173 100644
    ---
    > index b263222ee751..ab644f301fad 100644
    cfu-40226:gecko cfu$

where HEAD is jhao's original patch (applied to the same revision as it was) and 10788750... is the new one (applied to the latest code base).

If there is still any concern about the patches, please ni? me again.  Sorry for confusing you.

	Chung-Sheng Fu [:cfu] Assignee
	Comment 57 • 7 years ago

Now the prompt follows the UX designer's instruction in Bug 1382111 (svg, message, main/secondary actions).  Currently there is no available learn more page because fingerprinting resistance is still a hidden pref, so the designer suggests we add it in the future.  The designer may slightly update the notification message later, but I think we can start reviewing the patches.

Thank you very much.

	Ethan Tseng [:ethan]
	Comment 58 • 7 years ago

(In reply to Chung-Sheng Fu [:cfu] from comment #57)
> Currently there is no available learn more page because fingerprinting resistance is still a hidden pref, so the
> designer suggests we add it in the future.

I filed bug 1397757 to follow up adding this support page.

	Johann Hofmann [:johannh]
	Comment 63 • 7 years ago
mozreview-review

Comment on attachment 8901645 [details]
Bug 967895 - Prompt (w/ Site Permission) before allowing content to extract canvas data (Tor 6253).

https://reviewboard.mozilla.org/r/173058/#review183210

The code looks mostly good, just a bit of cleanup required around various ends.

Can you add a test that asserts that the parts in patch (showing the permission prompt and confirming/denying) work as expected? You could put it in browser/base/content/test/permissions, for example.

::: browser/base/content/browser.css:1393
(Diff revision 3)
>  .popup-notification-invalid-input[focused] {
>    box-shadow: 0 0 2px 2px rgba(255,0,0,0.4);
>  }
>  
> +.popup-notification-description[popupid=canvas-permissions-prompt] {
> +  white-space: pre-wrap;

Is this still needed?

::: browser/base/content/browser.js:1892
(Diff revision 3)
>        if (AppConstants.isPlatformAndVersionAtLeast("win", "10")) {
>          MenuTouchModeObserver.uninit();
>        }
>        BrowserOffline.uninit();
>        IndexedDBPromptHelper.uninit();
> +      if (Services.prefs.getBoolPref("privacy.resistFingerprinting", false)) {

Aren't we leaking observers if the user flipped the pref to false during the browsing session? I would recommend putting this in a try-catch block instead.

::: browser/base/content/browser.js:6668
(Diff revision 3)
> +        setCanvasPermission(uri, Ci.nsIPermissionManager.DENY_ACTION,
> +                            state && state.checkboxChecked);
> +      }
> +    }];
> +
> +    function setCanvasPermission(aURI, aPerm, aPersistent) {

Nit: I think it's a bit cleaner to put this function above of where it's first used.

::: browser/locales/en-US/chrome/browser/browser.properties:460
(Diff revision 3)
>  offlineApps.manageUsage=Show settings
>  offlineApps.manageUsageAccessKey=S
>  
> +# Canvas permission prompt
> +# LOCALIZATION NOTE (canvas.siteprompt): %S is hostname
> +canvas.siteprompt=Will you allow %S to use your HTML5 canvas image data?  This may be used to uniquely identify your computer.

Nit: Is there a double space between "?" and "T"?

::: browser/locales/en-US/chrome/browser/browser.properties:462
(Diff revision 3)
>  
> +# Canvas permission prompt
> +# LOCALIZATION NOTE (canvas.siteprompt): %S is hostname
> +canvas.siteprompt=Will you allow %S to use your HTML5 canvas image data?  This may be used to uniquely identify your computer.
> +canvas.notAllow=Don’t Allow
> +canvas.notAllowAccessKey=N

I think it's better practice to make the accesskeys come up in the strings case-sensitive, so please lowercase this "n".

::: browser/locales/en-US/chrome/browser/browser.properties:462
(Diff revision 3)
>  
> +# Canvas permission prompt
> +# LOCALIZATION NOTE (canvas.siteprompt): %S is hostname
> +canvas.siteprompt=Will you allow %S to use your HTML5 canvas image data?  This may be used to uniquely identify your computer.
> +canvas.notAllow=Don’t Allow
> +canvas.notAllowAccessKey=N

Please call this canvas.notAllow.accesskey instead (or canvas.dontAllow.accesskey)

::: browser/locales/en-US/chrome/browser/browser.properties:464
(Diff revision 3)
> +# LOCALIZATION NOTE (canvas.siteprompt): %S is hostname
> +canvas.siteprompt=Will you allow %S to use your HTML5 canvas image data?  This may be used to uniquely identify your computer.
> +canvas.notAllow=Don’t Allow
> +canvas.notAllowAccessKey=N
> +canvas.allow=Allow Data Access
> +canvas.allowAccessKey=Y

The access key must be present in your label string, so maybe "A"?

::: browser/locales/en-US/chrome/browser/browser.properties:464
(Diff revision 3)
> +# LOCALIZATION NOTE (canvas.siteprompt): %S is hostname
> +canvas.siteprompt=Will you allow %S to use your HTML5 canvas image data?  This may be used to uniquely identify your computer.
> +canvas.notAllow=Don’t Allow
> +canvas.notAllowAccessKey=N
> +canvas.allow=Allow Data Access
> +canvas.allowAccessKey=Y

Please call this canvas.allow.accesskey

::: browser/themes/shared/canvas-popup.svg:1
(Diff revision 3)
> +<svg xmlns="http://www.w3.org/2000/svg" width="481.156" height="508.687" viewBox="0 0 481.156 508.687">

I believe this file needs a license header:

<!-- This Source Code Form is subject to the terms of the Mozilla Public
   - License, v. 2.0. If a copy of the MPL was not distributed with this
   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->

::: browser/themes/shared/canvas-popup.svg:5
(Diff revision 3)
> +<svg xmlns="http://www.w3.org/2000/svg" width="481.156" height="508.687" viewBox="0 0 481.156 508.687">
> +  <defs>
> +    <style>
> +      .cls-1 {
> +        fill: #646464;

This shouldn't have a color hardcoded, please use fill="context-fill" instead.

::: browser/themes/shared/canvas-popup.svg:6
(Diff revision 3)
> +<svg xmlns="http://www.w3.org/2000/svg" width="481.156" height="508.687" viewBox="0 0 481.156 508.687">
> +  <defs>
> +    <style>
> +      .cls-1 {
> +        fill: #646464;
> +        fill-rule: evenodd;

I wonder if this is required. In any case, can you inline it and remove the class attribute?

::: browser/themes/shared/canvas-popup.svg:11
(Diff revision 3)
> +        fill-rule: evenodd;
> +      }
> +    </style>
> +  </defs>
> +  <path class="cls-1" d="M583,384C546.867,608.615,362.841,606.188,300,602,127.564,590.507,36.759,348.931,167.325,222.409L273.181,328.282c-7.183,24.6,2.7,54.1,42.418,70.589,42.865,17.8,87.4,9.748,87.4,9.748s-14.081-22.03-15.565-68.266c-1.1-34.2-40.15-55.995-72.513-50.482L214.1,189.033c30.6-12.34,59.034-1.8,69.9,6.967,26.871,21.688,16.617,68.436,53,54,70.87-28.119-40.743-132.319,53-154C469.027,77.722,610.517,212.945,583,384ZM212,370a41,41,0,1,0,41,41A41,41,0,0,0,212,370Zm149.5,92A43.5,43.5,0,1,0,405,505.5,43.5,43.5,0,0,0,361.5,462ZM459,189a40,40,0,1,0,40,40A40,40,0,0,0,459,189Zm24.5,141A45.5,45.5,0,1,0,529,375.5,45.5,45.5,0,0,0,483.5,330Z" transform="translate(-105.344 -94.344)"/>
> +  <path class="cls-1" d="M319,391c-36.083-15.022-42.678-42.92-33.519-64.423L141.191,182.269a17.732,17.732,0,0,1,25.075-25.078L310.794,301.737C338.87,291.7,378,310.59,379,342c1.239,38.716,13,58,13,58S354.8,405.905,319,391Z" transform="translate(-105.344 -94.344)"/>

You can get rid of the transform by using SVGO (https://github.com/svg/svgo). Let me know if you need help with that. :)

::: browser/themes/shared/jar.inc.mn:238
(Diff revision 3)
>    skin/classic/browser/privatebrowsing/private-browsing.svg    (../shared/privatebrowsing/private-browsing.svg)
>    skin/classic/browser/privatebrowsing/tracking-protection-off.svg (../shared/privatebrowsing/tracking-protection-off.svg)
>    skin/classic/browser/privatebrowsing/tracking-protection.svg (../shared/privatebrowsing/tracking-protection.svg)
>    skin/classic/browser/urlbar-tab.svg                          (../shared/urlbar-tab.svg)
> +
> +  skin/classic/browser/canvas-popup.svg                        (../shared/canvas-popup.svg)

I think this file should move to shared/notification-icons/canvas.svg

::: browser/themes/shared/notification-icons.inc.css:123
(Diff revision 3)
>  }
>  
> +#canvas-notification-icon,
> +.popup-notification-icon[popupid="canvas-permissions-prompt"] {
> +  list-style-image: url(chrome://browser/skin/canvas-popup.svg);
> +  -moz-context-properties: fill;

These icons should already have context fill set (https://searchfox.org/mozilla-central/rev/00fa5dacedb925022f53d025121f1a919508e7ce/browser/themes/shared/notification-icons.inc.css#7), I think you can remove the two attributes here.

	Chung-Sheng Fu [:cfu] Assignee
	Comment 68 • 7 years ago

Sorry for late response.

(In reply to Johann Hofmann [:johannh] from comment #63)
> Comment on attachment 8901645 [details]
> Bug 967895 - Prompt (w/ Site Permission) before allowing content to extract
> canvas data (Tor 6253).
> 
> https://reviewboard.mozilla.org/r/173058/#review183210
> 
> The code looks mostly good, just a bit of cleanup required around various
> ends.
> 
> Can you add a test that asserts that the parts in patch (showing the
> permission prompt and confirming/denying) work as expected? You could put it
> in browser/base/content/test/permissions, for example.

I rewrote the test, please see the "Bug 967895 - Add tests for canvas fingerprinting resistance." patch.  Now it works by the following steps:
1. Compare the extracted data of 2 canvases.  One is the placeholder, filled with white color.
2. Set privacy.resistFingerprinting = true.
3. Extract canvas data again to make the notification prompt.
4. Wait for the "popupshown" event of the notification.
5. Simulate pressing the "Allow Data Access" button.
6. Wait for the "popuphidden" event of the notification.
7. Check if permission is correctly set.
8. Extract canvas data and compare them again.
9. Repeat 1., but press the "Don't Allow" button in 5..

> ::: browser/base/content/browser.js:1892
> (Diff revision 3)
> >        if (AppConstants.isPlatformAndVersionAtLeast("win", "10")) {
> >          MenuTouchModeObserver.uninit();
> >        }
> >        BrowserOffline.uninit();
> >        IndexedDBPromptHelper.uninit();
> > +      if (Services.prefs.getBoolPref("privacy.resistFingerprinting", false)) {
> 
> Aren't we leaking observers if the user flipped the pref to false during the
> browsing session? I would recommend putting this in a try-catch block
> instead.

When writing the test, I realize that we should always listen the canvas prompt event, not just when privacy.resistFingerprinting = true.  If the browser is launched with privacy.resistFingerprinting = false and then we set the pref to true, the notification will not prompt when content script attempts to extract canvas data.

> ::: browser/themes/shared/canvas-popup.svg:11
> (Diff revision 3)
> > +        fill-rule: evenodd;
> > +      }
> > +    </style>
> > +  </defs>
> > +  <path class="cls-1" d="M583,384C546.867,608.615,362.841,606.188,300,602,127.564,590.507,36.759,348.931,167.325,222.409L273.181,328.282c-7.183,24.6,2.7,54.1,42.418,70.589,42.865,17.8,87.4,9.748,87.4,9.748s-14.081-22.03-15.565-68.266c-1.1-34.2-40.15-55.995-72.513-50.482L214.1,189.033c30.6-12.34,59.034-1.8,69.9,6.967,26.871,21.688,16.617,68.436,53,54,70.87-28.119-40.743-132.319,53-154C469.027,77.722,610.517,212.945,583,384ZM212,370a41,41,0,1,0,41,41A41,41,0,0,0,212,370Zm149.5,92A43.5,43.5,0,1,0,405,505.5,43.5,43.5,0,0,0,361.5,462ZM459,189a40,40,0,1,0,40,40A40,40,0,0,0,459,189Zm24.5,141A45.5,45.5,0,1,0,529,375.5,45.5,45.5,0,0,0,483.5,330Z" transform="translate(-105.344 -94.344)"/>
> > +  <path class="cls-1" d="M319,391c-36.083-15.022-42.678-42.92-33.519-64.423L141.191,182.269a17.732,17.732,0,0,1,25.075-25.078L310.794,301.737C338.87,291.7,378,310.59,379,342c1.239,38.716,13,58,13,58S354.8,405.905,319,391Z" transform="translate(-105.344 -94.344)"/>
> 
> You can get rid of the transform by using SVGO
> (https://github.com/svg/svgo). Let me know if you need help with that. :)

This tool is pretty cool, thank!

	Georg Koppen
	Comment 69 • 7 years ago

Do you have a try build with your current patch somewhere I could test? Or a 64-bit Linux build I could use?

	Chung-Sheng Fu [:cfu] Assignee
	Comment 70 • 7 years ago

(In reply to Georg Koppen from comment #69)
> Do you have a try build with your current patch somewhere I could test? Or a
> 64-bit Linux build I could use?

I just triggered one, please check the link.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f8be14ca5ea89c64ea9f909422bf78bbd80c3b9
(Maybe it still needs a while to build)

	Georg Koppen
	Comment 71 • 7 years ago

(In reply to Chung-Sheng Fu [:cfu] from comment #70)
> (In reply to Georg Koppen from comment #69)
> > Do you have a try build with your current patch somewhere I could test? Or a
> > 64-bit Linux build I could use?
> 
> I just triggered one, please check the link.
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=4f8be14ca5ea89c64ea9f909422bf78bbd80c3b9
> (Maybe it still needs a while to build)

Thanks. So I ended up using https://queue.taskcluster.net/v1/task/NxBuafZXR96vpdCAXnlSRg/runs/0/artifacts/public/build/target.tar.bz2 but it seems I am unable to get the canvas prompt/notification shown. I did

1) use a new profile for the try build
2) set `privacy.resistFingerprinting` to `true`
3) restarted
4) visited github.com

but while Tor Browser shows a doorhanger in this case your build does not. Am I missing some steps to trigger that?

	Chung-Sheng Fu [:cfu] Assignee
	Comment 72 • 7 years ago

(In reply to Georg Koppen from comment #71)
> Thanks. So I ended up using
> https://queue.taskcluster.net/v1/task/NxBuafZXR96vpdCAXnlSRg/runs/0/
> artifacts/public/build/target.tar.bz2 but it seems I am unable to get the
> canvas prompt/notification shown. I did
> 
> 1) use a new profile for the try build
> 2) set `privacy.resistFingerprinting` to `true`
> 3) restarted
> 4) visited github.com
> 
> but while Tor Browser shows a doorhanger in this case your build does not.
> Am I missing some steps to trigger that?

Sorry I would like to ask where you got the link?

I downloaded from "Linux x64 opt" -> "tc" -> "B"
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f8be14ca5ea89c64ea9f909422bf78bbd80c3b9&selectedJob=131240651
whose direct link is
https://queue.taskcluster.net/v1/task/Q2NqzyjESneQapuJj2_OKg/runs/0/artifacts/public/build/target.tar.bz2
and ran on my Ubuntu 16.04 64-bit VM.  After repeating your steps, the canvas notification showed properly.

Could you please try again with my link?  Sorry for inconvenience.

	Georg Koppen
	Comment 73 • 7 years ago

(In reply to Chung-Sheng Fu [:cfu] from comment #72)
> (In reply to Georg Koppen from comment #71)
> > Thanks. So I ended up using
> > https://queue.taskcluster.net/v1/task/NxBuafZXR96vpdCAXnlSRg/runs/0/
> > artifacts/public/build/target.tar.bz2 but it seems I am unable to get the
> > canvas prompt/notification shown. I did
> > 
> > 1) use a new profile for the try build
> > 2) set `privacy.resistFingerprinting` to `true`
> > 3) restarted
> > 4) visited github.com
> > 
> > but while Tor Browser shows a doorhanger in this case your build does not.
> > Am I missing some steps to trigger that?
> 
> Sorry I would like to ask where you got the link?

I downloaded the binary from "Linux x64 opt" -> "AB". It seems I was not aware of the fact that not all builds based on the same changeset should behave the same, sorry.
 
> I downloaded from "Linux x64 opt" -> "tc" -> "B"
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=4f8be14ca5ea89c64ea9f909422bf78bbd80c3b9&selectedJob=1
> 31240651
> whose direct link is
> https://queue.taskcluster.net/v1/task/Q2NqzyjESneQapuJj2_OKg/runs/0/
> artifacts/public/build/target.tar.bz2
> and ran on my Ubuntu 16.04 64-bit VM.  After repeating your steps, the
> canvas notification showed properly.
> 
> Could you please try again with my link?  Sorry for inconvenience.

No worries and thanks for your patience with me. :) So, I tried your link and it worked for me, thanks. The reason why I was asking for such a build was that I found a way to reproduce a crash bug related to our patch and wanted to check whether your patch is affected as well. And it seems it is. See: https://trac.torproject.org/projects/tor/ticket/23393 for the issue and a possible patch attached. I am actually not 100% sure whether a process in e10multi-mode crashes with the scenario mentioned in our bug but I verified that it does when setting `dom.ipc.processCount` to `1` in your try build.

So, could you amend your patches in this bug accordingly to take that finding into account?

	Chung-Sheng Fu [:cfu] Assignee
	Comment 74 • 7 years ago

(In reply to Georg Koppen from comment #73)
> No worries and thanks for your patience with me. :) So, I tried your link
> and it worked for me, thanks. The reason why I was asking for such a build
> was that I found a way to reproduce a crash bug related to our patch and
> wanted to check whether your patch is affected as well. And it seems it is.
> See: https://trac.torproject.org/projects/tor/ticket/23393 for the issue and
> a possible patch attached. I am actually not 100% sure whether a process in
> e10multi-mode crashes with the scenario mentioned in our bug but I verified
> that it does when setting `dom.ipc.processCount` to `1` in your try build.
> 
> So, could you amend your patches in this bug accordingly to take that
> finding into account?

Thank you so much for the very important information!  I will investigate it.

	Chung-Sheng Fu [:cfu] Assignee
	Comment 75 • 7 years ago

The crash is caused by repeatedly extracting canvas data, so there are many unhandled IPC requests.

Maybe we can set the permission denied before the notification prompts.  That is, if user doesn't click allow or don't allow, it will be regarded as denied.  Because we have this check in CanvasUtils::IsImageExtractionAllowed

    if (permission == nsIPermissionManager::ALLOW_ACTION) {
        return true;
    } else if (permission == nsIPermissionManager::DENY_ACTION) {
        return false;
    }

the notification will not prompt again.

If the solution looks good to you, I will merge it into the "Bug 967895 - Prompt (w/ Site Permission) before allowing content to extract canvas data (Tor 6253)." patch.

	Mark Smith [:mcs]
	Comment 76 • 7 years ago

(In reply to Chung-Sheng Fu [:cfu] from comment #75)
> Created attachment 8909184 [details] [diff] [review]
> 0001-Avoid-repeated-prompt.patch
> 
> The crash is caused by repeatedly extracting canvas data, so there are many
> unhandled IPC requests.
> 
> Maybe we can set the permission denied before the notification prompts. 
> That is, if user doesn't click allow or don't allow, it will be regarded as
> denied....

Maybe I am misunderstanding something, but won't this have the effect of denying future access without showing a prompt in the case where the user ignores the prompt? Do we want that behavior?

Even if we do, I think the patch referenced at the following should be includes as well for safety's sake:
https://trac.torproject.org/projects/tor/ticket/23393#comment:14
Based on the surrounding code and my understanding of what is happening, it just seems more correct to me to ignore the QI failure inside RecvShowCanvasPermissionPrompt().

	Chung-Sheng Fu [:cfu] Assignee
	Comment 78 • 7 years ago

(In reply to Mark Smith (:mcs) from comment #76)
> (In reply to Chung-Sheng Fu [:cfu] from comment #75)
> > Created attachment 8909184 [details] [diff] [review]
> > 0001-Avoid-repeated-prompt.patch
> > 
> > The crash is caused by repeatedly extracting canvas data, so there are many
> > unhandled IPC requests.
> > 
> > Maybe we can set the permission denied before the notification prompts. 
> > That is, if user doesn't click allow or don't allow, it will be regarded as
> > denied....
> 
> Maybe I am misunderstanding something, but won't this have the effect of
> denying future access without showing a prompt in the case where the user
> ignores the prompt? Do we want that behavior?

Thank you for pointing this out.

I added a "dismissed" event listener and set the permission back to PROMPT_ACTION when the user ignores the prompt.  Is the behavior more reasonable in this way?

> Even if we do, I think the patch referenced at the following should be
> includes as well for safety's sake:
> https://trac.torproject.org/projects/tor/ticket/23393#comment:14
> Based on the surrounding code and my understanding of what is happening, it
> just seems more correct to me to ignore the QI failure inside
> RecvShowCanvasPermissionPrompt().

Sorry I didn't notice this patch.  I will merge it to the "Bug 967895 - Ask for placeholder data when image extraction is not allowed (Tor 6253)." patch.

	Georg Koppen
	Comment 79 • 7 years ago

Comment on attachment 8909184 [details] [diff] [review]
0001-Avoid-repeated-prompt.patch

I guess there is no need for reviewing an old patch? FWIW I agree with the things mcs said.

	Chung-Sheng Fu [:cfu] Assignee
	Comment 80 • 7 years ago

(In reply to Georg Koppen from comment #79)
> Comment on attachment 8909184 [details] [diff] [review]
> 0001-Avoid-repeated-prompt.patch
> 
> I guess there is no need for reviewing an old patch? FWIW I agree with the
> things mcs said.

I'm a little worried about the unnecessary IPC overhead, but it seems that other popup notifications don't do such prevention either.  I will remove the 2 patches and apply the Tor patch.  Thank you very much :)

	Johann Hofmann [:johannh]
	Comment 85 • 7 years ago
mozreview-review

Comment on attachment 8901645 [details]
Bug 967895 - Prompt (w/ Site Permission) before allowing content to extract canvas data (Tor 6253).

https://reviewboard.mozilla.org/r/173058/#review188956

Sorry for the delay, r=me with the nits addressed.

::: browser/base/content/browser.js:6727
(Diff revision 5)
> +      checkbox.label = gBrowserBundle.GetStringFromName("canvas.remember");
> +    }
> +
> +    let options = {
> +      checkbox
> +      // TODO learnMoreURL

Nit: I'd prefer landing without this comment, we already have bug 1397757 to remind us that we need a learnMoreURL.

::: browser/themes/shared/notification-icons/canvas.svg:4
(Diff revision 5)
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" width="481.156" height="508.687" viewBox="0 0 481.156 508.687">

These icons are normally 16x16, can you try setting the width/height/viewBox attributes to 16 and see if it distorts anything? Also, are you able to remove the viewBox?

::: browser/themes/shared/notification-icons/canvas.svg:5
(Diff revision 5)
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" width="481.156" height="508.687" viewBox="0 0 481.156 508.687">
> +  <path fill="context-fill" d="M477.656 289.656c-36.133 224.615-220.16 222.188-283 218C22.22 496.163-68.584 254.586 61.98 128.066l105.857 105.872c-7.183 24.6 2.7 54.1 42.418 70.59 42.865 17.8 87.4 9.747 87.4 9.747s-14.08-22.03-15.565-68.266c-1.1-34.2-40.15-55.996-72.513-50.483L108.757 94.69c30.6-12.34 59.033-1.8 69.9 6.966 26.87 21.688 16.616 68.436 53 54 70.87-28.12-40.744-132.32 53-154 79.026-18.278 220.516 116.945 193 288zm-371-14a41 41 0 1 0 41 41 41 41 0 0 0-41-41zm149.5 92a43.5 43.5 0 1 0 43.5 43.5 43.5 43.5 0 0 0-43.5-43.5zm97.5-273a40 40 0 1 0 40 40 40 40 0 0 0-40-40zm24.5 141a45.5 45.5 0 1 0 45.5 45.5 45.5 45.5 0 0 0-45.5-45.5z"/>

This also needs fill-opacity="context-fill-opacity" to be able to set the opacity through CSS.

Since both paths have this attribute, I'd recommend just setting it on the <svg> element, like in https://searchfox.org/mozilla-central/source/browser/themes/shared/notification-icons/camera.svg#4

	Johann Hofmann [:johannh]
	Comment 86 • 7 years ago

(Also, I'm assuming that the previously discussed workaround wasn't necessary?) Feel free to put up another patch or re-request review otherwise... :)

	Chung-Sheng Fu [:cfu] Assignee
	Comment 87 • 7 years ago

(In reply to Johann Hofmann [:johannh] from comment #85)
> ::: browser/themes/shared/notification-icons/canvas.svg:4
> (Diff revision 5)
> > +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> > +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> > +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> > +<svg xmlns="http://www.w3.org/2000/svg" width="481.156" height="508.687" viewBox="0 0 481.156 508.687">
> 
> These icons are normally 16x16, can you try setting the width/height/viewBox
> attributes to 16 and see if it distorts anything? Also, are you able to
> remove the viewBox?

First I tried to set the dimensions to 16x16 with viewBox kept

    <svg xmlns="..." width="16" height="16" viewBox="0 0 481.156 508.687" fill="..." fill-opacity="...">
      <path d="..."/>
      <path d="..."/>
    </svg>

and it looks fine.  However, I have no idea how to modify the paths when trying to remove viewBox, and the removeViewBox plugin of SVGO doesn't work for this case, so I tried grouping the paths and use a transform

    <svg xmlns="..." width="16" height="16" fill="..." fill-opacity="...">
      <!-- viewBox="0 0 481.156 508.687", 16 / 481.156 = 0.033, 16 / 508.687 = 0.031 -->
      <g transform="scale(0.033,0.031)">
        <path d="..."/>
        <path d="..."/>
      </g>
    </svg>

and it looks the same.

Which one looks better to you?

	Chung-Sheng Fu [:cfu] Assignee
	Comment 88 • 7 years ago

(In reply to Johann Hofmann [:johannh] from comment #86)
> (Also, I'm assuming that the previously discussed workaround wasn't
> necessary?) Feel free to put up another patch or re-request review
> otherwise... :)

Yes we decided not to apply my workaround, just ignoring IPC requests for canvas prompt when tab is being closed.

Besides, could you also please help with the test patch (attachment 8901648 [details])?  If it will take too much of your time, could you suggest someone who can do this?

Thank you very much!

	Johann Hofmann [:johannh]
	Comment 89 • 7 years ago

(In reply to Chung-Sheng Fu [:cfu] from comment #87)
> (In reply to Johann Hofmann [:johannh] from comment #85)
> > ::: browser/themes/shared/notification-icons/canvas.svg:4
> > (Diff revision 5)
> > > +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> > > +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> > > +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> > > +<svg xmlns="http://www.w3.org/2000/svg" width="481.156" height="508.687" viewBox="0 0 481.156 508.687">
> > 
> > These icons are normally 16x16, can you try setting the width/height/viewBox
> > attributes to 16 and see if it distorts anything? Also, are you able to
> > remove the viewBox?
> 
> First I tried to set the dimensions to 16x16 with viewBox kept
> 
>     <svg xmlns="..." width="16" height="16" viewBox="0 0 481.156 508.687"
> fill="..." fill-opacity="...">
>       <path d="..."/>
>       <path d="..."/>
>     </svg>
> 
> and it looks fine.  However, I have no idea how to modify the paths when
> trying to remove viewBox, and the removeViewBox plugin of SVGO doesn't work
> for this case, so I tried grouping the paths and use a transform
> 
>     <svg xmlns="..." width="16" height="16" fill="..." fill-opacity="...">
>       <!-- viewBox="0 0 481.156 508.687", 16 / 481.156 = 0.033, 16 / 508.687
> = 0.031 -->
>       <g transform="scale(0.033,0.031)">
>         <path d="..."/>
>         <path d="..."/>
>       </g>
>     </svg>
> 
> and it looks the same.
> 
> Which one looks better to you?

Let's not do the scaling transform. I think we can leave the viewBox for now and fix it up in a follow-up bug if any problems should surface.

I'll take a look at the test as soon as possible (my time is a little strained from Photon right now, sorry).

	Johann Hofmann [:johannh]
	Comment 90 • 7 years ago
mozreview-review

Comment on attachment 8901648 [details]
Bug 967895 - Add tests for canvas fingerprinting resistance.

https://reviewboard.mozilla.org/r/173064/#review189708

Looks fine to me, but please resolve the issues below.

I'd say this is the bare minimum amount of testing we should do, especially considering that this is security/privacy sensitive code. On front-end side, it would be useful to have a test that asserts the checkbox functionality and I can imagine there's more edge cases to test on platform side as well.

::: browser/base/content/test/permissions/browser.ini:14
(Diff revision 5)
>    temporary_permissions_subframe.html
>    ../webrtc/get_user_media.html
>  [browser_temporary_permissions_expiry.js]
>  [browser_temporary_permissions_navigation.js]
>  [browser_temporary_permissions_tabs.js]
> +[browser_canvas_fingerprinting_resistance.js]

Nit: I usually prefer to sort test entries in .ini files alphabetically. I know it's not done everywhere but we can at least keep it consistent in the files where it is :)

::: browser/base/content/test/permissions/browser_canvas_fingerprinting_resistance.js:2
(Diff revision 5)
> +"use strict";
> +

Please add a top-level comment describing what this test is testing.

::: browser/base/content/test/permissions/browser_canvas_fingerprinting_resistance.js:7
(Diff revision 5)
> +
> +const kUrl = "https://example.com/";
> +const kPrincipal = Components.classes["@mozilla.org/scriptsecuritymanager;1"]
> +    .getService(Ci.nsIScriptSecurityManager)
> +    .createCodebasePrincipal(Services.io.newURI(kUrl), {});
> +const kPermission = "canvas/extractData"

Nit: missing semi-colon. Please run

./mach eslint browser/base/content/test/permissions

(it will also fail on try)

::: browser/base/content/test/permissions/browser_canvas_fingerprinting_resistance.js:10
(Diff revision 5)
> +    .getService(Ci.nsIScriptSecurityManager)
> +    .createCodebasePrincipal(Services.io.newURI(kUrl), {});
> +const kPermission = "canvas/extractData"
> +
> +function initTab() {
> +  const contentWindow = content.wrappedJSObject;

Nit: please use let instead of const if you're not declaring top-level constants.

::: browser/base/content/test/permissions/browser_canvas_fingerprinting_resistance.js:13
(Diff revision 5)
> +
> +function initTab() {
> +  const contentWindow = content.wrappedJSObject;
> +
> +  let drawCanvas = (fillStyle, id) => {
> +    const contentDocument = contentWindow.document;

Nit: let

::: browser/base/content/test/permissions/browser_canvas_fingerprinting_resistance.js:14
(Diff revision 5)
> +function initTab() {
> +  const contentWindow = content.wrappedJSObject;
> +
> +  let drawCanvas = (fillStyle, id) => {
> +    const contentDocument = contentWindow.document;
> +    const kWidth = 64, kHeight = 64;

Nit: either let and change the variable names or put this at the top

::: browser/base/content/test/permissions/browser_canvas_fingerprinting_resistance.js:31
(Diff revision 5)
> +    context.fillRect(0, 0, kWidth, kHeight);
> +
> +    return canvas;
> +  };
> +
> +  contentWindow.kCanvasId = "canvas-id-canvas";

Nit: Why can't you just hard-code that string into your test?

::: browser/base/content/test/permissions/browser_canvas_fingerprinting_resistance.js:48
(Diff revision 5)
> +      ["privacy.resistFingerprinting", true]
> +    ]
> +  });
> +}
> +
> +function promisePopupEvent(eventName) {

You can replace this function with BrowserTestUtils.waitForEvent(PopupNotifications.panel, eventName);

::: browser/base/content/test/permissions/browser_canvas_fingerprinting_resistance.js:81
(Diff revision 5)
> +  }
> +}
> +
> +function triggerCommand(button) {
> +  let notifications = PopupNotifications.panel.childNodes;
> +  // ok(notifications.length > 0, "At least 1 notification displayed");

Why is this commented out? I think you can remove it.

::: browser/base/content/test/permissions/browser_canvas_fingerprinting_resistance.js:83
(Diff revision 5)
> +
> +function triggerCommand(button) {
> +  let notifications = PopupNotifications.panel.childNodes;
> +  // ok(notifications.length > 0, "At least 1 notification displayed");
> +  let notification = notifications[0];
> +  // info(notification.id);

Remove this. ;)

::: browser/base/content/test/permissions/browser_canvas_fingerprinting_resistance.js:124
(Diff revision 5)
> +async function doTest(grantPermission) {
> +  Services.perms.removeFromPrincipal(kPrincipal, kPermission);
> +  await BrowserTestUtils.withNewTab(kUrl, withNewTab.bind(null, grantPermission));
> +}
> +
> +add_task(doTest.bind(null, false));

Can you add a short comment above each test as to what they're testing?

	Ethan Tseng [:ethan]
	Comment 91 • 7 years ago

Hi Jeff,

We are waiting for your review for several patches.
Do you have to review them?  Or could you recommend another reviewer for us?

	Jeff Muizelaar [:jrmuizel]
	Comment 92 • 7 years ago
mozreview-review

Comment on attachment 8901646 [details]
Bug 967895 - Add an option to return a placeholder when extracting image data (Tor 6253).

https://reviewboard.mozilla.org/r/173060/#review192468

	Jeff Muizelaar [:jrmuizel]
	Comment 93 • 7 years ago
mozreview-review

Comment on attachment 8901647 [details]
Bug 967895 - Ask for placeholder data when image extraction is not allowed (Tor 6253).

https://reviewboard.mozilla.org/r/173062/#review192470

	Code Review Bot [:reviewbot]
	Comment 98 • 7 years ago
mozreview-review

Comment on attachment 8901647 [details]
Bug 967895 - Ask for placeholder data when image extraction is not allowed (Tor 6253).

https://reviewboard.mozilla.org/r/173062/#review192602


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/canvas/CanvasUtils.cpp:137
(Diff revision 6)
> +                                           PERMISSION_CANVAS_EXTRACT_DATA,
> +                                           &permission);
> +    NS_ENSURE_SUCCESS(rv, false);
> +    if (permission == nsIPermissionManager::ALLOW_ACTION) {
> +        return true;
> +    } else if (permission == nsIPermissionManager::DENY_ACTION) {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]

	Chung-Sheng Fu [:cfu] Assignee
	Comment 101 • 7 years ago

Thank you very much for your help :)

	Pulsebot
	Comment 102 • 7 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0a482229bed6
Prompt (w/ Site Permission) before allowing content to extract canvas data (Tor 6253). r=johannh
https://hg.mozilla.org/integration/autoland/rev/d6ab8156f858
Add an option to return a placeholder when extracting image data (Tor 6253). r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/71790f0ea832
Ask for placeholder data when image extraction is not allowed (Tor 6253). r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/bf81d961c576
Add tests for canvas fingerprinting resistance. r=johannh

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 103 • 7 years ago

Backed out for rooting hazard:

https://hg.mozilla.org/integration/autoland/rev/9535f2ddf045626371e8ad02f355bf30c8e08ef1
https://hg.mozilla.org/integration/autoland/rev/d4307f283714cd6e69d2719151f1478545388e77
https://hg.mozilla.org/integration/autoland/rev/6fd0004b779171a3aa980c039e045fff8a9e6f8c
https://hg.mozilla.org/integration/autoland/rev/ffcb85a1cbe9e614fa4bc24b89183b22bbc735ad

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=bf81d961c576d63fbc95413b7437d7ed81a365cb&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
From the analysis file:

Time: Mon Oct 09 2017 16:40:36 GMT+0000 (UTC)

Function '_ZN7mozilla3dom24CanvasRenderingContext2D17GetImageDataArrayEP9JSContextiijjPP8JSObject$uint32 mozilla::dom::CanvasRenderingContext2D::GetImageDataArray(JSContext*, int32, int32, uint32, uint32, JSObject**)' has unrooted 'nogc' of type 'JS::AutoCheckCannotGC' live across GC call mozilla::gfx::DataSourceSurface.Unmap at dom/canvas/CanvasRenderingContext2D.cpp:5848
    CanvasRenderingContext2D.cpp:5828: Call(115,116, nogc.AutoCheckCannotGC(0))
    CanvasRenderingContext2D.cpp:5830: Call(116,117, __temp_49 := darray.operator 117())
    CanvasRenderingContext2D.cpp:5830: Call(117,118, data := JS_GetUint8ClampedArrayData(__temp_49**,isShared,nogc))
    CanvasRenderingContext2D.cpp:5831: Call(118,119, __temp_50 := __builtin_expect(isShared*,0))
    CanvasRenderingContext2D.cpp:5831: Assume(119,120, (__temp_50* != 0), true)
    CanvasRenderingContext2D.cpp:5831: Call(120,121, MOZ_ReportAssertionFailure("!isShared","/builds/worker/checkouts/gecko/dom/canvas/CanvasRenderingContext2D.cpp",5831))
    CanvasRenderingContext2D.cpp:5831: Assign(121,122, 0 := 5831)
    CanvasRenderingContext2D.cpp:5831: Call(122,123, abort())
    CanvasRenderingContext2D.cpp:5833: Assign(123,124, srcStride := rawData.mStride*)
    CanvasRenderingContext2D.cpp:5834: Assign(124,125, src := rawData.mData*[((srcReadRect.field:0.y* * srcStride*) + (srcReadRect.field:0.x* * 4))]{int8})
    CanvasRenderingContext2D.cpp:5839: Assign(125,126, usePlaceholder := 0)
    CanvasRenderingContext2D.cpp:5840: Call(126,127, __temp_51 := this*.field:0.mCanvasElement.operator 1())
    CanvasRenderingContext2D.cpp:5840: Assume(127,128, __temp_51*, true)
    CanvasRenderingContext2D.cpp:5841: Call(128,129, __temp_54 := this*.field:0.mCanvasElement.operator->())
    CanvasRenderingContext2D.cpp:5841: Call(129,130, __temp_53 := __temp_54*.field:0.field:0.field:0.field:0.field:0.field:0.field:0.OwnerDoc())
    CanvasRenderingContext2D.cpp:5841: Call(130,131, __temp_52*.nsCOMPtr(0,__temp_53*))
    CanvasRenderingContext2D.cpp:5841: Assign(131,132, ownerDoc := __temp_52*)
    CanvasRenderingContext2D.cpp:5841: Call(132,133, __temp_52.~nsCOMPtr())
    CanvasRenderingContext2D.cpp:5842: Call(133,134, __temp_55 := ownerDoc.operator 1())
    CanvasRenderingContext2D.cpp:5842: Assume(134,135, !__temp_55*, false)
    CanvasRenderingContext2D.cpp:5843: Call(135,136, __temp_57 := ownerDoc.operator 253())
    CanvasRenderingContext2D.cpp:5843: Call(136,137, __temp_56 := IsImageExtractionAllowed(__temp_57*,aCx*))
    CanvasRenderingContext2D.cpp:5843: Assume(137,139, !__temp_56*, false)
    CanvasRenderingContext2D.cpp:5842: Assign(139,140, usePlaceholder := 0)
    CanvasRenderingContext2D.cpp:5842: Call(140,141, ownerDoc.~nsCOMPtr())
    CanvasRenderingContext2D.cpp:5846: Assume(141,142, usePlaceholder*, true)
    CanvasRenderingContext2D.cpp:5847: Call(142,143, __temp_58 := readback.operator 1())
    CanvasRenderingContext2D.cpp:5847: Assume(143,144, __temp_58*, true)
    CanvasRenderingContext2D.cpp:5848: Call(144,145, __temp_59 := readback.operator->())
    CanvasRenderingContext2D.cpp:5848: Call(145,146, __temp_59*.Unmap*()) [[GC call]]
    CanvasRenderingContext2D.cpp:5851: Call(146,147, __temp_60 := len.value())
    CanvasRenderingContext2D.cpp:5851: Call(147,148, memset(data*,255,__temp_60*))
    CanvasRenderingContext2D.cpp:5852: Call(148,149, __temp_61 := darray.operator 117())
    CanvasRenderingContext2D.cpp:5852: Assign(149,150, aRetval* := __temp_61**)
    CanvasRenderingContext2D.cpp:5853: Assign(150,151, return := 0)
    CanvasRenderingContext2D.cpp:5853: Call(151,152, nogc.~AutoCheckCannotGC())

	Francesco Lodolo [:flod]
	Comment 104 • 7 years ago

:jsavory

Sorry if I missed something, but looking at bug 1382111 I can see a mock-up of the permission dialog, but not the tooltip.

https://hg.mozilla.org/mozilla-central/rev/0a482229bed6#l3.12

The tooltip is completely different from the existing ones: they all describe an action, e.g. ("permission to") "Open message panel". From the sound of it, this should be something like "Access image canvas data". 

Also, not sure if it's just me, but "canvas" is really cryptic for a user facing message.

Can we get confirmation on the current text before trying to reland this?

	Chung-Sheng Fu [:cfu] Assignee
	Comment 105 • 7 years ago

(In reply to Arthur Edelstein (Tor Browser dev) [:arthuredelstein] from comment #1)
> For reference, here's a link that tracks the latest version of the Tor
> Browser patch:
> https://torpat.ch/6253

Hi Arthur,

I'm fixing the rooting hazard but got a question about the patch.  In CanvasRenderingContext2D::GetImageDataArray, the code segment

+  // Check for site-specific permission and return all-white, opaque pixel
+  // data if no permission.  This check is not needed if the canvas was
+  // created with a docshell (that is only done for special internal uses).
+  bool usePlaceholder = false;
+  if (mCanvasElement) {
+    nsCOMPtr<nsIDocument> ownerDoc = mCanvasElement->OwnerDoc();
+    usePlaceholder = !ownerDoc ||
+      !CanvasUtils::IsImageExtractionAllowed(ownerDoc, aCx);
+  }
+
+  if (usePlaceholder) {
+    if (readback) {
+      readback->Unmap();
+    }
+
+    memset(data, 0xFF, len.value());
+    *aRetval = darray;
+    return NS_OK;
+  }

Can I do memset before readback->Unmap?

Could you please help me contact the patch author?

	Mark Smith [:mcs]
	Comment 106 • 7 years ago

(In reply to Chung-Sheng Fu [:cfu] from comment #105)
> Can I do memset before readback->Unmap?

I am one of the original authors of the patch.
I do not know of any reason that you cannot do the memset() before the Unmap() call (although I also do not know why changing the order is necessary).

	Chung-Sheng Fu [:cfu] Assignee
	Comment 107 • 7 years ago

(In reply to Mark Smith (:mcs) from comment #106)
> (In reply to Chung-Sheng Fu [:cfu] from comment #105)
> > Can I do memset before readback->Unmap?
> 
> I am one of the original authors of the patch.
> I do not know of any reason that you cannot do the memset() before the
> Unmap() call (although I also do not know why changing the order is
> necessary).

Thanks, Mark!

Because this code block looks like

    {
      JS::AutoCheckCannotGC nogc;
      ...
      if (usePlaceholder) {
        if (readback) {
          readback->Unmap();
        }
        memset(data, 0xFF, len.value());
        *aRetval = darray;
        return NS_OK;
      }
      ...
    }
    readback->Unmap();
    *aRetval = darray;
    return NS_OK;

The static analysis says that JS::AutoCheckCannotGC object should not be destructed after readback->Unmap().  So I would like to change this to

    do {
      JS::AutoCheckCannotGC nogc;
      ...
      if (usePlaceholder) {
        memset(data, 0xFF, len.value());
        break;
      }
      ...
    } while (false);
    readback->Unmap();
    *aRetval = darray;
    return NS_OK;

But in this way, memset() comes before readback->Unmap().  If this is not practicable, the code will be uglier :p

	Pulsebot
	Comment 114 • 7 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0c8403512a19
Prompt (w/ Site Permission) before allowing content to extract canvas data (Tor 6253). r=johannh
https://hg.mozilla.org/integration/autoland/rev/96d4c5d83b4c
Add an option to return a placeholder when extracting image data (Tor 6253). r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/1e1c64f987b7
Ask for placeholder data when image extraction is not allowed (Tor 6253). r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/11d5208791fe
Add tests for canvas fingerprinting resistance. r=johannh

	Francesco Lodolo [:flod]
	Comment 115 • 7 years ago

For the record, nobody addressed comment 104 yet.

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 116 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/0c8403512a19
https://hg.mozilla.org/mozilla-central/rev/96d4c5d83b4c
https://hg.mozilla.org/mozilla-central/rev/1e1c64f987b7
https://hg.mozilla.org/mozilla-central/rev/11d5208791fe

	Johann Hofmann [:johannh]
	Comment 117 • 7 years ago

(In reply to Francesco Lodolo [:flod] from comment #115)
> For the record, nobody addressed comment 104 yet.

At this point it's probably best to make a follow-up bug. Sorry for this, I didn't expect your comment to be ignored.

FWIW, I'm OK with this copy given that these tooltips are rarely viewed in combination with each other and that this protection is unlikely to be turned on for the general population of users (we'd have bigger problems explaining the various aspects of fingerprinting protection as a whole, making bikeshedding over a tooltip a little unnecessary in my view).

	Francesco Lodolo [:flod]
	Comment 118 • 7 years ago

(In reply to Johann Hofmann [:johannh] from comment #117)
> (In reply to Francesco Lodolo [:flod] from comment #115)
> > For the record, nobody addressed comment 104 yet.
> 
> At this point it's probably best to make a follow-up bug. Sorry for this, I
> didn't expect your comment to be ignored.

Not really your fault. I could have also asked for a back-out, but I don't feel like it was constructive in this case. There's an open NI for the original author of the copy, I'd prefer to get an answer before filing a follow up.

I agree with you that explaining fingerprinting would be extremely hard. But that doesn't mean we should ship less than optimal messages in our products. This is at least the second TOR-related bug I've seen involving strings and some UI, so I'd like to get a clear picture of the general approach we should have to these bugs.

	Chung-Sheng Fu [:cfu] Assignee
	Comment 119 • 7 years ago

(In reply to Francesco Lodolo [:flod] from comment #118)
> (In reply to Johann Hofmann [:johannh] from comment #117)
> > (In reply to Francesco Lodolo [:flod] from comment #115)
> > > For the record, nobody addressed comment 104 yet.
> > 
> > At this point it's probably best to make a follow-up bug. Sorry for this, I
> > didn't expect your comment to be ignored.
> 
> Not really your fault. I could have also asked for a back-out, but I don't
> feel like it was constructive in this case. There's an open NI for the
> original author of the copy, I'd prefer to get an answer before filing a
> follow up.
> 
> I agree with you that explaining fingerprinting would be extremely hard. But
> that doesn't mean we should ship less than optimal messages in our products.
> This is at least the second TOR-related bug I've seen involving strings and
> some UI, so I'd like to get a clear picture of the general approach we
> should have to these bugs.

I'm very sorry that I didn't notice your question.  Please have my apology.

I'm not sure if there is a general guideline for strings and UI, so I really need jsavory's help to answer it.

Apologize again for my miss.

	Sylvestre Ledru [:Sylvestre]
	Comment 120 • 7 years ago

Release Note Request (optional, but appreciated)
[Why is this notable]: Seems like a nice cool feature
[Affects Firefox for Android]: maybe?
[Suggested wording]: Improved privacy by notify the user in case of advanced tracking
(please provide a better wording)
[Links (documentation, blog post, etc)]:

	Jacqueline Savory [:jsavory] UX
	Comment 121 • 7 years ago

Very sorry for the delay here and thank you for pointing out the tooltip, I definitely missed this.

As Johann mentioned this item is hidden behind a pref, so my assumption is the user has at least some basic understanding behind fingerprinting. In terms of the word "canvas" I used what was found in the Tor browser as it seems to be the most accurate. However, I am open to suggestions if there is other terminology that we can use, or if removing the word "canvas" still has the same meaning. Johann, would you have an idea if this works? Could we reduce it to "HTML image data"?

Here are some possible tooltip messages. They seem to be a bit inconsistent when it comes to permissions in general, but I tried to use a string that was similar to a few of them. 
(Just to confirm, this is the message found over the hover state of the permission icon in the URL bar?)

Asking state:
"Manage sharing HTML canvas image data with this site"
Blocked state:
"You have blocked HTML canvas image data for this website"

Sorry again and let me know if there is anything else I can clarify.

	Johann Hofmann [:johannh]
	Comment 122 • 7 years ago

(In reply to Jacqueline Savory [:jsavory] UX from comment #121)
> Very sorry for the delay here and thank you for pointing out the tooltip, I
> definitely missed this.
> 
> As Johann mentioned this item is hidden behind a pref, so my assumption is
> the user has at least some basic understanding behind fingerprinting. In
> terms of the word "canvas" I used what was found in the Tor browser as it
> seems to be the most accurate. However, I am open to suggestions if there is
> other terminology that we can use, or if removing the word "canvas" still
> has the same meaning. Johann, would you have an idea if this works? Could we
> reduce it to "HTML image data"?

As mentioned in comment 117 I'm good with the existing text because this is a drop in the ocean of things that are not easy to understand in resist fingerprinting mode. I would absolutely accept a patch that improves things but I'm going to go great lengths to make it happen.

I don't really see why "HTML image data" would be easier to understand. This is a really hard concept to explain in one sentence and I think we have to live with that fact.

> (Just to confirm, this is the message found over the hover state of the
> permission icon in the URL bar?)

Yes :)

> 
> Asking state:
> "Manage sharing HTML canvas image data with this site"

Didn't flod note that "Manage" is the inconsistent term to use here? Maybe I misunderstand. Now that I'm reading comment 104 again, I notice that, flod, you're also wrong about the usage of this tooltip. The context is not "permission to". Hence "Access image canvas data" would be a very misleading tooltip. It's a tooltip on a button that let's you open a panel to manage settings for websites accessing image data.

I like this proposed text better, but I also think it's unimportant enough that we can just drop the issue.

> Blocked state:
> "You have blocked HTML canvas image data for this website"

There's no blocked state because this is not a regular permission (yet?).

	Francesco Lodolo [:flod]
	Comment 123 • 7 years ago

(In reply to Johann Hofmann [:johannh] from comment #122)
> > Asking state:
> > "Manage sharing HTML canvas image data with this site"
> 
> Didn't flod note that "Manage" is the inconsistent term to use here? Maybe I
> misunderstand. Now that I'm reading comment 104 again, I notice that, flod,
> you're also wrong about the usage of this tooltip. The context is not
> "permission to". Hence "Access image canvas data" would be a very misleading
> tooltip. It's a tooltip on a button that let's you open a panel to manage
> settings for websites accessing image data.

You're correct: if the action associated to the button it to open the setting, then it's correct as it is. I assumed it was a notification telling you that the current website has that permission.

	Tim Nguyen :ntim
	Comment 124 • 7 years ago

(In reply to Sylvestre Ledru [:sylvestre] from comment #120)
> Release Note Request (optional, but appreciated)
> [Why is this notable]: Seems like a nice cool feature
> [Affects Firefox for Android]: maybe?
> [Suggested wording]: Improved privacy by notify the user in case of advanced
> tracking
> (please provide a better wording)
> [Links (documentation, blog post, etc)]:

I think it's a bad idea to advertise this to the general public, because you have to enable the "privacy.resistFingerprinting" pref for this to work. A lot of people are unaware of the consequence of enabling this pref: the UA is spoofed to Firefox 50 (hence AMO thinks you can't use webextensions), timezones are spoofed to UTC (which can be annoying on Calendar websites), ...


If advertising this feature is something that's wanted, then the feature should probably have its own preference.

	Sylvestre Ledru [:Sylvestre]
	Comment 125 • 7 years ago

ok, thanks for the input!

	Katelyn Gadd (:kael)
	Comment 126 • 7 years ago

Hi,

If I'm understanding this correctly all canvas getImageData/etc will silently return white now? That seems like it will break applications (it will certainly break some of the html5 games I've written) silently, and that's not optimal. Is there a straightforward way that content JS can check to see whether it has getImageData permissions so that developers don't have to resort to weird hacks to figure out whether this is disabled?

Incidentally, 'paint an image to a canvas and read it back' is a common hack for getting pixel data from bitmaps in HTML5, and it seems like this will break that as well? I know the new ImageBitmap class is an alternative but I expect that is not widely used, especially because it replaces a synchronous operation with an asynchronous one.

If there's no way to feature detect this and it will break the paint-and-readback technique it seems important to clearly message this to developers so they know to update their content before FF58 breaks it.

	Tim Nguyen :ntim
	Comment 127 • 7 years ago

(In reply to K. Gadd (:kael) from comment #126)
> If there's no way to feature detect this and it will break the
> paint-and-readback technique it seems important to clearly message this to
> developers so they know to update their content before FF58 breaks it.

This feature is behind the `privacy.resistFingerprinting` preference (disabled by default for all users) for that specific reason.

	Johann Hofmann [:johannh]
	Comment 128 • 7 years ago

I agree with Tim, we really shouldn't advertise this without a solid UX story.

	moz-nuc
	Comment 129 • 6 years ago

I couldn't find any discussion about the fact that this is missing from the Page Info 'Permissions' tab as of FF58 final, so remembering the permission for a site is not revertable.

If that's right, is the only way of undoing it currently via sqlite, e.g.:

sqlite3 ~/.mozilla/firefox/*default/permissions.sqlite "delete from moz_perms where origin = 'https://example.com' and type = 'canvas/extractData';"

?

	moz-nuc
	Comment 130 • 6 years ago

Apologies, found https://bugzilla.mozilla.org/show_bug.cgi?id=1413780