Client serials are not correctly validated on set_selection

swaywm / sway

i3-compatible Wayland compositor

https://swaywm.org

MIT License

14.54k stars 1.11k forks source link

Client serials are not correctly validated on set_selection #4079

Open elinorbgr opened 5 years ago

elinorbgr commented 5 years ago

On sway 1.0 release.

The serials provided by clients when they issue a wl_data_device.set_selection call are apparently not validated other than checking that they are strictly greater than the previous one. This has two consequences:

A single client cannot use the same serial twice in a row to change the selection without losing and re-gaining focus in between.
A malicious client can craft a very large serial (for example but adding 1000000 to the serial it received on wl_keyboard.enter). This serial will be accepted by sway, and it will make all other clients unable to set the selection (as their serial is too small).

These issues were discovered with the help of @trimental, while working on https://github.com/smithay/smithay-clipboard

emersion commented 5 years ago

Regarding the first point, IIRC clients I've seen in the wild use the latest serial (not the wl_keyboard.enter serial). The protocol says: "serial number of the event that triggered this request" so I think this is correct behaviour.

Regarding the second one, currently wlroots only uses (weak) serial validation for protecting against race conditions and not as a security mechanism. Of course, I agree this should be fixed too. This probably requires to have a ring buffer of valid serials for each client?

Note that all of this also applies to Weston, which has a similar serial validation mechanism. Not sure about other compositors.

elinorbgr commented 5 years ago

Noted about the first point, I was not sure what to think about it.

About the second, a possibility would be to only allow setting the selection if the client currently has keyboard focus, and resetting the selection_serial of a seat whenever the keyboard focus change.

I suppose races can be mitigated by keeping the previous selection_serial as well. This is obviously not as robust as keeping track of all valid serials, though.

emersion commented 5 years ago

We still need to have a global selection_serial to prevent races between clients.

One thing we could do to protect against malicious clients is checking serial <= wl_display_get_serial().