From c0e607da612b0203a5357cadb9b345c7c321c163 Mon Sep 17 00:00:00 2001 From: Robert Obryk Date: Fri, 25 Aug 2023 21:51:27 +0200 Subject: [PATCH 1/6] nixos/tests/wrappers: test apparmor configuration Wrappers generate pieces of apparmor policies for inclusion, which are used only in a single place in nixpkgs, for `ping`. They are built only if apparmor is enabled. This change causes the test to test: - that the apparmor includes can be generated, - that `ping` works with apparmor enabled (as the only policy that references these includes). Ideally there would be some other NixOS test that verifies that `ping` specifically works. Sadly, there isn't one. --- nixos/tests/wrappers.nix | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/nixos/tests/wrappers.nix b/nixos/tests/wrappers.nix index 391e9b42b45..4c7a82f7dd0 100644 --- a/nixos/tests/wrappers.nix +++ b/nixos/tests/wrappers.nix @@ -21,6 +21,8 @@ in }; }; + security.apparmor.enable = true; + security.wrappers = { suidRoot = { owner = "root"; @@ -96,5 +98,11 @@ in machine.succeed("chmod u+s,a+w /run/wrappers/bin/suid_root_busybox") machine.fail(cmd_as_regular("/run/wrappers/bin/suid_root_busybox id -u")) + + # Test that the only user of apparmor policy includes generated by + # wrappers works. Ideally this'd be located in a test for the module that + # actually makes the apparmor policy for ping, but there's no convenient + # test for that one. + machine.succeed("ping -c 1 127.0.0.1") ''; }) From 44fde723be696020dc4c78d5deae3501b6cb088f Mon Sep 17 00:00:00 2001 From: Robert Obryk Date: Fri, 25 Aug 2023 21:52:40 +0200 Subject: [PATCH 2/6] nixos/security/wrappers: generate a separate and more complete apparmor policy fragment for each wrapper This change includes some stuff (e.g. reading of the `.real` file, execution of the wrapper's target) that belongs to the apparmor policy of the wrapper. This necessitates making them distinct for each wrapper. The main reason for this change is as a preparation for making each wrapper be a distinct binary. --- nixos/modules/security/wrappers/default.nix | 9 ++++++--- nixos/modules/tasks/network-interfaces.nix | 6 ++---- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/nixos/modules/security/wrappers/default.nix b/nixos/modules/security/wrappers/default.nix index 12255d8392f..2f886cef3a7 100644 --- a/nixos/modules/security/wrappers/default.nix +++ b/nixos/modules/security/wrappers/default.nix @@ -248,11 +248,14 @@ in export PATH="${wrapperDir}:$PATH" ''; - security.apparmor.includes."nixos/security.wrappers" = '' - include "${pkgs.apparmorRulesFromClosure { name="security.wrappers"; } [ + security.apparmor.includes = lib.mapAttrs' (wrapName: wrap: lib.nameValuePair + "nixos/security.wrappers/${wrapName}" '' + include "${pkgs.apparmorRulesFromClosure { name="security.wrappers.${wrapName}"; } [ securityWrapper ]}" - ''; + mrpx ${wrap.source}, + r /run/wrappers/wrappers.*/${wrapName}.real, + '') wrappers; ###### wrappers activation script system.activationScripts.wrappers = diff --git a/nixos/modules/tasks/network-interfaces.nix b/nixos/modules/tasks/network-interfaces.nix index eb1c7512d92..0d4033ca943 100644 --- a/nixos/modules/tasks/network-interfaces.nix +++ b/nixos/modules/tasks/network-interfaces.nix @@ -1396,14 +1396,12 @@ in security.apparmor.policies."bin.ping".profile = lib.mkIf config.security.apparmor.policies."bin.ping".enable (lib.mkAfter '' /run/wrappers/bin/ping { include - include + include rpx /run/wrappers/wrappers.*/ping, } /run/wrappers/wrappers.*/ping { include - include - r /run/wrappers/wrappers.*/ping.real, - mrpx ${config.security.wrappers.ping.source}, + include capability net_raw, capability setpcap, } From 1bdbc0b0fedcb5fdcb60a88f9781e53d9b12d5c8 Mon Sep 17 00:00:00 2001 From: Robert Obryk Date: Sat, 5 Nov 2022 00:09:32 +0100 Subject: [PATCH 3/6] nixos/security/wrappers: stop using `.real` files Before this change it was crucial that nonprivileged users are unable to create hardlinks to SUID wrappers, lest they be able to provide a different `.real` file alongside. That was ensured by not providing a location writable to them in the /run/wrappers tmpfs, (unless disabled) by the fs.protected_hardlinks=1 sysctl, and by the explicit own-path check in the wrapper. After this change, ensuring that property is no longer important, and the check is most likely redundant. The simplification of expectations of the wrapper will make it easier to remove some of the assertions in the wrapper (which currently cause the wrapper to fail in no_new_privs environments, instead of executing the target with non-elevated privileges). Note that wrappers had to be copied (not symlinked) into /run/wrappers due to the SUID/capability bits, and they couldn't be hard/softlinks of each other due to those bits potentially differing. Thus, this change doesn't increase the amount of memory used by /run/wrappers. This change removes part of the test that is obsoleted by the removal of `.real` files. --- nixos/modules/security/wrappers/default.nix | 13 ++++----- nixos/modules/security/wrappers/wrapper.c | 32 ++++++--------------- nixos/modules/security/wrappers/wrapper.nix | 3 +- nixos/tests/wrappers.nix | 7 ----- 4 files changed, 16 insertions(+), 39 deletions(-) diff --git a/nixos/modules/security/wrappers/default.nix b/nixos/modules/security/wrappers/default.nix index 2f886cef3a7..95ef7fa2c4d 100644 --- a/nixos/modules/security/wrappers/default.nix +++ b/nixos/modules/security/wrappers/default.nix @@ -5,8 +5,8 @@ let parentWrapperDir = dirOf wrapperDir; - securityWrapper = pkgs.callPackage ./wrapper.nix { - inherit parentWrapperDir; + securityWrapper = sourceProg : pkgs.callPackage ./wrapper.nix { + inherit parentWrapperDir sourceProg; }; fileModeType = @@ -91,8 +91,7 @@ let , ... }: '' - cp ${securityWrapper}/bin/security-wrapper "$wrapperDir/${program}" - echo -n "${source}" > "$wrapperDir/${program}.real" + cp ${securityWrapper source}/bin/security-wrapper "$wrapperDir/${program}" # Prevent races chmod 0000 "$wrapperDir/${program}" @@ -119,8 +118,7 @@ let , ... }: '' - cp ${securityWrapper}/bin/security-wrapper "$wrapperDir/${program}" - echo -n "${source}" > "$wrapperDir/${program}.real" + cp ${securityWrapper source}/bin/security-wrapper "$wrapperDir/${program}" # Prevent races chmod 0000 "$wrapperDir/${program}" @@ -251,10 +249,9 @@ in security.apparmor.includes = lib.mapAttrs' (wrapName: wrap: lib.nameValuePair "nixos/security.wrappers/${wrapName}" '' include "${pkgs.apparmorRulesFromClosure { name="security.wrappers.${wrapName}"; } [ - securityWrapper + (securityWrapper wrap.source) ]}" mrpx ${wrap.source}, - r /run/wrappers/wrappers.*/${wrapName}.real, '') wrappers; ###### wrappers activation script diff --git a/nixos/modules/security/wrappers/wrapper.c b/nixos/modules/security/wrappers/wrapper.c index 17776a97af8..b00ec942320 100644 --- a/nixos/modules/security/wrappers/wrapper.c +++ b/nixos/modules/security/wrappers/wrapper.c @@ -17,6 +17,10 @@ #include #include +#ifndef SOURCE_PROG +#error SOURCE_PROG should be defined via preprocessor commandline +#endif + // aborts when false, printing the failed expression #define ASSERT(expr) ((expr) ? (void) 0 : assert_failure(#expr)) // aborts when returns non-zero, printing the failed expression and errno @@ -198,11 +202,10 @@ int main(int argc, char **argv) { int didnt_sgid = (rgid == egid) && (egid == sgid); + // TODO: Determine if this is still useful, in particular if + // make_caps_ambient somehow relies on these properties. // Make sure that we are being executed from the right location, - // i.e., `safe_wrapper_dir'. This is to prevent someone from creating - // hard link `X' from some other location, along with a false - // `X.real' file, to allow arbitrary programs from being executed - // with elevated capabilities. + // i.e., `safe_wrapper_dir'. int len = strlen(wrapper_dir); if (len > 0 && '/' == wrapper_dir[len - 1]) --len; @@ -230,23 +233,6 @@ int main(int argc, char **argv) { // And, of course, we shouldn't be writable. ASSERT(!(st.st_mode & (S_IWGRP | S_IWOTH))); - // Read the path of the real (wrapped) program from .real. - char real_fn[PATH_MAX + 10]; - int real_fn_size = snprintf(real_fn, sizeof(real_fn), "%s.real", self_path); - ASSERT(real_fn_size < sizeof(real_fn)); - - int fd_self = open(real_fn, O_RDONLY); - ASSERT(fd_self != -1); - - char source_prog[PATH_MAX]; - len = read(fd_self, source_prog, PATH_MAX); - ASSERT(len != -1); - ASSERT(len < sizeof(source_prog)); - ASSERT(len > 0); - source_prog[len] = 0; - - close(fd_self); - // Read the capabilities set on the wrapper and raise them in to // the ambient set so the program we're wrapping receives the // capabilities too! @@ -256,10 +242,10 @@ int main(int argc, char **argv) { } free(self_path); - execve(source_prog, argv, environ); + execve(SOURCE_PROG, argv, environ); fprintf(stderr, "%s: cannot run `%s': %s\n", - argv[0], source_prog, strerror(errno)); + argv[0], SOURCE_PROG, strerror(errno)); return 1; } diff --git a/nixos/modules/security/wrappers/wrapper.nix b/nixos/modules/security/wrappers/wrapper.nix index e3620fb222d..afc5042768e 100644 --- a/nixos/modules/security/wrappers/wrapper.nix +++ b/nixos/modules/security/wrappers/wrapper.nix @@ -1,4 +1,4 @@ -{ stdenv, linuxHeaders, parentWrapperDir, debug ? false }: +{ stdenv, linuxHeaders, parentWrapperDir, sourceProg, debug ? false }: # For testing: # $ nix-build -E 'with import {}; pkgs.callPackage ./wrapper.nix { parentWrapperDir = "/run/wrappers"; debug = true; }' stdenv.mkDerivation { @@ -8,6 +8,7 @@ stdenv.mkDerivation { hardeningEnable = [ "pie" ]; CFLAGS = [ ''-DWRAPPER_DIR="${parentWrapperDir}"'' + ''-DSOURCE_PROG="${sourceProg}"'' ] ++ (if debug then [ "-Werror" "-Og" "-g" ] else [ diff --git a/nixos/tests/wrappers.nix b/nixos/tests/wrappers.nix index 4c7a82f7dd0..fc32ed41026 100644 --- a/nixos/tests/wrappers.nix +++ b/nixos/tests/wrappers.nix @@ -92,13 +92,6 @@ in machine.succeed(cmd_as_regular('/run/wrappers/bin/capsh_with_chown --has-p=CAP_CHOWN')) machine.fail(cmd_as_regular('/run/wrappers/bin/capsh_with_chown --has-p=CAP_SYS_ADMIN')) - # test a few "attacks" against which the wrapper protects itself - machine.succeed("cp /run/wrappers/bin/suid_root_busybox{,.real} /tmp/") - machine.fail(cmd_as_regular("/tmp/suid_root_busybox id -u")) - - machine.succeed("chmod u+s,a+w /run/wrappers/bin/suid_root_busybox") - machine.fail(cmd_as_regular("/run/wrappers/bin/suid_root_busybox id -u")) - # Test that the only user of apparmor policy includes generated by # wrappers works. Ideally this'd be located in a test for the module that # actually makes the apparmor policy for ping, but there's no convenient From e3550208de58dbf1ce92de85fd555674bc00ce82 Mon Sep 17 00:00:00 2001 From: Robert Obryk Date: Mon, 14 Nov 2022 14:45:36 +0100 Subject: [PATCH 4/6] nixos/security/wrappers: read capabilities off /proc/self/exe directly /proc/self/exe is a "fake" symlink. When it's opened, it always opens the actual file that was execve()d in this process, even if the file was deleted or renamed; if the file is no longer accessible from the current chroot/mount namespace it will at the very worst fail and never open the wrong file. Thus, we can make a much simpler argument that we're reading capabilities off the correct file after this change (and that argument doesn't rely on things such as protected_hardlinks being enabled, or no users being able to write to /run/wrappers, or the verification that the path readlink returns starts with /run/wrappers/). --- nixos/modules/security/wrappers/wrapper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nixos/modules/security/wrappers/wrapper.c b/nixos/modules/security/wrappers/wrapper.c index b00ec942320..d9875a5280d 100644 --- a/nixos/modules/security/wrappers/wrapper.c +++ b/nixos/modules/security/wrappers/wrapper.c @@ -236,7 +236,7 @@ int main(int argc, char **argv) { // Read the capabilities set on the wrapper and raise them in to // the ambient set so the program we're wrapping receives the // capabilities too! - if (make_caps_ambient(self_path) != 0) { + if (make_caps_ambient("/proc/self/exe") != 0) { free(self_path); return 1; } From c64bbd4466fd00163d97e40eac0c7ec849dfb2a9 Mon Sep 17 00:00:00 2001 From: Robert Obryk Date: Mon, 14 Nov 2022 14:52:16 +0100 Subject: [PATCH 5/6] nixos/security/wrappers: remove all the assertions about readlink(/proc/self/exe) Given that we are no longer inspecting the target of the /proc/self/exe symlink, stop asserting that it has any properties. Remove the plumbing for wrappersDir, which is no longer used. Asserting that the binary is located in the specific place is no longer necessary, because we don't rely on that location being writable only by privileged entities (we used to rely on that when assuming that readlink(/proc/self/exe) will continue to point at us and when assuming that the `.real` file can be trusted). Assertions about lack of write bits on the file were IMO meaningless since inception: ignoring the Linux's refusal to honor S[UG]ID bits on files-writeable-by-others, if someone could have modified the wrapper in a way that preserved the capability or S?ID bits, they could just remove this check. Assertions about effective UID were IMO just harmful: if we were executed without elevation, the caller would expect the result that would cause in a wrapperless distro: the targets gets executed without elevation. Due to lack of elevation, that cannot be used to abuse privileges that the elevation would give. This change partially fixes #98863 for S[UG]ID wrappers. The issue for capability wrappers remains. --- nixos/modules/security/wrappers/default.nix | 2 +- nixos/modules/security/wrappers/wrapper.c | 81 --------------------- nixos/modules/security/wrappers/wrapper.nix | 3 +- 3 files changed, 2 insertions(+), 84 deletions(-) diff --git a/nixos/modules/security/wrappers/default.nix b/nixos/modules/security/wrappers/default.nix index 95ef7fa2c4d..ad65f80bb2c 100644 --- a/nixos/modules/security/wrappers/default.nix +++ b/nixos/modules/security/wrappers/default.nix @@ -6,7 +6,7 @@ let parentWrapperDir = dirOf wrapperDir; securityWrapper = sourceProg : pkgs.callPackage ./wrapper.nix { - inherit parentWrapperDir sourceProg; + inherit sourceProg; }; fileModeType = diff --git a/nixos/modules/security/wrappers/wrapper.c b/nixos/modules/security/wrappers/wrapper.c index d9875a5280d..2cf1727a31c 100644 --- a/nixos/modules/security/wrappers/wrapper.c +++ b/nixos/modules/security/wrappers/wrapper.c @@ -28,10 +28,6 @@ extern char **environ; -// The WRAPPER_DIR macro is supplied at compile time so that it cannot -// be changed at runtime -static char *wrapper_dir = WRAPPER_DIR; - // Wrapper debug variable name static char *wrapper_debug = "WRAPPER_DEBUG"; @@ -155,92 +151,15 @@ static int make_caps_ambient(const char *self_path) { return 0; } -int readlink_malloc(const char *p, char **ret) { - size_t l = FILENAME_MAX+1; - int r; - - for (;;) { - char *c = calloc(l, sizeof(char)); - if (!c) { - return -ENOMEM; - } - - ssize_t n = readlink(p, c, l-1); - if (n < 0) { - r = -errno; - free(c); - return r; - } - - if ((size_t) n < l-1) { - c[n] = 0; - *ret = c; - return 0; - } - - free(c); - l *= 2; - } -} - int main(int argc, char **argv) { ASSERT(argc >= 1); - char *self_path = NULL; - int self_path_size = readlink_malloc("/proc/self/exe", &self_path); - if (self_path_size < 0) { - fprintf(stderr, "cannot readlink /proc/self/exe: %s", strerror(-self_path_size)); - } - - unsigned int ruid, euid, suid, rgid, egid, sgid; - MUSTSUCCEED(getresuid(&ruid, &euid, &suid)); - MUSTSUCCEED(getresgid(&rgid, &egid, &sgid)); - - // If true, then we did not benefit from setuid privilege escalation, - // where the original uid is still in ruid and different from euid == suid. - int didnt_suid = (ruid == euid) && (euid == suid); - // If true, then we did not benefit from setgid privilege escalation - int didnt_sgid = (rgid == egid) && (egid == sgid); - - - // TODO: Determine if this is still useful, in particular if - // make_caps_ambient somehow relies on these properties. - // Make sure that we are being executed from the right location, - // i.e., `safe_wrapper_dir'. - int len = strlen(wrapper_dir); - if (len > 0 && '/' == wrapper_dir[len - 1]) - --len; - ASSERT(!strncmp(self_path, wrapper_dir, len)); - ASSERT('/' == wrapper_dir[0]); - ASSERT('/' == self_path[len]); - - // If we got privileges with the fs set[ug]id bit, check that the privilege we - // got matches the one one we expected, ie that our effective uid/gid - // matches the uid/gid of `self_path`. This ensures that we were executed as - // `self_path', and not, say, as some other setuid program. - // We don't check that if we did not benefit from the set[ug]id bit, as - // can be the case in nosuid mounts or user namespaces. - struct stat st; - ASSERT(lstat(self_path, &st) != -1); - - // if the wrapper gained privilege with suid, check that we got the uid of the file owner - ASSERT(!((st.st_mode & S_ISUID) && !didnt_suid) || (st.st_uid == euid)); - // if the wrapper gained privilege with sgid, check that we got the gid of the file group - ASSERT(!((st.st_mode & S_ISGID) && !didnt_sgid) || (st.st_gid == egid)); - // same, but with suid instead of euid - ASSERT(!((st.st_mode & S_ISUID) && !didnt_suid) || (st.st_uid == suid)); - ASSERT(!((st.st_mode & S_ISGID) && !didnt_sgid) || (st.st_gid == sgid)); - - // And, of course, we shouldn't be writable. - ASSERT(!(st.st_mode & (S_IWGRP | S_IWOTH))); // Read the capabilities set on the wrapper and raise them in to // the ambient set so the program we're wrapping receives the // capabilities too! if (make_caps_ambient("/proc/self/exe") != 0) { - free(self_path); return 1; } - free(self_path); execve(SOURCE_PROG, argv, environ); diff --git a/nixos/modules/security/wrappers/wrapper.nix b/nixos/modules/security/wrappers/wrapper.nix index afc5042768e..aec43412404 100644 --- a/nixos/modules/security/wrappers/wrapper.nix +++ b/nixos/modules/security/wrappers/wrapper.nix @@ -1,4 +1,4 @@ -{ stdenv, linuxHeaders, parentWrapperDir, sourceProg, debug ? false }: +{ stdenv, linuxHeaders, sourceProg, debug ? false }: # For testing: # $ nix-build -E 'with import {}; pkgs.callPackage ./wrapper.nix { parentWrapperDir = "/run/wrappers"; debug = true; }' stdenv.mkDerivation { @@ -7,7 +7,6 @@ stdenv.mkDerivation { dontUnpack = true; hardeningEnable = [ "pie" ]; CFLAGS = [ - ''-DWRAPPER_DIR="${parentWrapperDir}"'' ''-DSOURCE_PROG="${sourceProg}"'' ] ++ (if debug then [ "-Werror" "-Og" "-g" From 13d3b0c73350d1bcee16316a1d5c0f327f466f5c Mon Sep 17 00:00:00 2001 From: Robert Obryk Date: Mon, 14 Nov 2022 15:09:25 +0100 Subject: [PATCH 6/6] nixos/security/wrappers: add one regression test for #98863 Note that this regression test checks only s[gu]id wrappers. The issue for capability wrappers is not fixed yet. --- nixos/tests/wrappers.nix | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/nixos/tests/wrappers.nix b/nixos/tests/wrappers.nix index fc32ed41026..1d4fa85d739 100644 --- a/nixos/tests/wrappers.nix +++ b/nixos/tests/wrappers.nix @@ -86,6 +86,17 @@ in test_as_regular_in_userns_mapped_as_root('/run/wrappers/bin/sgid_root_busybox id -g', '0') test_as_regular_in_userns_mapped_as_root('/run/wrappers/bin/sgid_root_busybox id -rg', '0') + # Test that in nonewprivs environment the wrappers simply exec their target. + test_as_regular('${pkgs.util-linux}/bin/setpriv --no-new-privs /run/wrappers/bin/suid_root_busybox id -u', '${toString userUid}') + test_as_regular('${pkgs.util-linux}/bin/setpriv --no-new-privs /run/wrappers/bin/suid_root_busybox id -ru', '${toString userUid}') + test_as_regular('${pkgs.util-linux}/bin/setpriv --no-new-privs /run/wrappers/bin/suid_root_busybox id -g', '${toString usersGid}') + test_as_regular('${pkgs.util-linux}/bin/setpriv --no-new-privs /run/wrappers/bin/suid_root_busybox id -rg', '${toString usersGid}') + + test_as_regular('${pkgs.util-linux}/bin/setpriv --no-new-privs /run/wrappers/bin/sgid_root_busybox id -u', '${toString userUid}') + test_as_regular('${pkgs.util-linux}/bin/setpriv --no-new-privs /run/wrappers/bin/sgid_root_busybox id -ru', '${toString userUid}') + test_as_regular('${pkgs.util-linux}/bin/setpriv --no-new-privs /run/wrappers/bin/sgid_root_busybox id -g', '${toString usersGid}') + test_as_regular('${pkgs.util-linux}/bin/setpriv --no-new-privs /run/wrappers/bin/sgid_root_busybox id -rg', '${toString usersGid}') + # We are only testing the permitted set, because it's easiest to look at with capsh. machine.fail(cmd_as_regular('${pkgs.libcap}/bin/capsh --has-p=CAP_CHOWN')) machine.fail(cmd_as_regular('${pkgs.libcap}/bin/capsh --has-p=CAP_SYS_ADMIN'))