Closed Bug 1418162 Opened 6 years ago Closed 6 years ago

nsRFPService::GetSpoofedUserAgent() should use UpdateUtils.getUpdateChannel for 'defaultUpdateChannel'

Categories

(Core :: DOM: Security, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla61
Tracking Status
firefox-esr60 60+ verified
firefox59 --- wontfix
firefox60 + verified
firefox61 --- verified

People

(Reporter: ethan, Assigned: tjr)

References

(Blocks 1 open bug)

Details

(Whiteboard: [tor][fingerprinting][domsecurity-backlog1][fp-triaged])

Attachments

(1 file)

See bug 1402866 for how Services.appinfo.defaultUpdateChannel is wrong for RC builds.
Priority: -- → P2
Whiteboard: [fingerprinting]
Assignee: nobody → ettseng
Blocks: 1402866
Whiteboard: [fingerprinting] → [fingerprinting][domsecurity-backlog1]
Priority: P2 → P1
Whiteboard: [fingerprinting][domsecurity-backlog1] → [fingerprinting][domsecurity-backlog1][fp-triaged]
Depends on: 1432031
Whiteboard: [fingerprinting][domsecurity-backlog1][fp-triaged] → [tor][fingerprinting][domsecurity-backlog1][fp-triaged]
Assignee: ethantseng → tom
Flags: needinfo?(tom)
Comment on attachment 8952145 [details]
Bug 1418162 Use a build constant to determine update channel, and update ESR equation for 60

https://reviewboard.mozilla.org/r/221382/#review227206

r=me but see important note on prefs usage below.

::: toolkit/components/resistfingerprinting/nsRFPService.cpp:306
(Diff revision 1)
> +  nsCOMPtr<nsIPrefBranch> prefs;
> +  prefService->GetDefaultBranch(nullptr, getter_AddRefs(prefs));
> +  MOZ_RELEASE_ASSERT(prefs);
>  
>    nsAutoCString updateChannel;
> -  rv = runtime->GetDefaultUpdateChannel(updateChannel);
> +  rv = prefs->GetStringPref("app.update.channel", nsDependentCString(NS_STRINGIFY(MOZ_UPDATE_CHANNEL)), 0, updateChannel);

So, this works, but it uses the default prefs. If we want the default update channel, we could just use the one that was compiled in only, ie MOZ_UPDATE_CHANNEL ? The only way that the default pref is modified is if this is an enterprise install which overrides it (which I think tor/rfp shouldn't care about), a funnelcake (ditto), a custom build (ditto, but then also it could modify MOZ_UPDATE_CHANNEL so anyway this is moot) or some chrome JS modified the value of the pref (which I also don't think we should care about).

In other words, for the purposes of the RFP (given that we're putting this into there directly anyway), should we just use the define? Might make sense to doublecheck this with one of the build peers to make sure, but I think that would make sense here. Especially if all we use it for is to work out if we're ESR... perhaps there's a better way to determine that?

::: toolkit/components/resistfingerprinting/nsRFPService.cpp:313
(Diff revision 1)
>  
>    // If we are running in Firefox ESR, determine whether the formula of ESR
>    // version has changed.  Once changed, we must update the formula in this
>    // function.
>    if (updateChannel.EqualsLiteral("esr")) {
> -    MOZ_ASSERT(((firefoxVersion % 7) == 3),
> +    MOZ_ASSERT(((firefoxVersion % 7) == 4),

Do we need to do this for 59 beta so that it doesn't think it's esr?
Attachment #8952145 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8952145 [details]
Bug 1418162 Use a build constant to determine update channel, and update ESR equation for 60

https://reviewboard.mozilla.org/r/221382/#review227278

::: commit-message-edd15:1
(Diff revisions 1 - 2)
> -Bug 1418162 Use app.update.channel to determine update channel, and update ESR equation for 60 r?gijs
> +Bug 1418162 Use a build constant to determine update channel, and update ESR equation for 60 r?gijs,Build

Where is the "update ESR equation" part of the patch?  Should the commit message be modified?  Or should the asserts in the code be changed?  Or both?
Attachment #8952145 - Flags: review+
(In reply to Nathan Froyd [:froydnj] from comment #4)
> Comment on attachment 8952145 [details]
> Bug 1418162 Use a build constant to determine update channel, and update ESR
> equation for 60
> 
> https://reviewboard.mozilla.org/r/221382/#review227278
> 
> ::: commit-message-edd15:1
> (Diff revisions 1 - 2)
> > -Bug 1418162 Use app.update.channel to determine update channel, and update ESR equation for 60 r?gijs
> > +Bug 1418162 Use a build constant to determine update channel, and update ESR equation for 60 r?gijs,Build
> 
> Where is the "update ESR equation" part of the patch?  Should the commit
> message be modified?  Or should the asserts in the code be changed?  Or both?

-  if (updateChannel.EqualsLiteral("esr")) {
-    MOZ_ASSERT(((firefoxVersion % 7) == 3),
+  if (!strcmp(NS_STRINGIFY(MOZ_UPDATE_CHANNEL), "esr")) {
+    MOZ_ASSERT(((firefoxVersion % 7) == 4),
       "Please udpate ESR version formula in nsRFPService.cpp");
   }
 
-  uint32_t spoofedVersion = firefoxVersion - ((firefoxVersion - 3) % 7);
+  // Starting from Firefox 10, Firefox ESR was released once every seven
+  // Firefox releases, e.g. Firefox 10, 17, 24, 31, and so on.
+  // Except we used 60 as an ESR instead of 59.
+  // We infer the last and closest ESR version based on this rule.
+  uint32_t spoofedVersion = firefoxVersion - ((firefoxVersion - 4) % 7);
Almost nothing except the updater should be using the update channel. What is the goal here?
See bug 1386076, and it seems to me what you really want is bug 1432737.
(In reply to :Gijs from comment #5)
> -  if (updateChannel.EqualsLiteral("esr")) {
> -    MOZ_ASSERT(((firefoxVersion % 7) == 3),
> +  if (!strcmp(NS_STRINGIFY(MOZ_UPDATE_CHANNEL), "esr")) {
> +    MOZ_ASSERT(((firefoxVersion % 7) == 4),
>        "Please udpate ESR version formula in nsRFPService.cpp");
>    }
>  
> -  uint32_t spoofedVersion = firefoxVersion - ((firefoxVersion - 3) % 7);
> +  // Starting from Firefox 10, Firefox ESR was released once every seven
> +  // Firefox releases, e.g. Firefox 10, 17, 24, 31, and so on.
> +  // Except we used 60 as an ESR instead of 59.
> +  // We infer the last and closest ESR version based on this rule.
> +  uint32_t spoofedVersion = firefoxVersion - ((firefoxVersion - 4) % 7);

Doh.  Look, mozreview even showed me....some reviewer I am. :)
I'm going to wait on Bug 1432737 and close all the dupes of this.
Depends on: 1432737
Flags: needinfo?(tom)
Comment on attachment 8952145 [details]
Bug 1418162 Use a build constant to determine update channel, and update ESR equation for 60

This patch has r+ from Nathan, removing from the queue.
Attachment #8952145 - Flags: review?(core-build-config-reviews)
[Tracking Requested - why for this release]: I just realized this bug may not have tracking flags, and it didn't.  We have an assertion that's going to start breaking things once 60 gets release. We're going to replace that assertion but we're waiting on Bug 1432737 to get fixed so we can do it properly.
I'm not convinced you should count on 1432737 getting in 60.
Attachment #8952145 - Flags: review?(core-build-config-reviews)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ac0b202721dc
Use a build constant to determine update channel, and update ESR equation for 60 r=froydnj,Gijs
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ac0b202721dc
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment on attachment 8952145 [details]
Bug 1418162 Use a build constant to determine update channel, and update ESR equation for 60

Approval Request Comment
[Feature/Bug causing the regression]: We had an assert that used the old ESR numbering scheme and when we bumped to 60 we needed to update it.

[User impact if declined]: ESR will crash.

[Is this code covered by automated tests?]: Yes

[Has the fix been verified in Nightly?]: Yes

[Needs manual test from QE? If yes, steps to reproduce]: Yes

Turn on privacy.resistFingerprinting in about:config
Go to any real website (e.g. mozilla.org)
Open the web console
Confirm navigator.userAgent says Version 60 (and not Version 59)

[List of other uplifts needed for the feature/fix]: None

[Is the change risky?]: No

[Why is the change risky/not risky?]: We do string comparison on a build constant, and remove a service call.

[String changes made/needed]: None
Attachment #8952145 - Flags: approval-mozilla-beta?
Comment on attachment 8952145 [details]
Bug 1418162 Use a build constant to determine update channel, and update ESR equation for 60

fix version number when enabling privacy.resistFingerprinting; approved for 60 and esr60.
Attachment #8952145 - Flags: approval-mozilla-beta? → approval-mozilla-release+
Flags: qe-verify+
Managed to reproduce the issue on Firefox 60.0b16 (20180426170554) on Windows 10 x64.

So far, the issue cannot be reproduced on the latest Nightly (20180501220047) and Firefox RC 60.0 (20180430165945) using the same STR on Windows 10 x64, Mac OS X 10.12 and Ubuntu 16.04 x64. 

Further investigations will be done tomorrow when the Firefox esr60 build will be landed for testing.
The issue cannot be reproduced on the latest esr60 (build ID: 20180503092946) on Windows Mac OS X 10.12, Windows 10 x64 and Ubuntu 16.04 x64, thus it is confirmed as Verified Fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.