Multiple binary support api #196

ChrisLovering · 2023-10-10T13:15:06Z

This allows setting a binary_path key in the request body to /eval to specify which binary to run under.

If not supplied, it defaults to "/snekbin/python/default/bin/python".

0c9b234 is needed as previously we were dropping the text after the last period when creating the path to install Python to. However, for versions such as3.13-dev this would result in it being installed into /snekbin/3. Instead, we now split on the last . or the last -

coveralls · 2023-10-10T13:53:27Z

coverage: 89.158% (-1.7%) from 90.826%
when pulling b58f98b on multi-version-api
into b276a04 on main.

MarkKoz

Should we add something in the readme about multiple version support? Can just be a blurb I think.

MarkKoz · 2023-10-11T15:36:12Z

snekbox/api/resources/eval.py

+        if binary_path:
+            binary_path = Path(binary_path)
+            if (
+                not binary_path.resolve().as_posix().startswith("/lang/")


Why restrict it to just files in that directory? It seems like an arbitrary restriction I don't think we should have.

Oh I missed this in your description

if supplied, snekbox ensures that the path is within /lang (to avoid executing arbitrary binaries on the fs) and that the file exists.

It is fairly trivial to bypass this restriction by using the execve syscall. We cannot block this syscall either (at least not through nsjail's seccomp filters). That is due to a quirk in which the filter is applied before nsjail's own execve call, causing nsjail to commit suicide.

Granted, there may be use cases which don't have binaries in /lang that make it possible to call syscalls. But I think if a request comes up to have such restriction, we can add it as a later feature.

MarkKoz · 2023-10-11T15:37:19Z

snekbox/api/resources/eval.py

+                not binary_path.resolve().as_posix().startswith("/lang/")
+                or not binary_path.is_file()
+            ):
+                raise falcon.HTTPBadRequest(title="binary_path file is invalid")


Let's be more specific. We should also check that the file exists if we aren't already (and that should be a distinct error message)

MarkKoz · 2023-10-11T15:47:44Z

snekbox/nsjail.py

        """
+        if not binary_path:
+            binary_path = "/lang/python/default/bin/python"


Let's set the default in the function parameters instead.

Had to do something slightly idfferent so that hte default cna be used both by calling python3 directly, and when calling the API 66c6d27

MarkKoz · 2023-10-20T03:03:27Z

snekbox/nsjail.py

@@ -188,7 +189,12 @@ 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.
+                If None, defaults to "/lang/python/default/bin/python"


I think we should rename /lang since it's going to be more prominent now. Maybe /snekbin? — doubles as a kinda clever play on "snekbox". If so, we also have to rename our references to the path in our CONTRIBUTING.md docs.

Yeah I agree this is a good change f726695

MarkKoz · 2023-10-20T03:11:46Z

tests/test_integration.py

+        """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"),


typo

Suggested change

("/bin/bash", "test files outside of /lang cannot be ran"),

("/bin/bash", "test files outside of /lang cannot be run"),

MarkKoz · 2023-10-20T03:12:00Z

tests/test_integration.py

+                    "/lang/../bin/bash",
+                    "test path traversal still stops files outside /lang from running",
+                ),
+                ("/foo/bar", "test non-existant files are not ran"),


typo

Suggested change

("/foo/bar", "test non-existant files are not ran"),

("/foo/bar", "test non-existant files are not run"),

ChrisLovering · 2024-01-24T20:55:46Z

I plan to pick this PR up soon just had time to rebase this evening

jchristgit

This is really cool!!!

jchristgit · 2024-04-06T14:39:13Z

snekbox/api/resources/eval.py

+            binary_path = Path(binary_path)
+            if (
+                not binary_path.resolve().as_posix().startswith("/lang/")
+                or not binary_path.is_file()


Should we also check that it's executable?

Suggested change

or not binary_path.is_file()

or not binary_path.is_file()

or not binary_path.stat().st_mode & 0o100 == 0o100

Yes! 81573b2

jchristgit · 2024-04-06T14:39:42Z

snekbox/api/resources/eval.py

+        if binary_path:
+            binary_path = Path(binary_path)
+            if (
+                not binary_path.resolve().as_posix().startswith("/lang/")


Should we move the resolve().as_posix() calls above so the pathlib calls below can make use of it? Or do they mutate the internal object (source of eldritch horrors)

changed this func quite a bit since then, so this is no longer relevant

HassanAbouelela · 2024-07-30T03:07:37Z

Hey how's this PR going? It would be very useful for a project I'm working on.

This is needed as dev builds such as 3.13-dev use the suffix -dev, rather than a patch version.

This was needed due to wanting a default value when calling python3 diurectly, but also when not specified via the API call

ChrisLovering · 2024-10-03T21:08:30Z

Now ready for (re)review

README.md

tests/test_integration.py

MarkKoz · 2024-10-05T00:50:07Z

snekbox/api/resources/eval.py

@@ -43,6 +44,7 @@ class EvalResource:
                    "required": ["path"],
                },
            },
+            "binary_path": {"type": "string"},


Maybe "executable_path" is a better name than "binary_path"? I'm also okay with the latter though.

Yeah I agree, makes it clearer what is supported here, as it's not limitted to just binaries. edcaed6

Co-authored-by: Mark <[email protected]>

MarkKoz

thanks!

ChrisLovering requested review from jb3, Den4200 and MarkKoz as code owners October 10, 2023 13:15

ChrisLovering changed the title ~~Multi version api~~ Multiple binary support api Oct 10, 2023

ChrisLovering force-pushed the multi-version-api branch 3 times, most recently from 1b512b8 to 5f67817 Compare October 10, 2023 13:49

ChrisLovering marked this pull request as draft October 10, 2023 14:16

ChrisLovering force-pushed the multi-version-api branch from b990ac2 to c97b74d Compare October 10, 2023 14:27

MarkKoz requested changes Oct 20, 2023

View reviewed changes

ChrisLovering force-pushed the multi-version-api branch 2 times, most recently from a784d78 to 6dcf094 Compare January 24, 2024 20:54

jchristgit reviewed Apr 6, 2024

View reviewed changes

ChrisLovering mentioned this pull request Oct 3, 2024

Py 3.13 rc #217

Merged

ChrisLovering added 8 commits October 3, 2024 21:24

Allow specifying a binary path when calling NsJail.python3

ed81df4

Add tests for multi-version support

6cc4dba

Allow setting a binary_path when calling /eval

c24785d

Also split on hyphens in build python script

0c9b234

This is needed as dev builds such as 3.13-dev use the suffix -dev, rather than a patch version.

Add additional tests to ensure invalid binary paths are not ran

d9b8ab3

Drop restriction of only running binaries from /lang

f957648

Add specific error for when the specified binary path does not exist

d936970

Also check if the specified binary path is execuatable

81573b2

ChrisLovering force-pushed the multi-version-api branch from 6dcf094 to 39c3e79 Compare October 3, 2024 21:03

ChrisLovering changed the base branch from main to py-3.13-rc October 3, 2024 21:03

ChrisLovering added 3 commits October 3, 2024 22:06

Use a shared default const

66c6d27

This was needed due to wanting a default value when calling python3 diurectly, but also when not specified via the API call

Correct spelling in test cases

30d51d1

Update all references to /lang to /snekbin

f726695

ChrisLovering force-pushed the multi-version-api branch from 39c3e79 to f726695 Compare October 3, 2024 21:06

ChrisLovering marked this pull request as ready for review October 3, 2024 21:08

ChrisLovering added 2 commits October 3, 2024 22:15

Update invalid binary paths test to cover new error handling

3e3dfa0

Add a note about multi-verison support to the README

76ba512

ChrisLovering mentioned this pull request Oct 3, 2024

Support both Python 3.12 and 3.13 in eval python-discord/bot#3179

Merged

Base automatically changed from py-3.13-rc to main October 3, 2024 22:33

ChrisLovering requested review from jchristgit and MarkKoz October 4, 2024 07:54

MarkKoz requested changes Oct 5, 2024

View reviewed changes

ChrisLovering and others added 3 commits October 5, 2024 09:45

Correct spelling errors

7afaffb

Co-authored-by: Mark <[email protected]>

Use 'executable' over 'binary' to be clearer as to what is supported

edcaed6

Reuse the default executable path const in tests

b58f98b

MarkKoz approved these changes Oct 5, 2024

View reviewed changes

ChrisLovering merged commit b58f98b into main Oct 5, 2024
8 checks passed

ChrisLovering deleted the multi-version-api branch October 5, 2024 21:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiple binary support api #196

Multiple binary support api #196

ChrisLovering commented Oct 10, 2023 •

edited

Loading

coveralls commented Oct 10, 2023 •

edited

Loading

MarkKoz left a comment

MarkKoz Oct 11, 2023

MarkKoz Oct 20, 2023

ChrisLovering Oct 3, 2024

MarkKoz Oct 11, 2023

ChrisLovering Oct 3, 2024

MarkKoz Oct 11, 2023

ChrisLovering Oct 3, 2024

MarkKoz Oct 20, 2023

ChrisLovering Oct 3, 2024

MarkKoz Oct 20, 2023

ChrisLovering Oct 3, 2024

MarkKoz Oct 20, 2023

ChrisLovering Oct 3, 2024

ChrisLovering commented Jan 24, 2024

jchristgit left a comment

jchristgit Apr 6, 2024

ChrisLovering Oct 3, 2024

jchristgit Apr 6, 2024

ChrisLovering Oct 3, 2024

HassanAbouelela commented Jul 30, 2024

ChrisLovering commented Oct 3, 2024

MarkKoz Oct 5, 2024

ChrisLovering Oct 5, 2024

MarkKoz left a comment

	("/bin/bash", "test files outside of /lang cannot be ran"),
	("/bin/bash", "test files outside of /lang cannot be run"),

	("/foo/bar", "test non-existant files are not ran"),
	("/foo/bar", "test non-existant files are not run"),

	or not binary_path.is_file()
	or not binary_path.is_file()
	or not binary_path.stat().st_mode & 0o100 == 0o100

Multiple binary support api #196

Multiple binary support api #196

Conversation

ChrisLovering commented Oct 10, 2023 • edited Loading

coveralls commented Oct 10, 2023 • edited Loading

MarkKoz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisLovering commented Jan 24, 2024

jchristgit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HassanAbouelela commented Jul 30, 2024

ChrisLovering commented Oct 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkKoz left a comment

Choose a reason for hiding this comment

ChrisLovering commented Oct 10, 2023 •

edited

Loading

coveralls commented Oct 10, 2023 •

edited

Loading