wayland: Privileged protocol security #208
Conversation
Drakulix
force-pushed
the
panel-sec-context_jammy
branch
from
ee2462c to
d6f2c81
Compare
|
At some point we'll likely want to not just have a binary For development, would it make sense to have a command line argument or env var to disable enforcement of privileged protocols? I'd still note that we can't expect this to actually be secure for apps that aren't sandboxed, but it's good not to encourage apps to use sensitive compositor-specific protocols that aren't available in Flatpak, etc. |
Right, I think the only reasonable thing to do here is get_client_credentials and try to match the binary. (Sadlyy this is not portable.) If we are reasonably sure it is the same the user originally allow-listed, we can allow additional protocols. Check should e.g. include the binary is not user-writable imo. If we figure out the flatpak-sandbox is actually the flatpak binary, we can also match on the app-id to allow specific protocols.
Yeah I also thought about just generally enabling all protocols for debug builds. Though those might be too slow for debugging, so an environment variable sounds like a good idea. |
|
Yeah. The BSDs all seem to support Restricting it to debug builds also isn't ideal because someone working on the panel, lock screen, or an applet doesn't necessarily want to have to rebuild the compositor. |
Drakulix
force-pushed
the
panel-sec-context_jammy
branch
from
d6f2c81 to
5cdfe46
Compare
Probably this should also be a toggle from the debug interface. Maybe we should make a "debug-interface wishlist" issue at some point. Anyway Maybe just in the debug-interface would be enough until we have proper configuration for security aspects? |
Drakulix
force-pushed
the
panel-sec-context_jammy
branch
from
5cdfe46 to
0b66651
Compare
Drakulix
changed the title
|
Ok, this PR now only includes inheriting the privileged flag for security-context clients already having it (and allowlisted for cosmic-panel for now), as well as a |
src/state.rs
Outdated
| XWaylandKeyboardGrabState::new::<Self>(&dh); | ||
| PointerConstraintsState::new::<Self>(&dh); | ||
| PointerGesturesState::new::<Self>(&dh); | ||
| SecurityContextState::new::<Self, _>(&dh, client_has_security_context); | ||
| SecurityContextState::new::<Self, _>(&dh, client_should_see_privileged_protocols); |
There was a problem hiding this comment.
This would prevent flatpak from using the security context protocol if COSMIC_ENABLE_WAYLAND_SECURITY is set, since it wouldn't be started with a privileged socket. Would it be better to use client_has_no_security_context here?
Though it doesn't make a difference until we start taking into account the security context of Flatpak apps.
There was a problem hiding this comment.
Would it be better to use client_has_no_security_context here?
Right, that wasn't on purpose, changing it back.
Though it doesn't make a difference until we start taking into account the security context of Flatpak apps.
Well it should already block all "privileged" protocols. Though once we enforce security there is indeed no difference here, but given there might be in the future this is still a good change.
| if let Err(err) = state.common.display_handle.insert_client( | ||
| client_stream, | ||
| Arc::new(ClientState { | ||
| security_context: Some(security_context.clone()), | ||
| privileged: privileged | ||
| && security_context.sandbox_engine.as_deref() | ||
| == Some("com.system76.CosmicPanel"), |
There was a problem hiding this comment.
We may want to further restrict which privileged protocols an applet has access to (define required permissions in desktop files?). But this is reasonable for now.
(And for that to really be secure, we'd also want to sandbox applets.)
There was a problem hiding this comment.
Defining necessary protocols in the desktop file would already be a good idea, I agree.
Drakulix
force-pushed
the
panel-sec-context_jammy
branch
from
0b66651 to
9fd521d
Compare
Drakulix
force-pushed
the
panel-sec-context_jammy
branch
from
9fd521d to
9576154
Compare
Draft until everything is updated to not break. Perhaps this should be split in two commits. One that enables security-contexts to inherit the privileged flag and one that enables all the enforcing, that can be merged later.
In any case this breaks parts of the desktop today, but once cosmic-panel makes use of security-context applets could even enjoy more privileges. E.g. always creating valid xdg-activiation tokens via #207.