To offer some protection against timing attacks by JS content pages, a patch was introduced in Tor Browser to round the various time-exposing APIs (such as Date and Event.timeStamps) to the nearest 100 ms or 250 ms. We would like to propose putting this behavior behind a pref and upstreaming it to Firefox. Here is the original ticket: https://trac.torproject.org/1517 And here is a link that tracks the current patch: https://torpat.ch/1517

You may also be interested in the discussion going on in this ticket, regarding various ways of obtaining high-precision timestamps from JS: https://github.com/lars-t-hansen/ecmascript_sharedmem/issues/1.

(In reply to Lars T Hansen [:lth] from comment #1) > You may also be interested in the discussion going on in this ticket, > regarding various ways of obtaining high-precision timestamps from JS: > https://github.com/lars-t-hansen/ecmascript_sharedmem/issues/1. Thank you for pointing this out. I have added a ticket here: https://trac.torproject.org/17412

Here's a WIP patch. It doesn't compile right now. The original Tor patch made changes to mozglue/misc/TimeStamp.h, but I can't read pref in there. If I tried to include Preference.h, I'd get this compilation error: http://searchfox.org/mozilla-central/source/modules/libpref/Preferences.h#10 > #error "This header is only usable from within libxul (MOZILLA_INTERNAL_API)." It makes sense because TimeStamp may be used by an application without prefs. We can of course try to change every caller in firefox, but then this patch will become so huge and affect too many code. I'm not sure if there's a better way to do this.

Hi Jonathan, Thank you for working on this. In case you haven't seen them, we recently have made some additional fixup patches in Tor Browser that change a few things and add protection for more places where ms-resolution time is leaked to content JS. We also added regression tests. You can see the latest patches here: https://torpat.ch/1517 Notice we also dropped the part of the patch in KeyboardEvent.h because it wasn't having any effect. In addition, there's an extra patch I posted recently for another timing leak: https://trac.torproject.org/19478

(In reply to Jonathan Hao [:jhao] from comment #3) > Created attachment 8764197 [details] [diff] [review] > Add a pref to reduce precision of time exposed by Javascript > > Here's a WIP patch. It doesn't compile right now. > > The original Tor patch made changes to mozglue/misc/TimeStamp.h, but I can't > read pref in there. If I tried to include Preference.h, I'd get this > compilation error: > > http://searchfox.org/mozilla-central/source/modules/libpref/Preferences.h#10 > > #error "This header is only usable from within libxul (MOZILLA_INTERNAL_API)." > > It makes sense because TimeStamp may be used by an application without > prefs. We can of course try to change every caller in firefox, but then this > patch will become so huge and affect too many code. I'm not sure if there's > a better way to do this. I think you could omit this part of the patch, and just focus on places where time is exposed in the content JS API. I think most of these are already covered by the Tor Browser patches. Probably the Performance API is one thing that still needs to be covered.

nsDOMNavigationTiming.h cannot include Preference.h either.

jsdate.cpp also can't include Preferences.h, so there's still one subtest in the regression test that fails TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_tor_bug1217238.html | 'new Date().getTime()' should be rounded to nearest 100 ms; saw 1467366512324 And I haven't decided what the pref name will be. I remember Dave said he wanted them all starting with "privacy." Also I'm a little confused with the minimum resolution used in this patch. Sometimes it's 100ms and sometimes it's 1ms or 10. Arthur, do you have any idea how we can work around js date time?

Also are we going to have a directory for all the tor regression tests?

(In reply to Jonathan Hao [:jhao] from comment #8) > Created attachment 8767115 [details] [diff] [review] > Add a pref to reduce precision of time exposed by Javascript Hi Jonathan -- thank you for your work on this. We recently landed a couple of further fixup patches in Tor Browser: https://gitweb.torproject.org/tor-browser.git/patch/?id=f2291c4 https://gitweb.torproject.org/tor-browser.git/patch/?id=0f60102 > jsdate.cpp also can't include Preferences.h, so there's still one subtest in > the regression test that fails > > TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_tor_bug1217238.html | > 'new Date().getTime()' should be rounded to nearest 100 ms; saw 1467366512324 > > And I haven't decided what the pref name will be. I remember Dave said he > wanted them all starting with "privacy." Please see my comment on this below. > Also I'm a little confused with the minimum resolution used in this patch. > Sometimes it's 100ms and sometimes it's 1ms or 10. The minimum resolution should always be the same (100 ms), in my view, so that probably means changes to the AnimationManager and nsRefreshDriver. I looked over your patch and didn't find any 10 ms cases, though -- maybe I am missing one. > Arthur, do you have any idea how we can work around js date time? Since Date() is exposed in both main thread contexts and workers, perhaps the best approach is to read the pref setting and then apply the value to a JSRuntime when that runtime is being initialized. I imagine we could introduce a JSRuntime::setReduceTimePrecision(bool) mechanism similar to .setNativeRegExp(bool)/nativeRegExp(): https://dxr.mozilla.org/mozilla-central/search?q=%22ativeRegExp%28%22&redirect=false That means we probably have to use a pref that begins with "javascript.options.", as you have done. How about "javascript.options.privacy.reduce_time_precision"? Then we can still search for it with the "privacy" keyword. > Also are we going to have a directory for all the tor regression tests? In general, when landing patches in mozilla-central, I have been placing tests in whatever existing directory seemed appropriate; I don't think a separate directory is necessary.

Dave and Arthur, could you take a look at these patches? I'm not sure if I should put the test in dom/media, or is there a better place? Also, do we care about command line js engine? My patch didn't take care of this: http://searchfox.org/mozilla-central/source/js/src/shell/js.cpp#7188 Thank you.

(In reply to Jonathan Hao [:jhao] from comment #12) > Created attachment 8767920 [details] [diff] [review] > Add a pref to reduce precision of time exposed by Javascript > > Dave and Arthur, could you take a look at these patches? From the Tor Browser point of view, I think these patches look good. > I'm not sure if I should put the test in dom/media, or is there a better > place? I don't know the answer to this one -- what is the usual Mozilla approach for a patch that overlaps several areas like this? > Also, do we care about command line js engine? My patch didn't take care of > this: http://searchfox.org/mozilla-central/source/js/src/shell/js.cpp#7188 I imagine it's better to include it so the behavior is consistent. > Thank you. Thanks for your efforts!

OK, much as I support privacy work and Tor, this is an indefensible security boundary. Essentially you run a loop in a background thread and check the status of the loop to determine sub-100ms timing. Or you just, you know. Ask a remote service what time it is. Meanwhile you've broken piles and piles of code that improve user experience. I don't mind the behavior behind a flag -- if Tor Browser wants to try to run this, sure. Just everyone involved should be aware this isn't operating on solid ground.

I added an command line js option and some comments. Dave, please take a look.

(In reply to Dan Kaminsky from comment #14) > OK, much as I support privacy work and Tor, this is an indefensible security > boundary. Essentially you run a loop in a background thread and check the > status of the loop to determine sub-100ms timing. Or you just, you know. > Ask a remote service what time it is. Meanwhile you've broken piles and > piles of code that improve user experience. > > I don't mind the behavior behind a flag -- if Tor Browser wants to try to > run this, sure. Just everyone involved should be aware this isn't operating > on solid ground. Thank you for this comment. The behavior is indeed behind a pref in this patch and is disabled by default. I'm not convinced that this patch has no value, though. I think you're unlikely to be able to match the precision of the JS API's built-in time values, either from a loop or from remote time signals. And in Tor Browser's experience, I am unaware of any complaints about a degraded UX because of this change. But I'm open to being shown otherwise. And I think we should look at more sophisticated approaches for making clock-based fingerprinting more difficult. At the Tor Project we would be interested in any suggestions (or criticisms), particularly if backed by careful research. Anyone is invited to open a ticket at trac.torproject.org.

tor browser trac that is also relevant to this bug: https://trac.torproject.org/projects/tor/ticket/16110

Comment on attachment 8768693 [details] [diff] [review] Add a pref to reduce precision of time exposed by Javascript Hi Eric. This is part of the Tor integration project. We're trying to uplift Tor browser's patches to mozilla-central (behind prefs), so that their effort on rebasing each year can be reduced. Could you review this patch, or point us to a suitable reviewer? Thank you.

Comment on attachment 8768693 [details] [diff] [review] Add a pref to reduce precision of time exposed by Javascript Review of attachment 8768693 [details] [diff] [review]: ----------------------------------------------------------------- From what I understand, requestAnimationFrame [1] callback can be used to forge a 16ms timer. Is there any reason to make granularity higher than 16ms? In a similar note, Web Worker can be used with SharedArrayBuffer & Atomics, to forge timers. [2] Also, I will recommend making a test case for the JS shell, such that we can make sure that we do not work-around/remove this code by (jitting?) accident. [1] https://developer.mozilla.org/en-US/docs/Web/API/window/requestAnimationFrame [2] https://groups.google.com/forum/#!topic/mozilla.dev.platform/HodzgJRbly8

bug 575230 looks like a duplicate of this issue, is that correct? If yes, we can close bug 575230.

Comment on attachment 8768693 [details] [diff] [review] Add a pref to reduce precision of time exposed by Javascript There are various issues in the dom parts of this that I would like to see addressed: 1) There's a lot of copy/paste. Commoning up some of these things would be a good idea. For example, maybe TimeStampToDOMHighRes should internally be clamping stuff in general, or we should have a helper that does TimeStampToDOMHighRes and then clamps. 2) There are various places that are now using Preferences::GetBool in code that can run on worker threads (blobs, Performance::RoundTime, events) 3) It seems like running with this setting can make things appear to be out of order (e.g. performance.now() will claim a time before the time when something should happen when the thing actually happens; most simply setTimeout(f, 10) will look like it fires sooner than 10ms from now). I'm not sure there's a good way to address this part, so maybe it's just something whoever sets this pref will have to live with. r- because #2 is just not OK at all and #1 could use some work as well.

This paper suggests merely rounding the time may not be enough. https://www.usenix.org/system/files/conference/usenixsecurity16/sec16_paper_kohlbrenner.pdf

Comment on attachment 8767919 [details] [diff] [review] Regression tests for reducing precision of time exposed by Javascript Review of attachment 8767919 [details] [diff] [review]: ----------------------------------------------------------------- Clearing this review. The addition of the pref still needs some work according to the review. I assume this patch doesn't make sense without this pref, therefore deferring the review until an updated patch.

Comment on attachment 8768693 [details] [diff] [review] Add a pref to reduce precision of time exposed by Javascript Haven't actually fixed the issues, just rebased.

Hi Boris and Hannes, please take a look at my revised patches. Thanks.

Comment on attachment 8819802 [details] Bug 1217238 - Propagate changes of javascript.options.resistFingerprinting to workers. https://reviewboard.mozilla.org/r/99458/#review99990 Instead of doing sync calls all over the place to the main thread from workers, and making things like getting .timeStamp on events much slower on main thread due to indirecting through runnables that get run immediately, it would be better to have workers handle this pref like they handle other ones: grab the value when the worker is constructed, broadcast changes in the (really rare) case the pref changes.

Comment on attachment 8819802 [details] Bug 1217238 - Propagate changes of javascript.options.resistFingerprinting to workers. Thanks, Boris. You're right.

I'm sorry this is being so laggy. At this point I won't get to it until Wednesday. :(

No worries, Boris. Happy holidays :)

Comment on attachment 8819802 [details] Bug 1217238 - Propagate changes of javascript.options.resistFingerprinting to workers. https://reviewboard.mozilla.org/r/99458/#review101940 ::: dom/events/Event.cpp:1075 (Diff revision 2) > > double > Event::TimeStamp() const > { > if (!sReturnHighResTimeStamp) { > - return static_cast<double>(mEvent->mTime); > + // Round to 100ms if javascript.options.privacy.reduce_time_precision is on. I'd really rather not have these comments scattered about that will become stale if we change the precision reducer to use some number other than 100ms. Please just take them out. ::: dom/events/Event.cpp:1112 (Diff revision 2) > workers::GetCurrentThreadWorkerPrivate(); > MOZ_ASSERT(workerPrivate); > > TimeDuration duration = > mEvent->mTimeStamp - workerPrivate->NowBaseTimeStamp(); > - return duration.ToMilliseconds(); > + return TimePrecisionReducer::ReduceAsMSecsIfPrefIsOn(duration.ToMilliseconds()); This is over 80 chars. ::: dom/html/HTMLMediaElement.cpp:2428 (Diff revision 2) > } > > return mDefaultPlaybackStartPosition; > } > > +double HTMLMediaElement::CurrentTime() const There are lots of internal uses of CurrentTime(). Are they all ok with the clamping? In particular, it's not clear to me whether seeking will still work correctly with this change. Please have someone familiar with the media code review this part. ::: dom/performance/Performance.cpp:249 (Diff revision 2) > // Round down to the nearest 5us, because if the timer is too accurate people > // can do nasty timing attacks with it. See similar code in the worker > // Performance implementation. > - const double maxResolutionMs = 0.005; > - return floor(aTime / maxResolutionMs) * maxResolutionMs; > + double maxResolutionMs = 0.005; > + // Round to 100ms if javascript.options.privacy.reduce_time_precision is on. > + return TimePrecisionReducer::ReduceAsMSecsIfPrefIsOn( This one is a bit weird, because we're now doing rounding twice. I wonder whether it makes sense to push this down into TimePrecisionReducer, with a Maybe<> arg. Followup is best for this part. ::: dom/security/TimePrecisionReducer.h:17 (Diff revision 2) > +// is on. > +class TimePrecisionReducer final : public mozilla::Runnable > +{ > +public: > + static void Init(); > + static double ReduceAsSecsIfPrefIsOn(double aTime); I don't know that IfPrefIsOn is all that useful, especially if we ever end up with a more complicated strategy for reduction. Probably better to remove that suffix, and just document that these methods all return their arg if we're not trying to do precision reduction. ::: dom/security/TimePrecisionReducer.cpp:31 (Diff revision 2) > + > +NS_IMETHODIMP > +TimePrecisionReducer::Run() > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + return mozilla::Preferences::AddBoolVarCache( This AddBoolVarCache can happen multiple times, right? That doesn't seem great. It's also not clear to me that it's safe to racily read sIsPrefOn from non-main threads while the main thread is mutating it. Again, it would be better if we used our normal worker setup for prefs. That would also avoid having to sprinkle TimePrecisionReducer::Init() calls all over.... ::: dom/workers/RuntimeService.cpp:309 (Diff revision 2) > .setThrowOnAsmJSValidationFailure(GetWorkerPref<bool>( > NS_LITERAL_CSTRING("throw_on_asmjs_validation_failure"))) > .setBaseline(GetWorkerPref<bool>(NS_LITERAL_CSTRING("baselinejit"))) > .setIon(GetWorkerPref<bool>(NS_LITERAL_CSTRING("ion"))) > .setNativeRegExp(GetWorkerPref<bool>(NS_LITERAL_CSTRING("native_regexp"))) > + .setReduceTimePrecision(GetWorkerPref<bool>(NS_LITERAL_CSTRING("privacy.reduce_time_precision"))) We're not observing this pref, so changing it won't affect already-running workers, right? ::: js/xpconnect/src/XPCJSContext.cpp:1444 (Diff revision 2) > bool useWasmBaseline = Preferences::GetBool(JS_OPTIONS_DOT_STR "wasm_baselinejit") && !safeMode; > bool throwOnAsmJSValidationFailure = Preferences::GetBool(JS_OPTIONS_DOT_STR > "throw_on_asmjs_validation_failure"); > bool useNativeRegExp = Preferences::GetBool(JS_OPTIONS_DOT_STR "native_regexp") && !safeMode; > > + bool reduceTimePrecision = Preferences::GetBool(JS_OPTIONS_DOT_STR "privacy.reduce_time_precision"); Do we register to observe this pref? I don't think we do, so if it changes we won't reset it on the JSContext? ::: layout/base/nsRefreshDriver.cpp:1704 (Diff revision 2) > if (innerWindow && innerWindow->IsInnerWindow()) { > mozilla::dom::Performance* perf = innerWindow->GetPerformance(); > if (perf) { > - timeStamp = perf->GetDOMTiming()->TimeStampToDOMHighRes(aNowTime); > + // Round to 100ms if javascript.options.privacy.reduce_time_precision > + // is on. > + timeStamp = TimePrecisionReducer::ReduceAsMSecsIfPrefIsOn( I don't think this makes any sense. Why would we want to make this change here?

Comment on attachment 8819802 [details] Bug 1217238 - Propagate changes of javascript.options.resistFingerprinting to workers. https://reviewboard.mozilla.org/r/99458/#review108206 Removing review. Boris already did a review and he is more knowledgable about this.

Hi Boris, Thanks for your review again. About the "worker pref setup" you mentioned, do you mean like adding a WORKER_SIMPLE_PREF to WorkerPref.h? So should I implement similar setups to Performance, blob, and event?

Yes, I think that's the setup I was thinking of.

I have modified the patches to use the worker setup and added tests for nested workers. I'll ask Boris for review when he starts accepting them.

That might be several weeks. I'll try to get to this before then. ;)

OK, I'm accepting reviews now... Which bits are ready for review?

Hi Boris, Could you review all three patches? One patch sets up the pref propagation along the worker tree. Another rounds up the timing APIs. The other is the test.

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #36) > Comment on attachment 8819802 [details] > Bug 1217238 - Propagate changes of javascript.options.reduce_time_precision > to workers. > > https://reviewboard.mozilla.org/r/99458/#review101940 > > ::: dom/html/HTMLMediaElement.cpp:2428 > (Diff revision 2) > > } > > > > return mDefaultPlaybackStartPosition; > > } > > > > +double HTMLMediaElement::CurrentTime() const > > There are lots of internal uses of CurrentTime(). Are they all ok with the > clamping? In particular, it's not clear to me whether seeking will still > work correctly with this change. Please have someone familiar with the > media code review this part. Hi JW, Could you review the media part? I can explain this to you in person after the tomb-sweeping holidays.

Comment on attachment 8848034 [details] Bug 1217238 - Reduce time precision when privacy.resistFingerprinting is on. https://reviewboard.mozilla.org/r/120998/#review129184 ::: dom/html/HTMLMediaElement.cpp:2540 (Diff revision 1) > } > > +double > +HTMLMediaElement::CurrentTime() const > +{ > + return ReduceTimePrecisionAsSecs(CurrentTimeImpl()); I don't think you need to change HTMLMediaElement. The currentTime attribute is updated about every 40ms so I don't think it is feasible to use it for timing attacks.

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #48) > Comment on attachment 8848034 [details] > Bug 1217238 - Reduce time precision when > javascript.options.privacy.reduce_time_precision is on. > > https://reviewboard.mozilla.org/r/120998/#review129184 > > ::: dom/html/HTMLMediaElement.cpp:2540 > (Diff revision 1) > > } > > > > +double > > +HTMLMediaElement::CurrentTime() const > > +{ > > + return ReduceTimePrecisionAsSecs(CurrentTimeImpl()); > > I don't think you need to change HTMLMediaElement. The currentTime attribute > is updated about every 40ms so I don't think it is feasible to use it for > timing attacks. Thanks, JW. Hi Arthur, do you think it's okay if we don't change HTMLMediaElement?

(In reply to Jonathan Hao [:jhao] from comment #49) > Hi Arthur, do you think it's okay if we don't change HTMLMediaElement? I think it's fine as long as there is no way for content to circumvent the 40-ms lower limit. JW, could you point us to where in the code 40 ms update timing comes from? Thanks.

http://searchfox.org/mozilla-central/rev/b8cce081200129a1307e2a682f121135b5ca1d19/dom/media/MediaDecoderStateMachine.cpp#3542 AUDIO_DURATION_USECS is a hard coded constant set to 40000.

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #51) > http://searchfox.org/mozilla-central/rev/ > b8cce081200129a1307e2a682f121135b5ca1d19/dom/media/MediaDecoderStateMachine. > cpp#3542 > > AUDIO_DURATION_USECS is a hard coded constant set to 40000. Thanks! Looks reasonable to me that we leave HTMLMediaElement as it is.

I removed the changes to HTMLMediaElement.

And the test cases as well.

Change the pref name to javascript.options.privacy.resistFingerprinting as it could be used by other anti-fingerprinting features related to javascript context. Also, this matches the existing privacy.resistFingerprinting pref name.

Hello Boris, Would you like to delegate the review to someone else if you're too busy? Thanks.

I'm sorry for the lag here. :( Going to try handing this off to Michael.

Comment on attachment 8819801 [details] Bug 1217238 - Regression tests for reducing precision of time exposed by Javascript. https://reviewboard.mozilla.org/r/99456/#review143142 ::: dom/security/test/general/test_reduce_time_precision.html:58 (Diff revision 4) > + worker.removeEventListener("message", onMessage); > + > + let timeStamps = event.data; > + for (let i = 0; i < timeStampCodes.length; i++) { > + let timeStamp = timeStamps[i]; > + let roundedTimeStamp = 100*Math.round(timeStamp/100); Can you pull the expected resolution out into a constant? ::: dom/security/test/general/test_reduce_time_precision.html:81 (Diff revision 4) > + let worker1 = new Worker("worker_child.js"); > + yield SpecialPowers.pushPrefEnv({ > + "set": [["javascript.options.privacy.resistFingerprinting", true]]}); > + let worker2 = new Worker("worker_child.js"); I'm guessing the purpose of creating one worker before setting the pref, and one after, is to check that the resolution is updated whether or not the worker was already started, rather than to test the behavior both with the pref set and without the pref set. Could you add a comment clarifying that? ::: dom/security/test/general/test_reduce_time_precision.html:87 (Diff revision 4) > + yield SpecialPowers.pushPrefEnv({ > + "set": [["javascript.options.privacy.resistFingerprinting", true]]}); > + let worker2 = new Worker("worker_child.js"); > + // Allow ~500 ms to elapse, so we can get non-zero > + // time values for all elements. > + yield later(500); Try delaying execution for an amount of time which is not a multiple of your expected resolution to reduce the chance that the unrounded number just happens to be the same as the rounded one. ::: dom/security/test/general/test_reduce_time_precision.html:101 (Diff revision 4) > + ]); > + // Loop through each timeStampCode, evaluate it, > + // and check if it is rounded to the nearest 100 ms. > + for (let timeStampCode of timeStampCodes) { > + let timeStamp = eval(timeStampCode); > + let roundedTimeStamp = 100*Math.round(timeStamp/100); Again, please re-use the constant which you define in checkWorker(). It might also be worthwhile to just do this check in a shared function. ::: dom/security/test/general/test_reduce_time_precision.html:102 (Diff revision 4) > + // Loop through each timeStampCode, evaluate it, > + // and check if it is rounded to the nearest 100 ms. > + for (let timeStampCode of timeStampCodes) { > + let timeStamp = eval(timeStampCode); > + let roundedTimeStamp = 100*Math.round(timeStamp/100); > + is(timeStamp, roundedTimeStamp, Why not just `ok(timeStamp % kExpectedResolution == 0)`? ::: dom/security/test/general/worker_child.js:21 (Diff revision 4) > + worker.postMessage(timeStampCodes); > +} > + > +// The worker grandchild will send results back. > +function listenToChild(event) { > + self.removeEventListener("message", listenToChild); This listener was added to worker below, not self.

Comment on attachment 8819802 [details] Bug 1217238 - Propagate changes of javascript.options.resistFingerprinting to workers. https://reviewboard.mozilla.org/r/99458/#review143166 ::: browser/app/profile/firefox.js:548 (Diff revision 5) > pref("privacy.panicButton.enabled", true); > > pref("privacy.firstparty.isolate", false); > pref("privacy.firstparty.isolate.restrict_opener_access", true); > pref("privacy.resistFingerprinting", false); > +pref("javascript.options.privacy.resistFingerprinting", false); Why are you adding the javascript.options.privacy.resistFingerprinting pref, rather than just re-using the privacy.resistFingerprinting pref. This particular use of the name "resistFingerprinting" doesn't seem particularially more js-related? ::: dom/workers/RuntimeService.cpp:1327 (Diff revision 5) > + bool resistFingerprinting = mozilla::Preferences::GetBool( > + "javascript.options.privacy.resistFingerprinting"); If you change this to just use privacy.resistFingerprinting, then you can use the method nsContentUtils::ShouldResistFingerprinting. If you don't change it, please add a BoolVarCache to nsContentUtils and add another method for fetching this pref value. ::: dom/workers/WorkerPrefs.h:49 (Diff revision 5) > WORKER_SIMPLE_PREF("dom.fetchObserver.enabled", FetchObserverEnabled, FETCHOBSERVER_ENABLED) > WORKER_PREF("intl.accept_languages", PrefLanguagesChanged) > WORKER_PREF("general.appname.override", AppNameOverrideChanged) > WORKER_PREF("general.appversion.override", AppVersionOverrideChanged) > WORKER_PREF("general.platform.override", PlatformOverrideChanged) > +WORKER_PREF("javascript.options.privacy.resistFingerprinting", ResistFingerprintingChanged) Why can't you use WORKER_SIMPLE_PREF here? If you use that then you don't need a bunch of these special methods to propagate this simple boolean around, and you could just poke it with ResistFingerprintingEnabled on the WorkerPrivate instance. My immediate impression is that this is due to the JS Context needing to be told about whether or not to resist fingerprinting? ::: js/src/shell/js.cpp:7942 (Diff revision 5) > enableIon = !op.getBoolOption("no-ion"); > enableAsmJS = !op.getBoolOption("no-asmjs"); > enableWasm = !op.getBoolOption("no-wasm"); > enableNativeRegExp = !op.getBoolOption("no-native-regexp"); > + enableResistFingerprinting = > + !op.getBoolOption("resist-fingerprinting"); This is negated of what you want, you're enabling resist fingerprinting when resist-fingerprinting is not sent.

Comment on attachment 8848034 [details] Bug 1217238 - Reduce time precision when privacy.resistFingerprinting is on. https://reviewboard.mozilla.org/r/120998/#review143160 ::: dom/security/TimePrecision.h:12 (Diff revision 3) > +double ReduceTimePrecisionAsMSecs(double aTime); > +double ReduceTimePrecisionAsUSecs(double aTime); > +double ReduceTimePrecisionAsSecs(double aTime); Please add documentation comments to these methods explaining that they limit time precision in order to help resist fingerprinting, and that when javascript.options.privacy.resistFingerprinting is not on, they are no-ops. ::: dom/security/TimePrecision.cpp:13 (Diff revision 3) > + > +namespace mozilla { > +namespace dom { > + > +static bool > +IsPrefOn() Please change this name to be more clear. I would prefer a name like "ResistFingerprintingPrefOn" ::: dom/security/TimePrecision.cpp:17 (Diff revision 3) > +static bool > +IsPrefOn() > +{ > + if (NS_IsMainThread()) { > + return > + Preferences::GetBool("javascript.options.privacy.resistFingerprinting"); Please use a BoolVarCache for this instead of going through Preferences every time we need to round a timestamp. ::: dom/security/TimePrecision.cpp:20 (Diff revision 3) > + workers::WorkerPrivate* workerPrivate = > + workers::GetCurrentThreadWorkerPrivate(); > + MOZ_ASSERT(workerPrivate); > + return workerPrivate->ResistFingerprinting(); I'd prefer it if we didn't ASSERT workerPrivate here. I could imagine one of these methods ending up being called internally off of a worker or main thread. We should fall back in that case to not rounding (a.k.a false) IMO. ::: dom/security/TimePrecision.cpp:33 (Diff revision 3) > +{ > + if (!IsPrefOn()) { > + return aTime; > + } > + // Reduce precision to 100 msecs. > + return floor(aTime / 100.0) * 100.0; Please pull the resolution callback out into a constant here and below. ::: js/src/jsdate.cpp:1255 (Diff revision 3) > static ClippedTime > -NowAsMillis() > +NowAsMillis(JSContext* cx) > { > - return TimeClip(static_cast<double>(PRMJ_Now()) / PRMJ_USEC_PER_MSEC); > + double now = static_cast<double>(PRMJ_Now()) / PRMJ_USEC_PER_MSEC; > + if (cx->options().resistFingerprinting()) { > + return TimeClip(floor(now / 100.0) * 100.0); Don't hard-code this number around the code base. Please pull this number into a constant which is accessible somwehere, and use that constant.

Comment on attachment 8819802 [details] Bug 1217238 - Propagate changes of javascript.options.resistFingerprinting to workers. https://reviewboard.mozilla.org/r/99458/#review143166 > If you change this to just use privacy.resistFingerprinting, then you can use the method nsContentUtils::ShouldResistFingerprinting. > > If you don't change it, please add a BoolVarCache to nsContentUtils and add another method for fetching this pref value. I tried using nsContentUtils::ShouldResistFingerprinting() here, but it doesn't return the latest pref value. It seems the reason is that this ResistFingerprintingChanged is also observing the pref change and is triggered even sooner that the one in nsRFPService(). So when the pref is changed, the nsRFPService hasn't updated its static variable. Michael, what do you think? > Why can't you use WORKER_SIMPLE_PREF here? If you use that then you don't need a bunch of these special methods to propagate this simple boolean around, and you could just poke it with ResistFingerprintingEnabled on the WorkerPrivate instance. > > My immediate impression is that this is due to the JS Context needing to be told about whether or not to resist fingerprinting? Yes, because I need a special path to call WorkerPrivate::UpdateResistFingerprintingInternal and do this: JS::ContextOptionsRef(mJSContext).setResistFingerprinting(aResistFingerprinting); I'm not sure if there's better way to update the JSContext

Comment on attachment 8848034 [details] Bug 1217238 - Reduce time precision when privacy.resistFingerprinting is on. https://reviewboard.mozilla.org/r/120998/#review143160 > I'd prefer it if we didn't ASSERT workerPrivate here. I could imagine one of these methods ending up being called internally off of a worker or main thread. > > We should fall back in that case to not rounding (a.k.a false) IMO. Arthur, are you OK about that?

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #63) > I'm sorry for the lag here. :( Going to try handing this off to Michael. Boris, don't worry and thanks for finding another reviewer for me. Michael, I addressed your comments, except for some questions above. Could you take a look again? Thanks.

(In reply to Jonathan Hao [:jhao] from comment #67) > I tried using nsContentUtils::ShouldResistFingerprinting() here, but it > doesn't return the latest pref value. > > It seems the reason is that this ResistFingerprintingChanged is also > observing the pref change and is triggered even sooner that the one in > nsRFPService(). So when the pref is changed, the nsRFPService hasn't > updated its static variable. > > Michael, what do you think? That makes sense. I think this problem should go away if we get to just directly use the pref where we need it instead of having to go through all of this custom pref propagation code to workers. > > Why can't you use WORKER_SIMPLE_PREF here? If you use that then you don't need a bunch of these special methods to propagate this simple boolean around, and you could just poke it with ResistFingerprintingEnabled on the WorkerPrivate instance. > > > > My immediate impression is that this is due to the JS Context needing to be told about whether or not to resist fingerprinting? > > Yes, because I need a special path to call > WorkerPrivate::UpdateResistFingerprintingInternal and do this: > JS::ContextOptionsRef(mJSContext). > setResistFingerprinting(aResistFingerprinting); > > I'm not sure if there's better way to update the JSContext jorendorff, do you know if there's a better way to update the JSContext workers than this? It would be nice to avoid this ugly propagation code and instead rely on an existing system for propagating pref changes into all workers' JS contexts. Basically we need to be able to read a boolean controlled by a pref in Gecko ("should I limit time resolution a.k.a. resistFingerprinting") in the code for Date.

Comment on attachment 8819801 [details] Bug 1217238 - Regression tests for reducing precision of time exposed by Javascript. https://reviewboard.mozilla.org/r/99456/#review143518 ::: dom/security/test/general/test_reduce_time_precision.html:32 (Diff revision 5) > + // __later(delay)__. > + // Return a promise that resolves after the requested delay in ms. > + let later = function (delay) { > + return new Promise((resolve, reject) => { > + window.setTimeout(resolve, delay); > + }); > + }; This is only used in testWorker. Why not use yield new Promise(resolve => setTimeout(resolve, 550)); at the callsite instead of writing this extra function? ::: dom/security/test/general/test_reduce_time_precision.html:97 (Diff revision 5) > + timeStampCodes = timeStampCodes.concat([ > + 'audioContext.currentTime * 1000', > + 'canvasStream.currentTime * 1000', > + ]); Can you define a local one here, rather than overwriting the global, and make the global a `const`? It seems weird to me that we're mutating it.

Comment on attachment 8819802 [details] Bug 1217238 - Propagate changes of javascript.options.resistFingerprinting to workers. https://reviewboard.mozilla.org/r/99458/#review143574 I'm going to hold off on reviewing this patch too deeply until I get a reply from Jason. ::: js/xpconnect/src/XPCJSRuntime.cpp:3018 (Diff revision 6) > + Preferences::RegisterPrefixCallback(ReloadPrefsCallback, > + "privacy.resistFingerprinting"); You should only need a callback here, and not a prefix callback. This is the full path to the pref you're interested in, not the prefix.

Comment on attachment 8848034 [details] Bug 1217238 - Reduce time precision when privacy.resistFingerprinting is on. https://reviewboard.mozilla.org/r/120998/#review143580 Generally looks good to me, but holding r- for now while we see if we can change part 2 (which might change this patch a decent amount). ::: dom/security/TimePrecision.cpp:41 (Diff revision 4) > +} > + > +double > +ReduceTimePrecisionAsUSecs(double aTime) > +{ > + return ReduceTimePrecisionAsMSecs(aTime / 1000) * 1000; It would be nicer to perform the scaling on kResolutionMs, as that number is guaranteed to not be clipped by this math, while aTime is not a constant. ::: js/src/jsdate.cpp:112 (Diff revision 4) > * hashCode > */ > > +// This is used to limit the time precision when privacy.resistFingerprinting > +// is on. > +const double kResolutionMs = 100; I'd prefer it if we could find a common file which we could put this in so we don't duplicate this constant between TimePrecision.cpp and here. If you can't find an appropriate file, can you add a comment both here and in TimePrecision.cpp that this constant is duplicated in the other file, and to update it there if you ever update it here?

(In reply to Jonathan Hao [:jhao] from comment #68) > Comment on attachment 8848034 [details] > Bug 1217238 - Reduce time precision when > javascript.options.privacy.resistFingerprinting is on. > > https://reviewboard.mozilla.org/r/120998/#review143160 > > > I'd prefer it if we didn't ASSERT workerPrivate here. I could imagine one of these methods ending up being called internally off of a worker or main thread. > > > > We should fall back in that case to not rounding (a.k.a false) IMO. > > Arthur, are you OK about that? That seems OK, probably. Is there any practical way to explicitly check that it is an internal (chrome) call and not something exposed to content?

Comment on attachment 8819802 [details] Bug 1217238 - Propagate changes of javascript.options.resistFingerprinting to workers. Clearing review until jorendorff gets back to us.

Probably a silly question: Is anyone aware of the removal of `dom.enable_user_timing` in Bug 1344669 (also Tor ticket https://trac.torproject.org/projects/tor/ticket/16336), and if not, is it a concern?

Comment on attachment 8819802 [details] Bug 1217238 - Propagate changes of javascript.options.resistFingerprinting to workers. https://reviewboard.mozilla.org/r/99458/#review147090 Please correct the commit message (`javascript.options.resistFingerprinting` is wrong), and remove the custom code for propagating the value to workers. I talked to :sfink about what the best option would be here on the JS side to avoid all of this gross code. He suggested adding a method to js/Public/Date.h to set the Date time resolution. Internally within the JS engine, this method would set an `Atomic<float>` (or a similar type) to the resolution, and that atomic would be read by the CurrentTimeMillis code to perform scaling. We'd probably want the Atomic to have a Relaxed ordering, as we don't really care about ordering for this pref, and that way we can avoid any unnecessary overhead. This should allow us to use the simpler pref macro for workers on the DOM side, which should simplify this patch and the next patch a lot.

(In reply to Simon Mainey from comment #82) > Probably a silly question: Is anyone aware of the removal of > `dom.enable_user_timing` in Bug 1344669 (also Tor ticket > https://trac.torproject.org/projects/tor/ticket/16336), and if not, is it a > concern? Yes, we're aware of that. Thank you.

(In reply to Michael Layzell [:mystor] from comment #83) > Comment on attachment 8819802 [details] > Bug 1217238 - Propagate changes of javascript.options.resistFingerprinting > to workers. > > https://reviewboard.mozilla.org/r/99458/#review147090 > > Please correct the commit message (`javascript.options.resistFingerprinting` > is wrong), and remove the custom code for propagating the value to workers. > > I talked to :sfink about what the best option would be here on the JS side > to avoid all of this gross code. He suggested adding a method to > js/Public/Date.h to set the Date time resolution. Internally within the JS > engine, this method would set an `Atomic<float>` (or a similar type) to the > resolution, and that atomic would be read by the CurrentTimeMillis code to > perform scaling. We'd probably want the Atomic to have a Relaxed ordering, > as we don't really care about ordering for this pref, and that way we can > avoid any unnecessary overhead. > > This should allow us to use the simpler pref macro for workers on the DOM > side, which should simplify this patch and the next patch a lot. Thanks Michael. In that case I think we could just use the nsContentUtils::ShouldResistFingerprinting(). It seems that our atomic can't hold a float, so I let it hold an uint32_t. Is that OK?

Comment on attachment 8848034 [details] Bug 1217238 - Reduce time precision when privacy.resistFingerprinting is on. https://reviewboard.mozilla.org/r/120998/#review149720 ::: dom/security/TimePrecision.h:14 (Diff revision 8) > +double ReduceTimePrecisionAsMSecs(double aTime); > +double ReduceTimePrecisionAsUSecs(double aTime); > +double ReduceTimePrecisionAsSecs(double aTime); Please make these methods static methods on nsRFPService, and share the constant with the constant on nsRFPService. In nsRFPService, we can then just check the `IsResistFingerprintingEnabled` method. ::: dom/security/TimePrecision.cpp:17 (Diff revision 8) > +const double kResolutionMSec = 100; > + > +double > +ReduceTimePrecisionAsMSecs(double aTime) > +{ > + if (!nsContentUtils::ShouldResistFingerprinting()) { This is not a threadsafe method. When moving these methods onto nsRFPService, make sPrivacyResistFingerprinting an atomic like in the JS engine, and add comments explaining that the Reduce methods can be called off main thread. When making sPrivacyResistFingerprinting an atomic, you'll want to change `nsRFPService::UpdatePref` and add an `MOZ_ASSERT(NS_IsMainThread())` to it. ::: js/src/jsdate.cpp:65 (Diff revision 8) > using JS::ClippedTime; > using JS::GenericNaN; > using JS::TimeClip; > using JS::ToInteger; > > +static Atomic<uint32_t> sResolutionUsec; Add a comment to this static. In addition, we can almost certainly relax the ordering requirements for this atomic (pass a second template parameter). I would probably use `ReleaseAcquire`, as I think it has pretty much no overhead on the reading side. ::: js/src/jsdate.cpp:115 (Diff revision 8) > +// This is used to limit the time precision when privacy.resistFingerprinting > +// is on. > + Remove this now-unused comment ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:35 (Diff revision 8) > NS_IMPL_ISUPPORTS(nsRFPService, nsIObserver) > > static StaticRefPtr<nsRFPService> sRFPService; > static bool sInitialized = false; > bool nsRFPService::sPrivacyResistFingerprinting = false; > +uint32_t kResolutionUSec = 100000; Please use this constant for all resolution calculations when the TimeResolution methods are moved onto nsRFPService. In addition, this should be `static`.

Comment on attachment 8848034 [details] Bug 1217238 - Reduce time precision when privacy.resistFingerprinting is on. https://reviewboard.mozilla.org/r/120998/#review150740 Thanks :)

(In reply to Michael Layzell [:mystor] from comment #93) > Comment on attachment 8848034 [details] > Bug 1217238 - Reduce time precision when privacy.resistFingerprinting is on. > > https://reviewboard.mozilla.org/r/120998/#review150740 > > Thanks :) Thank you so much!

Pushed by jhao@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e967714a341a Regression tests for reducing precision of time exposed by Javascript. r=mystor https://hg.mozilla.org/integration/autoland/rev/1a7356fac9ba Reduce time precision when privacy.resistFingerprinting is on. r=mystor

https://hg.mozilla.org/mozilla-central/rev/e967714a341a https://hg.mozilla.org/mozilla-central/rev/1a7356fac9ba

Nice to see this bug finally being resolved. Good job, Jonathan! Thanks for all the reviewers. :)

(In reply to Jonathan Hao (inactive) [:jhao] from comment #84) > (In reply to Simon Mainey from comment #82) > > Probably a silly question: Is anyone aware of the removal of > > `dom.enable_user_timing` in Bug 1344669 (also Tor ticket > > https://trac.torproject.org/projects/tor/ticket/16336), and if not, is it a > > concern? > > Yes, we're aware of that. Thank you. May I ask how you covered the issue raised in the linked Tor ticket about the User Timing API allowing to store names for timers in some unspecified scope ? Quoting the Tor ticket: > This API also allows content to store names for timers and timestamps (in what scope? who knows.. the ​privacy section of the W3C spec basically just takes a **** on any privacy concerns) I agree that the timing imprecision being added in this bug probably addresses most concerns, but what about this name + scope thing ?

https://hg.mozilla.org/mozilla-central/rev/e967714a341a#l4.25 > +<script type="application/javascript;version=1.7"> Please do not add new versioned JavaScript. See bug 1342144 for details. Fortunately, this patch does not actually use versioned script features (legacy generators). So it is sufficient to remove the version parameter.

Jonathan is inactive now. Needinfo myself to remind me to deal with this issue later.

(In reply to Lars T Hansen [:lth] from comment #1) > You may also be interested in the discussion going on in this ticket, > regarding various ways of obtaining high-precision timestamps from JS: > https://github.com/lars-t-hansen/ecmascript_sharedmem/issues/1. This link doesn't seem to work anymore. Lars, is there any other place I could get this information from? It seems to be working fine with Date.now() and performance.now(), but are there any other I should check? I tested this on Mac OS with Nightly 58.0a1 (2017-10-20) (64-bit) Here are my steps: 1. Open a new tab, and open Web Developers tools 2. In Console tab, enter: Date.now() 3. Repeat the above step multiple times and save results 4. Go to about:config and turn on privacy.resistFingerprinting 5. In Console tab, enter: Date.now() 6. Repeat the above step multiple times and save results Expected result: With privacy.resistFingerprinting turned off, the current timestamp's precision should be down to 1 ms. With privacy.resistFingerprinting turned on, the current timestamp's precision should be down to 100 ms. Results with privacy.resistFingerprinting turned off: 1508628778838 1508628779563 1508628780206 1508628780753 1508628781317 Results with privacy.resistFingerprinting turned on: 1508628789200 1508628789700 1508628790100 1508628791600 1508629020600 Similar results with performance.now(): Results with privacy.resistFingerprinting turned off: 3393795.475 3394619.3000000003 3395467.685 3396226.7600000002 3396968.3200000003 3397566.075 Results with privacy.resistFingerprinting turned on: 3404800 3405600 3406400 3407400 3408400

The repo moved, so the link is now: https://github.com/tc39/ecmascript_sharedmem/issues/1

Ethan, does the introduction of 'dom.enable_performance_navigation_timing' in Bug 1403926 resolved for FF58 have any consequences for RFP timing mitigations? Thanks

I've added a section about this here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/getTime#Reduced_time_precision I can add the same section to these pages if we're okay with what I've described: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/now https://developer.mozilla.org/en-US/docs/Web/API/Performance/now https://developer.mozilla.org/en-US/docs/Web/API/Event/timeStamp https://developer.mozilla.org/en-US/docs/Web/API/File/lastModified https://developer.mozilla.org/en-US/docs/Web/API/File/lastModifiedDate https://developer.mozilla.org/en-US/docs/Web/API/BaseAudioContext/currentTime https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement/currentTime https://developer.mozilla.org/en-US/docs/Web/API/Animation/currentTime https://developer.mozilla.org/en-US/docs/Web/API/AnimationTimeline/currentTime https://developer.mozilla.org/en-US/docs/Web/API/AnimationPlaybackEvent/currentTime https://developer.mozilla.org/en-US/docs/Web/API/Animation/startTime (others?) Also, should this be mentioned on developer release notes? Fx 55? https://developer.mozilla.org/en-US/Firefox/Releases/55

(In reply to Florian Scholz [:fscholz] (MDN) from comment #110) > I've added a section about this here: > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/ > Global_Objects/Date/getTime#Reduced_time_precision > > I can add the same section to these pages if we're okay with what I've > described: > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/ > Global_Objects/Date/now > https://developer.mozilla.org/en-US/docs/Web/API/Performance/now > https://developer.mozilla.org/en-US/docs/Web/API/Event/timeStamp > https://developer.mozilla.org/en-US/docs/Web/API/File/lastModified > https://developer.mozilla.org/en-US/docs/Web/API/File/lastModifiedDate > https://developer.mozilla.org/en-US/docs/Web/API/BaseAudioContext/currentTime > https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement/currentTime > https://developer.mozilla.org/en-US/docs/Web/API/Animation/currentTime > https://developer.mozilla.org/en-US/docs/Web/API/AnimationTimeline/ > currentTime > https://developer.mozilla.org/en-US/docs/Web/API/AnimationPlaybackEvent/ > currentTime > https://developer.mozilla.org/en-US/docs/Web/API/Animation/startTime > (others?) > > Also, should this be mentioned on developer release notes? Fx 55? > https://developer.mozilla.org/en-US/Firefox/Releases/55 Hey Florian, That sounds great! The one prefname is wrong however, and I would change the ordering. I'd put first: privacy.reduceTimerPrecision is on by default and defaults to 20us in 59; in 60 it will be 2ms. privacy.resistFingerprinting.reduceTimerPrecision.microseconds adjusts that value. Then after that: If privacy.resistFingerprinting is enabled, the precision is 100ms or the value of privacy.resistFingerprinting.reduceTimerPrecision.microseconds, whichever is larger.

Awesome, thank you! Pages updated.