From b6d5e3f05aab6a369ddcbd005cc571dc22809a46 Mon Sep 17 00:00:00 2001 From: Raito Bezarius Date: Fri, 2 Jan 2026 21:03:32 +0100 Subject: [PATCH] libstore/platform/linux: allow xattrs in the sandbox Fixes #838. xattrs were historically disabled in 2017, since then, NFS v4.2 gained support for xattrs and almost all filesystems support xattrs. In addition, moving the build directory to a xattrs-supporting filesystem is always an option. Programs may exploit xattrs (including ACLs) during their build process and testing phase, to better serve these programs, we will remove this limitation. Lix will use POSIX ACLs for its UNIX domain sockets in the future and be one of these programs which will run tests making use of xattrs, while not writing any xattrs in the derivation outputs themselves. xattrs are still scrubbed from derivation outputs so it is not possible to obtain an advantage by writing a security-related xattr to a well chosen file. Tests were added to test these scenarios on Linux. Darwin is carefully excluded because of #1008 and #1090, solving that is welcome. In the meantime, they are marked xfail. Change-Id: Ia3255eeb8442e83db4f10dcb5a51cbc368a2550d Signed-off-by: Raito Bezarius --- lix/libstore/platform/linux.cc | 14 +- package.nix | 29 ++++- tests/functional2/build/assets/xattrs.nix | 102 +++++++++++++++ tests/functional2/build/test_xattrs.py | 46 +++++++ tests/functional2/store/test_xattrs.py | 123 ++++++++++++++++++ .../functional2/testlib/fixtures/test_env.py | 3 +- .../testlib/global_assets/config.nix.template | 3 + .../dependencies/dependencies.nix | 2 +- tests/functional2/testlib/xattrs.py | 53 ++++++++ 9 files changed, 359 insertions(+), 16 deletions(-) create mode 100644 tests/functional2/build/assets/xattrs.nix create mode 100644 tests/functional2/build/test_xattrs.py create mode 100644 tests/functional2/store/test_xattrs.py create mode 100644 tests/functional2/testlib/xattrs.py diff --git a/lix/libstore/platform/linux.cc b/lix/libstore/platform/linux.cc index b072e77d3..7822502de 100644 --- a/lix/libstore/platform/linux.cc +++ b/lix/libstore/platform/linux.cc @@ -223,7 +223,6 @@ static std::vector compileSyscallFilter() // Run check-syscalls to determine which new syscalls should be added. // New syscalls must be audited and handled in a way that blocks the following dangerous operations: // * Creation of non-empty setuid/setgid files - // * Creation of extended attributes (including ACLs) // // BEGIN extract-syscalls allowSyscall(ctx, SCMP_SYS(accept)); @@ -313,7 +312,7 @@ static std::vector compileSyscallFilter() allowSyscall(ctx, SCMP_SYS(fork)); allowSyscall(ctx, SCMP_SYS(fremovexattr)); allowSyscall(ctx, SCMP_SYS(fsconfig)); - // skip fsetxattr (dangerous) + allowSyscall(ctx, SCMP_SYS(fsetxattr)); allowSyscall(ctx, SCMP_SYS(fsmount)); allowSyscall(ctx, SCMP_SYS(fsopen)); allowSyscall(ctx, SCMP_SYS(fspick)); @@ -417,7 +416,7 @@ static std::vector compileSyscallFilter() allowSyscall(ctx, SCMP_SYS(lookup_dcookie)); allowSyscall(ctx, SCMP_SYS(lremovexattr)); allowSyscall(ctx, SCMP_SYS(lseek)); - // skip lsetxattr (dangerous) + allowSyscall(ctx, SCMP_SYS(lsetxattr)); allowSyscall(ctx, SCMP_SYS(lstat)); allowSyscall(ctx, SCMP_SYS(lstat64)); allowSyscall(ctx, SCMP_SYS(madvise)); @@ -615,7 +614,7 @@ static std::vector compileSyscallFilter() allowSyscall(ctx, SCMP_SYS(set_tls)); allowSyscall(ctx, SCMP_SYS(setuid)); allowSyscall(ctx, SCMP_SYS(setuid32)); - // skip setxattr (dangerous) + allowSyscall(ctx, SCMP_SYS(setxattr)); allowSyscall(ctx, SCMP_SYS(sgetmask)); allowSyscall(ctx, SCMP_SYS(shmat)); allowSyscall(ctx, SCMP_SYS(shmctl)); @@ -722,13 +721,6 @@ static std::vector compileSyscallFilter() ALLOW_CHMOD_IF_SAFE(ctx, SCMP_SYS(fchmodat), 2); ALLOW_CHMOD_IF_SAFE(ctx, SCMP_SYS(fchmodat2), 2); - // setxattr family: prevent creation of extended attributes or ACLs. - // Not all filesystems support them, and they're incompatible with the NAR format. - if (seccomp_rule_add(ctx, SCMP_ACT_ERRNO(ENOTSUP), SCMP_SYS(setxattr), 0) != 0 || - seccomp_rule_add(ctx, SCMP_ACT_ERRNO(ENOTSUP), SCMP_SYS(lsetxattr), 0) != 0 || - seccomp_rule_add(ctx, SCMP_ACT_ERRNO(ENOTSUP), SCMP_SYS(fsetxattr), 0) != 0) - throw SysError("unable to add seccomp rule"); - Pipe filterPipe; filterPipe.create(); auto filterBytes_ = std::async([&]() { diff --git a/package.nix b/package.nix index 9e496984e..7b202b9d9 100644 --- a/package.nix +++ b/package.nix @@ -245,13 +245,19 @@ stdenv.mkDerivation (finalAttrs: { p.python-frontmatter p.pycapnp ] - ++ lib.optionals finalAttrs.doCheck [ + ++ lib.optionals finalAttrs.doCheck ([ (dontWrapPython p.pytest) p.pytest-xdist p.pytest-timeout p.ruff p.aiohttp - ] + + # Remove once https://github.com/NixOS/nixpkgs/pull/476848 lands here. + (p.pyxattr.overrideAttrs (old: { + buildInputs = lib.filter (lib.meta.availableOn stdenv.hostPlatform) old.buildInputs; + meta.platforms = old.meta.platforms ++ lib.platforms.darwin; + })) + ]) ); buildTestShell = @@ -262,7 +268,18 @@ stdenv.mkDerivation (finalAttrs: { else null; - buildTestEnv = if hostPlatform.isLinux then "${pkgsStatic.busybox}/bin" else null; + buildTestEnv = + if hostPlatform.isLinux then + lib.makeBinPath [ + pkgsStatic.busybox + pkgsStatic.acl + ] + else + lib.makeBinPath [ + pkgs.darwin.file_cmds + pkgs.darwin.shell_cmds + pkgs.darwin.text_cmds + ]; src = fileset.toSource { root = ./.; @@ -618,6 +635,12 @@ stdenv.mkDerivation (finalAttrs: { p.xdg-base-dirs p.packaging p.xonsh + + # Remove once https://github.com/NixOS/nixpkgs/pull/476848 lands here. + (p.pyxattr.overrideAttrs (old: { + buildInputs = lib.filter (lib.meta.availableOn stdenv.hostPlatform) old.buildInputs; + meta.platforms = old.meta.platforms ++ lib.platforms.darwin; + })) ] ); pythonEnv = python3.pythonOnBuildForHost.withPackages pythonPackages; diff --git a/tests/functional2/build/assets/xattrs.nix b/tests/functional2/build/assets/xattrs.nix new file mode 100644 index 000000000..c00ec49de --- /dev/null +++ b/tests/functional2/build/assets/xattrs.nix @@ -0,0 +1,102 @@ +with import ./config.nix; +let + setXattrLinux = inodeTgt: name: value: "setfattr -n ${name} -v \"${value}\" ${inodeTgt}"; + setExtendedAttribute = if isDarwin then setXattrDarwin else setXattrLinux; + setAndCheckACLLinux = inodeTgt: perm: '' + setfacl -m ${perm} ${inodeTgt} + getfacl -an ${inodeTgt} | grep -i "mask::rw." || { echo no mask in the ACL, setfacl ineffective?; exit 1; } + ''; + setAndCheckACL = if isDarwin then setAndCheckACLDarwin else setAndCheckACLLinux; + + wellKnownLinuxNames = { + "user.mime_type" = "text/html5"; + "user.deriver" = "yue"; + + # On Linux, inside of the sandbox, you cannot write to these because CAP_SYS_ADMIN is required. + # Those are protected xattrs. + # Root inside the root namespace can set those. + # "trusted.md5sum" = "..."; + # "security.selinux" = "..."; + }; + wellKnownACLs = [ + # sets xattr system.posix_acl_access on Linux. + "u:1000:rwX" + ]; + mapAttrsToList = fn: attrs: map (k: fn k attrs.${k}) (builtins.attrNames attrs); + + linuxScenariosForAnInode = withACL: inodeTgt: + builtins.concatStringsSep "\n" ( + (mapAttrsToList (name: value: setExtendedAttribute inodeTgt name value) wellKnownLinuxNames) + ++ (if withACL then map (setAndCheckACL inodeTgt) wellKnownACLs else [ ]) + ); + + setXattrDarwin = inodeTgt: name: value: "xattr -w ${name} \"${value}\" ${inodeTgt}"; + setAndCheckACLDarwin = inodeTgt: perm: '' + chmod +a "${perm}" "${inodeTgt}" + ls -el "${inodeTgt}" + ls -el "${inodeTgt}" | grep -Ei '${perm}' + ''; + # NOTE(Qyriad): Darwin ACLs have some kind of sorting order, so ideally we should parse them to check them. + # However: fuck that, for now. So we've just written them in The Order here so the text-match. + wellKnownACLsDarwin = [ + "group:everyone allow read,write" + "group:staff allow read,delete,writesecurity,chown" + ]; + darwinScenariosForAnInode = withACL: inodeTgt: + builtins.concatStringsSep "\n" ( + (mapAttrsToList (name: value: setXattrDarwin inodeTgt name value) wellKnownLinuxNames) + ++ (if withACL then map (setAndCheckACLDarwin inodeTgt) wellKnownACLsDarwin else [ ]) + ); + + scenariosForAnInode = if isDarwin then darwinScenariosForAnInode else linuxScenariosForAnInode; +in +{ + during-build = mkDerivation { + name = "xattrs-during-build"; + buildCommand = '' + touch work + ${scenariosForAnInode true "./work"} + echo meow > $out + ''; + }; + + in-root-outputs-file = mkDerivation { + name = "xattrs-on-root-output-file"; + buildCommand = '' + touch $out + + ${ + # We are not allowed to set ACLs on the root of the output directory. + # There's an explicit suspicious permission check that will cause this build to be rejected. + scenariosForAnInode false "$out" + } + ''; + }; + + in-root-outputs-dir = mkDerivation { + name = "xattrs-on-root-output-dir"; + buildCommand = '' + touch $out + + ${ + # We are not allowed to set ACLs on the root of the output directory. + # There's an explicit suspicious permission check that will cause this build to be rejected. + scenariosForAnInode false "$out" + } + ''; + }; + + in-output-content = mkDerivation { + name = "xattrs-under-directory-output"; + buildCommand = '' + mkdir -p $out + touch $out/test + mkdir -p $out/test2 $out/test3 + touch $out/test3/test4 + + ${scenariosForAnInode true "$out/test"} + ${scenariosForAnInode true "$out/test2"} + ${scenariosForAnInode true "$out/test3/test4"} + ''; + }; +} diff --git a/tests/functional2/build/test_xattrs.py b/tests/functional2/build/test_xattrs.py new file mode 100644 index 000000000..7143313ef --- /dev/null +++ b/tests/functional2/build/test_xattrs.py @@ -0,0 +1,46 @@ +import sys + +import pytest +from pathlib import Path +from functional2.testlib.fixtures.file_helper import with_files +from functional2.testlib.fixtures.nix import Nix +from functional2.testlib.utils import get_global_asset, CopyFile +from functional2.testlib.xattrs import verify_no_xattrs_in_tree, skip_if_xattrs_are_unsupported + + +@with_files( + {"config.nix": get_global_asset("config.nix"), "xattrs.nix": CopyFile("assets/xattrs.nix")} +) +def test_xattrs_during_build(nix: Nix): + skip_if_xattrs_are_unsupported(nix.env) + + nix.nix_build(["xattrs.nix", "-A", "during-build", "--no-out-link"]).run().ok() + + +def build_and_get_store_path(nix: Nix, asset: str, attribute: str) -> Path: + return nix.physical_store_path_for( + nix.nix_build([asset, "-A", attribute, "--no-out-link"]).run().ok().stdout_plain + ) + + +@with_files( + {"config.nix": get_global_asset("config.nix"), "xattrs.nix": CopyFile("assets/xattrs.nix")} +) +@pytest.mark.skipif( + sys.platform != "linux", reason="xattrs scrubbing is not expected to function outside of Linux" +) +def test_xattrs_in_output(nix: Nix): + skip_if_xattrs_are_unsupported(nix.env) + + # We assert that xattrs producing derivations in the outputs should complete with no xattrs in the final output path. + # NOTE: if another platform is added, another `verify_no_acl_in_tree` + # should be added to ensure that ACLs are truly removed. + # On Linux, this is not necessary. + output_path = build_and_get_store_path(nix, "xattrs.nix", "in-root-outputs-file") + verify_no_xattrs_in_tree(output_path) + + output_path = build_and_get_store_path(nix, "xattrs.nix", "in-root-outputs-dir") + verify_no_xattrs_in_tree(output_path) + + output_path = build_and_get_store_path(nix, "xattrs.nix", "in-output-content") + verify_no_xattrs_in_tree(output_path) diff --git a/tests/functional2/store/test_xattrs.py b/tests/functional2/store/test_xattrs.py new file mode 100644 index 000000000..de352b153 --- /dev/null +++ b/tests/functional2/store/test_xattrs.py @@ -0,0 +1,123 @@ +import pytest +import sys + +from functional2.testlib.fixtures.file_helper import with_files, File +from functional2.testlib.fixtures.nix import Nix +from functional2.testlib.xattrs import verify_no_xattrs_in_tree, skip_if_xattrs_are_unsupported + +import xattr +import json +import re + + +@with_files({"file-with-xattrs": File("hi")}) +@pytest.mark.parametrize("hash_fn", ["sha1", "sha256"]) +@pytest.mark.skipif( + sys.platform != "linux", reason="xattrs scrubbing is not expected to function outside of Linux" +) +def test_add_xattrs_fixed_flat_file(nix: Nix, hash_fn: str): + skip_if_xattrs_are_unsupported(nix.env) + + xattr.set(nix.env.dirs.home / "file-with-xattrs", "user.deriver", "test") + + res = nix.nix_store(["--add-fixed", hash_fn, "file-with-xattrs"]).run().ok() + verify_no_xattrs_in_tree(nix.physical_store_path_for(res.stdout_plain)) + + +@with_files({"file-with-xattrs": File("hi")}) +@pytest.mark.skipif( + sys.platform != "linux", reason="xattrs scrubbing is not expected to function outside of Linux" +) +def test_add_xattrs_flat_file(nix: Nix): + skip_if_xattrs_are_unsupported(nix.env) + + xattr.set(nix.env.dirs.home / "file-with-xattrs", "user.deriver", "test") + + res = nix.nix_store(["--add", "file-with-xattrs"]).run().ok() + verify_no_xattrs_in_tree(nix.physical_store_path_for(res.stdout_plain)) + + +@with_files({"dir-with-xattrs": {"test1": File("hi")}}) +@pytest.mark.parametrize("hash_fn", ["sha1", "sha256"]) +@pytest.mark.skipif( + sys.platform != "linux", reason="xattrs scrubbing is not expected to function outside of Linux" +) +def test_add_xattrs_directories(nix: Nix, hash_fn: str): + skip_if_xattrs_are_unsupported(nix.env) + + xattr.set(nix.env.dirs.home / "dir-with-xattrs", "user.deriver", "hi") + xattr.set(nix.env.dirs.home / "dir-with-xattrs/test1", "user.deriver", "hi") + + res = nix.nix_store(["--add-fixed", hash_fn, "--recursive", "dir-with-xattrs"]).run().ok() + verify_no_xattrs_in_tree(nix.physical_store_path_for(res.stdout_plain)) + + +@with_files({"file-with-xattrs": File("hi"), "dir-with-xattrs": {"test1": File("hi")}}) +@pytest.mark.skipif( + sys.platform != "linux", reason="xattrs scrubbing is not expected to function outside of Linux" +) +def test_interpolating_xattrs_file(nix: Nix): + skip_if_xattrs_are_unsupported(nix.env) + + xattr.set(nix.env.dirs.home / "file-with-xattrs", "user.deriver", "test") + xattr.set(nix.env.dirs.home / "dir-with-xattrs", "user.deriver", "hi") + xattr.set(nix.env.dirs.home / "dir-with-xattrs/test1", "user.deriver", "hi") + + res = nix.eval('"${./file-with-xattrs}"', flags=["--impure"]).ok() + store_path = json.loads(res.stdout_plain) + verify_no_xattrs_in_tree(nix.physical_store_path_for(store_path)) + + res = nix.eval('"${./dir-with-xattrs}"', flags=["--impure"]).ok() + store_path = json.loads(res.stdout_plain) + verify_no_xattrs_in_tree(nix.physical_store_path_for(store_path)) + + +@with_files({"file-with-xattrs": File("hi"), "dir-with-xattrs": {"test1": File("hi")}}) +@pytest.mark.skipif( + sys.platform != "linux", reason="xattrs scrubbing is not expected to function outside of Linux" +) +def test_builtins_path_xattrs(nix: Nix): + skip_if_xattrs_are_unsupported(nix.env) + + xattr.set(nix.env.dirs.home / "file-with-xattrs", "user.deriver", "test") + xattr.set(nix.env.dirs.home / "dir-with-xattrs", "user.deriver", "hi") + xattr.set(nix.env.dirs.home / "dir-with-xattrs/test1", "user.deriver", "hi") + + res = nix.eval( + 'builtins.path { name = "test-file"; path = ./file-with-xattrs; }', flags=["--impure"] + ).ok() + store_path = json.loads(res.stdout_plain) + verify_no_xattrs_in_tree(nix.physical_store_path_for(store_path)) + + res = nix.eval( + 'builtins.path { name = "test-file"; path = ./file-with-xattrs; recursive = true; }', + flags=["--impure"], + ).ok() + store_path = json.loads(res.stdout_plain) + verify_no_xattrs_in_tree(nix.physical_store_path_for(store_path)) + + res = nix.eval( + 'builtins.path { name = "test-dir"; path = ./dir-with-xattrs; recursive = true; }', + flags=["--impure"], + ).ok() + store_path = json.loads(res.stdout_plain) + verify_no_xattrs_in_tree(nix.physical_store_path_for(store_path)) + + +@with_files({"file-with-xattrs": File("hi"), "dir-with-xattrs": {"test1": File("hi")}}) +@pytest.mark.skipif( + sys.platform != "linux", reason="xattrs scrubbing is not expected to function outside of Linux" +) +def test_nix_prefetch_xattrs(nix: Nix): + skip_if_xattrs_are_unsupported(nix.env) + + xattr.set(nix.env.dirs.home / "file-with-xattrs", "user.deriver", "test") + xattr.set(nix.env.dirs.home / "dir-with-xattrs", "user.deriver", "hi") + xattr.set(nix.env.dirs.home / "dir-with-xattrs/test1", "user.deriver", "hi") + + locate_path_re = re.compile(r"path is '(.*)'") + for p in ("file-with-xattrs", "dir-with-xattrs"): + res = nix.nix_prefetch_url([f"file://{nix.env.dirs.home / p}"]).run().ok() + matches = locate_path_re.findall(res.stderr_plain) + assert matches, "nix prefetch must return at least one path" + verify_no_xattrs_in_tree(nix.physical_store_path_for(matches[0])) diff --git a/tests/functional2/testlib/fixtures/test_env.py b/tests/functional2/testlib/fixtures/test_env.py index c4576016c..9c7d80006 100644 --- a/tests/functional2/testlib/fixtures/test_env.py +++ b/tests/functional2/testlib/fixtures/test_env.py @@ -134,7 +134,8 @@ def test_env_to_env(tmp_path: Path): "PATH", "BUILD_TEST_SHELL", "TMPDIR", - } | ({"_NIX_TEST_NO_SANDBOX"} if sys.platform == "darwin" else {"BUILD_TEST_ENV"}) + "BUILD_TEST_ENV", + } | ({"_NIX_TEST_NO_SANDBOX"} if sys.platform == "darwin" else set()) def test_path_inits_build_shell(): diff --git a/tests/functional2/testlib/global_assets/config.nix.template b/tests/functional2/testlib/global_assets/config.nix.template index ad68f5507..21db5ef0d 100644 --- a/tests/functional2/testlib/global_assets/config.nix.template +++ b/tests/functional2/testlib/global_assets/config.nix.template @@ -12,6 +12,9 @@ rec { system = "@system@"; + isLinux = (builtins.match ".+-linux$" system) != null; + isDarwin = (builtins.match ".+-darwin$" system) != null; + shared = builtins.getEnv "_NIX_TEST_SHARED"; mkDerivation = args: diff --git a/tests/functional2/testlib/global_assets/dependencies/dependencies.nix b/tests/functional2/testlib/global_assets/dependencies/dependencies.nix index d9e3e3f1f..f939c0d51 100644 --- a/tests/functional2/testlib/global_assets/dependencies/dependencies.nix +++ b/tests/functional2/testlib/global_assets/dependencies/dependencies.nix @@ -17,7 +17,7 @@ let buildCommand = '' mkdir $out # Space-filler to test GC stats reporting - head -c 100k /dev/zero > $out/filler + head -c 100000 /dev/zero > $out/filler echo BAR > $out/bar echo ${input0} > $out/input0 echo ${input3} > $out/input3 diff --git a/tests/functional2/testlib/xattrs.py b/tests/functional2/testlib/xattrs.py new file mode 100644 index 000000000..2e454d2b3 --- /dev/null +++ b/tests/functional2/testlib/xattrs.py @@ -0,0 +1,53 @@ +import xattr +import pytest + +from pathlib import Path +from functional2.testlib.fixtures.env import ManagedEnv + + +def has_xattrs(path: Path) -> bool: + """ + Check if a file or directory has any extended attributes. + """ + try: + xattrs_list = xattr.list(str(path), nofollow=True) + return bool(xattrs_list) + except OSError: + # If an error occurs, either because the file doesn't exist or no xattrs, return False + return False + + +def verify_no_xattrs_in_tree(root_dir: Path) -> None: + """ + Traverse a directory tree and verify no file or directory has xattrs. + """ + if root_dir.is_file(): + assert not has_xattrs(root_dir) + return + + for entry in root_dir.iterdir(): + assert not has_xattrs(entry) + if entry.is_dir(): + verify_no_xattrs_in_tree(entry) + + +def test_set_clear_xattrs_in(dir_: Path) -> bool: + try: + xattr.set(dir_, "user.test", "1", nofollow=True) + xattr.remove(dir_, "user.test", nofollow=True) + return True + except OSError: + return False + + +def skip_if_xattrs_are_unsupported(env: ManagedEnv) -> None: + """ + Skip the current test if xattrs are unsupported in, either: + - the test root + - the fixture root + """ + if not test_set_clear_xattrs_in(env.dirs.test_root): + pytest.skip("xattrs cannot be used in the test root") + + if not test_set_clear_xattrs_in(env.dirs.home): + pytest.skip("xattrs cannot be used in the fixture directory")