See also Bug 1424341 (but do not link these bugs publicly yet). We want to create a Gaussian fuzz factor to all timestamps, enabled by prefs, and with the shape controlled by prefs. performance.now is guaranteed to always increase, so we will have to handle that one specially, but otherwise we will allow timestamps to go backwards.

> performance.now is guaranteed to always increase, so we will have to handle that one specially Actually, a bunch of web things are specified in terms of a single monotonic clock (shared with performance.now()). Pretty much any time we're using TimeStamp in a web-facing API, its probably for a thing guaranteed to nondecrease relative to other things that use TimeStamp....

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #1) > > performance.now is guaranteed to always increase, so we will have to handle that one specially > > Actually, a bunch of web things are specified in terms of a single monotonic > clock (shared with performance.now()). Pretty much any time we're using > TimeStamp in a web-facing API, its probably for a thing guaranteed to > nondecrease relative to other things that use TimeStamp.... Ah. My thinking for performance.now() was to save the last value returned, and if our fuzzed factor was less, just return the last value. It seems like I may need to do that for all timers then. I was hoping I could just store one value inside nsRFPService, but at a minimum I'd probably need to store two (epoch and relative) - but there are probably a _lot_ of relative values and they're all relative to different things. (Like video playback...) So I'll wind up littering a lot of places with local member variables.

I would think that what we really want to do is to have a thing that wraps TimeStamp and gets fuzzed when you grab a timestamp, in a nondecreasing way. That thing would store a single "last value returned" thing, and we would make all the DOM code that is supposed to be sharing a monotonic clock go through that one wrapper around TimeStamp.

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #3) > I would think that what we really want to do is to have a thing that wraps > TimeStamp and gets fuzzed when you grab a timestamp, in a nondecreasing way. > That thing would store a single "last value returned" thing, and we would > make all the DOM code that is supposed to be sharing a monotonic clock go > through that one wrapper around TimeStamp. Okay, to make sure I understand what you mean: Here's the idl method for navigationStart: https://searchfox.org/mozilla-central/rev/c8e791091973825680bbba807fc1c4f5bda0f5a1/dom/performance/PerformanceTiming.h#117 Right now I'm changing that function to return > return nsRFPService::ReduceTimePrecisionAsMSecs(GetDOMTiming()->GetNavigationStart()); For the fuzzing, I should back up, past https://searchfox.org/mozilla-central/rev/c8e791091973825680bbba807fc1c4f5bda0f5a1/dom/base/nsDOMNavigationTiming.h#45 , past GetNavigationStartHighRes(), and into these members: https://searchfox.org/mozilla-central/rev/c8e791091973825680bbba807fc1c4f5bda0f5a1/dom/base/nsDOMNavigationTiming.h#192 And change them to a new class that wraps the original value, fuzzes it on return, returning the last value if the fuzzed value is less than it. This will move the fuzzing to before the rounding though; and I'm not sure that's the best order. (Also, I'm not sure what you're up to, but I'm in the meal hall)

> And change them to a new class that wraps the original value, fuzzes it on return, > returning the last value if the fuzzed value is less than it. That, or change the TimeStamp() calls that initially set those values... There's an issue around this stuff where the shared infrastructure bits like TimeStamp() need to be threadsafe. Maybe there's a better solution. Let me give a concrete example of the sort of thing I'm worried about. <iframe id="f"></iframe> <script> var now = performance.now(); f.onload = checkTimes; f.src = "something"; function checkTimes() { var nowTime = performance.timeOrigin + now; var loadTime = frames[0].performance.timing.navigationStart; if (nowTime < loadTime) { alert("This is a spec violation"); } } </script>

Okay, I've created a pref that adds jitter (in nsRFPService, not in TimeStamp() yet) and then a test that it fails (because it allows time to go backwards). I'm going to use this as a starting point to write some more tests and then work on a correct implementation.

Okay, the next thing that needs to be written are tests that do proper cross-worker/page/domain communication and confirm that those don't cause spec violations.

Okay, here's a test that will compare different timestamps to each other. It needs to be fleshed out to make more comparisons, but it's a good framework. I think at this point we can start work on a real implementation

The theory I am currently investigating is that where-ever we call ReduceTimePrecision we call it with either an absolute timestamp (milliseconds since epoch) or a relative timestamp. Whenever we call it with a relative timestamp, there is, nearby, something that contains the absolute time (in milliseconds since the epoch) that the relative timestamp is relative to. If we provide ReduceTimePrecision with both the timestamp and it's 'anchor point' (0 for absolute timestamps), we can be certain we never fuzz a timestamp backwards in time (by storing what the highest time we have ever returned is). I'm working my way through the callsites to validate this. So far I didn't have much difficulty doing performance.now() and Event.timestamp; and the entire performance API should be similarly easy. One I am seeing a lot of in my callstacks is in the Animation stuff, and from a look at the spec, that may be the thing that prevents this approach from working. So I'm going to investigate that next.

Now that this is public I can stop attaching patches and use mozreview. I've also rebased onto -central, but Bug 1430841 will probably land first and require some additional rebasing.

Okay, change of approach. (I still have the old patches locally.) My approach before was clamping and jittering a timestamp not when it was created, but before we provided it to the runtime, and was (hoping) that by tracking the anchor point of a timestamp (its offset from epoch) as well as the largest timestamp I'd ever returned that I'd be able to prevent time from going backwards. Some timestamps are created and then returned right away, like performance.now(). But others are not. That's why that approach wouldn't work - some timestamps are returned not when they are created (which would be when to assert time hadn't gone backwards) but some time (imagine tens of seconds) after they are created. (Like PerformanceResourceTiming.) My new plan is to Clamp and Jitter timestamps precisely when they are created (meaning we will be storing an inexact time in our variables, so c'est la view.) I'm also going to take a very incremental approach to addressing each segment of timestamps, beginning with the performance APIs. The attached patches jitter performance.now() (only). I haven't written tests yet, but I'm fairly certain time does not go backwards. *Except* when you communicate cross-process. Each process has its own 'greatestProvidedTimestamp' variable which means that *in theory* if an origin in process A can communicate to an origin in process B, and they generate timestamps such that T1 _should_ be greater than T2, it's possible for T1 and T2 to be jittered such that T2 is less than T1. I had spoken with :baku about this in the past, and I'm pretty sure that this is already a problem, it's just that jitter will probably increase the edge case occurrence. There aren't a lot of ways for origins to communicate if they're in different processes. BroadcastChannel is one, Storage Notification, and in theory (but not yet implemented) SharedWorker.

Okay, another change of approach. Construct a fuzzyfox like implementation for explicit clocks. Initial commit also contains several tricks for speed improvements. Need to iterate on this. Next up - test vectors.

Couple questions about the fuzzyfox/chrome-y implementation. 1) Is the key shared between contexts? The relevant case here would be if you can start one worker thread, run along for a while learning when the randomly chosen deadlines are, then start a new worker thread and use that knowledge to refine the new thread's clock (which will replay the previous thread's clock since it shares a key and time is relative to context start). 2) If not, I think you only need to cache the most-recently-derived random value. It should be the case that a given time check will either be in the current period (ie: cache hit on the most-recently-derived value) or in the next period (ie: always cache miss).

(In reply to David Kohlbrenner from comment #56) > Couple questions about the fuzzyfox/chrome-y implementation. Thanks for looking David! > 1) Is the key shared between contexts? > The relevant case here would be if you can start one worker thread, run > along for a while learning when the randomly chosen deadlines are, then > start a new worker thread and use that knowledge to refine the new thread's > clock (which will replay the previous thread's clock since it shares a key > and time is relative to context start). Yes. Poop. I will have to look for some sort of per-context 'secret' I can use. First thought that comes to mind is the memory address of the context (worker thread, nsDocument, etc). That sounds really sketchy (it's not even 64 bits of randomness AND it's a value we don't want to leak), but when it's combined with a 128 bit truly random secret through a secure hash function... it might be okay. I'll have to think about it. > 2) If not, I think you only need to cache the most-recently-derived random > value. > It should be the case that a given time check will either be in the current > period (ie: cache hit on the most-recently-derived value) or in the next > period (ie: always cache miss). Yea, given an time period, I won't need previous time periods (especially if I mix in the per-context secret) only future ones. (We call performance.now() quite a lot) But implementation-wise I think it's easier to do it this way, with the multiple-of-resolution - that makes keying easier and such. I'll have to think about it if I can figure out a simple way to make it more efficient...

Writing this down so I don't forget it. ekr had the idea of using AES-CTR for the Deterministic Random Midpoint instead of a hash function. If you have AES NI it should be very fast. I ought to be able to do the key schedule once and then use the clamp as the counter....

I'll second using an AES-CTR DRBG, we'd been talking about it being a good option as well. It will be slower on systems that don't support AES-NI than a secure hash function. (Which can be an issue if you look at Intel pre-2010 or so)

Comment on attachment 8943763 [details] Bug 1425462 When reducing the precision of timestamps, also apply fuzzytime to them https://reviewboard.mozilla.org/r/214140/#review225338 Code analysis found 2 defects in this patch: - 2 defects found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check path/to/file.cpp` (C/C++) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:140 (Diff revision 6) > +// TODO: Fix Race Conditions > +class LRUCache > +{ > +public: > + LRUCache() { > + for (int i=0; i<kCacheSize; i++) { Warning: Use range-based for loop instead [clang-tidy: modernize-loop-convert] ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:148 (Diff revision 6) > + this->cache[i].data = nullptr; > + } > + } > + > + shared_ptr<nsAutoCStringN<HASH_DIGEST_SIZE_BYTES>> get(long long aKey) { > + for (int i=0; i<kCacheSize; i++) { Warning: Use range-based for loop instead [clang-tidy: modernize-loop-convert]

(In reply to David Kohlbrenner from comment #60) > I'll second using an AES-CTR DRBG, we'd been talking about it being a good > option as well. > It will be slower on systems that don't support AES-NI than a secure hash > function. (Which can be an issue if you look at Intel pre-2010 or so) Presumably on such systemns you could use ChaCha

Comment on attachment 8943763 [details] Bug 1425462 When reducing the precision of timestamps, also apply fuzzytime to them https://reviewboard.mozilla.org/r/214140/#review227842 Code analysis found 2 defects in this patch: - 2 defects found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check path/to/file.cpp` (C/C++) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:145 (Diff revision 8) > +// TODO: Fix Race Conditions > +class LRUCache > +{ > +public: > + LRUCache() { > + for (int i=0; i<kCacheSize; i++) { Warning: Use range-based for loop instead [clang-tidy: modernize-loop-convert] ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:153 (Diff revision 8) > + this->cache[i].data = nullptr; > + } > + } > + > + shared_ptr<nsAutoCStringN<HASH_DIGEST_SIZE_BYTES>> get(long long aKey) { > + for (int i=0; i<kCacheSize; i++) { Warning: Use range-based for loop instead [clang-tidy: modernize-loop-convert]

For simplicity's sake, I decided not to land the random seed for new contexts in this bug. We'll have a pointer in the function definitions, but no one will use it. Correcting that will be in Bug 1440195.

Comment on attachment 8943763 [details] Bug 1425462 When reducing the precision of timestamps, also apply fuzzytime to them https://reviewboard.mozilla.org/r/214140/#review228126 Code analysis found 2 defects in this patch: - 2 defects found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check path/to/file.cpp` (C/C++) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:145 (Diff revision 9) > +// TODO: Fix Race Conditions > +class LRUCache > +{ > +public: > + LRUCache() { > + for (int i=0; i<kCacheSize; i++) { Warning: Use range-based for loop instead [clang-tidy: modernize-loop-convert] ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:153 (Diff revision 9) > + this->cache[i].data = nullptr; > + } > + } > + > + shared_ptr<nsAutoCStringN<HASH_DIGEST_SIZE_BYTES>> get(long long aKey) { > + for (int i=0; i<kCacheSize; i++) { Warning: Use range-based for loop instead [clang-tidy: modernize-loop-convert]

Comment on attachment 8953325 [details] Bug 1425462 Normalize the JavaScript Engine behavior by adding a callback https://reviewboard.mozilla.org/r/222600/#review228758 r+ with these issues addressed: ::: js/public/Date.h:174 (Diff revision 1) > // Otherwise |time| *must* correspond to a time within the valid year |year|. > // This should usually be ensured by computing |year| as |JS::DayFromYear(time)|. > JS_PUBLIC_API(double) > DayWithinYear(double time, double year); > > -// Sets the time resolution for fingerprinting protection. > +using ReduceMicrosecondTimePrecisionCallback = double(*)(double, void*, int); Could you provide a comment here documenting the signature? ::: js/src/jsdate.cpp:410 (Diff revision 1) > JS::DayWithinYear(double time, double year) > { > return ::DayWithinYear(time, year); > } > > +static JS::ReduceMicrosecondTimePrecisionCallback reduceMicrosecondTimePrecisionCallback = nullptr; For symmetry, could you move this up to be right under sResolutionUsec at the top of this file, prefix with 's', and make it an Atomic<Relaxed>? ::: js/src/jsdate.cpp:1312 (Diff revision 1) > static ClippedTime > NowAsMillis() > { > double now = PRMJ_Now(); > - if (sResolutionUsec) { > + if (reduceMicrosecondTimePrecisionCallback) { > + now = reduceMicrosecondTimePrecisionCallback(now, nullptr, 1); This 'nullptr' and '1' are way to magical and could silently break if TimePrecisionType changes. Since this callback is calling a stub anyway, how about we change the signature of ReduceMicrosecondTimePrecisionCallback to be `double (*)(double)` so that the Gecko callee stub can pass the appropriate enum and null ::: js/src/jsdate.cpp:1314 (Diff revision 1) > { > double now = PRMJ_Now(); > - if (sResolutionUsec) { > + if (reduceMicrosecondTimePrecisionCallback) { > + now = reduceMicrosecondTimePrecisionCallback(now, nullptr, 1); > + } > + else if (sResolutionUsec) { nit: this would be '} else if {' but, by SM style (https://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style#Other_whitespace), no curlies around single-line then/else branches

Okay, I think this is in a decent place right now. What's left to do for this bug: 1: I should collapse patches 1, 3, and 4 into a single patch. I'm avoiding doing this right now because I think it's probably easier to understand what is happening in them with them separate. 2: The NSS bits in Patch 4 have some concerning things in them: a) Can PK11_Encrypt be called with the same PK11SymKey object on multiple threads? b) I assume that by keeping a static PK11SymKey around, I can avoid the key schedule overhead on later encryptions - right? c) I need to initialize NSS in here so a gtest can pass, but it seems really unnecessary and ugly. Any better way around this? 3: The LRU Cache is not thread safe 4: I need to figure out what the best cache size for the LRUCache is. 5: I need to go through tests that break as a result of jitter being on, and investigate them. I should also go through the tests in Patch 2 and see if disabling jitter is actually important to them. 6: We want to turn jitter on by default (in 60) - right? Another random this: I'm not actually doing CTR mode, I'm doing ECB mode. I don't think there's any serious performance improvement from switching to ECB mode, but as long as I'm encrypting an all-zero plaintext block, I ought to switch to it to simplify the code and avoid some very minor overheard. I'll probably do that in Bug 1440195. I'm going to flag a few NSS folks to take a look...

Just a quick comment on the AES-CTR DRBG, I'll suggest taking a look at the NIST standard (https://csrc.nist.gov/publications/detail/sp/800-90a/rev-1/final) if you haven't yet.

So the NSS Initialization stuff definitely seems dodgy. I'm see > Child 7487, Main Thread] WARNING: NSS_Shutdown failed - some NSS resources are still in use (see bugs 1417680 and 1230312): file /Users/tritter/Documents/moz/mozilla-unified-fuzz-4/xpcom/build/XPCOMInit.cpp, line 1019 But I'm not sure if that was pre-existing. On tests the story is worse: https://public-artifacts.taskcluster.net/feXLWujDS8qtnTn8t7zo4w/0/public/logs/live_backing.log > INFO - GECKO(1049) | [Parent 1049, Main Thread] WARNING: NSS_Shutdown failed - some NSS resources are still in use (see bugs 1417680 and 1230312): file /builds/worker/workspace/build/src/xpcom/build/XPCOMInit.cpp, line 1019 > INFO - GECKO(1049) | firefox: /builds/worker/workspace/build/src/db/sqlite3/src/sqlite3.c:23769: sqlite3MutexAlloc: Assertion `mutexIsInit' failed. > INFO - TEST-INFO | Main app process: killed by out-of-range signal, number 134 > INFO - Buffered messages finished > ERROR - TEST-UNEXPECTED-FAIL | accessible/tests/mochitest/value/test_range.html | application terminated with exit code -134 This causes everything to fail. I've tried poking around at this, but I'm not really seeing anything that jumps out at me.

Comment on attachment 8943763 [details] Bug 1425462 When reducing the precision of timestamps, also apply fuzzytime to them https://reviewboard.mozilla.org/r/214140/#review228846 Code analysis found 2 defects in this patch: - 2 defects found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check path/to/file.cpp` (C/C++) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:145 (Diff revision 10) > +// TODO: Fix Race Conditions > +class LRUCache > +{ > +public: > + LRUCache() { > + for (int i=0; i<kCacheSize; i++) { Warning: Use range-based for loop instead [clang-tidy: modernize-loop-convert] ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:153 (Diff revision 10) > + this->cache[i].data = nullptr; > + } > + } > + > + shared_ptr<nsAutoCStringN<HASH_DIGEST_SIZE_BYTES>> get(long long aKey) { > + for (int i=0; i<kCacheSize; i++) { Warning: Use range-based for loop instead [clang-tidy: modernize-loop-convert]

Comment on attachment 8953744 [details] Bug 1425462 Sprinkle some thread safety on the LRU Cache https://reviewboard.mozilla.org/r/222960/#review228848 Code analysis found 1 defect in this patch: - 1 defect found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check path/to/file.cpp` (C/C++) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:172 (Diff revision 1) > - ("LRU Cache HIT with %lli == %lli", > + ("LRU Cache HIT with %lli == %lli", > - aKey, cacheEntry.key)); > + aKey, cacheEntry.key)); > -#endif > + #endif > - return cacheEntry.data; > + return cacheEntry.data; > - } > + } > + else { Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]

Comment on attachment 8952947 [details] Bug 1425462 Convert from a hash function to a AES-CTR based DRBG https://reviewboard.mozilla.org/r/222206/#review228962 I added a couple technical comments inline. But I have two general concerns with this approach. i) I don't think that this type of anti-fingerprinting is particularly effective. With enough samples it's easy enough to remove the randomness you add. ii) Assuming the randomess we add here is actually doing something, I don't think implementing a custom random number generator here is what we should be doing. First, `GenerateRandomBytes` is already a PRNG so adding another one on top is a little pointless. Second, it's hard to verify that this is actually a sane PRNG. If you want a CTR-based PRNG, we can easily add one to NSS and you won't have to do any of this. So if you really want to go this path of anti-fingerprinting I'd suggest to simply use the random bytes you get from `GenerateRandomBytes` and all the other problems go away. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:359 (Diff revision 3) > > - /* > - * Use a cryptographicly secure hash function, but do _not_ use an HMAC. > - * Obviously we're not using this data for authentication purposes, but > - * even still an HMAC is a perfect fit here, as we're hashing a value > - * using a seed that never changes, and an input that does. So why not > + // It's possible that NSS hasn't been initialized, observed when running gtests > + // This will double check that it has been initialized. It will assert if not done > + // in the parent process, which doesn't make any sense to me. > + // REVIEWER: This is certainly wrong, but I'm not sure what it is I'm supposed to do. > + // REVIEWER: gtest complains: c) I need to initialize NSS in here so a gtest can pass, but it seems really unnecessary and ugly. Any better way around this? NSS should really be initialised here. If not, the bug is somewhere else. The warning you see though suggests that not all ressources get freed. (See later comments.) ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:379 (Diff revision 3) > - * provide the attacker much control over the input. Nor do we let them > - * have the prefix. > - */ > + if (!arena) { > + arena = UniquePLArenaPool(PORT_NewArena(DER_DEFAULT_CHUNKSIZE)); > + MOZ_ASSERT(arena); > + } > + if (!symKey || !symKey.get()) { > + SECItem keyItem = { siBuffer, nullptr, 0 }; a) Can PK11_Encrypt be called with the same PK11SymKey object on multiple threads? No one here really knows what the PK11 layer is doing. But I'd say no. b) I assume that by keeping a static PK11SymKey around, I can avoid the key schedule overhead on later encryptions - right? Not really. We can't keep AES state around because of the PK11 API. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:379 (Diff revision 3) > - * provide the attacker much control over the input. Nor do we let them > - * have the prefix. > - */ > + if (!arena) { > + arena = UniquePLArenaPool(PORT_NewArena(DER_DEFAULT_CHUNKSIZE)); > + MOZ_ASSERT(arena); > + } > + if (!symKey || !symKey.get()) { > + SECItem keyItem = { siBuffer, nullptr, 0 }; You should use scoped NSS types. See https://searchfox.org/mozilla-central/rev/b469db5c90d618f4b202d5ef280e1a78224eb43b/security/manager/ssl/ScopedNSSTypes.h#339 It looks to me like the `keyItem` leaks here. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:395 (Diff revision 3) > + > + // Set up CTR Parameters > + SECItem param = { siBuffer, nullptr, 0 }; > + CK_AES_CTR_PARAMS ctrParams; > + // REVIEWER: This is the length of the entire IV right? Encompassing nonce || counter > + ctrParams.ulCounterBits = 128; This is the length of the counter (128 is the maximum).

> Ah. My thinking for performance.now() was to save the last value returned, and if our fuzzed factor was less, just return the last value. It seems like I may need to do that for all timers then. Why do you need to return the last value? performance.now() is floating point. You can put fuzz in the picoseconds, nanoseconds. Put some pseudorandom fuzz there too. Nothing will break. This also makes global monotonic increases feasible too. That'll allow tighter fuzz (e.g. 20us jitter) with better security than even the current 2ms hard-edge solution in FireFox 60 (as of Feb 26th, 2018). Remember to vary your fuzz distribution, don't use fixed 20us fuzz distribution. Gaussian is a good idea. But you can even vary the Gaussian randomly a bit too -- nanoseconds/picoseconds too -- to stymie a user agent's ability to figure out the gaussian simply by monitoring if there's an "edgeiness" (e.g. things increments in 1/10,000,000 granularities). By throwing nanoseconds/picoseconds into the mix which is harmless, you solve a lot of problems, you can keep things monotonic useragent-wide... Besides, performance.now often returns numbers like 452343.42789999998 or 20303.427000007 because of how doubles are stored -- even on a system with 1/10,000,000 or 1/1,000,000 .... So user agents already "expect" muddy data in all data. So this change is safe and does not break performance.now. If the numbers increment (even if fuzzed) only every time a timer increments, then a useragent can use that data to guess the granularity of the incrementing (despite a double-pseudorandom fuzzing) By randomizing ALL digits to the full precision of the double, you thusly maximize pseudorandom entropy. And 20us with this amount of randomness (all the way into the nanosecs) is much safer than FireFox 60's 2 millisecond hard-edge.

(errr... all the way to femtosecs, according to the amount of precision available in a double).

P.S. I write about my concerns about 2 milliseconds in these comments: https://bugzilla.mozilla.org/show_bug.cgi?id=1435296#c116 and https://bugzilla.mozilla.org/show_bug.cgi?id=1435296#c116 Which is the double-pseudorandom approach. However, you shouldn't execute the double-pseudorandom approach only at the intervals whenever the system counter increments (e.g. if the system uses a clock that increases at 100KHz, 1MHz, or 10MHz). Meaning, incorrect implementation would be that performance.now() updates at granular intervals representing the system's timer granularity -- despite already using my suggested double-pseudorandom (fuzzy time) approach. That's timer-precision-information leakage to the useragent that can be used for creating a constructed timer. It would take a brilliant programmer to do so, but the granularity of the increments (even if the increments have double-pseudorandomness in value and in realworld time) is valuable timing information for intelligent performance profiling algorithms. It would be much easier at 100KHz, but the mathematics theory is there. So you definitely, should, ideally only randomly return the same time value, but also sometimes pseudorandomly increment anyway -- all the way to the femtoseconds. Make sure it's completely fuzzy though (e.g. ideally dynamically random gaussians, not a single fixed-range gaussian) -- the malware authors have access to your useragent source code to determine how FireFox is fuzzing things! Outfox that (pun intended). Yes, having a good algorithm may be challenging (multiple layers of pseudorandom) but you can achieve monotonic useragent-wide while having tight fuzz, while being secure from Meltdown/Spectre (projected). Eventually, so you should fuzz all of this: * The /time/ increment * The /realworld/ interval between increments * The /granularity/ of the increments should not reveal the underlying clock's granularity. * The varyingness of the random should not be predictable (e.g. if polling performance.now very quickly and it's designed to monotonically increase, try to make sure info of monotonicness-compensation algorithms is not being revealed to the useragent (e.g. known random-range variances) * All precision in a double should be fuzzed (all the way into femtoseconds) You can begin with time/realworld (quickly for FF60) then improve randomization of granularity in FF61 if there's no time, but ideally, you should try to also do granularity too.

Comment on attachment 8953744 [details] Bug 1425462 Sprinkle some thread safety on the LRU Cache https://reviewboard.mozilla.org/r/222960/#review229386 Just a couple small comments, because "sprinkle a little thread safety" in patch descriptions always piques my interest. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:194 (Diff revision 6) > memset(&lowestTime, 0xFF, sizeof(PRTime)); > lowestTime &= ~(1ULL << ((sizeof(PRTime) * 8) - 1)); > > + MutexAutoLock lock(mLock); > int lowestKey = 0; > - for (int i=0; i<kCacheSize; i++) { > + for (int i=0; i<LRU_CACHE_SIZE; i++) { Nit: could just `for (auto& cacheEntry : this->cache)` here. You'd have to do something about `lowestKey`, but maybe that could become a `cache_entry*`, initialized to `&this->cache[0]`? ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:221 (Diff revision 6) > } > > > private: > struct cache_entry { > long long key; Nit: this wants to be `Atomic<long long, Relaxed>` if you are really going to try to read keys of entries opportunistically. Otherwise TSan will complain about you touching `key` without proper synchronization.

Comment on attachment 8954073 [details] Bug 1425462 Turn jitter on by default https://reviewboard.mozilla.org/r/223224/#review229432

Comment on attachment 8943763 [details] Bug 1425462 When reducing the precision of timestamps, also apply fuzzytime to them https://reviewboard.mozilla.org/r/214140/#review229596 Overally looks reasonable, but r- for the `shared_ptr` stuff. I think replacing it with `nsCString` might be the easiest fix. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:141 (Diff revision 14) > > +#define HASH_DIGEST_SIZE_BITS (256) > +#define HASH_DIGEST_SIZE_BYTES (HASH_DIGEST_SIZE_BITS / 8) > + > +// TODO: Fix Race Conditions > +class LRUCache Some comments about what this LRU is intended to store and be used for would be helpful. Also, when will the race conditions be fixed? Is this class meant to be threadsafe? ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:152 (Diff revision 14) > + cacheEntry.key = 0xFFFFFFFFFFFFFFFF; > + cacheEntry.data = nullptr; > + } > + } > + > + shared_ptr<nsAutoCStringN<HASH_DIGEST_SIZE_BYTES>> get(long long aKey) { We try to avoid using `shared_ptr<>` in gecko AFAIK. I think at this point you probably need to create a class with uint8_t[HASH_DIGEST_SIZE_BYTES] and NS_INLINE_DECL_REFCOUNTING, etc. Or, after looking at how this is used below, maybe just operate on `nsCString` directly. It uses an efficient thread-safe copy-on-write buffer sharing implementation. You can add assertions to verify the strings are the correct length. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:168 (Diff revision 14) > + } > + } > + return nullptr; > + } > + > + void store(long long aKey, nsAutoCStringN<HASH_DIGEST_SIZE_BYTES> aValue) { nit: Gecko style uses uppercase method names. `Store()` instead of `store()`, etc. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:173 (Diff revision 14) > + void store(long long aKey, nsAutoCStringN<HASH_DIGEST_SIZE_BYTES> aValue) { > + PRTime lowestTime; > + // Set lowestTime to the maximum value of PRTime, which we assume is signed > + // but do not make an assumption about the size thereof. > + memset(&lowestTime, 0xFF, sizeof(PRTime)); > + lowestTime &= ~(1ULL << ((sizeof(PRTime) * 8) - 1)); Is this really the best way to do this? The bit math here seems needlessly opaque to me. It seems you could just assign lowestTime the first time value in the array, etc. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:176 (Diff revision 14) > + // but do not make an assumption about the size thereof. > + memset(&lowestTime, 0xFF, sizeof(PRTime)); > + lowestTime &= ~(1ULL << ((sizeof(PRTime) * 8) - 1)); > + > + int lowestKey = 0; > + for (int i=0; i<kCacheSize; i++) { nit: spaces around operators like `i = 0` and `i < kCacheSize`. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:177 (Diff revision 14) > + memset(&lowestTime, 0xFF, sizeof(PRTime)); > + lowestTime &= ~(1ULL << ((sizeof(PRTime) * 8) - 1)); > + > + int lowestKey = 0; > + for (int i=0; i<kCacheSize; i++) { > + if (this->cache[i].accessTime < lowestTime) { Should you see if the given key is already in the cache instead of overwriting a potentially different key and producing a duplicate? ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:194 (Diff revision 14) > +#endif > + } > + > + > +private: > + struct cache_entry { nit: Camel case please. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:201 (Diff revision 14) > + PRTime accessTime = 0; > + shared_ptr<nsAutoCStringN<HASH_DIGEST_SIZE_BYTES>> data; > + }; > + > + static const int kCacheSize = 45; > + cache_entry cache[kCacheSize]; It would be better to use `nsTArray<CacheEntry>` here rather than a fixed static array. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:229 (Diff revision 14) > + * 395. So comparing an (actual) timestamp of 325 and 351 could see the 325 clamped > + * up to 400 and the 351 clamped down to 300. The seed is per-process, so this case > + * occurs when one can compare timestamps cross-process. This is uncommon (because > + * we don't have site isolation.) The circumstances this could occur are > + * BroadcastChannel, Storage Notification, and in theory (but not yet implemented) > + * SharedWorker. This should be an exhaustive list (at time of comment writing!). Our network stack records TimeStamp values in the parent process and sends them back to a content process where they are compared to values in that process. Its unclear if that is impacted by this paragraph. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:255 (Diff revision 14) > + > +static LRUCache* sCache = nullptr; > + > +/* static */ > +nsresult > +nsRFPService::RandomMidpoint(long long aClampedTime, I don't think we use `long long` in gecko. Can you use `uint32_t` or `uint64_t` here? Although I see other code in the file uses `long long`. Why is this necessary? ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:257 (Diff revision 14) > + > +/* static */ > +nsresult > +nsRFPService::RandomMidpoint(long long aClampedTime, > + long long aResolutionUSec, > + long long* aMidpoint, Please name this something like `aMidpointOut` to indicate its an output. Also, please add a code comment for the method indicating the possible values and the units of the output. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:263 (Diff revision 14) > + uint8_t * aSecretSeed /* = nullptr */) > +{ > + nsresult rv; > + const int kSeedSize = 16; > + const int kClampTimesPerDigest = HASH_DIGEST_SIZE_BITS / 32; > + static uint8_t * secretSeed = nullptr; Please start static variables with `s`. So `sSecretSeed`. Also, what thread is this expected to run on? If its main thread only please add a `MOZ_ASSERT(NS_IsMainThread())` to the top of the function here. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:266 (Diff revision 14) > + const int kSeedSize = 16; > + const int kClampTimesPerDigest = HASH_DIGEST_SIZE_BITS / 32; > + static uint8_t * secretSeed = nullptr; > + > + if (!sCache) { > + sCache = new LRUCache(); How do you free this memory at shutdown? You likely want to use a `StaticAutoPtr` here and then call `ClearOnShutdown()`. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:293 (Diff revision 14) > + */ > + long long reducedResolution = aResolutionUSec * kClampTimesPerDigest; > + long long extraClampedTime = (aClampedTime / reducedResolution) * reducedResolution; > + > + // While this code compiled and runs I am really unsure I am doing the right thing > + // with shared_ptr and string types. See my other comment about creating a ref-counted class with a fixed length array in it. Or, just operate on `nsCString` and add assertions that its always of the correct length. Our `nsCString` class internally does a thread-safe copy-on-write buffer sharing optimization. Its probably the most performant thing to do here. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:295 (Diff revision 14) > + long long extraClampedTime = (aClampedTime / reducedResolution) * reducedResolution; > + > + // While this code compiled and runs I am really unsure I am doing the right thing > + // with shared_ptr and string types. > + nsAutoCStringN<HASH_DIGEST_SIZE_BYTES> fred; > + shared_ptr<nsAutoCStringN<HASH_DIGEST_SIZE_BYTES>> bob = sCache->get(extraClampedTime); nit: Better names would be great here. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:308 (Diff revision 14) > + secretSeed = new uint8_t[kSeedSize]; > + memcpy(secretSeed, aSecretSeed, kSeedSize); > + } > + > + // If we don't have a seed, we need to get one > + if(!secretSeed) { We need thread assertions as I mentioned in the other comment or locking for this to be safe. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:341 (Diff revision 14) > + * provide the attacker much control over the input. Nor do we let them > + * have the prefix. > + */ > + > + // Then hash extraClampedTime and store it in the cache > + nsCOMPtr<nsICryptoHash> hasher = do_CreateInstance("@mozilla.org/security/hash;1", &rv); If we're single threaded (main thread only), it might make sense to cache the `nsICryptoHash` object somewhere so we don't have to keep creating it over and over. You can just call `Init()` again on the same object. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:361 (Diff revision 14) > + > + // Finally, store it in the cache > + sCache->store(extraClampedTime, derivedSecret); > + fred = derivedSecret; > + } > + else { // Cache Hit! nit: Move the cache hit to be the first branch in the conditional. That way we can see both branches of the conditional right next to the if-statement. Here we have to scroll back up to see what this conditional came from. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:370 (Diff revision 14) > + // Offset the appropriate index into the hash output, and then turn it into a random midpoint > + // between 0 and aResolutionUSec > + int byteOffset = ((aClampedTime - extraClampedTime) / aResolutionUSec) * 4; > + uint32_t deterministiclyRandomValue = *BitwiseCast<uint32_t*>(PromiseFlatCString(fred).get() + byteOffset); > + deterministiclyRandomValue %= aResolutionUSec; > + *aMidpoint = deterministiclyRandomValue; Please assert or runtime check that aMidpoint is not nullptr. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:729 (Diff revision 14) > } > } > + > + if (sCache) { > + delete sCache; > + sCache = nullptr; I guess this is why you don't need `ClearOnShutdown()` at the moment.

Comment on attachment 8951436 [details] Bug 1425462 Address tests for Time Jittering https://reviewboard.mozilla.org/r/220740/#review229602

Comment on attachment 8951437 [details] Bug 1425462 Add an empty context pointer for our Timer Precision Reduction Functions https://reviewboard.mozilla.org/r/220742/#review229604 ::: commit-message-7c463:7 (Diff revision 9) > + > +We need to include a seed for each context (origin, iframe, worker, etc) we reduce > +the time precision of. This prevents a replay attack of the random midpoint sequence. > + > +This per-context seed will be added in Bug 1440195, in this patch we simply provide > +a parameter for it. Why are we adding an empty `void*` now? Why not wait until bug 1440195 is implemented? For example, its unclear to me why this needs to be `void*` vs a real type other than that we don't know what the real type is yet.

Comment on attachment 8943763 [details] Bug 1425462 When reducing the precision of timestamps, also apply fuzzytime to them https://reviewboard.mozilla.org/r/214140/#review229596 > Some comments about what this LRU is intended to store and be used for would be helpful. > > Also, when will the race conditions be fixed? Is this class meant to be threadsafe? Race conditions are fixed in a patch below. > Is this really the best way to do this? The bit math here seems needlessly opaque to me. > > It seems you could just assign lowestTime the first time value in the array, etc. Yea, Nathan pointed this out, and I corrected it in the make-threadsafe patch below, mostly because rebasing would have been annoying based on other changes I was making. > Should you see if the given key is already in the cache instead of overwriting a potentially different key and producing a duplicate? Also fixed in the thread-safety patch below. > Our network stack records TimeStamp values in the parent process and sends them back to a content process where they are compared to values in that process. Its unclear if that is impacted by this paragraph. Hmmmmmmmmm. I *think* this is okay for two reasons. 1) Those timer values are already exposed via the Performance APIs, so if they do cause cross-process time weirdness, that's already occuring. 2) There's a bunch of code in the Performance APIs that do some sanity checks about what can and can't occur before each other, so my assumption is that it is correcting any such possible cases. Not a definitive all-clear, but I don't think we're making anything worse in this patch with regards to that behavior. > I don't think we use `long long` in gecko. Can you use `uint32_t` or `uint64_t` here? > > Although I see other code in the file uses `long long`. Why is this necessary? I confess I got a code snippet recommendation from another Mozillian when fixing float fuzziness in ReduceTimeImpl that used long long and followed that pattern. > Please start static variables with `s`. So `sSecretSeed`. > > Also, what thread is this expected to run on? If its main thread only please add a `MOZ_ASSERT(NS_IsMainThread())` to the top of the function here. It can run on any thread (hence the thread saftey concerns.) > nit: Better names would be great here. Oh geez I forgot to rename those from my fiddling around. > I guess this is why you don't need `ClearOnShutdown()` at the moment. I presume StaticAutoPtr and ClearOnShutdown is the better pattern, so I will investigate that.

Comment on attachment 8951437 [details] Bug 1425462 Add an empty context pointer for our Timer Precision Reduction Functions https://reviewboard.mozilla.org/r/220742/#review229604 > Why are we adding an empty `void*` now? Why not wait until bug 1440195 is implemented? For example, its unclear to me why this needs to be `void*` vs a real type other than that we don't know what the real type is yet. Originally I had intended to land the whole feature in this bug, then split it for simplicity. And the original plan was to pass in a pointer to a nsDocument or something like that and mix that in; I had believed I could do this safely when the backing crypto construct was a hash; but with AES-CTr I am less sure. I can move this patch over to the other bug.

(In reply to Tom Ritter [:tjr] from comment #131) > > Please start static variables with `s`. So `sSecretSeed`. > > > > Also, what thread is this expected to run on? If its main thread only please add a `MOZ_ASSERT(NS_IsMainThread())` to the top of the function here. > > It can run on any thread (hence the thread saftey concerns.) Maybe this is in the other patch, but the we need locking around the sSecretSeed initialization then as well. Or an explicit initialization step when the service is created. Same goes for creating the LRU itself.

Comment on attachment 8943763 [details] Bug 1425462 When reducing the precision of timestamps, also apply fuzzytime to them https://reviewboard.mozilla.org/r/214140/#review229676 Code analysis found 1 defect in this patch: - 1 defect found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check path/to/file.cpp` (C/C++) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:184 (Diff revision 15) > + lowestKey = &cacheEntry; > + } > + } > + > + lowestKey->key = aKey; > + lowestKey->data = aValue; Warning: Parameter 'avalue' is passed by value and only copied once; consider moving it to avoid unnecessary copies [clang-tidy: performance-unnecessary-value-param]

Okay, I addressed everything except the long long thing, which I will play with tomorrow.

Comment on attachment 8953744 [details] Bug 1425462 Sprinkle some thread safety on the LRU Cache Thread behavior looks solid.

Comment on attachment 8943763 [details] Bug 1425462 When reducing the precision of timestamps, also apply fuzzytime to them https://reviewboard.mozilla.org/r/214140/#review229924 Thanks for the updates. I don't love all the separate static variables, but I don't want to bikeshed too much here. Can we maybe clean that up in a follow-on bug? It feels like they can be unified into nsRFPService somehow or a separate singleton class. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:156 (Diff revision 17) > + for (int i = 0; i < LRU_CACHE_SIZE; i++) { > + CacheEntry cacheEntry; > + cacheEntry.accessTime = 0; > + cacheEntry.key = 0xFFFFFFFFFFFFFFFF; > + cacheEntry.data = nullptr; > + this->cache.AppendElement(cacheEntry); Can you please move the default values here into a CacheEntry constructor? You can then just do `this->cache.SetLength(LRU_CACHE_SIZE)` to initialize them all in one step. I was also kind of thinking you would dynamically add the entries as `Store()` was called up to the limit when you switched to `nsTArray`, but I guess this is still ok. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:175 (Diff revision 17) > + } > + } > + return EmptyCString(); > + } > + > + void Store(long long aKey, nsCString& aValue) { Please make the argument at least `const nsCString&`. It would be slightly more correct to use `const nsACString&`, but I don't think it really matters here. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:184 (Diff revision 17) > + lowestKey = &cacheEntry; > + } > + } > + > + lowestKey->key = aKey; > + lowestKey->data = aValue; Can you `MOZ_DIAGNOSTIC_ASSERT(aValue.Length() == HASH_DIGEST_SIZE_BYTES)`? ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:199 (Diff revision 17) > + long long key; > + PRTime accessTime = 0; > + nsCString data; > + }; > + > + AutoTArray<CacheEntry, LRU_CACHE_SIZE> cache; I would probably use `nsTArray` here, but ok. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:605 (Diff revision 17) > MOZ_ASSERT(NS_IsMainThread()); > > nsresult rv; > > + sStaticLock = new Mutex("mozilla.resistFingerprinting.sStaticLock"); > + ClearOnShutdown(&sStaticLock); If you can do this, can the `LRUCache`, seed, and mutex all just live on the `nsRFPService` itself?

Okay, I think this is ready to go, I'm going to do one last full try run and if that's green, mark it for checkin.

Comment on attachment 8955107 [details] Bug 1425462 Refactor the static members into static globals to avoid leakchecks https://reviewboard.mozilla.org/r/224256/#review230216 I am fine with that. r=me

Hey Julien - I just missed the merge window with this, but I was hoping to get it in before the freeze to give it more time to be tested. It's a subtle change, and any issues with it will probably only be exposed through someone browsing with the build, noticing something weird, and manually checking (flipping the pref off) to see if this caused it. I'm not sure when merges happen, but I'm hoping this can get pulled in as feasible.

As discussed on IRC, this can go to autoland tomorrow, so it doesn't go into 60.0b1 right away but can get some exposure in nightly over the following days before the 60.0b2 build next Thursday. Setting tracking+ for 60 to keep this on my radar.

Comment on attachment 8953744 [details] Bug 1425462 Sprinkle some thread safety on the LRU Cache Since I did the first r+ without mozreview, it won't allow me to use it now. Ugh.