Don't allow an unrelated client to unlock the session

[kelnos]

Description

Here's the "attack" sequence that was possible:

1. Legit lock client binds to interface, sends lock, creates lock surfaces, and does things as expected. Session is locked.
2. Malicious (or just buggy) client binds to the interface and sends lock. Smithay calls SessionLockHandler::lock(). Here the compositor does have enough information; it can know that the session is already locked, and that it's a different client requesting to lock, so it can drop the SessionLocker instance, which causes smithay to emit finished on the protocol object.
3. But finished is not a destructor, so the client still has a live ext_session_lock_v1 object. It can immediately request unlock_and_destroy on it.
4. Smithay's handler for unlock_and_destroy unconditionally clears the list of lock surfaces, and calls SessionLockHandler::unlock(). unlock() takes no parameters, so the compositor has no idea what client is asking to unlock the session. Presumably it will unlock the session, without the user having authenticated.

One possible mitigation the compositor could do (which doesn't require changes to smithay) would be to immediately send a protocol error to the client after step #2, which would disconnect the client entirely. I don't think that's a very nice solution, though, as it's possible the client isn't malicious or even buggy, but just raced the client that successfully locked the screen. Regardless, this would be a huge foot-gun for compositors; it's not obvious or clear that the compositor should have to do this, and arguably smithay should just do the thing that is correct in all circumstances.

So, this change:

1. Moves lock_status to SessionLockManagerState, since whether or not the session is locked is a property of the compositor/manager, not of a specific ExtSessionLockV1 instance.
2. lock_status is now an enum that records the ExtSessionLockV1 instance that the compositor allowed to successfully lock the session.
3. If another ExtSessionLockV1 instance tries to unlock the session, an error is sent back (invalid_unlock), the internal list of lock surfaces is not cleared, and unlock() is not called on the compositor's handler.

Checklist

• I agree to smithay's Developer Certificate of Origin.

[kelnos]

I realized there's also an issue (perhaps exacerbated by the first commit in this PR) where the locked_outputs vec on SessionLockManagerState isn't the right way to do things. So I pushed a second commit to fix that up as well.

[Drakulix]

2. Malicious (or just buggy) client binds to the interface and sends `lock`.  Smithay calls `SessionLockHandler::lock()`.  Here the compositor does have enough information; it can know that the session is already locked, and that it's a different client requesting to lock, so it can drop the `SessionLocker` instance, which causes smithay to emit `finished` on the protocol object.

3. But `finished` is not a destructor, so the client still has a live `ext_session_lock_v1` object.  It can immediately request `unlock_and_destroy` on it.

It feels like we should mark the object as "done" on finished and either ignore or protocol error on any other request to it. (I would go for ignore, as we might be racing in-flight requests, but unlock_and_destroy is invalid either way for an object that never received a locked event.)

We need to make sure, that we don't break a new lock client taking over after an old one died. So this still needs to work:

1. A well-behaved client locks the session.
2. At some point later it crashes and thus never sends unlock or destroy.
3. The compositor will now not have a lock surface anymore and likely render an empty screen.
4. A new lock client connects (either because the compositor or some other DE-compoenent restarted it or because the user did somehow) and needs to be able to successfully acquire the locked state.
5. The session gets recovered by the new client sending unlock_and_destroy.

[kelnos]

Ok, here's another pass at this.

For starters, I simplified a few things in my original implementation (mainly, SessionLockState doesn't need its own arc-clone of lock_status, as the places that need it can get it through the handler state).

Added the "done" flag. It's mainly just used now to decide whether or not to call functions on SessionLockHandler, as we shouldn't bother the compositor with requests that come from a supposed-to-be-inert instance.

A judgment call I made: the get_lock_surface handler still does nearly everything it used to, even for an instance that is "done". The only thing it doesn't do is call the handler or send a configure. Since the client could legitimately send get_lock_surface after the compositor has sent finished, but before the client has actually received/processed that event, and we're not sending an error, I think it makes sense to keep things consistent and actually set up the surface as a the client suggested, even if it's a sort of "stub" that doesn't do anything. Happy to do something else here if you think that's not the way to go.

Regarding allowing a new locker to take over if an old locker crashes... I looked through the old code, and I think that was only supported by accident. There was never anything that would ever set lock_status back to false (even unlock_and_destroy left it as true), but there was also never anything that checked that it was false in the lock/locked path, so clients and the compositor could lock and relock and it would still work, no matter what the current state was.

The new code did end up breaking this, so I implemented Dispatch2::destroyed for ExtSessionLockV1/SessionLockState: if lock_status is Locked, and the owning instance of that lock is the instance being destroyed, lock_status gets reset to Unlocked. It does not call SessionLockHandler::unlock, as there's no way for the compositor to distinguish a true unlock vs. a crash through that path.

Do we need a SessionLockHandler::locker_disconnected trait function? I'm thinking maybe no: my compositor tracks Client instances and notices when the client currently locking the session disconnects, and responds appropriately, and I think that's probably fine for compositors to do? The one thing that makes me uncertain here is that "destroyed ExtSessionLockV1 without unlocking" and "locking client disconnected" aren't technically the same thing. Because of how the protocol is defined, if the client tries to actively destroy the locked ExtSessionLockV1 instance without unlocking, they get a protocol error and are disconnected, so in practice it is the same thing, but is that safe to rely on?

[Drakulix]

A judgment call I made: the get_lock_surface handler still does nearly everything it used to, even for an instance that is "done". The only thing it doesn't do is call the handler or send a configure. Since the client could legitimately send get_lock_surface after the compositor has sent finished, but before the client has actually received/processed that event, and we're not sending an error, I think it makes sense to keep things consistent and actually set up the surface as a the client suggested, even if it's a sort of "stub" that doesn't do anything. Happy to do something else here if you think that's not the way to go.

I think that is fine.

The new code did end up breaking this, so I implemented Dispatch2::destroyed for ExtSessionLockV1/SessionLockState: if lock_status is Locked, and the owning instance of that lock is the instance being destroyed, lock_status gets reset to Unlocked. It does not call SessionLockHandler::unlock, as there's no way for the compositor to distinguish a true unlock vs. a crash through that path.

Setting it to "Unlocked" is kind of a misnomer, if the session very much is still locked, even if this does the correct thing. I wonder if we should introduce a Defunct state just to make sure we don't introduce any new bugs later.

Do we need a SessionLockHandler::locker_disconnected trait function? I'm thinking maybe no: my compositor tracks Client instances and notices when the client currently locking the session disconnects, and responds appropriately, and I think that's probably fine for compositors to do?

Yeah, I wouldn't add that, if nobody is asking for it.

The one thing that makes me uncertain here is that "destroyed ExtSessionLockV1 without unlocking" and "locking client disconnected" aren't technically the same thing. Because of how the protocol is defined, if the client tries to actively destroy the locked ExtSessionLockV1 instance without unlocking, they get a protocol error and are disconnected, so in practice it is the same thing, but is that safe to rely on?

I think it is, we end up in the defunct state since the protocol explicitly forbids it. If the client is misbehaving either by crashing or handling the protocol wrong the compositor cannot know the intent and has to prioritize the users privacy.

[kelnos]

Setting it to "Unlocked" is kind of a misnomer, if the session very much is still locked, even if this does the correct thing. I wonder if we should introduce a Defunct state just to make sure we don't introduce any new bugs later.

Yes, in my compositor I use Orphaned for that state. Inside smithay it doesn't matter right now, since Unlocked and Defunct would be treated the same way, but I agree it's a good idea to have the extra state in case future work needs to know the difference.

Ok, updated!

[Drakulix]

LGTM now! Thanks :)