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

Message ID 1461568339-8698-1-git-send-email-christian.ehrhardt@canonical.com
State Accepted
Headers show

Commit Message

Christian Ehrhardt April 25, 2016, 7:12 a.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.

*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(+)

Comments

Christian Ehrhardt May 3, 2016, 6:48 p.m. UTC | #1
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
>
>
Aaron Conole May 3, 2016, 7:23 p.m. UTC | #2
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
Ben Pfaff May 16, 2016, 9:14 p.m. UTC | #3
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.

Patch
diff mbox

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;