Enforce the filesystem layer with Landlock instead of bubblewrap - #8
Merged
Merged
Conversation
Bubblewrap needs an unprivileged user namespace, which a stock Docker container denies three times over: the default seccomp profile blocks unshare/mount/pivot_root, the docker-default AppArmor profile carries a blanket "deny mount", and Ubuntu 23.10+ additionally requires an explicit userns rule. Lifting all three means shipping a custom AppArmor profile to every build host and starting containers with --security-opt, so the sandbox could never be enabled from inside the container alone. Landlock inverts that: a task may always restrict itself further, so no privileges, no capabilities and no container flags are required. The policy is applied by a small static helper that reads the same ro/rw path spec the bubblewrap call used, then execs the build command. - core/phobos-landlock.c: applies the ruleset and execs. Refuses to run when the kernel cannot enforce the requested minimum ABI rather than degrading silently, and sets PR_SET_NO_NEW_PRIVS before restricting. - core/phobos-filesystem.sh: --ro-bind/--bind become --rox/--rw. Landlock cannot overlay a path with emptiness, so [hide] paths are no longer masked; they are denied but stay visible, and each one is logged so the weaker guarantee is never silent. Write paths ending in "/" are created first, because a rule needs an existing path to open. - core/config/TailPhobos.cfg: namespace flags are gone, only --chdir remains. - The AppArmor profile, the seccomp profile and their deploy script are removed: nothing on the host has to be prepared any more. Verified in an ordinary container started without --privileged, --cap-add or --security-opt: denied paths stay denied for reads and writes, allowed paths keep working, the restriction is inherited by child processes and a second JVM, it holds for root and non-root alike, and the timeout still terminates a JVM that blocks SIGTERM with a shutdown hook. In the same container "unshare -Ur" fails, so bubblewrap could not have run there at all.
This was referenced Sep 8, 2026
MarkusPaulsen
pushed a commit
that referenced
this pull request
Sep 8, 2026
Two corrections to the previous commit, both found by comparing against the Ares2 originals rather than by reading my own version again. Section 4 had lost its fill-in scaffold. Ares2 ends the section with Prerequisites, Steps, Expected result, Negative case and the modes exercised, so an author is asked for the negative case in the body of the pull request, where a reviewer looks for it. My version dropped all of it and then reintroduced the same idea as a checklist tick, which is weaker: a tick claims the case was covered, the scaffold makes the author write down what it was. The scaffold is back, with the four Ares enforcement combinations replaced by the three layers this project composes, each of which can be disabled on its own. Section 4 had also lost both recurring meta-rules, its character limit and the "simple words" paragraph, even though the template states in its own header that they are repeated in every section where they apply. The counts now match Ares2 exactly: seven limits, six "simple words" paragraphs, eleven "always required" statements. The template check itself is adopted, which is what makes the template binding rather than advisory: - .github/scripts/CheckPullRequestTemplate.java differs from the Ares2 version in a single line, the escape phrase for section 5, because this repository has no production Java code to report coverage for and answers that section with "No behaviour covered by the suites changed". - .github/workflows/pullrequest-template.yml is taken over unchanged. It runs the checker in single-file source mode, so there is no build step, exempts Renovate and Dependabot at step level so a required check never hangs pending, and passes the pull request body through the environment rather than interpolating it into the shell, since a fork author controls it verbatim. Running the checker locally against the open pull requests: #11 and #12 pass, #8, #9 and #10 do not yet, and their descriptions are being brought into shape separately.
added 2 commits
September 8, 2026 16:57
…osed Three findings from comparing the two gcc lines against each other and against what they actually produce. The netblocker library was built without any optimisation level. On this toolchain that is what decides whether _FORTIFY_SOURCE is set at all: Ubuntu's gcc turns it on at level 3 as soon as optimisation is requested, and off entirely otherwise. The library that sits on every connection a submission makes was therefore the one artefact built without it. It now gets the same -O2 -Wall -Wextra as the other. -static overrides the -fPIE default and produces a fixed-address executable, so the binary that applies the sandbox was the one part of the image with no address space randomisation. -static-pie produces a position-independent one, costs 480 bytes, and the acceptance suites pass unchanged. Both are now linked with -Wl,-z,now, which completes RELRO. Measured with readelf before and after rather than assumed: phobos-landlock EXEC, partial RELRO -> DYN, full RELRO libnetblocker.so no FORTIFY -> FORTIFY at level 3 Stack canaries and stack-clash protection were already in place; they come from Ubuntu's gcc defaults and need no flags. Separately, the legacy wrapper treated an unenforceable hide rule as a warning while phobos-filesystem.sh aborts on it. That gap matters most for the restricted command paths it also handles: those live under /usr, which the base policies allow as a read-only tree, so Landlock cannot except them and the rule can never hold. Warning and continuing there states a guarantee that is not given. Both entry points now refuse the policy instead.
…-of-bubblewrap # Conflicts: # docker/run_phase/java/docker-compose.yaml
MarkusPaulsen
force-pushed
the
feat/landlock-instead-of-bubblewrap
branch
from
September 8, 2026 14:57
234a2b7 to
b6782e5
Compare
added 10 commits
September 8, 2026 17:13
main was 190 lines and did everything in sequence: parse, ask the kernel what it can enforce, build the ruleset, add path rules, add port rules, change directory, restrict, exec. It now names those stages and does nothing else, in 14 lines. Every stage is one function, and the two loop bodies that were the longest passages inside main became functions of their own, so add_path_rule handles exactly one path and add_port_rule exactly one port. The command line ends up in a struct rather than in eight locals, which is why each stage takes one argument. It lives in static storage now: at 4096 rules it is too large to keep on the stack for no reason. No behaviour change is intended. The ordering that matters is unchanged and still explicit: chdir before the restriction, because the working directory may sit outside the allow-list while the paths reached from it are inside it, and no_new_privs before landlock_restrict_self, because Landlock requires it. Nothing about this refactoring could have been caught by the existing suites, because coverage showed that --connect-tcp, --bind-tcp and --min-abi were never executed by any of them. That is now fixed rather than noted: the extra suite starts two loopback servers and asserts that an allowed port is reachable while another one is not, and checks that a --min-abi above the kernel's aborts, that one below it runs, and that an unknown option is refused. coverage 64.60% of lines -> 79.79%, 58.62% of branches -> 71.30% suites 11, 10, 13 -> 11, 15, 13, and 58 and 20 unchanged Builds with -Werror -fanalyzer, clean under cppcheck.
The acceptance suites run against a real kernel, which is what shows the sandbox works, but they cannot reach the paths that decide whether it fails closed: a kernel that refuses a rule, an ABI older than this machine has, an exec that fails. Coverage put a number on that gap at 64.60% of lines and 58.62% of branches. tests/unit/ closes it. The source is included rather than linked so the static functions are reachable, main is renamed so the file can provide its own, and every syscall is interposed through the linker's --wrap, so a failure can be injected without a special kernel or any container flags. 56 cases now cover the rights tables at every ABI from 1 to 8, all five ways to be refused before anything is applied, the three table limits, the ABI gate in both directions, each of the ten syscalls failing, and the ten calls that go all the way through. lines 64.60% -> 100.00% of 191 branches 58.62% -> 100.00% of 104 Two findings came out of chasing the last few branches, and they are worth more than the number. A test was passing without exercising what it claimed to. The case for "the path cannot be opened" set a mock and got the expected exit code, but from a different failure: coverage showed the branch was never taken. It now names a path that genuinely does not exist and no longer depends on a mock. The condition "flag[3] == 'w' || flag[4] == 'w'" was dead in its second half. The flags are --ro, --rox, --rw and --rwx, so position 4 is either x or the end of the string and can never be w. Removed rather than covered. The measurement itself needed two corrections before it could be believed. A first attempt dumped coverage before the restriction was applied, so everything after it looked unexecuted. A second used longjmp instead of fork, which loses counters because it leaves the control flow gcov updates them on. Both showed higher or lower numbers than the truth. tests/unit/run.sh builds and runs the suite, and prints the coverage summary with --coverage. It needs gcc and nothing else.
cppcheck runs over every .c file in the repository, including the new test file, and flags an ignored return value. Silencing the child's stderr is not optional here: if it fails, the case would be judged on output that was never suppressed. It now exits with a distinct status instead. Found by the CI rather than locally, because I ran the acceptance suites and shellcheck before pushing but not the C gates over the new file.
The file was readable only to someone who already knew it. LL_FS_MAKE_REG, LL_DIR_ONLY, attr, fd, abi, opts, and single-letter loop counters all asked the reader to hold a glossary in their head. The access-right constants get the names the kernel itself uses: LANDLOCK_ACCESS_FS_MAKE_REG rather than LL_FS_MAKE_REG. That is both the spelled-out form and the one the Landlock documentation uses, so the code and the manual page can now be read side by side. Everything else is spelled out plainly. A few of the renames: LL_DIR_ONLY DIRECTORY_ONLY_ACCESS_RIGHTS MAX_RULES MAXIMUM_PATH_RULES EXIT_POLICY EXIT_CODE_POLICY_ERROR fs_rights_for_abi filesystem_rights_for_version grant_for rights_granted_for vlog log_verbose fail / fail_msg exit_with_system_error / exit_with_message usage print_usage_and_exit abi landlock_version attr / beneath attributes / path_rule_attributes fd / ruleset_fd descriptor / ruleset_descriptor st file_status rule->write / ->exec rule->writable / rule->executable r, p, i rule_index, port_index, argument_index argc, argv argument_count, arguments "ABI" is gone as well. The kernel calls these numbers ABI versions, short for application binary interface, and they are versions and nothing else, so the file calls them versions. The header says once that the kernel documentation spells it ABI, so searching for that term still leads here. The command line option follows: --min-abi is now --minimum-landlock-version. The usage text spells out its placeholders too: PATH, PORT, DIRECTORY, NUMBER, COMMAND and ARGUMENTS rather than P, N, D, CMD and ARGS. Reformatted to a 96 column limit afterwards, because the longer names pushed several lines well past it. No behaviour change. All suites pass unchanged (11, 15, 13, 58, 20), the unit tests still cover 100% of lines and branches, and gcc with -Werror -fanalyzer and cppcheck are clean.
…iven The names now say what they mean, including the ones taken from the kernel headers. The header carries a mapping table, so the kernel documentation stays searchable from this file: LANDLOCK_ACCESS_FILESYSTEM_* against the kernel's LANDLOCK_ACCESS_FS_*, MAKE_SOCKET against MAKE_SOCK, READ_DIRECTORY against READ_DIR, struct landlock_ruleset_attributes against landlock_ruleset_attr, SYSCALL_NUMBER_LANDLOCK_* against __NR_landlock_*. TCP keeps its abbreviation, because that is the name of the protocol. Alongside that, three input problems, and one of them mattered. atoi answers 0 for anything that is not a number, so "--minimum-landlock-version abc" silently asked for version 0 rather than being refused. In a tool whose whole point is to fail closed, a typo made the policy weaker without saying a word. strtoull had the same hole for ports, and no range check either, so "--connect-tcp 99999" reached the kernel as a port that cannot exist. Both now go through one function that rejects an empty value, a value with anything after the digits, an overflow, and anything outside the range the option can mean: 1 to 65535 for a port, and a Landlock version this build actually knows. The third is smaller: flag_name[strlen(flag_name) - 1] reads index -1 if the flag is ever the empty string. All four flags are longer than that today, so this is a guard against a future edit rather than a live defect, and it says so. strcmp stays. It reads rather than writes, and its arguments come from argv, which the kernel guarantees to be null-terminated. The dangerous relatives (strcpy, strcat, sprintf) do not appear in this file at all. One test had to change with the behaviour: a demanded version of 99 is now refused while the arguments are read, so the comparison against the kernel's version is reached with a demand of 5 against a kernel reporting 3. Nine new cases cover the rejected values. Coverage stays at 100% of lines and branches, now over 210 lines and 116 branches, with 65 cases. All suites pass unchanged, gcc with -Werror -fanalyzer and cppcheck are clean.
Preparation for the move to C++. The file had grown to 522 lines and held five
structures with the functions belonging to each of them mixed in between. Each
structure that will become a class now has its own pair of files, so that the
later step is a translation rather than a rewrite.
phobos-landlock-path-rule struct path_rule and the two questions asked of
it: which rights it grants at a given version,
and how it has to be opened. -> class PathRule
phobos-landlock-options struct options and the reading of the command
line into it. -> class Options
phobos-landlock-ruleset the kernel object: create it, add a rule, apply
it. -> class Ruleset
phobos-landlock-diagnostics reporting and giving up. -> free functions
phobos-landlock.c the stages and nothing else, 97 lines.
The three landlock_*_attributes structures stay raw data inside the ruleset
module. Their layout is fixed by the kernel and they exist only to be passed
to a system call, so making classes of them would force a second, raw
structure alongside each one.
One boundary is drawn tighter than before: the ruleset no longer sees the
options at all. It takes one path rule, one port, one minimum version, so the
later Ruleset::addPathRule(const PathRule&) needs no knowledge of how the
command line was written. The loops over the options moved to the stage
sequence, where they belong.
The split also removed a workaround. The unit tests used to include the whole
source file to reach its static functions; now they link the modules and
include only the stage sequence, for its main.
No behaviour change. All suites pass unchanged (11, 15, 13, 58, 20), the 65
unit tests still cover 100% of lines and branches, now measured per module,
and gcc with -Werror -fanalyzer, cppcheck and shellcheck are clean.
The suite had every line and every branch covered, yet a mutation run killed only 69 percent of the faults it introduced. The reason was one gap: the wrapped syscall read the attribute pointer as an unsigned and therefore never looked at it. The tests proved that the calls happened and never what they carried, which is the whole of the policy. A file could be given the rights only a directory may hold, a port rule could be dropped, the rule loop could be cut to its first turn, and every case stayed green. The mock now reads each call's arguments in the branch that knows their types and records them in shared memory, so the parent can judge what the forked child handed over. On top of that the cases that sit exactly on a limit: the highest port, a kernel exactly at the demanded version, version 4 with network rules, descriptor 0 from both the ruleset and an opened path, and a rule table filled to the brim. One case turned out to be green for the wrong reason. With the table guard moved by one, "one rule more than the table holds" still exited 125, because the run wrote past the table and failed later at something else. It now also asserts that a refused table hands no rule to the kernel at all. tests/unit/mutation.sh reproduces the measurement. It mounts the repository read-only and works on a copy, so a run leaves nothing behind. Mutation score 69 to 87 percent, 66 survivors to 26. The rest are assertions about message text and logging rather than about behaviour.
Splitting the wrapper into one file per structure left the documented build context copying phobos-landlock.c alone. The Dockerfile compiles phobos-landlock*.c, so following the README verbatim fails on the missing modules and headers.
The remaining mutants all changed a message and nothing else: a warning that appears one version too early, a refusal that names the wrong reason, a verbose line that vanishes. The child's diagnostics went to /dev/null, so no case could tell. The message is what a person reads when a policy is turned down, and it is the only thing that separates "this kernel has no Landlock" from "this kernel is too old", so it is worth as much as the exit code. expect_exit now sends the child's stderr to a temporary file and reads it back once the child has been waited for. Every refusal is now judged by what it names, a run that goes through has to say nothing at all, and --verbose has to report the version and each rule it applied. The wrapped close records which descriptors were handed back. One assertion was too weak, and the mutation run said so. Requiring only that the empty value is reported left the abort behind it removable: the value then falls through to the conversion, which reports the same input a second time as something else. The case now also requires that second message to be absent. Mutation score 87 to 98 percent, 26 survivors to 4. Of those four, three are an artefact of the measurement: removing exit() from a _Noreturn function is undefined behaviour that clang has already assumed away, and the suite kills all three under gcc. The fourth, verbose = 1 mutated to another non-zero value, is genuinely equivalent.
The run-phase image is the artefact: it is where both C products are compiled and what an exercise runs in. Nothing built it, so the acceptance suites had only ever run by hand, and the claim they exist to make, that the sandbox works in an ordinary container, rested on someone having run them. The image is built and exercised in one job. Loading an image populates one runner's Docker daemon and no other, so a separate acceptance job would quietly build a second image and test that instead. Three things had to be repaired before any of it could run. The unit runner built its instrumented copy in a directory it removed on the way out, so asking for coverage and then looking for the data found nothing; --coverage now takes a directory and keeps it. The run-phase compose file pointed at a context holding no Dockerfile and no file its COPY lines match. And the python prune image could not build at all: Ubuntu 26.04 ships a PEP 668 Python and pip stops with externally-managed-environment. The acceptance suites now run with no network at all. The phase test compiled online, because the probe POM named no compiler plugin version and Maven's default is 3.13.0 while the image carries only 3.14.0; naming it lets every Maven call run offline. A network failure can no longer arrive dressed as a Landlock regression. Nothing is mounted over /root/.m2 either. The base image ships a populated repository and a cache volume mounted there hides it, which makes the unrestricted clean steps fail for want of a plugin. The build context is assembled by one script that both CI and the acceptance README call, so the recipe cannot drift from the Dockerfile again. It refuses a destination it did not create and clears one it did. The weekly run builds with no cache and pulls its base images, since a cache hit is exactly what would hide the drift it exists to find.
14 tasks
The Build workflow never started. This organisation allows only actions it owns and actions GitHub publishes, so docker/setup-buildx-action and docker/build-push-action were refused before any job ran, whatever their pinned commit. Every workflow already here uses actions/* alone, which is why nothing had run into this before. Plain docker build does the same work: buildx is on the runner, --pull and --no-cache carry the weekly freshness the cache backend would have undermined anyway, and the run-phase image still lands in this runner's own daemon, so the suites still exercise the image the job just built. The matrix values reach the shell through the environment rather than by interpolation, which is the shape to avoid having anywhere, safe values or not. Lint and the template check also filter their pull_request trigger on main, so neither ran on a pull request stacked on the feature branch. Both now name that branch too, with the same note to remove it once the branch has landed.
The unit suite wraps open and close through the linker, which is how it injects syscall failures. The gcov runtime writes its .gcda files with those same calls, so an instrumented run has gcov failing to write and reporting so on stderr. That output breaks the test asserting that a run which goes through says nothing, and it means any figure would have come from a perturbed run. It passed locally and failed in CI, which is the worst shape for a check to have. Removed rather than silenced: tests/unit/mutation.sh already measures this suite, and it measures the thing coverage only gestures at, namely whether a test would notice the code being wrong. The --coverage directory argument stays. It fixes a real defect, that the instrumented build was deleted before anything could read it, and it is still what someone investigating locally would reach for.
The acceptance suite's environment header read the ABI out of the wrapper's --verbose output by matching on "Landlock ABI", which the wrapper has never printed: it names the number "Landlock version". The sed therefore matched nothing and the line rendered blank. Nothing asserts on the header, so no check ever failed over it. That is precisely why it went unnoticed, and it matters more now that CI runs this suite: a reviewer reading the log finds a blank where the kernel's ABI version should be.
Build the run-phase image in CI and hold that exact image to the suites
This was referenced Sep 11, 2026
added 2 commits
September 12, 2026 21:15
The sources were built with no -std at all, so the language was whatever the compiler happened to default to. Measured in the run-phase image: it is Ubuntu 24.04.3 with gcc 13.3, defaulting to C17, and gcc changed that default between its own versions 14 and 15. A base image update could therefore have changed the language the sandbox is built in without anyone touching a line. gcc 13 does not know -std=gnu23 at all; it answers "unrecognized command-line option" and suggests the draft name. So gcc-14 is installed rather than assumed, and the same -std=gnu23 now appears at all seven places that compile C: both artefacts in the Dockerfile, the lint workflow, the two builds in the unit runner, the two in the network suite, and the mutation run under clang-18. The unit runner names gcov-14 as well, because coverage notes can only be read by the gcov that matches the compiler that wrote them, and the build workflow installs gcc-14 rather than relying on the runner's default. cppcheck runs with --std=c23, which the installed 2.13.0 accepts. The Dockerfile also stops claiming its hardening and starts proving it. The comments explained why -static-pie replaced -static and why -Wl,-z,now was added, but a comment cannot notice a later edit removing them. A readelf check now fails the build unless the wrapper is DYN and both artefacts carry BIND_NOW. No behaviour change: the existing sources are valid C23 and every suite passes unchanged.
… needs A path rule carried two flags, writable and executable, and "writable" meant thirteen kernel rights at once: write, truncate, rename, delete, and create files, directories, sockets, pipes, symbolic links, character devices and block devices. A policy could not ask for less. Granting a build directory the right to make device nodes is not a decision anyone took; it is one nobody could avoid. struct path_rule now holds six independent fields, and --rights=LETTERS PATH replaces --ro, --rox, --rw and --rwx: r read, w write, x execute, m create, d delete, i ioctl on a device. Letters are validated, an unknown one or a repeated one is refused rather than absorbed, since a repetition in a policy is a typo far more often than an intention. The old flags are removed outright rather than kept as aliases: reusing --rw with a narrower meaning would have been the one change that fails quietly. Creating device nodes and symbolic links is now granted by no combination at all. They stay in the handled set, so they are denied rather than unregulated. REFER, which is what lets a file move between two directories, is granted only where both creating and deleting are, because that is what moving is. ioctl gets its own letter and no section default hands it out. Landlock unions the rights of every rule along a path: a nested rule can only add, never take away. Measured on Landlock 8 with a static ELF, since a shell script only proves that its interpreter was executable: ancestor rx, child r -> still executes ancestor r, child rx -> executes ancestor r, no child -> refused So a policy declaring fewer rights on a nested path states a restriction that will not hold, and phobos-common.sh now refuses it with PHB-EPOLICY, the same way an unenforceable [hide] entry is already refused. A policy declaring merely different rights is normal and stays allowed, but the inherited widening is reported and the effective union is what reaches the kernel, so the verbose output cannot disagree with what is enforced. Paths are compared after symlink resolution, because /bin is a symlink to /usr/bin in the run-phase image and a textual comparison sees two trees where the kernel sees one. A kernel too old to handle a right is a gap Landlock offers no hook to close, so it is reported unconditionally instead of silently tolerated: below version 3 a file on a read-only path can still be emptied with truncate(2), which the suites confirm, and below version 5 ioctl on a device is unrestricted. phobos_wrapper.sh now sources phobos-common.sh, so both entry points share one translation rather than keeping two that already drifted apart once. 134 unit tests to 177, still 100% of lines and branches; acceptance suites 11, 15 to 24, and 13; timeout and network suites unchanged at 58 and 20.
added 3 commits
September 12, 2026 21:18
The [network] section reached libnetblocker and nothing else. That library is injected with LD_PRELOAD, which is a request to the dynamic loader rather than a boundary: a statically linked program, a direct system call, or simply clearing the variable before a child walks straight past it. The wrapper meanwhile had --connect-tcp and --bind-tcp options that no policy file could ever reach, so the one mechanism that cannot be stepped around sat unused. Both entry points now translate the section through the same function. A rule naming a real port becomes --connect-tcp, duplicates are folded, and the kernel denies every other connection. libnetblocker stays and is still the only layer that can judge a hostname or an address, which Landlock cannot express at all; the two are not alternatives. Landlock filters by port and knows nothing else, so a rule without a port cannot be translated. The three shipped policies are exactly that case: 127.0.0.1:*, ::1:* and localhost. A section made only of such rules therefore leaves the Landlock network layer off and says so on stderr rather than quietly. A section mixing a wildcard with a concrete port is refused with PHB-EPOLICY instead, because the explicit port would read as enforced and would not be. A port that is not a number, or is outside 1..65535, is a policy error and no longer treated as a wildcard. The ruleset now handles only the directions the policy speaks about. Handling both because one was mentioned would have denied the other outright: a policy naming where a build may connect would have stopped it from listening at all, breaking any exercise whose tests start a local server. That was not a decision anyone took, and the two new cases assert it does not happen. Kernel 6.7 is required only by a policy that names concrete ports, so nothing else changes its reach. 177 unit tests to 183, still 100% of lines and branches; every suite unchanged.
Three files conflicted, and two of them were decided by main: .gitignore, whose commented version is a superset of the terse block on this branch, and the python prune image, where the difference was a pytest and ruff bump. The run-phase image needed judging rather than choosing a side. main pins the base image by digest, which this branch keeps. main also still installs bubblewrap and compiles the preload library without the hardening flags, and neither may come back: removing bubblewrap is what this branch is for, and the flags were added here deliberately. So the result is this branch's build with main's digest. The readelf check that proves the hardening was rewritten to use files instead of pipes. hadolint fails at warning level, and a pipe in a RUN without pipefail is DL4006, so the check as written would have broken CI the moment this merge landed. main brought two new C sources, and the lint gate on this branch is stricter than the one they were written against: gcc-14 with -std=gnu23, -Werror and -fanalyzer, plus cppcheck at --std=c23. Both pass unchanged, as does the rewritten netblocker.c. Verified against the merged tree: 183 unit tests, acceptance suites 11, 24 and 13, timeout 58, network 20, shellcheck clean over all 20 shell files, and the run-phase image builds.
AGENTS.md landed on main while this branch was being written, and the translation added here broke three of its rules. They are worth following rather than arguing with, and one of them found a real problem. "No comments inside a function body. A function that needs one is a function that should be two." build_path_args carried seven comment blocks, which is the rule saying plainly that it did seven things. It is now a sequence of named steps in the same shape phobos-landlock.c already uses: collect_rights_table, fold_table_by_target, report_folded_widenings, resolve_rights_hierarchy, emit_rights_arguments, with refuse_dangling_symlink and materialise_write_path beside them. build_network_args is split the same way, into collect_network_ports and refuse_unusable_port. Splitting build_network_args surfaced the trap that had already been fixed twice in this branch: report() prints on stdout, so a helper returning data through stdout would swallow its own refusal. collect_network_ports therefore returns both of its results through named files. "One variable or function declaration per line, in every language." Eleven local lines declared several at once. "Every function in core/*.sh says what it does and what it assumes about its environment." The new helpers now say both. No behaviour change, and the three direct tests of the translation produce byte-identical output: the alias case, the hierarchy cases including the one through a symlink, the dangling symlink, and every network classification. 183 unit tests, acceptance 11, 24 and 13, timeout 58, network 20. The readelf check in the Dockerfile also moves here in spirit: hadolint rejects a pipe in RUN without pipefail at warning level, which is this repository's failure threshold, so the check writes files and greps them instead.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The filesystem sandbox is enforced by Landlock instead of bubblewrap, so it runs in an ordinary build container with nothing prepared on the host. Access is named right by right rather than as two coarse groups, so a policy can permit writing without also permitting the creation of device nodes. A policy that names real TCP ports is enforced by the kernel and not only by a library a submission can step around.
Linked issues
Relates to ls1intum/artemis-ansible-collection#162, the closed pull request that would have rolled the AppArmor profile out to every build host.
1. Problem
Three things, all in the filesystem sandbox, the part that decides which paths a submission may reach.
Bubblewrap needs an unprivileged user namespace, and a stock container denies that three times over: the default seccomp profile blocks
unshare,mountandpivot_root, thedocker-defaultAppArmor profile carries a blanketdeny mount, and Ubuntu 23.10 and later want an explicitusernsrule. The AppArmor half cannot be supplied from inside the container at all, so the sandbox could never be switched on by the build alone. In the container used for this workunshare -Urfails, so bubblewrap could not have run there.A policy could say only "read" or "write", and "write" meant thirteen kernel rights at once, creating device nodes and symbolic links among them. Nobody chose that; nobody could avoid it.
The
[network]section reached only a preload library, which is a request to the dynamic loader rather than a boundary.2. Improvement from the user's perspective
An instructor no longer depends on an administrator having installed a profile on the build agent before an exercise can be sandboxed. The sandbox works in an ordinary container, started with a plain
docker run.A policy can now be narrower than before. Writing into a file, creating one, and deleting one are separate permissions, so a directory that only has to be written into no longer also gains the right to create device nodes and symbolic links in it. Those two are no longer granted at all.
Where a policy names real TCP port numbers, the kernel enforces them, so a submission cannot reach an endpoint by avoiding the preload library.
Where something cannot be enforced, Phobos says so rather than appearing to hold. A policy whose nested entry claims fewer rights than its parent grants is refused, a kernel too old to restrict truncation is named on standard error, and a network section that cannot be expressed switches the kernel layer off out loud.
3. Improvement from the maintainer's perspective
943 lines of host security configuration and its deployment script are gone, with the class of problem they brought: a profile that has to match across a fleet, hosts drifting apart, and a container that fails to start when its profile is absent.
Both entry points now share one translation from a parsed policy into wrapper arguments. They had already drifted apart once.
The language the C is built in is pinned rather than inherited from the base image, and the hardening the Dockerfile claims is proved by
readelfduring the build instead of asserted in a comment.The wrapper is covered at 100% of lines and branches by a suite needing one compiler and nothing else, with every system call interposed through the linker.
README.mdstill describes bubblewrap and is deliberately not rewritten here: it narrates the whole sandbox, and that belongs with the configuration format change still to come. The documentation box is therefore left unticked.4. Testing manual
Prerequisites
Steps
Build the run-phase image from the repository root. The context is assembled by the script both CI and this manual call, so the recipe cannot drift from the Dockerfile.
Expected result: the build succeeds. It contains a
readelfstep that fails the build unless the wrapper is position independent and both compiled artefacts carryBIND_NOW, so a successful build is itself the evidence that the hardening is in place.Run the main acceptance suite. Note the plain invocation: no
--privileged, no--cap-add, no--security-opt.Expected result: the last line reads
bestanden: 11, fehlgeschlagen: 0. Read the lines above it rather than the exit status: each names a path or an endpoint and says whether it was permitted or denied, and both directions appear.Run the additional suite the same way, replacing
run-tests.shwithextra-tests.sh.Expected result:
bestanden: 24, fehlgeschlagen: 0. Section F shows each right being granted on its own:wwrites an existing file but cannot create one,mcreates but cannot delete,ddeletes. Section G shows that creating a device node and creating a symbolic link are refused while creating an ordinary file still works. Section C prints thatunshare -Urfails in this very container, which is the point: the mechanism this replaces could not have run here.Run the phase suite the same way, replacing the script with
phase-test.sh.Expected result:
bestanden: 13, fehlgeschlagen: 0. It tightens and widens the rights across four phases in one container and checks that a child process cannot shed the restriction.Run the three suites that need no image, from a checkout with
gcc-14installed.Expected result:
183 passed, 0 failed, then58 passed, then20 passed. The first asserts what the wrapper hands to the kernel, not only that it ran.Negative case (what must still be rejected)
A path outside the allow-list stays unreadable and unwritable, and a write into a read-only tree is refused. Both hold for a child process started with
ProcessBuilder, for a second JVM launched from inside the sandbox, and as root and as a non-root user alike. A control probe in section E reads the same file without the sandbox, so the denial is shown to come from Landlock and not from file permissions. Creating a device node or a symbolic link in a writable directory is refused. A policy whose nested entry claims fewer rights than an enclosing entry grants is refused withPHB-EPOLICYrather than run. The timeout still terminates a JVM that blocksSIGTERMwith a shutdown hook.Layers exercised
5. Test case coverage regarding this PR
tests/landlock-acceptance/run-tests.shtests/landlock-acceptance/extra-tests.shtests/landlock-acceptance/phase-test.shtests/unit/run.shtests/timeout_units.shtests/network_cache_ports.shRun on
ls1tum/artemis-maven-template:java17-25, kernel 7.0.12-linuxkit, aarch64, Landlock version 8, started with a plaindocker run --rmand no--privileged,--cap-addor--security-opt.Breaking changes and migration
[hide]changes meaning. Landlock withholds access but cannot overlay a path with emptiness, so a hidden path with no allowed ancestor is denied yet stays visible by name, which is weaker than the tmpfs mask, and it is logged on every run. A hidden path beneath an allowed tree cannot be excepted at all, so that policy is refused withPHB-EPOLICY; list the permitted siblings instead.The wrapper's path flags
--ro,--rox,--rwand--rwxare replaced by--rights=LETTERS PATH. A caller that invokes the wrapper directly has to be changed; a policy file does not.Creating device nodes and symbolic links is no longer granted anywhere.
A policy that names concrete TCP ports requires Landlock version 4, that is kernel 6.7. Every other policy runs as before.
The host no longer needs the AppArmor profile, and containers must no longer be started with
--security-opt apparmor=docker-bwrap.Checklist
--privileged,--cap-addor--security-opt.README.md, the comments incore/) was updated where the change is user-facing.Review progress