On Tue, Sep 8, 2015 at 5:29 PM, Ben Pfaff <[email protected]> wrote:
> On Thu, Sep 03, 2015 at 04:33:42PM -0700, Andy Zhou wrote:
>> Allow daemon running as root to accept --user option, that accepts
>> "user:group" string as input. Performs sanity check on the input,
>> and store the converted uid and gid.
>>
>> daemon_become_new_user() needs to be called to make the actual
>> switch.
>>
>> Signed-off-by: Andy Zhou <[email protected]>
>
> Thanks for implementing this.
>
> There should be a new-line after void here:
>> +void daemon_set_new_user(const char *user_spec)
>> +{
>
oops. Will fix.
> I think the ability to use setuid() and friends only depends on having
> uid 0, not gid 0:
>> + uid = getuid();
>> + gid = getgid();
>> +
>> + if (gid || uid) {
>> + VLOG_FATAL("%s: only root can use --user option", pidfile);
>> + }
>
You are right. I will fix this and check for effective uid Instead.
> Here in daemon_set_new_user() I would use xmemdup0() instead:
>> + if (len) {
>> + user = xzalloc(len + 1);
>> + strncpy(user, user_spec, len);
>
> and this should be xstrdup():
>> + user = strdup(pwd.pw_name);
>> + }
>
Thanks for pointing out. Will change.
> I think that the parsing in daemon_set_new_user() assumes that white
> space, if present, will precede ':'. If not, then 'len' will end up
> negative, which looks bad to me. I think I'd just not bother worrying
> about white space in the parameter.
>
Sure, I can drop the check. Why would len end up being negative? If pos is set,
meaning : is part of the string, then strspn should not look beyond pos, right?
> I am not sure that it is valuable to check that the user belongs to the
> specified group. I don't think that other software tends to perform
> such a check; I don't see one in Apache, for example.
>
I got that idea from the "daemon" program. Would it otherwise be a security risk
of creating an illegal combination of user/group?
> I might have other comments when I look at the final patch.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
