From e66766ead8f4902487a7c613b8a77775c466f823 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 5 Feb 2024 15:15:57 +0100 Subject: [PATCH 01/10] parse-helpers: indent according to coding style --- src/shared/parse-helpers.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/shared/parse-helpers.h b/src/shared/parse-helpers.h index 3e4ad3c0a1..3abb7bfd30 100644 --- a/src/shared/parse-helpers.h +++ b/src/shared/parse-helpers.h @@ -19,11 +19,11 @@ int path_simplify_and_warn( const char *lvalue); int parse_socket_bind_item( - const char *str, - int *address_family, - int *ip_protocol, - uint16_t *nr_ports, - uint16_t *port_min); + const char *str, + int *address_family, + int *ip_protocol, + uint16_t *nr_ports, + uint16_t *port_min); int config_parse_path_or_ignore( const char *unit, From b63e3bd308a7c7a890bbcfadbacef58a46147d50 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 5 Feb 2024 15:36:29 +0100 Subject: [PATCH 02/10] exec-invoke: rework apply_working_directory() around chase() let's be more careful and get rid of one more prefix_roota() use, in favour of the safe chase(). --- src/core/exec-invoke.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/src/core/exec-invoke.c b/src/core/exec-invoke.c index 3f64cc7827..1a6f606448 100644 --- a/src/core/exec-invoke.c +++ b/src/core/exec-invoke.c @@ -3337,31 +3337,39 @@ static int apply_working_directory( const char *home, int *exit_status) { - const char *d, *wd; + const char *wd; + int r; assert(context); assert(exit_status); if (context->working_directory_home) { - if (!home) { *exit_status = EXIT_CHDIR; return -ENXIO; } wd = home; - } else wd = empty_to_root(context->working_directory); if (params->flags & EXEC_APPLY_CHROOT) - d = wd; - else - d = prefix_roota((runtime ? runtime->ephemeral_copy : NULL) ?: context->root_directory, wd); + r = RET_NERRNO(chdir(wd)); + else { + _cleanup_close_ int dfd = -EBADF; - if (chdir(d) < 0 && !context->working_directory_missing_ok) { + r = chase(wd, + (runtime ? runtime->ephemeral_copy : NULL) ?: context->root_directory, + CHASE_PREFIX_ROOT|CHASE_AT_RESOLVE_IN_ROOT, + /* ret_path= */ NULL, + &dfd); + if (r >= 0) + r = RET_NERRNO(fchdir(dfd)); + } + + if (r < 0 && !context->working_directory_missing_ok) { *exit_status = EXIT_CHDIR; - return -errno; + return r; } return 0; From 0fb08bd5e3097ecd79def22872ef3c522d2a3114 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 5 Feb 2024 15:38:20 +0100 Subject: [PATCH 03/10] mountpoint-util: add small helper that checks if a path is below the API VFS hierarchies --- src/basic/mountpoint-util.c | 7 +++++++ src/basic/mountpoint-util.h | 2 ++ src/shutdown/umount.c | 2 +- 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/basic/mountpoint-util.c b/src/basic/mountpoint-util.c index 0ff1ed6018..5f1887c6c9 100644 --- a/src/basic/mountpoint-util.c +++ b/src/basic/mountpoint-util.c @@ -785,3 +785,10 @@ int mount_option_supported(const char *fstype, const char *key, const char *valu return true; /* works! */ } + +bool path_below_api_vfs(const char *p) { + assert(p); + + /* API VFS are either directly mounted on any of these three paths, or below it. */ + return PATH_STARTSWITH_SET(p, "/dev", "/sys", "/proc"); +} diff --git a/src/basic/mountpoint-util.h b/src/basic/mountpoint-util.h index 499403a4d8..b95d1985fa 100644 --- a/src/basic/mountpoint-util.h +++ b/src/basic/mountpoint-util.h @@ -69,3 +69,5 @@ bool mount_new_api_supported(void); unsigned long ms_nosymfollow_supported(void); int mount_option_supported(const char *fstype, const char *key, const char *value); + +bool path_below_api_vfs(const char *p); diff --git a/src/shutdown/umount.c b/src/shutdown/umount.c index 1a9b99d761..ca6d36e054 100644 --- a/src/shutdown/umount.c +++ b/src/shutdown/umount.c @@ -95,7 +95,7 @@ int mount_points_list_get(const char *mountinfo, MountPoint **head) { * we might lack the rights to unmount these things, hence don't bother. */ if (mount_point_is_api(path) || mount_point_ignore(path) || - PATH_STARTSWITH_SET(path, "/dev", "/sys", "/proc")) + path_below_api_vfs(path)) continue; is_api_vfs = fstype_is_api_vfs(fstype); From 0d1332841e96526a713c71d8618939b2443107b5 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 5 Feb 2024 15:38:55 +0100 Subject: [PATCH 04/10] parse-helpers: add new PATH_CHECK_NON_API_VFS flag In various contexts it's a bit icky to allow paths below /proc/, /sys/, /dev/ i.e. file hierarchies where API VFS are placed. Let's add a new flag for path_simplify_and_warn() to check for this and refuse a path if in these paths. Enable this when parsing WorkingDirectory=. This is inspired by CVE-2024-21626, which uses trickery around the cwd and /proc/self/fd/. AFAICS we are not actually vulnerable to the same issue as explained in the CVE since we execute the WorkingDirectory= setting very late, i.e. long after we set up the new mount namespace. But let's filter out icky stuff better earlier than later, as extra safety precaution. --- src/core/load-fragment.c | 2 +- src/shared/parse-helpers.c | 23 ++++++++++++++++------- src/shared/parse-helpers.h | 13 +++++++------ src/test/test-parse-helpers.c | 35 +++++++++++++++++++++++++++++++++++ 4 files changed, 59 insertions(+), 14 deletions(-) diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index 819cbb2772..6c2402b7f1 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -2660,7 +2660,7 @@ int config_parse_working_directory( return missing_ok ? 0 : -ENOEXEC; } - r = path_simplify_and_warn(k, PATH_CHECK_ABSOLUTE | (missing_ok ? 0 : PATH_CHECK_FATAL), unit, filename, line, lvalue); + r = path_simplify_and_warn(k, PATH_CHECK_ABSOLUTE|PATH_CHECK_NON_API_VFS|(missing_ok ? 0 : PATH_CHECK_FATAL), unit, filename, line, lvalue); if (r < 0) return missing_ok ? 0 : -ENOEXEC; diff --git a/src/shared/parse-helpers.c b/src/shared/parse-helpers.c index bad3af8ebf..d397f73131 100644 --- a/src/shared/parse-helpers.c +++ b/src/shared/parse-helpers.c @@ -4,6 +4,7 @@ #include "extract-word.h" #include "ip-protocol-list.h" #include "log.h" +#include "mountpoint-util.h" #include "parse-helpers.h" #include "parse-util.h" #include "path-util.h" @@ -11,36 +12,38 @@ int path_simplify_and_warn( char *path, - unsigned flag, + PathSimplifyWarnFlags flags, const char *unit, const char *filename, unsigned line, const char *lvalue) { - bool fatal = flag & PATH_CHECK_FATAL; + bool fatal = flags & PATH_CHECK_FATAL; - assert(!FLAGS_SET(flag, PATH_CHECK_ABSOLUTE | PATH_CHECK_RELATIVE)); + assert(path); + assert(!FLAGS_SET(flags, PATH_CHECK_ABSOLUTE | PATH_CHECK_RELATIVE)); + assert(lvalue); if (!utf8_is_valid(path)) return log_syntax_invalid_utf8(unit, LOG_ERR, filename, line, path); - if (flag & (PATH_CHECK_ABSOLUTE | PATH_CHECK_RELATIVE)) { + if (flags & (PATH_CHECK_ABSOLUTE | PATH_CHECK_RELATIVE)) { bool absolute; absolute = path_is_absolute(path); - if (!absolute && (flag & PATH_CHECK_ABSOLUTE)) + if (!absolute && (flags & PATH_CHECK_ABSOLUTE)) return log_syntax(unit, LOG_ERR, filename, line, SYNTHETIC_ERRNO(EINVAL), "%s= path is not absolute%s: %s", lvalue, fatal ? "" : ", ignoring", path); - if (absolute && (flag & PATH_CHECK_RELATIVE)) + if (absolute && (flags & PATH_CHECK_RELATIVE)) return log_syntax(unit, LOG_ERR, filename, line, SYNTHETIC_ERRNO(EINVAL), "%s= path is absolute%s: %s", lvalue, fatal ? "" : ", ignoring", path); } - path_simplify_full(path, flag & PATH_KEEP_TRAILING_SLASH ? PATH_SIMPLIFY_KEEP_TRAILING_SLASH : 0); + path_simplify_full(path, flags & PATH_KEEP_TRAILING_SLASH ? PATH_SIMPLIFY_KEEP_TRAILING_SLASH : 0); if (!path_is_valid(path)) return log_syntax(unit, LOG_ERR, filename, line, SYNTHETIC_ERRNO(EINVAL), @@ -52,6 +55,12 @@ int path_simplify_and_warn( "%s= path is not normalized%s: %s", lvalue, fatal ? "" : ", ignoring", path); + if (FLAGS_SET(flags, PATH_CHECK_NON_API_VFS) && path_below_api_vfs(path)) + return log_syntax(unit, LOG_ERR, filename, line, SYNTHETIC_ERRNO(EINVAL), + "%s= path is below API VFS%s: %s", + lvalue, fatal ? ", refusing" : ", ignoring", + path); + return 0; } diff --git a/src/shared/parse-helpers.h b/src/shared/parse-helpers.h index 3abb7bfd30..6d1034b6de 100644 --- a/src/shared/parse-helpers.h +++ b/src/shared/parse-helpers.h @@ -3,16 +3,17 @@ #include -enum { - PATH_CHECK_FATAL = 1 << 0, /* If not set, then error message is appended with 'ignoring'. */ - PATH_CHECK_ABSOLUTE = 1 << 1, - PATH_CHECK_RELATIVE = 1 << 2, +typedef enum PathSimplifyWarnFlags { + PATH_CHECK_FATAL = 1 << 0, /* If not set, then error message is appended with 'ignoring'. */ + PATH_CHECK_ABSOLUTE = 1 << 1, + PATH_CHECK_RELATIVE = 1 << 2, PATH_KEEP_TRAILING_SLASH = 1 << 3, -}; + PATH_CHECK_NON_API_VFS = 1 << 4, +} PathSimplifyWarnFlags; int path_simplify_and_warn( char *path, - unsigned flag, + PathSimplifyWarnFlags flags, const char *unit, const char *filename, unsigned line, diff --git a/src/test/test-parse-helpers.c b/src/test/test-parse-helpers.c index 4943871379..20d4c2f554 100644 --- a/src/test/test-parse-helpers.c +++ b/src/test/test-parse-helpers.c @@ -93,4 +93,39 @@ TEST(invalid_items) { test_invalid_item("ipv6:tcp:6666\n zupa"); } +static int test_path_simplify_and_warn_one(const char *p, const char *q, PathSimplifyWarnFlags f) { + _cleanup_free_ char *s = ASSERT_PTR(strdup(p)); + int a, b; + + a = path_simplify_and_warn(s, f, /* unit= */ NULL, /* filename= */ NULL, /* line= */ 0, "Foobar="); + assert(streq_ptr(s, q)); + + free(s); + s = ASSERT_PTR(strdup(p)); + + b = path_simplify_and_warn(s, f|PATH_CHECK_FATAL, /* unit= */ NULL, /* filename= */ NULL, /* line= */ 0, "Foobar="); + assert(streq_ptr(s, q)); + + assert(a == b); + + return a; +} + +TEST(path_simplify_and_warn) { + + assert_se(test_path_simplify_and_warn_one("", "", 0) == -EINVAL); + assert_se(test_path_simplify_and_warn_one("/", "/", 0) == 0); + assert_se(test_path_simplify_and_warn_one("/foo/../bar", "/foo/../bar", 0) == -EINVAL); + assert_se(test_path_simplify_and_warn_one("/foo/./bar", "/foo/bar", 0) == 0); + assert_se(test_path_simplify_and_warn_one("/proc/self///fd", "/proc/self/fd", 0) == 0); + assert_se(test_path_simplify_and_warn_one("/proc/self///fd", "/proc/self/fd", PATH_CHECK_NON_API_VFS) == -EINVAL); + assert_se(test_path_simplify_and_warn_one("aaaa", "aaaa", 0) == 0); + assert_se(test_path_simplify_and_warn_one("aaaa", "aaaa", PATH_CHECK_ABSOLUTE) == -EINVAL); + assert_se(test_path_simplify_and_warn_one("aaaa", "aaaa", PATH_CHECK_RELATIVE) == 0); + assert_se(test_path_simplify_and_warn_one("/aaaa", "/aaaa", 0) == 0); + assert_se(test_path_simplify_and_warn_one("/aaaa", "/aaaa", PATH_CHECK_ABSOLUTE) == 0); + assert_se(test_path_simplify_and_warn_one("/aaaa", "/aaaa", PATH_CHECK_RELATIVE) == -EINVAL); + +} + DEFINE_TEST_MAIN(LOG_INFO); From a13fb98ba76cfad4dcc8f68530d06b4a9b633e0b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 5 Feb 2024 15:42:46 +0100 Subject: [PATCH 05/10] exec-invoke: extend comment on placement of apply_working_directory() call Inspired by CVE-2024-21626, let's add a longer comment explaining why the code really shouldn#t be moved any earlier. Just in the hope that anyone who feels tempted to move this around maybe actually reads the comment and reconsiders. --- src/core/exec-invoke.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/core/exec-invoke.c b/src/core/exec-invoke.c index 1a6f606448..81d243c5b2 100644 --- a/src/core/exec-invoke.c +++ b/src/core/exec-invoke.c @@ -5040,8 +5040,10 @@ int exec_invoke( } } - /* Apply working directory here, because the working directory might be on NFS and only the user running - * this service might have the correct privilege to change to the working directory */ + /* Apply working directory here, because the working directory might be on NFS and only the user + * running this service might have the correct privilege to change to the working directory. Also, it + * is absolutely 💣 crucial 💣 we applied all mount namespacing rearrangements before this, so that + * the cwd cannot be used to pin directories outside of the sandbox. */ r = apply_working_directory(context, params, runtime, home, exit_status); if (r < 0) return log_exec_error_errno(context, params, r, "Changing to the requested working directory failed: %m"); From 14631951cea807de2d482a430841c604c2040718 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 5 Feb 2024 16:16:59 +0100 Subject: [PATCH 06/10] dbus: make dbus property parsing of WorkingDirectory= equally strict as loading it from the unit files --- src/core/dbus-execute.c | 41 ++++++++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 15 deletions(-) diff --git a/src/core/dbus-execute.c b/src/core/dbus-execute.c index cad6a200b5..b2e5ae4f85 100644 --- a/src/core/dbus-execute.c +++ b/src/core/dbus-execute.c @@ -2721,8 +2721,9 @@ int bus_exec_context_set_transient_property( return 1; } else if (streq(name, "WorkingDirectory")) { + _cleanup_free_ char *simplified = NULL; + bool missing_ok, is_home; const char *s; - bool missing_ok; r = sd_bus_message_read(message, "s", &s); if (r < 0) @@ -2734,23 +2735,33 @@ int bus_exec_context_set_transient_property( } else missing_ok = false; - if (!isempty(s) && !streq(s, "~") && !path_is_absolute(s)) - return sd_bus_error_set(error, SD_BUS_ERROR_INVALID_ARGS, "WorkingDirectory= expects an absolute path or '~'"); + if (isempty(s)) + is_home = false; + else if (streq(s, "~")) + is_home = true; + else { + if (!path_is_absolute(s)) + return sd_bus_error_set(error, SD_BUS_ERROR_INVALID_ARGS, "WorkingDirectory= expects an absolute path or '~'"); + + r = path_simplify_alloc(s, &simplified); + if (r < 0) + return r; + + if (!path_is_normalized(simplified)) + return sd_bus_error_set(error, SD_BUS_ERROR_INVALID_ARGS, "WorkingDirectory= expects a normalized path or '~'"); + + if (path_below_api_vfs(simplified)) + return sd_bus_error_set(error, SD_BUS_ERROR_INVALID_ARGS, "WorkingDirectory= may not be below /proc/, /sys/ or /dev/."); + + is_home = false; + } if (!UNIT_WRITE_FLAGS_NOOP(flags)) { - if (streq(s, "~")) { - c->working_directory = mfree(c->working_directory); - c->working_directory_home = true; - } else { - r = free_and_strdup(&c->working_directory, empty_to_null(s)); - if (r < 0) - return r; - - c->working_directory_home = false; - } - + free_and_replace(c->working_directory, simplified); + c->working_directory_home = is_home; c->working_directory_missing_ok = missing_ok; - unit_write_settingf(u, flags|UNIT_ESCAPE_SPECIFIERS, name, "WorkingDirectory=%s%s", missing_ok ? "-" : "", s); + + unit_write_settingf(u, flags|UNIT_ESCAPE_SPECIFIERS, name, "WorkingDirectory=%s%s", missing_ok ? "-" : "", c->working_directory_home ? "+" : ASSERT_PTR(c->working_directory)); } return 1; From e0f6eaeb4f171329a6cdea4768555de8e2be3d39 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 5 Feb 2024 16:32:57 +0100 Subject: [PATCH 07/10] parse-helpers: adjust log level when we say we ignore to LOG_WARNING --- src/shared/parse-helpers.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/shared/parse-helpers.c b/src/shared/parse-helpers.c index d397f73131..ca6842d363 100644 --- a/src/shared/parse-helpers.c +++ b/src/shared/parse-helpers.c @@ -19,6 +19,7 @@ int path_simplify_and_warn( const char *lvalue) { bool fatal = flags & PATH_CHECK_FATAL; + int level = fatal ? LOG_ERR : LOG_WARNING; assert(path); assert(!FLAGS_SET(flags, PATH_CHECK_ABSOLUTE | PATH_CHECK_RELATIVE)); @@ -33,12 +34,12 @@ int path_simplify_and_warn( absolute = path_is_absolute(path); if (!absolute && (flags & PATH_CHECK_ABSOLUTE)) - return log_syntax(unit, LOG_ERR, filename, line, SYNTHETIC_ERRNO(EINVAL), + return log_syntax(unit, level, filename, line, SYNTHETIC_ERRNO(EINVAL), "%s= path is not absolute%s: %s", lvalue, fatal ? "" : ", ignoring", path); if (absolute && (flags & PATH_CHECK_RELATIVE)) - return log_syntax(unit, LOG_ERR, filename, line, SYNTHETIC_ERRNO(EINVAL), + return log_syntax(unit, level, filename, line, SYNTHETIC_ERRNO(EINVAL), "%s= path is absolute%s: %s", lvalue, fatal ? "" : ", ignoring", path); } @@ -46,17 +47,17 @@ int path_simplify_and_warn( path_simplify_full(path, flags & PATH_KEEP_TRAILING_SLASH ? PATH_SIMPLIFY_KEEP_TRAILING_SLASH : 0); if (!path_is_valid(path)) - return log_syntax(unit, LOG_ERR, filename, line, SYNTHETIC_ERRNO(EINVAL), + return log_syntax(unit, level, filename, line, SYNTHETIC_ERRNO(EINVAL), "%s= path has invalid length (%zu bytes)%s.", lvalue, strlen(path), fatal ? "" : ", ignoring"); if (!path_is_normalized(path)) - return log_syntax(unit, LOG_ERR, filename, line, SYNTHETIC_ERRNO(EINVAL), + return log_syntax(unit, level, filename, line, SYNTHETIC_ERRNO(EINVAL), "%s= path is not normalized%s: %s", lvalue, fatal ? "" : ", ignoring", path); if (FLAGS_SET(flags, PATH_CHECK_NON_API_VFS) && path_below_api_vfs(path)) - return log_syntax(unit, LOG_ERR, filename, line, SYNTHETIC_ERRNO(EINVAL), + return log_syntax(unit, level, filename, line, SYNTHETIC_ERRNO(EINVAL), "%s= path is below API VFS%s: %s", lvalue, fatal ? ", refusing" : ", ignoring", path); From 8049bce6195f8dc5901cb4f60f1ac56206de887b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 5 Feb 2024 16:38:11 +0100 Subject: [PATCH 08/10] load-fragment: set PATH_CHECK_NON_API_VFS flag at various other places I tried to be conservative here, and hence in doubt I left the flag off, but in some cases I really can't see any reason why it would make sense to specifiy paths into API VFS, hence add it there, to lock things down a bit. --- src/core/load-fragment.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index 6c2402b7f1..8f1e5e1543 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -606,7 +606,7 @@ int config_parse_socket_listen( return 0; } - r = path_simplify_and_warn(k, PATH_CHECK_ABSOLUTE, unit, filename, line, lvalue); + r = path_simplify_and_warn(k, PATH_CHECK_ABSOLUTE|PATH_CHECK_NON_API_VFS, unit, filename, line, lvalue); if (r < 0) return 0; @@ -5422,7 +5422,7 @@ int config_parse_mount_images( continue; } - r = path_simplify_and_warn(sresolved, PATH_CHECK_ABSOLUTE, unit, filename, line, lvalue); + r = path_simplify_and_warn(sresolved, PATH_CHECK_ABSOLUTE|PATH_CHECK_NON_API_VFS, unit, filename, line, lvalue); if (r < 0) continue; @@ -5438,7 +5438,7 @@ int config_parse_mount_images( continue; } - r = path_simplify_and_warn(dresolved, PATH_CHECK_ABSOLUTE, unit, filename, line, lvalue); + r = path_simplify_and_warn(dresolved, PATH_CHECK_ABSOLUTE|PATH_CHECK_NON_API_VFS, unit, filename, line, lvalue); if (r < 0) continue; @@ -5580,7 +5580,7 @@ int config_parse_extension_images( continue; } - r = path_simplify_and_warn(sresolved, PATH_CHECK_ABSOLUTE, unit, filename, line, lvalue); + r = path_simplify_and_warn(sresolved, PATH_CHECK_ABSOLUTE|PATH_CHECK_NON_API_VFS, unit, filename, line, lvalue); if (r < 0) continue; @@ -5801,7 +5801,7 @@ int config_parse_pid_file( return log_oom(); /* Check that the result is a sensible path */ - r = path_simplify_and_warn(n, PATH_CHECK_ABSOLUTE, unit, filename, line, lvalue); + r = path_simplify_and_warn(n, PATH_CHECK_ABSOLUTE|PATH_CHECK_NON_API_VFS, unit, filename, line, lvalue); if (r < 0) return r; From f94025a13681e001011dd52e0dcecc143b4501b9 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 5 Feb 2024 17:10:40 +0100 Subject: [PATCH 09/10] nspawn: also refuse paths below API VFS in nspawn's --chdir= field --- src/nspawn/nspawn.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c index c0ec076b24..957bc58217 100644 --- a/src/nspawn/nspawn.c +++ b/src/nspawn/nspawn.c @@ -1368,17 +1368,27 @@ static int parse_argv(int argc, char *argv[]) { break; - case ARG_CHDIR: + case ARG_CHDIR: { + _cleanup_free_ char *wd = NULL; + if (!path_is_absolute(optarg)) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Working directory %s is not an absolute path.", optarg); - r = free_and_strdup(&arg_chdir, optarg); + r = path_simplify_alloc(optarg, &wd); if (r < 0) - return log_oom(); + return log_error_errno(r, "Failed to simplify path %s: %m", optarg); + if (!path_is_normalized(wd)) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Working dirctory path is not normalized: %s", wd); + + if (path_below_api_vfs(wd)) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Working directory is below API VFS, refusing: %s", wd); + + free_and_replace(arg_chdir, wd); arg_settings_mask |= SETTING_WORKING_DIRECTORY; break; + } case ARG_PIVOT_ROOT: r = pivot_root_parse(&arg_pivot_root_new, &arg_pivot_root_old, optarg); From 83d5dab4de8a143e2380f09b1091551a7e33bd5b Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 5 Feb 2024 17:11:12 +0100 Subject: [PATCH 10/10] nspawn: and also add comment, making clear chdir() should come late --- src/nspawn/nspawn.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c index 957bc58217..48394a3100 100644 --- a/src/nspawn/nspawn.c +++ b/src/nspawn/nspawn.c @@ -3522,6 +3522,9 @@ static int inner_child( if (!barrier_place_and_sync(barrier)) /* #5 */ return log_error_errno(SYNTHETIC_ERRNO(ESRCH), "Parent died too early"); + /* Note, this should be done this late (💣 and not moved earlier! 💣), so that all namespacing + * changes are already in effect by now, so that any resolved paths here definitely reference + * resources inside the container, and not outside of them. */ if (arg_chdir) if (chdir(arg_chdir) < 0) return log_error_errno(errno, "Failed to change to specified working directory %s: %m", arg_chdir);