From 7ab7547a40d456d34120b2f44b26385ac1338ebd Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 14 Sep 2021 23:03:37 +0200 Subject: [PATCH 1/9] fileio: lower maximum virtual file buffer size by one byte MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When reading virtual files (i.e. procfs, sysfs, …) we currently put a limit of 4M-1 on that. We have to pick something, and we have to read these files in a single read() (since the kernel generally doesn't support continuation read()s for them). 4M-1 is actually the maximum size the kernel allows for reads from files in /proc/sys/, all larger reads will result in an ENOMEM error (which is really weird, but the kernel does what the kernel does). Hence 4M-1 sounds like a smart choice. However, we made one mistake here: in order to be able to detect EOFs properly we actually read one byte more than we actually intend to return: if that extra byte can be read, then we know the file is actually larger than our limit and we can generate an EFBIG error from that. However, if it cannot be read then we know EOF was hit, and we are good. So ultimately after all we issued a single 4M read, which the kernel then responds with ENOMEM to. And that means read_virtual_file() actually doesn't work properly right now on /proc/sys/. Let's fix that. The fix is simple, lower the limit of the the buffer we intend to return by one, i.e. 4M-2. That way, the read() we'll issue is exactly as large as the limit the kernel allows, and we still get safely detect EOF from it. --- src/basic/fileio.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/basic/fileio.c b/src/basic/fileio.c index f44fa57de4..af7ae5b1b8 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -30,14 +30,16 @@ /* The maximum size of the file we'll read in one go in read_full_file() (64M). */ #define READ_FULL_BYTES_MAX (64U*1024U*1024U - 1U) -/* The maximum size of virtual files we'll read in one go in read_virtual_file() (4M). Note that this limit - * is different (and much lower) than the READ_FULL_BYTES_MAX limit. This reflects the fact that we use - * different strategies for reading virtual and regular files: virtual files are generally size constrained: - * there we allocate the full buffer size in advance. Regular files OTOH can be much larger, and here we grow - * the allocations exponentially in a loop. In glibc large allocations are immediately backed by mmap() - * making them relatively slow (measurably so). Thus, when allocating the full buffer in advance the large - * limit is a problem. When allocating piecemeal it's not. Hence pick two distinct limits. */ -#define READ_VIRTUAL_BYTES_MAX (4U*1024U*1024U - 1U) +/* The maximum size of virtual files (i.e. procfs, sysfs, and other virtual "API" files) we'll read in one go + * in read_virtual_file(). Note that this limit is different (and much lower) than the READ_FULL_BYTES_MAX + * limit. This reflects the fact that we use different strategies for reading virtual and regular files: + * virtual files we generally have to read in a single read() syscall since the kernel doesn't support + * continuation read()s for them. Thankfully they are somewhat size constrained. Thus we can allocate the + * full potential buffer in advance. Regular files OTOH can be much larger, and there we grow the allocations + * exponentially in a loop. We use a size limit of 4M-2 because 4M-1 is the maximum buffer that /proc/sys/ + * allows us to read() (larger reads will fail with ENOMEM), and we want to read one extra byte so that we + * can detect EOFs. */ +#define READ_VIRTUAL_BYTES_MAX (4U*1024U*1024U - 2U) int fopen_unlocked(const char *path, const char *options, FILE **ret) { assert(ret); From be991d7678c35aa037ef79672c0c70781eebed9c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 14 Sep 2021 23:11:55 +0200 Subject: [PATCH 2/9] fileio: set O_NOCTTY when reading virtual files Better be safe than sorry, maybe someone points this call to a TTY one day, and we'd rather not make it our controlling TTY in that case. --- src/basic/fileio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/basic/fileio.c b/src/basic/fileio.c index af7ae5b1b8..efed64cda0 100644 --- a/src/basic/fileio.c +++ b/src/basic/fileio.c @@ -395,7 +395,7 @@ int read_virtual_file(const char *filename, size_t max_size, char **ret_contents * contents* may be returned. (Though the read is still done using one syscall.) Returns 0 on * partial success, 1 if untruncated contents were read. */ - fd = open(filename, O_RDONLY|O_CLOEXEC); + fd = open(filename, O_RDONLY|O_NOCTTY|O_CLOEXEC); if (fd < 0) return -errno; From 13239c86e01b4d96ca08b321ffc01a30905d46ce Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 14 Sep 2021 23:19:38 +0200 Subject: [PATCH 3/9] sysctl-util: rework sysctl_write() to wrap write_string_file() The sysctl_write_ip_property() call already uses write_string_file(), so let's do so here, too, to make the codepaths more uniform. While we are at it, let's also validate the passed path a bit, since we shouldn't allow sysctls with /../ or such in the name. Hence simplify the path first, and then check if it is normalized, and refuse if not. --- src/basic/sysctl-util.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/basic/sysctl-util.c b/src/basic/sysctl-util.c index 8913e6ff85..60eec3dfec 100644 --- a/src/basic/sysctl-util.c +++ b/src/basic/sysctl-util.c @@ -44,25 +44,19 @@ char *sysctl_normalize(char *s) { int sysctl_write(const char *property, const char *value) { char *p; - _cleanup_close_ int fd = -1; assert(property); assert(value); - log_debug("Setting '%s' to '%.*s'.", property, (int) strcspn(value, NEWLINE), value); - p = strjoina("/proc/sys/", property); - fd = open(p, O_WRONLY|O_CLOEXEC); - if (fd < 0) - return -errno; - if (!endswith(value, "\n")) - value = strjoina(value, "\n"); + path_simplify(p); + if (!path_is_normalized(p)) + return -EINVAL; - if (write(fd, value, strlen(value)) < 0) - return -errno; + log_debug("Setting '%s' to '%s'", p, value); - return 0; + return write_string_file(p, value, WRITE_STRING_FILE_VERIFY_ON_FAILURE | WRITE_STRING_FILE_DISABLE_BUFFER); } int sysctl_writef(const char *property, const char *format, ...) { From 6aebfec3a5b56f3f210a4b06bfb806089cbf95b7 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 15 Sep 2021 09:19:04 +0200 Subject: [PATCH 4/9] sysctl-util: make sysctl_write_ip_property() a wrapper around sysctl_write() It does the same stuff, let's use the same codepaths as much as we can. And while we are at it, let's generate good error codes in case we are called with unsupported parameters/let's validate stuff more that might originate from user input. --- src/basic/socket-util.c | 2 +- src/basic/socket-util.h | 7 ++++--- src/basic/sysctl-util.c | 18 ++++++++++++------ 3 files changed, 17 insertions(+), 10 deletions(-) diff --git a/src/basic/socket-util.c b/src/basic/socket-util.c index 1c353e86b0..9fc89a5f6e 100644 --- a/src/basic/socket-util.c +++ b/src/basic/socket-util.c @@ -793,7 +793,7 @@ bool ifname_valid_full(const char *p, IfnameValidFlags flags) { /* Let's refuse "all" and "default" as interface name, to avoid collisions with the special sysctl * directories /proc/sys/net/{ipv4,ipv6}/conf/{all,default} */ - if (STR_IN_SET(p, "all", "default")) + if (!FLAGS_SET(flags, IFNAME_VALID_SPECIAL) && STR_IN_SET(p, "all", "default")) return false; for (const char *t = p; *t; t++) { diff --git a/src/basic/socket-util.h b/src/basic/socket-util.h index 7fd591f6e2..0d76eace7d 100644 --- a/src/basic/socket-util.h +++ b/src/basic/socket-util.h @@ -135,9 +135,10 @@ int ip_tos_to_string_alloc(int i, char **s); int ip_tos_from_string(const char *s); typedef enum { - IFNAME_VALID_ALTERNATIVE = 1 << 0, - IFNAME_VALID_NUMERIC = 1 << 1, - _IFNAME_VALID_ALL = IFNAME_VALID_ALTERNATIVE | IFNAME_VALID_NUMERIC, + IFNAME_VALID_ALTERNATIVE = 1 << 0, /* Allow "altnames" too */ + IFNAME_VALID_NUMERIC = 1 << 1, /* Allow decimal formatted ifindexes too */ + IFNAME_VALID_SPECIAL = 1 << 2, /* Allow the special names "all" and "default" */ + _IFNAME_VALID_ALL = IFNAME_VALID_ALTERNATIVE | IFNAME_VALID_NUMERIC | IFNAME_VALID_SPECIAL, } IfnameValidFlags; bool ifname_valid_char(char a); bool ifname_valid_full(const char *p, IfnameValidFlags flags); diff --git a/src/basic/sysctl-util.c b/src/basic/sysctl-util.c index 60eec3dfec..a19f3e2649 100644 --- a/src/basic/sysctl-util.c +++ b/src/basic/sysctl-util.c @@ -5,11 +5,13 @@ #include #include +#include "af-list.h" #include "fd-util.h" #include "fileio.h" #include "log.h" #include "macro.h" #include "path-util.h" +#include "socket-util.h" #include "string-util.h" #include "sysctl-util.h" @@ -77,17 +79,21 @@ int sysctl_writef(const char *property, const char *format, ...) { int sysctl_write_ip_property(int af, const char *ifname, const char *property, const char *value) { const char *p; - assert(IN_SET(af, AF_INET, AF_INET6)); assert(property); assert(value); - p = strjoina("/proc/sys/net/ipv", af == AF_INET ? "4" : "6", - ifname ? "/conf/" : "", strempty(ifname), - property[0] == '/' ? "" : "/", property); + if (!IN_SET(af, AF_INET, AF_INET6)) + return -EAFNOSUPPORT; - log_debug("Setting '%s' to '%s'", p, value); + if (ifname) { + if (!ifname_valid_full(ifname, IFNAME_VALID_SPECIAL)) + return -EINVAL; - return write_string_file(p, value, WRITE_STRING_FILE_VERIFY_ON_FAILURE | WRITE_STRING_FILE_DISABLE_BUFFER); + p = strjoina("net/", af_to_ipv4_ipv6(af), "/conf/", ifname, "/", property); + } else + p = strjoina("net/", af_to_ipv4_ipv6(af), "/", property); + + return sysctl_write(p, value); } int sysctl_read(const char *property, char **ret) { From f9755203b972966d545bba7f3d257cf4c6a4f8a0 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 15 Sep 2021 09:20:49 +0200 Subject: [PATCH 5/9] sysctl-util: modernize sysctl_read() a bit Let's add similar path validation to sysctl_read() as we already have in sysctl_write(). Let's also drop the trailing newline from the returned string, like sysctl_read_ip_property() already does it. (I checked all users of this, they don't care) --- src/basic/sysctl-util.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/basic/sysctl-util.c b/src/basic/sysctl-util.c index a19f3e2649..9c81001e69 100644 --- a/src/basic/sysctl-util.c +++ b/src/basic/sysctl-util.c @@ -98,12 +98,23 @@ int sysctl_write_ip_property(int af, const char *ifname, const char *property, c int sysctl_read(const char *property, char **ret) { char *p; + int r; assert(property); - assert(ret); p = strjoina("/proc/sys/", property); - return read_full_virtual_file(p, ret, NULL); + + path_simplify(p); + if (!path_is_normalized(p)) /* Filter out attempts to write to /proc/sys/../../…, just in case */ + return -EINVAL; + + r = read_full_virtual_file(p, ret, NULL); + if (r < 0) + return r; + if (ret) + delete_trailing_chars(*ret, NEWLINE); + + return r; } int sysctl_read_ip_property(int af, const char *ifname, const char *property, char **ret) { From d1469b70959660b400a2868867c564342605c74d Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 15 Sep 2021 09:22:06 +0200 Subject: [PATCH 6/9] sysctl-util: make sysctl_read_ip_property() a wrapper around sysctl_read() let's do what we did for sysctl_write()/sysctl_write_ip_property() also for the read paths: i.e. make one a wrapper of the other, and add more careful input validation. --- src/basic/sysctl-util.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/src/basic/sysctl-util.c b/src/basic/sysctl-util.c index 9c81001e69..4e168dd48a 100644 --- a/src/basic/sysctl-util.c +++ b/src/basic/sysctl-util.c @@ -118,24 +118,20 @@ int sysctl_read(const char *property, char **ret) { } int sysctl_read_ip_property(int af, const char *ifname, const char *property, char **ret) { - _cleanup_free_ char *value = NULL; const char *p; - int r; - assert(IN_SET(af, AF_INET, AF_INET6)); assert(property); - p = strjoina("/proc/sys/net/ipv", af == AF_INET ? "4" : "6", - ifname ? "/conf/" : "", strempty(ifname), - property[0] == '/' ? "" : "/", property); + if (!IN_SET(af, AF_INET, AF_INET6)) + return -EAFNOSUPPORT; - r = read_full_virtual_file(p, &value, NULL); - if (r < 0) - return r; + if (ifname) { + if (!ifname_valid_full(ifname, IFNAME_VALID_SPECIAL)) + return -EINVAL; - truncate_nl(value); - if (ret) - *ret = TAKE_PTR(value); + p = strjoina("net/", af_to_ipv4_ipv6(af), "/conf/", ifname, "/", property); + } else + p = strjoina("net/", af_to_ipv4_ipv6(af), "/", property); - return r; + return sysctl_read(p, ret); } From 20240b071b669d51db2b48adb4cfa01a2876ec9f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 15 Sep 2021 09:23:56 +0200 Subject: [PATCH 7/9] sysctl-util: per coding style, compare chars explicit against 0. --- src/basic/sysctl-util.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/basic/sysctl-util.c b/src/basic/sysctl-util.c index 4e168dd48a..bb01d09696 100644 --- a/src/basic/sysctl-util.c +++ b/src/basic/sysctl-util.c @@ -38,7 +38,7 @@ char *sysctl_normalize(char *s) { path_simplify(s); /* Kill the leading slash, but keep the first character of the string in the same place. */ - if (*s == '/' && *(s+1)) + if (s[0] == '/' && s[1] != 0) memmove(s, s+1, strlen(s)); return s; From 89edc243e4b50094df4cef81a7637a9c6a38962a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 14 Sep 2021 23:13:23 +0200 Subject: [PATCH 8/9] test: make array in test-sysctl fully read-only --- src/test/test-sysctl-util.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/test-sysctl-util.c b/src/test/test-sysctl-util.c index a6f449b764..991a1e3363 100644 --- a/src/test/test-sysctl-util.c +++ b/src/test/test-sysctl-util.c @@ -4,7 +4,7 @@ #include "sysctl-util.h" #include "tests.h" -static const char* cases[] = { +static const char* const cases[] = { "a.b.c", "a/b/c", "a/b/c", "a/b/c", "a/b.c/d", "a/b.c/d", @@ -24,7 +24,7 @@ static void test_sysctl_normalize(void) { log_info("/* %s */", __func__); const char **s, **expected; - STRV_FOREACH_PAIR(s, expected, cases) { + STRV_FOREACH_PAIR(s, expected, (const char**) cases) { _cleanup_free_ char *t; assert_se(t = strdup(*s)); From 3448711b7e04cfcda0e0e046b5f94c7f35105185 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 14 Sep 2021 23:13:36 +0200 Subject: [PATCH 9/9] test: add test case for sysctl-util.[ch] --- src/test/test-sysctl-util.c | 38 +++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/src/test/test-sysctl-util.c b/src/test/test-sysctl-util.c index 991a1e3363..17ec04e732 100644 --- a/src/test/test-sysctl-util.c +++ b/src/test/test-sysctl-util.c @@ -1,5 +1,9 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ +#include "sd-id128.h" + +#include "errno-util.h" +#include "hostname-util.h" #include "strv.h" #include "sysctl-util.h" #include "tests.h" @@ -35,10 +39,44 @@ static void test_sysctl_normalize(void) { } } +static void test_sysctl_read(void) { + _cleanup_free_ char *s = NULL, *h = NULL; + sd_id128_t a, b; + int r; + + assert_se(sysctl_read("kernel/random/boot_id", &s) >= 0); + assert_se(sd_id128_from_string(s, &a) >= 0); + assert_se(sd_id128_get_boot(&b) >= 0); + assert_se(sd_id128_equal(a, b)); + s = mfree(s); + + assert_se(sysctl_read_ip_property(AF_INET, "lo", "forwarding", &s)); + assert_se(STR_IN_SET(s, "0", "1")); + + r = sysctl_write_ip_property(AF_INET, "lo", "forwarding", s); + assert_se(r >= 0 || ERRNO_IS_PRIVILEGE(r) || r == -EROFS); + s = mfree(s); + + assert_se(sysctl_read_ip_property(AF_INET, NULL, "ip_forward", &s)); + assert_se(STR_IN_SET(s, "0", "1")); + + r = sysctl_write_ip_property(AF_INET, NULL, "ip_forward", s); + assert_se(r >= 0 || ERRNO_IS_PRIVILEGE(r) || r == -EROFS); + s = mfree(s); + + assert_se(sysctl_read("kernel/hostname", &s) >= 0); + assert_se(gethostname_full(GET_HOSTNAME_ALLOW_NONE|GET_HOSTNAME_ALLOW_LOCALHOST, &h) >= 0); + assert_se(streq(s, h)); + + r = sysctl_write("kernel/hostname", s); + assert_se(r >= 0 || ERRNO_IS_PRIVILEGE(r) || r == -EROFS); +} + int main(int argc, char *argv[]) { test_setup_logging(LOG_INFO); test_sysctl_normalize(); + test_sysctl_read(); return 0; }