Skip to content
This repository has been archived by the owner on Jan 25, 2022. It is now read-only.

Document the implications of the absense of a same-Realm check for WeakRefs #52

Open
marjakh opened this issue Nov 20, 2018 · 7 comments

Comments

@marjakh
Copy link
Contributor

marjakh commented Nov 20, 2018

makeCell and makeRef rely on the sameRealm operation:

"If SameRealm(target, this), then ..."

Why does it make sense to treat the reference strongly? What happens then - the cleanup function just never gets called, and the WeakCell stays alive forever (unless it's explicitly cleared)?

In addition, we don't have the SameRealm operation in V8 and when I talked with the engineers responsible for this area, they said "we don't want the objects to know which Realm they come from", that is, they think the SameRealm operation should not be possible and should not end up in the spec.

@tschneidereit
Copy link
Member

See #10, #35, tc39/ecma262/issues/1333, and the relevant section of weakrefs.md for some context.

cc @erights, @dtribble.

[T]hey think the SameRealm operation should not be possible and should not end up in the spec.

Just to be able to better interpret this statement: is this a matter of technical feasibility, or an opinion? I.e., would it theoretically be feasible for V8 to implement this, if the arguments for doing so are convincing, or is it simply not an option because of V8's architecture?

@marjakh
Copy link
Contributor Author

marjakh commented Nov 22, 2018

Re: https://mail.mozilla.org/pipermail/es-discuss/2013-January/028542.html - interesting! I think it would be better to throw an error for attempting cross-realm references than to silently do a strong reference. An unintentional silent strong reference creates potential hard to debug bugs: it ends up keeping the object alive, also the WeakCell is kept alive unless the user does clear().

Re: whether it's feasible or not, I'll relay this question to folks who can answer it better than me :)

@marjakh
Copy link
Contributor Author

marjakh commented Nov 22, 2018

Re: feasibility of SameRealm:

V8 currently has a function creationContext https://cs.chromium.org/chromium/src/v8/src/objects.cc?type=cs&q=creationcontext&g=0&l=3973 which is kinda related but not necessarily the same as SameRealm.

You'd need to spec what's the Realm in all possible cases and as the spec is not written out, we're not sure how exactly it's supposed to work.

Depending on how you spec SameRealm, we might or might not be able to use creationContext. If not, the object needs to store its ream which increases memory usage. This will also happen if we want a fast implementation of SameRealm.

Example (copy-paste from another discussion): "There are some tricky cases that we'd currently get "wrong", like creating an object with Reflect.construct with a new.target that's a function without a jsobject .prototype. What realm it's suppose to belong to? Because you can in Realm A use a function from realm B without prototype and a base class from realm C."

@erights
Copy link
Contributor

erights commented Apr 14, 2019

From offline discussions at the last tc39, it seems as if v8 (chrome) and jsc (safari) cannot fesibly support SameRealm. This means that the weakrefs possible on those platforms create the security hazard explained at https://github.com/tc39/proposal-weakrefs/blob/d2f8ebcc2303f859f7fba781b569f48b9dd4fe7c/history/weakrefs.md#cross-realm-references , a severe form of inter-realm side-channel and covert-channel leakage.

At the same time, because of the needs of JS/wasm interoperation, they will proceed to implement weakrefs anyway, and therefore ignoring these security considerations.

As we all know, this is effectively a veto on SameRealm. We are still better off codifying weakrefs in tc39 with this vulnerability than we are to avoid codifying it, and see it become part of de-facto JavaScript anyway likely with even worse security.

Given that we proceed in this manner, we must be very clear about this vulnerability. We should provide a standard means for an implementation to indicate that it does not have this inter-realm leakage, so that a shim can know it does not need to engage in extraordinary measures to protect itself from this leakage.

Where in the current documentation are these leakage dangers explained? Is there anything more recent than https://github.com/tc39/proposal-weakrefs/blob/d2f8ebcc2303f859f7fba781b569f48b9dd4fe7c/history/weakrefs.md#cross-realm-references ? The historical links at https://github.com/tc39/proposal-weakrefs#historical-documents all seem to be broken.

@allenwb
Copy link
Member

allenwb commented Apr 14, 2019

@erights
Copy link
Contributor

erights commented Apr 14, 2019

@allenwb thanks!

@littledan littledan changed the title SameRealm not specified / no consensus whether we should have it Document the implications of the absense of a same-Realm check for WeakRefs Apr 16, 2019
@littledan
Copy link
Member

Now that we have consensus on Stage 3 with no realm check, is anyone who sees security implications planning on documenting them, or would it be better to just close this issue?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants