Bug 3253 - start-stop-daemon --chuid does not set supplemental groups
Summary: start-stop-daemon --chuid does not set supplemental groups
Status: RESOLVED FIXED
Alias: None
Product: Busybox
Classification: Unclassified
Component: Standard Compliance (show other bugs)
Version: 1.13.x
Hardware: All Linux
: P5 normal
Target Milestone: ---
Assignee: unassigned
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-15 16:41 UTC by Andreas Pretzsch
Modified: 2011-09-16 16:33 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:


Attachments
In busybox start-stop-daemon applet --chuid option, also set the supplemental groups of the specified user. (855 bytes, application/octet-stream)
2011-02-15 16:41 UTC, Andreas Pretzsch
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Pretzsch 2011-02-15 16:41:39 UTC
Created attachment 2965 [details]
In busybox start-stop-daemon applet --chuid option, also set the supplemental groups of the specified user.

In contrast to Debian reference start-stop-daemon, the busybox variant does not set the supplemental groups of the user when changing uid/gid.

From start-stop-daemon(8) of dpkg 1.15.8.10:
       -c, --chuid username|uid
              Change to this username/uid before starting the process. You can
              also  specify a group by appending a :, then the group or gid in
              the same way as you would for the `chown' command  (user:group).
              If a user is specified without a group, the primary GID for that
              user is used.  When using this option you must realize that  the
              primary  and  supplemental  groups  are set as well, even if the
              --group option is not specified. The --group option is only  for
              groups that the user isn't normally a member of (like adding per
              process group membership for generic users like nobody).

While observed on an old busybox 1.13.4, based on the code of 1.18.3 this still seems to be the case.

Please find attached an experimental patch for old 1.13.4 adding the missing initgroups() call. Please be aware that this patch only solves the immediate issue and is by no means checked for side effects and/or correctness. Also it's probably not the most elegant approach.
=> Use only as proof of concept.
Comment 1 Denys Vlasenko 2011-09-15 10:37:53 UTC
I don't have Debian machine to test it, so I need your input.

Do you mean that this is wrong?

# busybox start-stop-daemon -S -x id
uid=0(root) gid=0(root) groups=0(root),1(bin),2(daemon),3(sys)

# busybox start-stop-daemon -S -c 0:111 -x id
uid=0(root) gid=111 groups=0(root),1(bin),2(daemon),3(sys)
                    ^^^groups should be reset to 111 too!^^^^


Correct?
Comment 2 Andreas Pretzsch 2011-09-15 11:31:46 UTC
(In reply to comment #1)
> I don't have Debian machine to test it, so I need your input.
> 
> Do you mean that this is wrong?
> 
> # busybox start-stop-daemon -S -x id
> uid=0(root) gid=0(root) groups=0(root),1(bin),2(daemon),3(sys)
> 
> # busybox start-stop-daemon -S -c 0:111 -x id
> uid=0(root) gid=111 groups=0(root),1(bin),2(daemon),3(sys)
>                     ^^^groups should be reset to 111 too!^^^^
> 
> 
> Correct?

Not reset, but setup according to groups file. Plus the specified as gid.
See initgroups(3) for reference.

Suppose you've got such a setup:
  /etc/passwd
    appuser:x:500:500::/home/appuser:/bin/false
  /etc/group
    dialout:x:106:appuser,otheruser
    somegroup:x:111:irrelevantuser
    audio:x:114:appuser,anotheruser
    appgroup:x:500:

Starting something as "appuser" has to setup his additional groups (106,114), too. And as gid the one you gave as group argument to -c.

With the patch, the outcome will be (taken and adapted from live system)
# busybox start-stop-daemon -S -c 500:111 -x /usr/bin/id
uid=500(appgroup) gid=111(somegroup) groups=106(dialout),114(audio)

Without, groups would be empty (iirc, but would make sense), making start-stop-daemon partly useless for finer-grained group setups.

BTW, patch works without problems for months in a live system, albeit comments still apply.
Comment 3 Denys Vlasenko 2011-09-15 13:03:19 UTC
(In reply to comment #2)
> Not reset, but setup according to groups file. Plus the specified as gid.
> See initgroups(3) for reference.

Did you confirm that standard Debian's start-stop-daemon does that?

Specifically, start-stop-daemon -c user1:1234 will still set user1's supplementary groups, and will add group 1234 to them?

I'm asking because it's not the only one sensible behavior. Clearly,

start-stop-daemon -c user1

should set uid to user1's uid, gid to user1's gid, and set his supplementary groups too. But

start-stop-daemon -c user1:1234

may be interpreted as "set uid to user1's uid, set gid to 1234, and DONT set user1's supplementary groups (because we explicitly say that group should be 1234, not the user1's usual one)!"

To me, both interpretations make sense.

Therefore, the question is: what does standard Debian's start-stop-daemon do?
Comment 4 Denys Vlasenko 2011-09-15 16:27:44 UTC
Since current code is wrong for sure, for the time being (before I get info about "standard" behavior) I'm going to change -c [USER][:GROUP] handling as follows:

if (opt & OPT_c) {
        struct bb_uidgid_t ugid = { -1, -1 };
        parse_chown_usergroup_or_die(&ugid, chuid);
        if (ugid.uid != (uid_t) -1) {
                struct passwd *pw = xgetpwuid(ugid.uid);
                if (ugid.gid != (gid_t) -1)
                        pw->pw_gid = ugid.gid;
                /* initgroups, setgid, setuid: */
                change_identity(pw);
        } else if (ugid.gid != (gid_t) -1) {
                xsetgid(ugid.gid);
                setgroups(1, &ugid.gid);
        }
}
Comment 5 Denys Vlasenko 2011-09-15 16:28:14 UTC
Fixed in git:

commit 585541e8e338a85b9f18cf5f6ed88758b29e61f2
Author: Denys Vlasenko <vda.linux@googlemail.com>
Date:   Thu Sep 15 18:27:05 2011 +0200

    start_stop_daemon: set complementary group ids too. Closes 3253
Comment 6 Andreas Pretzsch 2011-09-15 17:06:44 UTC
(In reply to comment #3)
> Did you confirm that standard Debian's start-stop-daemon does that?

Well, yes. More or less, minus a bug.
Just verified on latest Debian stable 6.0.2 (vanilla live image) and also on current Debian testing (dpkg 1.16.0.3).

It'll set the supplemental groups as described in the man page. But only if the username is given in cleartext, not when passed as number.

user@debian:~$ dpkg -s dpkg | grep Version
Version: 1.15.8.11
user@debian:~$ grep "user" /etc/passwd
user:x:1000:1000:Debian Live user,,,:/home/user:/bin/bash
user@debian:~$ grep "user" /etc/group
cdrom:x:24:user
floppy:x:25:user
audio:x:29:user
dip:x:30:user
video:x:44:user
plugdev:x:46:user
users:x:100:
user:x:1000:
user@debian:~$ sudo -i
root@debian:~# start-stop-daemon -S -c user:dip -x /usr/bin/id
uid=1000(user) gid=30(dip) groups=1000(user),24(cdrom),25(floppy),29(audio),30(dip),44(video),46(plugdev)
root@debian:~# start-stop-daemon -S -c user:30 -x /usr/bin/id
uid=1000(user) gid=30(dip) groups=1000(user),24(cdrom),25(floppy),29(audio),30(dip),44(video),46(plugdev)
root@debian:~# start-stop-daemon -S -c 1000:dip -x /usr/bin/id
uid=1000(user) gid=30(dip) groups=1000(user),30(dip)
root@debian:~# start-stop-daemon -S -c 1000:30 -x /usr/bin/id
uid=1000(user) gid=30(dip) groups=1000(user),30(dip)
root@debian:~# 

Tracking it down in the source, the culprit is the call to initgroups().
It's defined as "int initgroups(const char *user, gid_t group)", but apparently only works with cleartext usernames, but not with an uid passed as text. Which is what happens in the latter two cases (-c 1000:whatever).
Unfortunately, initgroups() still returns 0 for success in this case...

So far, no bug report found in Debian BTS. Will file one later, as time permits.


> Specifically, start-stop-daemon -c user1:1234 will still set user1's
> supplementary groups, and will add group 1234 to them?

Not exactly. It'll set 1234 as the main gid. And feed the remaining gids as suppl. groups to the kernel (resp. the process context).

 
> I'm asking because it's not the only one sensible behavior. Clearly,
> 
> start-stop-daemon -c user1
> 
> should set uid to user1's uid, gid to user1's gid, and set his supplementary
> groups too. But
> 
> start-stop-daemon -c user1:1234
> 
> may be interpreted as "set uid to user1's uid, set gid to 1234, and DONT set
> user1's supplementary groups (because we explicitly say that group should be
> 1234, not the user1's usual one)!"

Good point, but no, they're always set, even with explicit specified additional group the user is not in:

root@debian:~# start-stop-daemon -S -c user -x /usr/bin/id
uid=1000(user) gid=1000(user) groups=1000(user),24(cdrom),25(floppy),29(audio),30(dip),44(video),46(plugdev)
root@debian:~# start-stop-daemon -S -c user:dip -x /usr/bin/id
uid=1000(user) gid=30(dip) groups=1000(user),24(cdrom),25(floppy),29(audio),30(dip),44(video),46(plugdev)
root@debian:~# start-stop-daemon -S -c user:nogroup -x /usr/bin/id
uid=1000(user) gid=65534(nogroup) groups=1000(user),24(cdrom),25(floppy),29(audio),30(dip),44(video),46(plugdev),65534(nogroup)
root@debian:~# start-stop-daemon -S -c user -g nogroup -x /usr/bin/id
uid=1000(user) gid=65534(nogroup) groups=1000(user),24(cdrom),25(floppy),29(audio),30(dip),44(video),46(plugdev),65534(nogroup)
root@debian:~# start-stop-daemon -S -c user:dip -g nogroup -x /usr/bin/id
uid=1000(user) gid=65534(nogroup) groups=1000(user),24(cdrom),25(floppy),29(audio),30(dip),44(video),46(plugdev),65534(nogroup)
root@debian:~# 

 
> To me, both interpretations make sense.

True. Looks like the latter one (no suppl. groups) is not possible, despite there may be a use for that.
N.B.: Not sure if this is possible at all, as the user is still listed in the remaining groups (wrt to /etc/group) and therefore might switch/add group membership later on. Depending on the various effective/saved uid/gid. Honestly, I don't want to dig into this too deep right now...


> Therefore, the question is: what does standard Debian's start-stop-daemon do?

Adding the suppl. groups, as proposed.
As busybox will do with my patch. Of course minus the numerical-uid bug on Debian side. Which is not present in busybox resp. my patch, as there the textual name is used.

=> I'd suggest to merge in my original patch, maybe after some proof-reading.
Comment 7 Denys Vlasenko 2011-09-16 09:51:41 UTC
(In reply to comment #6)
> => I'd suggest to merge in my original patch, maybe after some proof-reading.

I already did it - see comment 4.
Comment 8 Andreas Pretzsch 2011-09-16 16:33:32 UTC
(In reply to comment #7)
> (In reply to comment #6)
> > => I'd suggest to merge in my original patch, maybe after some proof-reading.
> 
> I already did it - see comment 4.

Thanks. Was interrupted while writing my answer, so I missed that one.
Didn't test it, but I'm sure I don't need, either ;-)
=> bug closed (as it already is).

Just for reference:
(In reply to comment #6)
> So far, no bug report found in Debian BTS. Will file one later, as time permits.

Done, Debian bug #641834 (http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=641834).