From f28501279d2c28fdbb31d8273b723e9bf71d3b98 Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Sun, 19 Feb 2017 13:19:50 -0500 Subject: firewall-util: add an assert that we're not overwriting a buffer Check for CID #1368267. --- src/shared/firewall-util.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'src') diff --git a/src/shared/firewall-util.c b/src/shared/firewall-util.c index 9c29b0afca..952fc48c45 100644 --- a/src/shared/firewall-util.c +++ b/src/shared/firewall-util.c @@ -76,8 +76,11 @@ static int entry_fill_basics( } if (out_interface) { + size_t l = strlen(out_interface); + assert(l < sizeof entry->ip.outiface && l < sizeof entry->ip.outiface_mask); + strcpy(entry->ip.outiface, out_interface); - memset(entry->ip.outiface_mask, 0xFF, strlen(out_interface)+1); + memset(entry->ip.outiface_mask, 0xFF, l + 1); } if (destination) { entry->ip.dst = destination->in; -- cgit v1.2.3-54-g00ecf From 5c5d9f26d69060a72d63fa8dcd0ee3d3c75fb9f5 Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Sun, 19 Feb 2017 13:36:17 -0500 Subject: core/killall: add (void) CID #1368238. --- src/core/killall.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'src') diff --git a/src/core/killall.c b/src/core/killall.c index 7a9df546ee..3fe9fa2ed0 100644 --- a/src/core/killall.c +++ b/src/core/killall.c @@ -213,7 +213,8 @@ static int killall(int sig, Set *pids, bool send_sighup) { if (get_ctty_devnr(pid, NULL) >= 0) - kill(pid, SIGHUP); + /* it's OK if the process is gone, just ignore the result */ + (void) kill(pid, SIGHUP); } } -- cgit v1.2.3-54-g00ecf From df8067ef6d341efe8f86c6b24c7fb27fc6056bc1 Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Sun, 19 Feb 2017 13:39:08 -0500 Subject: importd: check setenv return value CID #1368235. --- src/import/importd.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) (limited to 'src') diff --git a/src/import/importd.c b/src/import/importd.c index 9d31a956a5..3d379d6de9 100644 --- a/src/import/importd.c +++ b/src/import/importd.c @@ -449,8 +449,11 @@ static int transfer_start(Transfer *t) { stdio_unset_cloexec(); - setenv("SYSTEMD_LOG_TARGET", "console-prefixed", 1); - setenv("NOTIFY_SOCKET", "/run/systemd/import/notify", 1); + if (setenv("SYSTEMD_LOG_TARGET", "console-prefixed", 1) < 0 || + setenv("NOTIFY_SOCKET", "/run/systemd/import/notify", 1) < 0) { + log_error_errno(errno, "setenv() failed: %m"); + _exit(EXIT_FAILURE); + } if (IN_SET(t->type, TRANSFER_IMPORT_TAR, TRANSFER_IMPORT_RAW)) cmd[k++] = SYSTEMD_IMPORT_PATH; -- cgit v1.2.3-54-g00ecf From e8f3e7a707f9362d8c2b481304c0e435258de6aa Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Sun, 19 Feb 2017 13:43:22 -0500 Subject: logind: check return value from lseek In practice this doesn't matter much because the read that follows will likely fail, but we'll get a better error message. CID #1368233. --- src/login/logind-seat.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) (limited to 'src') diff --git a/src/login/logind-seat.c b/src/login/logind-seat.c index e4836fcaab..30dac7997b 100644 --- a/src/login/logind-seat.c +++ b/src/login/logind-seat.c @@ -379,7 +379,8 @@ int seat_read_active_vt(Seat *s) { if (!seat_has_vts(s)) return 0; - lseek(s->manager->console_active_fd, SEEK_SET, 0); + if (lseek(s->manager->console_active_fd, SEEK_SET, 0) < 0) + return log_error_errno(errno, "lseek on console_active_fd failed: %m"); k = read(s->manager->console_active_fd, t, sizeof(t)-1); if (k <= 0) { @@ -396,10 +397,8 @@ int seat_read_active_vt(Seat *s) { } r = safe_atou(t+3, &vtnr); - if (r < 0) { - log_error("Failed to parse VT number %s", t+3); - return r; - } + if (r < 0) + return log_error_errno(r, "Failed to parse VT number \"%s\": %m", t+3); if (!vtnr) { log_error("VT number invalid: %s", t+3); -- cgit v1.2.3-54-g00ecf From 643f4706b0f4712f4bce2884d1c0e94e1eb63d9e Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Sun, 19 Feb 2017 13:48:59 -0500 Subject: core/execute: add (void) CID #778045. --- src/core/execute.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) (limited to 'src') diff --git a/src/core/execute.c b/src/core/execute.c index f455afa962..d7798387c5 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -1144,11 +1144,13 @@ static int setup_pam( /* Tell the parent that our setup is done. This is especially * important regarding dropping privileges. Otherwise, unit - * setup might race against our setresuid(2) call. */ - barrier_place(&barrier); + * setup might race against our setresuid(2) call. + * + * If the parent aborted, we'll detect this below, hence ignore + * return failure here. */ + (void) barrier_place(&barrier); - /* Check if our parent process might already have - * died? */ + /* Check if our parent process might already have died? */ if (getppid() == parent_pid) { sigset_t ss; -- cgit v1.2.3-54-g00ecf From 0357fa0dcea7e6bd599fbd5b6aac6df9b6961c8e Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Sun, 19 Feb 2017 13:52:18 -0500 Subject: shared/pager: abort if we cannot set environment variables This most likely means oom, it's better to exit than to run less with incomplete settings. CID #714383. --- src/shared/pager.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) (limited to 'src') diff --git a/src/shared/pager.c b/src/shared/pager.c index af667a83f4..f00ba9e1e7 100644 --- a/src/shared/pager.c +++ b/src/shared/pager.c @@ -104,7 +104,8 @@ int pager_open(bool no_pager, bool jump_to_end) { less_opts = "FRSXMK"; if (jump_to_end) less_opts = strjoina(less_opts, " +G"); - setenv("LESS", less_opts, 1); + if (setenv("LESS", less_opts, 1) < 0) + _exit(EXIT_FAILURE); /* Initialize a good charset for less. This is * particularly important if we output UTF-8 @@ -112,8 +113,9 @@ int pager_open(bool no_pager, bool jump_to_end) { less_charset = getenv("SYSTEMD_LESSCHARSET"); if (!less_charset && is_locale_utf8()) less_charset = "utf-8"; - if (less_charset) - setenv("LESSCHARSET", less_charset, 1); + if (less_charset && + setenv("LESSCHARSET", less_charset, 1) < 0) + _exit(EXIT_FAILURE); /* Make sure the pager goes away when the parent dies */ if (prctl(PR_SET_PDEATHSIG, SIGTERM) < 0) -- cgit v1.2.3-54-g00ecf From 2fa4861ad5a203bff604cac660136834e3b70108 Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Sun, 19 Feb 2017 14:17:19 -0500 Subject: sd-device: replace lstat() + open() with open(O_NOFOLLOW) Coverity was complaining about TOCTOU (CID #745806). Indeed, it seems better to open the file and avoid the stat altogether: - O_NOFOLLOW means we'll get ELOOP, which we can translate to EINVAL as before, - similarly, open(O_WRONLY) on a directory will fail with EISDIR, - and finally, it makes no sense to check access mode ourselves: just let the kernel do it and propagate the error. v2: - fix memleak, don't clober input arg --- src/libsystemd/sd-device/sd-device.c | 43 ++++++++++++++---------------------- 1 file changed, 16 insertions(+), 27 deletions(-) (limited to 'src') diff --git a/src/libsystemd/sd-device/sd-device.c b/src/libsystemd/sd-device/sd-device.c index efeadf0cd4..04ead29338 100644 --- a/src/libsystemd/sd-device/sd-device.c +++ b/src/libsystemd/sd-device/sd-device.c @@ -1859,8 +1859,7 @@ _public_ int sd_device_set_sysattr_value(sd_device *device, const char *sysattr, _cleanup_free_ char *value = NULL; const char *syspath; char *path; - struct stat statbuf; - size_t value_len = 0; + size_t len = 0; ssize_t size; int r; @@ -1878,8 +1877,14 @@ _public_ int sd_device_set_sysattr_value(sd_device *device, const char *sysattr, return r; path = strjoina(syspath, "/", sysattr); - r = lstat(path, &statbuf); - if (r < 0) { + + fd = open(path, O_WRONLY | O_CLOEXEC | O_NOFOLLOW); + if (fd < 0) { + if (errno == ELOOP) + return -EINVAL; + if (errno == EISDIR) + return -EISDIR; + value = strdup(""); if (!value) return -ENOMEM; @@ -1891,46 +1896,30 @@ _public_ int sd_device_set_sysattr_value(sd_device *device, const char *sysattr, return -ENXIO; } - if (S_ISLNK(statbuf.st_mode)) - return -EINVAL; - - /* skip directories */ - if (S_ISDIR(statbuf.st_mode)) - return -EISDIR; - - /* skip non-readable files */ - if ((statbuf.st_mode & S_IRUSR) == 0) - return -EACCES; - - value_len = strlen(_value); + len = strlen(_value); /* drop trailing newlines */ - while (value_len > 0 && _value[value_len - 1] == '\n') - _value[--value_len] = '\0'; + while (len > 0 && _value[len - 1] == '\n') + len --; /* value length is limited to 4k */ - if (value_len > 4096) + if (len > 4096) return -EINVAL; - fd = open(path, O_WRONLY | O_CLOEXEC); - if (fd < 0) - return -errno; - - value = strdup(_value); + value = strndup(_value, len); if (!value) return -ENOMEM; - size = write(fd, value, value_len); + size = write(fd, value, len); if (size < 0) return -errno; - if ((size_t)size != value_len) + if ((size_t)size != len) return -EIO; r = device_add_sysattr_value(device, sysattr, value); if (r < 0) return r; - value = NULL; return 0; -- cgit v1.2.3-54-g00ecf