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)
Core
DOM: Security
Tracking
()
VERIFIED
FIXED
mozilla61
People
(Reporter: ethan, Assigned: tjr)
References
(Blocks 1 open bug)
Details
(Whiteboard: [tor][fingerprinting][domsecurity-backlog1][fp-triaged])
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
Gijs
:
review+
jcristau
:
approval-mozilla-release+
|
Details |
See bug 1402866 for how Services.appinfo.defaultUpdateChannel is wrong for RC builds.
Reporter | ||
Updated•6 years ago
|
Priority: -- → P2
Whiteboard: [fingerprinting]
Reporter | ||
Updated•6 years ago
|
Assignee: nobody → ettseng
Updated•6 years ago
|
Whiteboard: [fingerprinting] → [fingerprinting][domsecurity-backlog1]
Reporter | ||
Updated•6 years ago
|
Priority: P2 → P1
Whiteboard: [fingerprinting][domsecurity-backlog1] → [fingerprinting][domsecurity-backlog1][fp-triaged]
Updated•6 years ago
|
Whiteboard: [fingerprinting][domsecurity-backlog1][fp-triaged] → [tor][fingerprinting][domsecurity-backlog1][fp-triaged]
Assignee | ||
Updated•6 years ago
|
Assignee: ethantseng → tom
Flags: needinfo?(tom)
Comment hidden (mozreview-request) |
Comment 2•6 years ago
|
||
mozreview-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/#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 hidden (mozreview-request) |
Comment 4•6 years ago
|
||
mozreview-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+
Comment 5•6 years ago
|
||
(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);
Comment 6•6 years ago
|
||
Almost nothing except the updater should be using the update channel. What is the goal here?
Comment 7•6 years ago
|
||
See bug 1386076, and it seems to me what you really want is bug 1432737.
Comment 8•6 years ago
|
||
(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. :)
Assignee | ||
Comment 9•6 years ago
|
||
I'm going to wait on Bug 1432737 and close all the dupes of this.
Depends on: 1432737
Flags: needinfo?(tom)
Comment 10•6 years ago
|
||
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)
Assignee | ||
Comment 11•6 years ago
|
||
[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.
status-firefox60:
--- → affected
status-firefox-esr60:
--- → affected
tracking-firefox60:
--- → ?
tracking-firefox-esr60:
--- → ?
Comment 12•6 years ago
|
||
I'm not convinced you should count on 1432737 getting in 60.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8952145 -
Flags: review?(core-build-config-reviews)
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Updated•6 years ago
|
status-firefox59:
--- → wontfix
status-firefox61:
--- → affected
Comment 14•6 years ago
|
||
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
Comment 16•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ac0b202721dc
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee | ||
Comment 17•6 years ago
|
||
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 18•6 years ago
|
||
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+
Comment 19•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-release/rev/aed7099d7d46 https://hg.mozilla.org/releases/mozilla-beta/rev/eee5fa942701d2e4673df7c74b5ca86a122dcfd5 (FIREFOX_60b_RELBRANCH)
Updated•6 years ago
|
Flags: qe-verify+
Comment 20•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr60/rev/aed7099d7d46
Flags: in-testsuite+
Comment 21•6 years ago
|
||
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.
Updated•6 years ago
|
Comment 22•6 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•