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 6dd5355c5 Validate the container id and invoke runc without a shell in
reap-oci-container (#9008)
6dd5355c5 is described below
commit 6dd5355c5362927161c62a60f019b575f15a20ba
Author: reiabreu <[email protected]>
AuthorDate: Mon Aug 24 11:58:04 2026 +0100
Validate the container id and invoke runc without a shell in
reap-oci-container (#9008)
* 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 <[email protected]>
* 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 <[email protected]>
* 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 <[email protected]>
---------
Co-authored-by: Claude Opus 4.8 <[email protected]>
---
storm-core/src/native/worker-launcher/impl/main.c | 27 ++++++++++++---
.../worker-launcher/impl/oci/oci_launch_cmd.c | 2 +-
.../worker-launcher/impl/oci/oci_launch_cmd.h | 10 ++++++
.../src/native/worker-launcher/impl/oci/oci_reap.c | 35 ++++++++++++++------
.../native/worker-launcher/impl/worker-launcher.c | 6 ++++
.../worker-launcher/test/test-worker-launcher.c | 38 ++++++++++++++++++++++
6 files changed, 102 insertions(+), 16 deletions(-)
diff --git a/storm-core/src/native/worker-launcher/impl/main.c
b/storm-core/src/native/worker-launcher/impl/main.c
index 28e841e8d..d99cec7d3 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 <errno.h>
@@ -246,8 +247,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) {
@@ -312,9 +321,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 7ebe07d7b..8cd69af2c 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 0a147cdcb..d63799598 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 <stdbool.h>
+
#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 62762a033..cd4e4f736 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 <sys/types.h>
#include <sys/mount.h>
#include <sys/stat.h>
+#include <sys/wait.h>
#include <dirent.h>
#include <errno.h>
#include <fcntl.h>
@@ -751,20 +752,34 @@ 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) {
+ // "--" 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
+ };
+ 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/impl/worker-launcher.c
b/storm-core/src/native/worker-launcher/impl/worker-launcher.c
index 3d1910105..53b8abef8 100644
--- a/storm-core/src/native/worker-launcher/impl/worker-launcher.c
+++ b/storm-core/src/native/worker-launcher/impl/worker-launcher.c
@@ -1276,6 +1276,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,
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 4226d527d..3aa659636 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 <errno.h>
#include <fcntl.h>
@@ -150,6 +151,40 @@ 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.
+ // 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);
+ }
+ 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 +345,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);
