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

Use explicit_bzero(3) to clear passwords, if it is available #353

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nmeum
Copy link

@nmeum nmeum commented Mar 30, 2024

This is a revived version of #148 which is less invasive by only modifying the implementation of clear_buffer().

Relying on the volatile keyword to clear memory is, in many cases, insufficient and may result in the clearing operation being optimized out by the compiler. Recognizing the difficulty of zero'ing memory, many libcs provide an explicit_bzero(3) function (which the libc guarantees not to be optimized out). This is presently implemented by OpenBSD, musl libc and glibc. This patch makes swaylock use this function and, if it is not available, falls back to the existing code and prints a warning during build.

For more details on the complexity of zero'ing memory, refer to the following 35c3 talk: https://media.ccc.de/v/35c3-9788-memsad

Relying on volatile to clear memory is, in many cases, insufficient
and may result in the clearing operation being optimized out by the
compiler. Recognizing the difficulty of zero'ing memory, many libcs
provide an explicit_bzero(3) function (which the libc guarantees not
to be optimized out). This is presently implemented by OpenBSD, musl
libc and glibc.

This patch makes swaylock use this function and, if it is not available,
falls back to the existing code and prints a warning during build.

For more details on the complexity of zero'ing memory, refer to the
following 35c3 talk: https://media.ccc.de/v/35c3-9788-memsad
@@ -1,3 +1,4 @@
#define _BSD_SOURCE // for explicit_bzero
Copy link
Author

@nmeum nmeum Mar 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defining feature tests macro in the source file is somewhat unideal, only doing it here because that's what is also done elsewhere in the code. Refer to this post for more information. Also using _BSD_SOURCE instead of _GNU_SOURCE here as the BSDs may not define the prototype otherwise. musl defines it on both _GNU_SOURCE and _BSD_SOURCE, not sure about glibc.

@michaelortmann
Copy link

michaelortmann commented Mar 30, 2024

you could also opt to use a more generic solution to clear_buffer().
i can recommend to just copy the code from here:
https://github.com/jedisct1/libsodium/blob/master/src/libsodium/sodium/utils.c#L125-L157
but ive got the feeling, i recommended this before, and this project opted not to import "that much" code and doesnt fear security issues here.

@nmeum
Copy link
Author

nmeum commented Mar 31, 2024

The libsodium compat code is also recommended in the referenced talk. However, given the prior discussion in #148 and the comments regarding non-posix functions, I assumed that something like this is even more less likely to get merged. My goal here is to propose a minimally invasive integration, in the hopes that this will be considered for inclusion. Also recall that explicit_bzero should nowadays be supported by all major platforms targeted by swaylock, hence I am not sure if additional fallbacks are even needed these days.

@emersion
Copy link
Member

Note, the standard equivalent of this is C23's memset_explicit. However, it doesn't seem implemented in libcs yet.

@nmeum
Copy link
Author

nmeum commented Apr 1, 2024

Keep in mind though that memset_explicit has only recently been standardized with C23, hence it will so it will be a while before it is widely available.

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

Successfully merging this pull request may close these issues.

None yet

3 participants