This is an automated email from the ASF dual-hosted git repository.
rzo1 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/storm.git
The following commit(s) were added to refs/heads/master by this push:
new e33d74df9 Restrict OCI bind-mount sources to configured directories
and check the launch command username (#9010)
e33d74df9 is described below
commit e33d74df98155bfc464de58f466d7c05b517b28c
Author: reiabreu <[email protected]>
AuthorDate: Mon Aug 24 12:35:10 2026 +0100
Restrict OCI bind-mount sources to configured directories and check the
launch command username (#9010)
---
docs/OCI-support.md | 16 ++++
storm-core/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 +-
.../native/worker-launcher/impl/oci/oci_config.h | 6 ++
.../worker-launcher/impl/oci/oci_launch_cmd.c | 100 ++++++++++++++++++++-
.../worker-launcher/impl/oci/oci_launch_cmd.h | 20 +++++
.../worker-launcher/test/test-worker-launcher.c | 91 +++++++++++++++++++
8 files changed, 262 insertions(+), 14 deletions(-)
diff --git a/docs/OCI-support.md b/docs/OCI-support.md
index a44959ea1..57185b05d 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`) [...]
+
+```
+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 d99cec7d3..c4c96cc1a 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 b473c2ce0..8eca5d900 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 @@ umount_and_cleanup:
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 4e4e39717..625313b33 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 <stdbool.h>
+#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 5793380f9..c33cd9444 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 8cd69af2c..15eea76b8 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 d63799598..d8831bb62 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 48ad24d3b..2a5784540 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) {
@@ -407,6 +493,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);
run_test_in_child("test_get_values_degenerate", test_get_values_degenerate);
