From d5d1ae153f394aa08e436a747d20702b18e79f00 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 19 Mar 2019 21:42:21 +0100 Subject: [PATCH 1/7] cryptsetup: small refactoring --- src/cryptsetup/cryptsetup.c | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/src/cryptsetup/cryptsetup.c b/src/cryptsetup/cryptsetup.c index 87e0cefbab..1f0e34b038 100644 --- a/src/cryptsetup/cryptsetup.c +++ b/src/cryptsetup/cryptsetup.c @@ -612,6 +612,24 @@ static int help(void) { return 0; } +static uint32_t determine_flags(void) { + uint32_t flags = 0; + + if (arg_readonly) + flags |= CRYPT_ACTIVATE_READONLY; + + if (arg_discards) + flags |= CRYPT_ACTIVATE_ALLOW_DISCARDS; + + if (arg_same_cpu_crypt) + flags |= CRYPT_ACTIVATE_SAME_CPU_CRYPT; + + if (arg_submit_from_crypt_cpus) + flags |= CRYPT_ACTIVATE_SUBMIT_FROM_CRYPT_CPUS; + + return flags; +} + static int run(int argc, char *argv[]) { _cleanup_(crypt_freep) struct crypt_device *cd = NULL; int r; @@ -676,17 +694,7 @@ static int run(int argc, char *argv[]) { return 0; } - if (arg_readonly) - flags |= CRYPT_ACTIVATE_READONLY; - - if (arg_discards) - flags |= CRYPT_ACTIVATE_ALLOW_DISCARDS; - - if (arg_same_cpu_crypt) - flags |= CRYPT_ACTIVATE_SAME_CPU_CRYPT; - - if (arg_submit_from_crypt_cpus) - flags |= CRYPT_ACTIVATE_SUBMIT_FROM_CRYPT_CPUS; + flags = determine_flags(); if (arg_timeout == USEC_INFINITY) until = 0; From e1ed99c8c89d6e4ca4df3e47617ff1f3e194cabb Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 20 Mar 2019 10:29:20 +0100 Subject: [PATCH 2/7] ask-password: erase character read with _cleanup_ This is much nicer, since it means we erase the character regardless how we exit the scope. --- src/basic/memory-util.h | 5 +++++ src/shared/ask-password-api.c | 5 +---- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/basic/memory-util.h b/src/basic/memory-util.h index e1e6624d3b..915c24a5dd 100644 --- a/src/basic/memory-util.h +++ b/src/basic/memory-util.h @@ -77,3 +77,8 @@ static inline void* explicit_bzero_safe(void *p, size_t l) { #else void *explicit_bzero_safe(void *p, size_t l); #endif + +/* Use with _cleanup_ to erase a single 'char' when leaving scope */ +static inline void erase_char(char *p) { + explicit_bzero_safe(p, sizeof(char)); +} diff --git a/src/shared/ask-password-api.c b/src/shared/ask-password-api.c index 4122362b3c..88335c6d08 100644 --- a/src/shared/ask-password-api.c +++ b/src/shared/ask-password-api.c @@ -306,9 +306,9 @@ int ask_password_tty( }; for (;;) { + _cleanup_(erase_char) char c; int sleep_for = -1, k; ssize_t n; - char c; if (until > 0) { usec_t y; @@ -452,9 +452,6 @@ int ask_password_tty( dirty = true; } - - /* Let's forget this char, just to not keep needlessly copies of key material around */ - c = 'x'; } x = strndup(passphrase, p); From e5e21a05076e60fe46eab4eb040735250ae3b814 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 20 Mar 2019 10:30:31 +0100 Subject: [PATCH 3/7] alloc-util: extra paranoid overflow check --- src/basic/alloc-util.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/basic/alloc-util.c b/src/basic/alloc-util.c index b28fb95ccf..15b67665d2 100644 --- a/src/basic/alloc-util.c +++ b/src/basic/alloc-util.c @@ -27,6 +27,9 @@ void* memdup_suffix0(const void *p, size_t l) { /* The same as memdup() but place a safety NUL byte after the allocated memory */ + if (_unlikely_(l == SIZE_MAX)) /* prevent overflow */ + return NULL; + ret = malloc(l + 1); if (!ret) return NULL; From 23964f7faf86df7fd297dc5b0a9f1dfb03a0eb56 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 20 Mar 2019 10:31:13 +0100 Subject: [PATCH 4/7] alloc-util: add extra overflow checks to GREEDY_REALLOC() --- src/basic/alloc-util.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/basic/alloc-util.c b/src/basic/alloc-util.c index 15b67665d2..f4bd33f4e0 100644 --- a/src/basic/alloc-util.c +++ b/src/basic/alloc-util.c @@ -48,13 +48,17 @@ void* greedy_realloc(void **p, size_t *allocated, size_t need, size_t size) { if (*allocated >= need) return *p; - newalloc = MAX(need * 2, 64u / size); - a = newalloc * size; - - /* check for overflows */ - if (a < size * need) + if (_unlikely_(need > SIZE_MAX/2)) /* Overflow check */ return NULL; + newalloc = need * 2; + if (size_multiply_overflow(newalloc, size)) + return NULL; + + a = newalloc * size; + if (a < 64) /* Allocate at least 64 bytes */ + a = 64; + q = realloc(*p, a); if (!q) return NULL; From d4b604baeadbb2498e4f2c3e260260eed210f5d6 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 20 Mar 2019 10:31:38 +0100 Subject: [PATCH 5/7] alloc-util: use malloc_usable_size() to determine allocated size It's a glibc-specific API, but supported on FreeBSD and musl too at least, hence fairly common. This way we can reduce our calls to realloc() as much as possible. --- src/basic/alloc-util.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/basic/alloc-util.c b/src/basic/alloc-util.c index f4bd33f4e0..1e4ee722f2 100644 --- a/src/basic/alloc-util.c +++ b/src/basic/alloc-util.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: LGPL-2.1+ */ +#include #include #include @@ -64,7 +65,7 @@ void* greedy_realloc(void **p, size_t *allocated, size_t need, size_t size) { return NULL; *p = q; - *allocated = newalloc; + *allocated = _unlikely_(size == 0) ? newalloc : malloc_usable_size(q) / size; return q; } From 32ae18206f3917373e40e2cb89e31f8be0b0f200 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 20 Mar 2019 10:33:23 +0100 Subject: [PATCH 6/7] alloc-util: typo fix --- src/basic/alloc-util.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/basic/alloc-util.h b/src/basic/alloc-util.h index 78ee34bb71..9b20be4773 100644 --- a/src/basic/alloc-util.h +++ b/src/basic/alloc-util.h @@ -156,8 +156,8 @@ void* greedy_realloc0(void **p, size_t *allocated, size_t need, size_t size); (void*)memset(_new_, 0, _xsize_); \ }) -/* Takes inspiration from Rusts's Option::take() method: reads and returns a pointer, but at the same time resets it to - * NULL. See: https://doc.rust-lang.org/std/option/enum.Option.html#method.take */ +/* Takes inspiration from Rust's Option::take() method: reads and returns a pointer, but at the same time + * resets it to NULL. See: https://doc.rust-lang.org/std/option/enum.Option.html#method.take */ #define TAKE_PTR(ptr) \ ({ \ typeof(ptr) _ptr_ = (ptr); \ From 054b6be05b15a8b4ddcad44711698e9a3abb700b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 20 Mar 2019 10:33:45 +0100 Subject: [PATCH 7/7] ask-password: add extra paranoid overflow check --- src/shared/ask-password-api.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/shared/ask-password-api.c b/src/shared/ask-password-api.c index 88335c6d08..ab0c34692f 100644 --- a/src/shared/ask-password-api.c +++ b/src/shared/ask-password-api.c @@ -77,13 +77,18 @@ static int retrieve_key(key_serial_t serial, char ***ret) { n = keyctl(KEYCTL_READ, (unsigned long) serial, (unsigned long) p, (unsigned long) m, 0); if (n < 0) return -errno; - if (n < m) break; explicit_bzero_safe(p, n); - free(p); + + if (m > LONG_MAX / 2) /* overflow check */ + return -ENOMEM; m *= 2; + if ((long) (size_t) m != m) /* make sure that this still fits if converted to size_t */ + return -ENOMEM; + + free(p); } l = strv_parse_nulstr(p, n);