| Message ID | 1461333866-22035-1-git-send-email-christian.ehrhardt@canonical.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Fri, Apr 22, 2016 at 04:04:26PM +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. > > Fixes: e91b927d lib/daemon: support --user option for all OVS daemon > > Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> Thanks for the patch. This does not compile: ../lib/daemon-unix.c:975:18: error: invalid operands to binary expression ('struct passwd' and 'void *') ../lib/daemon-unix.c:1018:22: error: invalid operands to binary expression ('struct group' and 'void *')
Thanks Ben, I wrote that for something in DPDK inspired by that code when I found the bug and to eventually converted it back to help OVS as well. Sorry to miss that part on converting it back - will come up with a fixed version soon. Christian Ehrhardt Software Engineer, Ubuntu Server Canonical Ltd On Fri, Apr 22, 2016 at 8:37 PM, Ben Pfaff <blp@ovn.org> wrote: > On Fri, Apr 22, 2016 at 04:04:26PM +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. > > > > Fixes: e91b927d lib/daemon: support --user option for all OVS daemon > > > > Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> > > Thanks for the patch. > > This does not compile: > > ../lib/daemon-unix.c:975:18: error: invalid operands to binary > expression ('struct passwd' and 'void *') > ../lib/daemon-unix.c:1018:22: error: invalid operands to binary > expression ('struct group' and 'void *') >
diff --git a/lib/daemon-unix.c b/lib/daemon-unix.c index 182f76b..bbd1fd7 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. 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(+)