From 0c7e2461a71aa18dc2ce4fb43af35fdf4ffc0a8c Mon Sep 17 00:00:00 2001 From: Rui Abreu Date: Sun, 23 Aug 2026 18:46:16 +0100 Subject: [PATCH 1/3] Validate the container id and invoke runc without a shell in reap-oci-container reap-oci-container now checks the container id with validate_container_id (hex digits and dashes, 36-42 chars) before it is used. validate_container_id is exported from oci_launch_cmd.c. cleanup_oci_container runs "runc delete" via fork/execv with an explicit argument vector instead of system(). Adds test_validate_container_id. Co-Authored-By: Claude Opus 4.8 --- .../src/native/worker-launcher/impl/main.c | 15 ++++++-- .../worker-launcher/impl/oci/oci_launch_cmd.c | 2 +- .../worker-launcher/impl/oci/oci_launch_cmd.h | 10 +++++ .../worker-launcher/impl/oci/oci_reap.c | 34 ++++++++++++----- .../test/test-worker-launcher.c | 37 +++++++++++++++++++ 5 files changed, 84 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 b3497a6b31e..3cb7b8b122e 100644 --- a/storm-core/src/native/worker-launcher/impl/main.c +++ b/storm-core/src/native/worker-launcher/impl/main.c @@ -19,6 +19,7 @@ #include "configuration.h" #include "worker-launcher.h" #include "oci/oci.h" +#include "oci/oci_launch_cmd.h" #include "oci/oci_reap.h" #include @@ -308,9 +309,17 @@ int main(int argc, char **argv) { } else { char* container_id = argv[optind++]; int num_reap_layers_keep = atoi(argv[optind]); - //becomes root. - setuid(0); - exit_code = cleanup_oci_container_by_id(container_id, num_reap_layers_keep); + // Validate the container id before it is used in a filesystem path or + // the runc command line. + if (!validate_container_id(container_id)) { + fprintf(ERRORFILE, "ERROR: Bad container id in reap-oci-container: %s\n", container_id); + fflush(ERRORFILE); + exit_code = INVALID_ARGUMENT_NUMBER; + } else { + //becomes root. + setuid(0); + exit_code = cleanup_oci_container_by_id(container_id, num_reap_layers_keep); + } } } else if (strcasecmp("profile-oci-container", command) == 0) { if (argc != 5) { 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 7ebe07d7bf7..8cd69af2c4e 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 @@ -445,7 +445,7 @@ static bool all_uuid_digit(const char* input) { return true; } -static bool validate_container_id(const char* input) { +bool validate_container_id(const char* input) { // The container id will be the same as the worker id, with a prefix of "PORTNUM-" // Worker id is a type 4 UUID, e.g. 85afb30b-286e-4d32-ab7a-9d5aad89bb88 // Container id for this worker on port 6702 would be: 6702-85afb30b-286e-4d32-ab7a-9d5aad89bb88 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 0a147cdcbd0..d637995984b 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 @@ -18,6 +18,8 @@ #ifndef OCI_OCI_LAUNCH_CMD_H #define OCI_OCI_LAUNCH_CMD_H +#include + #include "utils/cJSON.h" // NOTE: Update free_oci_launch_cmd when this is changed. @@ -66,4 +68,12 @@ void free_oci_launch_cmd(oci_launch_cmd* olc); */ oci_launch_cmd* parse_oci_launch_cmd(const char* command_filename); +/** + * Validate a container id: a type 4 UUID with an optional "PORTNUM-" + * prefix, i.e. only hex digits and dashes, 36 to 42 characters. + * + * Returns true if the id is well-formed. + */ +bool validate_container_id(const char* input); + #endif /* OCI_OCI_LAUNCH_CMD_H */ \ No newline at end of file diff --git a/storm-core/src/native/worker-launcher/impl/oci/oci_reap.c b/storm-core/src/native/worker-launcher/impl/oci/oci_reap.c index 62762a033de..876a4c12ebd 100644 --- a/storm-core/src/native/worker-launcher/impl/oci/oci_reap.c +++ b/storm-core/src/native/worker-launcher/impl/oci/oci_reap.c @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -751,20 +752,33 @@ int cleanup_oci_container(const char* container_id, const char* mount_path, cons } } - char* cmd = NULL; - if (asprintf(&cmd, "%s delete %s", runc_path, container_id) == -1) { - rc = 1; - goto cleanup; - } - - fprintf(LOGFILE, "oci cleanup container command: %s\n", cmd); - if (system(cmd) != 0) { - fprintf(ERRORFILE, "WARN: oci cleanup container command %s failed\n", cmd); + fprintf(LOGFILE, "oci cleanup container command: %s delete %s\n", runc_path, container_id); + // Invoke runc with an explicit argument vector via fork/execv rather than + // through system(). + pid_t child = fork(); + if (child == -1) { + fprintf(ERRORFILE, "WARN: Failed to fork to delete oci container %s : %s\n", + container_id, strerror(errno)); rc = 1; + } else if (child == 0) { + char* const delete_args[] = { + runc_path, "delete", (char*) container_id, NULL + }; + execv(runc_path, delete_args); + fprintf(ERRORFILE, "ERROR: Failed to exec %s delete %s : %s\n", + runc_path, container_id, strerror(errno)); + _exit(1); + } else { + int status = 0; + if (waitpid(child, &status, 0) == -1 + || !WIFEXITED(status) || WEXITSTATUS(status) != 0) { + fprintf(ERRORFILE, "WARN: oci cleanup container command %s delete %s failed\n", + runc_path, container_id); + rc = 1; + } } cleanup: free(runc_path); - free(cmd); return rc; } \ 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 4226d527d54..597c7d62920 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 @@ -17,6 +17,7 @@ */ #include "configuration.h" #include "worker-launcher.h" +#include "oci/oci_launch_cmd.h" #include #include @@ -150,6 +151,39 @@ void test_check_user() { } } +void test_validate_container_id() { + // well-formed: bare worker UUID and port-prefixed container id + if (!validate_container_id("85afb30b-286e-4d32-ab7a-9d5aad89bb88")) { + printf("FAIL: rejected valid worker id\n"); + exit(1); + } + if (!validate_container_id("6702-85afb30b-286e-4d32-ab7a-9d5aad89bb88")) { + printf("FAIL: rejected valid container id\n"); + exit(1); + } + // ids with characters outside [0-9a-fA-F-], or of the wrong length, are rejected + if (validate_container_id("6702-x nope; other stuff")) { + printf("FAIL: accepted id with disallowed characters\n"); + exit(1); + } + if (validate_container_id("6702/85afb30b-286e-4d32-ab7a-9d5aad89bb88")) { + printf("FAIL: accepted id containing a slash\n"); + exit(1); + } + if (validate_container_id("85afb30b-286e-4d32-ab7a-9d5aad89bb88-aaaaaaaa")) { + printf("FAIL: accepted id that is too long\n"); + exit(1); + } + if (validate_container_id("")) { + printf("FAIL: accepted empty id\n"); + exit(1); + } + if (validate_container_id("abc")) { + printf("FAIL: accepted too-short id\n"); + exit(1); + } +} + void test_check_configuration_permissions() { printf("\nTesting check_configuration_permissions\n"); if (check_configuration_permissions("/etc/passwd") != 0) { @@ -310,6 +344,9 @@ int main(int argc, char **argv) { printf("\nTesting check_user()\n"); test_check_user(); + printf("\nTesting validate_container_id()\n"); + test_validate_container_id(); + // 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 run_test_in_child("test_signal_container", test_signal_container); From 31b8cbe1325c14077a1ab968b5618281d7e17ab8 Mon Sep 17 00:00:00 2001 From: Rui Abreu Date: Sun, 23 Aug 2026 18:49:12 +0100 Subject: [PATCH 2/3] Validate the worker id in profile-docker-container profile-docker-container now checks the worker id with validate_container_id before get_docker_container_pid builds the docker command line, and get_docker_container_pid returns pid -1 instead of dereferencing a NULL stream when popen fails. Co-Authored-By: Claude Opus 4.8 --- storm-core/src/native/worker-launcher/impl/main.c | 12 ++++++++++-- .../native/worker-launcher/impl/worker-launcher.c | 6 ++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/storm-core/src/native/worker-launcher/impl/main.c b/storm-core/src/native/worker-launcher/impl/main.c index 3cb7b8b122e..a5542c7249b 100644 --- a/storm-core/src/native/worker-launcher/impl/main.c +++ b/storm-core/src/native/worker-launcher/impl/main.c @@ -243,8 +243,16 @@ int main(int argc, char **argv) { exit_code = INVALID_ARGUMENT_NUMBER; } else { const char * worker_id = argv[optind++]; - int pid = get_docker_container_pid(worker_id); - exit_code = profile_oci_container(pid, argv[optind]); + // Validate the worker id (a type 4 UUID, the same shape as a container + // id) before it is used to build the docker command line. + if (!validate_container_id(worker_id)) { + fprintf(ERRORFILE, "ERROR: Bad worker id in profile-docker-container: %s\n", worker_id); + fflush(ERRORFILE); + exit_code = INVALID_ARGUMENT_NUMBER; + } else { + int pid = get_docker_container_pid(worker_id); + exit_code = profile_oci_container(pid, argv[optind]); + } } } else if (strcasecmp("profiler", command) == 0) { if (argc != 5) { diff --git a/storm-core/src/native/worker-launcher/impl/worker-launcher.c b/storm-core/src/native/worker-launcher/impl/worker-launcher.c index 43b736ade55..a3f306bf75f 100644 --- a/storm-core/src/native/worker-launcher/impl/worker-launcher.c +++ b/storm-core/src/native/worker-launcher/impl/worker-launcher.c @@ -1249,6 +1249,12 @@ int get_docker_container_pid(const char *worker_id) { fflush(LOGFILE); FILE *inspect_docker = popen(docker_inspect_command, "r"); int pid = -1; + if (inspect_docker == NULL) { + fprintf(ERRORFILE, + "ERROR: Could not run %s in get_docker_container_pid\n", docker_inspect_command); + fflush(ERRORFILE); + goto cleanup; + } int res = fscanf(inspect_docker, "%d", &pid); if (pclose(inspect_docker) != 0 || res <= 0) { fprintf(ERRORFILE, From 84b51fa81ef3e50df3b6d727bb4a9c64c79691a9 Mon Sep 17 00:00:00 2001 From: Rui Abreu Date: Mon, 24 Aug 2026 10:13:18 +0100 Subject: [PATCH 3/3] Pass -- before the container id to runc delete, and test the character check at a valid length validate_container_id accepts a leading dash, so end runc option parsing with -- before the id in the delete argument vector. The test string that stood in for a disallowed-character id was short enough to be rejected on length; replace it with a 40-character one so it exercises the character check. Co-Authored-By: Claude Opus 4.8 --- storm-core/src/native/worker-launcher/impl/oci/oci_reap.c | 3 ++- .../src/native/worker-launcher/test/test-worker-launcher.c | 5 +++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/storm-core/src/native/worker-launcher/impl/oci/oci_reap.c b/storm-core/src/native/worker-launcher/impl/oci/oci_reap.c index 876a4c12ebd..cd4e4f73669 100644 --- a/storm-core/src/native/worker-launcher/impl/oci/oci_reap.c +++ b/storm-core/src/native/worker-launcher/impl/oci/oci_reap.c @@ -761,8 +761,9 @@ int cleanup_oci_container(const char* container_id, const char* mount_path, cons container_id, strerror(errno)); rc = 1; } else if (child == 0) { + // "--" ends option parsing so a container id is never treated as a runc flag. char* const delete_args[] = { - runc_path, "delete", (char*) container_id, NULL + runc_path, "delete", "--", (char*) container_id, NULL }; execv(runc_path, delete_args); fprintf(ERRORFILE, "ERROR: Failed to exec %s delete %s : %s\n", 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 597c7d62920..3aa659636f8 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 @@ -161,8 +161,9 @@ void test_validate_container_id() { printf("FAIL: rejected valid container id\n"); exit(1); } - // ids with characters outside [0-9a-fA-F-], or of the wrong length, are rejected - if (validate_container_id("6702-x nope; other stuff")) { + // ids with characters outside [0-9a-fA-F-], or of the wrong length, are rejected. + // this one is a valid length (40) so it exercises the character check, not the length check. + if (validate_container_id("6702-85afb30b-286e-4d32-ab7a-9d5aad89bZZ")) { printf("FAIL: accepted id with disallowed characters\n"); exit(1); }