TL;DR: * Two objects that are parts of the same stream can live in different compartments. * Nuking can therefore break internal edges of a stream. * We should either prevent this or have test coverage for it. Streams can have internal slots that can contain cross-compartment wrappers. Of these slots, ReadableStream.[[storedError]] is really not a problem; but these other slots might be: ReadableStream.[[reader]] ReadableStream{Default,BYOB}Reader.[[ownerReadableStream]] ReadableStream{Default,BYOB}Reader.[[closedPromise]] the elements of ReadableStreamDefaultReader.[[readRequests]] and ReadableStreamBYOBReader.[[readIntoRequests]] Any one of these wrappers could be individually nuked (turned into a DeadObjectProxy) while everything else in the system is unchanged. For example, the stream.[[reader]]->reader wrapper can be nuked while the writer.[[ownerReadableStream]]->stream edge is just fine. This is so weird that it breaks an assertion in the standard! (See step 2 of <https://streams.spec.whatwg.org/#readable-stream-reader-generic-release>. We actually implement that assertion, so probably we have at least one bug here.) We should, when accessing these slots, simply throw an exception if we see a dead object. Fine, then we won't crash--but we'd better test that. And even then, nuking multiplies the state space of streams, and streams are already complicated. It seems likely some state machine in the spec could get stuck in a broken state that the standard didn't anticipate. Seems bad. Since in practice Gecko only nukes chrome-to-content edges, we could cut the knot here by banning streams from having any internal edges that cross such trust boundaries.

Comment 0 is framed in terms of nuking, but I'm also worried about algorithms being broken because internal CheckedUnwrap calls fail. Same example -- suppose we have a chrome reader for a content stream. Chrome can unwrap reader.[[onwerReadableStream]], but the stream's compartment can't unwrap stream.[[reader]]. Same fix: ban streams from having internal edges that cross such trust boundaries.

<bholley> jorendorff: this stuff is a bit too paged out for me to give you useful synchronous guidance. I think you should ask bz, and if he doesn't know, the two of us should sit down and piece it together from first principles <jorendorff> bholley: Agreed. [...]

> Since in practice Gecko only nukes chrome-to-content edges Not quite true. Gecko also nukes edges in/out of sandboxes in some cases (we have explicit API for this), and there's something about window teardown and addons. See the two calls to xpc::NukeAllWrappersForCompartment in the tree. In a world in which we stop exposing non-transparent wrappers to web content we could conceivably forbid all cases when the wrapper is not a transparent CCW. What's not clear to me is how this affects Xrays for streams, nor how those should generally behave. I suspect that given how the streams spec is written Xrays for streams are not really possible without either having cross-trust-boundary wrappers or violating the spec in some way.

Suppose we're chrome code and we have an Xray wrapper to a content window. We want to call xrayContentWindow.fetch() and read from the response body. I'm not sure what our Xray support for promises is like, but I believe the rest of that should work fine. Note that this use case does *not* require any cross-domain edges of the sort I am worried about! The stream, controller, reader, and all relevant buffers and promises live together in the content domain, and we access whatever we need using Xray wrappers. To say that another way: `xrayContentResponse.body.getReader()` will call the getReader method in the content compartment. The new reader will be a content object.

> `xrayContentResponse.body.getReader()` will call the getReader method in the content compartment. It will? Why?

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #5) > It will? Why? I thought only some DOM Xrays got the magic "method runs in the caller compartment" behavior. For normal JSNatives I thought the default behavior was to enter the compartment where the method itself lives (in this case, I think that's the content compartment) and rewrap all arguments before calling it.

Of course, if `xrayContentResponse.body.getReader()` doesn't run in the content compartment, none of the rest of comment 4 works. We would get xrays in internal slots, and I can't vouch for that working. The spec is huge.

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #3) > In a world in which we stop exposing non-transparent wrappers to web content > we could conceivably forbid all cases when the wrapper is not a transparent > CCW. Setting aside Xrays, is that the world we are living in? Or close enough?

> For normal JSNatives I thought the default behavior was to enter the compartment where the method itself lives When we do .getReader (no call yet) on an Xray to a stream (presumably that's what xrayContentResponse.body got us), we land in TryResolvePropertyFromSpecs in js/xpconnect/wrappers/XrayWrapper.cpp. This walks the JSFunctionSpec* it got from the JSClass (in this case presumably ReadableStream_methods) until it finds the right name. Then it creates a new JSFunction in the caller (xray) compartment from that JSFunctionSpec and calls it. There's some caching involved, but that's the basic idea. So yes, we are entering the compartment where the method lives, but the method lives in the caller compartment to start with. So unless I'm missing something, in the Xray case we would end up creating the reader in the caller compartment. This is the basic mismatch between Xrays and specs that create objects in the current Realm popping up to bite us again... :( > Setting aside Xrays, is that the world we are living in? It's not yet, but we're trying to get to that world, based on the work jandem has been doing... One problem there is that arrival of that world is uncertain schedule-wise.

Thanks for explaining this yet again, bz. ---- As it stands, I think even basic functionality does not work cross-domain. For example, suppose a content stream has a chrome reader. The source detects end-of-file; it calls JS::ReadableStreamClose to tell the JS engine. This fails because the engine tries to unwrap stream.[[reader]] in order to notify the reader; but the current compartment isn't allowed to unwrap the more-privileged reader. https://searchfox.org/mozilla-central/rev/007b66c1f5f7a1b9a900a2038652a16a020f010c/js/src/builtin/Stream.cpp#1417 The state machine stalls: the stream is closed, but the chrome consumer is never notified; its ongoing read remains outstanding forever. This is speculation. I haven't tried it. If all that is right, I could change these particular CheckedUnwrap calls to UncheckedUnwrap, if appropriate; but auditing the whole file for this kind of possibility is a daunting amount of work. Therefore I want to instead check, whenever we create cross-compartment edges internal to streams, and throw if the new edge would cross into a compartment with different principals. Same-domain wrappers would continue to work fine. With cross-domain wrappers, we would fail early and in an obvious way, rather than leave this code exposed to a large, untested state space.

Sorry for the lag here.. > and throw if the new edge would cross into a compartment with different principals That seems ok. We'll need to figure out document.domain, but hopefully the changes we're making there will help. Basically, we want to only allow creating a reader if the stream's principal subsumes the reader's principal, right? The converse is always true, since we got to the "create a reader" part. That said, it sounds like this would basically completely break Xrays to streams, but those might already not be working? I wish we had tests for this stuff. Or do we?

Of course another option would be is that if the stream's principal does not subsume the principal we plan to create the reader with then we create the reader in the stream's compartment. That case can't happen on the web, so there should be no web-observable fallout. But it would maybe make Xrays to streams work.

> Basically, we want to only allow creating a reader if the stream's principal subsumes the reader's principal, right? The converse is always true, since we got to the "create a reader" part. Data flow between the reader and the stream is bidirectional. I think for things to work reliably, each compartment's principal must subsume the other's. (?) > That said, it sounds like this would basically completely break Xrays to streams, but those might already not be working? I wish we had tests for this stuff. Or do we? We only have tests for same-principal cross-compartment cases, as far as I know.

bz and I discussed this briefly this morning. (In reply to Jason Orendorff [:jorendorff] from comment #13) > Data flow between the reader and the stream is bidirectional. I think for > things to work reliably, each compartment's principal must subsume the > other's. (?) Boris agrees that this is the right invariant, and was just pointing out that only one check is necessary, the other being redundant with CheckedUnwrap calls we're already doing. The plan is as follows. In this bug: - Ban cross-domain stream readers, by adding an extra principals check when creating a reader. - Write both document.domain tests and Web extension tests for the case where this initial check fails. - Test the document.domain case where this initial check passes, but (under document.domain's current behavior) we fail later, one of the windows having changed its document.domain. - Look at edges other than the stream.[[reader]] edge, decide what must be done now, and file follow-ups. In follow-up bugs: - Test that the state machine doesn't get stuck when we break internal edges by nuking. In these cases, each part of the broken object graph should get into an error state eventually; we shouldn't leave either side waiting forever. In the case of Promises, this may be impossible, but other cases should be fixable if broken. - Figure out how to make X-ray streams work. This might involve carefully selecting PromiseResolve calls in the spec where the target Promise is certain to have been created by the streams implementation itself, and allowing those calls to affect that Promise regardless of domain.

I forgot how much time it takes to get a mochitest working. Still plugging away at the list above. So far so good. It looks like extensions can be tested from xpcshell, great news for me: https://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/test_ext_contentscript_xrays.js