Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mlock() hardening does not apply to key events in libwayland #289

Open
mstoeckl opened this issue Mar 16, 2023 · 0 comments
Open

mlock() hardening does not apply to key events in libwayland #289

mstoeckl opened this issue Mar 16, 2023 · 0 comments

Comments

@mstoeckl
Copy link
Contributor

mstoeckl commented Mar 16, 2023

Currently swaylock uses mlock() when available to ensure that the contents of password buffers (see comm.c#L23 and main.c#L1180 ) are never swapped out. This was introduced by d8f0fb1 .

However, once a password has been typed in, its contents are still available in memory in other forms. Each wl_keyboard::key event received by swaylock is read into a ring buffer (see wl_connection_read()) and then copied into a heap allocated object (see use of wl_connection_demarshal()) . The compositor's connection also retains a copy of these events, until the compositor's outgoing ringbuffer and any wl_closure objects are overwritten. None of these are protected from being put into swap.

This suggests that the current strategy of using mlock() on buffers with password content is insufficient to protect against the (admittedly very rare) scenarios in which an adversary can ensure some of swaylock's memory is swapped out after a password has been typed in but before the screen has been unlocked and swaylock exits. (Note: mlock() does not affect hibernation or core dumps.)

What should be done about this, I don't know -- perhaps it is possible to prevent the relevant parts of libwayland from swapping, or perhaps applying mlockall() is worth the tradeoffs; or perhaps the current use of mlock() is not worth the increased code complexity, and efforts are better spent ensuring that distributions use encrypted swap by default, if they don't already have full disk encryption; or maybe the best thing to do is nothing.

For comparison: I suspect xsecurelock has the same problem of not mlock()'ing the X11 event queue; xscreensaver doesn't use mlock at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

1 participant