| Message ID | 1461568339-8698-1-git-send-email-christian.ehrhardt@canonical.com |
|---|---|
| State | Accepted |
| Headers | show |
Aarons last patch reminded me that we didn't went on on the fixed patch for this code path that Aaron just modified. So giving this a bump to show up again. Christian Ehrhardt Software Engineer, Ubuntu Server Canonical Ltd On Mon, Apr 25, 2016 at 2:12 AM, Christian Ehrhardt < christian.ehrhardt@canonical.com> wrote: > From the manpages of getgrnam_r (getpwnam_r is similar): > "If no matching group record was found, these functions return 0 and > store NULL in *result." > > The code checked only against errors, but non existing users didn't set > e != 0 therefore the code could try to set arbitrary uid/gid values. > > *Update in v2* > fix wrong pointer usage of *res and running full set of unit tests to be > sure. > > Fixes: e91b927d lib/daemon: support --user option for all OVS daemon > > Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> > --- > lib/daemon-unix.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c > index 182f76b..28f76da 100644 > --- a/lib/daemon-unix.c > +++ b/lib/daemon-unix.c > @@ -972,6 +972,9 @@ daemon_set_new_user(const char *user_spec) > VLOG_FATAL("%s: Failed to retrive user %s's uid (%s), > aborting.", > pidfile, user, ovs_strerror(e)); > } > + if (res == NULL) { > + VLOG_FATAL("%s: user %s not found, aborting.", pidfile, user); > + } > } else { > /* User name is not specified, use current user. */ > while ((e = getpwuid_r(uid, &pwd, buf, bufsize, &res)) == ERANGE) > { > @@ -1012,6 +1015,10 @@ daemon_set_new_user(const char *user_spec) > "(%s), aborting.", pidfile, grpstr, > ovs_strerror(e)); > } > + if (res == NULL) { > + VLOG_FATAL("%s: group %s not found, aborting.", pidfile, > + grpstr); > + } > > if (gid != grp.gr_gid) { > char **mem; > -- > 2.7.4 > >
Christian Ehrhardt <christian.ehrhardt@canonical.com> writes: > Aarons last patch reminded me that we didn't went on on the fixed patch for > this code path that Aaron just modified. > So giving this a bump to show up again. > > Christian Ehrhardt > Software Engineer, Ubuntu Server > Canonical Ltd > > On Mon, Apr 25, 2016 at 2:12 AM, Christian Ehrhardt < > christian.ehrhardt@canonical.com> wrote: > >> From the manpages of getgrnam_r (getpwnam_r is similar): >> "If no matching group record was found, these functions return 0 and >> store NULL in *result." >> >> The code checked only against errors, but non existing users didn't set >> e != 0 therefore the code could try to set arbitrary uid/gid values. >> >> *Update in v2* >> fix wrong pointer usage of *res and running full set of unit tests to be >> sure. >> >> Fixes: e91b927d lib/daemon: support --user option for all OVS daemon >> >> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> >> --- Cool! From my recent series (in the ovs_strtousr function): + if (e != 0 || !res) { + if (!res) e = ENOENT; + VLOG_ERR("Failed to retrieve user pwentry (%s), aborting.", + ovs_strerror(e)); + goto release; + } I think I did testing with the ovs_strtousr function, and figured I missed the check when moving it over. >> lib/daemon-unix.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c >> index 182f76b..28f76da 100644 >> --- a/lib/daemon-unix.c >> +++ b/lib/daemon-unix.c >> @@ -972,6 +972,9 @@ daemon_set_new_user(const char *user_spec) >> VLOG_FATAL("%s: Failed to retrive user %s's uid (%s), >> aborting.", >> pidfile, user, ovs_strerror(e)); >> } >> + if (res == NULL) { >> + VLOG_FATAL("%s: user %s not found, aborting.", pidfile, user); >> + } >> } else { >> /* User name is not specified, use current user. */ >> while ((e = getpwuid_r(uid, &pwd, buf, bufsize, &res)) == ERANGE) >> { >> @@ -1012,6 +1015,10 @@ daemon_set_new_user(const char *user_spec) >> "(%s), aborting.", pidfile, grpstr, >> ovs_strerror(e)); >> } >> + if (res == NULL) { >> + VLOG_FATAL("%s: group %s not found, aborting.", pidfile, >> + grpstr); >> + } >> >> if (gid != grp.gr_gid) { >> char **mem; >> -- >> 2.7.4 >> >> > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
On Mon, Apr 25, 2016 at 09:12:19AM +0200, Christian Ehrhardt wrote: > From the manpages of getgrnam_r (getpwnam_r is similar): > "If no matching group record was found, these functions return 0 and > store NULL in *result." > > The code checked only against errors, but non existing users didn't set > e != 0 therefore the code could try to set arbitrary uid/gid values. > > *Update in v2* > fix wrong pointer usage of *res and running full set of unit tests to be sure. > > Fixes: e91b927d lib/daemon: support --user option for all OVS daemon > > Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Thanks, applied to master and branch-2.5.
diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c index 182f76b..28f76da 100644 --- a/lib/daemon-unix.c +++ b/lib/daemon-unix.c @@ -972,6 +972,9 @@ daemon_set_new_user(const char *user_spec) VLOG_FATAL("%s: Failed to retrive user %s's uid (%s), aborting.", pidfile, user, ovs_strerror(e)); } + if (res == NULL) { + VLOG_FATAL("%s: user %s not found, aborting.", pidfile, user); + } } else { /* User name is not specified, use current user. */ while ((e = getpwuid_r(uid, &pwd, buf, bufsize, &res)) == ERANGE) { @@ -1012,6 +1015,10 @@ daemon_set_new_user(const char *user_spec) "(%s), aborting.", pidfile, grpstr, ovs_strerror(e)); } + if (res == NULL) { + VLOG_FATAL("%s: group %s not found, aborting.", pidfile, + grpstr); + } if (gid != grp.gr_gid) { char **mem;
From the manpages of getgrnam_r (getpwnam_r is similar): "If no matching group record was found, these functions return 0 and store NULL in *result." The code checked only against errors, but non existing users didn't set e != 0 therefore the code could try to set arbitrary uid/gid values. *Update in v2* fix wrong pointer usage of *res and running full set of unit tests to be sure. Fixes: e91b927d lib/daemon: support --user option for all OVS daemon Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- lib/daemon-unix.c | 7 +++++++ 1 file changed, 7 insertions(+)