Restrict OCI bind-mount sources to configured directories and check the launch command username

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"

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) {

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;
 }

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.

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 */

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;
 }

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 */

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);