diff --git a/docs/OCI-support.md b/docs/OCI-support.md index a44959ea1c..57185b05d8 100644 --- a/docs/OCI-support.md +++ b/docs/OCI-support.md @@ -315,6 +315,22 @@ 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 (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, 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 +``` + For example, ```bash storm.resource.isolation.plugin: "org.apache.storm.container.oci.RuncLibContainerManager" diff --git a/storm-core/src/native/worker-launcher/impl/main.c b/storm-core/src/native/worker-launcher/impl/main.c index d99cec7d3a..c4c96cc1a1 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 b473c2ce00..8eca5d900d 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 4e4e39717d..625313b334 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 5793380f95..c33cd94441 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 8cd69af2c4..15eea76b8b 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,100 @@ 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. + * + * 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)) { + fprintf(ERRORFILE, + "ERROR: OCI config mount source is not a normalized absolute path: %s\n", + 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) { + 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; + } + } + 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); + } + free(resolved_source); + 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 +452,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 d637995984..d8831bb627 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 3aa659636f..4b8f8eca89 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,92 @@ 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 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=%s\n", allowed); + fclose(f); + read_config(cfg); + + 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. + 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(allowed), "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 +436,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);