From 07ac4d553130a3556271305ea4d6cc731f58c71c Mon Sep 17 00:00:00 2001 From: Rui Abreu Date: Sun, 23 Aug 2026 18:54:24 +0100 Subject: [PATCH 1/3] Restrict OCI bind-mount sources to configured directories and check the launch command username run-oci-container parses the launch command file before setup_dir_permissions changes the worker directory's ownership, and requires the command file's username to match the user passed to the worker-launcher. run_oci_container now takes the parsed oci_launch_cmd, which main owns and frees. Bind-mount sources must be absolute paths with no "." or ".." component, equal to or under a directory listed in the new worker.launcher.oci.allowed.mount.source.dirs config key; if none are configured, all sources are rejected. Adds test_mount_path_helpers and test_mount_source_allowed_dirs. Co-Authored-By: Claude Opus 4.8 --- .../src/native/worker-launcher/impl/main.c | 28 +++++-- .../src/native/worker-launcher/impl/oci/oci.c | 7 +- .../src/native/worker-launcher/impl/oci/oci.h | 8 +- .../worker-launcher/impl/oci/oci_config.h | 6 ++ .../worker-launcher/impl/oci/oci_launch_cmd.c | 76 ++++++++++++++++++- .../worker-launcher/impl/oci/oci_launch_cmd.h | 20 +++++ .../test/test-worker-launcher.c | 64 ++++++++++++++++ 7 files changed, 195 insertions(+), 14 deletions(-) diff --git a/storm-core/src/native/worker-launcher/impl/main.c b/storm-core/src/native/worker-launcher/impl/main.c index d99cec7d3aa..c4c96cc1a16 100644 --- a/storm-core/src/native/worker-launcher/impl/main.c +++ b/storm-core/src/native/worker-launcher/impl/main.c @@ -302,15 +302,31 @@ int main(int argc, char **argv) { working_dir = argv[optind++]; const char* command_file = argv[optind++]; const char* worker_artifacts_dir = argv[optind]; - exit_code = setup_dir_permissions(working_dir, 1, TRUE); - if (exit_code == 0) { - exit_code = setup_worker_tmp_permissions(working_dir); + // Parse the launch command file before setup_dir_permissions changes + // the ownership of the worker directory (which contains the command + // file) below. + oci_launch_cmd* olc = parse_oci_launch_cmd(command_file); + if (olc == NULL) { + exit_code = INVALID_CONFIG_FILE; + } else if (strcmp(olc->username, user_name) != 0) { + // The launch command file's username must match the user passed to + // the worker-launcher on the command line. + fprintf(ERRORFILE, "ERROR: OCI command file username %s does not match %s\n", + olc->username, user_name); + fflush(ERRORFILE); + exit_code = INVALID_USER_NAME; + } else { + exit_code = setup_dir_permissions(working_dir, 1, TRUE); if (exit_code == 0) { - //becomes root. - setuid(0); - exit_code = run_oci_container(command_file, worker_artifacts_dir); + exit_code = setup_worker_tmp_permissions(working_dir); + if (exit_code == 0) { + //becomes root. + setuid(0); + exit_code = run_oci_container(olc, worker_artifacts_dir); + } } } + free_oci_launch_cmd(olc); } } else if (strcasecmp("reap-oci-container", command) == 0) { if (argc != 5) { diff --git a/storm-core/src/native/worker-launcher/impl/oci/oci.c b/storm-core/src/native/worker-launcher/impl/oci/oci.c index b473c2ce00a..8eca5d900d0 100644 --- a/storm-core/src/native/worker-launcher/impl/oci/oci.c +++ b/storm-core/src/native/worker-launcher/impl/oci/oci.c @@ -797,10 +797,9 @@ static void exec_runc(const char* container_id, const char* runc_config_path, exit(ERROR_OCI_RUN_FAILED); } -int run_oci_container(const char* command_file, const char* worker_artifacts_dir) { +int run_oci_container(oci_launch_cmd* olc, const char* worker_artifacts_dir) { int rc = 0; char* runc_config_path = NULL; - oci_launch_cmd* olc = NULL; oci_launch_cmd_ctx* ctx = setup_oci_launch_cmd_ctx(); if (ctx == NULL) { @@ -809,9 +808,8 @@ int run_oci_container(const char* command_file, const char* worker_artifacts_dir goto cleanup; } - olc = parse_oci_launch_cmd(command_file); if (olc == NULL) { - fputs("ERROR: parse_oci_launch_cmd Failed\n", ERRORFILE); + fputs("ERROR: no parsed OCI launch command in run_oci_container\n", ERRORFILE); rc = INVALID_CONFIG_FILE; goto cleanup; } @@ -875,7 +873,6 @@ int run_oci_container(const char* command_file, const char* worker_artifacts_dir cleanup: free(runc_config_path); - free_oci_launch_cmd(olc); free_oci_launch_cmd_ctx(ctx); return rc; } diff --git a/storm-core/src/native/worker-launcher/impl/oci/oci.h b/storm-core/src/native/worker-launcher/impl/oci/oci.h index 4e4e39717d9..625313b334e 100644 --- a/storm-core/src/native/worker-launcher/impl/oci/oci.h +++ b/storm-core/src/native/worker-launcher/impl/oci/oci.h @@ -20,10 +20,14 @@ #include +#include "oci_launch_cmd.h" + /** - * Run a container via OCI. + * Run a container via OCI from an already parsed launch command. The caller + * parses the command file with parse_oci_launch_cmd and owns the returned + * structure. */ -int run_oci_container(const char* command_file, const char* worker_artifacts_dir); +int run_oci_container(oci_launch_cmd* olc, const char* worker_artifacts_dir); // NOTE: Update init_oci_overlay_desc and destroy_oci_overlay_desc // when this is changed. diff --git a/storm-core/src/native/worker-launcher/impl/oci/oci_config.h b/storm-core/src/native/worker-launcher/impl/oci/oci_config.h index 5793380f95d..c33cd94441f 100644 --- a/storm-core/src/native/worker-launcher/impl/oci/oci_config.h +++ b/storm-core/src/native/worker-launcher/impl/oci/oci_config.h @@ -30,4 +30,10 @@ #define OCI_RUNC_CONFIG_KEY OCI_CONFIG_PREFIX "runc" #define DEFAULT_OCI_RUNC "/usr/bin/runc" +// Configuration for the comma-separated list of directories that bind-mount +// sources in an OCI launch command may come from. A mount source is allowed +// if it is one of the listed paths or a path underneath one of them. +// If this is not set, no bind mounts are allowed. +#define OCI_ALLOWED_MOUNT_SRCS_CONFIG_KEY OCI_CONFIG_PREFIX "allowed.mount.source.dirs" + #endif /* OCI_OCI_CONFIG_H */ diff --git a/storm-core/src/native/worker-launcher/impl/oci/oci_launch_cmd.c b/storm-core/src/native/worker-launcher/impl/oci/oci_launch_cmd.c index 8cd69af2c4e..4503e2d6299 100644 --- a/storm-core/src/native/worker-launcher/impl/oci/oci_launch_cmd.c +++ b/storm-core/src/native/worker-launcher/impl/oci/oci_launch_cmd.c @@ -27,7 +27,9 @@ #include "utils/file-utils.h" #include "utils/string-utils.h" +#include "configuration.h" #include "worker-launcher.h" +#include "oci_config.h" #include "oci_launch_cmd.h" #define SQUASHFS_MEDIA_TYPE "application/vnd.squashfs" @@ -297,6 +299,76 @@ static bool is_valid_mount_options(const cJSON* mo) { return true; } +/** + * Check whether a path contains a "." or ".." component. + */ +bool has_relative_path_component(const char* path) { + const char* p = path; + while (*p != '\0') { + while (*p == '/') { + ++p; + } + const char* start = p; + while (*p != '\0' && *p != '/') { + ++p; + } + size_t len = (size_t)(p - start); + if ((len == 1 && start[0] == '.') + || (len == 2 && start[0] == '.' && start[1] == '.')) { + return true; + } + } + return false; +} + +/** + * Check whether the mount source equals the allowed path or is underneath it. + */ +bool is_mount_source_under(const char* source, const char* allowed) { + size_t allowed_len = strlen(allowed); + while (allowed_len > 1 && allowed[allowed_len - 1] == '/') { + --allowed_len; + } + if (strncmp(source, allowed, allowed_len) != 0) { + return false; + } + return source[allowed_len] == '\0' || source[allowed_len] == '/'; +} + +/** + * Check a mount source against the directories configured in + * worker-launcher.cfg under OCI_ALLOWED_MOUNT_SRCS_CONFIG_KEY. The source + * must be an absolute path with no "." or ".." component and must be equal + * to or under one of the configured directories. If none are configured, + * the source is rejected. + */ +bool is_valid_mount_source(const char* source) { + if (source[0] != '/' || has_relative_path_component(source)) { + fprintf(ERRORFILE, + "ERROR: OCI config mount source is not a normalized absolute path: %s\n", + source); + return false; + } + bool allowed = false; + char** allowed_dirs = get_values(OCI_ALLOWED_MOUNT_SRCS_CONFIG_KEY); + if (allowed_dirs != NULL) { + char** entry; + for (entry = allowed_dirs; *entry != NULL; ++entry) { + if (is_mount_source_under(source, *entry)) { + allowed = true; + break; + } + } + free_values(allowed_dirs); + } + if (!allowed) { + fprintf(ERRORFILE, + "ERROR: OCI config mount source %s is not under any directory in %s\n", + source, OCI_ALLOWED_MOUNT_SRCS_CONFIG_KEY); + } + return allowed; +} + static bool is_valid_mount(const cJSON* mount) { if (!cJSON_IsObject(mount)) { fputs("ERROR: OCI config mount entry is not an object\n", ERRORFILE); @@ -356,7 +428,9 @@ static bool is_valid_mount(const cJSON* mount) { return false; } - // TODO: Need to add mount source/dest whitelist checking here. + if (!is_valid_mount_source(source)) { + return false; + } return true; } diff --git a/storm-core/src/native/worker-launcher/impl/oci/oci_launch_cmd.h b/storm-core/src/native/worker-launcher/impl/oci/oci_launch_cmd.h index d637995984b..d8831bb627f 100644 --- a/storm-core/src/native/worker-launcher/impl/oci/oci_launch_cmd.h +++ b/storm-core/src/native/worker-launcher/impl/oci/oci_launch_cmd.h @@ -76,4 +76,24 @@ oci_launch_cmd* parse_oci_launch_cmd(const char* command_filename); */ bool validate_container_id(const char* input); +/** + * Return true if the path contains a "." or ".." component. + */ +bool has_relative_path_component(const char* path); + +/** + * Return true if source equals allowed or is a path underneath it. Trailing + * slashes on allowed are ignored, and matching is on whole path components, + * so "/data/storm" matches "/data/storm/x" but not "/data/storm-evil". + */ +bool is_mount_source_under(const char* source, const char* allowed); + +/** + * Return true if source is a normalized absolute path that is equal to or + * under one of the directories configured in + * worker.launcher.oci.allowed.mount.source.dirs. If no such directories are + * configured, every source is rejected. + */ +bool is_valid_mount_source(const char* source); + #endif /* OCI_OCI_LAUNCH_CMD_H */ \ No newline at end of file diff --git a/storm-core/src/native/worker-launcher/test/test-worker-launcher.c b/storm-core/src/native/worker-launcher/test/test-worker-launcher.c index 3aa659636f8..03115c2ed60 100644 --- a/storm-core/src/native/worker-launcher/test/test-worker-launcher.c +++ b/storm-core/src/native/worker-launcher/test/test-worker-launcher.c @@ -185,6 +185,65 @@ void test_validate_container_id() { } } +// Fail the test (with a message) unless the condition holds. +#define EXPECT(cond, msg) do { if (!(cond)) { printf("FAIL: %s\n", (msg)); exit(1); } } while (0) + +// Path helpers used by is_valid_mount_source. No config needed. +void test_mount_path_helpers() { + // has_relative_path_component: only real "." / ".." components count + EXPECT(!has_relative_path_component("/data/storm"), "clean path flagged as relative"); + EXPECT(!has_relative_path_component("/data/storm/resolv.conf"), "clean nested path flagged as relative"); + EXPECT(has_relative_path_component("/data/../etc"), "'..' component not detected"); + EXPECT(has_relative_path_component("/data/./x"), "'.' component not detected"); + EXPECT(has_relative_path_component("/.."), "leading '..' not detected"); + EXPECT(has_relative_path_component("/a/b/.."), "trailing '..' not detected"); + EXPECT(!has_relative_path_component("/data/..storm"), "'..storm' wrongly treated as '..'"); + EXPECT(!has_relative_path_component("/data/storm."), "'storm.' wrongly treated as '.'"); + EXPECT(!has_relative_path_component(""), "empty path flagged as relative"); + + // is_mount_source_under: whole-component containment + EXPECT(is_mount_source_under("/data/storm/x", "/data/storm"), "path under allowed dir rejected"); + EXPECT(is_mount_source_under("/data/storm", "/data/storm"), "allowed dir itself rejected"); + EXPECT(is_mount_source_under("/data/storm/a/b", "/data/storm"), "deep path under allowed dir rejected"); + EXPECT(!is_mount_source_under("/data/storm-evil/x", "/data/storm"), "dir with shared name prefix accepted"); + EXPECT(!is_mount_source_under("/data/stormx", "/data/storm"), "prefix without separator accepted"); + EXPECT(is_mount_source_under("/data/storm/x", "/data/storm/"), "trailing slash in allowed dir not tolerated"); + EXPECT(is_mount_source_under("/data/storm", "/data/storm/"), "trailing slash vs equal path not tolerated"); + EXPECT(!is_mount_source_under("/data", "/data/storm"), "parent of allowed dir accepted"); + EXPECT(!is_mount_source_under("/etc/passwd", "/data/storm"), "unrelated path accepted"); +} + +// is_valid_mount_source reads global config, so run it via run_test_in_child to +// keep read_config from leaking into later tests. The rejected cases print to +// stderr, which is expected. +void test_mount_source_allowed_dirs() { + const char* cfg = TEST_ROOT "/mount-allowed.cfg"; + FILE* f = fopen(cfg, "w"); + EXPECT(f != NULL, "could not write mount-allowed.cfg"); + fprintf(f, "min.user.id=%d\n", getuid()); + fprintf(f, "worker.launcher.oci.allowed.mount.source.dirs=/data/storm,/etc/storm-mounts/\n"); + fclose(f); + read_config(cfg); + + EXPECT(is_valid_mount_source("/data/storm"), "allowed dir itself rejected"); + EXPECT(is_valid_mount_source("/data/storm/resolv.conf"), "path under allowed dir rejected"); + EXPECT(is_valid_mount_source("/etc/storm-mounts/hosts"), "path under trailing-slash entry rejected"); + EXPECT(!is_valid_mount_source("/data/storm-evil/x"), "dir with shared name prefix accepted"); + EXPECT(!is_valid_mount_source("/etc/passwd"), "path outside all allowed dirs accepted"); + EXPECT(!is_valid_mount_source("/data/storm/../etc/passwd"), "path with '..' accepted"); + EXPECT(!is_valid_mount_source("relative/path"), "non-absolute path accepted"); + + // With no directories configured, every source is rejected. + const char* cfg_none = TEST_ROOT "/mount-none.cfg"; + f = fopen(cfg_none, "w"); + EXPECT(f != NULL, "could not write mount-none.cfg"); + fprintf(f, "min.user.id=%d\n", getuid()); + fclose(f); + read_config(cfg_none); + + EXPECT(!is_valid_mount_source("/data/storm/x"), "source accepted with no directories configured"); +} + void test_check_configuration_permissions() { printf("\nTesting check_configuration_permissions\n"); if (check_configuration_permissions("/etc/passwd") != 0) { @@ -350,6 +409,11 @@ int main(int argc, char **argv) { // the tests that change user need to be run in a subshell, so that // when they change user they don't give up our privs + printf("\nTesting mount path helpers\n"); + test_mount_path_helpers(); + + run_test_in_child("test_mount_source_allowed_dirs", test_mount_source_allowed_dirs); + run_test_in_child("test_signal_container", test_signal_container); run_test_in_child("test_signal_container_group", test_signal_container_group); From 4cd2ef697984e7c1e5f96925ae158e6cdcfd6f13 Mon Sep 17 00:00:00 2001 From: Rui Abreu Date: Sun, 23 Aug 2026 19:20:27 +0100 Subject: [PATCH 2/3] Document the OCI allowed bind-mount source directories setting Adds an OCI-support.md section describing worker.launcher.oci.allowed.mount.source.dirs (set in worker-launcher.cfg): OCI/runc bind-mount sources must be equal to or under one of the configured directories, and OCI workers do not start when it is unset. Co-Authored-By: Claude Opus 4.8 --- docs/OCI-support.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/docs/OCI-support.md b/docs/OCI-support.md index a44959ea1c3..ed204564dfb 100644 --- a/docs/OCI-support.md +++ b/docs/OCI-support.md @@ -315,6 +315,20 @@ Then you need to set up storm with the following configs: | `storm.oci.resources.localizer` | The plugin to use for oci resources localization. | | `storm.oci.resources.local.dir` | The local directory for localized oci resources. | +### Allowed OCI bind-mount sources + +Bind-mount sources for OCI/runc workers are restricted to a configured set of directories, set in the root-owned `worker-launcher.cfg`: + +| Setting | Description | +|---------|-------------| +| `worker.launcher.oci.allowed.mount.source.dirs` | Comma-separated list of directories from which OCI bind-mount sources may come. A mount source is permitted only if it is one of these directories or a path underneath one of them. | + +If `worker.launcher.oci.allowed.mount.source.dirs` is not set, no bind mounts are permitted and OCI/runc workers will not start. When enabling the OCI/runc manager, set it to the directories the supervisor mounts — this includes the paths in `storm.oci.readonly.bindmounts` and `storm.oci.readwrite.bindmounts`, and the system paths the runtime needs (for example `/etc/resolv.conf`, `/etc/hostname`, `/etc/hosts`, the nscd directory, the Storm home directory, the cgroup root, the supervisor local directory, and the worker/artifacts/tmp roots). For example, in `worker-launcher.cfg`: + +``` +worker.launcher.oci.allowed.mount.source.dirs=/etc/resolv.conf,/etc/hostname,/etc/hosts,/var/run/nscd,/usr/lib/storm,/sys/fs/cgroup,/data/storm +``` + For example, ```bash storm.resource.isolation.plugin: "org.apache.storm.container.oci.RuncLibContainerManager" From 25c09eee55dd93d71177ce0a411a3090bf8cc8e6 Mon Sep 17 00:00:00 2001 From: Rui Abreu Date: Mon, 24 Aug 2026 10:32:34 +0100 Subject: [PATCH 3/3] Resolve OCI bind-mount sources with realpath before the allowed-directory check is_valid_mount_source now resolves the mount source and each configured directory with realpath() before the containment check, so an entry whose spelling is under an allowed directory but whose target is elsewhere is matched on its resolved target. Sources that cannot be resolved are rejected. The docs note that a source is resolved again by runc at mount time, so only directories the topology user cannot write to should be listed, drop the writable-tree example, and note that "/" is not a usable entry. test_mount_source_allowed_dirs builds a real tree (including a symlink that points outside the allowed directory) to cover this. Co-Authored-By: Claude Opus 4.8 --- docs/OCI-support.md | 8 +-- .../worker-launcher/impl/oci/oci_launch_cmd.c | 26 +++++++++- .../test/test-worker-launcher.c | 49 ++++++++++++++----- 3 files changed, 68 insertions(+), 15 deletions(-) diff --git a/docs/OCI-support.md b/docs/OCI-support.md index ed204564dfb..57185b05d8f 100644 --- a/docs/OCI-support.md +++ b/docs/OCI-support.md @@ -321,12 +321,14 @@ Bind-mount sources for OCI/runc workers are restricted to a configured set of di | Setting | Description | |---------|-------------| -| `worker.launcher.oci.allowed.mount.source.dirs` | Comma-separated list of directories from which OCI bind-mount sources may come. A mount source is permitted only if it is one of these directories or a path underneath one of them. | +| `worker.launcher.oci.allowed.mount.source.dirs` | Comma-separated list of directories from which OCI bind-mount sources may come. A mount source is permitted only if it (resolved with `realpath`) is one of these directories or a path underneath one of them. | -If `worker.launcher.oci.allowed.mount.source.dirs` is not set, no bind mounts are permitted and OCI/runc workers will not start. When enabling the OCI/runc manager, set it to the directories the supervisor mounts — this includes the paths in `storm.oci.readonly.bindmounts` and `storm.oci.readwrite.bindmounts`, and the system paths the runtime needs (for example `/etc/resolv.conf`, `/etc/hostname`, `/etc/hosts`, the nscd directory, the Storm home directory, the cgroup root, the supervisor local directory, and the worker/artifacts/tmp roots). For example, in `worker-launcher.cfg`: +If `worker.launcher.oci.allowed.mount.source.dirs` is not set, no bind mounts are permitted and OCI/runc workers will not start. When enabling the OCI/runc manager, set it to the directories the supervisor mounts — this includes the paths in `storm.oci.readonly.bindmounts` and `storm.oci.readwrite.bindmounts`, and the system paths the runtime needs (for example `/etc/resolv.conf`, `/etc/hostname`, `/etc/hosts`, the nscd directory, the Storm home directory, and the cgroup root). + +List only directories that the container (topology) user cannot write to. A source is resolved with `realpath` before it is matched, but runc resolves it again at mount time, so a directory that the topology user can write to — such as the supervisor local directory or the worker/artifacts/tmp roots — could have a symlink planted in it that points outside the allowed set. Point bind mounts at specific, non-writable paths (for example the exact files under `storm.oci.readonly.bindmounts`) rather than at a directory the topology user owns. List specific directories: the root directory `/` is not a usable entry, since a path is only accepted when it equals or sits under a listed directory as a whole component. For example, in `worker-launcher.cfg`: ``` -worker.launcher.oci.allowed.mount.source.dirs=/etc/resolv.conf,/etc/hostname,/etc/hosts,/var/run/nscd,/usr/lib/storm,/sys/fs/cgroup,/data/storm +worker.launcher.oci.allowed.mount.source.dirs=/etc/resolv.conf,/etc/hostname,/etc/hosts,/var/run/nscd,/usr/lib/storm,/sys/fs/cgroup ``` For example, diff --git a/storm-core/src/native/worker-launcher/impl/oci/oci_launch_cmd.c b/storm-core/src/native/worker-launcher/impl/oci/oci_launch_cmd.c index 4503e2d6299..15eea76b8bf 100644 --- a/storm-core/src/native/worker-launcher/impl/oci/oci_launch_cmd.c +++ b/storm-core/src/native/worker-launcher/impl/oci/oci_launch_cmd.c @@ -341,6 +341,15 @@ bool is_mount_source_under(const char* source, const char* allowed) { * must be an absolute path with no "." or ".." component and must be equal * to or under one of the configured directories. If none are configured, * the source is rejected. + * + * The source and the configured directories are resolved with realpath() + * before the containment check, so a symlink whose textual path is under an + * allowed directory but which points outside of it is rejected on its + * resolved target rather than on its spelling. A source that cannot be + * resolved (for example one that does not exist) is rejected. This narrows + * but does not fully close the window, since runc resolves the path again at + * mount time; only allow-list directories that the container user cannot + * write to. */ bool is_valid_mount_source(const char* source) { if (source[0] != '/' || has_relative_path_component(source)) { @@ -349,13 +358,27 @@ bool is_valid_mount_source(const char* source) { source); return false; } + char* resolved_source = realpath(source, NULL); + if (resolved_source == NULL) { + fprintf(ERRORFILE, "ERROR: Cannot resolve OCI config mount source %s: %s\n", + source, strerror(errno)); + return false; + } bool allowed = false; char** allowed_dirs = get_values(OCI_ALLOWED_MOUNT_SRCS_CONFIG_KEY); if (allowed_dirs != NULL) { char** entry; for (entry = allowed_dirs; *entry != NULL; ++entry) { - if (is_mount_source_under(source, *entry)) { + char* resolved_allowed = realpath(*entry, NULL); + if (resolved_allowed == NULL) { + // a configured directory that cannot be resolved cannot contain anything + continue; + } + if (is_mount_source_under(resolved_source, resolved_allowed)) { allowed = true; + } + free(resolved_allowed); + if (allowed) { break; } } @@ -366,6 +389,7 @@ bool is_valid_mount_source(const char* source) { "ERROR: OCI config mount source %s is not under any directory in %s\n", source, OCI_ALLOWED_MOUNT_SRCS_CONFIG_KEY); } + free(resolved_source); return allowed; } diff --git a/storm-core/src/native/worker-launcher/test/test-worker-launcher.c b/storm-core/src/native/worker-launcher/test/test-worker-launcher.c index 03115c2ed60..4b8f8eca895 100644 --- a/storm-core/src/native/worker-launcher/test/test-worker-launcher.c +++ b/storm-core/src/native/worker-launcher/test/test-worker-launcher.c @@ -213,24 +213,51 @@ void test_mount_path_helpers() { EXPECT(!is_mount_source_under("/etc/passwd", "/data/storm"), "unrelated path accepted"); } -// is_valid_mount_source reads global config, so run it via run_test_in_child to -// keep read_config from leaking into later tests. The rejected cases print to -// stderr, which is expected. +// is_valid_mount_source resolves paths with realpath and reads global config, so it needs a real +// tree and is run via run_test_in_child to keep read_config from leaking into later tests. The +// rejected cases print to stderr, which is expected. void test_mount_source_allowed_dirs() { + // A real tree, since is_valid_mount_source now resolves with realpath: + // .../allowed an allow-listed directory + // .../allowed/real.conf a real file under it + // .../allowed/escape a symlink under it that points outside it + // .../allowed-evil a sibling whose name shares the prefix + // .../outside/secret a file outside every allowed directory + const char* base = TEST_ROOT "/mounts"; + const char* allowed = TEST_ROOT "/mounts/allowed"; + const char* real_conf = TEST_ROOT "/mounts/allowed/real.conf"; + const char* escape = TEST_ROOT "/mounts/allowed/escape"; + const char* evil = TEST_ROOT "/mounts/allowed-evil"; + const char* outside = TEST_ROOT "/mounts/outside"; + const char* secret = TEST_ROOT "/mounts/outside/secret"; + + EXPECT(mkdir(base, 0755) == 0 || errno == EEXIST, "could not create mounts base"); + EXPECT(mkdir(allowed, 0755) == 0 || errno == EEXIST, "could not create allowed dir"); + EXPECT(mkdir(evil, 0755) == 0 || errno == EEXIST, "could not create sibling dir"); + EXPECT(mkdir(outside, 0755) == 0 || errno == EEXIST, "could not create outside dir"); + FILE* rc = fopen(real_conf, "w"); + EXPECT(rc != NULL, "could not create real.conf"); + fclose(rc); + FILE* sc = fopen(secret, "w"); + EXPECT(sc != NULL, "could not create secret"); + fclose(sc); + EXPECT(symlink(secret, escape) == 0 || errno == EEXIST, "could not create escaping symlink"); + const char* cfg = TEST_ROOT "/mount-allowed.cfg"; FILE* f = fopen(cfg, "w"); EXPECT(f != NULL, "could not write mount-allowed.cfg"); fprintf(f, "min.user.id=%d\n", getuid()); - fprintf(f, "worker.launcher.oci.allowed.mount.source.dirs=/data/storm,/etc/storm-mounts/\n"); + fprintf(f, "worker.launcher.oci.allowed.mount.source.dirs=%s\n", allowed); fclose(f); read_config(cfg); - EXPECT(is_valid_mount_source("/data/storm"), "allowed dir itself rejected"); - EXPECT(is_valid_mount_source("/data/storm/resolv.conf"), "path under allowed dir rejected"); - EXPECT(is_valid_mount_source("/etc/storm-mounts/hosts"), "path under trailing-slash entry rejected"); - EXPECT(!is_valid_mount_source("/data/storm-evil/x"), "dir with shared name prefix accepted"); - EXPECT(!is_valid_mount_source("/etc/passwd"), "path outside all allowed dirs accepted"); - EXPECT(!is_valid_mount_source("/data/storm/../etc/passwd"), "path with '..' accepted"); + EXPECT(is_valid_mount_source(allowed), "allowed dir itself rejected"); + EXPECT(is_valid_mount_source(real_conf), "real path under allowed dir rejected"); + // the key case: a symlink under the allowed dir that resolves outside it must be rejected + EXPECT(!is_valid_mount_source(escape), "symlink resolving outside the allowed dir accepted"); + EXPECT(!is_valid_mount_source(secret), "path outside all allowed dirs accepted"); + EXPECT(!is_valid_mount_source(evil), "sibling dir with shared name prefix accepted"); + EXPECT(!is_valid_mount_source(TEST_ROOT "/mounts/does-not-exist"), "unresolvable source accepted"); EXPECT(!is_valid_mount_source("relative/path"), "non-absolute path accepted"); // With no directories configured, every source is rejected. @@ -241,7 +268,7 @@ void test_mount_source_allowed_dirs() { fclose(f); read_config(cfg_none); - EXPECT(!is_valid_mount_source("/data/storm/x"), "source accepted with no directories configured"); + EXPECT(!is_valid_mount_source(allowed), "source accepted with no directories configured"); } void test_check_configuration_permissions() {