Il 11/10/2012 09:25, M. Mohan Kumar ha scritto: > Also as per the man page: > When glibc determines that the argument is not a valid user ID, > it will return -1 and set errno to EINVAL > without attempting the system call. > > If it mean a nonexistent id by 'not a valid user ID' it may be a > problem in virtfs case.
I think only -1 would be an invalid user ID, or perhaps a user ID > 65535 if the kernel only supports 16-bit user IDs. Rather than dealing with the kernel, can we just use setresuid/setresgid like in the following (untested) patch? Paolo ps: so far in my short life I had managed to stay away from privilege dropping, so please review with extra care. ------------------- 8< ----------------------- From: Paolo Bonzini <pbon...@redhat.com> Date: Thu, 11 Oct 2012 14:20:23 +0200 Subject: [PATCH] virtfs-proxy-helper: use setresuid and setresgid The setfsuid and setfsgid system calls are obscure and they complicate the error checking (that glibc's warn_unused_result "feature" forces us to do). Switch to the standard setresuid and setresgid functions. --- diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c index f9a8270..07b3b5b 100644 --- a/fsdev/virtfs-proxy-helper.c +++ b/fsdev/virtfs-proxy-helper.c @@ -272,31 +272,76 @@ static int send_status(int sockfd, struct iovec *iovec, int status) /* * from man 7 capabilities, section * Effect of User ID Changes on Capabilities: - * 4. If the file system user ID is changed from 0 to nonzero (see setfsuid(2)) - * then the following capabilities are cleared from the effective set: - * CAP_CHOWN, CAP_DAC_OVERRIDE, CAP_DAC_READ_SEARCH, CAP_FOWNER, CAP_FSETID, - * CAP_LINUX_IMMUTABLE (since Linux 2.2.30), CAP_MAC_OVERRIDE, and CAP_MKNOD - * (since Linux 2.2.30). If the file system UID is changed from nonzero to 0, - * then any of these capabilities that are enabled in the permitted set - * are enabled in the effective set. + * If the effective user ID is changed from nonzero to 0, then the permitted + * set is copied to the effective set. If the effective user ID is changed + * from 0 to nonzero, then all capabilities are are cleared from the effective + * set. + * + * The setfsuid/setfsgid man pages warn that changing the effective user ID may + * expose the program to unwanted signals, but this is not true anymore: for an + * unprivileged (without CAP_KILL) program to send a signal, the real or + * effective user ID of the sending process must equal the real or saved user + * ID of the target process. Even when dropping privileges, it is enough to + * keep the saved UID to a "privileged" value and virtfs-proxy-helper won't + * be exposed to signals. So just use setresuid/setresgid. */ -static int setfsugid(int uid, int gid) +static int setugid(int uid, int gid, int *suid, int *sgid) { + int retval; + /* - * We still need DAC_OVERRIDE because we don't change + * We still need DAC_OVERRIDE because we don't change * supplementary group ids, and hence may be subjected DAC rules */ cap_value_t cap_list[] = { CAP_DAC_OVERRIDE, }; - setfsgid(gid); - setfsuid(uid); + /* + * If suid/sgid are NULL, the saved uid/gid is set to the + * new effective uid/gid. If they are not, the saved uid/gid + * is set to the current effective user id and stored into + * *suid and *sgid. + */ + if (!suid) { + suid = &uid; + } else { + *suid = geteuid(); + } + if (!sgid) { + sgid = &gid; + } else { + *sgid = getegid(); + } + + if (setresuid(-1, uid, *suid) == -1) { + retval = -errno; + goto err_out; + } + if (setresgid(-1, gid, *sgid) == -1) { + retval = -errno; + goto err_suid; + } if (uid != 0 || gid != 0) { - return do_cap_set(cap_list, ARRAY_SIZE(cap_list), 0); + if (do_cap_set(cap_list, ARRAY_SIZE(cap_list), 0) < 0) { + retval = -errno; + goto err_sgid; + } } + return 0; + +err_sgid: + if (setresgid(-1, *sgid, *sgid) == -1) { + abort(); + } +err_suid: + if (setresuid(-1, *suid, *suid) == -1) { + abort(); + } +err_out: + return retval; } /* @@ -578,17 +623,14 @@ static int do_create_others(int type, struct iovec *iovec) v9fs_string_init(&path); v9fs_string_init(&oldpath); - cur_uid = geteuid(); - cur_gid = getegid(); retval = proxy_unmarshal(iovec, offset, "dd", &uid, &gid); if (retval < 0) { return retval; } offset += retval; - retval = setfsugid(uid, gid); + retval = setugid(uid, gid, &cur_uid, &cur_gid); if (retval < 0) { - retval = -errno; goto err_out; } switch (type) { @@ -621,7 +663,7 @@ static int do_create_others(int type, struct iovec *iovec) err_out: v9fs_string_free(&path); v9fs_string_free(&oldpath); - setfsugid(cur_uid, cur_gid); + setugid(cur_uid, cur_gid, NULL, NULL); return retval; } @@ -641,24 +683,17 @@ static int do_create(struct iovec *iovec) if (ret < 0) { goto unmarshal_err_out; } - cur_uid = geteuid(); - cur_gid = getegid(); - ret = setfsugid(uid, gid); + ret = setugid(uid, gid, &cur_uid, &cur_gid); if (ret < 0) { - /* - * On failure reset back to the - * old uid/gid - */ - ret = -errno; - goto err_out; + goto unmarshal_err_out; } ret = open(path.data, flags, mode); if (ret < 0) { ret = -errno; } -err_out: - setfsugid(cur_uid, cur_gid); + setugid(cur_uid, cur_gid, NULL, NULL); + unmarshal_err_out: v9fs_string_free(&path); return ret; >> Am 10.10.2012 18:54, schrieb Stefan Weil: >>> Am 10.10.2012 18:36, schrieb Paolo Bonzini: >>>> Il 10/10/2012 18:23, Stefan Weil ha scritto: >>>>> < 0 would be wrong because it looks like both functions never >>>>> return negative values. >>>>> I just wrote a small test program (see >>>>> below) and called it with different uids with and without root >>>>> rights. This pattern should be fine: >>>>> >>>>> new_uid = setfsuid(uid); >>>>> if (new_uid != 0 && new_uid != uid) { >>>>> return -1; >>>>> } >>>> I didn't really care about this case. I assumed that the authors knew >>>> what they were doing... >>>> >>>> What I cared about is: "When glibc determines that the argument is not a >>>> valid group ID, it will return -1 and set errno to EINVAL >>>> without >>>> attempting the system call". >>> >>> I was not able to get -1 with my test program: any value which I tried >>> seemed to work when the program was called with sudo. >>> >>>> >>>> I think this would also work: >>>> >>>> if (setfsuid(uid) < 0 || setfsuid(uid) != uid) { >>>> return -1; >>>> } >>>> >>>> but it seems wasteful to do four syscalls instead of two. >>>> >>>> Paolo >>> >>> I added a local variable in my example to avoid those extra >>> syscalls. >>> >>> Your last patch v2 does not handle missing rights (no root) >>> because in that case the functions don't return a value < 0 >>> but fail nevertheless.Calling a program which requires >>> root privileges from a normal user account is usually a >>> very common error. I don't know the use cases for virtfs - >>> maybe that's no problem here. >>> >>> The functions have an additional problem: they don't set >>> errno (see manpages). I tested this, and here the manpages >>> are correct. The code in virtfs-proxy-helper expects that >>> errno was set, so the patch must set errno = EPERM or >>> something like that. >>> >>> Stefan >> >> Maybe the author of those code can tell us more on the >> use cases and which errors must be handled. >> >> Is it necessary to use those functions at all (they are very >> Linux specific), or can they be replaced by seteuid, setegid? >> >> Regards >> >> Stefan W. >