Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fingerprinting attribute: navigator.deviceMemory #1157

Closed
jumde opened this issue Sep 17, 2018 · 21 comments · Fixed by brave/brave-core#3669 or brave/brave-core#6965
Closed

Fingerprinting attribute: navigator.deviceMemory #1157

jumde opened this issue Sep 17, 2018 · 21 comments · Fixed by brave/brave-core#3669 or brave/brave-core#6965
Labels
feature/shields/fingerprint The fingerprinting (aka: "device recognition") protection provided in Shields priority/P4 Planned work. We expect to get to it "soon". privacy QA Pass-Linux QA Pass-macOS QA Pass-Win64 QA/Test-Plan-Specified QA/Yes release-notes/include

Comments

@jumde
Copy link
Contributor

jumde commented Sep 17, 2018

From: brave/browser-laptop#14996

The attribute navigator.deviceMemory enables to obtain information about the device's memory (https://developer.mozilla.org/en-US/docs/Web/API/Navigator/deviceMemory). I consider it could be used as a fingerprinting vector.

Test Plan

  1. Open developer console
  2. Type in: navigator.deviceMemory === undefined
  3. Should return true
@jumde jumde added privacy feature/shields/fingerprint The fingerprinting (aka: "device recognition") protection provided in Shields labels Sep 17, 2018
@bbondy bbondy added this to the 1.x Backlog milestone Sep 22, 2018
@rebron rebron added the priority/P3 The next thing for us to work on. It'll ride the trains. label Sep 28, 2018
@tildelowengrimm tildelowengrimm added priority/P4 Planned work. We expect to get to it "soon". and removed priority/P3 The next thing for us to work on. It'll ride the trains. labels Oct 31, 2018
@riastradh-brave
Copy link
Contributor

Candidate solution: always return 1 (meaning 1 GB). The number is already limited to a power of two between 1/4 and 8 inclusive.

@snyderp does not yet have crawl data about how widely this is used.

@rebron rebron removed this from the 1.x Backlog milestone Feb 7, 2019
@pes10k
Copy link
Contributor

pes10k commented Aug 9, 2019

relevant to #2655

@bershanskiy
Copy link

So what is the plan for this? Do you want just to hard code navigator.deviceMemory to always be 1 GB as @riastradh-brave suggested?

Also, the same number is also exposed in HTTP header Client Hints with identifier device-memory. Does Brave suppress Client Hints?

@pes10k
Copy link
Contributor

pes10k commented Sep 30, 2019

So what is the plan for this? Do you want just to hard code navigator.deviceMemory to always be 1 GB as @riastradh-brave suggested?

SGTM, though fwiw I think this is a pretty low priority FP vector (though since the fix is easy, i think its good).

Also, the same number is also exposed in HTTP header Client Hints with identifier device-memory. Does Brave suppress Client Hints?

Yes, currently we dont' respect CH requests (thanks to great @jumde work). We may respect UA client-hint request if / when that becomes shipping

@bershanskiy
Copy link

@snyderp

Yes, currently we dont' respect CH requests

Which versions of the browser does this include? I did some testing earlier and it seemed like Brave does send Client Hints, even when Shields are set to "Block device recognition". I used snaps package (which might not be an official one):

Version 0.63.48 Chromium: 74.0.3729.108 (Official Build) (64-bit)

@bershanskiy
Copy link

@snyderp

though since the fix is easy, i think its good

Is there a wiki page explaining how to prepare a PR? I'm asking because I would most likely would make a PR to the brave-core repo (since it's the one with all the code) and I'm not sure if I can just commit the whole patched file or I should commit just a one-line patch. Also, do I need to write unit or integration tests? (The current fix allows all upstream tests to pass.)

@pes10k
Copy link
Contributor

pes10k commented Oct 1, 2019

@bershanskiy we discussed and the plan forward is to just remove the API all together. So, no need to lie, we'll just not say anything at all

@bsclifton
Copy link
Member

bsclifton commented Oct 1, 2019

Hey there @bershanskiy 😄

We don't have a wiki page (yet) helping with creation of a PR, but I can help guide you through that!

First step would be to clone and build the project per the docs here:
https://github.com/brave/brave-browser/wiki

Once that finishes (ex: after you run npm i && npm run init), you'll have a directory under src/brave which is pointing at brave/brave-core

The actual build process- it's up to you if you want to run it. It would be great to help ensure the code compiles, but at the same time, it can take several hours (6+ on a laptop) to compile the entire project (and you can submit patches without testing locally and maybe @snyderp or another one of us can help you test / verify)

For submitting the patch, you'll want to fork the brave-core repo if you haven't already and update your remote list (under src/brave) to add your fork in there (ex: git remote add bershanskiy [email protected]:bershanskiy/brave-core.git). You can then create a branch off master for your work and push that to your remote. GitHub will then show a "pull request" button the UI which let's you submit the patch against brave/brave-core

Tests are encouraged - although there's a learning curve. I'd be happy to help when you get further along

@bershanskiy
Copy link

@snyderp

we discussed and the plan forward is to just remove the API all together. So, no need to lie, we'll just not say anything at all

Just to make sure: remove the API even if the shields are down, right? This includes Client Hints Device-Memory as well, right? I agree that this is actually the best solution because it causes the least amount of confusion (no lies) without compatibility issues (since most UAs don't support it anyway).

@bershanskiy
Copy link

@bsclifton

Thanks for the detailed explanation! The build time is the primary roadblock I face. As a daily driver, I use a laptop from ~2013 and Chromium takes 8-10 hours to compile. I tried to use cloud VMs, but they come with the usual inconveniences of being in the cloud.

@jumde
Copy link
Contributor Author

jumde commented Oct 1, 2019

@bershanskiy - Client Hints should be disabled by default. Can you try to repro with the official Brave package? https://brave-browser.readthedocs.io/en/latest/installing-brave.html#linux

I checked with Shields enabled/disabled and I don't see any Client Hints on this demo page: https://client-hints-demo.appspot.com/

@bershanskiy
Copy link

@jumde Yes, official release has Client Hints disabled (I just tested).

@bershanskiy
Copy link

bershanskiy commented Oct 6, 2019

@bsclifton @snyderp
Removing navigator.deviceMemory is actually very trivial (and everything compiles properly and I verified manually that navigator.deviceMemory is actually gone). The changes are just:

diff --git a/third_party/blink/renderer/core/frame/navigator.h b/third_party/blink/renderer/core/frame/navigator.h
index 4572df017cf4..377827ad9a3c 100644
--- a/third_party/blink/renderer/core/frame/navigator.h
+++ b/third_party/blink/renderer/core/frame/navigator.h
@@ -24,7 +24,6 @@
 #include "third_party/blink/renderer/core/core_export.h"
 #include "third_party/blink/renderer/core/execution_context/context_lifecycle_observer.h"
 #include "third_party/blink/renderer/core/frame/navigator_concurrent_hardware.h"
-#include "third_party/blink/renderer/core/frame/navigator_device_memory.h"
 #include "third_party/blink/renderer/core/frame/navigator_id.h"
 #include "third_party/blink/renderer/core/frame/navigator_language.h"
 #include "third_party/blink/renderer/core/frame/navigator_on_line.h"
@@ -40,7 +39,6 @@ class LocalFrame;
 
 class CORE_EXPORT Navigator final : public ScriptWrappable,
                                     public NavigatorConcurrentHardware,
-                                    public NavigatorDeviceMemory,
                                     public NavigatorID,
                                     public NavigatorLanguage,
                                     public NavigatorOnLine,

and

diff --git a/third_party/blink/renderer/core/frame/navigator.idl b/third_party/blink/renderer/core/frame/navigator.idl
index f13febb148e9..b8e0bbc2b66e 100644
--- a/third_party/blink/renderer/core/frame/navigator.idl
+++ b/third_party/blink/renderer/core/frame/navigator.idl
@@ -36,7 +36,6 @@
 
 Navigator includes NavigatorConcurrentHardware;
 Navigator includes NavigatorCookies;
-Navigator includes NavigatorDeviceMemory;
 Navigator includes NavigatorID;
 Navigator includes NavigatorLanguage;
 Navigator includes NavigatorOnLine;

I prepared a more holistic patch that fixes (at least some of) the tests here. Please note that since these tests check existence (or rather absence) of deviceMemory everywhere, they might actually be sufficient.

I did not prepare a proper patch and PR for brave-core because I bet you have very specific guidelines how to do it but I could not find those.

P.S.: Obviously, the hashes in the patches are incorrect.

@pes10k
Copy link
Contributor

pes10k commented Oct 8, 2019

@bershanskiy thanks for all this! I appreciate all the work in this!

@jumde or @bsclifton, it sounds like the PR process is a bit much for @bershanskiy (I can def understand). Is there anyway someone internally could either handle the PR, or point @bershanskiy to the relevant docs?

@pes10k
Copy link
Contributor

pes10k commented Oct 8, 2019

also of interested maybe to @fmarier

@bershanskiy
Copy link

@snyderp To move this issue forward, I made a PR based on wiki instructions, this PR is basically what I posted above.

The browser builds and passes manual tests, but there are no automatic tests yet. Are there instructions on how to add these tests and, more importantly, how to run them? npm run test -- brave_unit_tests and npm run test -- brave_browser_tests just skip all the tests.

@pes10k
Copy link
Contributor

pes10k commented Oct 15, 2019

Fantastic @bershanskiy ! Thanks!

@fmarier @bsclifton @jumde can either of you point @bershanskiy towards where to find docs on writing good tests? It'd be great to pull this in if possible!

@bsclifton
Copy link
Member

@bsclifton bsclifton added this to the 1.5.x - Nightly milestone Jan 6, 2020
@btlechowski
Copy link

btlechowski commented Feb 28, 2020

Verification passed on

Brave 1.5.101 Chromium: 80.0.3987.116 (Official Build) beta (64-bit)
Revision dc00a510e4c2ae25c4d084cc3d946fc782249224-refs/branch-heads/3987@{#917}
OS Ubuntu 18.04 LTS

Reproduced on 1.4.95
image

Verified the test plan from the description
image
Verified in normal window, private window, private window with tor, guest window

Verification passed on

Brave 1.5.106 Chromium: 80.0.3987.122 (Official Build) beta (64-bit)
Revision cf72c4c4f7db75bc3da689cd76513962d31c7b52-refs/branch-heads/3987@{#943}
OS Windows 10 OS Version 1803 (Build 17134.1006)
  • Verified the STR from the description
  • Verified in a normal window, private window, private window with tor and guest window
    image

Verified passed with

Brave 1.5.106 Chromium: 80.0.3987.122 (Official Build) beta (64-bit)
Revision cf72c4c4f7db75bc3da689cd76513962d31c7b52-refs/branch-heads/3987@{#943}
OS macOS Version 10.14.6 (Build 18G3020)
  • Reproduced the issue using 1.4.95:

Screen Shot 2020-03-03 at 10 37 50 AM

  • Verified fixed with 1.5.x:

Screen Shot 2020-03-03 at 10 39 08 AM

  • Verified on clean and upgrade profiles
  • Verified using normal, private, Tor, and guest windows

@fmarier
Copy link
Member

fmarier commented Oct 28, 2020

The original fix for this was reverted in brave/brave-core#6965.

@fmarier fmarier reopened this Oct 28, 2020
@fmarier
Copy link
Member

fmarier commented Oct 28, 2020

New solution tracked in #12348.

@fmarier fmarier closed this as completed Oct 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/shields/fingerprint The fingerprinting (aka: "device recognition") protection provided in Shields priority/P4 Planned work. We expect to get to it "soon". privacy QA Pass-Linux QA Pass-macOS QA Pass-Win64 QA/Test-Plan-Specified QA/Yes release-notes/include
Projects
None yet
Development

Successfully merging a pull request may close this issue.