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

[Aaron Conole]

Christian Ehrhardt <christian.ehrhardt at 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 at 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 at 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 at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev