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

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

[Paolo Bonzini]

Fixes the following error with glibc 2.16 on Fedora 18:

virtfs-proxy-helper.c: In function ‘setfsugid’:
virtfs-proxy-helper.c:293:13: error: ignoring return value of ‘setfsgid’, declared with attribute warn_unused_result [-Werror=unused-result]
virtfs-proxy-helper.c:294:13: error: ignoring return value of ‘setfsuid’, declared with attribute warn_unused_result [-Werror=unused-result]
cc1: all warnings being treated as errors

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 fsdev/virtfs-proxy-helper.c | 8 ++++++--
 1 file modificato, 6 inserzioni(+), 2 rimozioni(-)

diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
index f9a8270..b34a84a 100644
--- a/fsdev/virtfs-proxy-helper.c
+++ b/fsdev/virtfs-proxy-helper.c
@@ -290,8 +290,12 @@ static int setfsugid(int uid, int gid)
         CAP_DAC_OVERRIDE,
     };
 
-    setfsgid(gid);
-    setfsuid(uid);
+    if (setfsgid(gid) != 0) {
+        return -1;
+    }
+    if (setfsuid(uid) != 0) {
+        return -1;
+    }
 
     if (uid != 0 || gid != 0) {
         return do_cap_set(cap_list, ARRAY_SIZE(cap_list), 0);
-- 
1.7.12.1

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

[Stefan Weil]

Am 10.10.2012 13:32, schrieb Paolo Bonzini:
> Fixes the following error with glibc 2.16 on Fedora 18:
>
> virtfs-proxy-helper.c: In function ‘setfsugid’:
> virtfs-proxy-helper.c:293:13: error: ignoring return value of ‘setfsgid’, declared with attribute warn_unused_result [-Werror=unused-result]
> virtfs-proxy-helper.c:294:13: error: ignoring return value of ‘setfsuid’, declared with attribute warn_unused_result [-Werror=unused-result]
> cc1: all warnings being treated as errors
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   fsdev/virtfs-proxy-helper.c | 8 ++++++--
>   1 file modificato, 6 inserzioni(+), 2 rimozioni(-)
>
> diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
> index f9a8270..b34a84a 100644
> --- a/fsdev/virtfs-proxy-helper.c
> +++ b/fsdev/virtfs-proxy-helper.c
> @@ -290,8 +290,12 @@ static int setfsugid(int uid, int gid)
>           CAP_DAC_OVERRIDE,
>       };
>   
> -    setfsgid(gid);
> -    setfsuid(uid);
> +    if (setfsgid(gid) != 0) {
> +        return -1;
> +    }

Wouldn't setfsgid(gid) == gid be also ok?

I have no idea whether it is a realistic scenario to call
that function with a gid which is already set.


> +    if (setfsuid(uid) != 0) {
> +        return -1;
> +    }
>   

The same question applies here.

>       if (uid != 0 || gid != 0) {
>           return do_cap_set(cap_list, ARRAY_SIZE(cap_list), 0);

The Linux manpage for both functions says something
completely different. Extract from manpage of setfsuid:

"On success, the previous value of fsuid is returned.
  On error, the current value of fsuid is returned."

In reality, the functions return 0 when called by root.

Regards

Stefan

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

[Paolo Bonzini]

Il 10/10/2012 18:14, Stefan Weil ha scritto:
>>
>>
>> diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
>> index f9a8270..b34a84a 100644
>> --- a/fsdev/virtfs-proxy-helper.c
>> +++ b/fsdev/virtfs-proxy-helper.c
>> @@ -290,8 +290,12 @@ static int setfsugid(int uid, int gid)
>>           CAP_DAC_OVERRIDE,
>>       };
>>   -    setfsgid(gid);
>> -    setfsuid(uid);
>> +    if (setfsgid(gid) != 0) {
>> +        return -1;
>> +    }
> 
> Wouldn't setfsgid(gid) == gid be also ok?

Of course, it should be < 0.  I have no idea how to test this thing...

Paolo

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

[Stefan Weil]

Am 10.10.2012 18:17, schrieb Paolo Bonzini:
> Il 10/10/2012 18:14, Stefan Weil ha scritto:
>>>
>>> diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
>>> index f9a8270..b34a84a 100644
>>> --- a/fsdev/virtfs-proxy-helper.c
>>> +++ b/fsdev/virtfs-proxy-helper.c
>>> @@ -290,8 +290,12 @@ static int setfsugid(int uid, int gid)
>>>            CAP_DAC_OVERRIDE,
>>>        };
>>>    -    setfsgid(gid);
>>> -    setfsuid(uid);
>>> +    if (setfsgid(gid) != 0) {
>>> +        return -1;
>>> +    }
>> Wouldn't setfsgid(gid) == gid be also ok?
> Of course, it should be < 0.  I have no idea how to test this thing...
>
> Paolo

< 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;
}

Stefan

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h> /* glibc uses <sys/fsuid.h> */
#include <sys/fsuid.h>

int main(int argc, char *argv[])
{
   uid_t fsuid = strtoul(argv[1], 0, 0);
   int r = setfsuid(fsuid);
   printf("setfsuid(%u) returned %u\n", fsuid, r);
   return 0;
}

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

[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 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

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

[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

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

[Stefan Weil]

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.

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

[Paolo Bonzini]

Il 10/10/2012 18:54, Stefan Weil ha scritto:
>>
>>     if (setfsuid(uid) < 0 || setfsuid(uid) != uid) {
>>         return -1;
>>     }
>>
>> but it seems wasteful to do four syscalls instead of two.
> 
> I added a local variable in my example to avoid those extra
> syscalls.

Note that the two setfsuid() calls are different.

The first checks the "-1" error from glibc.  The second says "if the
first call succeeded, the second call should see "uid" as the current
fsuid and the second call will be a no-op; if not, the first call must
have failed".

> 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.

So it would be

    if (setfsuid(uid) < 0) {
        return -1;
    }
    if (setfsuid(uid) != uid) {
        errno = EPERM;
        return -1;
    }

I still prefer my v2 (v1 is wrong).  The return path seems to be dead,
but it's not worse than before...

Paolo

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 901 bytes --]

On 10/10/2012 05:32 AM, Paolo Bonzini wrote:
> Fixes the following error with glibc 2.16 on Fedora 18:
> 
> virtfs-proxy-helper.c: In function ‘setfsugid’:
> virtfs-proxy-helper.c:293:13: error: ignoring return value of ‘setfsgid’, declared with attribute warn_unused_result [-Werror=unused-result]
> virtfs-proxy-helper.c:294:13: error: ignoring return value of ‘setfsuid’, declared with attribute warn_unused_result [-Werror=unused-result]
> cc1: all warnings being treated as errors
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  fsdev/virtfs-proxy-helper.c | 8 ++++++--
>  1 file modificato, 6 inserzioni(+), 2 rimozioni(-)

How does this differ from this earlier posting?
https://lists.gnu.org/archive/html/qemu-devel/2012-10/msg00851.html

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 617 bytes --]

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 995 bytes --]

On 10/10/2012 10:17 AM, Paolo Bonzini wrote:
> Il 10/10/2012 18:14, Stefan Weil ha scritto:
>>>
>>>
>>> diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
>>> index f9a8270..b34a84a 100644
>>> --- a/fsdev/virtfs-proxy-helper.c
>>> +++ b/fsdev/virtfs-proxy-helper.c
>>> @@ -290,8 +290,12 @@ static int setfsugid(int uid, int gid)
>>>           CAP_DAC_OVERRIDE,
>>>       };
>>>   -    setfsgid(gid);
>>> -    setfsuid(uid);
>>> +    if (setfsgid(gid) != 0) {
>>> +        return -1;
>>> +    }
>>
>> Wouldn't setfsgid(gid) == gid be also ok?
> 
> Of course, it should be < 0.  I have no idea how to test this thing...

POSIX states that uid_t and gid_t may be unsigned, so checking for < 0
is not necessarily possible (really, all you can check for is equality
with the same value as ((uid_t)-1) when put through integer promotion
rules).

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 617 bytes --]

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

[M. Mohan Kumar]

Stefan Weil <sw@weilnetz.de> writes:

We need to change fsuid and fsgid in 9p server side when 9p client wants
to create a file with given uid and gid.

In my case setfsuid and setfsgid never return -1 even if a normal user tries to
change fsuid.

I am running F17 and glibc is 2.15-56.fc17

IMHO setfsuid/setfsgid need to return -1 & set errno to EPERM when calling user
lacks CAP_SETUID capability. I can work on the kernel side to return
proper error values.

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. Client sends the user id details which may be
a nonexistent user id in host system. Ideally setfsuid setfsgid sets
whatever uid/gid passed if the caller is root as of now.   


> 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.

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

[Paolo Bonzini]

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 <pbonini@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.
> 

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

[Aneesh Kumar K.V]

Paolo Bonzini <pbonzini@redhat.com> writes:

> 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 <pbonini@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();
> +    }
> +

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 <pbonini@redhat.com>
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;
+    }
 
     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;
 }
 
 /*
@@ -586,9 +614,8 @@ static int do_create_others(int type, struct iovec *iovec)
         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 +648,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, cur_uid, cur_gid);
     return retval;
 }
 
@@ -641,24 +668,20 @@ 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, cur_uid, cur_gid);
+
 unmarshal_err_out:
     v9fs_string_free(&path);
     return ret;

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

[M. Mohan Kumar]

"Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> 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 <pbonini@redhat.com>
> 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 <mohan@in.ibm.com>

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

[Aneesh Kumar K.V]

Hi,

I have tested the below patch. Currently i don't have a signed-off-by on
the patch. One change noted by mohan which I incorporated in the patch
is we need to call setresgid before calling setresuid. If you are ok
with this change I can send it upstream.

commit d11bb5bb31a68e3eefb4e592161978dba29137f5
Author: Paolo Bonzini <pbonini@redhat.com>
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.
    
    Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>

diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
index f9a8270..df2a939 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);
+    *suid = geteuid();
+    *sgid = getegid();
+
+    if (setresgid(-1, gid, *sgid) == -1) {
+        retval = -errno;
+        goto err_out;
+    }
+
+    if (setresuid(-1, uid, *suid) == -1) {
+        retval = -errno;
+        goto err_sgid;
+    }
 
     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_suid;
+        }
     }
     return 0;
+
+err_suid:
+    if (setresuid(-1, *suid, *suid) == -1) {
+        abort();
+    }
+err_sgid:
+    if (setresgid(-1, *sgid, *sgid) == -1) {
+        abort();
+    }
+err_out:
+    return retval;
+}
+
+/*
+ * This is used to reset the ugid back with the saved values
+ * There is nothing much we can do checking error values here.
+ */
+static void resetugid(int suid, int sgid)
+{
+    if (setresgid(-1, sgid, sgid) == -1) {
+        abort();
+    }
+    if (setresuid(-1, suid, suid) == -1) {
+        abort();
+    }
 }
 
 /*
@@ -578,18 +623,15 @@ 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;
+        goto unmarshal_err_out;
     }
     switch (type) {
     case T_MKNOD:
@@ -619,9 +661,10 @@ static int do_create_others(int type, struct iovec *iovec)
     }
 
 err_out:
+    resetugid(cur_uid, cur_gid);
+unmarshal_err_out:
     v9fs_string_free(&path);
     v9fs_string_free(&oldpath);
-    setfsugid(cur_uid, cur_gid);
     return retval;
 }
 
@@ -641,24 +684,16 @@ 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);
+    resetugid(cur_uid, cur_gid);
 unmarshal_err_out:
     v9fs_string_free(&path);
     return ret;

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

[Paolo Bonzini]

Il 05/12/2012 09:35, Aneesh Kumar K.V ha scritto:
> I have tested the below patch. Currently i don't have a signed-off-by on
> the patch. One change noted by mohan which I incorporated in the patch
> is we need to call setresgid before calling setresuid. If you are ok
> with this change I can send it upstream.

Yes, please.

Either your version without pointers or mine is okay for me.

Paolo

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

[Paolo Bonzini]

Il 05/12/2012 13:37, Paolo Bonzini ha scritto:
>> > I have tested the below patch. Currently i don't have a signed-off-by on
>> > the patch. One change noted by mohan which I incorporated in the patch
>> > is we need to call setresgid before calling setresuid. If you are ok
>> > with this change I can send it upstream.
> Yes, please.
> 
> Either your version without pointers or mine is okay for me.

Ping?

Paolo

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

[Aneesh Kumar K.V]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 05/12/2012 13:37, Paolo Bonzini ha scritto:
>>> > I have tested the below patch. Currently i don't have a signed-off-by on
>>> > the patch. One change noted by mohan which I incorporated in the patch
>>> > is we need to call setresgid before calling setresuid. If you are ok
>>> > with this change I can send it upstream.
>> Yes, please.
>> 
>> Either your version without pointers or mine is okay for me.
>
> Ping?

Merged upstream with commit 9fd2ecdc8cb2dc1a8a7c57b6c9c60bc9947b6a73

-aneesh