From d2e25475296d0403b14a0d02c73326d0bbef5a2f Mon Sep 17 00:00:00 2001 From: Christoph Ruegge Date: Tue, 25 Feb 2020 00:51:19 +0100 Subject: [PATCH] Move most subprocess code to separate helper, improve signal safety The calling process might be multi-threaded, which severely limits what can be done safely between fork and execve. The additional fork introdcued recently (e4f06d0) rendered the code unsafe in that setting. This commit moves most logic to a separate helper that is exec'd after fork/setuid/dup. It also improves signal handling somewhat: SIGPIPE is avoided by leaving open the reading end, and the signal mask is reset after fork. --- configure.ac | 4 + src/Makefile.am | 4 + src/helper.c | 151 +++++++++++++++++++++++++++++ src/pam_gnupg.c | 251 +++++++++++++++++++++--------------------------- 4 files changed, 266 insertions(+), 144 deletions(-) create mode 100644 src/helper.c diff --git a/configure.ac b/configure.ac index 2baa7d9..7daa736 100644 --- a/configure.ac +++ b/configure.ac @@ -47,6 +47,10 @@ fi AC_DEFINE_UNQUOTED([GPG_CONNECT_AGENT], "$GPG_CONNECT_AGENT", [path to gpg-connect-agent]) +# From https://github.com/gpg/gnupg/blob/4c43fab/agent/agent.h#L54 +AC_DEFINE([MAX_PASSPHRASE_LEN], [255], + [Maximum passphrase length accepted by gpg-agent]) + AC_CONFIG_HEADERS([config.h]) AC_CONFIG_FILES([Makefile src/Makefile]) AC_OUTPUT diff --git a/src/Makefile.am b/src/Makefile.am index 635822b..0f5cb01 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -2,6 +2,10 @@ module_LTLIBRARIES = pam_gnupg.la pam_gnupg_la_SOURCES = pam_gnupg.c pam_gnupg_la_LIBADD = -lpam pam_gnupg_la_LDFLAGS = -module -avoid-version +pam_gnupg_la_CPPFLAGS = -DPAM_GNUPG_HELPER='"$(libexecdir)/pam_gnupg_helper"' + +libexec_PROGRAMS = pam_gnupg_helper +pam_gnupg_helper_SOURCES = helper.c install-data-hook: rm -f $(DESTDIR)$(moduledir)/pam_gnupg.la diff --git a/src/helper.c b/src/helper.c new file mode 100644 index 0000000..8be5a85 --- /dev/null +++ b/src/helper.c @@ -0,0 +1,151 @@ +#define _GNU_SOURCE + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "config.h" + +#define KEYGRIP_LEN 40 + +#define xstr(x) str(x) +#define str(x) #x + +char tohex(char n) { + if (n < 10) { + return n + '0'; + } else { + return n - 10 + 'A'; + } +} + +void nextline(FILE *f) { + for (;;) { + switch (getc(f)) { + case EOF: + case '\n': + return; + } + } +} + +bool nextkeygrip(FILE *f) { + int c; + for (;;) { + do c = getc(f); + while (c != EOF && strchr(" \t\n\r\f\v", c)); + if (c == EOF) { + return false; + } + if (c != '#') { + ungetc(c, f); + return true; + } + nextline(f); + } +} + +int main(int argc, char **argv) { + bool autostart = false; + for (int i = 1; i < argc; i++) { + if (strcmp(argv[i], "--autostart") == 0) { + autostart = true; + } + } + + struct passwd *pwd = getpwuid(getuid()); + if (pwd == NULL) { + exit(EXIT_FAILURE); + } + + int dirfd = open(pwd->pw_dir, O_RDONLY | O_DIRECTORY | O_CLOEXEC); + if (dirfd < 0) { + exit(EXIT_FAILURE); + } + int fd = openat(dirfd, ".pam-gnupg", O_RDONLY | O_CLOEXEC); + if (fd < 0) { + exit(errno == ENOENT ? EXIT_SUCCESS : EXIT_FAILURE); + } + FILE *f = fdopen(fd, "r"); + if (f == NULL) { + exit(EXIT_FAILURE); + } + + char tok[MAX_PASSPHRASE_LEN + 1]; + if (fgets(tok, MAX_PASSPHRASE_LEN + 1, stdin) == NULL) { + exit(EXIT_FAILURE); + } + + char hextok[2 * MAX_PASSPHRASE_LEN + 1]; + char *s = tok, *h = hextok; + while (*s != '\0' && *s != '\n') { + *h++ = tohex((*s >> 4) & 15); + *h++ = tohex(*s & 15); + s++; + } + *s = *h = '\0'; + + int pipefd[2]; + if (pipe2(pipefd, O_CLOEXEC) < 0) { + exit(EXIT_FAILURE); + } + + FILE *p = fdopen(pipefd[1], "w"); + if (p == NULL) { + exit(EXIT_FAILURE); + } + + signal(SIGCHLD, SIG_DFL); + pid_t pid = fork(); + if (pid == -1) { + exit(EXIT_FAILURE); + } else if (pid == 0) { + if (dup2(pipefd[0], STDIN_FILENO) < 0) { + exit(EXIT_FAILURE); + } + // gpg-connect-agent has an option --no-autostart, which *should* return + // non-zero when the agent is not running. Unfortunately, the exit code is + // always 0 in version 2.1. Passing an invalid agent program here is a + // workaround. See https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=797334 + char *cmd[] = {GPG_CONNECT_AGENT, "--agent-program", "/dev/null", NULL}; + if (autostart) { + cmd[1] = NULL; + } + execv(cmd[0], cmd); + exit(EXIT_FAILURE); + } + + close(pipefd[0]); + + signal(SIGPIPE, SIG_IGN); + bool finished = true; + for (; nextkeygrip(f); nextline(f)) { + char keygrip[KEYGRIP_LEN + 1]; + if (fscanf(f, "%" xstr(KEYGRIP_LEN) "[0-9A-Fa-f]", keygrip) < 1) { + continue; + } + if (strlen(keygrip) < KEYGRIP_LEN) { + continue; + } + for (s = keygrip; *s; s++) { + *s = toupper(*s); + } + if (fprintf(p, "preset_passphrase %s -1 %s\n", keygrip, hextok) < 0) { + finished = false; + break; + } + } + + fclose(p); + + int status; + waitpid(pid, &status, 0); + return (finished && WIFEXITED(status)) ? WEXITSTATUS(status) : EXIT_FAILURE; +} diff --git a/src/pam_gnupg.c b/src/pam_gnupg.c index 357919a..bf65a56 100644 --- a/src/pam_gnupg.c +++ b/src/pam_gnupg.c @@ -1,5 +1,6 @@ #define _GNU_SOURCE +#include #include #include #include @@ -12,42 +13,14 @@ #define PAM_SM_AUTH #define PAM_SM_SESSION -#include -#include #include +#include #include "config.h" -#define KEYGRIP_LENGTH 40 +#define TOKEN_DATA_NAME "pam-gnupg-token" -#define READ_END 0 -#define WRITE_END 1 - -char tohex(char n) { - if (n < 10) { - return n + '0'; - } else { - return n - 10 + 'A'; - } -} - -/* Copied from gnupg */ -char *hexify(const char *token) { - char *result = malloc(2*strlen(token)+1); - char *r; - const char *s; - if (result == NULL) { - return NULL; - } - for (s = token, r = result; *s; s++) { - *r++ = tohex((*s>>4) & 15); - *r++ = tohex(*s & 15); - } - *r = 0; - return result; -} - -/* Copied from gnome-keyring */ +// Copied from gnome-keyring void wipestr(char *data) { volatile char *vp; size_t len; @@ -76,166 +49,156 @@ bool preset_passphrase(pam_handle_t *pamh, const char *tok, bool autostart) { return false; } - struct passwd *pwd = getpwnam(user); + struct passwd *pwd = pam_modutil_getpwnam(pamh, user); if (pwd == NULL) { return false; } + uid_t uid = pwd->pw_uid; + gid_t gid = pwd->pw_gid; - struct sigaction sigchld, old_sigchld; - memset(&sigchld, 0, sizeof(sigchld)); - memset(&old_sigchld, 0, sizeof(old_sigchld)); - sigchld.sa_handler = SIG_DFL; - sigaction(SIGCHLD, &sigchld, &old_sigchld); - - pid_t pid = fork(); - if (pid == -1) { - sigaction(SIGCHLD, &old_sigchld, NULL); + int pipefd[2]; + if (pipe2(pipefd, O_CLOEXEC) < 0) { return false; - } else if (pid > 0) { - int status; - waitpid(pid, &status, 0); - sigaction(SIGCHLD, &old_sigchld, NULL); - return (WIFEXITED(status) && WEXITSTATUS(status) == EXIT_SUCCESS); } - // We're in the child process now. From here on, the function will not return. + // Reset SIGCHLD handler so we can use waitpid(). If the calling process + // used a handler to manage its own child processes, and one of the + // children exits while we're busy, things will probably break, but there + // does not appear to be a sane way of avoiding this. + // + // TODO Add a noreap option like pam_unix to selectively disable this for + // services that are able to handle it. + struct sigaction sa, saved_sigchld; + sigemptyset(&sa.sa_mask); + sa.sa_handler = SIG_DFL; + sa.sa_flags = 0; + sigaction(SIGCHLD, &sa, &saved_sigchld); - if (setregid(pwd->pw_gid, pwd->pw_gid) < 0 || setreuid(pwd->pw_uid, pwd->pw_uid) < 0) { - exit(EXIT_FAILURE); - } + // pam_getenvlist() allocates, so we can't call it after fork(). + char **env = pam_getenvlist(pamh); - int inp[2] = {-1, -1}; - if (pipe(inp) < 0) { - exit(EXIT_FAILURE); - } - signal(SIGPIPE, SIG_IGN); + bool ret = true; - int dev_null = open("/dev/null", O_RDWR); - if (dev_null != -1) { - dup2(dev_null, STDIN_FILENO); - dup2(dev_null, STDOUT_FILENO); - dup2(dev_null, STDERR_FILENO); - close(dev_null); + pid_t pid = fork(); + if (pid < 0) { + close(pipefd[0]); + close(pipefd[1]); + ret = false; } - int dirfd = open(pwd->pw_dir, O_RDONLY | O_CLOEXEC); - if (dirfd < 0) { - exit(EXIT_FAILURE); - } - int fd = openat(dirfd, ".pam-gnupg", O_RDONLY | O_CLOEXEC); - if (fd < 0) { - exit(EXIT_FAILURE); - } - FILE *file = fdopen(fd, "r"); - if (file == NULL) { - exit(EXIT_FAILURE); - } + else if (pid == 0) { + if (setregid(gid, gid) < 0 || setreuid(uid, uid) < 0) { + exit(EXIT_FAILURE); + } - pid = fork(); - if (pid == -1) { - exit(EXIT_FAILURE); - } else if (pid == 0) { - // Grandchild - if (dup2(inp[READ_END], STDIN_FILENO) < 0) { + // Unblock all signals. fork() clears pending signals in the child, so + // this is safe. + sigset_t emptyset; + sigemptyset(&emptyset); + sigprocmask(SIG_SETMASK, &emptyset, NULL); + + if (dup2(pipefd[0], STDIN_FILENO) < 0) { exit(EXIT_FAILURE); } - close(inp[READ_END]); - close(inp[WRITE_END]); - - // gpg-connect-agent has an option --no-autostart, which *should* return - // non-zero when the agent is not running. Unfortunately, the exit code is - // always 0 in version 2.1. Passing an invalid agent program here is a - // workaround. See https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=797334 - const char *cmd[] = {GPG_CONNECT_AGENT, "--agent-program", "/dev/null", NULL}; - if (autostart) { - cmd[1] = NULL; + int dev_null = open("/dev/null", O_WRONLY | O_CLOEXEC); + if (dev_null != -1) { + dup2(dev_null, STDOUT_FILENO); + dup2(dev_null, STDERR_FILENO); } - char **env = pam_getenvlist(pamh); + int maxfd = getdtablesize(); + for (int n = 3; n < maxfd; n++) { + close(n); + } + + char * cmd[] = {PAM_GNUPG_HELPER, "--autostart", NULL}; + if (!autostart) { + cmd[1] = NULL; + } if (env != NULL) { - execve(cmd[0], (char * const *) cmd, env); + execve(cmd[0], cmd, env); } else { - execv(cmd[0], (char * const *) cmd); + execv(cmd[0], cmd); } exit(EXIT_FAILURE); } - close(inp[READ_END]); - - char *presetcmd; - const int presetlen = asprintf(&presetcmd, "PRESET_PASSPHRASE xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx -1 %s\n", tok); - if (presetlen < 0) { - exit(EXIT_FAILURE); - } - char * const keygrip = presetcmd + 18; - - char *line = NULL; - size_t len = 0; - ssize_t rd; - while ((rd = getline(&line, &len, file)) != -1) { - if (line[rd-1] == '\n') { - line[rd-1] = '\0'; - } - const char *cur = line; - while (*cur && strchr(" \t\n\r\f\v", *cur)) { - cur++; - } - if (!*cur || *cur == '#') { - continue; - } - strncpy(keygrip, cur, KEYGRIP_LENGTH); - if (strlen(keygrip) < KEYGRIP_LENGTH) { - // We hit an unexpected eol or null byte. - continue; - } - if (write(inp[WRITE_END], presetcmd, presetlen) < 0) { - exit(EXIT_FAILURE); + else { + if (pam_modutil_write(pipefd[1], tok, strlen(tok)) < 0) { + ret = false; } + // We close the read fd after writing in order to avoid SIGPIPE. Since + // we write at most MAX_PASSPHRASE_LEN bytes, the pipe buffer won't + // fill up and block us even if the child process dies. + close(pipefd[0]); + close(pipefd[1]); + int status; + while (waitpid(pid, &status, 0) < 0 && errno == EINTR) + ; + ret = ret && WIFEXITED(status) && WEXITSTATUS(status) == EXIT_SUCCESS; } - int status; - close(inp[WRITE_END]); - waitpid(pid, &status, 0); - if (WIFEXITED(status) && WEXITSTATUS(status) == 0) { - exit(EXIT_SUCCESS); - } else { - exit(EXIT_FAILURE); - } + free(env); + sigaction(SIGCHLD, &saved_sigchld, NULL); + return ret; } int pam_sm_authenticate(pam_handle_t *pamh, int flags, int argc, const char **argv) { const char *tok = NULL; - if (pam_get_item(pamh, PAM_AUTHTOK, (const void **) &tok) == PAM_SUCCESS && tok != NULL) { - tok = hexify(tok); - if (tok != NULL) { - pam_set_data(pamh, "pam-gnupg-token", (void *) tok, cleanup_token); - } + if (pam_get_item(pamh, PAM_AUTHTOK, (const void **) &tok) != PAM_SUCCESS + || tok == NULL + ) { + return PAM_AUTHINFO_UNAVAIL; } + // Copy not more bytes than gpg-agent is able to handle. + tok = strndup(tok, MAX_PASSPHRASE_LEN); + if (tok == NULL) { + return PAM_SYSTEM_ERR; + } + pam_set_data(pamh, TOKEN_DATA_NAME, (void *) tok, cleanup_token); return PAM_SUCCESS; } int pam_sm_setcred(pam_handle_t *pamh, int flags, int argc, const char **argv) { const char *tok = NULL; - if ((argc > 0 && strcmp(argv[0], "store-only") == 0) || - (flags & PAM_DELETE_CRED) || - pam_get_data(pamh, "pam-gnupg-token", (const void **) &tok) != PAM_SUCCESS || - tok == NULL) { + if (flags & PAM_DELETE_CRED) { return PAM_SUCCESS; } + if (pam_get_data(pamh, TOKEN_DATA_NAME, (const void **) &tok) != PAM_SUCCESS + || tok == NULL + ) { + return PAM_IGNORE; + } + for (int i = 0; i < argc; i++) { + if (strcmp(argv[i], "store-only") == 0) { + return PAM_SUCCESS; + } + } if (!preset_passphrase(pamh, tok, false)) { return PAM_IGNORE; } - pam_set_data(pamh, "pam-gnupg-token", NULL, NULL); + pam_set_data(pamh, TOKEN_DATA_NAME, NULL, NULL); return PAM_SUCCESS; } int pam_sm_open_session(pam_handle_t *pamh, int flags, int argc, const char **argv) { const char *tok = NULL; - if (pam_get_data(pamh, "pam-gnupg-token", (const void **) &tok) == PAM_SUCCESS && tok != NULL) { - preset_passphrase(pamh, tok, (argc == 0 || strcmp(argv[0], "no-autostart") != 0)); - pam_set_data(pamh, "pam-gnupg-token", NULL, NULL); + int ret = PAM_SUCCESS; + bool autostart = true; + for (int i = 0; i < argc; i++) { + if (strcmp(argv[i], "no-autostart") == 0) { + autostart = false; + } } - return PAM_SUCCESS; + if (pam_get_data(pamh, TOKEN_DATA_NAME, (const void **) &tok) == PAM_SUCCESS + && tok != NULL + ) { + if (!preset_passphrase(pamh, tok, autostart)) { + ret = PAM_SESSION_ERR; + } + pam_set_data(pamh, TOKEN_DATA_NAME, NULL, NULL); + } + return ret; } int pam_sm_close_session(pam_handle_t *pamh, int flags, int argc, const char **argv) {