[PATCH] libbb: Only setgid/setuid when necessary

[Denys Vlasenko]

On Sun, May 21, 2017 at 12:57 AM, Steven McDonald
<steven at steven-mcdonald.id.au> wrote:
> This makes 'unshare --user' work correctly in the case where the user's
> shell is provided by busybox itself.
>
> 'unshare --user' creates a new user namespace without any uid mappings.
> As a result, /bin/busybox is setuid nobody:nogroup within the
> namespace, as that is the only user. However, since no uids are mapped,
> attempting to call setgid/setuid fails, even though this would do
> nothing:
>
>   $ unshare --user ./busybox.broken ash
>   ash: setgid: Invalid argument
>
> 'unshare --map-root-user' still works, but because Linux only allows
> uid/gid mappings to be set up once, creating a root mapping makes such
> a namespace useless for creating multi-user containers.
>
> With this patch, setgid and setuid will not be called in the case where
> they would do nothing, which is always the case inside a new user
> namespace because all uids are effectively mapped to nobody:
>
>   $ id -u
>   1000
>   $ ls -lh busybox.fixed
>   -rwsr-xr-x    1 root     root      826.2K May 21 00:33 busybox.fixed
>   $ unshare --user ./busybox.fixed ash
>   $ id -u
>   65534
> ---
>  libbb/appletlib.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/libbb/appletlib.c b/libbb/appletlib.c
> index 7f0d62060..25831a420 100644
> --- a/libbb/appletlib.c
> +++ b/libbb/appletlib.c
> @@ -670,8 +670,11 @@ static void check_suid(int applet_no)
>                 if (geteuid())
>                         bb_error_msg_and_die("must be suid to work properly");
>         } else if (APPLET_SUID(applet_no) == BB_SUID_DROP) {
> -               xsetgid(rgid);  /* drop all privileges */
> -               xsetuid(ruid);
> +               /* drop all privileges */
> +               if (getegid() != rgid)
> +                       xsetgid(rgid);
> +               if (geteuid() != ruid)
> +                       xsetuid(ruid);
>         }
>  #  if ENABLE_FEATURE_SUID_CONFIG
>   ret: ;
> --

How about this then?

                /*
                 * Drop all privileges.
                 *
                 * Don't check for errors: in normal use, they are impossible,
                 * and in special cases, exiting is harmful. Example:
                 * 'unshare --user' when user's shell is also from busybox.
                 *
                 * 'unshare --user' creates a new user namespace without any
                 * uid mappings. Thus, busybox binary is setuid nobody:nogroup
                 * within the namespace, as that is the only user. However,
                 * since no uids are mapped, calls to setgid/setuid
                 * fail (even though they would do nothing).
                 */
                setgid(rgid);
                setuid(ruid);