1586761 - Check crossOriginIsolated for all nsRFPService::ReduceTimePrecision* callers

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Anne (:annevk)]

Once bug 1477743 lands we have the infrastructure in place to bring back a high-resolution performance.now() if the correct headers are set and a COOP+COEP process is created. I.e., a process that allows SharedArrayBuffer in postMessage().

This would be good to to do at that point as making folks use SharedArrayBuffer as a workaround seems cumbersome and wasteful.

Comment 1

[Anne (:annevk)]

Tom, apparently there's a single switch in our code protecting high-resolution timers such as performance.now(). That's probably what we want to toggle here. Could you provide some context for whoever ends up working on this?

Comment 2

[Tom Ritter [:tjr]]

So presently all our code looks like this:

if(!systemPrinciapl) {
   timestamp = nsRFPService::ReduceTimePrecision*(timestamp,  ..., TimerPrecisionType:All);
}

Inside nsRFPService::ReduceTimePrecision* (which call nsRFPService::ReduceTimePrecisionImpl) we check our prefs and decided what to do to the timestamp; in order:

1. privacy.resistFingerprinting enabled - clamp to 100ms or the value of privacy.resistFingerprinting.reduceTimerPrecision.microseconds - whichever is greater
2. privacy.reduceTimerPrecision enabled - clamp to the value of privacy.resistFingerprinting.reduceTimerPrecision.microseconds
3. privacy.reduceTimerPrecision.unconditional enabled - clamp to 5usec
4. Nothing enabled - do not clamp

• privacy.resistFingerprinting.reduceTimerPrecision.jitter enabled - perform jitting if (1) or (2) from above is active.

We pass a TimerPrecision Type into the function to indicate that for a few callsites in Animations, we only clamp for RFP mode and nothing else.

For this bug I think what we'll want to do is change

if(!systemPrinciapl) {
   timestamp = nsRFPService::ReduceTimePrecision*(timestamp,  ..., TimerPrecisionType:All);
}

into

auto precisionType = systemContext ? 
                                       TimerPrecisionType:System : 
                                       (COOPCOEPPass ? TimerPrecisionType:HighResAllowed : TimerPrecisionType:Normal)
timestamp = nsRFPService::ReduceTimePrecision*(timestamp, ..., precisionType);
// ::Normal is renamed from ::All
// The ergonomics of this leave a bit to be desired - open to suggestions.

And then inside ReduceTimePrecisionImpl

1. TimerPrecisionType:System - no clamping
2. TimerPrecisionType:HighResAllowed - clamp to 5usec if privacy.reduceTimerPrecision.unconditional else no clamping
3. TimerPrecisionType:Normal / TimerPrecisionType:RFPOnly - the current behavior from above

Comment 3

[Anne (:annevk)]

Minor change since last I looked at this: COOPCOEPPass above is now CrossOriginIsolated.

Comment 4

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1586761 - P1 - Expose CrossOriginIsolated to nsIGlobalObject;

Comment 5

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1586761 - P2 - Introduce new TimerPrecisionTypes and a set of new Reduce methods to decide the TimerPrecisionType in the nsRFPService;

Depends on D63238

Comment 6

[Tom Tung [:tt, :ttung]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ca804b30b8cb4613c720335ac01732b0ec59257

Comment 7

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1586761 - P3 - Support CrossOriginIsolated for Worklet;

Comment 8

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1586761 - P3 - Use new methods in dom/performance;

Note that this patch implements a member function CrossOriginIsIsolated in
PerformanceWorker and PerformanceMainThread. In PerformanceMainThread, we need
to cache boolean for CrossOriginIsIsolated() so that we don't need to find the
owning global on every callsites.

Depends on D63293

Comment 9

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1586761 - P4 - Use new methods in dom/file and dom/events;

For the usecases in dom/file, I'm not so sure how to get the global at
callsites, so I let aIsSystemPrincipal and aCrossOriginIsolated to be false for
now.

Depends on D63904

Comment 10

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1586761 - P5 - Use new methods in media

Depends on D63905

Comment 11

[Tom Tung [:tt, :ttung]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e87b73828cdb07d05d59417012adf85ca2a5b5a0

Comment 12

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1586761 - P6 - Check CrossOriginIsolated for ReduceTimePrecisionAsUSecsWrapper;

Comment 13

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1586761 - P7 - Remove the Reduce* functions with a parameter for TimerPrecisionType;

Comment 14

[Tom Tung [:tt, :ttung]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d364a2da31124be1b4f74253cf718471ccc4538

Comment 15

[Tom Tung [:tt, :ttung]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=931d3c4c6b191ef7b7fe0dd5d772d50c4ec6a1ca

Comment 16

[Tom Tung [:tt, :ttung]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3666d40e119c2c6e35db86242d8e1b421404e10e

if this looks good, I will file a follow-up bug for testing and consider landing these patches.

Comment 17

[Tom Tung [:tt, :ttung]]

I probably misinterpret this suggection, try before change: https://treeherder.mozilla.org/#/jobs?repo=try&revision=74a8bfa22e7086c13584c47453f177c5f2f0adda

Comment 18

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt, :ttung] from comment #16)

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3666d40e119c2c6e35db86242d8e1b421404e10e

if this looks good, I will file a follow-up bug for testing and consider landing these patches.

Not sure if the ni in Phabricator works or not so I needinfo :tjr again here.
:tjr, changing if (!unconditionalClamping && aContextMixin == 0 && aType == TimerPrecisionType::All && timeAsInt < kFeb282008) to if (aContextMixin == 0 && timeAsInt < kFeb282008 && !unconditionalClamping) breaks most of jobs on try.

For instance, if I run "dom/animation/test/mozilla/test_deferred_start.html". It would trigger this call and the current holding time is 0. Since the aContextMixin is 0 in this case, it entered the condtion (aContextMixin == 0 && timeAsInt(which is 0) < kFeb282008).

Should I check whether the TimerPrecisionType is normal or not here? Or, do you have any suggestions for fixing this? I will address that on another patch, then.

Comment 19

[Tom Ritter [:tjr]]

Sorry, I typed a response in phab but didn't submit it....

Comment 20

[Tom Tung [:tt, :ttung]]

While running an xpcshell test (netwerk/test/unit/test_protocolproxyservice.js), I hit this assertion(https://searchfox.org/mozilla-central/rev/7d0c94a0e9a9fe1f83553f49b10128567d21709d/js/xpconnect/wrappers/WrapperFactory.cpp#841-845).

The log for that is: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=292691966&repo=try&lineNumber=3506

I believe this is caused by D64324 and it probably means I cannot access the global object or context on non-main thread.
Jan, I found you have touched the code around, so I guess you might know why I hit this assertion? Could you help me to check if my understanding is correct? Or, if it's incorrect, maybe you have more insight into the meaning of that assertion?

If it's, then one solution I can think of is to let RealmCreationOptions carry the CrossOriginIsolated when it's constructed. Or, do you have any suggestions for fixing it? The goal is to check crossOriginIsolated from the global object.

Thanks in advance!

Comment 21

[Tom Tung [:tt, :ttung]]

try after applying tjr's comment: https://treeherder.mozilla.org/#/jobs?repo=try&revision=78ea7e7be38d0fc975024f13c70c7a074d6addb5&selectedJob=292861726

So there are two issues so far and I mentioned one in comment#20.

Another is /web-animations/timing-model/animations/setting-the-start-time-of-an-animation.html, I will check this tomorrow.

Comment 22

[Tom Ritter [:tjr]]

When we removed IsTimerPrecisionReductionEnabled we also removed the check that looked at the RFP pref.

So in ReduceTimePrecisionImpl you should add

 bool unconditionalClamping = false;
+if (aType == RFP  && !StaticPrefs::privacy_resistfingerprinting) {
+   aType = UnconditionalAKAHighRes;
+}
 if (aType == UnconditionalAKAHighRes || TimerResolution() <= 0) {

(ni? to ensure you see the comment)

Comment 23

[Tom Tung [:tt, :ttung]]

(In reply to Tom Ritter [:tjr] (ni for response to sec-[approval|rating|advisories|cve]) from comment #22)

When we removed IsTimerPrecisionReductionEnabled we also removed the check that looked at the RFP pref.

So in ReduceTimePrecisionImpl you should add

 bool unconditionalClamping = false;
+if (aType == RFP  && !StaticPrefs::privacy_resistfingerprinting) {
+   aType = UnconditionalAKAHighRes;
+}
 if (aType == UnconditionalAKAHighRes || TimerResolution() <= 0) {

(ni? to ensure you see the comment)

Ah, that fixes the second issue in comment 21. I will prepare one more patch to make it easier to be reviewed. Thanks!

Comment 24

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1586761 - P7 - Revert a few incorrect changes on P2;

P2 removed IsTimerPrecisionReductionEnabled and thus removed the check for RFP
pref. While most ReduceTimePrecision* functions are fine with that because
GetTimerPrecisionType checks that, the two ReduceTimePrecision*RFP functions
miss the check.

This patch mainly cover the check for that two functions and rename them to
*RFPOnly since they only use RFP when the pref is on.

Comment 25

[Tom Tung [:tt, :ttung]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=88ebb117d7c12572b7b1e8184cb3523c63cec8b8

Comment 26

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt, :ttung] from comment #25)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=88ebb117d7c12572b7b1e8184cb3523c63cec8b8

The Gtest failed because it hit the debug assertion I added to ensure the pref for "RFP" enables when it's TimerPrecisionType::RFP. I have a patch to fix it locally.

/css/css-animations/CSSAnimation-startTime.tentative.html failed. I suspect it's because the logic gets change due to P2. I will check the logic in ReduceTimePrecisionImpl again.

Comment 27

[Jan de Mooij [:jandem]]

(In reply to Tom Tung [:tt, :ttung] from comment #20)

I believe this is caused by D64324 and it probably means I cannot access the global object or context on non-main thread.
Jan, I found you have touched the code around, so I guess you might know why I hit this assertion? Could you help me to check if my understanding is correct? Or, if it's incorrect, maybe you have more insight into the meaning of that assertion?

Sorry for the delay, I didn't notice this NI last week. Unfortunately Treeherder is down right now so I can't look at the log, but as far as I can tell it should be fine to call NativeGlobal on worker threads. It would really help to see a stack trace, I'll keep the NI and look at that when Treeherder is back online...

Comment 28

[Jan de Mooij [:jandem]]

Oh bleh, it's the ProxyAutoConfig thread :/ Yeah, that's a JS runtime that's completely independent from other DOM machinery so we can't use xpc::NativeGlobal there. Using RealmCreationOptions is probably our safest option here.

Comment 29

[Tom Tung [:tt, :ttung]]

(In reply to Jan de Mooij [:jandem] from comment #28)

Oh bleh, it's the ProxyAutoConfig thread :/ Yeah, that's a JS runtime that's completely independent from other DOM machinery so we can't use xpc::NativeGlobal there. Using RealmCreationOptions is probably our safest option here.

I see. Thanks for the explanation!

It seems that ProxyAutoConfig is the main process thing which means even if we are in the main thread the crossOriginIsolated is false in theory. Also, if it's independent from DOM machinery, then I guess that time cannot go backward in this case (we were worried about this because timers are clamped by different factors).

Luke, D64324 doesn't take care of all the cases properly. For instance, this can be called on ProxyAutoConfig thread. I will prepare a patch to fix that, but I have some questions:

• Is this the only case that is not running on the main thread or a worker thread? If it's the only case which we need to take care of additionally, then maybe we can check if it's in the main process or not. Or something like that.
• Is there a way to check if we can get a global from a JSContext so that we can assume the CrossOriginIsolated is false when the check fails rather than hitting an assertion.

Comment 30

[Luke Wagner [:luke]]

• ProxyAutoConfig is the only other one I'm aware of. (searchfox seems to confirm.)
• Can you point me to the particular JS API you're using to get a global? In general, JSContexts don't "have a global", but can be queried for the currently-executing-global in various ways, but that can be "none" for a variety of reasons (like no running code).

Comment 31

[Tom Tung [:tt, :ttung]]

(In reply to Luke Wagner [:luke] from comment #30)

• ProxyAutoConfig is the only other one I'm aware of. (searchfox seems to confirm.)

I see. (I just learn that we can know this by checking the callers for JS_NewContext. Thanks!)

• Can you point me to the particular JS API you're using to get a global? In general, JSContexts don't "have a global", but can be queried for the currently-executing-global in various ways, but that can be "none" for a variety of reasons (like no running code).

I'm using CurrentNativeGlobal(cx) to get a global from given JSContext in D64324 line 607 in toolkit/components/resistfingerprinting/nsRFPService.cpp.

Then, I guess I should probably do:

// toolkit/components/resistfingerprinting/nsRFPService.cpp
  nsCOMPtr<nsIGlobalObject> global = xpc::CurrentNativeGlobal(aCx);
-  MOZ_ASSERT(global);
+ const bool crossOriginIsolated = global ? global->CrossOriginIsolated() : false;
  const auto type = GetTimerPrecisionType(/* aIsSystemPrincipal */ false,
-                                         global->CrossOriginIsolated());
+.                                        crossOriginIsolated

However, I would still hit this assertion. Do you have any suggestion to avoid hitting this debug assertion? (like checking thread or attribute of the JSContext)

Comment 32

[Jan de Mooij [:jandem]]

Maybe you could add something like this:

bool xpc::HasNativeGlobal(JSContext* cx) {
  JSObject* obj = CurrentGlobalOrNull(cx);
  constexpr unsigned flags = JSCLASS_PRIVATE_IS_NSISUPPORTS | JSCLASS_HAS_PRIVATE;
  return (GetObjectClass(obj)->flags & flags) == flags || dom::UnwrapDOMObjectToISupports(obj);
}

Ideally sharing code with the MOZ_ASSERT in NativeGlobal.

Comment 33

[Tom Tung [:tt, :ttung]]

(In reply to Jan de Mooij [:jandem] from comment #32)

Maybe you could add something like this:

bool xpc::HasNativeGlobal(JSContext* cx) {
  JSObject* obj = CurrentGlobalOrNull(cx);
  constexpr unsigned flags = JSCLASS_PRIVATE_IS_NSISUPPORTS | JSCLASS_HAS_PRIVATE;
  return (GetObjectClass(obj)->flags & flags) == flags || dom::UnwrapDOMObjectToISupports(obj);
}

Ideally sharing code with the MOZ_ASSERT in NativeGlobal.

Ah, I see. (I wasn't so sure if I can do this since I don't have enough understanding of the code around there) Will provide a patch then. Thanks!

Comment 34

[Jan de Mooij [:jandem]]

Or it might be simpler to initialize CrossOriginIsolated to false if XRE_IsParentProcess() is true, without calling NativeGlobal in that case?

Comment 35

[Tom Tung [:tt, :ttung]]

(In reply to Jan de Mooij [:jandem] from comment #34)

Or it might be simpler to initialize CrossOriginIsolated to false if XRE_IsParentProcess() is true, without calling NativeGlobal in that case?

Yes, I have considered that but that probably depends on whether we will have another exception on the content process. Because it assumes the only exception (which is ProxyAutoConfig) is on the parent process.

That's why I'm asking if there is a preferable way to fix this.

Comment 36

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1586761 - P8 - Set setClampAndJitterTime to false for ProxyAutoConfig to avoid getting the global from a JSContext on the parent process;

Depends on D66734

Comment 37

[Luke Wagner [:luke]]

I think it makes sense for IsCrossOriginIsolated to return false when !JS::CurrentGlobalOrNull(cx); that condition might arise within the content process in corner cases and it makes sense to default to safety.

However, within the parent process, we shouldn't ever be executing untrusted content JS. However, we might be running trusted JS that may legitimately want to use SAB and I think we don't have to guard against Spectre attacks in this situation. So I think it'd be fine to return true when XRE_IsParentProcess().

Comment 38

[Tom Tung [:tt, :ttung]]

(In reply to Luke Wagner [:luke] from comment #37)

Thanks for the feedback! I update P8 to address the comment.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb2e8d1db3a17a851b1d7ecb7ac719064019914b

Hopefully, the remaining test failures are in toolkit/components/passwordmgr/test/unit/ and I'm investigating why my patches cause them to fail.

Comment 39

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt, :ttung] from comment #38)

Hopefully, the remaining test failures are in toolkit/components/passwordmgr/test/unit/ and I'm investigating why my patches cause them to fail.

I haven't figured the reason why but it's caused by P3 and the code below:

 DOMHighResTimeStamp Performance::Now() {
   DOMHighResTimeStamp rawTime = NowUnclamped();
-  if (mSystemPrincipal) {
-    return rawTime;
-  }
-
-  return nsRFPService::ReduceTimePrecisionAsMSecs(rawTime,
-                                                  GetRandomTimelineSeed());
+  return nsRFPService::ReduceTimePrecisionAsMSecs(
+      rawTime, GetRandomTimelineSeed(), mSystemPrincipal,
+      CrossOriginIsolated());
 }

I tried to get some information in a failed test (test_LoginManagerParent_doAutocompleteSearch.js) and I got:

• It's about os.file.copy and the code which uses performace.now()
• !!mSystemPrincipal is true in Performance::Now, so ReduceTimePrecisionAsMSecs returns the same value as rawTime.
• I haven't figured out why but if I remove the if-check and returns rawTime by ReduceTimePrecisionAsMSecs as what the change does, it takes longer between performance.mark and performance.measure and that seems to cause the unit test to fail.

Comment 40

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt, :ttung] from comment #39)

• I haven't figured out why but if I remove the if-block and returns rawTime by ReduceTimePrecisionAsMSecs as what the change does, it takes longer between performance.mark and performance.measure and that seems to cause the unit test to fail.

Hmm, if I revert the change here, there is no test failure....

Comment 41

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1586761 - P9 - Revert a change for performance.now to avoid causing functions in pkcs11f.h to fail;

Depends on D67446

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b51663641450
P1 - Expose CrossOriginIsolated to nsIGlobalObject; r=baku
https://hg.mozilla.org/integration/autoland/rev/85317d582166
P2 - Introduce new TimerPrecisionTypes and a set of new Reduce methods to decide the TimerPrecisionType in the nsRFPService; r=tjr
https://hg.mozilla.org/integration/autoland/rev/1e09b1a3a7ee
P3 - Use new methods in dom/performance; r=tjr,baku
https://hg.mozilla.org/integration/autoland/rev/f185ed882616
P4 - Use new methods in dom/file and dom/events; r=baku
https://hg.mozilla.org/integration/autoland/rev/8d554870dce8
P5 - Use new methods in media r=tjr,baku
https://hg.mozilla.org/integration/autoland/rev/886102df9840
P6 - Check CrossOriginIsolated for ReduceTimePrecisionAsUSecsWrapper; r=tjr,luke
https://hg.mozilla.org/integration/autoland/rev/228fb5abeca5
P7 - Revert a few incorrect changes on P2; r=tjr
https://hg.mozilla.org/integration/autoland/rev/8a727b228708
P8 - Set setClampAndJitterTime to false for ProxyAutoConfig to avoid getting the global from a JSContext on the parent process; r=luke,tjr
https://hg.mozilla.org/integration/autoland/rev/6819eae19dfd
P9 - Revert a change for performance.now to avoid causing functions in pkcs11f.h to fail; r=tjr

Comment 43

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/b51663641450
https://hg.mozilla.org/mozilla-central/rev/85317d582166
https://hg.mozilla.org/mozilla-central/rev/1e09b1a3a7ee
https://hg.mozilla.org/mozilla-central/rev/f185ed882616
https://hg.mozilla.org/mozilla-central/rev/8d554870dce8
https://hg.mozilla.org/mozilla-central/rev/886102df9840
https://hg.mozilla.org/mozilla-central/rev/228fb5abeca5
https://hg.mozilla.org/mozilla-central/rev/8a727b228708
https://hg.mozilla.org/mozilla-central/rev/6819eae19dfd

Comment 44

[Anne (:annevk)]

This is great, thanks all and Tom in particular! I filed https://github.com/w3c/hr-time/issues/89 to get this standardized.