Re: [Qemu-devel] [PATCH] virtfs-proxy-helper: check return code of setfsgid/setfsuid

[M. Mohan Kumar]

"Aneesh Kumar K.V" <address@hidden> writes:

> I found this to be confusing. How about avoiding all those pointers, something
> like below ? If you are ok can I add the signed-off-by for this ? I can
> test this and get a pull request out with the build fix.
>
> commit 24cc9f0d07c2a505bfafbdcb72006f2eda1288a4
> Author: Paolo Bonzini <address@hidden>
> Date:   Thu Oct 11 14:20:23 2012 +0200
>
>     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..49ab0eb 100644
> --- a/fsdev/virtfs-proxy-helper.c
> +++ b/fsdev/virtfs-proxy-helper.c
> @@ -272,31 +272,59 @@ 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 (setresuid(-1, uid, suid) == -1) {
> +        retval = -errno;
> +        goto err_out;
> +    }
> +    if (setresgid(-1, gid, sgid) == -1) {
> +        retval = -errno;
> +        goto err_suid;
> +    }
>

After changing the order of setresuid and setresgid this patch works as
expected. Please move setresgid before setresuid.

Tested-by: M. Mohan Kumar <address@hidden>