Initial implementation of landlock calls #903

Riolku · 2021-09-11T04:58:24Z

No description provided.

dmoj-build · 2021-09-11T04:58:26Z

Can one of the admins verify this patch?

codecov-commenter · 2021-09-11T05:04:04Z

Codecov Report

Base: 81.44% // Head: 84.17% // Increases project coverage by +2.72% 🎉

Coverage data is based on head (244c4a1) compared to base (1f17cd2).
Patch coverage: 85.71% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #903      +/-   ##
==========================================
+ Coverage   81.44%   84.17%   +2.72%     
==========================================
  Files         137      137              
  Lines        4920     4953      +33     
==========================================
+ Hits         4007     4169     +162     
+ Misses        913      784     -129

Impacted Files	Coverage Δ
dmoj/cptbox/__init__.py	`100.00% <ø> (ø)`
dmoj/executors/base_executor.py	`86.86% <ø> (ø)`
dmoj/cptbox/compiler_isolate.py	`56.60% <71.42%> (+7.66%)`	⬆️
dmoj/cptbox/tracer.py	`76.82% <84.21%> (+16.39%)`	⬆️
dmoj/cptbox/isolate.py	`89.75% <100.00%> (+38.53%)`	⬆️
dmoj/executors/RUST.py	`100.00% <100.00%> (ø)`
dmoj/executors/SCALA.py	`95.74% <100.00%> (+0.09%)`	⬆️
dmoj/judge.py	`54.68% <100.00%> (+1.34%)`	⬆️
dmoj/result.py	`83.11% <0.00%> (-1.30%)`	⬇️
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

kiritofeng · 2021-09-11T05:57:08Z

ok to test

dmoj/cptbox/_cptbox.pyx

dmoj/cptbox/helper.cpp

dmoj/cptbox/landlock_helper.c

dmoj/cptbox/helper.cpp

dmoj/cptbox/ptproc.cpp

dmoj/cptbox/helper.h

dmoj/cptbox/ptbox.h

Riolku · 2021-09-15T17:03:35Z

Landlock applies LANDLOCK_FS_READ_DIR recursively. This means that since we allow READ_DIR /, a process can search the whole filesystem tree. We can limit getdents, but a process can still arbitrarily determine the existence of a file/directory. This might be acceptable.

Riolku · 2021-09-17T04:19:12Z

Given that stuff is now functional, we can begin to review this.

dmoj/cptbox/_cptbox.pyx

dmoj/cptbox/helper.cpp

dmoj/cptbox/landlock_header.h

dmoj/cptbox/landlock_helpers.cpp

dmoj/cptbox/tracer.py

dmoj/cptbox/isolate.py

Riolku · 2023-01-03T06:28:42Z

Landlock ABI 3 shipped in 5.19 which I think is able to supplant our syscall simulation here.

We should hard-require it; @quantum5 is working on making a 6.x kernel available in the ARM64 CI runner.

Would this also mean we can do away with some of the header files that we copied? landlock_header.h specifically.

Xyene · 2023-01-03T06:31:09Z

No, we need them to be able to do the initial Landlock calls to figure out if Landlock is supported at all.

A helper header is provided so as to avoid issues with missing headers, so we can compile unconditionally.

One key note to make here is that WRITE permissions imply READ permissions, because otherwise, calls with O_RDWR will fail even if one rule grants read and another grants write. Another thing to note is that under landlock, only the main process can access /proc. This is because we can't add rules when the children spawn.

Note that under the current version of landlock, some syscalls are not handled by landlock, including `stat` and `access`.

Under landlock, linking and renaming throw EXDEV if `src` and `dst` are not in the same directory. This is unacceptable for us, so we simulate the calls. Because we are simulating, we want to make sure that `flags` is zero for `linkat`, otherwise it's likely that we have done something unintended.

Under landlock, since /proc is not accessible in a subprocess, SCALA calls `mincore`. We allow this in order for it to pass. Also, since `execve` is checked under landlock, we need to add `/bin` to the list of readable directories.

Xyene · 2023-01-08T05:05:06Z

dmoj/cptbox/helper.cpp

+
+    int landlock_version = get_landlock_version();
+    if (landlock_version < 2) {
+        // ABI not old enough. Skip to seccomp


Suggested change

// ABI not old enough. Skip to seccomp

// ABI not new enough. Skip to seccomp

Xyene · 2023-01-08T05:06:39Z

dmoj/cptbox/helper.cpp

+                               LANDLOCK_ACCESS_FS_REMOVE_FILE | LANDLOCK_ACCESS_FS_READ_DIR |
+                               LANDLOCK_ACCESS_FS_MAKE_REG | LANDLOCK_ACCESS_FS_MAKE_SYM | LANDLOCK_ACCESS_FS_MAKE_DIR |
+                               LANDLOCK_ACCESS_FS_REFER)) {
+        // landlock_add_rules logs errors


Could this comment have more details? I don't understand what it's referring to, is this comment unrelated to the following close?

Xyene · 2023-01-08T05:09:09Z

dmoj/cptbox/helper.cpp

+        return 0;
+    return -1;
+#else
+    return 0;  // FreeBSD does not have landlock


We should abort() here.

How? Consumers of this function throw a python error, but what would be appropriate to do here?

Got it. I think has_landlock should be implemented here rather than in Python, so we can return false order FREEBSD.

Xyene · 2023-01-08T05:10:23Z

dmoj/cptbox/landlock_header.h

+#define LANDLOCK_ACCESS_FS_MAKE_SYM     (1ULL << 12)
+#endif /* _LINUX_LANDLOCK_H */
+
+// Not always defined, depends on ABI version.


Xyene · 2023-01-08T05:12:36Z

dmoj/cptbox/landlock_helpers.h

@@ -0,0 +1,3 @@
+#include "landlock_header.h"
+
+int landlock_add_rules(const int ruleset_fd, const char **paths, __u64 access_rule);


Could we name this cptbox_landlock_add_rules? Makes it easier to keep track of what's provided by the kernel versus by us.

Xyene · 2023-01-08T05:13:42Z

dmoj/cptbox/landlock_helpers.cpp

+
+    if (rule.parent_fd < 0) {
+        if (errno == ENOENT)
+            goto close_fd;  // missing files are ignored


Nit: parens even around single-statement branches / loops / etc. I know not all cptbox code is written like this, but all new code should be.

Xyene · 2023-01-08T05:15:29Z

dmoj/cptbox/tracer.py

+        if security is not None:
+            self.configure_files(security.read_fs, security.write_fs)
+        else:
+            self.configure_files([], [])


Is this branch necessary at all? Seems a little weird to have.

Unsure. security defaults to None. I don't know why.

My question was more: it seems weird that the default uninitialized state would be different from initializing with [].

We could maybe avoid it by having a default of empty list in _cptbox.pyx, but I don't see how else to avoid it. configure_files is the function that sets the landlock_* attributes.

We could, in theory, have them defaulted to empty and then have this branch to set them to something, but I don't love that.

Xyene · 2023-01-08T05:17:34Z

dmoj/cptbox/tracer.py

@@ -223,6 +244,8 @@ def wait(self) -> int:
                raise RuntimeError('failed to spawn child')
            elif self.returncode == PTBOX_SPAWN_FAIL_SETAFFINITY:
                raise RuntimeError('failed to set child affinity')
+            elif self.returncode == PTBOX_SPAWN_FAIL_LANDLOCK:
+                raise RuntimeError('landlock configuration failed')


Suggested change

raise RuntimeError('landlock configuration failed')

raise RuntimeError('Landlock configuration failed')

Nit, here and in other user-facing strings as well as comments. The kernel docs refer to it as "Landlock" (despite "seccomp" being lowercased).

Xyene · 2023-01-08T05:18:15Z

dmoj/executors/SCALA.py

@@ -20,6 +20,7 @@ class Executor(JavaExecutor):
        ExactFile('/bin/bash'),
        RecursiveDir('/etc/alternatives'),
    ]
+    compiler_syscalls = ['mincore']


Why did this change?

SCALA takes a different path under landlock last I checked, and that path requires mincore. I believe it had to do with how landlock deals with /proc/<pid>.

We decided at some point to simply allow the call.

Ack, thanks for explaining. Could we split this out in its own commit?

Eh, I guess it kind of is already. Fine to leave it as-is.

Xyene · 2023-01-08T05:20:42Z

dmoj/cptbox/helper.cpp

+int get_landlock_version() {
+#if !PTBOX_FREEBSD
+    char *sandbox_mode = getenv("DMOJ_SANDBOX_MODE");
+    if (sandbox_mode != nullptr && strcmp(sandbox_mode, "ptrace+seccomp") == 0) {


This is a surprising place for this check to live, and I think the check should be stricter than this.

We should validate that the string is one of:

ptrace+seccomp

ptrace+seccomp+landlock

auto

and otherwise bail out. Otherwise we can't confidently make changes to the interpretation of this environment variable, as we'd have been too lax in its inputs in earlier versions.

I agree that this location is strange. Where should it live?

I think living in this file is probably fine, and it should return an enum. Then it should be consulted in the spawn routine alongside the Landlock version we have available. My concern here is more about subtly overloading the semantics of get_landlock_version than anything else.

We should bail if someone requests:

seccomp or seccomp+landlock support on FreeBSD

ptrace-only support on Linux

seccomp+landlock on a Linux with too old a Landlock ABI

This sanity check could exist in judge.py alongside all the other sanity checks, but I don't feel strongly about that.

Xyene · 2023-01-08T05:25:24Z

dmoj/cptbox/helper.h

@@ -27,8 +28,16 @@ struct child_config {
    int *seccomp_handlers;
    // 64 cores ought to be enough for anyone.
    unsigned long cpu_affinity_mask;
+    const char **landlock_read_exact_files;


This may end up looking a little cleaner if we have a

struct { const char **read_exact_files; ... } landlock;

or even

const struct { char **read_exact_files; ... } landlock;

Xyene · 2023-01-08T05:26:47Z

dmoj/executors/RUST.py

+        assert self._executable is not None
+        # Under landlock we need this for execve to work.
+        # We use `self._executable` because it is copied when caching executors, but other properties are not.
+        return super().get_fs() + [ExactFile(self._executable)]


The fact that both get_executable and get_compiled_file exist but return possibly different values is an abomination, and we should fix this. You don't have to do so in this PR, but please leave a comment to the effect of this being a hack to work around that.

The problem isn't because get_executable and get_compiled_file both exist. Indeed, I expect them to return the same file for RUST.

The reason they don't is because the cached executor isn't a full copy of the original. Instead, it simply sets _executable and _dir on the returned object, and nothing else.

For rust, that means that we try and lock a new directory, which is obviously wrong.

To bypass it I use _executable here, but the proper fix may be instead to look into the caching.

Here's the relevant line: https://github.com/DMOJ/judge-server/blob/master/dmoj/executors/compiled_executor.py#L58

Also, as for get_executable vs get_compiled_file, check out Java's usage of the methods:

https://github.com/DMOJ/judge-server/blob/master/dmoj/executors/java_executor.py#L82

Maybe these exist because Java's compiled file isn't the executable?

Xyene · 2023-01-08T05:27:20Z

dmoj/executors/base_executor.py

@@ -33,6 +33,7 @@
    ExactFile('/dev/urandom'),
    ExactFile('/dev/random'),
    *USR_DIR,
+    RecursiveDir('/bin'),  # required under landlock when /bin is not a symlink, since we check execve.


Fine to not leave a comment here.

Xyene · 2023-01-08T05:29:04Z

dmoj/cptbox/helper.cpp

+        return PTBOX_SPAWN_FAIL_LANDLOCK;
+    }
+
+    // Note: WRITE must imply READ. This is required because even if one rule allows writing and another allows reading,


The O_TRUNC handling in Landlock scares me. Could we add some testcases with a custom grader that make sure that we don't allow truncating etc.?

Riolku force-pushed the landlock-impl branch from 0c95e3d to 394de48 Compare September 11, 2021 05:02

Xyene reviewed Sep 11, 2021

View reviewed changes

dmoj/cptbox/helper.h Outdated Show resolved Hide resolved

Xyene reviewed Sep 12, 2021

View reviewed changes

dmoj/cptbox/ptbox.h Outdated Show resolved Hide resolved

Riolku force-pushed the landlock-impl branch 12 times, most recently from e2644de to 7f33dfa Compare September 15, 2021 17:02

Riolku force-pushed the landlock-impl branch 5 times, most recently from 1bdd49c to 7c5316f Compare September 17, 2021 04:14

Riolku marked this pull request as ready for review September 17, 2021 04:18

quantum5 reviewed Sep 17, 2021

View reviewed changes

Riolku force-pushed the landlock-impl branch from 93f63b4 to 81d4bb3 Compare September 26, 2021 05:34

Riolku closed this Sep 26, 2021

Riolku force-pushed the landlock-impl branch from 510c369 to d6555c7 Compare January 3, 2023 06:02

cptbox: add landlock helpers

3c7abc1

A helper header is provided so as to avoid issues with missing headers, so we can compile unconditionally.

Riolku force-pushed the landlock-impl branch 2 times, most recently from a2c7e1e to e8f4ed0 Compare January 4, 2023 04:49

Riolku added 6 commits January 4, 2023 00:14

cptbox: add landlock to tracer

e91eccb

cptbox: add landlock to normal isolate tracer

2e83f68

Note that under the current version of landlock, some syscalls are not handled by landlock, including `stat` and `access`.

executors: fixup executors for landlock

4687eea

Under landlock, since /proc is not accessible in a subprocess, SCALA calls `mincore`. We allow this in order for it to pass. Also, since `execve` is checked under landlock, we need to add `/bin` to the list of readable directories.

judge: add startup message for landlock

b366e00

Riolku force-pushed the landlock-impl branch 2 times, most recently from de66427 to d20bd11 Compare January 4, 2023 05:15

ci: add pure seccomp to arm testing

244c4a1

Riolku force-pushed the landlock-impl branch from d20bd11 to 244c4a1 Compare January 4, 2023 05:24

Xyene reviewed Jan 8, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation of landlock calls #903

Initial implementation of landlock calls #903

Riolku commented Sep 11, 2021

dmoj-build commented Sep 11, 2021

codecov-commenter commented Sep 11, 2021 •

edited

Loading

kiritofeng commented Sep 11, 2021

Riolku commented Sep 15, 2021

Riolku commented Sep 17, 2021

Riolku commented Jan 3, 2023

Xyene commented Jan 3, 2023

Xyene Jan 8, 2023

Xyene Jan 8, 2023

Xyene Jan 8, 2023

Riolku Jan 8, 2023

Xyene Jan 9, 2023

Xyene Jan 8, 2023

Xyene Jan 8, 2023

Xyene Jan 8, 2023

Xyene Jan 8, 2023 •

edited

Loading

Riolku Jan 8, 2023

Xyene Jan 9, 2023

Riolku Jan 9, 2023

Xyene Jan 8, 2023

Xyene Jan 8, 2023

Riolku Jan 8, 2023

Xyene Jan 9, 2023

Xyene Jan 9, 2023

Xyene Jan 8, 2023

Riolku Jan 8, 2023

Xyene Jan 9, 2023

Xyene Jan 8, 2023

Xyene Jan 8, 2023

Riolku Jan 8, 2023

Xyene Jan 8, 2023

Xyene Jan 8, 2023

	// ABI not old enough. Skip to seccomp
	// ABI not new enough. Skip to seccomp

		@@ -0,0 +1,3 @@
		#include "landlock_header.h"

		int landlock_add_rules(const int ruleset_fd, const char **paths, __u64 access_rule);

	raise RuntimeError('landlock configuration failed')
	raise RuntimeError('Landlock configuration failed')

Initial implementation of landlock calls #903

Are you sure you want to change the base?

Initial implementation of landlock calls #903

Conversation

Riolku commented Sep 11, 2021

dmoj-build commented Sep 11, 2021

codecov-commenter commented Sep 11, 2021 • edited Loading

Codecov Report

kiritofeng commented Sep 11, 2021

Riolku commented Sep 15, 2021

Riolku commented Sep 17, 2021

Riolku commented Jan 3, 2023

Xyene commented Jan 3, 2023

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

Xyene Jan 8, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 11, 2021 •

edited

Loading

Xyene Jan 8, 2023 •

edited

Loading