1918690 - Extend canvas randomization permission test

Status: RESOLVED FIXED

(Core :: Privacy: Anti-Tracking, task, P1)

Description

[Fatih Kilic [:fkilic]]

The goal of this bug is to investigate if granting canvas permission allows canvas reading without any randomization or placeholder value.

Comment 1

[Fatih Kilic [:fkilic]]

Attachment: Bug 1918690: Improve canvas randomization permission test. r?tjr

Comment 2

[Fatih Kilic [:fkilic]]

Okay so I just submitted a try, it should hopefully let us know if the granting permission actually works or not. https://treeherder.mozilla.org/jobs?repo=try&revision=a7f0676e0f2ab447464c4ce2b6a65c1e5033b130. Also do note the test doesn't cover offscreen canvas because right now offscreen canvas doesn't check for permission and it is a known issue.

Comment 3

[Thorin [:thorin]]

Tor Browser hat ... now I'm worried RFP doesn't cover all methods of offscreen canvas?

Comment 4

[Fatih Kilic [:fkilic]]

It does cover it (or at least my local testing shows it, by failing tests with permission granted, i.e. expects no randomization but still gets randomized value from offscreen canvas), but I just disabled them in the test because granting permission would still return placeholder/RFP random. So offscreen canvas returns random regardless of the permission state.

also here's the bug i mentioned https://bugzilla.mozilla.org/show_bug.cgi?id=1422862

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c02a71489d9d
Improve canvas randomization permission test. r=tjr

Comment 6

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/c02a71489d9d

Comment 7

[Thorin [:thorin]]

https://bugzilla.mozilla.org/show_bug.cgi?id=1896175#c7 still pertains - adding a test that doesn't seem to capture the issue hasn't solved the problem - do you want me to open another issue?

Comment 8

[Fatih Kilic [:fkilic]]

Sure, but do you know what is missing from the test I wrote? We can create a new bug sure but if tzp and my test do the same thing then I don't really know we would move forward.

Comment 9

[Thorin [:thorin]]

but do you know what is missing from the test I wrote?

Not really. I can't follow it TBH :) These are in effect known pixel tests, but where are the known values? I see true value as null for the five tests which doesn't make sense to me, but not my test.

Which leaves us at the point that a proven real world test on a live HTTPS site dedicated to these things, shows the issue still exists. I can't do any more than that. If we don't reopen (or open another issue), then nothing will change (and the issue isn't resolved). Of course, it is feasible that TZP is wrong, but it's not - you can see the hashes change - they are stable without any randomization and they change when FPP or RFP is used.

So, now what? Reopen is my suggestion so all info is in the same place. Ask for help/review.

Comment 10

[Fatih Kilic [:fkilic]]

Sure let's reopen it. About the test, I replace nulls with actual values in add_setup. I put nulls as a placeholder.

Comment 11

[Fatih Kilic [:fkilic]]

Okay so I was able reproduce the issue, and I have no idea why it might happen. If you set canvas's size, we get different values sometimes (not always). It tends to occur especially when the canvas size is small. So for example with a 100x100 canvas, it never happens, and for 10x10 it always happens, for 20x20 it happens sometimes. I have no idea what might cause this. AFAIK the only check RFP does about canvas size is whether it is bigger than 1x1 or not.

Comment 12

[Thorin [:thorin]]

You'd have to step me thru your testing methodology for me to fully understand it (on irc at some stage). I did do a double take on the 3x1 canvas size, but ignored it in the meantime. TZP uses a set size with set known values - i.e we don't need to calculate them on the fly, but for here, we should, e.g. if png compression changes (as it has in the past) - see this for compression changes and for sizes and known results. This makes it easy to detect if it's protected or not. As for detecting RFP vs FPP that's more complicated. For you, it's easy - all three should differ: RFP/FPP/nothing (and you don't have to worry about extensions like I do)

Comment 13

[Fatih Kilic [:fkilic]]

Attachment: Bug 1918690: Extend test to also test Offscreen canvases. r?tjr

Comment 14

[Fatih Kilic [:fkilic]]

Hmm okay so I changed the test to set the size to 3x1, and it fails. getImageData works but not toDataURL or toBlob (I believe both toDataURL and toBlob call the same codepath, but haven't confirmed it). My suspicion is https://searchfox.org/mozilla-central/rev/56fd3b43187bca036141c384b809e61d51a447de/dom/canvas/CanvasRenderingContext2D.cpp#2078 here we don't check for permission. I'll see if checking permission here fixes it at least for toDataURL.

Comment 15

[Fatih Kilic [:fkilic]]

Yeah the issue seems to be that line. The problem is plumbing the principal all the way to there.

Comment 16

[Fatih Kilic [:fkilic]]

canvas context has the principal but we would have to plumb it to all the overrides here https://searchfox.org/mozilla-central/search?q=symbol:_ZN33nsICanvasRenderingContextInternal14GetInputStreamEPKcRK12nsTSubstringIDsEPP14nsIInputStream&redirect=false. I'll try to fix it and see if it works.

Comment 17

[Fatih Kilic [:fkilic]]

The patch that I'm currently working on builds on top of bug 1422862, so it is blocking. We can skip offscreen canvases if fixing this permission issue is really needed though.

Comment 18

[Fatih Kilic [:fkilic]]

Attachment: Bug 1918690: Check canvas permission in RandomizePixels function. r?tjr

Comment 19

[Fatih Kilic [:fkilic]]

Attachment: Bug 1918690: Implement IsImageExtractionAllowed_impl. r?tjr

Comment 20

[Phabricator Automation]

Comment on attachment 9434300 [details]
Bug 1918690: Implement IsImageExtractionAllowed_impl. r?tjr

Revision D227385 was moved to bug 1422862. Setting attachment 9434300 [details] to obsolete.

Comment 21

[Thorin [:thorin]]

Where are we at - TB users are getting confused - https://forum.torproject.org/t/is-new-identity-enough-to-protect-me-as-i-never-allow-extract-canvas-data/17412/4

Comment 22

[Fatih Kilic [:fkilic]]

We are still waiting for a review

Comment 23

[Thorin [:thorin]]

Attachment: 100% reproducible.png

OK, but fixing the test won't exactly fix the actual problem - it'll just tell you what I've been saying for months, since bug 1896175 landed :)

Comment 24

[Thorin [:thorin]]

FYI: in FF137

• normal window with ETP Standard: i.e FPP is not enabled
• RFP enabled
• site exception for RFP canvas
• we still get FPP being applied to toBlob and toDataURL

Comment 25

[Fatih Kilic [:fkilic]]

(In reply to Thorin [:thorin] from comment #23)

Created attachment 9467039 [details]
100% reproducible.png

OK, but fixing the test won't exactly fix the actual problem - it'll just tell you what I've been saying for months, since bug 1896175 landed :)

Is this image with all the patches applied in https://phabricator.services.mozilla.com/D227385 ? The bug is named extend tests but we have 5 patches in that stack 4 of which aren't tests.

Comment 26

[Thorin [:thorin]]

Is this image with all the patches applied in https://phabricator.services.mozilla.com/D227385

That image is actually Tor Browser. But tests using Nightly - Built from https://hg.mozilla.org/mozilla-central/rev/f47117ef97a7314f4c3d0f844b7c908b0c97038c - shows the same behavior

Comment 27

[Fatih Kilic [:fkilic]]

Alright then I would say https://phabricator.services.mozilla.com/D222869 should be able to fix this, but it hasn't landed yet.

Comment 28

[Thorin [:thorin]]

For the record: Bug 1910796 (landed 2 days ago) changes the compression - so if known pixel tests are relying on hashes they will fail

• TZP checks each pixel's rbga in getImageData and is not affected
• TZP uses a known hash for toBlob and toDataURL and is affected
  • but never fear I am patching to allow for the new known hashes as I type

Comment 29

[BugBot [:suhaib / :marco/ :calixte]]

The following patch is waiting for review from an inactive reviewer:

ID	Title	Author	Reviewer Status
D222869	Bug 1918690: Check canvas permission in RandomizePixels function. r?tjr	fkilic	jgilbert: Disabled

:fkilic, could you please find another reviewer or abandon the patch if it is no longer relevant?

For more information, please visit BugBot documentation.

Comment 30

[Fatih Kilic [:fkilic]]

Changed it to gfx-reviewers

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/bf7503d01e6d
https://hg.mozilla.org/integration/autoland/rev/c64ad97247b7
Check canvas permission in RandomizePixels function. r=tjr,gfx-reviewers,nical
https://github.com/mozilla-firefox/firefox/commit/4d63bbaa5e65
https://hg.mozilla.org/integration/autoland/rev/81e871035004
Extend test to also test Offscreen canvases. r=tjr

Comment 32

[amarc]

Backed out for causing bc failures @ browser_etp_permission.js

• Backout link
• Push with failures
• Failure Log

Comment 33

[Fatih Kilic [:fkilic]]

Thank you ill look into it

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/0b66c5a96af2
https://hg.mozilla.org/integration/autoland/rev/59801de9e2c8
Check canvas permission in RandomizePixels function. r=tjr,gfx-reviewers,nical
https://github.com/mozilla-firefox/firefox/commit/b188ced5fceb
https://hg.mozilla.org/integration/autoland/rev/1e26b83dce12
Extend test to also test Offscreen canvases. r=tjr

Comment 35

[agoloman]

https://hg.mozilla.org/mozilla-central/rev/c64ad97247b7
https://hg.mozilla.org/mozilla-central/rev/81e871035004
https://hg.mozilla.org/mozilla-central/rev/59801de9e2c8
https://hg.mozilla.org/mozilla-central/rev/1e26b83dce12

Comment 36

[Fatih Kilic [:fkilic]]

Attachment: Bug 1918690: Check canvas permission in RandomizePixels function. r?tjr

Original Revision: https://phabricator.services.mozilla.com/D222869

Comment 37

[Fatih Kilic [:fkilic]]

Attachment: Bug 1918690: Extend test to also test Offscreen canvases. r?tjr

Previously this test didn't test for offscreen canvses. After all the previous patches in this stack, we can now enable offscreen canvas permission tests.
We are basically removing if (isOffscreen) continue; logic

Original Revision: https://phabricator.services.mozilla.com/D222838

Comment 38

[Tom Ritter [:tjr] (OOTO Oct 18-26)]

To be clear, we are requesting uplift to put this into ESR for Tor. It affects he behavior of Resist Fingerprinting, but not for supported FF users. It doesn't need to be uplifted, but given that it's been stable for us and doesn't change the functionality of ESR we thought we'd ask.

Comment 39

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/9cdd75dd7ace
https://hg.mozilla.org/releases/mozilla-esr140/rev/e3e764f52911