| Message ID | 1445228952-22445-6-git-send-email-yamamoto@midokura.com |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
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?
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.
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.
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.
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?
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?
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?
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()
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(-)