-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Restrict when MessagePort's onclose event can fire #10201
Comments
@asutherland Same-origin is defnitely safe. I'm not sure how much same-origin MessagePort usage there is since same-origin has more direct methods available. I wonder if we could also fire it
So then at least we would cover
"no handler" also potentially leaks some info. Unclear to me if that would also be considered a problem. |
What's Chrome's solution for it all being observable through Web Locks as well? At least when the |
@annevk I'm not familiar with that. Since WebLocks are per-origin, I don't see how it leaks information cross-origin (which is Chrome Security's concern). As for emulation, yes, I believe this demo demonstrates that this information is already pretty readily available. However, the concern was about explictly baking it in. I would argue that the difference is slight but then maybe that's why I'm not on the security team... |
But that demo can be defeated by tying the lifetime of |
I'm not sure what you mean by defeated. Perhaps you're trying the demo in Safari which GCs aggressively. The demo does quite different things in different browsers.
So in Safari, tying the port to the global would keep it alive, presumably until navigation but that is exactly Chrome Security's concern. I don't think there's a great resolution to his that preserves the current spec, unless someone has a good argument why leaking this GC (which may or may not coincide with navigation) doesn't really matter. |
I mean that we should align on the Gecko model. |
It should be possible to provide a service over |
@annevk To be clear, you are proposing that we spec that MessagePorts should be leaked and not GCed when they are cross-site? If we do that are OK with collecting them when they have had a listener added? |
If by "leaked" you mean tied to the lifetime of the relevant global, sure. I don't really understand your second question as there's a grammar error. If you meant to write "are you OK" I don't understand why adding a listener would make them more collectable. It seems that would make them less collectable, but I'm probably missing something. |
The intention is to find circumstances where the port can be disentangled without an explicit close. There have been a couple of suggestions usage
opt-inAdd a boolean attribute like |
Looking at this again I wonder how Firefox defeats the attack. I was misunderstanding something initially. What does Firefox never collect? Maybe @asutherland can help out here. I think an opt-in makes sense. I do wonder about the OP's suggestion of having different defaults for same and cross-origin. That is kinda counter to treating message passing as a capability-based mechanism as it's written to be in the specification. (With a notable exception for |
In Firefox, all shipped MessagePorts go through our IPC. When our IPC layer is processing the implicit close for the collected port:
Honestly, it's not clear this behavior was entirely intentional (we will certainly spew NS_WARNINGs in the parent process when this case happens if the closed port sends us messages); the code has a bunch of technical debt and is due for a cleanup. But I think I might have avoided changing the behavior in a few bugs because it seemed like it might have been equally been intentional to avoid leaking GC, but it's hard to tell because our infrastructure makes it hard to find old discussion of things. |
What is the issue with the HTML Standard?
As described here security concerns mean that Chrome will not implement
onclose
as specced. TL;DR it bakes into the standard a signal that garbage collection has occurred in the holder of the other end of the port.Suggestion from @asutherland:
The text was updated successfully, but these errors were encountered: