From c89e00a97e6bb04c6b4b5c906befdb4767540dbe Mon Sep 17 00:00:00 2001 From: Drew DeVault Date: Sat, 6 Oct 2018 12:17:36 -0400 Subject: [PATCH] Fix swaylock w/shadow on glibc, improve security Today I learned that GNU flaunts the POSIX standard in yet another creative way. Additionally, this adds some security improvements, namely: - Zeroing out password buffers in the privileged child process - setuid/setgid after reading /etc/shadow --- meson.build | 1 + swaylock/meson.build | 3 +++ swaylock/shadow.c | 27 +++++++++++++++++++++++++++ 3 files changed, 31 insertions(+) diff --git a/meson.build b/meson.build index 1e7ce281a..42386fbc2 100644 --- a/meson.build +++ b/meson.build @@ -44,6 +44,7 @@ gdk_pixbuf = dependency('gdk-pixbuf-2.0', required: false) pixman = dependency('pixman-1') libinput = dependency('libinput', version: '>=1.6.0') libpam = cc.find_library('pam', required: false) +crypt = cc.find_library('crypt', required: false) systemd = dependency('libsystemd', required: false) elogind = dependency('libelogind', required: false) math = cc.find_library('m') diff --git a/swaylock/meson.build b/swaylock/meson.build index 6605340b4..f3321a78c 100644 --- a/swaylock/meson.build +++ b/swaylock/meson.build @@ -26,6 +26,9 @@ else warning('The swaylock binary must be setuid when compiled without libpam') warning('You must do this manually post-install: chmod a+s /path/to/swaylock') sources += ['shadow.c'] + if crypt.found() + dependencies += [crypt] + endif endif executable('swaylock', diff --git a/swaylock/shadow.c b/swaylock/shadow.c index 1f10514c8..f928eaa3a 100644 --- a/swaylock/shadow.c +++ b/swaylock/shadow.c @@ -6,9 +6,21 @@ #include #include #include "swaylock/swaylock.h" +#ifdef __GLIBC__ +// GNU, you damn slimy bastard +#include +#endif static int comm[2][2]; +static void clear_buffer(void *buf, size_t bytes) { + volatile char *buffer = buf; + volatile char zero = '\0'; + for (size_t i = 0; i < bytes; ++i) { + buffer[i] = zero; + } +} + void run_child(void) { /* This code runs as root */ struct passwd *pwent = getpwuid(getuid()); @@ -25,6 +37,17 @@ void run_child(void) { } encpw = swent->sp_pwdp; } + + /* We don't need any additional logging here because the parent process will + * also fail here and will handle logging for us. */ + if (setgid(getgid()) != 0) { + exit(EXIT_FAILURE); + } + if (setuid(getuid()) != 0) { + exit(EXIT_FAILURE); + } + + /* This code does not run as root */ wlr_log(WLR_DEBUG, "prepared to authorize user %s", pwent->pw_name); size_t size; @@ -60,10 +83,14 @@ void run_child(void) { result = strcmp(c, encpw) == 0; if (write(comm[1][1], &result, sizeof(result)) != sizeof(result)) { wlr_log_errno(WLR_ERROR, "failed to write pw check result"); + clear_buffer(buf, size); exit(EXIT_FAILURE); } + clear_buffer(buf, size); free(buf); } + + clear_buffer(encpw, strlen(encpw)); exit(EXIT_SUCCESS); }