Bug 1425462 will land jitter in explicit clocks. However it will be possible to reply a jitter sequence in a Worker, because the context will start over at zero. (See Comment 56) For simplicity's sake, that bug doesn't add a random seed in new contexts. This bug will do that.

Comment on attachment 8954439 [details] Bug 1440195 Add a per-context mix-in value for our Timer Precision Reduction Functions https://reviewboard.mozilla.org/r/223520/#review232550 I need to see this patch again. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:465 (Diff revision 11) > // We think this is okay, and we codify it in some tests. > double timeScaled = aTime * (1000000 / aTimeScale); > // Cut off anything less than a microsecond. > long long timeAsInt = timeScaled; > + > + if (aContextMixin == 0 && aType == TimerPrecisionType::All && timeAsInt < 1204233985000) { Add a comment related to 1204233985000 value. It seems that if aContextMixin is 0, we have a bug somewhere. But if timeAsInt is < 10years, maybe we should not crash. Can we just ignore this issue and do not reduce the time precision? ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:492 (Diff revision 11) > long long clamped = floor(double(timeAsInt) / resolutionAsInt) * resolutionAsInt; > > > long long midpoint = 0, > clampedAndJittered = clamped; > - // RandomMidpoint uses crypto functions from NSS. But we wind up in this code _very_ early > + if (sJitter && aType != TimerPrecisionType::VideoCalculation) { why only for !VideoCalculation? Why not doing this check in RandomMidpoint instead, passing aType too? In general, it seems an hack... ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:522 (Diff revision 11) > + int64_t randomSeed = 0; > + while (randomSeed == 0) { > + nsresult rv; > + nsCOMPtr<nsIRandomGenerator> randomGenerator = > + do_GetService("@mozilla.org/security/random-generator;1", &rv); > + if (!NS_FAILED(rv)) { quick return: if (NS_WARN_IF(NS_FAILED(rv))) { return rand(); } ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:525 (Diff revision 11) > + nsCOMPtr<nsIRandomGenerator> randomGenerator = > + do_GetService("@mozilla.org/security/random-generator;1", &rv); > + if (!NS_FAILED(rv)) { > + uint8_t* buffer = nullptr; > + rv = randomGenerator->GenerateRandomBytes(sizeof(randomSeed), &buffer); > + if (!NS_FAILED(rv)) { NS_SUCCEEDED(rv) ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:528 (Diff revision 11) > + uint8_t* buffer = nullptr; > + rv = randomGenerator->GenerateRandomBytes(sizeof(randomSeed), &buffer); > + if (!NS_FAILED(rv)) { > + memcpy(&randomSeed, buffer, sizeof(randomSeed)); > + } else { > + MOZ_ASSERT(false, "Could not get a secure random number"); Don't assert here... Instead, I prefer to have this method as: nsresult nsRFPService::GetRandomContextSeed(int64_t* aSeed) and propagate the error. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:535 (Diff revision 11) > + } > + if (buffer) { > + free(buffer); > + } > + } else { > + MOZ_ASSERT(false, "Could not get a secure random number"); don't crash. propagate the error. If we are shutting down, do_GetService() can fail. Same thing if the method is used before XPCOM initialization. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:556 (Diff revision 11) > > /* static */ > double > nsRFPService::ReduceTimePrecisionAsUSecsWrapper(double aTime) > { > - return nsRFPService::ReduceTimePrecisionImpl(aTime, MicroSeconds, TimerResolution(), TimerPrecisionType::All); > + return nsRFPService::ReduceTimePrecisionImpl(aTime, MicroSeconds, TimerResolution(), 0, TimerPrecisionType::All); comment about 0. Usually I see code like: ...., 0 /* context mixin */, ... ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:590 (Diff revision 11) > > /* static */ > uint32_t > nsRFPService::GetSpoofedTotalFrames(double aTime) > { > - double time = ReduceTimePrecisionAsSecs(aTime); > + double time = ReduceTimePrecisionAsSecs(aTime, 0, TimerPrecisionType::VideoCalculation); ditto ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:607 (Diff revision 11) > // report a zero dropped rate for this case. > if (targetRes >= aWidth * aHeight) { > return 0; > } > > - double time = ReduceTimePrecisionAsSecs(aTime); > + double time = ReduceTimePrecisionAsSecs(aTime, 0, TimerPrecisionType::VideoCalculation); here as well.

Comment on attachment 8954944 [details] Bug 1440195 For timestamps that are absolute, specify a null context pointer https://reviewboard.mozilla.org/r/224118/#review232568 ::: dom/file/BaseBlobImpl.cpp:67 (Diff revision 9) > int64_t > BaseBlobImpl::GetLastModified(ErrorResult& aRv) > { > MOZ_ASSERT(mIsFile, "Should only be called on files"); > if (IsDateUnknown()) { > - mLastModificationDate = nsRFPService::ReduceTimePrecisionAsUSecs(PR_Now()); > + mLastModificationDate = nsRFPService::ReduceTimePrecisionAsUSecs(PR_Now(), 0); Write a comment about what this 0 is about. ::: dom/file/MultipartBlobImpl.cpp:273 (Diff revision 9) > // var x = new Date(); var f = new File(...); > // x.getTime() < f.dateModified.getTime() > // could fail. > mLastModificationDate = nsRFPService::ReduceTimePrecisionAsUSecs( > - lastModifiedSet ? lastModified * PR_USEC_PER_MSEC : JS_Now()); > + lastModifiedSet ? lastModified * PR_USEC_PER_MSEC : JS_Now(), 0); > } here as well. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:557 (Diff revision 9) > /* static */ > double > nsRFPService::ReduceTimePrecisionAsUSecsWrapper(double aTime) > { > + // Timestamps that come in from the JS Engine are all absolute (I think!) > return nsRFPService::ReduceTimePrecisionImpl(aTime, MicroSeconds, TimerResolution(), 0, TimerPrecisionType::All); 80chars max.

Comment on attachment 8955634 [details] Bug 1440195 Add a random context seed for AudioContext and MediaStream https://reviewboard.mozilla.org/r/224728/#review232570 ::: dom/media/DOMMediaStream.h:743 (Diff revision 8) > // True if this stream only sets mActive to false when its playback stream > // finishes. This is a hack to maintain legacy functionality for playing a > // HTMLMediaElement::MozCaptureStream(). See bug 1302379. > bool mSetInactiveOnFinish; > > + uint64_t mRandomSeed; Does it need to be protected? move it to private.

Comment on attachment 8957656 [details] Bug 1440195 Actually integrate the context mix-in into the hash function calculation https://reviewboard.mozilla.org/r/226568/#review232572 ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:155 (Diff revision 4) > LRUCache() > : mLock("mozilla.resistFingerprinting.LRUCache") { > this->cache.SetLength(LRU_CACHE_SIZE); > } > > - nsCString Get(long long aKey) { > + nsCString Get(long long aKeyPart1, long long aKeyPart2) { can we change the style in the meantime? nsCString Get(long long aKeyPart1, long long aKeyPart2) { ...

Comment on attachment 8955635 [details] Bug 1440195 Add a random context seed to the Performance APIs https://reviewboard.mozilla.org/r/224730/#review232576 I would like to see something like a class implementing GetRandomSeed() method and having all these classes inherit it. ::: dom/events/Event.cpp:1108 (Diff revision 8) > if (mPresContext && mPresContext->Document() && mPresContext->Document()->NodePrincipal()) > isSystemCaller = nsContentUtils::IsSystemPrincipal(mPresContext->Document()->NodePrincipal()); > > if (!sReturnHighResTimeStamp) { > double ret = static_cast<double>(mEvent->mTime); > - return isSystemCaller ? ret : nsRFPService::ReduceTimePrecisionAsMSecs(ret); > + return isSystemCaller ? ret : nsRFPService::ReduceTimePrecisionAsMSecs(ret, 0); comment here this 0.

Comment on attachment 8956236 [details] Bug 1440195 Address CSS Animations https://reviewboard.mozilla.org/r/225144/#review232632 ::: layout/style/nsAnimationManager.cpp:249 (Diff revision 6) > - elapsedTime = nsRFPService::ReduceTimePrecisionAsSecs(elapsedTime); > + // 0 is an inappropriate value for this callsite. What we need to do is > + // use a single random value for all increasing times reportable. > + // That is to say, whenever elapsedTime goes negative (because an > + // animation restarts, something rewinds the animation, or otherwise) > + // a new random value for the mix-in must be generated. > + elapsedTime = nsRFPService::ReduceTimePrecisionAsSecs(elapsedTime, 0, TimerPrecisionType::RFPOnly); I don't really follow. We only reduce the precision for cancel events anyway (since they're the only ones that use real times) and you can only get two cancel events by re-starting and then re-canceling (and even that you need to use an unshipped API for). Furthermore, we're already getting uninvestigated intermittent failures from this code from the original reduced time precision patches prior to this bug so I wonder what is prompting the "0" here. Do we get intermittent failures without it? If so, these patches are probably just aggravating an existing bug. ::: layout/style/nsTransitionManager.cpp:270 (Diff revision 6) > - elapsedTime = nsRFPService::ReduceTimePrecisionAsSecs(elapsedTime); > + // 0 is an inappropriate value for this callsite. What we need to do is > + // use a single random value for all increasing times reportable. > + // That is to say, whenever elapsedTime goes negative (because an > + // animation restarts, something rewinds the animation, or otherwise) > + // a new random value for the mix-in must be generated. > + elapsedTime = nsRFPService::ReduceTimePrecisionAsSecs(elapsedTime, 0, TimerPrecisionType::RFPOnly); Likewise here.

For my reference, here are the intermittent failures that appear to be due to the existing time precision reduction steps that I was referring to in comment 55: bug 1443122, bug 1443574, bug 1443778.

(In reply to Andrea Marchesini [:baku] from comment #54) > I would like to see something like a class implementing GetRandomSeed() > method and having all these classes inherit it. I took this approach. I did not propagate the error. For one, the function signature would be weird; I'd be passing in a member as an out variable... I could probably fudge things around, but it wouldn't be very intuitive. Secondly, in the places this is called, there's no meaningful place to propagate the error too; none of the callers have a way to escalate it further, and many of these functions are bound to exposed javascript fields. (In reply to Andrea Marchesini [:baku] from comment #50) > ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:492 > (Diff revision 11) > > long long clamped = floor(double(timeAsInt) / resolutionAsInt) * resolutionAsInt; > > > > > > long long midpoint = 0, > > clampedAndJittered = clamped; > > - // RandomMidpoint uses crypto functions from NSS. But we wind up in this code _very_ early > > + if (sJitter && aType != TimerPrecisionType::VideoCalculation) { > > why only for !VideoCalculation? Why not doing this check in RandomMidpoint > instead, passing aType too? > > In general, it seems an hack... It is a hack. The ReduceTimePrecision functions were overloaded as a handy way to calculate the (spoofed) media statistics APIs. The clearest thing to do I think is duplicate the core two lines of logic in the (non-jittered) Reduce Time Precision function and copy it into the Spoof Video functions. that way I don't have to complicate the ReduceTimePrecision function with handling their stuff. (In reply to Brian Birtles (:birtles) from comment #55) > I don't really follow. We only reduce the precision for cancel events anyway > (since they're the only ones that use real times) and you can only get two > cancel events by re-starting and then re-canceling (and even that you need > to use an unshipped API for). > > Furthermore, we're already getting uninvestigated intermittent failures from > this code from the original reduced time precision patches prior to this bug > so I wonder what is prompting the "0" here. Do we get intermittent failures > without it? If so, these patches are probably just aggravating an existing > bug. There are two changes here, but I only mentioned the one. Putting a 0 here is because I want a function signature that requires a value (meaning, no default value.) I have to give it something. I don't know what to give it (I'm hoping I'll be able to help you figure that out.) So for now I give it a 0 even though that's the wrong value to give it. The other change is the important one: I change these two call sites to RFPOnly (Resist Fingerprinting Only). Now all CSS Animations are only clamped/jittered in Resist Fingerprinting mode. The intermittents should go away. I need to file the followup bug about correctly reducing the precision of CSS Animations (which will now encompass the cancel event as well.)

Oh, ok, that makes sense! Thanks!

Comment on attachment 8956236 [details] Bug 1440195 Address CSS Animations https://reviewboard.mozilla.org/r/225144/#review232674 ::: dom/animation/AnimationUtils.h:40 (Diff revision 7) > + // 0 is an inappropriate mixin for this this area; however CSS Animations needs to > + // have it's Time Reduction Logic refactored, so it's currently only clamping for > + // RFP mode. RFP mode gives a much lower time precision, so we accept the security > + // leak here for now > result.SetValue( > - nsRFPService::ReduceTimePrecisionAsMSecs(aTime.Value().ToMilliseconds(), TimerPrecisionType::RFPOnly) > + nsRFPService::ReduceTimePrecisionAsMSecs(aTime.Value().ToMilliseconds(), 0, TimerPrecisionType::RFPOnly) Please wrap to 80 lines. ::: layout/style/nsAnimationManager.cpp:254 (Diff revision 7) > - elapsedTime = nsRFPService::ReduceTimePrecisionAsSecs(elapsedTime); > + // 0 is an inappropriate value for this callsite. What we need to do is > + // use a single random value for all increasing times reportable. > + // That is to say, whenever elapsedTime goes negative (because an > + // animation restarts, something rewinds the animation, or otherwise) > + // a new random value for the mix-in must be generated. > + elapsedTime = nsRFPService::ReduceTimePrecisionAsSecs(elapsedTime, 0, TimerPrecisionType::RFPOnly); Line length. ::: layout/style/nsTransitionManager.cpp:275 (Diff revision 7) > - elapsedTime = nsRFPService::ReduceTimePrecisionAsSecs(elapsedTime); > + // 0 is an inappropriate value for this callsite. What we need to do is > + // use a single random value for all increasing times reportable. > + // That is to say, whenever elapsedTime goes negative (because an > + // animation restarts, something rewinds the animation, or otherwise) > + // a new random value for the mix-in must be generated. > + elapsedTime = nsRFPService::ReduceTimePrecisionAsSecs(elapsedTime, 0, TimerPrecisionType::RFPOnly); Line length.

Comment on attachment 8956236 [details] Bug 1440195 Address CSS Animations https://reviewboard.mozilla.org/r/225144/#review232674 > Please wrap to 80 lines. Err, 80 characters. :)

Comment on attachment 8954439 [details] Bug 1440195 Add a per-context mix-in value for our Timer Precision Reduction Functions https://reviewboard.mozilla.org/r/223520/#review232720 You should use mozilla::RelativeTimeline in the other patches, now. ::: toolkit/components/resistfingerprinting/RelativeTimeline.h:8 (Diff revision 13) > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#ifndef __RelativeTimeline_h__ > +#define __RelativeTimeline_h__ > + mozilla namespace here. ::: toolkit/components/resistfingerprinting/RelativeTimeline.cpp:25 (Diff revision 13) > + } > + > + uint8_t* buffer = nullptr; > + rv = randomGenerator->GenerateRandomBytes(sizeof(mRandomTimelineSeed), &buffer); > + if (NS_WARN_IF(NS_FAILED(rv))) { > + mRandomTimelineSeed = rand(); return rand(); ::: toolkit/components/resistfingerprinting/RelativeTimeline.cpp:26 (Diff revision 13) > + > + uint8_t* buffer = nullptr; > + rv = randomGenerator->GenerateRandomBytes(sizeof(mRandomTimelineSeed), &buffer); > + if (NS_WARN_IF(NS_FAILED(rv))) { > + mRandomTimelineSeed = rand(); > + } else { no else if you do a quick return rand() ::: toolkit/components/resistfingerprinting/RelativeTimeline.cpp:30 (Diff revision 13) > + mRandomTimelineSeed = rand(); > + } else { > + memcpy(&mRandomTimelineSeed, buffer, sizeof(mRandomTimelineSeed)); > + } > + if (buffer) { > + free(buffer); MOZ_ASSERT(buffer); if you do the quick return. ::: toolkit/components/resistfingerprinting/moz.build:18 (Diff revision 13) > > FINAL_LIBRARY = 'xul' > > EXPORTS += [ > 'nsRFPService.h', > + 'RelativeTimeline.h', EXPORTS.mozilla += [ 'RelativeTimeline.h', ] when you add the mozilla namespace.

Comment on attachment 8958027 [details] Bug 1440195 Do not check for NSS Initialization before performing jitter https://reviewboard.mozilla.org/r/226968/#review232852 I just had a thought - do we ever need jitter in the parent process? If not, then this all is moot - it's always safe to initialize NSS in the child process (since it runs in memory-only mode in child processes). If we do need jitter in the parent process but not for the system principal, then I thought the work you did to whitelist those calls was actually fairly promising - only some file blob and indexedDB operations were failing. It would require some work, but I'm fairly confident we could thread the appropriate context through those implementations so that we know when we're operating on the system principal. If we always need jitter, then I'm concerned this introduces technical debt that will give us a nasty surprise later ("didn't we implement jitter?" "yes - it's right here... oh..."). We do have some options to address this: * implement a cryptographically-secure source of randomness independent of NSS * modify NSS to expose a static csprng API (so it wouldn't even have to be initialized to use it) * modify PSM to be able to initialize NSS in memory-only mode when something only needs non-certificate/key APIs and load the databases on-demand * at the very least, check for the availability of the profile directory here so if NSS hasn't been initialized by this point, it can be if it's safe to do so (still not perfect, but maybe this would be sufficient) In any case, I'm not a peer on this module, so you don't need my r+. ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:533 (Diff revision 3) > long long midpoint = 0, > clampedAndJittered = clamped; > - // RandomMidpoint uses crypto functions from NSS. But we wind up in this code _very_ early > - // on in and we don't want to initialize NSS earlier than it would be initialized naturally. > + // RandomMidpoint uses crypto functions from NSS. Previously, we checked > + // NSS_IsInitialized assuming that because we wind up in this code _very_ early > + // on we didn't want to initialize NSS earlier than it would be initialized naturally. > // Doing so caused nearly every xpcshell test to fail, as well as Marionette. You might add that the underlying issue is that (in the parent process) if NSS is initialized before we have a profile directory, things that expect or require certain certificates and/or keys to be available will fail.

So I think I got this resolved nicely. Mostly. The massive number of non-system principal js calls to jitter that we had seen were red herrings. Workers don't have a principa set on them, so they weren't flagged as being System Principal. I got some advice and resolved the JS Engine part of Bug 1443943 so Workers in a system context don't clamp/jitter - and all of those calls went away. I didn't (and still don't) populate a principal for BlobImpl (or for DOMMediaStream or AudioContext) - but that doesn't seem to matter. https://treeherder.mozilla.org/#/jobs?repo=try&revision=777259cb0c7221f7e8c2f0c68974e985fd282b43 is a full try run with no checks for NSS being Initialized or anything like that. I searched the logs of every job for the NSS profile directory warning (I used https://gist.github.com/tomrittervg/9e99de9b3c517b8ba4e87d2a86985616 if you're curious) and found only one test that triggers this warning: browser/components/translation/test/browser_translation_bing.js - specifically the test_handling_out_of_valid_key_error part It makes sense, we wind up generating a call to get Date.now() https://searchfox.org/mozilla-central/rev/8fa0b32c84f924c6809c690117dbd59591f79607/browser/components/translation/BingTranslator.jsm#407 and for whatever reason this is must not be considered System code; and we wind up initializing NSS. It does not cause the test to fail though. I think this is okay. So in the last patch we should be able to drop the check for NSS_IsInitialized() and simply things.

Comment on attachment 8958027 [details] Bug 1440195 Do not check for NSS Initialization before performing jitter https://reviewboard.mozilla.org/r/226968/#review233534 Awesome!

I have a full try run, I think this is good to go. https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b76774a5826607c31500f183c71cf55ff6e1acc

Autoland can't push this while there are still pending unresolved issues in MozReview.

(In reply to Ryan VanderMeulen [:RyanVM] from comment #101) > Autoland can't push this while there are still pending unresolved issues in > MozReview. Sorry :(

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/23ac25f89e34 Add a per-context mix-in value for our Timer Precision Reduction Functions r=baku https://hg.mozilla.org/integration/autoland/rev/01d8021842f9 For timestamps that are absolute, specify a null context pointer r=baku https://hg.mozilla.org/integration/autoland/rev/90dd04c11480 Add a random context seed for AudioContext and MediaStream r=baku https://hg.mozilla.org/integration/autoland/rev/b67bb0e7edde Add a random context seed to the Performance APIs r=baku https://hg.mozilla.org/integration/autoland/rev/d79b4a990405 Address CSS Animations r=birtles https://hg.mozilla.org/integration/autoland/rev/afe8115a96df Actually integrate the context mix-in into the hash function calculation r=baku https://hg.mozilla.org/integration/autoland/rev/caa3c1fa4dc2 Do not check for NSS Initialization before performing jitter r=keeler

https://hg.mozilla.org/mozilla-central/rev/23ac25f89e34 https://hg.mozilla.org/mozilla-central/rev/01d8021842f9 https://hg.mozilla.org/mozilla-central/rev/90dd04c11480 https://hg.mozilla.org/mozilla-central/rev/b67bb0e7edde https://hg.mozilla.org/mozilla-central/rev/d79b4a990405 https://hg.mozilla.org/mozilla-central/rev/afe8115a96df https://hg.mozilla.org/mozilla-central/rev/caa3c1fa4dc2

Comment on attachment 8954439 [details] Bug 1440195 Add a per-context mix-in value for our Timer Precision Reduction Functions Approval Request Comment [Feature/Bug causing the regression]: The initial version of Jitter that landed (Bug 1425462) had a (known) security flaw. The flaw isn't memory corruption or anything like that, it's just that the jitter added was not completely random. This fixes that. [User impact if declined]: We might as well turn jitter off since it's not going to be secure. [Is this code covered by automated tests?]: Indirectly, yes. [Has the fix been verified in Nightly?]: Yes, it's baked for a few days [Needs manual test from QE? If yes, steps to reproduce]: No. [List of other uplifts needed for the feature/fix]: Bug 1443943 and Bug 1444588 [Is the change risky?]: No. [Why is the change risky/not risky?]: There is always a risk I got the patch wrong, but it's been reviewed and people seem to think it's good. [String changes made/needed]: None

Comment on attachment 8954439 [details] Bug 1440195 Add a per-context mix-in value for our Timer Precision Reduction Functions more timestamp work for beta60

https://hg.mozilla.org/releases/mozilla-beta/rev/e2434dc01519 https://hg.mozilla.org/releases/mozilla-beta/rev/f5fca57484d0 https://hg.mozilla.org/releases/mozilla-beta/rev/86ba2db234ae https://hg.mozilla.org/releases/mozilla-beta/rev/cf00a5369f69 https://hg.mozilla.org/releases/mozilla-beta/rev/0b094ed5c2f8 https://hg.mozilla.org/releases/mozilla-beta/rev/683adcd07af8 https://hg.mozilla.org/releases/mozilla-beta/rev/15e6ad92c29d