Better isolation for malicious python submissions

Author: jmgasper

ecs-runner/README.md

@@ -19,7 +19,7 @@ This image is the runtime container for marathon match scoring tasks launched by
 - The container entrypoint starts as `root`. Do not override the ECS task-definition `user` for this container; root is needed for trusted bootstrap work and for preparing runner-owned files before the child JVM starts.
 - The trusted parent runner performs network bootstrap work: fetch challenge config, download tester/submission artifacts, upload artifacts, and post the scoring callback.
 - The tester executes in a separate child JVM launched through `mm-runner-isolate` as uid/gid `10001` (`runner`) with a scrubbed environment, so `ACCESS_TOKEN` and other runner env vars are not inherited by untrusted code.
-- Generic submitted solution commands execute through the setuid-root `mm-scorer-isolate` bridge as uid/gid `10002` (`scorer`). The bridge drops its supervisor back to the invoking `runner` uid after it forks the solution child, then supervises the solution process group so tester timeouts can still terminate lower-privilege processes.
+- Generic submitted solution commands execute through the setuid-root `mm-scorer-isolate` bridge as uid/gid `10002` (`scorer`). The bridge drops its supervisor back to the invoking `runner` uid after it forks the solution child, retaining only `CAP_KILL` so tester timeouts can still terminate the lower-privilege solution process group.
 - Downloaded tester JARs are mode `0400` runner-owned files. The serialized scorer config starts as a mode `0400` runner-owned handoff file and the child JVM deletes it immediately after loading it, before tester or submitted solution code executes.
 - Generic Marathon seed execution resets scorer-owned writable state, including `/tmp`, before and after every test case, so files written by one seed are not visible to later seeds in the same submission run.
 - Generic submitted solution commands run under a Landlock filesystem allowlist. Runtime and toolchain files are readable, `/tmp` and the scorer home are writable, and infrastructure-revealing paths such as `/etc/hostname`, `/etc/resolv.conf`, `/proc/self/cgroup`, `/proc/self/mounts`, and proc network tables are not readable by submitted code.

ecs-runner/boilerplate/src/main/java/com/topcoder/marathon/MarathonTester.java

@@ -35,6 +35,7 @@ public abstract class MarathonTester {
     private final List<BufferedWriter> solInputWriters = new ArrayList<BufferedWriter>();
     private static final int maxSolutionOutputLength = 10_000_000;
     private static final long processDrainTimeoutMillis = 250;
+    private static final long processTimeoutDestroyMillis = 250;
     private static final long errorReaderDrainTimeoutMillis = 250;
     private BufferedWriter solOutputWriter;
     private BufferedWriter solErrorWriter;
@@ -110,17 +111,20 @@ protected final void startTime() {
                 lastTimeoutThread = new Thread() {
                     public void run() {
                         try {
-                            boolean finished = process.waitFor(timeLimit - elapsedTime, TimeUnit.NANOSECONDS);
+                            long remainingTime = Math.max(1L, timeLimit - elapsedTime);
+                            boolean finished = process.waitFor(remainingTime, TimeUnit.NANOSECONDS);
                             if (!finished) {
+                                boolean notifyTimeout = false;
                                 synchronized (timeLock) {
                                     if (lastStart > 0) elapsedTime += System.nanoTime() - lastStart;
                                     lastStart = 0;
-                                    if (process != null) process.destroyForcibly();
                                     if (!timeout) {
                                         timeout = true;
-                                        timeout();
+                                        notifyTimeout = true;
                                     }
                                 }
+                                terminateTimedOutProcess();
+                                if (notifyTimeout) timeout();
                             }
                         } catch (Exception e) {
                         }
@@ -186,6 +190,73 @@ public final double runTest() {
         return score;
     }
 
+    /**
+     * Terminates the submitted solution process after its measured execution
+     * time expires and closes runner-side streams so blocked tester reads unwind.
+     *
+     * <p>The ECS scorer command is wrapped by a small native supervisor. A
+     * graceful destroy gives that wrapper a catchable signal it can relay to the
+     * low-privilege scorer process group before this method escalates to a hard
+     * process kill.
+     */
+    private void terminateTimedOutProcess() {
+        Process processToStop = process;
+        if (processToStop != null) {
+            processToStop.destroy();
+            try {
+                if (!processToStop.waitFor(processTimeoutDestroyMillis, TimeUnit.MILLISECONDS)) {
+                    processToStop.destroyForcibly();
+                    processToStop.waitFor(processDrainTimeoutMillis, TimeUnit.MILLISECONDS);
+                }
+            } catch (InterruptedException e) {
+                Thread.currentThread().interrupt();
+                processToStop.destroyForcibly();
+            } catch (Exception e) {
+                processToStop.destroyForcibly();
+            }
+        }
+
+        closeSolutionStreamsAfterTimeout();
+    }
+
+    /**
+     * Closes solution pipes after timeout termination.
+     *
+     * <p>Some failed process-tree terminations can leave descendant processes
+     * holding the stdout pipe open. Closing the Java-side reader prevents
+     * {@link #readLine()} from blocking forever after the timeout has already
+     * been recorded.
+     */
+    private void closeSolutionStreamsAfterTimeout() {
+        for (BufferedWriter out : solInputWriters) {
+            try {
+                out.close();
+            } catch (Exception e) {
+            }
+        }
+        if (solOutputReader != null) {
+            try {
+                solOutputReader.close();
+            } catch (Exception e) {
+            }
+        }
+        if (solOutputWriter != null) {
+            try {
+                solOutputWriter.close();
+            } catch (Exception e) {
+            }
+        }
+        if (solErrorReader != null) {
+            solErrorReader.closeAndWait(errorReaderDrainTimeoutMillis);
+            solutionError = solErrorReader.getOutput();
+        } else if (solErrorWriter != null) {
+            try {
+                solErrorWriter.close();
+            } catch (Exception e) {
+            }
+        }
+    }
+
     protected final void writeLine(int v) throws Exception {
         writeLine(String.valueOf(v));
     }

ecs-runner/scripts/mm-net-isolate.c

@@ -5,6 +5,7 @@
 #include <fcntl.h>
 #include <grp.h>
 #include <linux/audit.h>
+#include <linux/capability.h>
 #include <linux/filter.h>
 #include <linux/landlock.h>
 #include <linux/seccomp.h>
@@ -104,6 +105,10 @@
 #define __NR_landlock_restrict_self 446
 #endif
 
+#ifndef CAP_KILL
+#define CAP_KILL 5
+#endif
+
 #if MM_SUPERVISE_CHILD
 static volatile sig_atomic_t supervised_child_pid = -1;
 #endif
@@ -659,14 +664,31 @@ static int enter_isolated_execution(void) {
  * because its real UID is still the runner UID.
  */
 #if MM_SUPERVISE_CHILD
+static int signal_child_process_group(pid_t child_pid, int signo) {
+    if (kill(-child_pid, signo) == 0 || errno == ESRCH) {
+        return 0;
+    }
+
+    if (kill(child_pid, signo) == 0 || errno == ESRCH) {
+        return 0;
+    }
+
+    return errno;
+}
+
 static void forward_signal_to_child(int signo) {
     pid_t child_pid = (pid_t) supervised_child_pid;
     if (child_pid > 0) {
-        if (kill(-child_pid, signo) != 0) {
-            if (kill(child_pid, signo) != 0 && errno == EPERM) {
-                _exit(128 + signo);
+        int signal_error = signal_child_process_group(child_pid, signo);
+        if (signo == SIGTERM) {
+            int kill_error = signal_child_process_group(child_pid, SIGKILL);
+            if (signal_error == 0) {
+                signal_error = kill_error;
             }
         }
+        if (signal_error == EPERM) {
+            _exit(128 + signo);
+        }
     }
 }
 
@@ -702,18 +724,26 @@ static int wait_status_to_exit_code(int status) {
 
 /**
  * Drops the supervising wrapper back to the real user that invoked the setuid
- * scorer helper. The already-forked child keeps the temporary root privilege it
- * needs to switch to the configured isolated UID before exec.
+ * scorer helper while retaining only CAP_KILL. The already-forked child keeps
+ * the temporary root privilege it needs to switch to the configured isolated
+ * UID before exec.
  */
 static int drop_supervisor_to_invoker(void) {
 #if MM_DROP_SUPERVISOR_PRIVS
     uid_t uid = getuid();
     gid_t gid = getgid();
+    struct __user_cap_header_struct cap_header;
+    struct __user_cap_data_struct cap_data[_LINUX_CAPABILITY_U32S_3];
 
     if (uid == 0 && gid == 0) {
         return 0;
     }
 
+    if (prctl(PR_SET_KEEPCAPS, 1, 0, 0, 0) != 0) {
+        perror("supervisor prctl(PR_SET_KEEPCAPS)");
+        return 1;
+    }
+
     if (setgroups(0, NULL) != 0) {
         perror("supervisor setgroups");
         return 1;
@@ -728,6 +758,22 @@ static int drop_supervisor_to_invoker(void) {
         perror("supervisor setuid");
         return 1;
     }
+
+    memset(&cap_header, 0, sizeof(cap_header));
+    memset(&cap_data, 0, sizeof(cap_data));
+    cap_header.version = _LINUX_CAPABILITY_VERSION_3;
+    cap_header.pid = 0;
+    cap_data[CAP_KILL / 32].effective = 1U << (CAP_KILL % 32);
+    cap_data[CAP_KILL / 32].permitted = 1U << (CAP_KILL % 32);
+    if (syscall(SYS_capset, &cap_header, &cap_data) != 0) {
+        perror("supervisor capset(CAP_KILL)");
+        return 1;
+    }
+
+    if (prctl(PR_SET_KEEPCAPS, 0, 0, 0, 0) != 0) {
+        perror("supervisor prctl(PR_SET_KEEPCAPS clear)");
+        return 1;
+    }
 #endif
     return 0;
 }

scripts/fixtures/Blocks/marathon-match-test.json

@@ -468,7 +468,7 @@
         "fileName": "submission-issue-45-ignore-sigterm.zip",
         "language": "cpp",
         "issue": 45,
-        "validation": "Regression submission ignores SIGTERM and spins so per-test timeout must use destroyForcibly/SIGKILL."
+        "validation": "Regression submission ignores SIGTERM and spins so per-test timeout must still terminate the scorer process group."
       }
     },
     {

src/api/marathon-match-config/marathon-match-config.service.spec.ts

@@ -1,6 +1,7 @@
 import {
   BadRequestException,
   ConflictException,
+  HttpException,
   NotFoundException,
 } from '@nestjs/common';
 import {
@@ -528,6 +529,93 @@ describe('MarathonMatchConfigService', () => {
     );
   });
 
+  it('preserves Review API validation upload authorization failures', async () => {
+    const {
+      service,
+      ecsService,
+      httpService,
+      m2mService,
+      prisma,
+      prismaErrorService,
+    } = createService();
+
+    prisma.marathonMatchConfig.findUnique.mockResolvedValue({
+      id: 'config-1',
+      challengeId: '30000123',
+      active: true,
+      submissionApiUrl: 'https://submissions.example.com/v6',
+      testerId: 'tester-1',
+      taskDefinitionName: 'mm-runner',
+      taskDefinitionVersion: '7',
+      tester: {
+        compilationStatus: CompilationStatus.SUCCESS,
+      },
+      phaseConfigs: [
+        {
+          id: 'phase-provisional',
+          configType: PhaseConfigType.PROVISIONAL,
+          phaseId: 'provisional-phase',
+          startSeed: BigInt(100),
+          numberOfTests: 50,
+        },
+      ],
+    });
+    m2mService.getM2MToken.mockResolvedValue('m2m-token');
+    httpService.post.mockReturnValue(
+      throwError(() => ({
+        isAxiosError: true,
+        message: 'Request failed with status code 403',
+        response: {
+          status: 403,
+          data: {
+            message: 'Insufficient permissions',
+            code: 'FORBIDDEN',
+          },
+        },
+      })),
+    );
+
+    let thrown: unknown;
+    try {
+      await service.uploadTestSubmission(
+        '30000123',
+        {
+          configType: PhaseConfigType.PROVISIONAL,
+        },
+        {
+          buffer: Buffer.from('zip'),
+          mimetype: 'application/zip',
+          originalname: 'solution.zip',
+          size: 3,
+        } as Express.Multer.File,
+        {
+          isMachine: false,
+          userId: '40051399',
+        } as never,
+      );
+    } catch (error) {
+      thrown = error;
+    }
+
+    expect(thrown).toBeInstanceOf(HttpException);
+    const exception = thrown as HttpException;
+    expect(exception.getStatus()).toBe(403);
+    expect(exception.getResponse()).toEqual({
+      message:
+        'Review API rejected the validation submission upload with 403 Forbidden. Confirm the Marathon Match M2M credentials are authorized for create:submission in Review API. Upstream message: Insufficient permissions',
+      code: 'VALIDATION_SUBMISSION_UPLOAD_REJECTED',
+      details: {
+        challengeId: '30000123',
+        memberId: '40051399',
+        upstreamCode: 'FORBIDDEN',
+        upstreamMessage: 'Insufficient permissions',
+        upstreamStatusCode: 403,
+      },
+    });
+    expect(prismaErrorService.handleError).not.toHaveBeenCalled();
+    expect(ecsService.launchScorerTask).not.toHaveBeenCalled();
+  });
+
   it('rejects validation submissions when the tester is not compiled', async () => {
     const { service, ecsService, httpService, prisma } = createService();