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)

defect

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)

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.
Whiteboard: [tor
Priority: -- → P2
Whiteboard: [tor → [tor][fingerprinting]
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)
(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?
(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)
(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: nobody → ettseng
Whiteboard: [tor][fingerprinting] → [tor][fingerprinting][domsecurity-active]
> 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
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)
(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)
(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!
Attached patch bug-1393283.patch (obsolete) — Splinter Review
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 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+
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 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)
Update the test case for User Agent.
Attachment #8904147 - Attachment is obsolete: true
Attachment #8904300 - Flags: review?(tihuang)
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+
(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.
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)
Attachment #8904465 - Flags: review?(mcmanus) → review+
Refresh commit message "r=tihuang, r=mcmanus".
Attachment #8904465 - Attachment is obsolete: true
Attachment #8904554 - Flags: review+
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/52937159a1b6
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
> 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)
(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.
(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
ethan: added the bug for you - see Bug 1428111 (I needinfo'd you on the new one)
Flags: needinfo?(ettseng)
Blocks: 1428111
You need to log in before you can comment on or make changes to this bug.