From ab670a431afc8c4d787c112b691698a50e7be3ca Mon Sep 17 00:00:00 2001 From: Johannes Bauer Date: Fri, 25 Oct 2019 13:02:35 +0200 Subject: [PATCH] Refactor command execution to not use tempfile Previously, we wrote the passphrase contents to a temporary file on /dev/shm and then wiped it afterwards. This is odd, why don't we use a pipe for this purpose, like it's intended to be used? Replace all of that previous code by piped IPC. --- client.c | 2 +- exec.c | 64 +++++++++++++++++++++++++++++--------- exec.h | 13 ++++++-- luks.c | 93 ++++++++++++++------------------------------------------ luks.h | 3 +- 5 files changed, 85 insertions(+), 90 deletions(-) diff --git a/client.c b/client.c index 9e21a39..c27aeea 100644 --- a/client.c +++ b/client.c @@ -65,7 +65,7 @@ static bool unlock_luks_volume(const struct volume_entry_t *volume, const struct bool success = true; char luks_passphrase[LUKS_PASSPHRASE_TEXT_SIZE_BYTES]; if (ascii_encode(luks_passphrase, sizeof(luks_passphrase), unlock_msg->luks_passphrase_raw, sizeof(unlock_msg->luks_passphrase_raw))) { - success = open_luks_device_pw(volume->volume_uuid, volume->devmapper_name, luks_passphrase, strlen(luks_passphrase)); + success = open_luks_device(volume->volume_uuid, volume->devmapper_name, luks_passphrase, strlen(luks_passphrase)); } else { log_msg(LLVL_FATAL, "Failed to transcribe raw LUKS passphrase to text form."); success = false; diff --git a/exec.c b/exec.c index bae2928..849149e 100644 --- a/exec.c +++ b/exec.c @@ -1,6 +1,6 @@ /* luksrku - Tool to remotely unlock LUKS disks using TLS. - Copyright (C) 2016-2016 Johannes Bauer + Copyright (C) 2016-2019 Johannes Bauer This file is part of luksrku. @@ -27,6 +27,7 @@ #include #include #include +#include #include #include "exec.h" @@ -78,45 +79,80 @@ static char **argv_dup(const char **argv) { return result; } -struct runresult_t exec_command(const char **argv, bool show_output) { - struct runresult_t runresult; - char **argvcopy = argv_dup(argv); +struct exec_result_t exec_command(const struct exec_cmd_t *command) { + char **argvcopy = argv_dup(command->argv); + if (!argvcopy) { + return (struct exec_result_t) { .success = false }; + } - memset(&runresult, 0, sizeof(runresult)); + int pipefd[2]; + if (pipe(pipefd) == -1) { + log_libc(LLVL_ERROR, "Creation of pipe(2) failed trying to execute %s", argvcopy[0]); + argv_free(argvcopy); + return (struct exec_result_t) { .success = false }; + } + const int pipe_read_end = pipefd[0]; + const int pipe_write_end = pipefd[1]; pid_t pid = fork(); if (pid == -1) { perror("fork"); - runresult.success = false; argv_free(argvcopy); - return runresult; + return (struct exec_result_t) { .success = false }; } if (pid == 0) { /* Child */ - if (!show_output) { + + close(pipe_write_end); + if (dup2(pipe_read_end, STDIN_FILENO) == -1) { + log_libc(LLVL_ERROR, "Could not dup2(2) stdin while trying to execute %s", argvcopy[0]); + exit(EXIT_FAILURE); + } + + if (!command->show_output) { /* Shut up the child if user did not request debug output */ - close(1); - close(2); + close(STDOUT_FILENO); + close(STDERR_FILENO); } execvp(argvcopy[0], argvcopy); log_libc(LLVL_ERROR, "Execution of %s in forked child process failed execvp(3)", argvcopy[0]); - /* Exec failed, terminate child with EXIT_FAILUR (parent will catch + /* Exec failed, terminate child with EXIT_FAILURE (parent will catch * this as the return code) */ exit(EXIT_FAILURE); } + /* Parent process */ + struct exec_result_t runresult = { + .success = true, + }; + close(pipe_read_end); + + if (command->stdin_data && command->stdin_length) { + unsigned int offset = 0; + unsigned int remaining_bytes = command->stdin_length; + const uint8_t *byte_buffer = (const uint8_t*)command->stdin_data; + while (remaining_bytes) { + ssize_t written = write(pipe_write_end, byte_buffer + offset, remaining_bytes); + if (written <= 0) { + log_libc(LLVL_ERROR, "writing to pipe returned %d", written); + runresult.success = false; + } + offset += written; + remaining_bytes -= written; + } + } + close(pipe_write_end); + int status; if (waitpid(pid, &status, 0) == (pid_t)-1) { log_libc(LLVL_ERROR, "exec_command %s failed executing waitpid(2)", argvcopy[0]); runresult.success = false; - runresult.returncode = -1; } else { - runresult.success = true; runresult.returncode = WEXITSTATUS(status); } argv_free(argvcopy); - log_msg(LLVL_DEBUG, "Subprocess (PID %d): %s exited with returncode %d", pid, argv[0], runresult.returncode); + log_msg(LLVL_DEBUG, "Subprocess (PID %d): %s exited with returncode %d", pid, command->argv[0], runresult.returncode); return runresult; } diff --git a/exec.h b/exec.h index 96cc988..3e1a2ec 100644 --- a/exec.h +++ b/exec.h @@ -1,6 +1,6 @@ /* luksrku - Tool to remotely unlock LUKS disks using TLS. - Copyright (C) 2016-2016 Johannes Bauer + Copyright (C) 2016-2019 Johannes Bauer This file is part of luksrku. @@ -26,14 +26,21 @@ #include -struct runresult_t { +struct exec_cmd_t { + const char **argv; + bool show_output; + const void *stdin_data; + unsigned int stdin_length; +}; + +struct exec_result_t { bool success; int returncode; }; /*************** AUTO GENERATED SECTION FOLLOWS ***************/ void argv_dump(const char **argv); -struct runresult_t exec_command(const char **argv, bool show_output); +struct exec_result_t exec_command(const struct exec_cmd_t *cmd); /*************** AUTO GENERATED SECTION ENDS ***************/ #endif diff --git a/luks.c b/luks.c index 049562e..164c506 100644 --- a/luks.c +++ b/luks.c @@ -37,85 +37,38 @@ #include "uuid.h" bool is_luks_device_opened(const char *mapping_name) { - const char *command[] = { - "dmsetup", - "status", - mapping_name, - NULL, + struct exec_cmd_t cmd = { + .argv = (const char *[]){ + "dmsetup", + "status", + mapping_name, + NULL, + }, + .show_output = should_log(LLVL_TRACE), }; - struct runresult_t runresult = exec_command(command, should_log(LLVL_TRACE)); + struct exec_result_t runresult = exec_command(&cmd); return runresult.success && (runresult.returncode == 0); } -bool open_luks_device(const uint8_t *encrypted_device_uuid, const char *mapping_name, const char *passphrase_file) { +bool open_luks_device(const uint8_t *encrypted_device_uuid, const char *mapping_name, const char *passphrase, unsigned int passphrase_length) { char encrypted_device[64]; strcpy(encrypted_device, "UUID="); sprintf_uuid(encrypted_device + 5, encrypted_device_uuid); log_msg(LLVL_INFO, "Trying to unlock LUKS mapping %s based on %s", mapping_name, encrypted_device); - const char *command[] = { - "cryptsetup", - "luksOpen", - "-T", "1", - "-d", passphrase_file, - encrypted_device, - mapping_name, - NULL, - + struct exec_cmd_t cmd = { + .argv = (const char *[]){ + "cryptsetup", + "luksOpen", + "-T", "1", + encrypted_device, + mapping_name, + NULL, + }, + .stdin_data = passphrase, + .stdin_length = passphrase_length, + .show_output = should_log(LLVL_DEBUG), }; - struct runresult_t runresult = exec_command(command, should_log(LLVL_DEBUG)); + struct exec_result_t runresult = exec_command(&cmd); return runresult.success && (runresult.returncode == 0); } - -static bool wipe_passphrase_file(const char *filename, int length) { - uint8_t wipe_buf[length]; - memset(wipe_buf, 0, length); - - int fd = open(filename, O_WRONLY); - if (fd == -1) { - log_libc(LLVL_ERROR, "Wiping of passphrase file %s failed in open(2)", filename); - return false; - } - - if (write(fd, wipe_buf, length) != length) { - log_libc(LLVL_ERROR, "Wiping of passphrase file %s failed in write(2)", filename); - close(fd); - return false; - } - close(fd); - unlink(filename); - return true; -} - -static const char *write_passphrase_file(const void *passphrase, int passphrase_length) { - //const char *filename = "/dev/shm/luksrku_passphrase.bin"; /* TODO make this variable */ - const char *filename = "/tmp/luksrku_passphrase.bin"; - int fd = open(filename, O_CREAT | O_WRONLY | O_TRUNC, 0600); - if (fd == -1) { - log_libc(LLVL_ERROR, "Creation of passphrase file %s failed", filename); - return NULL; - } - - if (write(fd, passphrase, passphrase_length) != passphrase_length) { - log_libc(LLVL_ERROR, "Writing to passphrase file %s failed", filename); - wipe_passphrase_file(filename, passphrase_length); - close(fd); - return NULL; - } - - close(fd); - return filename; -} - -bool open_luks_device_pw(const uint8_t *encrypted_device_uuid, const char *mapping_name, const char *passphrase, unsigned int passphrase_length) { - const char *pw_filename = write_passphrase_file(passphrase, passphrase_length); - if (!pw_filename) { - return false; - } - bool success = open_luks_device(encrypted_device_uuid, mapping_name, pw_filename); - if (!wipe_passphrase_file(pw_filename, passphrase_length)) { - log_libc(LLVL_ERROR, "Wiping of passphrase file failed -- treating this unlock as failed (luksOpen %s)", success ? "succeeded" : "also failed"); - success = false; - } - return success; -} diff --git a/luks.h b/luks.h index 749ae0d..53521d9 100644 --- a/luks.h +++ b/luks.h @@ -29,8 +29,7 @@ /*************** AUTO GENERATED SECTION FOLLOWS ***************/ bool is_luks_device_opened(const char *mapping_name); -bool open_luks_device(const uint8_t *encrypted_device_uuid, const char *mapping_name, const char *passphrase_file); -bool open_luks_device_pw(const uint8_t *encrypted_device_uuid, const char *mapping_name, const char *passphrase, unsigned int passphrase_length); +bool open_luks_device(const uint8_t *encrypted_device_uuid, const char *mapping_name, const char *passphrase, unsigned int passphrase_length); /*************** AUTO GENERATED SECTION ENDS ***************/ #endif