From ed81df456f40df253590ab41b5ddf05f677c16ad Mon Sep 17 00:00:00 2001 From: Chris Lovering Date: Mon, 9 Oct 2023 22:17:05 +0100 Subject: [PATCH 01/16] Allow specifying a binary path when calling NsJail.python3 --- snekbox/nsjail.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/snekbox/nsjail.py b/snekbox/nsjail.py index 0d5e3c61..55c620ac 100644 --- a/snekbox/nsjail.py +++ b/snekbox/nsjail.py @@ -168,7 +168,7 @@ def _consume_stdout(self, nsjail: subprocess.Popen) -> str: return "".join(output) def _build_args( - self, py_args: Iterable[str], nsjail_args: Iterable[str], log_path: str, fs_home: str + self, py_args: Iterable[str], nsjail_args: Iterable[str], log_path: str, fs_home: str, binary_path: str ) -> Sequence[str]: if self.cgroup_version == 2: nsjail_args = ("--use_cgroupv2", *nsjail_args) @@ -197,10 +197,7 @@ def _build_args( log_path, *nsjail_args, "--", - self.config.exec_bin.path, - # Filter out empty strings at start of Python args - # (causes issues with python cli) - *iter_lstrip(self.config.exec_bin.arg), + binary_path, *iter_lstrip(py_args), ] @@ -259,6 +256,7 @@ def python3( py_args: Iterable[str], files: Iterable[FileAttachment] = (), nsjail_args: Iterable[str] = (), + binary_path: Path = "/lang/python/default/bin/python", ) -> EvalResult: """ Execute Python 3 code in an isolated environment and return the completed process. @@ -267,13 +265,14 @@ def python3( py_args: Arguments to pass to Python. files: FileAttachments to write to the sandbox prior to running Python. nsjail_args: Overrides for the NsJail configuration. + binary_path: The path to the binary to execute under. """ with NamedTemporaryFile() as nsj_log, MemFS( instance_size=self.memfs_instance_size, home=self.memfs_home, output=self.memfs_output, ) as fs: - args = self._build_args(py_args, nsjail_args, nsj_log.name, str(fs.home)) + args = self._build_args(py_args, nsjail_args, nsj_log.name, str(fs.home), binary_path) try: files_written = self._write_files(fs.home, files) From 6cc4dbaf1c9df0cae57850184469cbfb0040d819 Mon Sep 17 00:00:00 2001 From: Chris Lovering Date: Mon, 9 Oct 2023 22:17:35 +0100 Subject: [PATCH 02/16] Add tests for multi-version support --- tests/test_integration.py | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/tests/test_integration.py b/tests/test_integration.py index 91b01e68..4ea9b65f 100644 --- a/tests/test_integration.py +++ b/tests/test_integration.py @@ -52,6 +52,35 @@ def test_memory_limit_separate_per_process(self): self.assertTrue(all(status == 200 for status in statuses)) self.assertTrue(all(json.loads(response)["returncode"] == 0 for response in responses)) + def test_multi_binary_support(self): + """Test eval requests with different binary paths set.""" + with run_gunicorn(): + get_python_version_body = { + "input": "import sys; print('.'.join(map(str, sys.version_info[:2])))" + } + cases = [ + ( + get_python_version_body, + "3.12\n", + "test default binary is used when binary_path not specified", + ), + ( + get_python_version_body | {"binary_path": "/lang/python/3.12/bin/python"}, + "3.12\n", + "test default binary is used when explicitly set", + ), + ( + get_python_version_body | {"binary_path": "/lang/python/3.13/bin/python"}, + "3.13\n", + "test alternative binary is used when set", + ), + ] + for body, expected, msg in cases: + with self.subTest(msg=msg, body=body, expected=expected): + response, status = snekbox_request(body) + self.assertEqual(status, 200) + self.assertEqual(json.loads(response)["stdout"], expected) + def test_eval(self): """Test normal eval requests without files.""" with run_gunicorn(): From c24785d488e3fefeff50bc79b0fd262d09c3043a Mon Sep 17 00:00:00 2001 From: Chris Lovering Date: Mon, 9 Oct 2023 22:17:54 +0100 Subject: [PATCH 03/16] Allow setting a binary_path when calling /eval --- config/snekbox.cfg | 5 ----- snekbox/api/resources/eval.py | 13 +++++++++++++ tests/test_nsjail.py | 4 +++- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/config/snekbox.cfg b/config/snekbox.cfg index b4055219..33cd9e95 100644 --- a/config/snekbox.cfg +++ b/config/snekbox.cfg @@ -103,8 +103,3 @@ cgroup_pids_max: 6 cgroup_pids_mount: "/sys/fs/cgroup/pids" iface_no_lo: true - -exec_bin { - path: "/lang/python/default/bin/python" - arg: "" -} diff --git a/snekbox/api/resources/eval.py b/snekbox/api/resources/eval.py index 55bba984..98babc94 100644 --- a/snekbox/api/resources/eval.py +++ b/snekbox/api/resources/eval.py @@ -1,6 +1,7 @@ from __future__ import annotations import logging +from pathlib import Path import falcon from falcon.media.validators.jsonschema import validate @@ -43,6 +44,7 @@ class EvalResource: "required": ["path"], }, }, + "binary_path": {"type": "string"}, }, "anyOf": [ {"required": ["input"]}, @@ -122,10 +124,21 @@ def on_post(self, req: falcon.Request, resp: falcon.Response) -> None: if "input" in body: body.setdefault("args", ["-c"]) body["args"].append(body["input"]) + + binary_path = body.get("binary_path") + if binary_path: + binary_path = Path(binary_path) + if ( + not binary_path.resolve().as_posix().startswith("/lang/") + or not binary_path.is_file() + ): + raise falcon.HTTPBadRequest(title="binary_path file is invalid") + try: result = self.nsjail.python3( py_args=body["args"], files=[FileAttachment.from_dict(file) for file in body.get("files", [])], + binary_path=binary_path, ) except ParsingError as e: raise falcon.HTTPBadRequest(title="Request file is invalid", description=str(e)) diff --git a/tests/test_nsjail.py b/tests/test_nsjail.py index dde20bc8..2d21ad3f 100644 --- a/tests/test_nsjail.py +++ b/tests/test_nsjail.py @@ -26,6 +26,8 @@ def setUp(self): # Hard-coded because it's non-trivial to parse the mount options. self.shm_mount_size = 40 * Size.MiB + self.default_binary_path = "/lang/python/default/bin/python" + def eval_code(self, code: str): return self.nsjail.python3(["-c", code]) @@ -547,7 +549,7 @@ def test_py_args(self): for args, expected in cases: with self.subTest(args=args): result = self.nsjail.python3(py_args=args) - idx = result.args.index(self.nsjail.config.exec_bin.path) + idx = result.args.index(self.default_binary_path) self.assertEqual(result.args[idx + 1 :], expected) self.assertEqual(result.returncode, 0) From 0c9b234011f21ce7bdc993957867610dfec1258f Mon Sep 17 00:00:00 2001 From: Chris Lovering Date: Mon, 9 Oct 2023 22:18:47 +0100 Subject: [PATCH 04/16] Also split on hyphens in build python script This is needed as dev builds such as 3.13-dev use the suffix -dev, rather than a patch version. --- scripts/build_python.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/build_python.sh b/scripts/build_python.sh index da937c25..1e6d423d 100755 --- a/scripts/build_python.sh +++ b/scripts/build_python.sh @@ -7,8 +7,8 @@ py_version="${1}" # Install Python interpreter under e.g. /lang/python/3.11/ (no patch version). "${PYENV_ROOT}/plugins/python-build/bin/python-build" \ "${py_version}" \ - "/lang/python/${py_version%.*}" -"/lang/python/${py_version%.*}/bin/python" -m pip install -U pip + "/lang/python/${py_version%[-.]*}" +"/lang/python/${py_version%[-.]*}/bin/python" -m pip install -U pip # Clean up some unnecessary files to reduce image size bloat. find /lang/python/ -depth \ From d9b8ab36df6b788b4e925d1dd49acd89a0e9bf7c Mon Sep 17 00:00:00 2001 From: Chris Lovering Date: Tue, 10 Oct 2023 15:25:35 +0100 Subject: [PATCH 05/16] Add additional tests to ensure invalid binary paths are not ran --- tests/test_integration.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/tests/test_integration.py b/tests/test_integration.py index 4ea9b65f..ce01bb89 100644 --- a/tests/test_integration.py +++ b/tests/test_integration.py @@ -81,6 +81,25 @@ def test_multi_binary_support(self): self.assertEqual(status, 200) self.assertEqual(json.loads(response)["stdout"], expected) + def invalid_binary_paths(self): + """Test that passing invalid binary paths result in no code execution.""" + with run_gunicorn(): + cases = [ + ("/bin/bash", "test files outside of /lang cannot be ran"), + ( + "/lang/../bin/bash", + "test path traversal still stops files outside /lang from running", + ), + ("/foo/bar", "test non-existant files are not ran"), + ] + for path, msg in cases: + with self.subTest(msg=msg, path=path): + body = {"args": ["-c", "echo", "hi"], "binary_path": path} + response, status = snekbox_request(body) + self.assertEqual(status, 400) + expected = {"title": "binary_path file is invalid"} + self.assertEqual(json.loads(response)["stdout"], expected) + def test_eval(self): """Test normal eval requests without files.""" with run_gunicorn(): From f957648d8facd2f731e327c5cdced6532dfd5953 Mon Sep 17 00:00:00 2001 From: Chris Lovering Date: Thu, 3 Oct 2024 19:35:42 +0100 Subject: [PATCH 06/16] Drop restriction of only running binaries from /lang --- snekbox/api/resources/eval.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/snekbox/api/resources/eval.py b/snekbox/api/resources/eval.py index 98babc94..d083b191 100644 --- a/snekbox/api/resources/eval.py +++ b/snekbox/api/resources/eval.py @@ -128,11 +128,8 @@ def on_post(self, req: falcon.Request, resp: falcon.Response) -> None: binary_path = body.get("binary_path") if binary_path: binary_path = Path(binary_path) - if ( - not binary_path.resolve().as_posix().startswith("/lang/") - or not binary_path.is_file() - ): - raise falcon.HTTPBadRequest(title="binary_path file is invalid") + if not binary_path.is_file(): + raise falcon.HTTPBadRequest(title="binary_path is not a file") try: result = self.nsjail.python3( From d936970e5f6460e4dc28a17b2a332211f5a6d850 Mon Sep 17 00:00:00 2001 From: Chris Lovering Date: Thu, 3 Oct 2024 19:36:58 +0100 Subject: [PATCH 07/16] Add specific error for when the specified binary path does not exist --- snekbox/api/resources/eval.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/snekbox/api/resources/eval.py b/snekbox/api/resources/eval.py index d083b191..88bbf965 100644 --- a/snekbox/api/resources/eval.py +++ b/snekbox/api/resources/eval.py @@ -128,6 +128,8 @@ def on_post(self, req: falcon.Request, resp: falcon.Response) -> None: binary_path = body.get("binary_path") if binary_path: binary_path = Path(binary_path) + if not binary_path.exists(): + raise falcon.HTTPBadRequest(title="binary_path does not exist") if not binary_path.is_file(): raise falcon.HTTPBadRequest(title="binary_path is not a file") From 81573b2d0144ece5490948706918233b887f86c1 Mon Sep 17 00:00:00 2001 From: Chris Lovering Date: Thu, 3 Oct 2024 19:45:43 +0100 Subject: [PATCH 08/16] Also check if the specified binary path is execuatable --- snekbox/api/resources/eval.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/snekbox/api/resources/eval.py b/snekbox/api/resources/eval.py index 88bbf965..943ecff7 100644 --- a/snekbox/api/resources/eval.py +++ b/snekbox/api/resources/eval.py @@ -132,6 +132,8 @@ def on_post(self, req: falcon.Request, resp: falcon.Response) -> None: raise falcon.HTTPBadRequest(title="binary_path does not exist") if not binary_path.is_file(): raise falcon.HTTPBadRequest(title="binary_path is not a file") + if not binary_path.stat().st_mode & 0o100 == 0o100: + raise falcon.HTTPBadRequest(title="binary_path is not executable") try: result = self.nsjail.python3( From 66c6d27335a5d1fc23bd0889d299cb79f3418219 Mon Sep 17 00:00:00 2001 From: Chris Lovering Date: Thu, 3 Oct 2024 19:46:50 +0100 Subject: [PATCH 09/16] Use a shared default const This was needed due to wanting a default value when calling python3 diurectly, but also when not specified via the API call --- snekbox/api/resources/eval.py | 7 +++++-- snekbox/nsjail.py | 12 +++++++++--- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/snekbox/api/resources/eval.py b/snekbox/api/resources/eval.py index 943ecff7..3172f609 100644 --- a/snekbox/api/resources/eval.py +++ b/snekbox/api/resources/eval.py @@ -6,7 +6,7 @@ import falcon from falcon.media.validators.jsonschema import validate -from snekbox.nsjail import NsJail +from snekbox.nsjail import DEFAULT_BINARY_PATH, NsJail from snekbox.snekio import FileAttachment, ParsingError __all__ = ("EvalResource",) @@ -126,7 +126,9 @@ def on_post(self, req: falcon.Request, resp: falcon.Response) -> None: body["args"].append(body["input"]) binary_path = body.get("binary_path") - if binary_path: + if not binary_path: + binary_path = DEFAULT_BINARY_PATH + else: binary_path = Path(binary_path) if not binary_path.exists(): raise falcon.HTTPBadRequest(title="binary_path does not exist") @@ -134,6 +136,7 @@ def on_post(self, req: falcon.Request, resp: falcon.Response) -> None: raise falcon.HTTPBadRequest(title="binary_path is not a file") if not binary_path.stat().st_mode & 0o100 == 0o100: raise falcon.HTTPBadRequest(title="binary_path is not executable") + binary_path = binary_path.resolve().as_posix() try: result = self.nsjail.python3( diff --git a/snekbox/nsjail.py b/snekbox/nsjail.py index 55c620ac..fe95f802 100644 --- a/snekbox/nsjail.py +++ b/snekbox/nsjail.py @@ -26,6 +26,7 @@ LOG_PATTERN = re.compile( r"\[(?P(I)|[DWEF])\]\[.+?\](?(2)|(?P\[\d+\] .+?:\d+ )) ?(?P.+)" ) +DEFAULT_BINARY_PATH = "/snekbin/python/default/bin/python" class NsJail: @@ -168,7 +169,12 @@ def _consume_stdout(self, nsjail: subprocess.Popen) -> str: return "".join(output) def _build_args( - self, py_args: Iterable[str], nsjail_args: Iterable[str], log_path: str, fs_home: str, binary_path: str + self, + py_args: Iterable[str], + nsjail_args: Iterable[str], + log_path: str, + fs_home: str, + binary_path: str, ) -> Sequence[str]: if self.cgroup_version == 2: nsjail_args = ("--use_cgroupv2", *nsjail_args) @@ -185,7 +191,7 @@ def _build_args( nsjail_args = ( # Mount `home` with Read/Write access "--bindmount", - f"{fs_home}:home", + f"{fs_home}:home", # noqa: E231 *nsjail_args, ) @@ -256,7 +262,7 @@ def python3( py_args: Iterable[str], files: Iterable[FileAttachment] = (), nsjail_args: Iterable[str] = (), - binary_path: Path = "/lang/python/default/bin/python", + binary_path: Path = DEFAULT_BINARY_PATH, ) -> EvalResult: """ Execute Python 3 code in an isolated environment and return the completed process. From 30d51d19ed6eb425cbfb85c5dcbebd5e77816732 Mon Sep 17 00:00:00 2001 From: Chris Lovering Date: Thu, 3 Oct 2024 19:47:53 +0100 Subject: [PATCH 10/16] Correct spelling in test cases --- tests/test_integration.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_integration.py b/tests/test_integration.py index ce01bb89..9ae2da80 100644 --- a/tests/test_integration.py +++ b/tests/test_integration.py @@ -85,12 +85,12 @@ def invalid_binary_paths(self): """Test that passing invalid binary paths result in no code execution.""" with run_gunicorn(): cases = [ - ("/bin/bash", "test files outside of /lang cannot be ran"), + ("/bin/bash", "test files outside of /lang cannot be run"), ( "/lang/../bin/bash", "test path traversal still stops files outside /lang from running", ), - ("/foo/bar", "test non-existant files are not ran"), + ("/foo/bar", "test non-existant files are not run"), ] for path, msg in cases: with self.subTest(msg=msg, path=path): From f726695b1a1d99b5c61ea09bd3a364103a08d7bf Mon Sep 17 00:00:00 2001 From: Chris Lovering Date: Thu, 3 Oct 2024 19:50:12 +0100 Subject: [PATCH 11/16] Update all references to /lang to /snekbin --- .flake8 | 2 +- .github/CONTRIBUTING.md | 4 ++-- Dockerfile | 8 ++++---- README.md | 2 +- config/snekbox.cfg | 4 ++-- scripts/build_python.sh | 8 ++++---- scripts/install_eval_deps.sh | 2 +- tests/test_integration.py | 10 +++++----- tests/test_nsjail.py | 4 ++-- 9 files changed, 22 insertions(+), 22 deletions(-) diff --git a/.flake8 b/.flake8 index 0f42e666..6742d3dc 100644 --- a/.flake8 +++ b/.flake8 @@ -18,4 +18,4 @@ ignore = # Type Annotations ANN002,ANN003,ANN101,ANN102,ANN204,ANN206 -per-file-ignores = tests/*:D1,ANN +per-file-ignores = tests/*:D1,ANN,E202,E231,E241,E272,E702 diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md index 1124b8ed..67ad71b4 100644 --- a/.github/CONTRIBUTING.md +++ b/.github/CONTRIBUTING.md @@ -64,11 +64,11 @@ Other things to look out for are breaking changes to NsJail's config format, its ## Adding and Updating Python Interpreters -Python interpreters are built using pyenv via the `scripts/build_python.sh` helper script. This script accepts a pyenv version specifier (`pyenv install --list`) and builds the interpreter in a version-specific directory under `/lang/python`. In the image, each minor version of a Python interpreter should have its own build stage and the resulting `/lang/python` directory can be copied from that stage into the `base` stage. +Python interpreters are built using pyenv via the `scripts/build_python.sh` helper script. This script accepts a pyenv version specifier (`pyenv install --list`) and builds the interpreter in a version-specific directory under `/snekbin/python`. In the image, each minor version of a Python interpreter should have its own build stage and the resulting `/snekbin/python` directory can be copied from that stage into the `base` stage. When updating a patch version (e.g. 3.11.3 to 3.11.4), edit the existing build stage in the image for the minor version (3.11); do not add a new build stage. To have access to a new version, pyenv likely needs to be updated. To do so, change the tag in the `git clone` command in the image, but only for the build stage that needs access to the new version. Updating pyenv for all build stages will just cause unnecessary build cache invalidations. -To change the default interpreter used by NsJail, update the target of the `/lang/python/default` symlink created in the `base` stage. +To change the default interpreter used by NsJail, update the target of the `/snekbin/python/default` symlink created in the `base` stage. [readme]: ../README.md [Dockerfile]: ../Dockerfile diff --git a/Dockerfile b/Dockerfile index ed198c4f..cb990269 100644 --- a/Dockerfile +++ b/Dockerfile @@ -54,11 +54,11 @@ RUN apt-get -y update \ && rm -rf /var/lib/apt/lists/* COPY --link --from=builder-nsjail /nsjail/nsjail /usr/sbin/ -COPY --link --from=builder-py-3_12 /lang/ /lang/ -COPY --link --from=builder-py-3_13 /lang/ /lang/ +COPY --link --from=builder-py-3_12 /snekbin/ /snekbin/ +COPY --link --from=builder-py-3_13 /snekbin/ /snekbin/ RUN chmod +x /usr/sbin/nsjail \ - && ln -s /lang/python/3.12/ /lang/python/default + && ln -s /snekbin/python/3.12/ /snekbin/python/default # ------------------------------------------------------------------------------ FROM base as venv @@ -79,7 +79,7 @@ RUN if [ -n "${DEV}" ]; \ then \ pip install -U -r requirements/coverage.pip \ && export PYTHONUSERBASE=/snekbox/user_base \ - && /lang/python/default/bin/python -m pip install --user numpy~=1.19; \ + && /snekbin/python/default/bin/python -m pip install --user numpy~=1.19; \ fi # At the end to avoid re-installing dependencies when only a config changes. diff --git a/README.md b/README.md index c4779a44..c92ebdf7 100644 --- a/README.md +++ b/README.md @@ -105,7 +105,7 @@ To expose third-party Python packages during evaluation, install them to a custo ```sh docker exec snekbox /bin/sh -c \ - 'PYTHONUSERBASE=/snekbox/user_base /lang/python/default/bin/python -m pip install --user numpy' + 'PYTHONUSERBASE=/snekbox/user_base /snekbin/python/default/bin/python -m pip install --user numpy' ``` In the above command, `snekbox` is the name of the running container. The name may be different and can be checked with `docker ps`. diff --git a/config/snekbox.cfg b/config/snekbox.cfg index 33cd9e95..778f90e8 100644 --- a/config/snekbox.cfg +++ b/config/snekbox.cfg @@ -81,8 +81,8 @@ mount { } mount { - src: "/lang" - dst: "/lang" + src: "/snekbin" + dst: "/snekbin" is_bind: true rw: false } diff --git a/scripts/build_python.sh b/scripts/build_python.sh index 1e6d423d..77f50ab6 100755 --- a/scripts/build_python.sh +++ b/scripts/build_python.sh @@ -4,14 +4,14 @@ shopt -s inherit_errexit py_version="${1}" -# Install Python interpreter under e.g. /lang/python/3.11/ (no patch version). +# Install Python interpreter under e.g. /snekbin/python/3.11/ (no patch version). "${PYENV_ROOT}/plugins/python-build/bin/python-build" \ "${py_version}" \ - "/lang/python/${py_version%[-.]*}" -"/lang/python/${py_version%[-.]*}/bin/python" -m pip install -U pip + "/snekbin/python/${py_version%[-.]*}" +"/snekbin/python/${py_version%[-.]*}/bin/python" -m pip install -U pip # Clean up some unnecessary files to reduce image size bloat. -find /lang/python/ -depth \ +find /snekbin/python/ -depth \ \( \ \( -type d -a \( \ -name test -o -name tests -o -name idle_test \ diff --git a/scripts/install_eval_deps.sh b/scripts/install_eval_deps.sh index 8fa53169..b57a6543 100644 --- a/scripts/install_eval_deps.sh +++ b/scripts/install_eval_deps.sh @@ -1,5 +1,5 @@ set -euo pipefail export PYTHONUSERBASE=/snekbox/user_base -find /lang/python -mindepth 1 -maxdepth 1 -type d -print0 | xargs -0I{} bash -c \ +find /snekbin/python -mindepth 1 -maxdepth 1 -type d -print0 | xargs -0I{} bash -c \ '{}/bin/python -m pip install --user -U -r requirements/eval-deps.pip' \; diff --git a/tests/test_integration.py b/tests/test_integration.py index 9ae2da80..7935b6d2 100644 --- a/tests/test_integration.py +++ b/tests/test_integration.py @@ -65,12 +65,12 @@ def test_multi_binary_support(self): "test default binary is used when binary_path not specified", ), ( - get_python_version_body | {"binary_path": "/lang/python/3.12/bin/python"}, + get_python_version_body | {"binary_path": "/snekbin/python/3.12/bin/python"}, "3.12\n", "test default binary is used when explicitly set", ), ( - get_python_version_body | {"binary_path": "/lang/python/3.13/bin/python"}, + get_python_version_body | {"binary_path": "/snekbin/python/3.13/bin/python"}, "3.13\n", "test alternative binary is used when set", ), @@ -85,10 +85,10 @@ def invalid_binary_paths(self): """Test that passing invalid binary paths result in no code execution.""" with run_gunicorn(): cases = [ - ("/bin/bash", "test files outside of /lang cannot be run"), + ("/bin/bash", "test files outside of /snekbin cannot be run"), ( - "/lang/../bin/bash", - "test path traversal still stops files outside /lang from running", + "/snekbin/../bin/bash", + "test path traversal still stops files outside /snekbin from running", ), ("/foo/bar", "test non-existant files are not run"), ] diff --git a/tests/test_nsjail.py b/tests/test_nsjail.py index 2d21ad3f..a3f13121 100644 --- a/tests/test_nsjail.py +++ b/tests/test_nsjail.py @@ -26,7 +26,7 @@ def setUp(self): # Hard-coded because it's non-trivial to parse the mount options. self.shm_mount_size = 40 * Size.MiB - self.default_binary_path = "/lang/python/default/bin/python" + self.default_binary_path = "/snekbin/python/default/bin/python" def eval_code(self, code: str): return self.nsjail.python3(["-c", code]) @@ -84,7 +84,7 @@ def test_subprocess_resource_unavailable(self): for _ in range({max_pids}): print(subprocess.Popen( [ - '/lang/python/default/bin/python', + '/snekbin/python/default/bin/python', '-c', 'import time; time.sleep(1)' ], From 3e3dfa003425e2d3114cd29e554e1b9ebce73565 Mon Sep 17 00:00:00 2001 From: Chris Lovering Date: Thu, 3 Oct 2024 22:15:06 +0100 Subject: [PATCH 12/16] Update invalid binary paths test to cover new error handling --- tests/test_integration.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/test_integration.py b/tests/test_integration.py index 7935b6d2..62a7d69c 100644 --- a/tests/test_integration.py +++ b/tests/test_integration.py @@ -85,19 +85,19 @@ def invalid_binary_paths(self): """Test that passing invalid binary paths result in no code execution.""" with run_gunicorn(): cases = [ - ("/bin/bash", "test files outside of /snekbin cannot be run"), + ("/abc/def", "test non-existant files are not run", "binary_path does not exist"), + ("/snekbin", "test directories are not ran", "binary_path is not a file"), ( - "/snekbin/../bin/bash", - "test path traversal still stops files outside /snekbin from running", + "/etc/hostname", + "test non-executable files are not ran", + "binary_path is not executable", ), - ("/foo/bar", "test non-existant files are not run"), ] - for path, msg in cases: - with self.subTest(msg=msg, path=path): + for path, msg, expected in cases: + with self.subTest(msg=msg, path=path, expected=expected): body = {"args": ["-c", "echo", "hi"], "binary_path": path} response, status = snekbox_request(body) self.assertEqual(status, 400) - expected = {"title": "binary_path file is invalid"} self.assertEqual(json.loads(response)["stdout"], expected) def test_eval(self): From 76ba51279da40d4361aa30f5b356d5fc5815b936 Mon Sep 17 00:00:00 2001 From: Chris Lovering Date: Thu, 3 Oct 2024 22:33:00 +0100 Subject: [PATCH 13/16] Add a note about multi-verison support to the README --- README.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/README.md b/README.md index c92ebdf7..2b4e9fd6 100644 --- a/README.md +++ b/README.md @@ -50,6 +50,14 @@ To run it in the background, use the `-d` option. See the documentation on [`doc The above command will make the API accessible on the host via `http://localhost:8060/`. Currently, there's only one endpoint: `http://localhost:8060/eval`. +### Python multi-version support + +By default, the binary ran within nsjail is the binary specified by `DEFAULT_BINARY_PATH` at the top of [`nsjail.py`]. This can be overridden by specifying `binary_path` in the request body of calls to `POST /eval` or by setting the `binary_path` kwarg if calling `NSJail.python3()` directly. + +Any binary that exists within the container is a valid value for `binary_path`. The main use case of this feature is currently to specify the version of Python to use. + +Python versions currently available can be found in the [`Dockerfile`] by looking for build stages that match `builder-py-*`. These binaries are then copied into the `base` build stage further down. + ## Configuration Configuration files can be edited directly. However, this requires rebuilding the image. Alternatively, a Docker volume or bind mounts can be used to override the configuration files at their default locations. @@ -126,9 +134,11 @@ See [CONTRIBUTING.md](.github/CONTRIBUTING.md). [7]: https://github.com/google/nsjail/blob/master/config.proto [`gunicorn.conf.py`]: config/gunicorn.conf.py [`snekbox.cfg`]: config/snekbox.cfg +[`nsjail.py`]: snekbox/nsjail.py [`snekapi.py`]: snekbox/api/snekapi.py [`resources`]: snekbox/api/resources [`docker-compose.yml`]: docker-compose.yml +[`Dockerfile`]: Dockerfile [`docker run`]: https://docs.docker.com/engine/reference/commandline/run/ [nsjail]: https://github.com/google/nsjail [falcon]: https://falconframework.org/ From 7afaffb13baf8e6f87ef84bccaf0806b9aa1c7eb Mon Sep 17 00:00:00 2001 From: ChrisJL Date: Sat, 5 Oct 2024 09:45:16 +0100 Subject: [PATCH 14/16] Correct spelling errors Co-authored-by: Mark <1515135+MarkKoz@users.noreply.github.com> --- README.md | 2 +- tests/test_integration.py | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 2b4e9fd6..6e9b0e81 100644 --- a/README.md +++ b/README.md @@ -52,7 +52,7 @@ The above command will make the API accessible on the host via `http://localhost ### Python multi-version support -By default, the binary ran within nsjail is the binary specified by `DEFAULT_BINARY_PATH` at the top of [`nsjail.py`]. This can be overridden by specifying `binary_path` in the request body of calls to `POST /eval` or by setting the `binary_path` kwarg if calling `NSJail.python3()` directly. +By default, the binary that runs within nsjail is the binary specified by `DEFAULT_BINARY_PATH` at the top of [`nsjail.py`]. This can be overridden by specifying `binary_path` in the request body of calls to `POST /eval` or by setting the `binary_path` kwarg if calling `NSJail.python3()` directly. Any binary that exists within the container is a valid value for `binary_path`. The main use case of this feature is currently to specify the version of Python to use. diff --git a/tests/test_integration.py b/tests/test_integration.py index 62a7d69c..e173dd36 100644 --- a/tests/test_integration.py +++ b/tests/test_integration.py @@ -85,11 +85,11 @@ def invalid_binary_paths(self): """Test that passing invalid binary paths result in no code execution.""" with run_gunicorn(): cases = [ - ("/abc/def", "test non-existant files are not run", "binary_path does not exist"), - ("/snekbin", "test directories are not ran", "binary_path is not a file"), + ("/abc/def", "test non-existent files are not run", "binary_path does not exist"), + ("/snekbin", "test directories are not run", "binary_path is not a file"), ( "/etc/hostname", - "test non-executable files are not ran", + "test non-executable files are not run", "binary_path is not executable", ), ] From edcaed6ad90f71ff61f6ceb7194de611ceeec592 Mon Sep 17 00:00:00 2001 From: Chris Lovering Date: Sat, 5 Oct 2024 09:54:03 +0100 Subject: [PATCH 15/16] Use 'executable' over 'binary' to be clearer as to what is supported --- README.md | 4 ++-- snekbox/api/resources/eval.py | 28 ++++++++++++++-------------- snekbox/nsjail.py | 18 ++++++++++++------ tests/test_integration.py | 32 +++++++++++++++++++------------- 4 files changed, 47 insertions(+), 35 deletions(-) diff --git a/README.md b/README.md index 6e9b0e81..52cc7f15 100644 --- a/README.md +++ b/README.md @@ -52,9 +52,9 @@ The above command will make the API accessible on the host via `http://localhost ### Python multi-version support -By default, the binary that runs within nsjail is the binary specified by `DEFAULT_BINARY_PATH` at the top of [`nsjail.py`]. This can be overridden by specifying `binary_path` in the request body of calls to `POST /eval` or by setting the `binary_path` kwarg if calling `NSJail.python3()` directly. +By default, the executable that runs within nsjail is defined by `DEFAULT_EXECUTABLE_PATH` at the top of [`nsjail.py`]. This can be overridden by specifying `executable_path` in the request body of calls to `POST /eval` or by setting the `executable_path` kwarg if calling `NSJail.python3()` directly. -Any binary that exists within the container is a valid value for `binary_path`. The main use case of this feature is currently to specify the version of Python to use. +Any executable that exists within the container is a valid value for `executable_path`. The main use case of this feature is currently to specify the version of Python to use. Python versions currently available can be found in the [`Dockerfile`] by looking for build stages that match `builder-py-*`. These binaries are then copied into the `base` build stage further down. diff --git a/snekbox/api/resources/eval.py b/snekbox/api/resources/eval.py index 3172f609..b53899a5 100644 --- a/snekbox/api/resources/eval.py +++ b/snekbox/api/resources/eval.py @@ -6,7 +6,7 @@ import falcon from falcon.media.validators.jsonschema import validate -from snekbox.nsjail import DEFAULT_BINARY_PATH, NsJail +from snekbox.nsjail import DEFAULT_EXECUTABLE_PATH, NsJail from snekbox.snekio import FileAttachment, ParsingError __all__ = ("EvalResource",) @@ -44,7 +44,7 @@ class EvalResource: "required": ["path"], }, }, - "binary_path": {"type": "string"}, + "executable_path": {"type": "string"}, }, "anyOf": [ {"required": ["input"]}, @@ -125,24 +125,24 @@ def on_post(self, req: falcon.Request, resp: falcon.Response) -> None: body.setdefault("args", ["-c"]) body["args"].append(body["input"]) - binary_path = body.get("binary_path") - if not binary_path: - binary_path = DEFAULT_BINARY_PATH + executable_path = body.get("executable_path") + if not executable_path: + executable_path = DEFAULT_EXECUTABLE_PATH else: - binary_path = Path(binary_path) - if not binary_path.exists(): - raise falcon.HTTPBadRequest(title="binary_path does not exist") - if not binary_path.is_file(): - raise falcon.HTTPBadRequest(title="binary_path is not a file") - if not binary_path.stat().st_mode & 0o100 == 0o100: - raise falcon.HTTPBadRequest(title="binary_path is not executable") - binary_path = binary_path.resolve().as_posix() + executable_path = Path(executable_path) + if not executable_path.exists(): + raise falcon.HTTPBadRequest(title="executable_path does not exist") + if not executable_path.is_file(): + raise falcon.HTTPBadRequest(title="executable_path is not a file") + if not executable_path.stat().st_mode & 0o100 == 0o100: + raise falcon.HTTPBadRequest(title="executable_path is not executable") + executable_path = executable_path.resolve().as_posix() try: result = self.nsjail.python3( py_args=body["args"], files=[FileAttachment.from_dict(file) for file in body.get("files", [])], - binary_path=binary_path, + executable_path=executable_path, ) except ParsingError as e: raise falcon.HTTPBadRequest(title="Request file is invalid", description=str(e)) diff --git a/snekbox/nsjail.py b/snekbox/nsjail.py index fe95f802..adbf69e0 100644 --- a/snekbox/nsjail.py +++ b/snekbox/nsjail.py @@ -26,7 +26,7 @@ LOG_PATTERN = re.compile( r"\[(?P(I)|[DWEF])\]\[.+?\](?(2)|(?P\[\d+\] .+?:\d+ )) ?(?P.+)" ) -DEFAULT_BINARY_PATH = "/snekbin/python/default/bin/python" +DEFAULT_EXECUTABLE_PATH = "/snekbin/python/default/bin/python" class NsJail: @@ -174,7 +174,7 @@ def _build_args( nsjail_args: Iterable[str], log_path: str, fs_home: str, - binary_path: str, + executable_path: str, ) -> Sequence[str]: if self.cgroup_version == 2: nsjail_args = ("--use_cgroupv2", *nsjail_args) @@ -203,7 +203,7 @@ def _build_args( log_path, *nsjail_args, "--", - binary_path, + executable_path, *iter_lstrip(py_args), ] @@ -262,7 +262,7 @@ def python3( py_args: Iterable[str], files: Iterable[FileAttachment] = (), nsjail_args: Iterable[str] = (), - binary_path: Path = DEFAULT_BINARY_PATH, + executable_path: Path = DEFAULT_EXECUTABLE_PATH, ) -> EvalResult: """ Execute Python 3 code in an isolated environment and return the completed process. @@ -271,14 +271,20 @@ def python3( py_args: Arguments to pass to Python. files: FileAttachments to write to the sandbox prior to running Python. nsjail_args: Overrides for the NsJail configuration. - binary_path: The path to the binary to execute under. + executable_path: The path to the executable to run within nsjail. """ with NamedTemporaryFile() as nsj_log, MemFS( instance_size=self.memfs_instance_size, home=self.memfs_home, output=self.memfs_output, ) as fs: - args = self._build_args(py_args, nsjail_args, nsj_log.name, str(fs.home), binary_path) + args = self._build_args( + py_args, + nsjail_args, + nsj_log.name, + str(fs.home), + executable_path, + ) try: files_written = self._write_files(fs.home, files) diff --git a/tests/test_integration.py b/tests/test_integration.py index e173dd36..0d8f700d 100644 --- a/tests/test_integration.py +++ b/tests/test_integration.py @@ -52,8 +52,8 @@ def test_memory_limit_separate_per_process(self): self.assertTrue(all(status == 200 for status in statuses)) self.assertTrue(all(json.loads(response)["returncode"] == 0 for response in responses)) - def test_multi_binary_support(self): - """Test eval requests with different binary paths set.""" + def test_alternate_executable_support(self): + """Test eval requests with different executable paths set.""" with run_gunicorn(): get_python_version_body = { "input": "import sys; print('.'.join(map(str, sys.version_info[:2])))" @@ -62,17 +62,19 @@ def test_multi_binary_support(self): ( get_python_version_body, "3.12\n", - "test default binary is used when binary_path not specified", + "test default executable is used when executable_path not specified", ), ( - get_python_version_body | {"binary_path": "/snekbin/python/3.12/bin/python"}, + get_python_version_body + | {"executable_path": "/snekbin/python/3.12/bin/python"}, "3.12\n", - "test default binary is used when explicitly set", + "test default executable is used when explicitly set", ), ( - get_python_version_body | {"binary_path": "/snekbin/python/3.13/bin/python"}, + get_python_version_body + | {"executable_path": "/snekbin/python/3.13/bin/python"}, "3.13\n", - "test alternative binary is used when set", + "test alternative executable is used when set", ), ] for body, expected, msg in cases: @@ -81,21 +83,25 @@ def test_multi_binary_support(self): self.assertEqual(status, 200) self.assertEqual(json.loads(response)["stdout"], expected) - def invalid_binary_paths(self): - """Test that passing invalid binary paths result in no code execution.""" + def invalid_executable_paths(self): + """Test that passing invalid executable paths result in no code execution.""" with run_gunicorn(): cases = [ - ("/abc/def", "test non-existent files are not run", "binary_path does not exist"), - ("/snekbin", "test directories are not run", "binary_path is not a file"), + ( + "/abc/def", + "test non-existent files are not run", + "executable_path does not exist", + ), + ("/snekbin", "test directories are not run", "executable_path is not a file"), ( "/etc/hostname", "test non-executable files are not run", - "binary_path is not executable", + "executable_path is not executable", ), ] for path, msg, expected in cases: with self.subTest(msg=msg, path=path, expected=expected): - body = {"args": ["-c", "echo", "hi"], "binary_path": path} + body = {"args": ["-c", "echo", "hi"], "executable_path": path} response, status = snekbox_request(body) self.assertEqual(status, 400) self.assertEqual(json.loads(response)["stdout"], expected) From b58f98bd5127cac43cba4dcff66f9d7a01527576 Mon Sep 17 00:00:00 2001 From: Chris Lovering Date: Sat, 5 Oct 2024 09:56:09 +0100 Subject: [PATCH 16/16] Reuse the default executable path const in tests --- tests/test_nsjail.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/tests/test_nsjail.py b/tests/test_nsjail.py index a3f13121..6764f1ef 100644 --- a/tests/test_nsjail.py +++ b/tests/test_nsjail.py @@ -9,7 +9,7 @@ from pathlib import Path from textwrap import dedent -from snekbox.nsjail import NsJail +from snekbox.nsjail import DEFAULT_EXECUTABLE_PATH, NsJail from snekbox.snekio import FileAttachment from snekbox.snekio.filesystem import Size @@ -26,8 +26,6 @@ def setUp(self): # Hard-coded because it's non-trivial to parse the mount options. self.shm_mount_size = 40 * Size.MiB - self.default_binary_path = "/snekbin/python/default/bin/python" - def eval_code(self, code: str): return self.nsjail.python3(["-c", code]) @@ -549,7 +547,7 @@ def test_py_args(self): for args, expected in cases: with self.subTest(args=args): result = self.nsjail.python3(py_args=args) - idx = result.args.index(self.default_binary_path) + idx = result.args.index(DEFAULT_EXECUTABLE_PATH) self.assertEqual(result.args[idx + 1 :], expected) self.assertEqual(result.returncode, 0)