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 <mail@isaacfreund.com>

1 files changed, 36 insertions, 8 deletions

diff --git a/staging/ext-session-lock/ext-session-lock-v1.xml b/staging/ext-session-lock/ext-session-lock-v1.xml
index 3bb3a09..3f73fdd 100644
--- a/staging/ext-session-lock/ext-session-lock-v1.xml
+++ b/staging/ext-session-lock/ext-session-lock-v1.xml
@@ -65,8 +65,8 @@
 
   <interface name="ext_session_lock_v1" version="1">
     <description summary="manage lock state and create lock surfaces">
-      On creation of this object either the locked or finished event will
-      immediately be sent.
+      In response to the creation of this object the compositor must send
+      either the locked or finished event.
 
       The locked event indicates that the session is locked. This means
       that the compositor must stop rendering and providing input to normal
@@ -78,6 +78,30 @@
       at the compositor's discretion, special privileged surfaces such as
       input methods or portions of desktop shell UIs.
 
+      The locked event must not be sent until a new "locked" frame (either
+      from a session lock surface or the compositor blanking the output) has
+      been presented on all outputs and no security sensitive normal/unlocked
+      content is possibly visible.
+
+      The finished event should be sent immediately on creation of this
+      object if the compositor decides that the locked event will not be sent.
+
+      The compositor may wait for the client to create and render session lock
+      surfaces before sending the locked event to avoid displaying intermediate
+      blank frames. However, it must impose a reasonable time limit if
+      waiting and send the locked event as soon as the hard requirements
+      described above can be met if the time limit expires. Clients should
+      immediately create lock surfaces for all outputs on creation of this
+      object to make this possible.
+
+      This behavior of the locked event is required in order to prevent
+      possible race conditions with clients that wish to suspend the system
+      or similar after locking the session. Without these semantics, clients
+      triggering a suspend after receiving the locked event would race with
+      the first "locked" frame being presented and normal/unlocked frames
+      might be briefly visible as the system is resumed if the suspend
+      operation wins the race.
+
       If the client dies while the session is locked, the compositor must not
       unlock the session in response. It is acceptable for the session to be
       permanently locked if this happens. The compositor may choose to continue
@@ -123,8 +147,9 @@
         This client is now responsible for displaying graphics while the
         session is locked and deciding when to unlock the session.
 
-        Either this event or the finished event will be sent immediately on
-        creation of this object.
+        The locked event must not be sent until a new "locked" frame has been
+        presented on all outputs and no security sensitive normal/unlocked
+        content is possibly visible.
 
         If this event is sent, making the destroy request is a protocol error,
         the lock object must be destroyed using the unlock_and_destroy request.
@@ -144,10 +169,13 @@
         be sent because the compositor implements some alternative, secure
         way to authenticate and unlock the session.
 
-        Either this event or the locked event will be sent exactly once on
-        creation of this object. If the locked event is sent on creation of
-        this object, the finished event may still be sent at some later time
-        in this object's lifetime, this is compositor policy.
+        The finished event should be sent immediately on creation of this
+        object if the compositor decides that the locked event will not
+        be sent.
+
+        If the locked event is sent on creation of this object the finished
+        event may still be sent at some later time in this object's
+        lifetime. This is compositor policy.
 
         Upon receiving this event, the client should make either the destroy
         request or the unlock_and_destroy request, depending on whether or