Skip to content
Commit 2e1e07e3 authored by Isaac Freund's avatar Isaac Freund Committed by Simon Ser
Browse files

ext-session-lock-v1: clarify to fix race

Clients such as swaylock [1] or waylock [2] provide options to fork and
detach from the controlling terminal when the session is locked. The
point of these options is avoid a race on suspending the system. A
command to suspend the system (e.g. zzz) may safely be chained with
e.g. waylock as so:

waylock -fork-on-lock && zzz

However, there is no guarantee that the compositor has actually
blanked all outputs before sending the locked event. Therefore there
is still a race as new "locked" frames may not have been presented on
all outputs before the system is suspended.

On my Linux system at least, the current framebuffer seems to be
preserved on suspend and restored on resume, leading to an "unlocked"
frame potentially being displayed when the system is resumed. Blanking
all outputs before suspend eliminates this vulnerability.

Currently clients could theoretically implement such -fork-on-lock
options a bit better if the compositor supports the presentation-time
protocol, however no clients I've seen currently do this and it seems
wise to make clients to do the right thing by default in this security
sensitive context. The presentation-time protocol is also not sufficient
in all cases, for example if the compositor has turned off power of an
output but still exposes it to clients. In this case the client would
wait forever to get a presentation feedback that will never come.

Unfortunately, the protocol currently states that the locked event will
be sent immediately on creation of the ext_session_lock_v1 object rather
than after all normal content is hidden.

Several different approaches have been considered for how to fix this in
the protocol specification.

One possibility would be to add a new event sent when all normal content
is hidden. This is however opt-in for clients and therefore less likely
to be properly implemented by all clients in practice.

Another alternative is to bump the version of the ext_session_lock_v1
interface and state that the semantics of when the compositor will send
the locked event. However, this still requires clients to opt-in by
binding version 2 of the interface. The compositor could technically
deny the attempts of any version 1 clients to lock the session, but this
would likely be a bad breaking change for users of version 1 clients.
While session lock clients should inform the user in some way that their
attempt to lock the session was denied (e.g. by exiting non-zero) it
does not seem to be the case that such exit codes are widely checked.

The option to fix the protocol that is all around the most secure is
changing the semantics of the locked event without bumping the version
of the interface. This is technically a breaking change, but the failure
mode is that a client relying on the locked event being sent immediately
hangs or crashes and the session stays locked.

I also have been unable to find any session lock client in the wild that
relies on the locked event being sent immediately.

The river wayland compositor [3] in fact already implements the fix for
this race condition since the 0.2.0 release and has not received any bug
reports about broken session lock clients yet.

Therefore, I think that making this technically breaking change to the
protocol is our all around best option in this situation. Prioritizing
security over compatibility seems like the right trade-off to make for a
security critical protocol.

[1]: https://github.com/swaywm/swaylock
[2]: https://github.com/ifreund/waylock
[3]: https://github.com/riverwm/river



Signed-off-by: Isaac Freund's avatarIsaac Freund <mail@isaacfreund.com>
parent 5dc6efde
Loading
Loading
Loading
Pipeline #805120 passed with stages
in 26 seconds
Loading
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment