Unverified Commit bfdf28be authored by Pierre Bourdon's avatar Pierre Bourdon Committed by GitHub
Browse files

Merge pull request #251770 from robryk/suidwrapapparm

nixos/security/wrappers: simplifications and a fix for #98863 (respin of #199599)
parents cd971bb6 13d3b0c7
Loading
Loading
Loading
Loading
+10 −10
Original line number Diff line number Diff line
@@ -5,8 +5,8 @@ let

  parentWrapperDir = dirOf wrapperDir;

  securityWrapper = pkgs.callPackage ./wrapper.nix {
    inherit parentWrapperDir;
  securityWrapper = sourceProg : pkgs.callPackage ./wrapper.nix {
    inherit 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}"
@@ -248,11 +246,13 @@ in
      export PATH="${wrapperDir}:$PATH"
    '';

    security.apparmor.includes."nixos/security.wrappers" = ''
      include "${pkgs.apparmorRulesFromClosure { name="security.wrappers"; } [
        securityWrapper
    security.apparmor.includes = lib.mapAttrs' (wrapName: wrap: lib.nameValuePair
     "nixos/security.wrappers/${wrapName}" ''
      include "${pkgs.apparmorRulesFromClosure { name="security.wrappers.${wrapName}"; } [
        (securityWrapper wrap.source)
      ]}"
    '';
      mrpx ${wrap.source},
    '') wrappers;

    ###### wrappers activation script
    system.activationScripts.wrappers =
+7 −102
Original line number Diff line number Diff line
@@ -17,6 +17,10 @@
#include <syscall.h>
#include <byteswap.h>

#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
@@ -24,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";

@@ -151,115 +151,20 @@ 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);


    // 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.
    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 path of the real (wrapped) program from <self>.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!
    if (make_caps_ambient(self_path) != 0) {
        free(self_path);
    if (make_caps_ambient("/proc/self/exe") != 0) {
        return 1;
    }
    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;
}
+2 −2
Original line number Diff line number Diff line
{ stdenv, linuxHeaders, parentWrapperDir, debug ? false }:
{ stdenv, linuxHeaders, sourceProg, debug ? false }:
# For testing:
# $ nix-build -E 'with import <nixpkgs> {}; pkgs.callPackage ./wrapper.nix { parentWrapperDir = "/run/wrappers"; debug = true; }'
stdenv.mkDerivation {
@@ -7,7 +7,7 @@ stdenv.mkDerivation {
  dontUnpack = true;
  hardeningEnable = [ "pie" ];
  CFLAGS = [
    ''-DWRAPPER_DIR="${parentWrapperDir}"''
    ''-DSOURCE_PROG="${sourceProg}"''
  ] ++ (if debug then [
    "-Werror" "-Og" "-g"
  ] else [
+2 −4
Original line number Diff line number Diff line
@@ -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 <abstractions/base>
        include <nixos/security.wrappers>
        include <nixos/security.wrappers/ping>
        rpx /run/wrappers/wrappers.*/ping,
      }
      /run/wrappers/wrappers.*/ping {
        include <abstractions/base>
        include <nixos/security.wrappers>
        r /run/wrappers/wrappers.*/ping.real,
        mrpx ${config.security.wrappers.ping.source},
        include <nixos/security.wrappers/ping>
        capability net_raw,
        capability setpcap,
      }
+18 −6
Original line number Diff line number Diff line
@@ -21,6 +21,8 @@ in
      };
    };

    security.apparmor.enable = true;

    security.wrappers = {
      suidRoot = {
        owner = "root";
@@ -84,17 +86,27 @@ 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'))
      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
      # test for that one.
      machine.succeed("ping -c 1 127.0.0.1")
    '';
})