[ovs-dev] lib: protect daemon_set_new_user against non existing user:group specs
diff mbox

Message ID 1461333866-22035-1-git-send-email-christian.ehrhardt@canonical.com
State Changes Requested
Headers show

Commit Message

Christian Ehrhardt April 22, 2016, 2:04 p.m. UTC
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(+)

Comments

Ben Pfaff April 22, 2016, 6:37 p.m. UTC | #1
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 *')
Christian Ehrhardt April 25, 2016, 5:55 a.m. UTC | #2
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 *')
>

Patch
diff mbox

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;