1782853 - Address nsIGlobalObject's ShouldRFP function

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Tom Ritter [:tjr]]

It's a virtual method, and inherited classes do the right thing, but the parent class does not, and the parent class doesn't seem to be abstract.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:dveditz, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 2

[Tom Ritter [:tjr]]

Alternately, as suggested in D151298 we could go all-in on GlobalObjects and sure it's RFP function as the canonical source...

Comment 3

[Tom Ritter [:tjr]]

Okay, I understand this class a bit better now, and will be investigating going the other direction and making ShouldRFP safe and always do the right thing here.

Comment 4

[Tom Ritter [:tjr]]

Okay, I've hit my first major issue. I sent in this patch to force child objects to implement ShouldRFP so I could be sure they were doing the right thing. I got a pretty orange try run, but they all seem to have the same stack:

 0  libxul.so!SandboxPrivate::ShouldResistFingerprinting() const [SandboxPrivate.h:6869742d524a2ea700071257df95bcf5507fd919 : 97 + 0x11]
 1  libxul.so!nsContentUtils::GetRTPCallerType(nsIGlobalObject*) [nsContentUtils.cpp:6869742d524a2ea700071257df95bcf5507fd919 : 2124 + 0x8]
 2  libxul.so!mozilla::dom::Blob::Constructor(mozilla::dom::GlobalObject const&, mozilla::dom::Optional<mozilla::dom::Sequence<mozilla::dom::OwningArrayBufferViewOrArrayBufferOrBlobOrUSVString> > const&, mozilla::dom::BlobPropertyBag 
 3  libxul.so!mozilla::dom::Blob_Binding::_constructor(JSContext*, unsigned int, JS::Value*) [BlobBinding.cpp: : 771 + 0x4]
 4  libxul.so!InternalConstruct(JSContext*, js::AnyConstructArgs const&, js::CallReason) [Interpreter.cpp:6869742d524a2ea700071257df95bcf5507fd919 : 693 + 0xd7]
 5  libxul.so!Interpret(JSContext*, js::RunState&) [Interpreter.cpp:6869742d524a2ea700071257df95bcf5507fd919 : 3359 + 0x48]
 6  libxul.so!js::RunScript(JSContext*, js::RunState&) [Interpreter.cpp:6869742d524a2ea700071257df95bcf5507fd919 : 430 + 0xa]
 7  libxul.so!js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) [Interpreter.cpp:6869742d524a2ea700071257df95bcf5507fd919 : 578 + 0x7]
 8  libxul.so!js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) [Interpreter.cpp:6869742d524a2ea700071257df95bcf5507fd919 : 645 + 0xf]
 9  libxul.so!js::SpreadCallOperation(JSContext*, JS::Handle<JSScript*>, unsigned char*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) [Interpreter.cpp:6869742d524a2ea700071257df95bcf5507fd919 : 5138 + 0x18]
10  libxul.so!Interpret(JSContext*, js::RunState&) [Interpreter.cpp:6869742d524a2ea700071257df95bcf5507fd919 : 3302 + 0x17]

So SandboxPrivate is involved with Blobs. It seems from the callstack that a SandboxPrivate is creating a Blob, and when the Blob asks the SandboxPrivate 'Hey, do you RFP' the SandboxPrivate asserts. Is the answer to tell the SandboxPrivate at time of construction whether it ShouldRFP?

Looking at SandboxPrivate I see that it has a Principal associated with it that can be provided at construction time. E.g. here's a principal that flows into it, and here's another. So it seems like there could be content principals flowing in here.

However I can't just use a content principal to determine RFP status - there's a reason that approach is labeled as dangerous. Specifically, if I have a domain exempted.com that is exempted from RFP but exempted.com is framed by evil.com (which is not exempted) - then exempted.com is not exempted in that circumstance. This is achieved by checking CookieJarSettings which is created once for the top frame, and propagates to all subframes.

So I'm trying to figure out how to determine if a SandboxPrivate should ResistFingerprinting or not. If an exempted domain creates a SandboxPrivate that runs script, then I guess we should exempt the SandboxPrivate from RFP. But only if the SandboxPrivate is constructed by an exempted domain that is actually exempted and not overridden by e.g. being framed by a non-exempted domain. The examples I linked above of principals that flow down into the SandoxPrivate don't seem to have a readily available Document/Channel/LoadInfo which are more reliable objects to use to check RFP.

Am I thinking about this right? This is my first foray into nsIGlobalObject's at all...

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

I understand there's generally 3 idioms where sandboxes get used:

• "junk globals" where we need to use a JS API but we don't want to side-effect the current window or do things under the system principal for hygiene reasons. A notable characteristic of this pattern is that we're just trying to call JS APIs and we never want to evaluate content-provided script in a junk global. XPCJSRuntime::UnprivilegedJunkScope is the canoncial main-thread version of this.
  • The worker script loader code you link is a slightly fancier example of this. The worker wants to use the DOM Cache API on the behalf of a content worker thread from the main thread. Since JS globals are tied to a thread, a new global must be created (or reused).
  • Another interesting edge case is when IndexedDB has to add a new index it creates a sandbox to deserialize objects for evaluation in. This will bring blobs into existence, for example.
• "content script"-like situations where the sandbox is sitting in a privileged context and exists to introspect the global it's wrapping but it's not technically a window global or a worker global. content (or extension) script is explicitly evaluated in these. ExtensionContent.jsm does this for both content scripts and user scripts.
• hacky privileged module loaders. devtools' worker-loader.js is an example of this. Note, however, that when this is used for workers, it's actually using SimpleGlobalObject.
  • I understand the SandboxPrivate mechanism to be a main-thread only construct that's coupled with XPConnect. There also exists the aforementioned SimpleGlobalObject which is used to:
    • do some DOM binding stuff
    • be an off-main-thread (and therefore on-worker) mechanism for the debugger to create sandboxes for its module import system. This ideally goes away with a migration to ESM thanks to Yulia's work.

In general I think it's probably reasonable to have globals like this just default to resisting fingerprinting if it's enabled. The most interesting situation is web extension content scripts, but that's a discussion that should probably be had with the web extensions team. It definitely seems like the case that there's no way the sandbox constructor has enough information to understand the manner in which the sandbox will be used as things exist right now. https://chat.mozilla.org/#/room/#xpcom:mozilla.org is probably the right place to discuss most potential changes to the API.

Comment 6

[Tom Ritter [:tjr]]

Okay, I'm getting closer to feeling I have all the ScriptGlobal stuff rounded out, but I am hung up on a comment here.

Actually, scratch that, I think the existing code here is correct, because it would actually be nice if we can get rid of WorkerPrivate::ShouldResistFingerprinting in favor of all the callers using nsIGlobalObject instead.

Presently, the nsIGlobalObject override of ShouldRFP in WorkerGlobalScopeBase is just asking workerPrivate->ShouldRFP() which is forwarding to mLoadInfo. Having WorkerGlobalScopeBase forward to mWorkerPrivate seems to be a pattern.

It seems like from your comment we would ideally drop WorkerPrivate::ShouldRFP and rely on WorkerGlobalScopeBase::ShouldRFP via calls to mWorkerPrivate->AsGlobal()->ShouldRFP() wherever necessary. Would that be by adding the shouldRFP boolean member to WorkerGlobalScopeBase, initialized during construction? Or did you have something else in mind?

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Tom Ritter [:tjr] from comment #6)

Would that be by adding the shouldRFP boolean member to WorkerGlobalScopeBase, initialized during construction? Or did you have something else in mind?

This sounds great, thank you.

The underlying goal is that we want to try and eliminate code that has WorkerPrivate pointers, especially raw ones. We have a mechanism in place that can help detect retained nsIGlobalObject references that should have been cleaned up at worker shutdown, but retained WorkerPrivate references cause all kinds of problems. Saving off the value so it's more explicitly located with the global that owns it seems like a great step in that direction.

Comment 8

[Tom Ritter [:tjr]]

Okay, I've put the patches that do this toward the top of the stack in Bug 1778510

Comment 9

[Tom Ritter [:tjr]]

Fixed in Bug 1778510