Closed Bug 1424341 Opened 7 years ago Closed 7 years ago

Allow independent and adjustable timer precision

Categories

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

Product:
Core
Component:
DOM: Events
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox-esr52 - wontfix
firefox57 --- wontfix
firefox58 blocking fixed
firefox59 + fixed

People

(Reporter: tjr, Assigned: tjr)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [fingerprinting][fp-triaged])

Attachments

(8 files, 3 obsolete files)

Bug 1424341 Add privacy.reduceTimerPrecision and privacy.reduceTimerPrecision.microseconds prefs
59 bytes, text/x-review-board-request
bkelly
: review+
timhuang
: review+
Details
Bug 1424341 Add tests for privacy.reduceTimerPrecision that confirm the default timer resolution works
59 bytes, text/x-review-board-request
timhuang
: review+
bkelly
: review+
Details
Bug 1424341 Round the Performance Timing APIs when privacy.reduceTimerPrecision is set
59 bytes, text/x-review-board-request
bkelly
: review+
timhuang
: review+
Details
Bug 1424341 Add tests for privacy.reduceTimerPrecision for the performance API
59 bytes, text/x-review-board-request
bkelly
: review+
timhuang
: review+
Details
Bug 1424341 Add tests for dynamically tuning the timer precision
59 bytes, text/x-review-board-request
bkelly
: review+
timhuang
: review+
Details
Bug 1424341 Add tests for Atomic*ReleaseAcquire
59 bytes, text/x-review-board-request
bkelly
: review+
Details
Bug 1424341 Turn the pref off for existing tests that perform fine-grained timing comparisons
59 bytes, text/x-review-board-request
mrbkap
: review+
Details
Beta backport of all commits
81.50 KB, patch
gchang
: approval-mozilla-beta+
gchang
: approval-mozilla-release+
Details | Diff | Splinter Review
To test issues like Bug 1403099 and Bug 1394735 we want to be able to control the timer precision value, and test with only this feature of resist fingerprinting enabled. So we're going to add another pref that can turn on timer reduction independent of privacy.resistFingerprinting as well as migrate the current hard-coded value into a pref as well.
Bug 1369303 addresses the performance API for Resist Fingerprinting, but all it does is disable it entirely; not round it. I think for these purposes we will want to round it.
Attachment #8935885 - Flags: review?(tihuang)
Attachment #8935886 - Flags: review?(tihuang)
Attachment #8935885 - Flags: review?(tihuang)
I still have to add tests that test the dynamically tuned parameter, round the performance API, and add tests for that, but I wanted to get something up and begin review.
Attachment #8935885 - Flags: review?(tihuang)
Attachment #8935885 - Flags: review?(bzbarsky)
Attachment #8935886 - Flags: review?(bzbarsky)
Attachment #8935885 - Flags: review?(bzbarsky) → review?(bkelly)
Attachment #8935886 - Flags: review?(bzbarsky) → review?(bkelly)
Comment on attachment 8935885 [details] Bug 1424341 Add privacy.reduceTimerPrecision and privacy.reduceTimerPrecision.microseconds prefs https://reviewboard.mozilla.org/r/206748/#review212974 ::: modules/libpref/init/all.js:1378 (Diff revision 1) > // Anti-fingerprinting, disabled by default > pref("privacy.resistFingerprinting", false); > +// A subset of Resist Fingerprinting protects for testing > +pref("privacy.reduceTimerPrecision", false); > +// Dynamically tune the resolution adjustment of the timer reduction for both of the two above prefs > +pref("privacy.reduceTimerPrecision.microseconds", 100000); I am wondering would it be better if the name of this pref is 'privacy.resistFingerprinting.reduceTimerPrecision.microseconds' since this pref will be shared with the fingerprinting resistance. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:55 (Diff revision 1) > > static StaticRefPtr<nsRFPService> sRFPService; > static bool sInitialized = false; > Atomic<bool, ReleaseAcquire> nsRFPService::sPrivacyResistFingerprinting; > -static uint32_t kResolutionUSec = 100000; > +Atomic<bool, ReleaseAcquire> nsRFPService::sPrivacyTimerPrecisionReduction; > +static uint32_t sResolutionUSec; We should we Atomic here since this value will be accessed both in the main-thread and the worker thread. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:249 (Diff revision 1) > NS_ENSURE_TRUE(prefs, NS_ERROR_NOT_AVAILABLE); > > rv = prefs->AddObserver(RESIST_FINGERPRINTING_PREF, this, false); > NS_ENSURE_SUCCESS(rv, rv); > > + rv = prefs->AddObserver(RFP_TIMER_PREF, this, false); Should we add an observer for RFP_TIMER_VALUE_PREF as well? In case someone wants to change this value on the fly wthout touching 'privacy.reduceTimerPrecision' or 'privacy.resistFingerprinting'. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:252 (Diff revision 1) > NS_ENSURE_SUCCESS(rv, rv); > > + rv = prefs->AddObserver(RFP_TIMER_PREF, this, false); > + NS_ENSURE_SUCCESS(rv, rv); > + > + Preferences::AddUintVarCache(&sResolutionUSec, I think we need to use Preferences::AddAtomicUintVarCache here since this value would be accessed both in the main-thread and the worker thread. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:283 (Diff revision 1) > void > nsRFPService::UpdatePref() > { > MOZ_ASSERT(NS_IsMainThread()); > sPrivacyResistFingerprinting = Preferences::GetBool(RESIST_FINGERPRINTING_PREF); > + sPrivacyTimerPrecisionReduction = Preferences::GetBool(RFP_TIMER_PREF); You may want to use Preferences::AddAtomicBoolVarCache[1] to maintain this pref. And adding a definiation[2] of this template function for ordering 'ReleaseAcquire' for succeeded. [1] https://searchfox.org/mozilla-central/source/modules/libpref/Preferences.h#304 [2] https://searchfox.org/mozilla-central/source/modules/libpref/Preferences.cpp#4986 ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:285 (Diff revision 1) > { > MOZ_ASSERT(NS_IsMainThread()); > sPrivacyResistFingerprinting = Preferences::GetBool(RESIST_FINGERPRINTING_PREF); > + sPrivacyTimerPrecisionReduction = Preferences::GetBool(RFP_TIMER_PREF); > > + // Timer stuff only I would suggest moving these stuff out of this function and making one for this, calling new created function when prefs get updated. The reason is that it could be error-prone and unnecessary if we run the rest of code of fingerprinting resistance when 'privacy.resistFingerprinting' is not touched, this could happen if we only flip 'privacy.reduceTimerPrecision'. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:343 (Diff revision 1) > > nsCOMPtr<nsIPrefBranch> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID); > > if (prefs) { > prefs->RemoveObserver(RESIST_FINGERPRINTING_PREF, this); > + prefs->RemoveObserver(RFP_TIMER_PREF, this); Same here for RFP_TIMER_VALUE_PREF.
Attachment #8935885 - Flags: review?(tihuang) → review-
Comment on attachment 8935886 [details] Bug 1424341 Add tests for privacy.reduceTimerPrecision that confirm the default timer resolution works https://reviewboard.mozilla.org/r/206750/#review212982 In general, this looks good to me. But, there are some trailing spaces. Could you move them out?
Attachment #8935886 - Flags: review?(tihuang) → review+
Comment on attachment 8935885 [details] Bug 1424341 Add privacy.reduceTimerPrecision and privacy.reduceTimerPrecision.microseconds prefs Dropping flags here until other r- is resolved.
Attachment #8935885 - Flags: review?(bkelly)
Attachment #8935886 - Flags: review?(bkelly)
Priority: -- → P2
Attachment #8935885 - Flags: review?(tihuang)
Attachment #8936535 - Flags: review?(tihuang)
Attachment #8936535 - Flags: review?(bkelly)
Attachment #8936536 - Flags: review?(tihuang)
Attachment #8936536 - Flags: review?(bkelly)
Okay, I set up rounding for the various performance APIs to be complete - there are a ton of those APIs. I *think* I cover all the performance APIs between the three tests in these patchsets - but it would be good if someone more familiar with the standardized (and non-standardized) APIs could confirm that. In particular I found the performance.timeOrigin API - that wasn't rounded before, and it doesn't seem to be controlled by a pref either; but I don't see it in Tor Browser. It might also cause a crash when accessed via a worker, I'm going to grab someone to help me debug that tomorrow. I think the only thing missing is tests for the dynamically tuned parameter.
Comment on attachment 8935885 [details] Bug 1424341 Add privacy.reduceTimerPrecision and privacy.reduceTimerPrecision.microseconds prefs https://reviewboard.mozilla.org/r/206748/#review213122 ::: browser/app/profile/firefox.js:1229 (Diff revision 2) > pref("services.sync.prefs.sync.privacy.sanitize.sanitizeOnShutdown", true); > pref("services.sync.prefs.sync.privacy.trackingprotection.enabled", true); > pref("services.sync.prefs.sync.privacy.trackingprotection.pbmode.enabled", true); > pref("services.sync.prefs.sync.privacy.resistFingerprinting", true); > +pref("services.sync.prefs.sync.privacy.reduceTimerPrecision", true); > +pref("services.sync.prefs.sync.privacy.reduceTimerPrecision.microseconds", true); I am not familiar with Sync, but I think you should change this as well.
Attachment #8935885 - Flags: review+
Comment on attachment 8935885 [details] Bug 1424341 Add privacy.reduceTimerPrecision and privacy.reduceTimerPrecision.microseconds prefs https://reviewboard.mozilla.org/r/206748/#review213230 r=me with comments addressed. ::: browser/app/profile/firefox.js:1229 (Diff revision 3) > pref("services.sync.prefs.sync.privacy.sanitize.sanitizeOnShutdown", true); > pref("services.sync.prefs.sync.privacy.trackingprotection.enabled", true); > pref("services.sync.prefs.sync.privacy.trackingprotection.pbmode.enabled", true); > pref("services.sync.prefs.sync.privacy.resistFingerprinting", true); > +pref("services.sync.prefs.sync.privacy.reduceTimerPrecision", true); > +pref("services.sync.prefs.sync.privacy.resistFingerprinting.reduceTimerPrecision.microseconds", true); I'm not familiar with these prefs. ::: modules/libpref/Preferences.cpp:5010 (Diff revision 3) > + bool); > + > +template nsresult > +Preferences::AddAtomicUintVarCache(Atomic<uint32_t, ReleaseAcquire>*, > + const char*, > + uint32_t); I don't really know anything about the preferences stuff, but this looks reasonable. ::: modules/libpref/init/all.js:1377 (Diff revision 3) > pref("privacy.firstparty.isolate.restrict_opener_access", true); > // Anti-fingerprinting, disabled by default > pref("privacy.resistFingerprinting", false); > +// A subset of Resist Fingerprinting protects for testing > +pref("privacy.reduceTimerPrecision", false); > +// Dynamically tune the resolution adjustment of the timer reduction for both of the two above prefs Please clarify what APIs this actually effects. When I first started looking at this I assumed it impacted setTimeout(), but it only seems to be changing values we report out of things like Date and performance API. ::: toolkit/components/resistfingerprinting/nsRFPService.h:38 (Diff revision 3) > { > return sPrivacyResistFingerprinting; > } > + static bool IsTimerPrecisionReductionEnabled() > + { > + return sPrivacyTimerPrecisionReduction; Should this be: ``` return sPrivacyTimerPrecisionReduction || IsResistFingerprintingEnabled(); ``` Since timers are applied implicitly with general RFP enabled it might be easier to return true when either value is set here. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:84 (Diff revision 3) > > /* static */ > double > nsRFPService::ReduceTimePrecisionAsMSecs(double aTime) > { > - if (!IsResistFingerprintingEnabled()) { > + if (!IsResistFingerprintingEnabled() && !IsTimerPrecisionReductionEnabled()) { If you make my suggested change to `IsTimerPrecisionReductionEnabled()` above, then this can just be: ``` if (!IsTimerPrecisionReductionEnabled()) { ``` ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:95 (Diff revision 3) > > /* static */ > double > nsRFPService::ReduceTimePrecisionAsUSecs(double aTime) > { > - if (!IsResistFingerprintingEnabled()) { > + if (!IsResistFingerprintingEnabled() && !IsTimerPrecisionReductionEnabled()) { And here. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:112 (Diff revision 3) > > /* static */ > double > nsRFPService::ReduceTimePrecisionAsSecs(double aTime) > { > - if (!IsResistFingerprintingEnabled()) { > + if (!IsResistFingerprintingEnabled() && !IsTimerPrecisionReductionEnabled()) { And here. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:289 (Diff revision 3) > > void > -nsRFPService::UpdatePref() > +nsRFPService::UpdateTimers() { > + MOZ_ASSERT(NS_IsMainThread()); > + > + // Timer stuff only nit: Please provide better comments or maybe just remove this comment. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:299 (Diff revision 3) > + } > +} > + > + > +void > +nsRFPService::UpdateRPPref() nit: Should this be `FRP` instead of `RP`? So `UpdateRFPPref()`? ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:304 (Diff revision 3) > +nsRFPService::UpdateRPPref() > { > MOZ_ASSERT(NS_IsMainThread()); > sPrivacyResistFingerprinting = Preferences::GetBool(RESIST_FINGERPRINTING_PREF); > > + // Rest of Resist Fingerprinting nit: better comments please ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:373 (Diff revision 3) > - if (pref.EqualsLiteral(RESIST_FINGERPRINTING_PREF)) { > - UpdatePref(); > + if (pref.EqualsLiteral(RFP_TIMER_PREF) || pref.EqualsLiteral(RFP_TIMER_VALUE_PREF)) { > + UpdateTimers(); > + } > + else if (pref.EqualsLiteral(RESIST_FINGERPRINTING_PREF)) { > + UpdateRPPref(); > + UpdateTimers(); Can you call `UpdateTimers()` directly from `UpdateRPPref()` instead? Requiring successive calls like this makes it easier to accidentally forget to call `UpdateTimers()` in the future.
Attachment #8935885 - Flags: review?(bkelly) → review+
Comment on attachment 8935886 [details] Bug 1424341 Add tests for privacy.reduceTimerPrecision that confirm the default timer resolution works https://reviewboard.mozilla.org/r/206750/#review213242 rs+
Attachment #8935886 - Flags: review?(bkelly) → review+
Comment on attachment 8936535 [details] Bug 1424341 Round the Performance Timing APIs when privacy.reduceTimerPrecision is set https://reviewboard.mozilla.org/r/207216/#review213244
Attachment #8936535 - Flags: review?(bkelly) → review+
Comment on attachment 8936536 [details] Bug 1424341 Add tests for privacy.reduceTimerPrecision for the performance API https://reviewboard.mozilla.org/r/207218/#review213248 rs+
Attachment #8936536 - Flags: review?(bkelly) → review+
Comment on attachment 8936535 [details] Bug 1424341 Round the Performance Timing APIs when privacy.reduceTimerPrecision is set https://reviewboard.mozilla.org/r/207216/#review213310 This looks good to me. But there are some implicit type conversions, either unsigned long long to double and vise versa. I believe the compiler will eventually handle this for us. But should we fix this? I really don't know.
Attachment #8936535 - Flags: review?(tihuang) → review+
Comment on attachment 8936536 [details] Bug 1424341 Add tests for privacy.reduceTimerPrecision for the performance API https://reviewboard.mozilla.org/r/207218/#review213276 ::: browser/components/resistfingerprinting/test/browser/browser_performanceAPI.js:121 (Diff revision 2) > // Try to add some entries. > content.performance.mark("Test"); > content.performance.mark("Test-End"); > content.performance.measure("Test-Measure", "Test", "Test-End"); > > // Check that no entries for performance.getEntries/getEntriesByType/getEntriesByName. The comment here seems wrong to me.
Attachment #8936536 - Flags: review?(tihuang) → review+
Comment on attachment 8936535 [details] Bug 1424341 Round the Performance Timing APIs when privacy.reduceTimerPrecision is set https://reviewboard.mozilla.org/r/207216/#review213310 Me neither. Adding in the ULL variants did not fix any floating point errors, so I suspect it is not really needed.
Attachment #8936939 - Flags: review?(tihuang)
Attachment #8936939 - Flags: review?(bkelly)
Okay, added tests for the dynamic tuning. We aren't controlling setTimeout/setInterval though, and maybe we want to handle those too; to be complete... That might break *everything* though, and they are limited to 4 ms I believe....
Comment on attachment 8936939 [details] Bug 1424341 Add tests for dynamically tuning the timer precision https://reviewboard.mozilla.org/r/207662/#review213630 ::: browser/components/resistfingerprinting/test/browser/browser_performanceAPI.js:103 (Diff revision 3) > let tab = await BrowserTestUtils.openNewForegroundTab( > gBrowser, TEST_PATH + "file_dummy.html"); > > - await ContentTask.spawn(tab.linkedBrowser, PERFORMANCE_TIMINGS, async function(list) { > - const isRounded = x => (Math.floor(x / 100) * 100) === x; > + let runTests = async function(data) { > + let timerlist = data.list, > + expectedPrecision = data.precision; Could you move these declarations into standalone ones? like ``` let timerlist = data.list; let expectedPrecision = data.precision; ``` ::: browser/components/resistfingerprinting/test/browser/browser_performanceAPI.js:110 (Diff revision 3) > + let rounded = (Math.floor(x / expectedPrecision) * expectedPrecision); > + // First we do the perfectly normal check that should work just fine > + if (rounded === x || x === 0) > + return true; > + > + // Then we do the annoying shit to handle floating points. Could you provide a better comment around this? ::: browser/components/resistfingerprinting/test/browser/file_workerPerformance.js:25 (Diff revision 3) > + if (Math.abs(rounded - x + expectedPrecision) < .0000001) { > + return true; > + } else if (Math.abs(rounded - x) < .0000001) { > + return true; > + } > + Why we don't have to deal with the sub-millisecond case here? I believe you do the same test for sub-millisecond for workers as well right? I can be wrong about this. ::: browser/components/resistfingerprinting/test/browser/file_workerPerformance.js:34 (Diff revision 3) > + > + return false; > +} > + > function runRPTests() { > - const isRounded = x => (Math.floor(x / 100) * 100) === x; > + // ok(isRounded(performance.timeOrigin, 100), `For resistFingerprinting, performance.timeOrigin is not correctly rounded: ` + performance.timeOrigin); Could you add some comments regarding why you comment this line out? ::: browser/components/resistfingerprinting/test/browser/file_workerPerformance.js:57 (Diff revision 3) > // Try to add some entries. > performance.mark("Test"); > performance.mark("Test-End"); > performance.measure("Test-Measure", "Test", "Test-End"); > > // Check that no entries for performance.getEntries/getEntriesByType/getEntriesByName. It looks like this comment should be fixed as well. ::: browser/components/resistfingerprinting/test/mochitest/file_animation_api.html:42 (Diff revision 3) > + > + // Then we do the annoying shit to handle floating points. > + if (Math.abs(rounded - x + expectedPrecision) < .0000001) { > + return true; > + } > + Same here for sub-millisecond checks. ::: browser/components/resistfingerprinting/test/mochitest/test_reduce_time_precision.html:265 (Diff revision 3) > + ["privacy.resistFingerprinting.reduceTimerPrecision.microseconds", expectedPrecision * 1000] > + ]}); > + checkTimestamps("privacy.resistFingerprinting", expectedPrecision); > }); > > add_task(async function testDOMRTP() { Nit: s/testDOMRTP/testDOMRTP1 ::: browser/components/resistfingerprinting/test/mochitest/test_reduce_time_precision.html:275 (Diff revision 3) > - let timeStampCodesDOM = timeStampCodes.concat([ > - "audioContext.currentTime * 1000", > - "canvasStream.currentTime * 1000", > - ]); > - // Loop through each timeStampCode, evaluate it, > - // and check if it is rounded to the nearest 100 ms. > + ["privacy.resistFingerprinting.reduceTimerPrecision.microseconds", expectedPrecision * 1000] > + ]}); > + checkTimestamps("privacy.reduceTimerPrecision", expectedPrecision); > + }); > + > + add_task(async function testDOMRTP() { Nit: s/testDOMRTP/testDOMRTP2 ::: browser/components/resistfingerprinting/test/mochitest/test_reduce_time_precision.html:285 (Diff revision 3) > - timeStampCode + > - "' should be rounded to nearest 100 ms; saw " + > - timeStamp); > - } > + ["privacy.resistFingerprinting.reduceTimerPrecision.microseconds", expectedPrecision * 1000] > + ]}); > + checkTimestamps("privacy.reduceTimerPrecision", expectedPrecision); > + }); > + > + add_task(async function testDOMRTP() { Nit: s/testDOMRTP/testDOMRTP3
Attachment #8936939 - Flags: review?(tihuang) → review+
Depends on: 1425509
Comment on attachment 8936939 [details] Bug 1424341 Add tests for dynamically tuning the timer precision https://reviewboard.mozilla.org/r/207662/#review213714 rs+
Attachment #8936939 - Flags: review?(bkelly) → review+
Comment on attachment 8938141 [details] Bug 1424341 Add tests for Atomic*ReleaseAcquire I am not the right person to review this, passing to njn.
Attachment #8938141 - Flags: review?(tihuang) → review?(n.nethercote)
Comment on attachment 8938141 [details] Bug 1424341 Add tests for Atomic*ReleaseAcquire https://reviewboard.mozilla.org/r/208856/#review214830 Looks reasonable to me, but I'll defer to njn as well since he has taken ownership of prefs stuff.
Attachment #8938141 - Flags: review?(bkelly) → review+
Comment on attachment 8935885 [details] Bug 1424341 Add privacy.reduceTimerPrecision and privacy.reduceTimerPrecision.microseconds prefs https://reviewboard.mozilla.org/r/206748/#review215018 ::: browser/app/profile/firefox.js:1228 (Diff revision 4) > pref("services.sync.prefs.sync.privacy.donottrackheader.enabled", true); > pref("services.sync.prefs.sync.privacy.sanitize.sanitizeOnShutdown", true); > pref("services.sync.prefs.sync.privacy.trackingprotection.enabled", true); > pref("services.sync.prefs.sync.privacy.trackingprotection.pbmode.enabled", true); > pref("services.sync.prefs.sync.privacy.resistFingerprinting", true); > +pref("services.sync.prefs.sync.privacy.reduceTimerPrecision", true); Are you sure these need to be synced? They are only set temporarily for testing purposes, right? ::: modules/libpref/Preferences.cpp:5008 (Diff revision 4) > +Preferences::AddAtomicBoolVarCache(Atomic<bool, ReleaseAcquire>*, > + const char*, > + bool); > + > +template nsresult > +Preferences::AddAtomicUintVarCache(Atomic<uint32_t, ReleaseAcquire>*, It would be good to have some explanation (in this bug, in a comment, somewhere) about why ReleaseAcquire semantics are needed for these prefs.
Comment on attachment 8938141 [details] Bug 1424341 Add tests for Atomic*ReleaseAcquire https://reviewboard.mozilla.org/r/208856/#review215020 This testing part is straightforward. I had some comments on the first patch, though.
Attachment #8938141 - Flags: review?(n.nethercote) → review+
[Tracking Requested - why for this release]: [Tracking Requested - why for this release]: (In reply to Nicholas Nethercote [:njn] (on PTO until Jan 29, 2018) from comment #45) > Comment on attachment 8935885 [details] > Bug 1424341 Add privacy.reduceTimerPrecision and > privacy.reduceTimerPrecision.microseconds prefs > > https://reviewboard.mozilla.org/r/206748/#review215018 > > ::: browser/app/profile/firefox.js:1228 > (Diff revision 4) > > pref("services.sync.prefs.sync.privacy.donottrackheader.enabled", true); > > pref("services.sync.prefs.sync.privacy.sanitize.sanitizeOnShutdown", true); > > pref("services.sync.prefs.sync.privacy.trackingprotection.enabled", true); > > pref("services.sync.prefs.sync.privacy.trackingprotection.pbmode.enabled", true); > > pref("services.sync.prefs.sync.privacy.resistFingerprinting", true); > > +pref("services.sync.prefs.sync.privacy.reduceTimerPrecision", true); > > Are you sure these need to be synced? They are only set temporarily for > testing purposes, right? These prefs may also be used as stopgap measures to mitigate the CPU speculation attacks. In that case, we would set the prefs globally, and would want to support users tweaking them. > ::: modules/libpref/Preferences.cpp:5008 > (Diff revision 4) > > +Preferences::AddAtomicBoolVarCache(Atomic<bool, ReleaseAcquire>*, > > + const char*, > > + bool); > > + > > +template nsresult > > +Preferences::AddAtomicUintVarCache(Atomic<uint32_t, ReleaseAcquire>*, > > It would be good to have some explanation (in this bug, in a comment, > somewhere) about why ReleaseAcquire semantics are needed for these prefs. I agree. I confess I cargo-culted ReleaseAcquire from the previous usages. Is ReleaseAcquire the correct semantic to be used here? I think we want to uplift this to 58. That uplift would encompass four patches as shown here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f878bd7455ea7db8c1d56253d8f5c4f9f65481f3 Do we want to uplift this to 52? That would be a _very_ considerable patch, so I think we don't want to take this particular patch there, and instead take Bug 1427870 only.
status-firefox58: --- → affected
status-firefox-esr52: --- → affected
tracking-firefox58: --- → ?
tracking-firefox-esr52: --- → ?
Flags: needinfo?(n.nethercote)
Track 58- as it's too late for 58 and we are only 1 week from RC week. Let's let it ride the train on 59.
status-firefox58: affected → wontfix
tracking-firefox58: ? → -
See Also: → 1427870
Priority: P2 → P1
Whiteboard: [fingerprinting] → [fingerprinting][fp-triaged]
Attachment #8938141 - Flags: review?(tihuang)
Clear and redirect review request back to njn. However, the flag is missing here. It can be seen on review board.
I'm rebasing these on top of Bug 1423495
Depends on: 1423495
Actually, after reviewing Server Timing (Bug 1423495) I think we should omit any rounding of these values. Server Timing is effectively a nice way for a server to tell a webpage "Oh yea it took me this amount of time to do these tasks" and the webpage to provide that information in a consistent interface. If a server is providing the time, and we decide to round it through this API, the server can easily circumvent that rounding by providing it in a less user-friendly manner. So a server that is cooperative with malicious javascript can circumvent our rounding. Additionally, rounding is intended to provide defense against attacks on the client - Spectre or Pixel Stealing or similar. These times are processing that occur on the server. There *is* a threat model where rounding these values makes sense though. An attacker is trying to learn secret data on a non-cooperative innocent server by submitting different requests and measuring the time it took to perform some non-constant-time operation. In that situation, rounding would make this attack harder. However, if the server is concerned about that - maybe don't send fine-grained timing information about your internal operations?
Attachment #8938141 - Flags: review?(tihuang)
[Tracking Requested - why for this release]: We may want to give ourselves the option of changing this in the future without having to make the changes at the C++ level so I'd like to see this in 58.
status-firefox58: wontfix → ?
tracking-firefox58: - → ?
Depends on: 1417431
Depends on: 1417741
Attached patch Beta backport of all commits (obsolete) — — Splinter Review
Attached is a backport for 58 that squashes all 6 commits together. Minor tweaks were needed, but nothing major.
Tracking for 59. If we landed today on 59, this could potentially make the 58 beta 16 build, or the 58 RC build next week. Is there any manual testing we could do in 59?
tracking-firefox59: --- → +
Attached patch Beta backport of all commits (obsolete) — — Splinter Review
Attachment #8941585 - Attachment is obsolete: true
Attached patch Beta backport of all commits (obsolete) — — Splinter Review
New backport adding one more test ignore
Attachment #8941600 - Attachment is obsolete: true
Removed Bug 1423495 as a dependency (it's not actually required logic-wise because we're not going to round it) and now the patches don't need it to apply cleanly.
No longer depends on: 1423495
Comment on attachment 8941590 [details] Bug 1424341 Turn the pref off for existing tests that perform fine-grained timing comparisons https://reviewboard.mozilla.org/r/211854/#review217674 ::: dom/animation/test/document-timeline/test_document-timeline.html:10 (Diff revision 3) > <div id="log"></div> > <script> > 'use strict'; > setup({explicit_done: true}); > SpecialPowers.pushPrefEnv( > - { "set": [["dom.animations-api.core.enabled", true]]}, > + { "set": [ Nit: Here and elsewhere, this indentation looks odd to me. I think that something like: ```js { "set": [["dom...", true], ["privacy...", false]] } ``` might be easier to read. ::: dom/media/test/test_streams_element_capture_createObjectURL.html:68 (Diff revision 3) > vout.play(); > }; > } > > +SpecialPowers.pushPrefEnv( > + { "set": [ Nit: this probably fits on a single line.
Attachment #8941590 - Flags: review+
Comment on attachment 8941590 [details] Bug 1424341 Turn the pref off for existing tests that perform fine-grained timing comparisons https://reviewboard.mozilla.org/r/211854/#review217678
Attachment #8941590 - Flags: review+
Okay, I think this is good to go for -central and -beta.
Keywords: checkin-needed
Blocks: 1429647
Blocks: 1429648
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/aea20cd2ae6a Add privacy.reduceTimerPrecision and privacy.reduceTimerPrecision.microseconds prefs r=bkelly,timhuang https://hg.mozilla.org/integration/autoland/rev/b1be9dcce773 Add tests for privacy.reduceTimerPrecision that confirm the default timer resolution works r=bkelly,timhuang https://hg.mozilla.org/integration/autoland/rev/99b6a31ece4d Round the Performance Timing APIs when privacy.reduceTimerPrecision is set r=bkelly,timhuang https://hg.mozilla.org/integration/autoland/rev/1382e00b5726 Add tests for privacy.reduceTimerPrecision for the performance API r=bkelly,timhuang https://hg.mozilla.org/integration/autoland/rev/ab399bd8412b Add tests for dynamically tuning the timer precision r=bkelly,timhuang https://hg.mozilla.org/integration/autoland/rev/4a6035d8a039 Add tests for Atomic*ReleaseAcquire r=bkelly,njn https://hg.mozilla.org/integration/autoland/rev/9fd027751d81 Turn the pref off for existing tests that perform fine-grained timing comparisons r=mrbkap
Keywords: checkin-needed
Comment on attachment 8941644 [details] [diff] [review] Beta backport of all commits [Feature/Bug causing the regression]: We would like the ability to easily tune timer precision on beta for the next release cycle to respond to Spectre. The pref is turned off by default right now. [User impact if declined]: We will have to do dot releases instead of doing a system addon to tweak a pref. Also, the dot releases will be more work (much more work if we don't uplift this in one of them) to cover all timers - previously our chemspill was only to address performance.now() [Is this code covered by automated tests?]: Yes (in fact it fixes a test and stops it from crashing.) [Has the fix been verified in Nightly?]: No, it landed yesterday [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: This is a dependency for 1424341 [Is the change risky?]: I don't think so, but it's not 'zero risk'. [Why is the change risky/not risky?]: It's a big patch, but most of it is tests and the pref is off by default. Even though the pref is off, we are adding function calls that run by default. Those calls have been tested in other locations considerably for months, but this patch adds them in a few new places that have not been tested. We also restructure some code that sets up and responds to observers, and some of that will be run regardless of the pref being off. [String changes made/needed]: None
Attachment #8941644 - Flags: approval-mozilla-beta?
Blocks: 1429885
Hi Tom, since ESR52 is marked as affected, could you please also nominate this for uplift to ESR52?
Flags: needinfo?(tom)
This won't be uplifted to 52. If we do anything in 52 it will be in another bug.
Flags: needinfo?(tom)
Comment on attachment 8941644 [details] [diff] [review] Beta backport of all commits Take this to response to Spectre issue. Beta58+.
Attachment #8941644 - Flags: approval-mozilla-release+
Attachment #8941644 - Flags: approval-mozilla-beta?
Attachment #8941644 - Flags: approval-mozilla-beta+
ESR is being discussed in Bug 1427870
Keywords: checkin-needed
Depends on: 1430975
Blocks: 1431425
Blocks: 1431455
Depends on: 1371906
Flags: needinfo?(n.nethercote)
Blocks: 1425462
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: