openui / open-ui

Maintain an open standard for UI and promote its adherence and adoption.
https://open-ui.org
Other
3.59k stars 191 forks source link

[Invokers] Security implications? #904

Open bahrus opened 1 year ago

bahrus commented 1 year ago

This seems like a great idea. The only concern I have is if there are any security implications? It doesn't seem out of line with other attributes (like specifying the target for a form), but I wonder if this should be an attribute that is stripped from HTML Sanitizing algorithms by default? Or maybe not?

lukewarlow commented 1 year ago

I can't think of how this is introducing any new attack vectors* off the top of my head?

Having said that it's definitely something that'd be good to get more thoughts on.

* Aside from the full screen action which would be a new capability when JS is disabled. That one potentially needs some thought put into it.

lukewarlow commented 1 year ago

https://demo.lukewarlow.dev/fullscreen/ - These two iframes demonstrate a potential new security risk of the fullscreen actions within a sandboxed iframe.

Currently fullscreen API requires javascript so a sandbox without "allow-scripts" won't be able to do go fullscreen. Where as with invoketargets they don't need this.

There's various potential mitigations for this if we want to, ranging from requiring scripts when it's in a sandboxed frame, to blocking these actions within an iframe entirely.

cc @keithamus

lukewarlow commented 1 year ago

"Would these attributes be subject to CSP, sandboxing or specific HTML sanitizer behaviour at all?" - a question raised in the Mozilla I2P.

mfreed7 commented 1 year ago

This is an interesting question, and one that was (kind of) raised for the popover popovertarget attribute: https://github.com/whatwg/html/issues/4633#issuecomment-733785375. The concern there was that the popover feature allowed a declarative way to put content on top of the page, which was previously only possible via javascript (requestFullscreen).

I'm still not sure "no-JS" implies strong guarantees about rendering or even state changes like expanding or closing a <details> element. If user-provided HTML needs to be rendered, sanitizers must be present and well-configured to strip out anything not explicitly on an allow-list. The new invoketarget and interesttarget attributes should not be on that allow-list. (And if the sanitizer is only using a deny-list, then it's already broken.)

The one place where it does seem like CSP/sandboxing should apply is iframes. But shouldn't existing scripting protections keep someone from connecting an invoker in one frame to an invokee in another frame? That can't be done with idrefs. I don't quite understand the iframe attack vector.

css-meeting-bot commented 1 year ago

The Open UI Community Group just discussed [Invokers] Security implications?, and agreed to the following:

The full IRC log of that discussion <jarhar> masonf: i could look at usecounter to see how sites are using it
<bkardell_> q+
<gregwhitworth> ack bkardell_
<Luke> q+
<jarhar> keithamus: opened an itp for firefox for invokers
<jarhar> keithamus: someone raised security concerns, tim from apple has also raised concerns about security
<jarhar> keithamus: this gives buttons new capabilities to fullscreen parts of the page or iframes and stuff like that
<jarhar> keithamus: as it stands today, there are no restrictions on invoketarget, so it just does the thing
<gregwhitworth> q+
<jarhar> keithamus: luke and i explored the iframe situation and the new capability that it currently has is if you have a sandboxed iframe that is allowed fullscreen but disallowed scripting, then an invoker can make an element fullscreen
<jarhar> keithamus: theres maybe a concern there
<jarhar> keithamus: the only way to do that today is to allow scripting to have an onclick handler
<jarhar> keithamus: i dont know if we want to handle each of these separately?
<jarhar> keithamus: is popover a concern? i dont think so, we already have popovertarget
<gregwhitworth> q-
<jarhar> keithamus: is dialog a concern? like popover but focus traps. is that enough for an opt in or out?
<jarhar> keithamus: more controversial one is playing of media elements and fullscreen
<masonf> q+
<scotto_> q+
<gregwhitworth> ack Luke
<jarhar> Luke: keith gave a good overview, my feeling is that popover dialog details are no additional concern. webpages already have access to the innerHTML attribute that does focus trapping, they could do stuff if they wanted to
<jarhar> Luke: theres showpicker which we have disallowed in cross origin iframes, so no concern there, main thing is that pickers can escape iframes
<jarhar> Luke: banning them in cross origin deals with that
<scotto_> q-
<jarhar> Luke: playback of audio and video arent new capabilities because media controls let you do that without js, but i guess now you can style things to look different but that might already be possible with pseudo elements
<jarhar> Luke: the only one i find concerning is fullscreen api with sandboxed iframes
<jarhar> Luke: same origin sandboxed iframes you dont event need ??? because you already have same origin iframes by default
<jarhar> Luke: the main thing is the cross origin fullscreen sandboxed iframe where you dont allow scripting
<jarhar> Luke: if you allow fullscreen and sandboxed it then youve explicitly allowed fullscreen
<jarhar> Luke: idk if this warrants CSP, not sure if thats the right thing, and if they already have sanitizers then they could remove this
<jarhar> Luke: should we have restrictions for fullscreen api? if its a cross origin iframe then we could make it not work
<gregwhitworth> ack masonf
<jarhar> masonf: this came up for popovertarget, and the concern was that top layer itself was something that you couldnt access before without js, and now you can
<jarhar> masonf: my argument is the same: allow javascript is not a security blocker for these things
<dbaron> s/blocker/boundary/
<jarhar> masonf: thats a bad precedent we should not continue. everything we are doing is moving stuff out of javascript, dont want to paint ourselves into a corner
<jarhar> masonf: there is a sandbox thing for allow fullscreen
<jarhar> masonf: if you are disabling script and disabling fullscreen then you can do that and it will work
<jarhar> masonf: my sense is using the word "security" for this feels wrong
<gregwhitworth> +100
<keithamus> q?
<keithamus> q+
<jarhar> masonf: we should make sure there arent corner cases, but generally speaking allowing details to expand and contract without js is fine, same for media and fullscreen with the right sandbox flag
<gregwhitworth> ack dbaron
<jarhar> dbaron: it wasnt clear to me what the concern about fullscreen was
<jarhar> dbaron: did it have something to do with things like how its dependent on a user interaction?
<jarhar> dbaron: there are things like fullscreen and one of the others - theres generally a requirement for some user interaction and not something that you can purely trigger via js
<jarhar> dbaron: its important to make sure that this isnt a way around that
<Luke> q+
<jarhar> dbaron: existing mechanisms for ensuring that its coming from a real user interaction should work, possibly with modifications, and beyond that idk what the concern is
<gregwhitworth> +1
<masonf> +1, well said
<jarhar> dbaron: if thats not it, it would be good to get more details
<gregwhitworth> ack keithamus
<jarhar> keithamus: the invokers by design require user activation
<jarhar> keithamus: you have to click the button, and thats the only way you can trigger an invoker
<jarhar> keithamus: the potential vector for attack is that the iframe has a button that they get the user to click and then they fullscreen
<jarhar> keithamus: several layers of security restrictions have been lifted, and the user has made the interaction before we get to the concering part
<jarhar> keithamus: the difference is that this allows - you can disallow scripting and this can still happen
<jarhar> dbaron: you said it requires user interaction? i would have expected things like sending a click event or calling the click method would still invoke it
<gregwhitworth> ack dbaron
<Zakim> dbaron, you wanted to react to keithamus
<jarhar> Luke: it requires a trusted event and js isnt trusted
<jarhar> dbaron: thats interesting and a little surprising
<jarhar> masonf: the behavior can be discussed, but it does sidestep everything here
<gregwhitworth> ack Luke
<gregwhitworth> chair: masonf
<jarhar> Luke: i dont think there are any actual security issues with this, but it has come up, and it was a novel capability for fullscreen
<jarhar> Luke: main concern is iframes trying to do dodgy fishing things with the fullscreen api
<brecht_dr> q+
<jarhar> Luke: given it does require the user to physically click within the iframe, but doesnt require anything more than that
<jarhar> Luke: they can just press the escape key and get out of it
<jarhar> Luke: fullscreen does get used for that
<jarhar> Luke: could come up when trying to standardize in whatwg
<jarhar> Luke: could say in whatwg that csp is not appropriate and go from there
<masonf> ack brecht
<jarhar> brecht_dr: thinking the same thing for a while. if phishing would be an issue. i also started thinking that maybe others would have bad intentions from the start - if they have bad intentions they have a way anyway, so do we make it easier in some way? is that the question?
<jarhar> brecht_dr: not a bigger security risk, but maybe easier to be malicious?
<jarhar> masonf: do we need a resolution? or should we post back on whatwg and see what the response is?
<jarhar> una: we can resolve, were all settling around the same agreement
<keithamus> Proposed resolution: We consider the current implementations to be sufficiently secure (details of which will be shared on the whatwg issue).
<Luke> +1
<una> LGTM
<keithamus> RESOLVED: Proposed resolution: We consider the current implementations to be sufficiently secure (details of which will be shared on the whatwg issue).
keithamus commented 1 year ago

Please see https://github.com/openui/open-ui/pull/942 which goes into some detail around the security considerations of the proposal.

github-actions[bot] commented 6 months ago

There hasn't been any discussion on this issue for a while, so we're marking it as stale. If you choose to kick off the discussion again, we'll remove the 'stale' label.