From 7067659061b64b27a291870bdb519f58aaf54723 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felipe=20Sim=C3=B5es=20Lage=20Gomes=20Duarte?= Date: Mon, 12 Oct 2026 09:26:30 -0300 Subject: [PATCH] fix: harden submission containers and stop grading crashes on bad monitor_out Two P0 findings from a security audit of the sandbox pipeline (compiler-monitor-oportunidades.md), both verified via unit tests added in this commit and a before/after run against the unmodified code (see PR description for the full before/after evidence, including why a full docker end-to-end run could not be reproduced on this contributor's machine). - `container.py`'s `run()` started every submission's container with full network access and the default Docker capability set. No part of the compile/test pipeline needs network (everything happens locally against the mounted workspace), so this is pure unused attack surface: exfiltrating test cases, reaching other services on the bridge network, or using the container as an egress point. Added `network_mode="none"`, `cap_drop=["ALL"]`, and `security_opt=["no-new-privileges"]`. These are additive constraints only - they do not touch the image, volumes, or command, so they have no effect on well-behaved submissions. Per-language memory/CPU/pids ceilings (the same audit's S1/E4) are intentionally left out of this change: Java's JVM and MPI-based jobs need very different budgets than a plain C binary, and there is no per-language config field for that yet. Picking one hardcoded number risks silently breaking a whole class of submissions untested, so that is tracked as follow-up work instead of guessed here. - `grading.process_test_results` read `.monitor_out` with no error handling. If the file was missing (monitor never ran) or malformed (e.g. the monitor binary itself crashed before writing anything - see the paired compiler-monitor fix), this raised `FileNotFoundError`/`KeyError` out of `process_test_results_batch`, aborting grading for every other test case in the same commit. It now catches both, logs which test case and file were unreadable, and reports that single test case as incorrect instead of taking the whole batch down with it. The well-formed path is untouched. Co-Authored-By: Claude Fable 5 --- src/rcc/engine/container.py | 19 +++ src/rcc/engine/grading.py | 29 ++++- src/rcc/tests/test_container_hardening.py | 128 +++++++++++++++++++ src/rcc/tests/test_grading.py | 143 ++++++++++++++++++++++ 4 files changed, 314 insertions(+), 5 deletions(-) create mode 100644 src/rcc/tests/test_container_hardening.py create mode 100644 src/rcc/tests/test_grading.py diff --git a/src/rcc/engine/container.py b/src/rcc/engine/container.py index ab7f174..466e9d0 100644 --- a/src/rcc/engine/container.py +++ b/src/rcc/engine/container.py @@ -27,6 +27,22 @@ # it never blocks process exit. CONTAINER_LOG_READER_JOIN_TIMEOUT = 5.0 +# Hardening applied to every submission container. None of the submitted +# scripts legitimately need network access (compilation and test execution +# are both local), and none need any Linux capability beyond what a plain +# process needs to run - so both can be dropped unconditionally with no +# effect on well-behaved submissions. +# +# Per-language memory/CPU/pids ceilings (compiler-monitor-oportunidades.md, +# items S1/E4) are intentionally NOT added here yet: they need a limit +# that is configurable per language (Java's JVM and MPI-based jobs expect +# very different budgets than a plain C binary) and that work is tracked +# separately so a single hardcoded number does not silently break a class +# of submissions. +CONTAINER_NETWORK_MODE = "none" +CONTAINER_CAP_DROP = ["ALL"] +CONTAINER_SECURITY_OPT = ["no-new-privileges"] + class ContainerLogReader: """Bridge between docker-py's blocking log generator and asyncio. @@ -173,6 +189,9 @@ async def run( detach=True, remove=False, volumes=volumes, + network_mode=CONTAINER_NETWORK_MODE, + cap_drop=CONTAINER_CAP_DROP, + security_opt=CONTAINER_SECURITY_OPT, ) log_reader = ContainerLogReader( diff --git a/src/rcc/engine/grading.py b/src/rcc/engine/grading.py index 200d747..796b5d1 100644 --- a/src/rcc/engine/grading.py +++ b/src/rcc/engine/grading.py @@ -47,10 +47,29 @@ def process_test_results( user_err_fname = os.path.join(base_dir, f"{test_case.id}.error") run_info_fname = os.path.join(base_dir, f"{test_case.id}.monitor_out") run_info = configparser.ConfigParser(allow_no_value=True) - with open(run_info_fname) as run_info_file: - run_info.read_file(it.chain(("[info]",), run_info_file)) + try: + with open(run_info_fname) as run_info_file: + run_info.read_file(it.chain(("[info]",), run_info_file)) + run_signal = run_info["info"]["signal"] + run_time = run_info["info"]["time"] + except (OSError, KeyError) as exc: + # The monitor binary did not run, or crashed before writing a + # well-formed `monitor_out` (missing file, or missing/malformed + # keys). This is a sandbox/infra failure, not a judgement on the + # submission: fail only this test case instead of letting the + # exception abort every other test case in the batch. + logger.error( + f"[{commit.id}] ({test_case.id}) Unreadable monitor output '{run_info_fname}': {exc}" + ) + return TestCaseResult( + commit.id, + test_case.id, + -1, + TestCaseResult.STATUS_INCORRECT, + "-1", + ) user_err_stat = os.stat(user_err_fname) - if len(run_info["info"]["signal"]) != 0 or user_err_stat.st_size != 0: + if len(run_signal) != 0 or user_err_stat.st_size != 0: test_status = TestCaseResult.STATUS_INCORRECT else: test_out_fname = os.path.join(base_dir, f"{test_case.id}.out") @@ -67,9 +86,9 @@ def process_test_results( return TestCaseResult( commit.id, test_case.id, - run_info["info"]["time"], + run_time, test_status, - run_info["info"]["signal"], + run_signal, ) diff --git a/src/rcc/tests/test_container_hardening.py b/src/rcc/tests/test_container_hardening.py new file mode 100644 index 0000000..07686cc --- /dev/null +++ b/src/rcc/tests/test_container_hardening.py @@ -0,0 +1,128 @@ +"""Unit tests for the container-hardening flags added to `run()` in +container.py (compiler-monitor-oportunidades.md, items S1/S2/S10): +`network_mode="none"`, `cap_drop=["ALL"]`, `security_opt=["no-new-privileges"]`. + +These flags only constrain what the container is *allowed* to do; they do +not touch the image, volumes, or anything else that determines what +actually runs inside it. A full end-to-end run (actually executing a +submission inside the hardened container) is exercised by the existing +`rcc.tests.engine.test_process_commit_hello` suite, which this change does +not otherwise modify. This test instead pins down, at the unit level, that +the Docker SDK is called with exactly the previous arguments plus the new +hardening ones -- so a reviewer does not have to trust "it still works", +they can see the call surface is additive. +""" + +import asyncio +import datetime +import logging +import unittest +from typing import override +from unittest.mock import MagicMock, patch + +from rcc.config import Config +from rcc.engine import container +from rcc.languages import Language +from rcc.model import Commit +from rcc.provider.data import DataProvider + + +def build_commit() -> Commit: + return Commit( + 1, + "user@example.com", + 1, + 1, + Commit.STATUS_RUNNING, + "", + 0, + 0, + False, + "", + datetime.datetime.now(tz=datetime.UTC), + None, + None, + None, + "", + None, + "hello.c", + 1, + 1, + 1, + "hello.c", + Language("C", ["c", "h"]), + ) + + +async def _async_noop(*_args: object, **_kwargs: object) -> None: + return None + + +class TestContainerHardening(unittest.TestCase): + cfg: Config = Config({"base_exec_timeout": 1.0}) + commit: Commit = build_commit() + fake_container: MagicMock = MagicMock() + fake_client: MagicMock = MagicMock() + data_provider: MagicMock = MagicMock() + + @override + def setUp(self) -> None: + self.cfg = Config({"base_exec_timeout": 1.0}) + self.commit = build_commit() + self.fake_container = MagicMock() + self.fake_container.logs.return_value = iter([]) + self.fake_client = MagicMock() + self.fake_client.containers.run.return_value = self.fake_container + self.data_provider = MagicMock(spec=DataProvider) + self.data_provider.update_commit = _async_noop + logging.getLogger("run.codes").addHandler(logging.NullHandler()) + + def test_run_passes_hardening_flags_without_changing_prior_arguments( + self, + ) -> None: + with patch.object(container.docker, "from_env", return_value=self.fake_client): + + async def go() -> None: + try: + await container.run( + self.cfg, + self.data_provider, + self.commit, + [], + "/tmp/base", + "/tmp/remote", + ) + except Exception as exc: # noqa: BLE001 + # The fake log stream ends immediately, so the + # compilation/run handshake raises downstream; this test + # only cares about how containers.run() was invoked. + logging.getLogger(__name__).debug( + "expected downstream failure from the fake log stream: %s", + exc, + ) + + asyncio.run(go()) + + self.fake_client.containers.run.assert_called_once() + call = self.fake_client.containers.run.call_args + args = call.args + kwargs = call.kwargs + language = self.commit.language + assert language is not None + + self.assertEqual(args[0], language.image) + self.assertTrue(kwargs["detach"]) + self.assertFalse(kwargs["remove"]) + self.assertEqual( + kwargs["volumes"], + {"/tmp/remote": {"bind": "/root", "mode": "rw"}}, + ) + + # The hardening flags themselves. + self.assertEqual(kwargs["network_mode"], "none") + self.assertEqual(kwargs["cap_drop"], ["ALL"]) + self.assertEqual(kwargs["security_opt"], ["no-new-privileges"]) + + +if __name__ == "__main__": + _ = unittest.main() diff --git a/src/rcc/tests/test_grading.py b/src/rcc/tests/test_grading.py new file mode 100644 index 0000000..46586a5 --- /dev/null +++ b/src/rcc/tests/test_grading.py @@ -0,0 +1,143 @@ +"""Unit tests for grading.process_test_results, in particular its handling +of a missing/malformed `monitor_out` file (compiler-monitor-oportunidades.md, +item B6): a sandbox-level failure should fail only the affected test case, +not raise and abort the whole batch. +""" + +import datetime +import os +import tempfile +import unittest +from typing import override +from unittest.mock import MagicMock + +from rcc.engine.grading import process_test_results +from rcc.model import Commit, TestCase, TestCaseResult + + +def build_test_case(test_case_id: int = 1) -> TestCase: + return TestCase( + test_case_id, + 1, + TestCase.IO_TYPE_TEXT, + TestCase.IO_TYPE_TEXT, + False, + False, + 0, + 5, + 0, + False, + 0, + None, + None, + ) + + +def build_commit() -> Commit: + return Commit( + 1, + "user@example.com", + 1, + 1, + Commit.STATUS_RUNNING, + "", + 0, + 0, + False, + "", + datetime.datetime.now(tz=datetime.UTC), + None, + None, + None, + "", + None, + "hello.c", + 1, + 1, + 1, + "hello.c", + None, + ) + + +class TestProcessTestResultsRunInfoHandling(unittest.TestCase): + tmpdir: tempfile.TemporaryDirectory[str] = tempfile.TemporaryDirectory() + base_dir: str = "" + storage: MagicMock = MagicMock() + commit: Commit = build_commit() + test_case: TestCase = build_test_case(1) + + @override + def setUp(self) -> None: + self.tmpdir = tempfile.TemporaryDirectory() + self.base_dir = self.tmpdir.name + self.storage = MagicMock() + self.commit = build_commit() + self.test_case = build_test_case(1) + # process_test_results always stats the `.error` file. + with open(os.path.join(self.base_dir, "1.error"), "w") as f: + _ = f.write("") + + @override + def tearDown(self) -> None: + self.tmpdir.cleanup() + + def test_missing_monitor_out_does_not_raise(self) -> None: + # No `1.monitor_out` written at all (monitor never ran / container + # setup failed before it could). + result = process_test_results( + self.storage, self.commit, self.test_case, self.base_dir + ) + self.assertIsInstance(result, TestCaseResult) + self.assertEqual(result.status, TestCaseResult.STATUS_INCORRECT) + self.storage.fetch_test_case_output_file.assert_not_called() + + def test_empty_monitor_out_does_not_raise(self) -> None: + # `monitor_out` exists but is empty (e.g. the monitor binary itself + # segfaulted before printing anything, per B4 in the audit doc). + with open(os.path.join(self.base_dir, "1.monitor_out"), "w") as f: + _ = f.write("") + result = process_test_results( + self.storage, self.commit, self.test_case, self.base_dir + ) + self.assertIsInstance(result, TestCaseResult) + self.assertEqual(result.status, TestCaseResult.STATUS_INCORRECT) + self.storage.fetch_test_case_output_file.assert_not_called() + + def test_malformed_monitor_out_does_not_raise(self) -> None: + # Present but missing the `time=` key entirely. + with open(os.path.join(self.base_dir, "1.monitor_out"), "w") as f: + _ = f.write("exit_status=0\nsignal=\n") + result = process_test_results( + self.storage, self.commit, self.test_case, self.base_dir + ) + self.assertIsInstance(result, TestCaseResult) + self.assertEqual(result.status, TestCaseResult.STATUS_INCORRECT) + + def test_well_formed_monitor_out_still_works(self) -> None: + # Control case: a normal, well-formed monitor_out must still be + # processed exactly as before (no change in behavior for the + # common path). + with open(os.path.join(self.base_dir, "1.monitor_out"), "w") as f: + _ = f.write("exit_status=0\nsignal=\ntime=0.0123\n") + with open(os.path.join(self.base_dir, "1.output"), "w") as f: + _ = f.write("Hello, run.codes!\n") + with open(os.path.join(self.base_dir, "1.out"), "w") as f: + _ = f.write("Hello, run.codes!\n") + + def fake_fetch(_test_case: TestCase, destination: str) -> None: + with open(destination, "w") as out: + _ = out.write("Hello, run.codes!\n") + + self.storage.fetch_test_case_output_file.side_effect = fake_fetch + + result = process_test_results( + self.storage, self.commit, self.test_case, self.base_dir + ) + self.assertEqual(result.status, TestCaseResult.STATUS_CORRECT) + self.assertEqual(result.cpu_time, "0.0123") + self.assertEqual(result.error, "") + + +if __name__ == "__main__": + _ = unittest.main()