Validate the container id and invoke runc without a shell in reap-oci-container

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>
@@ -242,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) {
@@ -308,9 +317,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) {

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

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

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

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,

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