Allow independent and adjustable timer precision

RESOLVED FIXED in Firefox 58

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: tjr, Assigned: tjr)

Tracking

(Depends on 1 bug, Blocks 4 bugs)

Trunk
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52- wontfix, firefox57 wontfix, firefox58blocking fixed, firefox59+ fixed)

Details

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

Attachments

(8 attachments, 3 obsolete attachments)

59 bytes, text/x-review-board-request
bkelly
: review+
timhuang
: review+
Details
59 bytes, text/x-review-board-request
timhuang
: review+
bkelly
: review+
Details
59 bytes, text/x-review-board-request
bkelly
: review+
timhuang
: review+
Details
59 bytes, text/x-review-board-request
bkelly
: review+
timhuang
: review+
Details
59 bytes, text/x-review-board-request
bkelly
: review+
timhuang
: review+
Details
59 bytes, text/x-review-board-request
bkelly
: review+
Details
59 bytes, text/x-review-board-request
mrbkap
: review+
Details
81.50 KB, patch
Details | Diff | Splinter Review
Assignee

Description

2 years ago
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.
Assignee

Comment 1

2 years ago
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8935885 - Flags: review?(tihuang)
Attachment #8935886 - Flags: review?(tihuang)
Assignee

Updated

2 years ago
Attachment #8935885 - Flags: review?(tihuang)
Assignee

Comment 5

2 years ago
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.
Assignee

Updated

2 years ago
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 6

2 years ago
mozreview-review
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 7

2 years ago
mozreview-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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
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)
Assignee

Comment 13

2 years ago
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 14

2 years ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 19

2 years ago
mozreview-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 20

2 years ago
mozreview-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 21

2 years ago
mozreview-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 22

2 years ago
mozreview-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 23

2 years ago
mozreview-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 24

2 years ago
mozreview-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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 31

2 years ago
mozreview-review-reply
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.
Assignee

Updated

2 years ago
Attachment #8936939 - Flags: review?(tihuang)
Attachment #8936939 - Flags: review?(bkelly)
Assignee

Comment 32

2 years ago
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 hidden (mozreview-request)

Comment 34

2 years ago
mozreview-review
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+
Assignee

Updated

2 years ago
Depends on: 1425509
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 37

2 years ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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 44

2 years ago
mozreview-review
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 45

2 years ago
mozreview-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 46

2 years ago
mozreview-review
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+
Assignee

Comment 47

2 years ago
[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.
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.
See Also: → 1427870
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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.
Assignee

Comment 56

2 years ago
I'm rebasing these on top of Bug 1423495
Depends on: 1423495
Assignee

Comment 57

2 years ago
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?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Tracked as a blocking bug for 58.
Assignee

Updated

2 years ago
Depends on: 1417431
Assignee

Updated

2 years ago
Depends on: 1417741
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 74

2 years ago
Posted 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.
Comment hidden (mozreview-request)
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?
Assignee

Comment 78

2 years ago
Posted patch Beta backport of all commits (obsolete) — Splinter Review
Attachment #8941585 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 86

2 years ago
Posted patch Beta backport of all commits (obsolete) — Splinter Review
New backport adding one more test ignore
Attachment #8941600 - Attachment is obsolete: true
Assignee

Comment 87

2 years ago
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 hidden (mozreview-request)
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+
Assignee

Comment 92

2 years ago
Okay, I think this is good to go for -central and -beta.
Keywords: checkin-needed
Assignee

Updated

2 years ago
Blocks: 1429647
Assignee

Updated

2 years ago
Blocks: 1429648

Comment 93

2 years ago
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
Assignee

Comment 96

2 years ago
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?
Assignee

Updated

2 years ago
Blocks: 1429885
Hi Tom, since ESR52 is marked as affected, could you please also nominate this for uplift to ESR52?
Flags: needinfo?(tom)
Assignee

Comment 98

Last year
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+
Assignee

Comment 101

Last year
ESR is being discussed in Bug 1427870
Keywords: checkin-needed
Assignee

Updated

Last year
Depends on: 1430975
Assignee

Updated

Last year
Blocks: 1431425
Assignee

Updated

Last year
Blocks: 1431455
Assignee

Updated

Last year
Blocks: 1415303
Depends on: 1371906
Flags: needinfo?(n.nethercote)
Assignee

Updated

11 months ago
Blocks: 1425462
You need to log in before you can comment on or make changes to this bug.