GNOME Bugzilla – Bug 784206
wl_keyboard::keymap fd is shared and can be modified from any client
Last modified: 2018-10-02 09:35:45 UTC
Created attachment 354498 [details] Minimal example that makes the keymap invalid I've had some issues on mutter/Wayland with clients randomly reverting to US-English layout with xkb keymap parse errors. I think that this is the root cause. The xkb keymap is sent to clients using the wl_keyboard::keymap() event that includes a file descriptor that should be mmap()ed and then given to xkb_keymap_new_from_string. Although there is commonly no reason to do so, the fd can be mmap()ed with PROT_WRITE and MAP_SHARED flags, so the client can modify it. Since mutter only uses one global fd that it shares with all clients, changes to the mmap()ed keymap are visible to other clients until the compositor changes keymap and thus resets the fd. This means that the keymap can be made invalid or replaced altogether. As the change is not announced by the compositor, this will only apply to newly started clients. Qt apps seem to crash at start when the keymap is invalid. This is definitely a bug and potentially a security hole because the Wayland protocol was designed specifically such that clients cannot interfere with each other. See https://bugs.freedesktop.org/show_bug.cgi?id=101595 for weston bug
Created attachment 354502 [details] Minimal example that makes the keymap invalid Uploaded wrong version of the test program, sorry
Created attachment 354507 [details] [review] wayland/keyboard: Don't send writable keymap fd to clients Don't send a writable keymap fd to clients, as they would be able to alter the memory of the keymap server side, causing memory corruption. As the same piece of memory was sent to each client, this would effectively let an abusive client have indirect write access to the mmap:ed keymap memory of another client. For now, solve this by re-opening the temporary file read-only before unlinking, and instead send the read-only file descriptor to the clients. This will cause non-abusive clients who by mistake mmap:ed the keymap fd with PROT_WRITE to fail reading the keymap data.
I really dislike that this involves leaving named temporary files around longer, but don't see any way around this. We can't use memfd + write seal (which would otherwise be perfect) for this, because memfd precludes MAP_SHARED mappings, so this would break every client out there.
(In reply to Daniel Stone from comment #3) > I really dislike that this involves leaving named temporary files around > longer, but don't see any way around this. We can't use memfd + write seal > (which would otherwise be perfect) for this, because memfd precludes > MAP_SHARED mappings, so this would break every client out there. I could open the read-only fd and unlink earlier than in the patch, but will it make much difference?
Review of attachment 354507 [details] [review]: Doing this is not enough, since a client can chmod and reopen the fd any way it wants via /proc/<pid>/<fd>, even from within flatpak.
Created attachment 354669 [details] [review] wayland/keyboard: Create a separate keymap shm file per resource By using the shm file when sending the keymap to all clients, we effectively allows any client to change the keymap, as any client has the ability to change the content of the file. Sending a read-only file descriptor, or making the file itself read-only before unlinking, can be worked around by the client by using chmod(2) and open(2) on /proc/<pid>/<fd>. Using memfd could potentially solve this issue, but as the usage of mmap with MAP_SHARED is wide spread among clients, such a change can not be introduced without causing wide spread compatibility issues. So, to avoid allowing clients to interfere with each other, create a separate shm file for each wl_keyboard resource when sending the keymap. We could eventually do this per client, but in most cases, there will only be one wl_keyboard resource per client anyway.
Review of attachment 354669 [details] [review]: looks good
The idea is to land this now-ish, and I'll put it on both master and gnome-3-28. Whether it is worthy a release or not I'm not sure.
Flatpak probably means it is worth a release?
Attachment 354669 [details] pushed as 21ce6f9 - wayland/keyboard: Create a separate keymap shm file per resource
On gnome-3-28 as well now.