From a5184ba1ef445b905c5d797fc1d2702793d1174c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C5=8Dshin?= Date: Mon, 18 Dec 2023 15:50:04 -0500 Subject: [PATCH] Remove constructor, do not verify fd with fcntl By the logic of the previous commit, it's not our job to verify that the user's whacked out set-id configuration is sane, only that the path they passed does not refer to a different file now than it did when ape found it. This is generally true of the files under /dev/fd in our case. So we just check that the file is in /dev/fd/, that it starts with not a '.', and that it does not contain a '/'. Since we don't care whether there actually is an fd by that name, we can do the check when called and save a constructor. --- libc/calls/getprogramexecutablename.greg.c | 48 +++++++--------------- 1 file changed, 15 insertions(+), 33 deletions(-) diff --git a/libc/calls/getprogramexecutablename.greg.c b/libc/calls/getprogramexecutablename.greg.c index fd57e4ae1..47bd8bc4d 100644 --- a/libc/calls/getprogramexecutablename.greg.c +++ b/libc/calls/getprogramexecutablename.greg.c @@ -23,7 +23,6 @@ #include "libc/cosmo.h" #include "libc/dce.h" #include "libc/errno.h" -#include "libc/fmt/conv.h" #include "libc/fmt/libgen.h" #include "libc/intrin/getenv.internal.h" #include "libc/serialize.h" @@ -33,7 +32,6 @@ #include "libc/runtime/runtime.h" #include "libc/str/str.h" #include "libc/sysv/consts/at.h" -#include "libc/sysv/consts/f.h" #include "libc/sysv/consts/ok.h" #define CTL_KERN 1 @@ -50,8 +48,6 @@ static struct { } u; } g_prog; -static bool g_ape_loaded; - static inline int IsAlpha(int c) { return ('A' <= c && c <= 'Z') || ('a' <= c && c <= 'z'); } @@ -109,29 +105,6 @@ static int TryPath(const char *q, int com) { return 0; } -__attribute__((__constructor__)) static void setugidNameCheck(void) { - char *end; - unsigned long rc; - if (__program_executable_name) { - g_ape_loaded = true; - if (issetugid()) { - /* we are running as a set-id interpreter script. this is highly unusual. - it means either someone installed their ape loader set-id, or they are - running a system that supports secure set-id interpreter scripts via a - /dev/fd/ path. check for the latter and allow that. otherwise, set the - __program_executable_name to NULL since there is an unavoidable TOCTOU - problem between the kernel and the loader. */ - if ((!IsNetbsd() && !IsOpenbsd() && !IsXnu()) /* any others? */ || - 0 != strncmp(DEV_FD, __program_executable_name, sizeof(DEV_FD) - 1) || - (rc = strtoul(__program_executable_name + sizeof(DEV_FD) - 1, &end, - 10)) > INT_MAX || - *end || rc < 3 || fcntl((int)rc, F_GETFD) == -1) { - __program_executable_name = NULL; - } - } - } -} - static inline void InitProgramExecutableNameImpl(void) { size_t n; ssize_t got; @@ -161,12 +134,21 @@ static inline void InitProgramExecutableNameImpl(void) { return; } - if (g_ape_loaded) { - /* the loader passed us a path. it may be relative. */ - if (!__program_executable_name) { - /* we rejected the loader path because we are running set-id and it - looked insecure. use the empty string. */ - goto UseEmpty; + // see if the loader passed us a path. + if (__program_executable_name) { + if (issetugid()) { + /* we are running as a set-id interpreter script. this is highly unusual. + it means either someone installed their ape loader set-id, or they are + running a system that supports secure set-id interpreter scripts via a + /dev/fd/ path. check for the latter and allow that. otherwise, use the + empty string to obviate the TOCTOU problem between loader and binary. + */ + if ((!IsNetbsd() && !IsOpenbsd() && !IsXnu()) /* any others? */ || + 0 != strncmp(DEV_FD, __program_executable_name, sizeof(DEV_FD) - 1) || + __program_executable_name[sizeof(DEV_FD) - 1] == '.' || + strchr(__program_executable_name + sizeof(DEV_FD) - 1, '/')) { + goto UseEmpty; + } } if (*__program_executable_name == '/') { return;