summary refs log tree commit diff
diff options
context:
space:
mode:
authorRobert Obryk <robryk@gmail.com>2022-11-14 14:52:16 +0100
committerRobert Obryk <robryk@gmail.com>2023-08-27 14:10:38 +0200
commitc64bbd4466fd00163d97e40eac0c7ec849dfb2a9 (patch)
tree00a8e0ce4da007f08d0527d50b6e1b76b5bc71c0
parente3550208de58dbf1ce92de85fd555674bc00ce82 (diff)
downloadnixpkgs-c64bbd4466fd00163d97e40eac0c7ec849dfb2a9.tar
nixpkgs-c64bbd4466fd00163d97e40eac0c7ec849dfb2a9.tar.gz
nixpkgs-c64bbd4466fd00163d97e40eac0c7ec849dfb2a9.tar.bz2
nixpkgs-c64bbd4466fd00163d97e40eac0c7ec849dfb2a9.tar.lz
nixpkgs-c64bbd4466fd00163d97e40eac0c7ec849dfb2a9.tar.xz
nixpkgs-c64bbd4466fd00163d97e40eac0c7ec849dfb2a9.tar.zst
nixpkgs-c64bbd4466fd00163d97e40eac0c7ec849dfb2a9.zip
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.
-rw-r--r--nixos/modules/security/wrappers/default.nix2
-rw-r--r--nixos/modules/security/wrappers/wrapper.c81
-rw-r--r--nixos/modules/security/wrappers/wrapper.nix3
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 <nixpkgs> {}; 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"