[ovs-dev,06/21] daemon_switch_user: Improve portablility
diff mbox

Message ID 1445228952-22445-6-git-send-email-yamamoto@midokura.com
State Awaiting Upstream
Headers show

Commit Message

Takashi Yamamoto Oct. 19, 2015, 4:28 a.m. UTC
NetBSD doesn't have [gs]etres[ug]id.

Signed-off-by: YAMAMOTO Takashi <yamamoto@midokura.com>
---
 lib/daemon-unix.c | 40 ++++++++++++++++++----------------------
 1 file changed, 18 insertions(+), 22 deletions(-)

Comments

Andy Zhou Oct. 19, 2015, 6:14 a.m. UTC | #1
On Sun, Oct 18, 2015 at 9:28 PM, YAMAMOTO Takashi <yamamoto@midokura.com> wrote:
> NetBSD doesn't have [gs]etres[ug]id.
>
> Signed-off-by: YAMAMOTO Takashi <yamamoto@midokura.com>
> ---
>  lib/daemon-unix.c | 40 ++++++++++++++++++----------------------
>  1 file changed, 18 insertions(+), 22 deletions(-)
>
Thanks for testing on NetBSD.

I am concerned that on platforms supports saved uid, Would this patch
leave that value not changed, thus open up a security risk?

How about we add a stub version of [gs]etres[ug]id for the NetBSD
platform that can safely ignore the saved uid/ gid for that platform?
Takashi Yamamoto Oct. 19, 2015, 6:48 a.m. UTC | #2
hi,

On Mon, Oct 19, 2015 at 3:14 PM, Andy Zhou <azhou@nicira.com> wrote:
> On Sun, Oct 18, 2015 at 9:28 PM, YAMAMOTO Takashi <yamamoto@midokura.com> wrote:
>> NetBSD doesn't have [gs]etres[ug]id.
>>
>> Signed-off-by: YAMAMOTO Takashi <yamamoto@midokura.com>
>> ---
>>  lib/daemon-unix.c | 40 ++++++++++++++++++----------------------
>>  1 file changed, 18 insertions(+), 22 deletions(-)
>>
> Thanks for testing on NetBSD.
>
> I am concerned that on platforms supports saved uid, Would this patch
> leave that value not changed, thus open up a security risk?
>
> How about we add a stub version of [gs]etres[ug]id for the NetBSD
> platform that can safely ignore the saved uid/ gid for that platform?

NetBSD has saved uid/gid.
saved ids are expected to be changed by set[ug]id.
http://pubs.opengroup.org/onlinepubs/9699919799/functions/setuid.html
http://man.netbsd.org/HEAD/usr/share/man/html2/setuid.html

i'm not sure what security risks you are concerning about.
Andy Zhou Oct. 19, 2015, 10:14 p.m. UTC | #3
I am going by the advice of paper " The Murky Issue of Changing
Process Identity: Revising “Setuid Demystified” "

On page 7, it says:

Specifically, all OSes that support getresuid (see Figure 3) also
support setresuid and setresgid. These offer the clearest and most
consistent semantics, and can be used by privileged and non-privileged
processes alike.

According to the paper,  setuid() may or may not change saved uid, it
is OS dependent and may only change effective uid in cause current uid
is not
zero.

Also according to the same paper in Figure 3, getresuid() is supported
by Linux, HPUX, FreeBSD and OpenBSD, it would be nice to let those OS
use this API. For NetBSD, we can resolve this by emulating the
getresuid() call.  Make sense?



On Sun, Oct 18, 2015 at 11:48 PM, Takashi Yamamoto
<yamamoto@midokura.com> wrote:
> hi,
>
> On Mon, Oct 19, 2015 at 3:14 PM, Andy Zhou <azhou@nicira.com> wrote:
>> On Sun, Oct 18, 2015 at 9:28 PM, YAMAMOTO Takashi <yamamoto@midokura.com> wrote:
>>> NetBSD doesn't have [gs]etres[ug]id.
>>>
>>> Signed-off-by: YAMAMOTO Takashi <yamamoto@midokura.com>
>>> ---
>>>  lib/daemon-unix.c | 40 ++++++++++++++++++----------------------
>>>  1 file changed, 18 insertions(+), 22 deletions(-)
>>>
>> Thanks for testing on NetBSD.
>>
>> I am concerned that on platforms supports saved uid, Would this patch
>> leave that value not changed, thus open up a security risk?
>>
>> How about we add a stub version of [gs]etres[ug]id for the NetBSD
>> platform that can safely ignore the saved uid/ gid for that platform?
>
> NetBSD has saved uid/gid.
> saved ids are expected to be changed by set[ug]id.
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/setuid.html
> http://man.netbsd.org/HEAD/usr/share/man/html2/setuid.html
>
> i'm not sure what security risks you are concerning about.
Takashi Yamamoto Oct. 20, 2015, 3:09 a.m. UTC | #4
hi,

On Tue, Oct 20, 2015 at 7:14 AM, Andy Zhou <azhou@nicira.com> wrote:
> I am going by the advice of paper " The Murky Issue of Changing
> Process Identity: Revising “Setuid Demystified” "
>
> On page 7, it says:
>
> Specifically, all OSes that support getresuid (see Figure 3) also
> support setresuid and setresgid. These offer the clearest and most
> consistent semantics, and can be used by privileged and non-privileged
> processes alike.
>
> According to the paper,  setuid() may or may not change saved uid, it
> is OS dependent and may only change effective uid in cause current uid
> is not
> zero.
>
> Also according to the same paper in Figure 3, getresuid() is supported
> by Linux, HPUX, FreeBSD and OpenBSD, it would be nice to let those OS
> use this API. For NetBSD, we can resolve this by emulating the
> getresuid() call.  Make sense?

well, this fallback code is currently for FreeBSD and NetBSD,
for which the semantics are consistent, right?

>
>
>
> On Sun, Oct 18, 2015 at 11:48 PM, Takashi Yamamoto
> <yamamoto@midokura.com> wrote:
>> hi,
>>
>> On Mon, Oct 19, 2015 at 3:14 PM, Andy Zhou <azhou@nicira.com> wrote:
>>> On Sun, Oct 18, 2015 at 9:28 PM, YAMAMOTO Takashi <yamamoto@midokura.com> wrote:
>>>> NetBSD doesn't have [gs]etres[ug]id.
>>>>
>>>> Signed-off-by: YAMAMOTO Takashi <yamamoto@midokura.com>
>>>> ---
>>>>  lib/daemon-unix.c | 40 ++++++++++++++++++----------------------
>>>>  1 file changed, 18 insertions(+), 22 deletions(-)
>>>>
>>> Thanks for testing on NetBSD.
>>>
>>> I am concerned that on platforms supports saved uid, Would this patch
>>> leave that value not changed, thus open up a security risk?
>>>
>>> How about we add a stub version of [gs]etres[ug]id for the NetBSD
>>> platform that can safely ignore the saved uid/ gid for that platform?
>>
>> NetBSD has saved uid/gid.
>> saved ids are expected to be changed by set[ug]id.
>> http://pubs.opengroup.org/onlinepubs/9699919799/functions/setuid.html
>> http://man.netbsd.org/HEAD/usr/share/man/html2/setuid.html
>>
>> i'm not sure what security risks you are concerning about.
Ben Pfaff Nov. 9, 2015, 11:26 p.m. UTC | #5
On Tue, Oct 20, 2015 at 12:09:46PM +0900, Takashi Yamamoto wrote:
> On Tue, Oct 20, 2015 at 7:14 AM, Andy Zhou <azhou@nicira.com> wrote:
> > I am going by the advice of paper " The Murky Issue of Changing
> > Process Identity: Revising “Setuid Demystified” "
> >
> > On page 7, it says:
> >
> > Specifically, all OSes that support getresuid (see Figure 3) also
> > support setresuid and setresgid. These offer the clearest and most
> > consistent semantics, and can be used by privileged and non-privileged
> > processes alike.
> >
> > According to the paper,  setuid() may or may not change saved uid, it
> > is OS dependent and may only change effective uid in cause current uid
> > is not
> > zero.
> >
> > Also according to the same paper in Figure 3, getresuid() is supported
> > by Linux, HPUX, FreeBSD and OpenBSD, it would be nice to let those OS
> > use this API. For NetBSD, we can resolve this by emulating the
> > getresuid() call.  Make sense?
> 
> well, this fallback code is currently for FreeBSD and NetBSD,
> for which the semantics are consistent, right?

Andy, any further comments on this?
Andy Zhou Nov. 10, 2015, 5 p.m. UTC | #6
No. I have Acked the change.

On Mon, Nov 9, 2015 at 3:26 PM, Ben Pfaff <blp@ovn.org> wrote:
> On Tue, Oct 20, 2015 at 12:09:46PM +0900, Takashi Yamamoto wrote:
>> On Tue, Oct 20, 2015 at 7:14 AM, Andy Zhou <azhou@nicira.com> wrote:
>> > I am going by the advice of paper " The Murky Issue of Changing
>> > Process Identity: Revising “Setuid Demystified” "
>> >
>> > On page 7, it says:
>> >
>> > Specifically, all OSes that support getresuid (see Figure 3) also
>> > support setresuid and setresgid. These offer the clearest and most
>> > consistent semantics, and can be used by privileged and non-privileged
>> > processes alike.
>> >
>> > According to the paper,  setuid() may or may not change saved uid, it
>> > is OS dependent and may only change effective uid in cause current uid
>> > is not
>> > zero.
>> >
>> > Also according to the same paper in Figure 3, getresuid() is supported
>> > by Linux, HPUX, FreeBSD and OpenBSD, it would be nice to let those OS
>> > use this API. For NetBSD, we can resolve this by emulating the
>> > getresuid() call.  Make sense?
>>
>> well, this fallback code is currently for FreeBSD and NetBSD,
>> for which the semantics are consistent, right?
>
> Andy, any further comments on this?
Ben Pfaff Nov. 10, 2015, 7:27 p.m. UTC | #7
OK, great, somehow  I missed that.

On Tue, Nov 10, 2015 at 09:00:05AM -0800, Andy Zhou wrote:
> No. I have Acked the change.
> 
> On Mon, Nov 9, 2015 at 3:26 PM, Ben Pfaff <blp@ovn.org> wrote:
> > On Tue, Oct 20, 2015 at 12:09:46PM +0900, Takashi Yamamoto wrote:
> >> On Tue, Oct 20, 2015 at 7:14 AM, Andy Zhou <azhou@nicira.com> wrote:
> >> > I am going by the advice of paper " The Murky Issue of Changing
> >> > Process Identity: Revising “Setuid Demystified” "
> >> >
> >> > On page 7, it says:
> >> >
> >> > Specifically, all OSes that support getresuid (see Figure 3) also
> >> > support setresuid and setresgid. These offer the clearest and most
> >> > consistent semantics, and can be used by privileged and non-privileged
> >> > processes alike.
> >> >
> >> > According to the paper,  setuid() may or may not change saved uid, it
> >> > is OS dependent and may only change effective uid in cause current uid
> >> > is not
> >> > zero.
> >> >
> >> > Also according to the same paper in Figure 3, getresuid() is supported
> >> > by Linux, HPUX, FreeBSD and OpenBSD, it would be nice to let those OS
> >> > use this API. For NetBSD, we can resolve this by emulating the
> >> > getresuid() call.  Make sense?
> >>
> >> well, this fallback code is currently for FreeBSD and NetBSD,
> >> for which the semantics are consistent, right?
> >
> > Andy, any further comments on this?

Patch
diff mbox

diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c
index 868e2c9..5b01d06 100644
--- a/lib/daemon-unix.c
+++ b/lib/daemon-unix.c
@@ -729,22 +729,20 @@  gid_matches(gid_t expected, gid_t value)
 }
 
 static bool
-gid_verify(gid_t real, gid_t effective, gid_t saved)
+gid_verify(gid_t gid)
 {
-    gid_t r, e, s;
+    gid_t r, e;
 
-    return (getresgid(&r, &e, &s) == 0 &&
-            gid_matches(real, r) &&
-            gid_matches(effective, e) &&
-            gid_matches(saved, s));
+    r = getgid();
+    e = getegid();
+    return (gid_matches(gid, r) &&
+            gid_matches(gid, e));
 }
 
 static void
-daemon_switch_group(gid_t real, gid_t effective,
-                    gid_t saved)
+daemon_switch_group(gid_t gid)
 {
-    if ((setresgid(real, effective, saved) == -1) ||
-        !gid_verify(real, effective, saved)) {
+    if ((setgid(gid) == -1) || !gid_verify(gid)) {
         VLOG_FATAL("%s: fail to switch group to gid as %d, aborting",
                    pidfile, gid);
     }
@@ -757,22 +755,20 @@  uid_matches(uid_t expected, uid_t value)
 }
 
 static bool
-uid_verify(const uid_t real, const uid_t effective, const uid_t saved)
+uid_verify(const uid_t uid)
 {
-    uid_t r, e, s;
+    uid_t r, e;
 
-    return (getresuid(&r, &e, &s) == 0 &&
-            uid_matches(real, r) &&
-            uid_matches(effective, e) &&
-            uid_matches(saved, s));
+    r = getuid();
+    e = geteuid();
+    return (uid_matches(uid, r) &&
+            uid_matches(uid, e));
 }
 
 static void
-daemon_switch_user(const uid_t real, const uid_t effective, const uid_t saved,
-                   const char *user)
+daemon_switch_user(const uid_t uid, const char *user)
 {
-    if ((setresuid(real, effective, saved) == -1) ||
-        !uid_verify(real, effective, saved)) {
+    if ((setuid(uid) == -1) || !uid_verify(uid)) {
         VLOG_FATAL("%s: fail to switch user to %s, aborting",
                    pidfile, user);
     }
@@ -794,12 +790,12 @@  daemon_become_new_user_unix(void)
      * that calling getuid() after each setuid() call to verify they
      * are actually set, because checking return code alone is not
      * sufficient.  */
-    daemon_switch_group(gid, gid, gid);
+    daemon_switch_group(gid);
     if (user && initgroups(user, gid) == -1) {
         VLOG_FATAL("%s: fail to add supplementary group gid %d, "
                    "aborting", pidfile, gid);
     }
-    daemon_switch_user(uid, uid, uid, user);
+    daemon_switch_user(uid, user);
 }
 
 /* Linux specific implementation of daemon_become_new_user()