Closed
Bug 1393283
Opened 7 years ago
Closed 7 years ago
privacy.resistFingerprinting should change the user agent to 52, not 50
Categories
(Core :: DOM: Security, defect, P2)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: francois, Assigned: ethan)
References
(Blocks 1 open bug)
Details
(Whiteboard: [tor][fingerprinting][domsecurity-active])
Attachments
(2 files, 4 obsolete files)
179.67 KB,
image/png
|
Details | |
5.59 KB,
patch
|
ethan
:
review+
|
Details | Diff | Splinter Review |
If the privacy.resistFingerprinting option aims to make everybody's user agent look like Tor, should it not use the same version of Firefox as Tor (ESR 52)? Right now, my user agent gets set to "Mozilla/5.0 (Windows NT 6.1; rv:50.0) Gecko/20100101 Firefox/50.0" when I turn the pref on in Nightly. This unnecessarily breaks Test Pilot: https://github.com/mozilla/testpilot/issues/2597#issuecomment-314693908 and makes many add-ons uninstallable.
Assignee | ||
Updated•7 years ago
|
Whiteboard: [tor
Assignee | ||
Updated•7 years ago
|
Priority: -- → P2
Whiteboard: [tor → [tor][fingerprinting]
Comment 1•7 years ago
|
||
The reason that we chose to round the version number down to the nearest 10 is twofold. 1. It is trivial to implement. 2. There is no official guideline to tell us that how to progress the version number for ESRs. So, the way, ESR for every seven releases, could be changed in the future. However, we didn't see the problem of breaking addons at that time. So, given this, I think we should change the spoofed version into the last ESR version just like Francois said. This can provide a better compatibility for fingerprinting resistance. Arthur, what do you think about this?
Flags: needinfo?(arthuredelstein)
Comment 2•7 years ago
|
||
(In reply to Tim Huang[:timhuang] from comment #1) > The reason that we chose to round the version number down to the nearest 10 > is twofold. > 1. It is trivial to implement. > 2. There is no official guideline to tell us that how to progress the > version number for ESRs. So, the way, ESR for every seven releases, could be > changed in the future. 3. There is no further maintenance required, the algorithm will work forever 4. (?) TBB will also eventually use privacy.resistFingerprinting come ESR59 (I assume) and this would bring it into line (In reply to François Marier [:francois] from comment #0) > and makes many add-ons uninstallable. I brought this up 7 months ago (comment 2), and again a few days ago in https://bugzilla.mozilla.org/show_bug.cgi?id=1333933#c37 I think just moving the version is a hack not a solution. Whitelist AMO so the site knows the correct value, displays the right information to end users, and behaves accordingly. Patch if necessary any fallout from direct installs via file. I don't understand how Test Pilot works, but do Web Extensions need to exempted somehow?
Comment 3•7 years ago
|
||
(In reply to Tim Huang[:timhuang] from comment #1) > The reason that we chose to round the version number down to the nearest 10 > is twofold. > 1. It is trivial to implement. > 2. There is no official guideline to tell us that how to progress the > version number for ESRs. So, the way, ESR for every seven releases, could be > changed in the future. > However, we didn't see the problem of breaking addons at that time. So, > given this, I think we should change the spoofed version into the last ESR > version just like Francois said. This can provide a better compatibility for > fingerprinting resistance. > > Arthur, what do you think about this? Hi Tim, yes, that sounds reasonable to me. I wonder if it would be helpful to add a warning something like let lastESRVersion = firefoxVersion - ((firefoxVersion - 3) % 7); if (firefox_version_string.contains("ESR") && (firefoxVersion % 7 != 3)) { debug_log_warning("Please update ESR version formula in nsRFPService.cpp"); } so that if ESRs change in the future, we won't forget to update the code.
Flags: needinfo?(arthuredelstein)
Reporter | ||
Comment 4•7 years ago
|
||
(In reply to Arthur Edelstein (Tor Browser dev) [:arthuredelstein] from comment #3) > I wonder if it would be helpful to add a warning something like > > let lastESRVersion = firefoxVersion - ((firefoxVersion - 3) % 7); > if (firefox_version_string.contains("ESR") && > (firefoxVersion % 7 != 3)) { > debug_log_warning("Please update ESR version formula in > nsRFPService.cpp"); > } > > so that if ESRs change in the future, we won't forget to update the code. Maybe even an assertion?
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → ettseng
Updated•7 years ago
|
Whiteboard: [tor][fingerprinting] → [tor][fingerprinting][domsecurity-active]
Comment 5•7 years ago
|
||
> Whitelist AMO so the site knows the correct value, displays the right information to end users, and behaves accordingly For AMO, it's not as easy as this. The AMO server needs to somehow figure out the version of Firefox to know if the add-on is compatible or not. Currently, AMO parses the user agent string for this. The current AMO gives you a "download anyway" button for incompatible add-ons. We should do the same in the new AMO (which isn't live yet). I filed this so we don't forget: https://github.com/mozilla/addons-frontend/issues/3021
Assignee | ||
Comment 6•7 years ago
|
||
I tried to reproduce this bug but got confused. Yes, I did see the warning message, however, 1. Containers was still enabled successfully and can be used 2. The message says "Containers requires Firefox 53 or later", not 51 If the minimum version requirement was changed to 53, then spoofing the version as the latest ESR (currently 52) won't help And I don't know why the Containers Test Pilot could be enabled even if AMO detected that my browser version is Firefox 50? Francois, any idea?
Flags: needinfo?(francois)
Assignee | ||
Comment 7•7 years ago
|
||
(In reply to Ethan Tseng [:ethan] from comment #6) > 1. Containers was still enabled successfully and can be used After some trials, I discovered it seems to be a flaw of the installation process of Test Pilot. If I install Test Pilot and Containers at the same time, Containers would be installed successfully (while still displaying the error message.) If I install Test Pilot first, then navigate to the home page of Test Pilot and choose to install Containers, it could not be installed. > If the minimum version requirement was changed to 53, then spoofing the > version as the latest ESR (currently 52) won't help ... On second thought, I think spoofing the version as the latest ESR is still a good idea anyway. So I'll move forward regardless of this issue.
Flags: needinfo?(francois)
Reporter | ||
Comment 8•7 years ago
|
||
(In reply to Ethan Tseng [:ethan] from comment #7) > > If the minimum version requirement was changed to 53, then spoofing the > > version as the latest ESR (currently 52) won't help ... > > On second thought, I think spoofing the version as the latest ESR is still a > good idea anyway. > So I'll move forward regardless of this issue. Yeah, it's not going to make it work in all cases, after all, we are potentially faking an earlier version, but it will help. Prior to this change, ESR59 would have claimed to be Firefox 50. That's quite a difference!
Assignee | ||
Comment 9•7 years ago
|
||
A WIP patch. I made a small refactor to move the logic of generating the spoofed value of User Agent from nsHttpHandler to nsRFPService. Tim, could you take a look?
Attachment #8903496 -
Flags: feedback?(tihuang)
Comment 10•7 years ago
|
||
Comment on attachment 8903496 [details] [diff] [review] bug-1393283.patch Review of attachment 8903496 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/resistfingerprinting/nsRFPService.cpp @@ +165,5 @@ > } > > +/* static */ > +void > +nsRFPService::GetSpoofedUserAgent(nsCString &userAgent) Should be nsACString. @@ +172,5 @@ > + // By doing so, the anonymity group will cover more versions instead of one > + // version. > + nsCOMPtr<nsIXULAppInfo> appInfo = > + do_GetService("@mozilla.org/xre/app-info;1"); > + nsCString appVersion; Should use nsAutoCString. @@ +173,5 @@ > + // version. > + nsCOMPtr<nsIXULAppInfo> appInfo = > + do_GetService("@mozilla.org/xre/app-info;1"); > + nsCString appVersion; > + appInfo->GetVersion(appVersion); I think it would be nice if you can add a null check here. Something like ``` if (!appInfo) { return; } ``` @@ +179,5 @@ > + nsresult rv; > + uint32_t firefoxVersion = appVersion.ToInteger(&rv); > + uint32_t spoofedVersion = firefoxVersion; > + if (NS_SUCCEEDED(rv)) { > + spoofedVersion = firefoxVersion - ((firefoxVersion - 3) % 7); Could you add a comment to describe this? I mean how this equation rounds the current version down to the nearest ESR version.
Attachment #8903496 -
Flags: feedback?(tihuang) → feedback+
Assignee | ||
Comment 11•7 years ago
|
||
What does this patch do? 1. Create a new function nsRFPService::GetSpoofedUserAgent() to move the logic of spoofing User Agent from nsHttpHandler to nsRFPService. 2. Change the formula to generate the spoofed value of Firefox version. Use the last ESR version instead of a multiple of 10.
Attachment #8903496 -
Attachment is obsolete: true
Attachment #8904147 -
Flags: review?(tihuang)
Comment 12•7 years ago
|
||
Comment on attachment 8904147 [details] [diff] [review] Use the last ESR version as the spoofed Firefox version Review of attachment 8904147 [details] [diff] [review]: ----------------------------------------------------------------- In general, your patch looks good to me. Although, you still need to modify the test case[1] for testing the userAgent spoofing. [1] http://searchfox.org/mozilla-central/source/browser/components/resistfingerprinting/test/browser/browser_navigator.js
Attachment #8904147 -
Flags: review?(tihuang)
Assignee | ||
Comment 13•7 years ago
|
||
Update the test case for User Agent.
Attachment #8904147 -
Attachment is obsolete: true
Attachment #8904300 -
Flags: review?(tihuang)
Comment 14•7 years ago
|
||
Comment on attachment 8904300 [details] [diff] [review] Use the last ESR version as the spoofed Firefox version Review of attachment 8904300 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/resistfingerprinting/nsRFPService.cpp @@ +169,5 @@ > +nsresult > +nsRFPService::GetSpoofedUserAgent(nsACString &userAgent) > +{ > + // This function generates the spoofed value of User Agent. > + // Firefox UA string is composed of four components: Actually, this is not totally correct, see [1]. I think you can leave a link like [1] to describe the composition of a userAgent string. And describe the format of the spoofed string here, something like 'Mozilla/5.0 (OS-or-CPU; rv:geckoversion) Gecko/legacybuildtime Firefox/firefoxversion'. [1] https://developer.mozilla.org/en-US/docs/Web/API/NavigatorID/userAgent#Value
Attachment #8904300 -
Flags: review?(tihuang) → review+
Assignee | ||
Comment 15•7 years ago
|
||
(In reply to Tim Huang[:timhuang] from comment #14) > Comment on attachment 8904300 [details] [diff] [review] > Actually, this is not totally correct, see [1]. I think you can leave a link > like [1] to describe the composition of a userAgent string. And describe the > format of the spoofed string here, something like 'Mozilla/5.0 (OS-or-CPU; > rv:geckoversion) Gecko/legacybuildtime Firefox/firefoxversion'. Tim, thanks for your review. I'll refine the code comments and put links to the reference.
Assignee | ||
Comment 16•7 years ago
|
||
Hi Patrick, Could you help to review this patch? What does the patch do was described in comment 11.
Attachment #8904300 -
Attachment is obsolete: true
Attachment #8904465 -
Flags: review?(mcmanus)
Updated•7 years ago
|
Attachment #8904465 -
Flags: review?(mcmanus) → review+
Assignee | ||
Comment 17•7 years ago
|
||
Refresh commit message "r=tihuang, r=mcmanus".
Attachment #8904465 -
Attachment is obsolete: true
Attachment #8904554 -
Flags: review+
Assignee | ||
Comment 18•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f68452ea3a51d0d0420c32a88ea50211eda7a98
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 19•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/52937159a1b6 Use the last ESR version as the spoofed Firefox version. r=tihuang, r=mcmanus
Keywords: checkin-needed
Comment 20•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/52937159a1b6
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 21•6 years ago
|
||
> uint32_t spoofedVersion = firefoxVersion - ((firefoxVersion - 3) % 7);
If the next ESR has been moved to 60, then the above formula will fail for FF59 users (we would expect them to be 52 as per ESR), and for FF60+ users (it will always be out by one - i.e currently code will use 59, IRL they should be 60)
Flags: needinfo?(ettseng)
Assignee | ||
Comment 22•6 years ago
|
||
(In reply to Simon Mainey from comment #21) > If the next ESR has been moved to 60, then the above formula will fail for > FF59 users (we would expect them to be 52 as per ESR), and for FF60+ users > (it will always be out by one - i.e currently code will use 59, IRL they > should be 60) Thanks for the notice. I just checked and realized that it was confirmed our next ESR will be 60. Leave the NI request to remind me to file a bug for this issue.
Comment 23•6 years ago
|
||
(In reply to Simon Mainey from comment #21) > and for FF60+ users (it will always be out by one - i.e currently code will use 59, IRL they should be 60) err... that was assuming that every subsequent ESR is still seven cycles. The next ESR after 60 might be 66 to get back "on track". I guess we'll have to remember to check ESR info every time Nightly gets near
Comment 24•6 years ago
|
||
ethan: added the bug for you - see Bug 1428111 (I needinfo'd you on the new one)
Flags: needinfo?(ettseng)
You need to log in
before you can comment on or make changes to this bug.
Description
•