From 6974df39a708abf8bafbdfa2b7827e0f70f874cb Mon Sep 17 00:00:00 2001 From: Serge Hallyn Date: Mon, 6 Feb 2023 22:49:42 -0600 Subject: [PATCH] newuidmap and newgidmap: support passing pid as fd Closes #635 newuidmap and newgidmap currently take an integner pid as the first argument, determining the process id on which to act. Accept also "fd:N", where N must be an open file descriptor to the /proc/pid directory for the process to act upon. This way, if you exec 10 --- lib/get_pid.c | 51 +++++++++++++++++++++++++++++++++++++++++++++ lib/prototypes.h | 2 ++ man/newgidmap.1.xml | 11 ++++++++++ man/newuidmap.1.xml | 11 ++++++++++ src/newgidmap.c | 41 ++++++++++++++---------------------- src/newuidmap.c | 40 +++++++++++++---------------------- 6 files changed, 106 insertions(+), 50 deletions(-) diff --git a/lib/get_pid.c b/lib/get_pid.c index 10184bf0..ab91d158 100644 --- a/lib/get_pid.c +++ b/lib/get_pid.c @@ -10,6 +10,9 @@ #include "prototypes.h" #include "defines.h" +#include +#include +#include int get_pid (const char *pidstr, pid_t *pid) { @@ -29,3 +32,51 @@ int get_pid (const char *pidstr, pid_t *pid) return 1; } +/* + * If use passed in fd:4 as an argument, then return the + * value '4', the fd to use. + */ +int get_pidfd_from_fd(const char *pidfdstr) +{ + long long int val; + char *endptr; + + errno = 0; + val = strtoll (pidfdstr, &endptr, 10); + if ( ('\0' == *pidfdstr) + || ('\0' != *endptr) + || (ERANGE == errno) + || (/*@+longintegral@*/val != (pid_t)val)/*@=longintegral@*/) { + return 0; + } + + return (int)val; +} + +int open_pidfd(const char *pidstr) +{ + int proc_dir_fd; + int written; + char proc_dir_name[32]; + pid_t target; + + if (get_pid(pidstr, &target) == 0) + return -ENOENT; + + /* max string length is 6 + 10 + 1 + 1 = 18, allocate 32 bytes */ + written = snprintf(proc_dir_name, sizeof(proc_dir_name), "/proc/%u/", + target); + if ((written <= 0) || ((size_t)written >= sizeof(proc_dir_name))) { + fprintf(stderr, "snprintf of proc path failed for %u: %s\n", + target, strerror(errno)); + return -EINVAL; + } + + proc_dir_fd = open(proc_dir_name, O_DIRECTORY); + if (proc_dir_fd < 0) { + fprintf(stderr, _("Could not open proc directory for target %u: %s\n"), + target, strerror(errno)); + return -EINVAL; + } + return proc_dir_fd; +} diff --git a/lib/prototypes.h b/lib/prototypes.h index 400d5b97..21df6f61 100644 --- a/lib/prototypes.h +++ b/lib/prototypes.h @@ -160,6 +160,8 @@ extern int getlong (const char *numstr, /*@out@*/long int *result); /* get_pid.c */ extern int get_pid (const char *pidstr, pid_t *pid); +extern int get_pidfd_from_fd(const char *pidfdstr); +extern int open_pidfd(const char *pidstr); /* getrange */ extern int getrange (const char *range, diff --git a/man/newgidmap.1.xml b/man/newgidmap.1.xml index e4ebc69e..9b7683eb 100644 --- a/man/newgidmap.1.xml +++ b/man/newgidmap.1.xml @@ -116,6 +116,17 @@ Note that newgidmap may be used only once for a given process. + + Instead of an integer process id, the first argument may be + specified as fd:N, where the integer N + is the file descriptor number for the calling process's opened + file for /proc/[pid[. In this case, + newgidmap will use + openat2 + to open the gid_map file under that + directory, avoiding a TOCTTOU in case the process exits and + the pid is immediately reused. + diff --git a/man/newuidmap.1.xml b/man/newuidmap.1.xml index f5cb5b48..ca917a77 100644 --- a/man/newuidmap.1.xml +++ b/man/newuidmap.1.xml @@ -116,6 +116,17 @@ Note that newuidmap may be used only once for a given process. + + Instead of an integer process id, the first argument may be + specified as fd:N, where the integer N + is the file descriptor number for the calling process's opened + file for /proc/[pid[. In this case, + newuidmap will use + openat2 + to open the uid_map file under that + directory, avoiding a TOCTTOU in case the process exits and + the pid is immediately reused. + diff --git a/src/newgidmap.c b/src/newgidmap.c index 01d0fe90..d6d29725 100644 --- a/src/newgidmap.c +++ b/src/newgidmap.c @@ -69,7 +69,7 @@ static void verify_ranges(struct passwd *pw, int ranges, static void usage(void) { - fprintf(stderr, _("usage: %s [ ] ... \n"), Prog); + fprintf(stderr, _("usage: %s [] [ ] ... \n"), Prog); exit(EXIT_FAILURE); } @@ -143,15 +143,12 @@ out: */ int main(int argc, char **argv) { - char proc_dir_name[32]; char *target_str; - pid_t target; int proc_dir_fd; int ranges; struct map_range *mappings; struct stat st; struct passwd *pw; - int written; bool allow_setgroups = false; Prog = Basename (argv[0]); @@ -168,25 +165,19 @@ int main(int argc, char **argv) /* Find the process that needs its user namespace * gid mapping set. */ - target_str = argv[1]; - if (!get_pid(target_str, &target)) - usage(); - /* max string length is 6 + 10 + 1 + 1 = 18, allocate 32 bytes */ - written = snprintf(proc_dir_name, sizeof(proc_dir_name), "/proc/%u/", - target); - if ((written <= 0) || (written >= sizeof(proc_dir_name))) { - fprintf(stderr, "%s: snprintf of proc path failed: %s\n", - Prog, strerror(errno)); - } - - proc_dir_fd = open(proc_dir_name, O_DIRECTORY); - if (proc_dir_fd < 0) { - fprintf(stderr, _("%s: Could not open proc directory for target %u\n"), - Prog, target); - return EXIT_FAILURE; + target_str = argv[1]; + if (strlen(target_str) > 3 && strncmp(target_str, "fd:", 3) == 0) { + /* the user passed in a /proc/pid fd for the process */ + target_str = &target_str[3]; + proc_dir_fd = get_pidfd_from_fd(target_str); + if (proc_dir_fd < 0) + usage(); + } else { + proc_dir_fd = open_pidfd(target_str); + if (proc_dir_fd < 0) + usage(); } - /* Who am i? */ pw = get_my_pwent (); if (NULL == pw) { @@ -200,8 +191,8 @@ int main(int argc, char **argv) /* Get the effective uid and effective gid of the target process */ if (fstat(proc_dir_fd, &st) < 0) { - fprintf(stderr, _("%s: Could not stat directory for target %u\n"), - Prog, target); + fprintf(stderr, _("%s: Could not stat directory for process\n"), + Prog); return EXIT_FAILURE; } @@ -213,8 +204,8 @@ int main(int argc, char **argv) (!getdef_bool("GRANT_AUX_GROUP_SUBIDS") && (getgid() != pw->pw_gid)) || (pw->pw_uid != st.st_uid) || (getgid() != st.st_gid)) { - fprintf(stderr, _( "%s: Target %u is owned by a different user: uid:%lu pw_uid:%lu st_uid:%lu, gid:%lu pw_gid:%lu st_gid:%lu\n" ), - Prog, target, + fprintf(stderr, _( "%s: Target process is owned by a different user: uid:%lu pw_uid:%lu st_uid:%lu, gid:%lu pw_gid:%lu st_gid:%lu\n" ), + Prog, (unsigned long int)getuid(), (unsigned long int)pw->pw_uid, (unsigned long int)st.st_uid, (unsigned long int)getgid(), (unsigned long int)pw->pw_gid, (unsigned long int)st.st_gid); return EXIT_FAILURE; diff --git a/src/newuidmap.c b/src/newuidmap.c index e8798409..e99655c9 100644 --- a/src/newuidmap.c +++ b/src/newuidmap.c @@ -64,7 +64,7 @@ static void verify_ranges(struct passwd *pw, int ranges, static void usage(void) { - fprintf(stderr, _("usage: %s [ ] ... \n"), Prog); + fprintf(stderr, _("usage: %s [|fd:] [ ] ... \n"), Prog); exit(EXIT_FAILURE); } @@ -73,15 +73,12 @@ static void usage(void) */ int main(int argc, char **argv) { - char proc_dir_name[32]; char *target_str; - pid_t target; int proc_dir_fd; int ranges; struct map_range *mappings; struct stat st; struct passwd *pw; - int written; Prog = Basename (argv[0]); log_set_progname(Prog); @@ -94,26 +91,20 @@ int main(int argc, char **argv) if (argc < 2) usage(); + target_str = argv[1]; /* Find the process that needs its user namespace * uid mapping set. */ - target_str = argv[1]; - if (!get_pid(target_str, &target)) - usage(); - - /* max string length is 6 + 10 + 1 + 1 = 18, allocate 32 bytes */ - written = snprintf(proc_dir_name, sizeof(proc_dir_name), "/proc/%u/", - target); - if ((written <= 0) || (written >= sizeof(proc_dir_name))) { - fprintf(stderr, "%s: snprintf of proc path failed: %s\n", - Prog, strerror(errno)); - } - - proc_dir_fd = open(proc_dir_name, O_DIRECTORY); - if (proc_dir_fd < 0) { - fprintf(stderr, _("%s: Could not open proc directory for target %u\n"), - Prog, target); - return EXIT_FAILURE; + if (strlen(target_str) > 3 && strncmp(target_str, "fd:", 3) == 0) { + /* the user passed in a /proc/pid fd for the process */ + target_str = &target_str[3]; + proc_dir_fd = get_pidfd_from_fd(target_str); + if (proc_dir_fd < 0) + usage(); + } else { + proc_dir_fd = open_pidfd(target_str); + if (proc_dir_fd < 0) + usage(); } /* Who am i? */ @@ -129,8 +120,7 @@ int main(int argc, char **argv) /* Get the effective uid and effective gid of the target process */ if (fstat(proc_dir_fd, &st) < 0) { - fprintf(stderr, _("%s: Could not stat directory for target %u\n"), - Prog, target); + fprintf(stderr, _("%s: Could not stat directory for target process\n"), Prog); return EXIT_FAILURE; } @@ -142,8 +132,8 @@ int main(int argc, char **argv) (!getdef_bool("GRANT_AUX_GROUP_SUBIDS") && (getgid() != pw->pw_gid)) || (pw->pw_uid != st.st_uid) || (getgid() != st.st_gid)) { - fprintf(stderr, _( "%s: Target process %u is owned by a different user: uid:%lu pw_uid:%lu st_uid:%lu, gid:%lu pw_gid:%lu st_gid:%lu\n" ), - Prog, target, + fprintf(stderr, _( "%s: Target process is owned by a different user: uid:%lu pw_uid:%lu st_uid:%lu, gid:%lu pw_gid:%lu st_gid:%lu\n" ), + Prog, (unsigned long int)getuid(), (unsigned long int)pw->pw_uid, (unsigned long int)st.st_uid, (unsigned long int)getgid(), (unsigned long int)pw->pw_gid, (unsigned long int)st.st_gid); return EXIT_FAILURE; -- 2.39.2 From 7ff33fae6f9cd79c0e012671c37a172e9a681d0b Mon Sep 17 00:00:00 2001 From: Serge Hallyn Date: Fri, 24 Feb 2023 13:52:32 -0600 Subject: [PATCH] get_pidfd_from_fd: return -1 on error, not 0 Fixes: 6974df39a: newuidmap and newgidmap: support passing pid as fd Signed-off-by: Serge Hallyn --- lib/get_pid.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/get_pid.c b/lib/get_pid.c index ab91d158..5b6d9da4 100644 --- a/lib/get_pid.c +++ b/lib/get_pid.c @@ -35,6 +35,7 @@ int get_pid (const char *pidstr, pid_t *pid) /* * If use passed in fd:4 as an argument, then return the * value '4', the fd to use. + * On error, return -1. */ int get_pidfd_from_fd(const char *pidfdstr) { @@ -47,7 +48,7 @@ int get_pidfd_from_fd(const char *pidfdstr) || ('\0' != *endptr) || (ERANGE == errno) || (/*@+longintegral@*/val != (pid_t)val)/*@=longintegral@*/) { - return 0; + return -1; } return (int)val; -- 2.39.2 From 05e2adf509ba0e3779dae66a276b86927a8e1e0e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vin=C3=ADcius=20dos=20Santos=20Oliveira?= Date: Fri, 24 Feb 2023 18:06:02 -0300 Subject: [PATCH] Validate fds created by the user write_mapping() will do the following: openat(proc_dir_fd, map_file, O_WRONLY); An attacker could create a directory containing a symlink named "uid_map" pointing to any file owned by root, and thus allow him to overwrite any root-owned file. --- lib/get_pid.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/lib/get_pid.c b/lib/get_pid.c index 5b6d9da4..8e5e6014 100644 --- a/lib/get_pid.c +++ b/lib/get_pid.c @@ -41,6 +41,8 @@ int get_pidfd_from_fd(const char *pidfdstr) { long long int val; char *endptr; + struct stat st; + dev_t proc_st_dev, proc_st_rdev; errno = 0; val = strtoll (pidfdstr, &endptr, 10); @@ -51,6 +53,21 @@ int get_pidfd_from_fd(const char *pidfdstr) return -1; } + if (stat("/proc/self/uid_map", &st) < 0) { + return -1; + } + + proc_st_dev = st.st_dev; + proc_st_rdev = st.st_rdev; + + if (fstat(val, &st) < 0) { + return -1; + } + + if (st.st_dev != proc_st_dev || st.st_rdev != proc_st_rdev) { + return -1; + } + return (int)val; } -- 2.39.2