From a183563cf06a578879a886bb686879f7ddf2e9b4 Mon Sep 17 00:00:00 2001 From: Shea Levy Date: Wed, 7 Mar 2018 17:09:05 -0500 Subject: Revert "Merge branch 'setuid-wrapper-readlink'" Kernel symlinks don't have st_size. Really thought I tested this, guess I ran the wrong NixOS test :( This reverts commit 6dab907ebe9c8015b8cbc4871313ed48c64c548c, reversing changes made to eab479a5f0e46ad461ebda9953477be8f1e5e2bb. --- nixos/modules/security/wrappers/wrapper.c | 36 ++++++++++--------------------- 1 file changed, 11 insertions(+), 25 deletions(-) (limited to 'nixos/modules/security') diff --git a/nixos/modules/security/wrappers/wrapper.c b/nixos/modules/security/wrappers/wrapper.c index af34c2f2fc55..7091e314bb22 100644 --- a/nixos/modules/security/wrappers/wrapper.c +++ b/nixos/modules/security/wrappers/wrapper.c @@ -1,4 +1,3 @@ -#define _GNU_SOURCE #include #include #include @@ -162,34 +161,22 @@ static int make_caps_ambient(const char *selfPath) int main(int argc, char * * argv) { - // O_PATH | O_NOFOLLOW gives us a fd pointing to the symlink - int selfExe = open("/proc/self/exe", O_PATH | O_CLOEXEC | O_NOFOLLOW); - assert(selfExe != -1); - struct stat st; - assert(fstat(selfExe, &st) != -1); - size_t selfPathCap = st.st_size + 1; - char *selfPath = malloc(selfPathCap); - assert(selfPath); - int selfPathSize = readlinkat(selfExe, "", selfPath, selfPathCap); + // I *think* it's safe to assume that a path from a symbolic link + // should safely fit within the PATH_MAX system limit. Though I'm + // not positive it's safe... + char selfPath[PATH_MAX]; + int selfPathSize = readlink("/proc/self/exe", selfPath, sizeof(selfPath)); assert(selfPathSize > 0); // Assert we have room for the zero byte, this ensures the path // isn't being truncated because it's too big for the buffer. // - // selfPathSize is the number of bytes readlinkat put into the - // buffer, which does *not* append a null byte. selfPathCap is the - // capacity of the buffer, which was set to the number of bytes in - // the link contents (again, without the null byte) plus one for - // the null byte. - // - // I don't think it's possible for the link contents to change - // between opening a symlink fd and readlinkat on it, so this is - // probably not necessary. Doubly so since this is /proc/self/exe, - // not a normal symlink. But it's trivial to check. - assert(selfPathSize < selfPathCap); - - assert(close(selfExe)); + // A better way to handle this might be to use something like the + // whereami library (https://github.com/gpakosz/whereami) or a + // loop that resizes the buffer and re-reads the link if the + // contents are being truncated. + assert(selfPathSize < sizeof(selfPath)); // Set the zero byte since readlink doesn't do that for us. selfPath[selfPathSize] = '\0'; @@ -210,6 +197,7 @@ int main(int argc, char * * argv) // `selfPath', and not, say, as some other setuid program. That // is, our effective uid/gid should match the uid/gid of // `selfPath'. + struct stat st; assert(lstat(selfPath, &st) != -1); assert(!(st.st_mode & S_ISUID) || (st.st_uid == geteuid())); @@ -240,8 +228,6 @@ int main(int argc, char * * argv) // capabilities too! make_caps_ambient(selfPath); - free(selfPath); - execve(sourceProg, argv, environ); fprintf(stderr, "%s: cannot run `%s': %s\n", -- cgit 1.4.1