[PATCH] Setting uid / gid is generally useful in nseneter

[PATCH] Setting uid / gid is generally useful in nseneter

[bobtfish]

From: Tomas Doran <bobtfish@bobtfish.net>

It's useful to be able to set the UID/GID even when not using user namespaces
(for example when creating a non-root shell in a pre-existing docker container)

Signed-off-by: Tomas Doran <bobtfish@bobtfish.net>
---
 sys-utils/nsenter.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sys-utils/nsenter.c b/sys-utils/nsenter.c
index d57edc8..23798f9 100644
--- a/sys-utils/nsenter.c
+++ b/sys-utils/nsenter.c
@@ -328,7 +328,7 @@ int main(int argc, char *argv[])
 	if (do_fork == 1)
 		continue_as_child();
 
-	if (namespaces & CLONE_NEWUSER) {
+	if (uid > 0 || gid > 0) {
 		if (setgroups(0, NULL))		/* drop supplementary groups */
 			err(EXIT_FAILURE, _("setgroups failed"));
 		if (setgid(gid) < 0)
-- 
2.0.1

Re: [PATCH] Setting uid / gid is generally useful in nseneter

[Karel Zak]

On Sat, Jul 26, 2014 at 01:22:54PM -0700, bobtfish@bobtfish.net wrote:
> It's useful to be able to set the UID/GID even when not using user namespaces
> (for example when creating a non-root shell in a pre-existing docker container)
> 
> Signed-off-by: Tomas Doran <bobtfish@bobtfish.net>
> ---
>  sys-utils/nsenter.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sys-utils/nsenter.c b/sys-utils/nsenter.c
> index d57edc8..23798f9 100644
> --- a/sys-utils/nsenter.c
> +++ b/sys-utils/nsenter.c
> @@ -328,7 +328,7 @@ int main(int argc, char *argv[])
>  	if (do_fork == 1)
>  		continue_as_child();
>  
> -	if (namespaces & CLONE_NEWUSER) {
> +	if (uid > 0 || gid > 0) {

 Well, it breaks the current behavior (the default for CLONE_NEWUSER
 is UID=0 and GID=0).


 The question is this is the right direction, because I guess that the next 
 patch for nsenter(1) will be "please, add supplementary groups support" ;-)

 Maybe the best will be to add to su(1) support for namespaces, something 
 like:

   su --ns <pid>[:mount,uts,ipc,net,pid,user]

 to enter namespaces after authenticate (if required) and before
 identity change. Not sure how huge is this Pandora's box, but it's
 definitely the final solution for all the requirements, because su(1)
 already supports all the UID/GID related features.

 Eric, any note?

   Karel


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] Setting uid / gid is generally useful in nseneter

[Eric W. Biederman]

Karel Zak <kzak@redhat.com> writes:

> On Sat, Jul 26, 2014 at 01:22:54PM -0700, bobtfish@bobtfish.net wrote:
>> It's useful to be able to set the UID/GID even when not using user namespaces
>> (for example when creating a non-root shell in a pre-existing docker container)
>>
>> Signed-off-by: Tomas Doran <bobtfish@bobtfish.net>
>> ---
>>  sys-utils/nsenter.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/sys-utils/nsenter.c b/sys-utils/nsenter.c
>> index d57edc8..23798f9 100644
>> --- a/sys-utils/nsenter.c
>> +++ b/sys-utils/nsenter.c
>> @@ -328,7 +328,7 @@ int main(int argc, char *argv[])
>>  	if (do_fork == 1)
>>  		continue_as_child();
>>  
>> -	if (namespaces & CLONE_NEWUSER) {
>> +	if (uid > 0 || gid > 0) {
>
>  Well, it breaks the current behavior (the default for CLONE_NEWUSER
>  is UID=0 and GID=0).
>
>  The question is this is the right direction, because I guess that the next 
>  patch for nsenter(1) will be "please, add supplementary groups
>  support" ;-)
>
>  Maybe the best will be to add to su(1) support for namespaces, something 
>  like:
>
>    su --ns <pid>[:mount,uts,ipc,net,pid,user]
>
>  to enter namespaces after authenticate (if required) and before
>  identity change. Not sure how huge is this Pandora's box, but it's
>  definitely the final solution for all the requirements, because su(1)
>  already supports all the UID/GID related features.
>
>  Eric, any note?

Thinking about it.

For my own use I believe I am still using an earlier version of nsenter
that does not even call setuid or setgid.

Thinking out loud.  Calling setuid(0) and setgid(0) in nsenter solves a
very practical problem of losing all capabilities on exec (because your
uid != 0).  Adding --setuid and --setgid is necessary because some
user namespaces do not have uid 0 or gid 0 mapped so switching to
those uids and gids would fail.

At the end of the day nsenter needs to be a simple tool, that is easy
to understand, because it is raw tool exposing the what the kernel
provides more or less directly without any significant abstractions.

Now it is a fact that nsenter must be run by root a process with
CAP_SYS_ADMIN or it must enter a user namespace.  Otherwise nsenter
does not have the privileges to do someting.  So assuming we have
CAP_SETUID or CAP_SETGID also is reasonable.

Hmm.

I think su (in general) is a bad fit.  There are two reasons.  nsenter
is not setuid and would not benefit from being so, most containers are
primarily managed as a separate system, and so the configuration in /etc
would not apply.

In general I would not trust the global root to run in a container 
so there is sense in the direction of this patch.  Especially if
su does not exist in the destination which makes chaining
nsenter and su difficult.

....

So the direction I could see this nsenter evolving is reading the
default uid and gid from the target process (A pain when in the case of
user namespaces because the value uid and gid will need to be mapped
into the target user namespace).

Likewise we could if it matters read the supplementary groups from from
the target process.  As a practical matter I don't think supplementary
groups are particularly interesting in simple cases and containers tend
to be simple cases.  So simply clearing the supplementary group ids
should be sufficient.

What I can see as productive (although possibly too much work) is to
allow setting the control groups and the security module identifiers to
that of the target process, or specified explicitly as we do with
namespaces now.

Ultimately nsenter should remain the simple raw tool that is gives you
raw access to what the kernel implements (without too much pain) so that
you can do things that whatever framework you are in does not implement.

...

So my gut feel about this patch it that we should add a uid/gid unset
state and generally implement this patch.  As the code is simple
and it implements a use case that quite hard to implement any other
way.

Eric

Re: [PATCH] Setting uid / gid is generally useful in nseneter

[Karel Zak]

On Mon, Jul 28, 2014 at 12:24:58PM -0700, Eric W. Biederman wrote:
> Thinking out loud.  Calling setuid(0) and setgid(0) in nsenter solves a
> very practical problem of losing all capabilities on exec (because your
> uid != 0).  Adding --setuid and --setgid is necessary because some
> user namespaces do not have uid 0 or gid 0 mapped so switching to
> those uids and gids would fail.

 OK, I have improved --setuid and --setguid, so now it's usable for
 all namespaces (originally it was only for user namespaces). The
 default is still 0 for user namepaces..
 
 The patch is below.

> So the direction I could see this nsenter evolving is reading the
> default uid and gid from the target process (A pain when in the case of

 That's nice idea, added to TODO.

> user namespaces because the value uid and gid will need to be mapped
> into the target user namespace).

 yep

    Karel

>From 47f42c1d14901b8bc2eb477fb0082bcbdd5e79af Mon Sep 17 00:00:00 2001
From: Karel Zak <kzak@redhat.com>
Date: Tue, 29 Jul 2014 13:07:44 +0200
Subject: [PATCH] nsenter: allow to use --set{uid,gid} for all namespaces

Now it's possible to set UID and GID for user namespaces only. This
patch removes this restriction and allow to use --set{uid,gid} in all
cases. The default for user namespaces is still GID=0, UID=0.

Reported-by: Tomas Doran <bobtfish@bobtfish.net>
Signed-off-by: Karel Zak <kzak@redhat.com>
---
 sys-utils/nsenter.1 |  9 +++++++--
 sys-utils/nsenter.c | 20 +++++++++++++-------
 2 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/sys-utils/nsenter.1 b/sys-utils/nsenter.1
index 998fd0c..487f731 100644
--- a/sys-utils/nsenter.1
+++ b/sys-utils/nsenter.1
@@ -126,10 +126,15 @@ the target process.  If file is specified, enter the user namespace specified by
 file.  See also the \fB\-\-setuid\fR and \fB\-\-setgid\fR options.
 .TP
 \fB\-G\fR, \fB\-\-setgid\fR \fIgid\fR
-Set the group ID which will be used in the entered user namespace.
+Set the group ID which will be used in the entered namespace and drop
+supplementary groups.
+.BR nsenter (1)
+always sets GID for user namespaces, the default is 0.
 .TP
 \fB\-S\fR, \fB\-\-setuid\fR \fIuid\fR
-Set the user ID which will be used in the entered user namespace.
+Set the user ID which will be used in the entered namespace.
+.BR nsenter (1)
+always sets UID for user namespaces, the default is 0.
 .TP
 \fB\-r\fR, \fB\-\-root\fR[=\fIdirectory\fR]
 Set the root directory.  If no directory is specified, set the root directory to
diff --git a/sys-utils/nsenter.c b/sys-utils/nsenter.c
index d57edc8..87c6549 100644
--- a/sys-utils/nsenter.c
+++ b/sys-utils/nsenter.c
@@ -73,8 +73,8 @@ static void usage(int status)
 	fputs(_(" -n, --net   [=<file>]  enter network namespace\n"), out);
 	fputs(_(" -p, --pid   [=<file>]  enter pid namespace\n"), out);
 	fputs(_(" -U, --user  [=<file>]  enter user namespace\n"), out);
-	fputs(_(" -S, --setuid <uid>     set uid in user namespace\n"), out);
-	fputs(_(" -G, --setgid <gid>     set gid in user namespace\n"), out);
+	fputs(_(" -S, --setuid <uid>     set uid in entered namespace\n"), out);
+	fputs(_(" -G, --setgid <gid>     set gid in entered namespace\n"), out);
 	fputs(_(" -r, --root  [=<dir>]   set the root directory\n"), out);
 	fputs(_(" -w, --wd    [=<dir>]   set the working directory\n"), out);
 	fputs(_(" -F, --no-fork          do not fork before exec'ing <program>\n"), out);
@@ -182,7 +182,7 @@ int main(int argc, char *argv[])
 
 	struct namespace_file *nsfile;
 	int c, namespaces = 0;
-	bool do_rd = false, do_wd = false;
+	bool do_rd = false, do_wd = false, force_uid = false, force_gid = false;
 	int do_fork = -1; /* unknown yet */
 	uid_t uid = 0;
 	gid_t gid = 0;
@@ -243,9 +243,11 @@ int main(int argc, char *argv[])
 			break;
 		case 'S':
 			uid = strtoul_or_err(optarg, _("failed to parse uid"));
+			force_uid = true;
 			break;
 		case 'G':
 			gid = strtoul_or_err(optarg, _("failed to parse gid"));
+			force_gid = true;
 			break;
 		case 'F':
 			do_fork = 0;
@@ -328,12 +330,16 @@ int main(int argc, char *argv[])
 	if (do_fork == 1)
 		continue_as_child();
 
-	if (namespaces & CLONE_NEWUSER) {
-		if (setgroups(0, NULL))		/* drop supplementary groups */
+	/* for user namespaces we always set UID and GID (default is 0) */
+	if (namespaces & CLONE_NEWUSER)
+		force_uid = true, force_gid = true;
+
+	if (force_uid || force_gid) {
+		if (force_gid && setgroups(0, NULL))		/* drop supplementary groups */
 			err(EXIT_FAILURE, _("setgroups failed"));
-		if (setgid(gid) < 0)
+		if (force_gid && setgid(gid) < 0)		/* change GID */
 			err(EXIT_FAILURE, _("setgid failed"));
-		if (setuid(uid) < 0)
+		if (force_uid && setuid(uid) < 0)		/* change UID */
 			err(EXIT_FAILURE, _("setuid failed"));
 	}
 
-- 
1.9.3