Unprivileged containers and co-ordinating user namespaces

Unprivileged containers and co-ordinating user namespaces

[James Bottomley]

It's always been annoying to me that we never co-ordinate our use of
namespace resources, but it's never really mattered until the user
namespace came along because namespaces didn't overlap and the only
annoyance was not being able to use existing tools to manipulate other
containers (mainly not being able to us ip netns).

However, with the user namespace, it's become necessary to co-ordinate
if you're giving users a range of uids beyond their own because you
don't want to have two separate container users owning overlapping uid
numbers (especially if they're unprivileged) because that will lead to
all sorts of security issues.  I think we need two things: a file
describing this for other things (like container orchestration systems
that want to know) and a mechanism for delegating the alloted uids to
the user.  One possible way of doing this would be to have the init
system set up the correctly owned user namespace at boot time.  It's
appealing to have the user sort out their own administration by simply
spawning new child user namespaces, but it adds the complexity that we
have to know what we're mapping to inside the namespace, whereas all
the administrator really cares about is what exterior uid range is
allocated (and that it remain that same range between reboots, because
these are the uids that's going to appear on disk).

Assuming everyone agrees it's a file and a utility, I'd propose the
file be

/etc/usernamespaces

and the format be <user>:<start>:<length>:<flags>

For the allocated uids.  <user>,<start> and <length> are obvious but
<flags> would be used for things like deny setgroups and possibly other
privilege reductions.

Then we need a utility, say userns that has cap_setuid+cap_setgid and
takes as an argument the raw uid_map, gid_map the user wants to install
plus similar arguments to unshare.  It then validates that the exterior
range are allowed by /etc/usernamespaces and sets up the usernamespace
with that range owned by the invoking user.

Container orchestration systems can either register their (probably
huge) ranges in the file, or simply use the file to know what ranges to
avoid.

If this sounds OK to people, I can code up a utility that does this,
which should probably belong in util-linux.

James

Unprivileged containers and co-ordinating user namespaces

[James Bottomley]

It's always been annoying to me that we never co-ordinate our use of
namespace resources, but it's never really mattered until the user
namespace came along because namespaces didn't overlap and the only
annoyance was not being able to use existing tools to manipulate other
containers (mainly not being able to us ip netns).

However, with the user namespace, it's become necessary to co-ordinate
if you're giving users a range of uids beyond their own because you
don't want to have two separate container users owning overlapping uid
numbers (especially if they're unprivileged) because that will lead to
all sorts of security issues.  I think we need two things: a file
describing this for other things (like container orchestration systems
that want to know) and a mechanism for delegating the alloted uids to
the user.  One possible way of doing this would be to have the init
system set up the correctly owned user namespace at boot time.  It's
appealing to have the user sort out their own administration by simply
spawning new child user namespaces, but it adds the complexity that we
have to know what we're mapping to inside the namespace, whereas all
the administrator really cares about is what exterior uid range is
allocated (and that it remain that same range between reboots, because
these are the uids that's going to appear on disk).

Assuming everyone agrees it's a file and a utility, I'd propose the
file be

/etc/usernamespaces

and the format be <user>:<start>:<length>:<flags>

For the allocated uids.  <user>,<start> and <length> are obvious but
<flags> would be used for things like deny setgroups and possibly other
privilege reductions.

Then we need a utility, say userns that has cap_setuid+cap_setgid and
takes as an argument the raw uid_map, gid_map the user wants to install
plus similar arguments to unshare.  It then validates that the exterior
range are allowed by /etc/usernamespaces and sets up the usernamespace
with that range owned by the invoking user.

Container orchestration systems can either register their (probably
huge) ranges in the file, or simply use the file to know what ranges to
avoid.

If this sounds OK to people, I can code up a utility that does this,
which should probably belong in util-linux.

James

Re: Unprivileged containers and co-ordinating user namespaces

[W. Trevor King]

[-- Attachment #1.1: Type: text/plain, Size: 735 bytes --]

On Thu, Apr 28, 2016 at 03:02:08PM -0700, James Bottomley wrote:
> /etc/usernamespaces
> 
> and the format be <user>:<start>:<length>:<flags>
>
> …
> 
> If this sounds OK to people, I can code up a utility that does this,
> which should probably belong in util-linux.

This sounds a lot like shadow's newuidmap and newgidmap [1,2,3].

Cheers,
Trevor

[1]: https://github.com/shadow-maint/shadow/commit/673c2a6f9aa6c69588f4c1be08589b8d3475a520
[2]: http://man7.org/linux/man-pages/man1/newuidmap.1.html
[3]: http://man7.org/linux/man-pages/man5/subuid.5.html

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 205 bytes --]

_______________________________________________
Containers mailing list
Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linuxfoundation.org/mailman/listinfo/containers

Re: Unprivileged containers and co-ordinating user namespaces

[W. Trevor King]

[-- Attachment #1: Type: text/plain, Size: 735 bytes --]

On Thu, Apr 28, 2016 at 03:02:08PM -0700, James Bottomley wrote:
> /etc/usernamespaces
> 
> and the format be <user>:<start>:<length>:<flags>
>
> …
> 
> If this sounds OK to people, I can code up a utility that does this,
> which should probably belong in util-linux.

This sounds a lot like shadow's newuidmap and newgidmap [1,2,3].

Cheers,
Trevor

[1]: https://github.com/shadow-maint/shadow/commit/673c2a6f9aa6c69588f4c1be08589b8d3475a520
[2]: http://man7.org/linux/man-pages/man1/newuidmap.1.html
[3]: http://man7.org/linux/man-pages/man5/subuid.5.html

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Unprivileged containers and co-ordinating user namespaces

[James Bottomley]

[-- Attachment #1.1: Type: text/plain, Size: 1141 bytes --]

On Thu, 2016-04-28 at 16:00 -0700, W. Trevor King wrote:
> On Thu, Apr 28, 2016 at 03:02:08PM -0700, James Bottomley wrote:
> > /etc/usernamespaces
> > 
> > and the format be :::
> > 
> > …
> > 
> > If this sounds OK to people, I can code up a utility that does this,
> > which should probably belong in util-linux.
> 
> This sounds a lot like shadow's newuidmap and newgidmap [1,2,3].
> 
> Cheers,
> Trevor
> 
> [1]: https://github.com/shadow-maint/shadow/commit/673c2a6f9aa6c69588f4c1be08589b8d3475a520
> [2]: http://man7.org/linux/man-pages/man1/newuidmap.1.html
> [3]: http://man7.org/linux/man-pages/man5/subuid.5.html

I think that mostly works.  No-one's packaging it yet, which is why I
didn't notice.  It also looks like the build dependencies have vastly
expanded, so I can't get it to build in the build service yet.

It looks like the only addition it needs is the setgroups flag for
newgidmap, which the security people will need, so I can patch that. 
 Plus it's trying to install newgidmap/newuidmap as setuid root rather
than cap_setuid/cap_setgid, but that's fixable in the spec file.

James

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 205 bytes --]

_______________________________________________
Containers mailing list
Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linuxfoundation.org/mailman/listinfo/containers

Re: Unprivileged containers and co-ordinating user namespaces

[James Bottomley]

[-- Attachment #1: Type: text/plain, Size: 1141 bytes --]

On Thu, 2016-04-28 at 16:00 -0700, W. Trevor King wrote:
> On Thu, Apr 28, 2016 at 03:02:08PM -0700, James Bottomley wrote:
> > /etc/usernamespaces
> > 
> > and the format be :::
> > 
> > …
> > 
> > If this sounds OK to people, I can code up a utility that does this,
> > which should probably belong in util-linux.
> 
> This sounds a lot like shadow's newuidmap and newgidmap [1,2,3].
> 
> Cheers,
> Trevor
> 
> [1]: https://github.com/shadow-maint/shadow/commit/673c2a6f9aa6c69588f4c1be08589b8d3475a520
> [2]: http://man7.org/linux/man-pages/man1/newuidmap.1.html
> [3]: http://man7.org/linux/man-pages/man5/subuid.5.html

I think that mostly works.  No-one's packaging it yet, which is why I
didn't notice.  It also looks like the build dependencies have vastly
expanded, so I can't get it to build in the build service yet.

It looks like the only addition it needs is the setgroups flag for
newgidmap, which the security people will need, so I can patch that. 
 Plus it's trying to install newgidmap/newuidmap as setuid root rather
than cap_setuid/cap_setgid, but that's fixable in the spec file.

James

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: Unprivileged containers and co-ordinating user namespaces

[Serge E. Hallyn]

Quoting James Bottomley (James.Bottomley@HansenPartnership.com):
> On Thu, 2016-04-28 at 16:00 -0700, W. Trevor King wrote:
> > On Thu, Apr 28, 2016 at 03:02:08PM -0700, James Bottomley wrote:
> > > /etc/usernamespaces
> > > 
> > > and the format be :::
> > > 
> > > …
> > > 
> > > If this sounds OK to people, I can code up a utility that does this,
> > > which should probably belong in util-linux.
> > 
> > This sounds a lot like shadow's newuidmap and newgidmap [1,2,3].
> > 
> > Cheers,
> > Trevor
> > 
> > [1]: https://github.com/shadow-maint/shadow/commit/673c2a6f9aa6c69588f4c1be08589b8d3475a520
> > [2]: http://man7.org/linux/man-pages/man1/newuidmap.1.html
> > [3]: http://man7.org/linux/man-pages/man5/subuid.5.html
> 
> I think that mostly works.  No-one's packaging it yet, which is why I

https://packages.debian.org/jessie/uidmap
https://launchpad.net/ubuntu/yakkety/+package/uidmap
http://rpm.pbone.net/index.php3/stat/45/idpl/28763248/numer/1/nazwa/newuidmap

> didn't notice.  It also looks like the build dependencies have vastly
> expanded, so I can't get it to build in the build service yet.
> 
> It looks like the only addition it needs is the setgroups flag for
> newgidmap, which the security people will need, so I can patch that. 
>  Plus it's trying to install newgidmap/newuidmap as setuid root rather
> than cap_setuid/cap_setgid, but that's fixable in the spec file.

That would prevent it being installed inside user namespaces, until
the user namespaced file capabilities (see separate thread :) hit.

-serge

Re: Unprivileged containers and co-ordinating user namespaces

[W. Trevor King]

[-- Attachment #1.1: Type: text/plain, Size: 944 bytes --]

On Fri, Apr 29, 2016 at 10:53:03AM -0500, Serge E. Hallyn wrote:
> Quoting James Bottomley:
> > I think that mostly works.  No-one's packaging it yet, which is why I
> 
> https://packages.debian.org/jessie/uidmap
> https://launchpad.net/ubuntu/yakkety/+package/uidmap
> http://rpm.pbone.net/index.php3/stat/45/idpl/28763248/numer/1/nazwa/newuidmap

On the other hand, Gentoo is waiting on a clean release tarball [1].
It looks like 4.3-1 was missing a clean autoreconf [2]?  Has anything
been pushed since?

Cheers,
Trevor

[1]: https://bugs.gentoo.org/show_bug.cgi?id=580432#c3
[2]: https://lists.alioth.debian.org/pipermail/pkg-shadow-devel/2016-March/010924.html
     Subject: [Pkg-shadow-devel] new shadow release candidate
     Date: Tue Mar 22 18:58:55 UTC 2016

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 205 bytes --]

_______________________________________________
Containers mailing list
Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linuxfoundation.org/mailman/listinfo/containers

Re: Unprivileged containers and co-ordinating user namespaces

[W. Trevor King]

[-- Attachment #1: Type: text/plain, Size: 944 bytes --]

On Fri, Apr 29, 2016 at 10:53:03AM -0500, Serge E. Hallyn wrote:
> Quoting James Bottomley:
> > I think that mostly works.  No-one's packaging it yet, which is why I
> 
> https://packages.debian.org/jessie/uidmap
> https://launchpad.net/ubuntu/yakkety/+package/uidmap
> http://rpm.pbone.net/index.php3/stat/45/idpl/28763248/numer/1/nazwa/newuidmap

On the other hand, Gentoo is waiting on a clean release tarball [1].
It looks like 4.3-1 was missing a clean autoreconf [2]?  Has anything
been pushed since?

Cheers,
Trevor

[1]: https://bugs.gentoo.org/show_bug.cgi?id=580432#c3
[2]: https://lists.alioth.debian.org/pipermail/pkg-shadow-devel/2016-March/010924.html
     Subject: [Pkg-shadow-devel] new shadow release candidate
     Date: Tue Mar 22 18:58:55 UTC 2016

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Unprivileged containers and co-ordinating user namespaces

[Serge E. Hallyn]

Quoting W. Trevor King (wking-vJI2gpByivqcqzYg7KEe8g@public.gmane.org):
> On Fri, Apr 29, 2016 at 10:53:03AM -0500, Serge E. Hallyn wrote:
> > Quoting James Bottomley:
> > > I think that mostly works.  No-one's packaging it yet, which is why I
> > 
> > https://packages.debian.org/jessie/uidmap
> > https://launchpad.net/ubuntu/yakkety/+package/uidmap
> > http://rpm.pbone.net/index.php3/stat/45/idpl/28763248/numer/1/nazwa/newuidmap
> 
> On the other hand, Gentoo is waiting on a clean release tarball [1].
> It looks like 4.3-1 was missing a clean autoreconf [2]?  Has anything
> been pushed since?

Nope, I've not had time to push it and noone has offered a patch.

Maybe I'll find time next week.

> Cheers,
> Trevor
> 
> [1]: https://bugs.gentoo.org/show_bug.cgi?id=580432#c3
> [2]: https://lists.alioth.debian.org/pipermail/pkg-shadow-devel/2016-March/010924.html
>      Subject: [Pkg-shadow-devel] new shadow release candidate
>      Date: Tue Mar 22 18:58:55 UTC 2016
> 
> -- 
> This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
> For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

Re: Unprivileged containers and co-ordinating user namespaces

[Serge E. Hallyn]

Quoting W. Trevor King (wking@tremily.us):
> On Fri, Apr 29, 2016 at 10:53:03AM -0500, Serge E. Hallyn wrote:
> > Quoting James Bottomley:
> > > I think that mostly works.  No-one's packaging it yet, which is why I
> > 
> > https://packages.debian.org/jessie/uidmap
> > https://launchpad.net/ubuntu/yakkety/+package/uidmap
> > http://rpm.pbone.net/index.php3/stat/45/idpl/28763248/numer/1/nazwa/newuidmap
> 
> On the other hand, Gentoo is waiting on a clean release tarball [1].
> It looks like 4.3-1 was missing a clean autoreconf [2]?  Has anything
> been pushed since?

Nope, I've not had time to push it and noone has offered a patch.

Maybe I'll find time next week.

> Cheers,
> Trevor
> 
> [1]: https://bugs.gentoo.org/show_bug.cgi?id=580432#c3
> [2]: https://lists.alioth.debian.org/pipermail/pkg-shadow-devel/2016-March/010924.html
>      Subject: [Pkg-shadow-devel] new shadow release candidate
>      Date: Tue Mar 22 18:58:55 UTC 2016
> 
> -- 
> This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
> For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

Re: Unprivileged containers and co-ordinating user namespaces

[James Bottomley]

[-- Attachment #1: Type: text/plain, Size: 9250 bytes --]

On Fri, 2016-04-29 at 08:38 -0700, James Bottomley wrote:
> On Thu, 2016-04-28 at 16:00 -0700, W. Trevor King wrote:
> > On Thu, Apr 28, 2016 at 03:02:08PM -0700, James Bottomley wrote:
> > > /etc/usernamespaces
> > > 
> > > and the format be :::
> > > 
> > > …
> > > 
> > > If this sounds OK to people, I can code up a utility that does
> > > this,
> > > which should probably belong in util-linux.
> > 
> > This sounds a lot like shadow's newuidmap and newgidmap [1,2,3].
> > 
> > Cheers,
> > Trevor
> > 
> > [1]: https://github.com/shadow-maint/shadow/commit/673c2a6f9aa6c695
> > 88f4c1be08589b8d3475a520
> > [2]: http://man7.org/linux/man-pages/man1/newuidmap.1.html
> > [3]: http://man7.org/linux/man-pages/man5/subuid.5.html
> 
> I think that mostly works.  No-one's packaging it yet, which is why I
> didn't notice.  It also looks like the build dependencies have vastly
> expanded, so I can't get it to build in the build service yet.
> 
> It looks like the only addition it needs is the setgroups flag for
> newgidmap, which the security people will need, so I can patch that. 

How does this look for a patch?

James

---

diff --git a/lib/subordinateio.c b/lib/subordinateio.c
index 0d64a91..aa3c968 100644
--- a/lib/subordinateio.c
+++ b/lib/subordinateio.c
@@ -18,9 +18,10 @@ struct subordinate_range {
 	const char *owner;
 	unsigned long start;
 	unsigned long count;
+	const char *flags;
 };
 
-#define NFIELDS 3
+#define NFIELDS 4
 
 /*
  * subordinate_dup: create a duplicate range
@@ -44,6 +45,16 @@ static /*@null@*/ /*@only@*/void *subordinate_dup (const void *ent)
 		free(range);
 		return NULL;
 	}
+	if (rangeent->flags) {
+		range->flags = strdup (rangeent->flags);
+		if (NULL == range->flags) {
+			free((void *)range->owner);
+			free(range);
+			return NULL;
+		}
+	} else {
+		range->flags = NULL;
+	}
 	range->start = rangeent->start;
 	range->count = rangeent->count;
 
@@ -111,13 +122,17 @@ static void *subordinate_parse (const char *line)
 	 * There must be exactly NFIELDS colon separated fields or
 	 * the entry is invalid.  Also, fields must be non-blank.
 	 */
-	if (i != NFIELDS || *fields[0] == '\0' || *fields[1] == '\0' || *fields[2] == '\0')
+	if ((i != NFIELDS && i != NFIELDS -1) || *fields[0] == '\0' || *fields[1] == '\0' || *fields[2] == '\0')
 		return NULL;
 	range.owner = fields[0];
 	if (getulong (fields[1], &range.start) == 0)
 		return NULL;
 	if (getulong (fields[2], &range.count) == 0)
 		return NULL;
+	if (i == NFIELDS)
+		range.flags = fields[3];
+	else
+		range.flags = NULL;
 
 	return ⦥
 }
@@ -134,10 +149,11 @@ static int subordinate_put (const void *ent, FILE * file)
 {
 	const struct subordinate_range *range = ent;
 
-	return fprintf(file, "%s:%lu:%lu\n",
+	return fprintf(file, "%s:%lu:%lu:%s\n",
 			       range->owner,
 			       range->start,
-			       range->count) < 0 ? -1  : 0;
+			       range->count,
+			       range->flags ? range->flags : "") < 0 ? -1  : 0;
 }
 
 static struct commonio_ops subordinate_ops = {
@@ -295,7 +311,8 @@ static const struct subordinate_range *find_range(struct commonio_db *db,
  * Returns true if @owner is authorized to use the range, false otherwise.
  */
 static bool have_range(struct commonio_db *db,
-		       const char *owner, unsigned long start, unsigned long count)
+		       const char *owner, unsigned long start, unsigned long count,
+		       const char **flags)
 {
 	const struct subordinate_range *range;
 	unsigned long end;
@@ -309,8 +326,11 @@ static bool have_range(struct commonio_db *db,
 		unsigned long last; 
 
 		last = range->start + range->count - 1;
-		if (last >= (start + count - 1))
+		if (last >= (start + count - 1)) {
+			if (flags)
+				*flags = range->flags;
 			return true;
+		}
 
 		count = end - last;
 		start = last + 1;
@@ -430,7 +450,7 @@ static int add_range(struct commonio_db *db,
 	range.count = count;
 
 	/* See if the range is already present */
-	if (have_range(db, owner, start, count))
+	if (have_range(db, owner, start, count, NULL))
 		return 1;
 
 	/* Otherwise append the range */
@@ -585,7 +605,7 @@ bool sub_uid_assigned(const char *owner)
 
 bool have_sub_uids(const char *owner, uid_t start, unsigned long count)
 {
-	return have_range (&subordinate_uid_db, owner, start, count);
+	return have_range (&subordinate_uid_db, owner, start, count, NULL);
 }
 
 int sub_uid_add (const char *owner, uid_t start, unsigned long count)
@@ -659,9 +679,9 @@ int sub_gid_open (int mode)
 	return commonio_open (&subordinate_gid_db, mode);
 }
 
-bool have_sub_gids(const char *owner, gid_t start, unsigned long count)
+bool have_sub_gids(const char *owner, gid_t start, unsigned long count, const char **flags)
 {
-	return have_range(&subordinate_gid_db, owner, start, count);
+	return have_range(&subordinate_gid_db, owner, start, count, flags);
 }
 
 bool sub_gid_assigned(const char *owner)
diff --git a/lib/subordinateio.h b/lib/subordinateio.h
index a21d72b..7e47659 100644
--- a/lib/subordinateio.h
+++ b/lib/subordinateio.h
@@ -25,7 +25,7 @@ extern int sub_uid_remove (const char *owner, uid_t start, unsigned long count);
 extern uid_t sub_uid_find_free_range(uid_t min, uid_t max, unsigned long count);
 
 extern int sub_gid_close(void);
-extern bool have_sub_gids(const char *owner, gid_t start, unsigned long count);
+extern bool have_sub_gids(const char *owner, gid_t start, unsigned long count, const char **flags);
 extern bool sub_gid_file_present (void);
 extern bool sub_gid_assigned(const char *owner);
 extern int sub_gid_lock (void);
diff --git a/libmisc/idmapping.c b/libmisc/idmapping.c
index 0dce634..7f7de88 100644
--- a/libmisc/idmapping.c
+++ b/libmisc/idmapping.c
@@ -106,7 +106,6 @@ void write_mapping(int proc_dir_fd, int ranges, struct map_range *mappings,
 	struct map_range *mapping;
 	size_t bufsize;
 	char *buf, *pos;
-	int fd;
 
 	bufsize = ranges * ((ULONG_DIGITS  + 1) * 3);
 	pos = buf = xmalloc(bufsize);
@@ -128,13 +127,20 @@ void write_mapping(int proc_dir_fd, int ranges, struct map_range *mappings,
 	}
 
 	/* Write the mapping to the maping file */
+	write_proc(proc_dir_fd, map_file, buf, pos - buf);
+}
+
+void write_proc(int proc_dir_fd, const char *map_file, void *buf, int len)
+{
+	int fd;
+
 	fd = openat(proc_dir_fd, map_file, O_WRONLY);
 	if (fd < 0) {
 		fprintf(stderr, _("%s: open of %s failed: %s\n"),
 			Prog, map_file, strerror(errno));
 		exit(EXIT_FAILURE);
 	}
-	if (write(fd, buf, pos - buf) != (pos - buf)) {
+	if (write(fd, buf, len) != len) {
 		fprintf(stderr, _("%s: write to %s failed: %s\n"),
 			Prog, map_file, strerror(errno));
 		exit(EXIT_FAILURE);
diff --git a/libmisc/idmapping.h b/libmisc/idmapping.h
index 58d000f..c2cec38 100644
--- a/libmisc/idmapping.h
+++ b/libmisc/idmapping.h
@@ -39,6 +39,7 @@ struct map_range {
 extern struct map_range *get_map_ranges(int ranges, int argc, char **argv);
 extern void write_mapping(int proc_dir_fd, int ranges,
 	struct map_range *mappings, const char *map_file);
+extern void write_proc(int proc_dir_fd, const char *map_file, void *buf, int len);
 
 #endif /* _ID_MAPPING_H_ */
 
diff --git a/src/newgidmap.c b/src/newgidmap.c
index 451c6a6..8d64e3b 100644
--- a/src/newgidmap.c
+++ b/src/newgidmap.c
@@ -46,14 +46,14 @@
  */
 const char *Prog;
 
-static bool verify_range(struct passwd *pw, struct map_range *range)
+static bool verify_range(struct passwd *pw, struct map_range *range, const char **flags)
 {
 	/* An empty range is invalid */
 	if (range->count == 0)
 		return false;
 
 	/* Test /etc/subgid */
-	if (have_sub_gids(pw->pw_name, range->lower, range->count))
+	if (have_sub_gids(pw->pw_name, range->lower, range->count, flags))
 		return true;
 
 	/* Allow a process to map it's own gid */
@@ -64,14 +64,14 @@ static bool verify_range(struct passwd *pw, struct map_range *range)
 }
 
 static void verify_ranges(struct passwd *pw, int ranges,
-	struct map_range *mappings)
+			  struct map_range *mappings, const char **flags)
 {
 	struct map_range *mapping;
 	int idx;
 
 	mapping = mappings;
 	for (idx = 0; idx < ranges; idx++, mapping++) {
-		if (!verify_range(pw, mapping)) {
+		if (!verify_range(pw, mapping, flags)) {
 			fprintf(stderr, _( "%s: gid range [%lu-%lu) -> [%lu-%lu) not allowed\n"),
 				Prog,
 				mapping->upper,
@@ -103,6 +103,7 @@ int main(int argc, char **argv)
 	struct stat st;
 	struct passwd *pw;
 	int written;
+	const char *flags;
 
 	Prog = Basename (argv[0]);
 
@@ -177,7 +178,18 @@ int main(int argc, char **argv)
 	if (!mappings)
 		usage();
 
-	verify_ranges(pw, ranges, mappings);
+	verify_ranges(pw, ranges, mappings, &flags);
+
+	if (flags && strlen(flags) != 0) {
+		/* only allowed flag is currently deny */
+		if (strcmp(flags, "deny") == 0) {
+			write_proc(proc_dir_fd, "setgroups", "deny", strlen("deny"));
+		} else {
+			fprintf(stderr, _("%s: illegal flag in map file: %s\n"),
+				Prog, flags);
+			exit(EXIT_FAILURE);
+		}
+	}	
 
 	write_mapping(proc_dir_fd, ranges, mappings, "gid_map");
 	sub_gid_close();

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: Unprivileged containers and co-ordinating user namespaces

[Serge Hallyn]

> > It looks like the only addition it needs is the setgroups flag for
> > newgidmap, which the security people will need, so I can patch that. 
> 
> How does this look for a patch?

Certainly deny is a terrible keyword for this, given the somewhat convoluted case it represents.  Maybe lockgroups or nosetgroup?  And of course it'll need a clear explanation of what it does in the subgid manpage. But really i don't know that this can work the way you want it to.  Can you describe your use case?

The intent of setgroups is to keep *unprivileged* writers of gid_map from dropping a gid which is blocking a file access from the user.  But newgidmap runs suid root (or cap_gid when you are through :), so the user can trivially assign just one authorized gid, not the whole range, and bypass the acls that way.

At the very least you would want to enforce the use of a whole range in the lockgroup case, but even there i question it's value.

-Serge



> James
> 
> ---
> 
> diff --git a/lib/subordinateio.c b/lib/subordinateio.c
> index 0d64a91..aa3c968 100644
> --- a/lib/subordinateio.c
> +++ b/lib/subordinateio.c
> @@ -18,9 +18,10 @@ struct subordinate_range {
>       const char *owner;
>       unsigned long start;
>       unsigned long count;
> +    const char *flags;
>   };
>   
> -#define NFIELDS 3
> +#define NFIELDS 4
>   
>   /*
>     * subordinate_dup: create a duplicate range
> @@ -44,6 +45,16 @@ static /*@null@*/ /*@only@*/void *subordinate_dup
> (const void *ent)         free(range);
>           return NULL;
>       }
> +    if (rangeent->flags) {
> +        range->flags = strdup (rangeent->flags);
> +        if (NULL == range->flags) {
> +            free((void *)range->owner);
> +            free(range);
> +            return NULL;
> +        }
> +    } else {
> +        range->flags = NULL;
> +    }
>       range->start = rangeent->start;
>       range->count = rangeent->count;
>   
> @@ -111,13 +122,17 @@ static void *subordinate_parse (const char *line)
>        * There must be exactly NFIELDS colon separated fields or
>        * the entry is invalid.   Also, fields must be non-blank.
>        */
> -    if (i != NFIELDS || *fields[0] == '\0' || *fields[1] == '\0' ||
> *fields[2] == '\0') +    if ((i != NFIELDS && i != NFIELDS -1) ||
> *fields[0] == '\0' || *fields[1] == '\0' || *fields[2] == '\0')         return
> NULL;     range.owner = fields[0];
>       if (getulong (fields[1], &range.start) == 0)
>           return NULL;
>       if (getulong (fields[2], &range.count) == 0)
>           return NULL;
> +    if (i == NFIELDS)
> +        range.flags = fields[3];
> +    else
> +        range.flags = NULL;
>   
>       return ⦥
>   }
> @@ -134,10 +149,11 @@ static int subordinate_put (const void *ent, FILE
> * file) {
>       const struct subordinate_range *range = ent;
>   
> -    return fprintf(file, "%s:%lu:%lu\n",
> +    return fprintf(file, "%s:%lu:%lu:%s\n",
>                            range->owner,
>                            range->start,
> -                         range->count) < 0 ? -1   : 0;
> +                         range->count,
> +                         range->flags ? range->flags : "") < 0 ? -1   : 0;
>   }
>   
>   static struct commonio_ops subordinate_ops = {
> @@ -295,7 +311,8 @@ static const struct subordinate_range
> *find_range(struct commonio_db *db,   * Returns true if @owner is
> authorized to use the range, false otherwise.   */
>   static bool have_range(struct commonio_db *db,
> -                     const char *owner, unsigned long start, unsigned long count)
> +                     const char *owner, unsigned long start, unsigned long count,
> +                     const char **flags)
>   {
>       const struct subordinate_range *range;
>       unsigned long end;
> @@ -309,8 +326,11 @@ static bool have_range(struct commonio_db *db,
>           unsigned long last; 
>   
>           last = range->start + range->count - 1;
> -        if (last >= (start + count - 1))
> +        if (last >= (start + count - 1)) {
> +            if (flags)
> +                *flags = range->flags;
>               return true;
> +        }
>   
>           count = end - last;
>           start = last + 1;
> @@ -430,7 +450,7 @@ static int add_range(struct commonio_db *db,
>       range.count = count;
>   
>       /* See if the range is already present */
> -    if (have_range(db, owner, start, count))
> +    if (have_range(db, owner, start, count, NULL))
>           return 1;
>   
>       /* Otherwise append the range */
> @@ -585,7 +605,7 @@ bool sub_uid_assigned(const char *owner)
>   
>   bool have_sub_uids(const char *owner, uid_t start, unsigned long count)
>   {
> -    return have_range (&subordinate_uid_db, owner, start, count);
> +    return have_range (&subordinate_uid_db, owner, start, count, NULL);
>   }
>   
>   int sub_uid_add (const char *owner, uid_t start, unsigned long count)
> @@ -659,9 +679,9 @@ int sub_gid_open (int mode)
>       return commonio_open (&subordinate_gid_db, mode);
>   }
>   
> -bool have_sub_gids(const char *owner, gid_t start, unsigned long count)
> +bool have_sub_gids(const char *owner, gid_t start, unsigned long count,
> const char **flags) {
> -    return have_range(&subordinate_gid_db, owner, start, count);
> +    return have_range(&subordinate_gid_db, owner, start, count, flags);
>   }
>   
>   bool sub_gid_assigned(const char *owner)
> diff --git a/lib/subordinateio.h b/lib/subordinateio.h
> index a21d72b..7e47659 100644
> --- a/lib/subordinateio.h
> +++ b/lib/subordinateio.h
> @@ -25,7 +25,7 @@ extern int sub_uid_remove (const char *owner, uid_t
> start, unsigned long count); extern uid_t sub_uid_find_free_range(uid_t
> min, uid_t max, unsigned long count); 
>   extern int sub_gid_close(void);
> -extern bool have_sub_gids(const char *owner, gid_t start, unsigned long
> count); +extern bool have_sub_gids(const char *owner, gid_t start,
> unsigned long count, const char **flags); extern bool
> sub_gid_file_present (void); extern bool sub_gid_assigned(const char
> *owner); extern int sub_gid_lock (void);
> diff --git a/libmisc/idmapping.c b/libmisc/idmapping.c
> index 0dce634..7f7de88 100644
> --- a/libmisc/idmapping.c
> +++ b/libmisc/idmapping.c
> @@ -106,7 +106,6 @@ void write_mapping(int proc_dir_fd, int ranges,
> struct map_range *mappings,     struct map_range *mapping;
>       size_t bufsize;
>       char *buf, *pos;
> -    int fd;
>   
>       bufsize = ranges * ((ULONG_DIGITS   + 1) * 3);
>       pos = buf = xmalloc(bufsize);
> @@ -128,13 +127,20 @@ void write_mapping(int proc_dir_fd, int ranges,
> struct map_range *mappings,     }
>   
>       /* Write the mapping to the maping file */
> +    write_proc(proc_dir_fd, map_file, buf, pos - buf);
> +}
> +
> +void write_proc(int proc_dir_fd, const char *map_file, void *buf, int
> len) +{
> +    int fd;
> +
>       fd = openat(proc_dir_fd, map_file, O_WRONLY);
>       if (fd < 0) {
>           fprintf(stderr, _("%s: open of %s failed: %s\n"),
>               Prog, map_file, strerror(errno));
>           exit(EXIT_FAILURE);
>       }
> -    if (write(fd, buf, pos - buf) != (pos - buf)) {
> +    if (write(fd, buf, len) != len) {
>           fprintf(stderr, _("%s: write to %s failed: %s\n"),
>               Prog, map_file, strerror(errno));
>           exit(EXIT_FAILURE);
> diff --git a/libmisc/idmapping.h b/libmisc/idmapping.h
> index 58d000f..c2cec38 100644
> --- a/libmisc/idmapping.h
> +++ b/libmisc/idmapping.h
> @@ -39,6 +39,7 @@ struct map_range {
>   extern struct map_range *get_map_ranges(int ranges, int argc, char
> **argv); extern void write_mapping(int proc_dir_fd, int ranges,
>       struct map_range *mappings, const char *map_file);
> +extern void write_proc(int proc_dir_fd, const char *map_file, void
> *buf, int len); 
>   #endif /* _ID_MAPPING_H_ */
>   
> diff --git a/src/newgidmap.c b/src/newgidmap.c
> index 451c6a6..8d64e3b 100644
> --- a/src/newgidmap.c
> +++ b/src/newgidmap.c
> @@ -46,14 +46,14 @@
>     */
>   const char *Prog;
>   
> -static bool verify_range(struct passwd *pw, struct map_range *range)
> +static bool verify_range(struct passwd *pw, struct map_range *range,
> const char **flags) {
>       /* An empty range is invalid */
>       if (range->count == 0)
>           return false;
>   
>       /* Test /etc/subgid */
> -    if (have_sub_gids(pw->pw_name, range->lower, range->count))
> +    if (have_sub_gids(pw->pw_name, range->lower, range->count, flags))
>           return true;
>   
>       /* Allow a process to map it's own gid */
> @@ -64,14 +64,14 @@ static bool verify_range(struct passwd *pw, struct
> map_range *range) }
>   
>   static void verify_ranges(struct passwd *pw, int ranges,
> -    struct map_range *mappings)
> +               struct map_range *mappings, const char **flags)
>   {
>       struct map_range *mapping;
>       int idx;
>   
>       mapping = mappings;
>       for (idx = 0; idx < ranges; idx++, mapping++) {
> -        if (!verify_range(pw, mapping)) {
> +        if (!verify_range(pw, mapping, flags)) {
>               fprintf(stderr, _( "%s: gid range [%lu-%lu) -> [%lu-%lu) not
> allowed\n"),                 Prog,
>                   mapping->upper,
> @@ -103,6 +103,7 @@ int main(int argc, char **argv)
>       struct stat st;
>       struct passwd *pw;
>       int written;
> +    const char *flags;
>   
>       Prog = Basename (argv[0]);
>   
> @@ -177,7 +178,18 @@ int main(int argc, char **argv)
>       if (!mappings)
>           usage();
>   
> -    verify_ranges(pw, ranges, mappings);
> +    verify_ranges(pw, ranges, mappings, &flags);
> +
> +    if (flags && strlen(flags) != 0) {
> +        /* only allowed flag is currently deny */
> +        if (strcmp(flags, "deny") == 0) {
> +            write_proc(proc_dir_fd, "setgroups", "deny", strlen("deny"));
> +        } else {
> +            fprintf(stderr, _("%s: illegal flag in map file: %s\n"),
> +                Prog, flags);
> +            exit(EXIT_FAILURE);
> +        }
> +    }   
>   
>       write_mapping(proc_dir_fd, ranges, mappings, "gid_map");
>       sub_gid_close();

_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/containers

Re: Unprivileged containers and co-ordinating user namespaces

[Serge Hallyn]

> > It looks like the only addition it needs is the setgroups flag for
> > newgidmap, which the security people will need, so I can patch that. 
> 
> How does this look for a patch?

Certainly deny is a terrible keyword for this, given the somewhat convoluted case it represents.  Maybe lockgroups or nosetgroup?  And of course it'll need a clear explanation of what it does in the subgid manpage. But really i don't know that this can work the way you want it to.  Can you describe your use case?

The intent of setgroups is to keep *unprivileged* writers of gid_map from dropping a gid which is blocking a file access from the user.  But newgidmap runs suid root (or cap_gid when you are through :), so the user can trivially assign just one authorized gid, not the whole range, and bypass the acls that way.

At the very least you would want to enforce the use of a whole range in the lockgroup case, but even there i question it's value.

-Serge



> James
> 
> ---
> 
> diff --git a/lib/subordinateio.c b/lib/subordinateio.c
> index 0d64a91..aa3c968 100644
> --- a/lib/subordinateio.c
> +++ b/lib/subordinateio.c
> @@ -18,9 +18,10 @@ struct subordinate_range {
>       const char *owner;
>       unsigned long start;
>       unsigned long count;
> +    const char *flags;
>   };
>   
> -#define NFIELDS 3
> +#define NFIELDS 4
>   
>   /*
>     * subordinate_dup: create a duplicate range
> @@ -44,6 +45,16 @@ static /*@null@*/ /*@only@*/void *subordinate_dup
> (const void *ent)         free(range);
>           return NULL;
>       }
> +    if (rangeent->flags) {
> +        range->flags = strdup (rangeent->flags);
> +        if (NULL == range->flags) {
> +            free((void *)range->owner);
> +            free(range);
> +            return NULL;
> +        }
> +    } else {
> +        range->flags = NULL;
> +    }
>       range->start = rangeent->start;
>       range->count = rangeent->count;
>   
> @@ -111,13 +122,17 @@ static void *subordinate_parse (const char *line)
>        * There must be exactly NFIELDS colon separated fields or
>        * the entry is invalid.   Also, fields must be non-blank.
>        */
> -    if (i != NFIELDS || *fields[0] == '\0' || *fields[1] == '\0' ||
> *fields[2] == '\0') +    if ((i != NFIELDS && i != NFIELDS -1) ||
> *fields[0] == '\0' || *fields[1] == '\0' || *fields[2] == '\0')         return
> NULL;     range.owner = fields[0];
>       if (getulong (fields[1], &range.start) == 0)
>           return NULL;
>       if (getulong (fields[2], &range.count) == 0)
>           return NULL;
> +    if (i == NFIELDS)
> +        range.flags = fields[3];
> +    else
> +        range.flags = NULL;
>   
>       return ⦥
>   }
> @@ -134,10 +149,11 @@ static int subordinate_put (const void *ent, FILE
> * file) {
>       const struct subordinate_range *range = ent;
>   
> -    return fprintf(file, "%s:%lu:%lu\n",
> +    return fprintf(file, "%s:%lu:%lu:%s\n",
>                            range->owner,
>                            range->start,
> -                         range->count) < 0 ? -1   : 0;
> +                         range->count,
> +                         range->flags ? range->flags : "") < 0 ? -1   : 0;
>   }
>   
>   static struct commonio_ops subordinate_ops = {
> @@ -295,7 +311,8 @@ static const struct subordinate_range
> *find_range(struct commonio_db *db,   * Returns true if @owner is
> authorized to use the range, false otherwise.   */
>   static bool have_range(struct commonio_db *db,
> -                     const char *owner, unsigned long start, unsigned long count)
> +                     const char *owner, unsigned long start, unsigned long count,
> +                     const char **flags)
>   {
>       const struct subordinate_range *range;
>       unsigned long end;
> @@ -309,8 +326,11 @@ static bool have_range(struct commonio_db *db,
>           unsigned long last; 
>   
>           last = range->start + range->count - 1;
> -        if (last >= (start + count - 1))
> +        if (last >= (start + count - 1)) {
> +            if (flags)
> +                *flags = range->flags;
>               return true;
> +        }
>   
>           count = end - last;
>           start = last + 1;
> @@ -430,7 +450,7 @@ static int add_range(struct commonio_db *db,
>       range.count = count;
>   
>       /* See if the range is already present */
> -    if (have_range(db, owner, start, count))
> +    if (have_range(db, owner, start, count, NULL))
>           return 1;
>   
>       /* Otherwise append the range */
> @@ -585,7 +605,7 @@ bool sub_uid_assigned(const char *owner)
>   
>   bool have_sub_uids(const char *owner, uid_t start, unsigned long count)
>   {
> -    return have_range (&subordinate_uid_db, owner, start, count);
> +    return have_range (&subordinate_uid_db, owner, start, count, NULL);
>   }
>   
>   int sub_uid_add (const char *owner, uid_t start, unsigned long count)
> @@ -659,9 +679,9 @@ int sub_gid_open (int mode)
>       return commonio_open (&subordinate_gid_db, mode);
>   }
>   
> -bool have_sub_gids(const char *owner, gid_t start, unsigned long count)
> +bool have_sub_gids(const char *owner, gid_t start, unsigned long count,
> const char **flags) {
> -    return have_range(&subordinate_gid_db, owner, start, count);
> +    return have_range(&subordinate_gid_db, owner, start, count, flags);
>   }
>   
>   bool sub_gid_assigned(const char *owner)
> diff --git a/lib/subordinateio.h b/lib/subordinateio.h
> index a21d72b..7e47659 100644
> --- a/lib/subordinateio.h
> +++ b/lib/subordinateio.h
> @@ -25,7 +25,7 @@ extern int sub_uid_remove (const char *owner, uid_t
> start, unsigned long count); extern uid_t sub_uid_find_free_range(uid_t
> min, uid_t max, unsigned long count); 
>   extern int sub_gid_close(void);
> -extern bool have_sub_gids(const char *owner, gid_t start, unsigned long
> count); +extern bool have_sub_gids(const char *owner, gid_t start,
> unsigned long count, const char **flags); extern bool
> sub_gid_file_present (void); extern bool sub_gid_assigned(const char
> *owner); extern int sub_gid_lock (void);
> diff --git a/libmisc/idmapping.c b/libmisc/idmapping.c
> index 0dce634..7f7de88 100644
> --- a/libmisc/idmapping.c
> +++ b/libmisc/idmapping.c
> @@ -106,7 +106,6 @@ void write_mapping(int proc_dir_fd, int ranges,
> struct map_range *mappings,     struct map_range *mapping;
>       size_t bufsize;
>       char *buf, *pos;
> -    int fd;
>   
>       bufsize = ranges * ((ULONG_DIGITS   + 1) * 3);
>       pos = buf = xmalloc(bufsize);
> @@ -128,13 +127,20 @@ void write_mapping(int proc_dir_fd, int ranges,
> struct map_range *mappings,     }
>   
>       /* Write the mapping to the maping file */
> +    write_proc(proc_dir_fd, map_file, buf, pos - buf);
> +}
> +
> +void write_proc(int proc_dir_fd, const char *map_file, void *buf, int
> len) +{
> +    int fd;
> +
>       fd = openat(proc_dir_fd, map_file, O_WRONLY);
>       if (fd < 0) {
>           fprintf(stderr, _("%s: open of %s failed: %s\n"),
>               Prog, map_file, strerror(errno));
>           exit(EXIT_FAILURE);
>       }
> -    if (write(fd, buf, pos - buf) != (pos - buf)) {
> +    if (write(fd, buf, len) != len) {
>           fprintf(stderr, _("%s: write to %s failed: %s\n"),
>               Prog, map_file, strerror(errno));
>           exit(EXIT_FAILURE);
> diff --git a/libmisc/idmapping.h b/libmisc/idmapping.h
> index 58d000f..c2cec38 100644
> --- a/libmisc/idmapping.h
> +++ b/libmisc/idmapping.h
> @@ -39,6 +39,7 @@ struct map_range {
>   extern struct map_range *get_map_ranges(int ranges, int argc, char
> **argv); extern void write_mapping(int proc_dir_fd, int ranges,
>       struct map_range *mappings, const char *map_file);
> +extern void write_proc(int proc_dir_fd, const char *map_file, void
> *buf, int len); 
>   #endif /* _ID_MAPPING_H_ */
>   
> diff --git a/src/newgidmap.c b/src/newgidmap.c
> index 451c6a6..8d64e3b 100644
> --- a/src/newgidmap.c
> +++ b/src/newgidmap.c
> @@ -46,14 +46,14 @@
>     */
>   const char *Prog;
>   
> -static bool verify_range(struct passwd *pw, struct map_range *range)
> +static bool verify_range(struct passwd *pw, struct map_range *range,
> const char **flags) {
>       /* An empty range is invalid */
>       if (range->count == 0)
>           return false;
>   
>       /* Test /etc/subgid */
> -    if (have_sub_gids(pw->pw_name, range->lower, range->count))
> +    if (have_sub_gids(pw->pw_name, range->lower, range->count, flags))
>           return true;
>   
>       /* Allow a process to map it's own gid */
> @@ -64,14 +64,14 @@ static bool verify_range(struct passwd *pw, struct
> map_range *range) }
>   
>   static void verify_ranges(struct passwd *pw, int ranges,
> -    struct map_range *mappings)
> +               struct map_range *mappings, const char **flags)
>   {
>       struct map_range *mapping;
>       int idx;
>   
>       mapping = mappings;
>       for (idx = 0; idx < ranges; idx++, mapping++) {
> -        if (!verify_range(pw, mapping)) {
> +        if (!verify_range(pw, mapping, flags)) {
>               fprintf(stderr, _( "%s: gid range [%lu-%lu) -> [%lu-%lu) not
> allowed\n"),                 Prog,
>                   mapping->upper,
> @@ -103,6 +103,7 @@ int main(int argc, char **argv)
>       struct stat st;
>       struct passwd *pw;
>       int written;
> +    const char *flags;
>   
>       Prog = Basename (argv[0]);
>   
> @@ -177,7 +178,18 @@ int main(int argc, char **argv)
>       if (!mappings)
>           usage();
>   
> -    verify_ranges(pw, ranges, mappings);
> +    verify_ranges(pw, ranges, mappings, &flags);
> +
> +    if (flags && strlen(flags) != 0) {
> +        /* only allowed flag is currently deny */
> +        if (strcmp(flags, "deny") == 0) {
> +            write_proc(proc_dir_fd, "setgroups", "deny", strlen("deny"));
> +        } else {
> +            fprintf(stderr, _("%s: illegal flag in map file: %s\n"),
> +                Prog, flags);
> +            exit(EXIT_FAILURE);
> +        }
> +    }   
>   
>       write_mapping(proc_dir_fd, ranges, mappings, "gid_map");
>       sub_gid_close();

Re: Unprivileged containers and co-ordinating user namespaces

[James Bottomley]

On Sun, 2016-05-01 at 12:37 -0400, Serge Hallyn wrote:
> > > It looks like the only addition it needs is the setgroups flag
> > > for
> > > newgidmap, which the security people will need, so I can patch
> > > that. 
> > 
> > How does this look for a patch?
> 
> Certainly deny is a terrible keyword for this, given the somewhat
> convoluted case it represents.  Maybe lockgroups or nosetgroup?

OK, nosegroups seems to be less scary.

>   And of course it'll need a clear explanation of what it does in the
> subgid manpage. But really i don't know that this can work the way
> you want it to.

Yes, docs and tests were to be added after everyone was happy with the
implementation.

>   Can you describe your use case?

basically not getting a CVE filed against this utility for allowing the
dropping of groups.  I don't have a use case for group membership
denying permissions.

> The intent of setgroups is to keep *unprivileged* writers of gid_map
> from dropping a gid which is blocking a file access from the user. 


You can't do that with the map write.  Even if you set no gid_map at
all, the attributes checks are done on the kernel side groups (i.e. the
ones you possess outside the namespace even if they have no interior
mapping).  The only way to drop groups is with the setgroups() system
call.

>  But newgidmap runs suid root (or cap_gid when you are through :), so
> the user can trivially assign just one authorized gid, not the whole
> range, and bypass the acls that way.

It does more than that: it also blocks the sys_setgroups() call in the
namespace, even for the namespace owner, which is the primary CVE
preventing use case.

> At the very least you would want to enforce the use of a whole range
> in the lockgroup case, but even there i question it's value.

I think, if you were going to enforce deny sys_setgroups() on a user
you otherwise trusted, you'd probably only allow them a single range. 
 But I suppose the flag should be or'd over all ranges.

James


> -Serge
> 
> 
> 
> > James
> > 
> > ---
> > 
> > diff --git a/lib/subordinateio.c b/lib/subordinateio.c
> > index 0d64a91..aa3c968 100644
> > --- a/lib/subordinateio.c
> > +++ b/lib/subordinateio.c
> > @@ -18,9 +18,10 @@ struct subordinate_range {
> >        const char *owner;
> >        unsigned long start;
> >        unsigned long count;
> > +    const char *flags;
> >    };
> >    
> > -#define NFIELDS 3
> > +#define NFIELDS 4
> >    
> >    /*
> >      * subordinate_dup: create a duplicate range
> > @@ -44,6 +45,16 @@ static /*@null@*/ /*@only@*/void
> > *subordinate_dup
> > (const void *ent)         free(range);
> >            return NULL;
> >        }
> > +    if (rangeent->flags) {
> > +        range->flags = strdup (rangeent->flags);
> > +        if (NULL == range->flags) {
> > +            free((void *)range->owner);
> > +            free(range);
> > +            return NULL;
> > +        }
> > +    } else {
> > +        range->flags = NULL;
> > +    }
> >        range->start = rangeent->start;
> >        range->count = rangeent->count;
> >    
> > @@ -111,13 +122,17 @@ static void *subordinate_parse (const char
> > *line)
> >         * There must be exactly NFIELDS colon separated fields or
> >         * the entry is invalid.   Also, fields must be non-blank.
> >         */
> > -    if (i != NFIELDS || *fields[0] == '\0' || *fields[1] == '\0'
> > ||
> > *fields[2] == '\0') +    if ((i != NFIELDS && i != NFIELDS -1) ||
> > *fields[0] == '\0' || *fields[1] == '\0' || *fields[2] == '\0')    
> >     return
> > NULL;     range.owner = fields[0];
> >        if (getulong (fields[1], &range.start) == 0)
> >            return NULL;
> >        if (getulong (fields[2], &range.count) == 0)
> >            return NULL;
> > +    if (i == NFIELDS)
> > +        range.flags = fields[3];
> > +    else
> > +        range.flags = NULL;
> >    
> >        return ⦥
> >    }
> > @@ -134,10 +149,11 @@ static int subordinate_put (const void *ent,
> > FILE
> > * file) {
> >        const struct subordinate_range *range = ent;
> >    
> > -    return fprintf(file, "%s:%lu:%lu\n",
> > +    return fprintf(file, "%s:%lu:%lu:%s\n",
> >                             range->owner,
> >                             range->start,
> > -                         range->count) < 0 ? -1   : 0;
> > +                         range->count,
> > +                         range->flags ? range->flags : "") < 0 ? 
> > -1   : 0;
> >    }
> >    
> >    static struct commonio_ops subordinate_ops = {
> > @@ -295,7 +311,8 @@ static const struct subordinate_range
> > *find_range(struct commonio_db *db,   * Returns true if @owner is
> > authorized to use the range, false otherwise.   */
> >    static bool have_range(struct commonio_db *db,
> > -                     const char *owner, unsigned long start,
> > unsigned long count)
> > +                     const char *owner, unsigned long start,
> > unsigned long count,
> > +                     const char **flags)
> >    {
> >        const struct subordinate_range *range;
> >        unsigned long end;
> > @@ -309,8 +326,11 @@ static bool have_range(struct commonio_db *db,
> >            unsigned long last; 
> >    
> >            last = range->start + range->count - 1;
> > -        if (last >= (start + count - 1))
> > +        if (last >= (start + count - 1)) {
> > +            if (flags)
> > +                *flags = range->flags;
> >                return true;
> > +        }
> >    
> >            count = end - last;
> >            start = last + 1;
> > @@ -430,7 +450,7 @@ static int add_range(struct commonio_db *db,
> >        range.count = count;
> >    
> >        /* See if the range is already present */
> > -    if (have_range(db, owner, start, count))
> > +    if (have_range(db, owner, start, count, NULL))
> >            return 1;
> >    
> >        /* Otherwise append the range */
> > @@ -585,7 +605,7 @@ bool sub_uid_assigned(const char *owner)
> >    
> >    bool have_sub_uids(const char *owner, uid_t start, unsigned long
> > count)
> >    {
> > -    return have_range (&subordinate_uid_db, owner, start, count);
> > +    return have_range (&subordinate_uid_db, owner, start, count,
> > NULL);
> >    }
> >    
> >    int sub_uid_add (const char *owner, uid_t start, unsigned long
> > count)
> > @@ -659,9 +679,9 @@ int sub_gid_open (int mode)
> >        return commonio_open (&subordinate_gid_db, mode);
> >    }
> >    
> > -bool have_sub_gids(const char *owner, gid_t start, unsigned long
> > count)
> > +bool have_sub_gids(const char *owner, gid_t start, unsigned long
> > count,
> > const char **flags) {
> > -    return have_range(&subordinate_gid_db, owner, start, count);
> > +    return have_range(&subordinate_gid_db, owner, start, count,
> > flags);
> >    }
> >    
> >    bool sub_gid_assigned(const char *owner)
> > diff --git a/lib/subordinateio.h b/lib/subordinateio.h
> > index a21d72b..7e47659 100644
> > --- a/lib/subordinateio.h
> > +++ b/lib/subordinateio.h
> > @@ -25,7 +25,7 @@ extern int sub_uid_remove (const char *owner,
> > uid_t
> > start, unsigned long count); extern uid_t
> > sub_uid_find_free_range(uid_t
> > min, uid_t max, unsigned long count); 
> >    extern int sub_gid_close(void);
> > -extern bool have_sub_gids(const char *owner, gid_t start, unsigned
> > long
> > count); +extern bool have_sub_gids(const char *owner, gid_t start,
> > unsigned long count, const char **flags); extern bool
> > sub_gid_file_present (void); extern bool sub_gid_assigned(const
> > char
> > *owner); extern int sub_gid_lock (void);
> > diff --git a/libmisc/idmapping.c b/libmisc/idmapping.c
> > index 0dce634..7f7de88 100644
> > --- a/libmisc/idmapping.c
> > +++ b/libmisc/idmapping.c
> > @@ -106,7 +106,6 @@ void write_mapping(int proc_dir_fd, int ranges,
> > struct map_range *mappings,     struct map_range *mapping;
> >        size_t bufsize;
> >        char *buf, *pos;
> > -    int fd;
> >    
> >        bufsize = ranges * ((ULONG_DIGITS   + 1) * 3);
> >        pos = buf = xmalloc(bufsize);
> > @@ -128,13 +127,20 @@ void write_mapping(int proc_dir_fd, int
> > ranges,
> > struct map_range *mappings,     }
> >    
> >        /* Write the mapping to the maping file */
> > +    write_proc(proc_dir_fd, map_file, buf, pos - buf);
> > +}
> > +
> > +void write_proc(int proc_dir_fd, const char *map_file, void *buf,
> > int
> > len) +{
> > +    int fd;
> > +
> >        fd = openat(proc_dir_fd, map_file, O_WRONLY);
> >        if (fd < 0) {
> >            fprintf(stderr, _("%s: open of %s failed: %s\n"),
> >                Prog, map_file, strerror(errno));
> >            exit(EXIT_FAILURE);
> >        }
> > -    if (write(fd, buf, pos - buf) != (pos - buf)) {
> > +    if (write(fd, buf, len) != len) {
> >            fprintf(stderr, _("%s: write to %s failed: %s\n"),
> >                Prog, map_file, strerror(errno));
> >            exit(EXIT_FAILURE);
> > diff --git a/libmisc/idmapping.h b/libmisc/idmapping.h
> > index 58d000f..c2cec38 100644
> > --- a/libmisc/idmapping.h
> > +++ b/libmisc/idmapping.h
> > @@ -39,6 +39,7 @@ struct map_range {
> >    extern struct map_range *get_map_ranges(int ranges, int argc,
> > char
> > **argv); extern void write_mapping(int proc_dir_fd, int ranges,
> >        struct map_range *mappings, const char *map_file);
> > +extern void write_proc(int proc_dir_fd, const char *map_file, void
> > *buf, int len); 
> >    #endif /* _ID_MAPPING_H_ */
> >    
> > diff --git a/src/newgidmap.c b/src/newgidmap.c
> > index 451c6a6..8d64e3b 100644
> > --- a/src/newgidmap.c
> > +++ b/src/newgidmap.c
> > @@ -46,14 +46,14 @@
> >      */
> >    const char *Prog;
> >    
> > -static bool verify_range(struct passwd *pw, struct map_range
> > *range)
> > +static bool verify_range(struct passwd *pw, struct map_range
> > *range,
> > const char **flags) {
> >        /* An empty range is invalid */
> >        if (range->count == 0)
> >            return false;
> >    
> >        /* Test /etc/subgid */
> > -    if (have_sub_gids(pw->pw_name, range->lower, range->count))
> > +    if (have_sub_gids(pw->pw_name, range->lower, range->count,
> > flags))
> >            return true;
> >    
> >        /* Allow a process to map it's own gid */
> > @@ -64,14 +64,14 @@ static bool verify_range(struct passwd *pw,
> > struct
> > map_range *range) }
> >    
> >    static void verify_ranges(struct passwd *pw, int ranges,
> > -    struct map_range *mappings)
> > +               struct map_range *mappings, const char **flags)
> >    {
> >        struct map_range *mapping;
> >        int idx;
> >    
> >        mapping = mappings;
> >        for (idx = 0; idx < ranges; idx++, mapping++) {
> > -        if (!verify_range(pw, mapping)) {
> > +        if (!verify_range(pw, mapping, flags)) {
> >                fprintf(stderr, _( "%s: gid range [%lu-%lu) -> [%lu
> > -%lu) not
> > allowed\n"),                 Prog,
> >                    mapping->upper,
> > @@ -103,6 +103,7 @@ int main(int argc, char **argv)
> >        struct stat st;
> >        struct passwd *pw;
> >        int written;
> > +    const char *flags;
> >    
> >        Prog = Basename (argv[0]);
> >    
> > @@ -177,7 +178,18 @@ int main(int argc, char **argv)
> >        if (!mappings)
> >            usage();
> >    
> > -    verify_ranges(pw, ranges, mappings);
> > +    verify_ranges(pw, ranges, mappings, &flags);
> > +
> > +    if (flags && strlen(flags) != 0) {
> > +        /* only allowed flag is currently deny */
> > +        if (strcmp(flags, "deny") == 0) {
> > +            write_proc(proc_dir_fd, "setgroups", "deny",
> > strlen("deny"));
> > +        } else {
> > +            fprintf(stderr, _("%s: illegal flag in map file:
> > %s\n"),
> > +                Prog, flags);
> > +            exit(EXIT_FAILURE);
> > +        }
> > +    }   
> >    
> >        write_mapping(proc_dir_fd, ranges, mappings, "gid_map");
> >        sub_gid_close();
> 

Re: Unprivileged containers and co-ordinating user namespaces

[James Bottomley]

On Sun, 2016-05-01 at 12:37 -0400, Serge Hallyn wrote:
> > > It looks like the only addition it needs is the setgroups flag
> > > for
> > > newgidmap, which the security people will need, so I can patch
> > > that. 
> > 
> > How does this look for a patch?
> 
> Certainly deny is a terrible keyword for this, given the somewhat
> convoluted case it represents.  Maybe lockgroups or nosetgroup?

OK, nosegroups seems to be less scary.

>   And of course it'll need a clear explanation of what it does in the
> subgid manpage. But really i don't know that this can work the way
> you want it to.

Yes, docs and tests were to be added after everyone was happy with the
implementation.

>   Can you describe your use case?

basically not getting a CVE filed against this utility for allowing the
dropping of groups.  I don't have a use case for group membership
denying permissions.

> The intent of setgroups is to keep *unprivileged* writers of gid_map
> from dropping a gid which is blocking a file access from the user. 


You can't do that with the map write.  Even if you set no gid_map at
all, the attributes checks are done on the kernel side groups (i.e. the
ones you possess outside the namespace even if they have no interior
mapping).  The only way to drop groups is with the setgroups() system
call.

>  But newgidmap runs suid root (or cap_gid when you are through :), so
> the user can trivially assign just one authorized gid, not the whole
> range, and bypass the acls that way.

It does more than that: it also blocks the sys_setgroups() call in the
namespace, even for the namespace owner, which is the primary CVE
preventing use case.

> At the very least you would want to enforce the use of a whole range
> in the lockgroup case, but even there i question it's value.

I think, if you were going to enforce deny sys_setgroups() on a user
you otherwise trusted, you'd probably only allow them a single range. 
 But I suppose the flag should be or'd over all ranges.

James


> -Serge
> 
> 
> 
> > James
> > 
> > ---
> > 
> > diff --git a/lib/subordinateio.c b/lib/subordinateio.c
> > index 0d64a91..aa3c968 100644
> > --- a/lib/subordinateio.c
> > +++ b/lib/subordinateio.c
> > @@ -18,9 +18,10 @@ struct subordinate_range {
> >        const char *owner;
> >        unsigned long start;
> >        unsigned long count;
> > +    const char *flags;
> >    };
> >    
> > -#define NFIELDS 3
> > +#define NFIELDS 4
> >    
> >    /*
> >      * subordinate_dup: create a duplicate range
> > @@ -44,6 +45,16 @@ static /*@null@*/ /*@only@*/void
> > *subordinate_dup
> > (const void *ent)         free(range);
> >            return NULL;
> >        }
> > +    if (rangeent->flags) {
> > +        range->flags = strdup (rangeent->flags);
> > +        if (NULL == range->flags) {
> > +            free((void *)range->owner);
> > +            free(range);
> > +            return NULL;
> > +        }
> > +    } else {
> > +        range->flags = NULL;
> > +    }
> >        range->start = rangeent->start;
> >        range->count = rangeent->count;
> >    
> > @@ -111,13 +122,17 @@ static void *subordinate_parse (const char
> > *line)
> >         * There must be exactly NFIELDS colon separated fields or
> >         * the entry is invalid.   Also, fields must be non-blank.
> >         */
> > -    if (i != NFIELDS || *fields[0] == '\0' || *fields[1] == '\0'
> > ||
> > *fields[2] == '\0') +    if ((i != NFIELDS && i != NFIELDS -1) ||
> > *fields[0] == '\0' || *fields[1] == '\0' || *fields[2] == '\0')    
> >     return
> > NULL;     range.owner = fields[0];
> >        if (getulong (fields[1], &range.start) == 0)
> >            return NULL;
> >        if (getulong (fields[2], &range.count) == 0)
> >            return NULL;
> > +    if (i == NFIELDS)
> > +        range.flags = fields[3];
> > +    else
> > +        range.flags = NULL;
> >    
> >        return ⦥
> >    }
> > @@ -134,10 +149,11 @@ static int subordinate_put (const void *ent,
> > FILE
> > * file) {
> >        const struct subordinate_range *range = ent;
> >    
> > -    return fprintf(file, "%s:%lu:%lu\n",
> > +    return fprintf(file, "%s:%lu:%lu:%s\n",
> >                             range->owner,
> >                             range->start,
> > -                         range->count) < 0 ? -1   : 0;
> > +                         range->count,
> > +                         range->flags ? range->flags : "") < 0 ? 
> > -1   : 0;
> >    }
> >    
> >    static struct commonio_ops subordinate_ops = {
> > @@ -295,7 +311,8 @@ static const struct subordinate_range
> > *find_range(struct commonio_db *db,   * Returns true if @owner is
> > authorized to use the range, false otherwise.   */
> >    static bool have_range(struct commonio_db *db,
> > -                     const char *owner, unsigned long start,
> > unsigned long count)
> > +                     const char *owner, unsigned long start,
> > unsigned long count,
> > +                     const char **flags)
> >    {
> >        const struct subordinate_range *range;
> >        unsigned long end;
> > @@ -309,8 +326,11 @@ static bool have_range(struct commonio_db *db,
> >            unsigned long last; 
> >    
> >            last = range->start + range->count - 1;
> > -        if (last >= (start + count - 1))
> > +        if (last >= (start + count - 1)) {
> > +            if (flags)
> > +                *flags = range->flags;
> >                return true;
> > +        }
> >    
> >            count = end - last;
> >            start = last + 1;
> > @@ -430,7 +450,7 @@ static int add_range(struct commonio_db *db,
> >        range.count = count;
> >    
> >        /* See if the range is already present */
> > -    if (have_range(db, owner, start, count))
> > +    if (have_range(db, owner, start, count, NULL))
> >            return 1;
> >    
> >        /* Otherwise append the range */
> > @@ -585,7 +605,7 @@ bool sub_uid_assigned(const char *owner)
> >    
> >    bool have_sub_uids(const char *owner, uid_t start, unsigned long
> > count)
> >    {
> > -    return have_range (&subordinate_uid_db, owner, start, count);
> > +    return have_range (&subordinate_uid_db, owner, start, count,
> > NULL);
> >    }
> >    
> >    int sub_uid_add (const char *owner, uid_t start, unsigned long
> > count)
> > @@ -659,9 +679,9 @@ int sub_gid_open (int mode)
> >        return commonio_open (&subordinate_gid_db, mode);
> >    }
> >    
> > -bool have_sub_gids(const char *owner, gid_t start, unsigned long
> > count)
> > +bool have_sub_gids(const char *owner, gid_t start, unsigned long
> > count,
> > const char **flags) {
> > -    return have_range(&subordinate_gid_db, owner, start, count);
> > +    return have_range(&subordinate_gid_db, owner, start, count,
> > flags);
> >    }
> >    
> >    bool sub_gid_assigned(const char *owner)
> > diff --git a/lib/subordinateio.h b/lib/subordinateio.h
> > index a21d72b..7e47659 100644
> > --- a/lib/subordinateio.h
> > +++ b/lib/subordinateio.h
> > @@ -25,7 +25,7 @@ extern int sub_uid_remove (const char *owner,
> > uid_t
> > start, unsigned long count); extern uid_t
> > sub_uid_find_free_range(uid_t
> > min, uid_t max, unsigned long count); 
> >    extern int sub_gid_close(void);
> > -extern bool have_sub_gids(const char *owner, gid_t start, unsigned
> > long
> > count); +extern bool have_sub_gids(const char *owner, gid_t start,
> > unsigned long count, const char **flags); extern bool
> > sub_gid_file_present (void); extern bool sub_gid_assigned(const
> > char
> > *owner); extern int sub_gid_lock (void);
> > diff --git a/libmisc/idmapping.c b/libmisc/idmapping.c
> > index 0dce634..7f7de88 100644
> > --- a/libmisc/idmapping.c
> > +++ b/libmisc/idmapping.c
> > @@ -106,7 +106,6 @@ void write_mapping(int proc_dir_fd, int ranges,
> > struct map_range *mappings,     struct map_range *mapping;
> >        size_t bufsize;
> >        char *buf, *pos;
> > -    int fd;
> >    
> >        bufsize = ranges * ((ULONG_DIGITS   + 1) * 3);
> >        pos = buf = xmalloc(bufsize);
> > @@ -128,13 +127,20 @@ void write_mapping(int proc_dir_fd, int
> > ranges,
> > struct map_range *mappings,     }
> >    
> >        /* Write the mapping to the maping file */
> > +    write_proc(proc_dir_fd, map_file, buf, pos - buf);
> > +}
> > +
> > +void write_proc(int proc_dir_fd, const char *map_file, void *buf,
> > int
> > len) +{
> > +    int fd;
> > +
> >        fd = openat(proc_dir_fd, map_file, O_WRONLY);
> >        if (fd < 0) {
> >            fprintf(stderr, _("%s: open of %s failed: %s\n"),
> >                Prog, map_file, strerror(errno));
> >            exit(EXIT_FAILURE);
> >        }
> > -    if (write(fd, buf, pos - buf) != (pos - buf)) {
> > +    if (write(fd, buf, len) != len) {
> >            fprintf(stderr, _("%s: write to %s failed: %s\n"),
> >                Prog, map_file, strerror(errno));
> >            exit(EXIT_FAILURE);
> > diff --git a/libmisc/idmapping.h b/libmisc/idmapping.h
> > index 58d000f..c2cec38 100644
> > --- a/libmisc/idmapping.h
> > +++ b/libmisc/idmapping.h
> > @@ -39,6 +39,7 @@ struct map_range {
> >    extern struct map_range *get_map_ranges(int ranges, int argc,
> > char
> > **argv); extern void write_mapping(int proc_dir_fd, int ranges,
> >        struct map_range *mappings, const char *map_file);
> > +extern void write_proc(int proc_dir_fd, const char *map_file, void
> > *buf, int len); 
> >    #endif /* _ID_MAPPING_H_ */
> >    
> > diff --git a/src/newgidmap.c b/src/newgidmap.c
> > index 451c6a6..8d64e3b 100644
> > --- a/src/newgidmap.c
> > +++ b/src/newgidmap.c
> > @@ -46,14 +46,14 @@
> >      */
> >    const char *Prog;
> >    
> > -static bool verify_range(struct passwd *pw, struct map_range
> > *range)
> > +static bool verify_range(struct passwd *pw, struct map_range
> > *range,
> > const char **flags) {
> >        /* An empty range is invalid */
> >        if (range->count == 0)
> >            return false;
> >    
> >        /* Test /etc/subgid */
> > -    if (have_sub_gids(pw->pw_name, range->lower, range->count))
> > +    if (have_sub_gids(pw->pw_name, range->lower, range->count,
> > flags))
> >            return true;
> >    
> >        /* Allow a process to map it's own gid */
> > @@ -64,14 +64,14 @@ static bool verify_range(struct passwd *pw,
> > struct
> > map_range *range) }
> >    
> >    static void verify_ranges(struct passwd *pw, int ranges,
> > -    struct map_range *mappings)
> > +               struct map_range *mappings, const char **flags)
> >    {
> >        struct map_range *mapping;
> >        int idx;
> >    
> >        mapping = mappings;
> >        for (idx = 0; idx < ranges; idx++, mapping++) {
> > -        if (!verify_range(pw, mapping)) {
> > +        if (!verify_range(pw, mapping, flags)) {
> >                fprintf(stderr, _( "%s: gid range [%lu-%lu) -> [%lu
> > -%lu) not
> > allowed\n"),                 Prog,
> >                    mapping->upper,
> > @@ -103,6 +103,7 @@ int main(int argc, char **argv)
> >        struct stat st;
> >        struct passwd *pw;
> >        int written;
> > +    const char *flags;
> >    
> >        Prog = Basename (argv[0]);
> >    
> > @@ -177,7 +178,18 @@ int main(int argc, char **argv)
> >        if (!mappings)
> >            usage();
> >    
> > -    verify_ranges(pw, ranges, mappings);
> > +    verify_ranges(pw, ranges, mappings, &flags);
> > +
> > +    if (flags && strlen(flags) != 0) {
> > +        /* only allowed flag is currently deny */
> > +        if (strcmp(flags, "deny") == 0) {
> > +            write_proc(proc_dir_fd, "setgroups", "deny",
> > strlen("deny"));
> > +        } else {
> > +            fprintf(stderr, _("%s: illegal flag in map file:
> > %s\n"),
> > +                Prog, flags);
> > +            exit(EXIT_FAILURE);
> > +        }
> > +    }   
> >    
> >        write_mapping(proc_dir_fd, ranges, mappings, "gid_map");
> >        sub_gid_close();
> 

Re: Unprivileged containers and co-ordinating user namespaces

[Serge E. Hallyn]

On Sun, May 01, 2016 at 04:29:26PM -0700, James Bottomley wrote:
> On Sun, 2016-05-01 at 12:37 -0400, Serge Hallyn wrote:
> > > > It looks like the only addition it needs is the setgroups flag
> > > > for
> > > > newgidmap, which the security people will need, so I can patch
> > > > that. 
> > > 
> > > How does this look for a patch?
> > 
> > Certainly deny is a terrible keyword for this, given the somewhat
> > convoluted case it represents.  Maybe lockgroups or nosetgroup?
> 
> OK, nosegroups seems to be less scary.
> 
> >   And of course it'll need a clear explanation of what it does in the
> > subgid manpage. But really i don't know that this can work the way
> > you want it to.
> 
> Yes, docs and tests were to be added after everyone was happy with the
> implementation.
> 
> >   Can you describe your use case?
> 
> basically not getting a CVE filed against this utility for allowing the
> dropping of groups.  I don't have a use case for group membership
> denying permissions.
> 
> > The intent of setgroups is to keep *unprivileged* writers of gid_map
> > from dropping a gid which is blocking a file access from the user. 
> 
> 
> You can't do that with the map write.  Even if you set no gid_map at
> all, the attributes checks are done on the kernel side groups (i.e. the
> ones you possess outside the namespace even if they have no interior
> mapping).  The only way to drop groups is with the setgroups() system
> call.
> 
> >  But newgidmap runs suid root (or cap_gid when you are through :), so
> > the user can trivially assign just one authorized gid, not the whole
> > range, and bypass the acls that way.
> 
> It does more than that: it also blocks the sys_setgroups() call in the
> namespace, even for the namespace owner, which is the primary CVE
> preventing use case.
> 
> > At the very least you would want to enforce the use of a whole range
> > in the lockgroup case, but even there i question it's value.
> 
> I think, if you were going to enforce deny sys_setgroups() on a user
> you otherwise trusted, you'd probably only allow them a single range. 
>  But I suppose the flag should be or'd over all ranges.

Ok, yeah.  My thought this afternoon was that the point of the CVE was
that an unprivileged user, in the absence of any admin-provided
/etc/sub{u,g}id or new{u,g}idmap at all, and who was being denied a file
access through one of his aux groups, could use a user namespace to drop
the aux group and access the file.  That if an admin went to the trouble
to grant subuid and subgids then it wasn't an issue.  But of course
allocations are being done automatically now so that's not a valid
point.

So let's proceed with the patch :)

A PR using nosetgroups against git://github.com/shadow-maint/shadow would
be great.

thanks,
-serge

Re: Unprivileged containers and co-ordinating user namespaces

[Serge E. Hallyn]

On Sun, May 01, 2016 at 04:29:26PM -0700, James Bottomley wrote:
> On Sun, 2016-05-01 at 12:37 -0400, Serge Hallyn wrote:
> > > > It looks like the only addition it needs is the setgroups flag
> > > > for
> > > > newgidmap, which the security people will need, so I can patch
> > > > that. 
> > > 
> > > How does this look for a patch?
> > 
> > Certainly deny is a terrible keyword for this, given the somewhat
> > convoluted case it represents.  Maybe lockgroups or nosetgroup?
> 
> OK, nosegroups seems to be less scary.
> 
> >   And of course it'll need a clear explanation of what it does in the
> > subgid manpage. But really i don't know that this can work the way
> > you want it to.
> 
> Yes, docs and tests were to be added after everyone was happy with the
> implementation.
> 
> >   Can you describe your use case?
> 
> basically not getting a CVE filed against this utility for allowing the
> dropping of groups.  I don't have a use case for group membership
> denying permissions.
> 
> > The intent of setgroups is to keep *unprivileged* writers of gid_map
> > from dropping a gid which is blocking a file access from the user. 
> 
> 
> You can't do that with the map write.  Even if you set no gid_map at
> all, the attributes checks are done on the kernel side groups (i.e. the
> ones you possess outside the namespace even if they have no interior
> mapping).  The only way to drop groups is with the setgroups() system
> call.
> 
> >  But newgidmap runs suid root (or cap_gid when you are through :), so
> > the user can trivially assign just one authorized gid, not the whole
> > range, and bypass the acls that way.
> 
> It does more than that: it also blocks the sys_setgroups() call in the
> namespace, even for the namespace owner, which is the primary CVE
> preventing use case.
> 
> > At the very least you would want to enforce the use of a whole range
> > in the lockgroup case, but even there i question it's value.
> 
> I think, if you were going to enforce deny sys_setgroups() on a user
> you otherwise trusted, you'd probably only allow them a single range. 
>  But I suppose the flag should be or'd over all ranges.

Ok, yeah.  My thought this afternoon was that the point of the CVE was
that an unprivileged user, in the absence of any admin-provided
/etc/sub{u,g}id or new{u,g}idmap at all, and who was being denied a file
access through one of his aux groups, could use a user namespace to drop
the aux group and access the file.  That if an admin went to the trouble
to grant subuid and subgids then it wasn't an issue.  But of course
allocations are being done automatically now so that's not a valid
point.

So let's proceed with the patch :)

A PR using nosetgroups against git://github.com/shadow-maint/shadow would
be great.

thanks,
-serge

Re: Unprivileged containers and co-ordinating user namespaces

[Serge E. Hallyn]

Quoting James Bottomley (James.Bottomley@HansenPartnership.com):
> On Thu, 2016-04-28 at 16:00 -0700, W. Trevor King wrote:
> > On Thu, Apr 28, 2016 at 03:02:08PM -0700, James Bottomley wrote:
> > > /etc/usernamespaces
> > > 
> > > and the format be :::
> > > 
> > > …
> > > 
> > > If this sounds OK to people, I can code up a utility that does this,
> > > which should probably belong in util-linux.
> > 
> > This sounds a lot like shadow's newuidmap and newgidmap [1,2,3].
> > 
> > Cheers,
> > Trevor
> > 
> > [1]: https://github.com/shadow-maint/shadow/commit/673c2a6f9aa6c69588f4c1be08589b8d3475a520
> > [2]: http://man7.org/linux/man-pages/man1/newuidmap.1.html
> > [3]: http://man7.org/linux/man-pages/man5/subuid.5.html
> 
> I think that mostly works.  No-one's packaging it yet, which is why I

https://packages.debian.org/jessie/uidmap
https://launchpad.net/ubuntu/yakkety/+package/uidmap
http://rpm.pbone.net/index.php3/stat/45/idpl/28763248/numer/1/nazwa/newuidmap

> didn't notice.  It also looks like the build dependencies have vastly
> expanded, so I can't get it to build in the build service yet.
> 
> It looks like the only addition it needs is the setgroups flag for
> newgidmap, which the security people will need, so I can patch that. 
>  Plus it's trying to install newgidmap/newuidmap as setuid root rather
> than cap_setuid/cap_setgid, but that's fixable in the spec file.

That would prevent it being installed inside user namespaces, until
the user namespaced file capabilities (see separate thread :) hit.

-serge
_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/containers

Re: Unprivileged containers and co-ordinating user namespaces

[James Bottomley]

[-- Attachment #1.1: Type: text/plain, Size: 9250 bytes --]

On Fri, 2016-04-29 at 08:38 -0700, James Bottomley wrote:
> On Thu, 2016-04-28 at 16:00 -0700, W. Trevor King wrote:
> > On Thu, Apr 28, 2016 at 03:02:08PM -0700, James Bottomley wrote:
> > > /etc/usernamespaces
> > > 
> > > and the format be :::
> > > 
> > > …
> > > 
> > > If this sounds OK to people, I can code up a utility that does
> > > this,
> > > which should probably belong in util-linux.
> > 
> > This sounds a lot like shadow's newuidmap and newgidmap [1,2,3].
> > 
> > Cheers,
> > Trevor
> > 
> > [1]: https://github.com/shadow-maint/shadow/commit/673c2a6f9aa6c695
> > 88f4c1be08589b8d3475a520
> > [2]: http://man7.org/linux/man-pages/man1/newuidmap.1.html
> > [3]: http://man7.org/linux/man-pages/man5/subuid.5.html
> 
> I think that mostly works.  No-one's packaging it yet, which is why I
> didn't notice.  It also looks like the build dependencies have vastly
> expanded, so I can't get it to build in the build service yet.
> 
> It looks like the only addition it needs is the setgroups flag for
> newgidmap, which the security people will need, so I can patch that. 

How does this look for a patch?

James

---

diff --git a/lib/subordinateio.c b/lib/subordinateio.c
index 0d64a91..aa3c968 100644
--- a/lib/subordinateio.c
+++ b/lib/subordinateio.c
@@ -18,9 +18,10 @@ struct subordinate_range {
 	const char *owner;
 	unsigned long start;
 	unsigned long count;
+	const char *flags;
 };
 
-#define NFIELDS 3
+#define NFIELDS 4
 
 /*
  * subordinate_dup: create a duplicate range
@@ -44,6 +45,16 @@ static /*@null@*/ /*@only@*/void *subordinate_dup (const void *ent)
 		free(range);
 		return NULL;
 	}
+	if (rangeent->flags) {
+		range->flags = strdup (rangeent->flags);
+		if (NULL == range->flags) {
+			free((void *)range->owner);
+			free(range);
+			return NULL;
+		}
+	} else {
+		range->flags = NULL;
+	}
 	range->start = rangeent->start;
 	range->count = rangeent->count;
 
@@ -111,13 +122,17 @@ static void *subordinate_parse (const char *line)
 	 * There must be exactly NFIELDS colon separated fields or
 	 * the entry is invalid.  Also, fields must be non-blank.
 	 */
-	if (i != NFIELDS || *fields[0] == '\0' || *fields[1] == '\0' || *fields[2] == '\0')
+	if ((i != NFIELDS && i != NFIELDS -1) || *fields[0] == '\0' || *fields[1] == '\0' || *fields[2] == '\0')
 		return NULL;
 	range.owner = fields[0];
 	if (getulong (fields[1], &range.start) == 0)
 		return NULL;
 	if (getulong (fields[2], &range.count) == 0)
 		return NULL;
+	if (i == NFIELDS)
+		range.flags = fields[3];
+	else
+		range.flags = NULL;
 
 	return ⦥
 }
@@ -134,10 +149,11 @@ static int subordinate_put (const void *ent, FILE * file)
 {
 	const struct subordinate_range *range = ent;
 
-	return fprintf(file, "%s:%lu:%lu\n",
+	return fprintf(file, "%s:%lu:%lu:%s\n",
 			       range->owner,
 			       range->start,
-			       range->count) < 0 ? -1  : 0;
+			       range->count,
+			       range->flags ? range->flags : "") < 0 ? -1  : 0;
 }
 
 static struct commonio_ops subordinate_ops = {
@@ -295,7 +311,8 @@ static const struct subordinate_range *find_range(struct commonio_db *db,
  * Returns true if @owner is authorized to use the range, false otherwise.
  */
 static bool have_range(struct commonio_db *db,
-		       const char *owner, unsigned long start, unsigned long count)
+		       const char *owner, unsigned long start, unsigned long count,
+		       const char **flags)
 {
 	const struct subordinate_range *range;
 	unsigned long end;
@@ -309,8 +326,11 @@ static bool have_range(struct commonio_db *db,
 		unsigned long last; 
 
 		last = range->start + range->count - 1;
-		if (last >= (start + count - 1))
+		if (last >= (start + count - 1)) {
+			if (flags)
+				*flags = range->flags;
 			return true;
+		}
 
 		count = end - last;
 		start = last + 1;
@@ -430,7 +450,7 @@ static int add_range(struct commonio_db *db,
 	range.count = count;
 
 	/* See if the range is already present */
-	if (have_range(db, owner, start, count))
+	if (have_range(db, owner, start, count, NULL))
 		return 1;
 
 	/* Otherwise append the range */
@@ -585,7 +605,7 @@ bool sub_uid_assigned(const char *owner)
 
 bool have_sub_uids(const char *owner, uid_t start, unsigned long count)
 {
-	return have_range (&subordinate_uid_db, owner, start, count);
+	return have_range (&subordinate_uid_db, owner, start, count, NULL);
 }
 
 int sub_uid_add (const char *owner, uid_t start, unsigned long count)
@@ -659,9 +679,9 @@ int sub_gid_open (int mode)
 	return commonio_open (&subordinate_gid_db, mode);
 }
 
-bool have_sub_gids(const char *owner, gid_t start, unsigned long count)
+bool have_sub_gids(const char *owner, gid_t start, unsigned long count, const char **flags)
 {
-	return have_range(&subordinate_gid_db, owner, start, count);
+	return have_range(&subordinate_gid_db, owner, start, count, flags);
 }
 
 bool sub_gid_assigned(const char *owner)
diff --git a/lib/subordinateio.h b/lib/subordinateio.h
index a21d72b..7e47659 100644
--- a/lib/subordinateio.h
+++ b/lib/subordinateio.h
@@ -25,7 +25,7 @@ extern int sub_uid_remove (const char *owner, uid_t start, unsigned long count);
 extern uid_t sub_uid_find_free_range(uid_t min, uid_t max, unsigned long count);
 
 extern int sub_gid_close(void);
-extern bool have_sub_gids(const char *owner, gid_t start, unsigned long count);
+extern bool have_sub_gids(const char *owner, gid_t start, unsigned long count, const char **flags);
 extern bool sub_gid_file_present (void);
 extern bool sub_gid_assigned(const char *owner);
 extern int sub_gid_lock (void);
diff --git a/libmisc/idmapping.c b/libmisc/idmapping.c
index 0dce634..7f7de88 100644
--- a/libmisc/idmapping.c
+++ b/libmisc/idmapping.c
@@ -106,7 +106,6 @@ void write_mapping(int proc_dir_fd, int ranges, struct map_range *mappings,
 	struct map_range *mapping;
 	size_t bufsize;
 	char *buf, *pos;
-	int fd;
 
 	bufsize = ranges * ((ULONG_DIGITS  + 1) * 3);
 	pos = buf = xmalloc(bufsize);
@@ -128,13 +127,20 @@ void write_mapping(int proc_dir_fd, int ranges, struct map_range *mappings,
 	}
 
 	/* Write the mapping to the maping file */
+	write_proc(proc_dir_fd, map_file, buf, pos - buf);
+}
+
+void write_proc(int proc_dir_fd, const char *map_file, void *buf, int len)
+{
+	int fd;
+
 	fd = openat(proc_dir_fd, map_file, O_WRONLY);
 	if (fd < 0) {
 		fprintf(stderr, _("%s: open of %s failed: %s\n"),
 			Prog, map_file, strerror(errno));
 		exit(EXIT_FAILURE);
 	}
-	if (write(fd, buf, pos - buf) != (pos - buf)) {
+	if (write(fd, buf, len) != len) {
 		fprintf(stderr, _("%s: write to %s failed: %s\n"),
 			Prog, map_file, strerror(errno));
 		exit(EXIT_FAILURE);
diff --git a/libmisc/idmapping.h b/libmisc/idmapping.h
index 58d000f..c2cec38 100644
--- a/libmisc/idmapping.h
+++ b/libmisc/idmapping.h
@@ -39,6 +39,7 @@ struct map_range {
 extern struct map_range *get_map_ranges(int ranges, int argc, char **argv);
 extern void write_mapping(int proc_dir_fd, int ranges,
 	struct map_range *mappings, const char *map_file);
+extern void write_proc(int proc_dir_fd, const char *map_file, void *buf, int len);
 
 #endif /* _ID_MAPPING_H_ */
 
diff --git a/src/newgidmap.c b/src/newgidmap.c
index 451c6a6..8d64e3b 100644
--- a/src/newgidmap.c
+++ b/src/newgidmap.c
@@ -46,14 +46,14 @@
  */
 const char *Prog;
 
-static bool verify_range(struct passwd *pw, struct map_range *range)
+static bool verify_range(struct passwd *pw, struct map_range *range, const char **flags)
 {
 	/* An empty range is invalid */
 	if (range->count == 0)
 		return false;
 
 	/* Test /etc/subgid */
-	if (have_sub_gids(pw->pw_name, range->lower, range->count))
+	if (have_sub_gids(pw->pw_name, range->lower, range->count, flags))
 		return true;
 
 	/* Allow a process to map it's own gid */
@@ -64,14 +64,14 @@ static bool verify_range(struct passwd *pw, struct map_range *range)
 }
 
 static void verify_ranges(struct passwd *pw, int ranges,
-	struct map_range *mappings)
+			  struct map_range *mappings, const char **flags)
 {
 	struct map_range *mapping;
 	int idx;
 
 	mapping = mappings;
 	for (idx = 0; idx < ranges; idx++, mapping++) {
-		if (!verify_range(pw, mapping)) {
+		if (!verify_range(pw, mapping, flags)) {
 			fprintf(stderr, _( "%s: gid range [%lu-%lu) -> [%lu-%lu) not allowed\n"),
 				Prog,
 				mapping->upper,
@@ -103,6 +103,7 @@ int main(int argc, char **argv)
 	struct stat st;
 	struct passwd *pw;
 	int written;
+	const char *flags;
 
 	Prog = Basename (argv[0]);
 
@@ -177,7 +178,18 @@ int main(int argc, char **argv)
 	if (!mappings)
 		usage();
 
-	verify_ranges(pw, ranges, mappings);
+	verify_ranges(pw, ranges, mappings, &flags);
+
+	if (flags && strlen(flags) != 0) {
+		/* only allowed flag is currently deny */
+		if (strcmp(flags, "deny") == 0) {
+			write_proc(proc_dir_fd, "setgroups", "deny", strlen("deny"));
+		} else {
+			fprintf(stderr, _("%s: illegal flag in map file: %s\n"),
+				Prog, flags);
+			exit(EXIT_FAILURE);
+		}
+	}	
 
 	write_mapping(proc_dir_fd, ranges, mappings, "gid_map");
 	sub_gid_close();

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 205 bytes --]

_______________________________________________
Containers mailing list
Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linuxfoundation.org/mailman/listinfo/containers

Re: Unprivileged containers and co-ordinating user namespaces

[Eric W. Biederman]

James Bottomley <James.Bottomley@HansenPartnership.com> writes:

> On Thu, 2016-04-28 at 16:00 -0700, W. Trevor King wrote:
>> On Thu, Apr 28, 2016 at 03:02:08PM -0700, James Bottomley wrote:
>> > /etc/usernamespaces
>> > 
>> > and the format be :::
>> > 
>> > …
>> > 
>> > If this sounds OK to people, I can code up a utility that does this,
>> > which should probably belong in util-linux.
>> 
>> This sounds a lot like shadow's newuidmap and newgidmap [1,2,3].
>> 
>> Cheers,
>> Trevor
>> 
>> [1]: https://github.com/shadow-maint/shadow/commit/673c2a6f9aa6c69588f4c1be08589b8d3475a520
>> [2]: http://man7.org/linux/man-pages/man1/newuidmap.1.html
>> [3]: http://man7.org/linux/man-pages/man5/subuid.5.html
>
> I think that mostly works.  No-one's packaging it yet, which is why I
> didn't notice.  It also looks like the build dependencies have vastly
> expanded, so I can't get it to build in the build service yet.

Both Fedora and Ubuntu should be packaging it.  Further Docker should
already be using these files.

> It looks like the only addition it needs is the setgroups flag for
> newgidmap, which the security people will need, so I can patch that. 
>  Plus it's trying to install newgidmap/newuidmap as setuid root rather
> than cap_setuid/cap_setgid, but that's fixable in the spec file.

I read the rest of this thread and I don't understand the setgroups flag
that you desire.  It sounds like someone with an incomplete grasp on the
situtation being cautious.

As far as I can tell the use cases for containers not supporting
setgroups is very limited.  Basically just using user namespaces to
drop privileges, and mapping the existing uids and gids to 0.

I don't think it actually makes sense to have a knob.  From a practical
standpoint entering any subordinate ids into the subgid file for a user
is a permission to use groups in such a way that can not use them as a
negative acl (because we allow them to be dropped).

Certainly it has been that way for quite a while now.

Except for the negative acl aspect there are no issues with dropping
groups, as setgroups will limit you to the groups allowed in your user
namespace.

Eric

_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/containers

Re: Unprivileged containers and co-ordinating user namespaces

[Eric W. Biederman]

James Bottomley <James.Bottomley@HansenPartnership.com> writes:

> On Thu, 2016-04-28 at 16:00 -0700, W. Trevor King wrote:
>> On Thu, Apr 28, 2016 at 03:02:08PM -0700, James Bottomley wrote:
>> > /etc/usernamespaces
>> > 
>> > and the format be :::
>> > 
>> > …
>> > 
>> > If this sounds OK to people, I can code up a utility that does this,
>> > which should probably belong in util-linux.
>> 
>> This sounds a lot like shadow's newuidmap and newgidmap [1,2,3].
>> 
>> Cheers,
>> Trevor
>> 
>> [1]: https://github.com/shadow-maint/shadow/commit/673c2a6f9aa6c69588f4c1be08589b8d3475a520
>> [2]: http://man7.org/linux/man-pages/man1/newuidmap.1.html
>> [3]: http://man7.org/linux/man-pages/man5/subuid.5.html
>
> I think that mostly works.  No-one's packaging it yet, which is why I
> didn't notice.  It also looks like the build dependencies have vastly
> expanded, so I can't get it to build in the build service yet.

Both Fedora and Ubuntu should be packaging it.  Further Docker should
already be using these files.

> It looks like the only addition it needs is the setgroups flag for
> newgidmap, which the security people will need, so I can patch that. 
>  Plus it's trying to install newgidmap/newuidmap as setuid root rather
> than cap_setuid/cap_setgid, but that's fixable in the spec file.

I read the rest of this thread and I don't understand the setgroups flag
that you desire.  It sounds like someone with an incomplete grasp on the
situtation being cautious.

As far as I can tell the use cases for containers not supporting
setgroups is very limited.  Basically just using user namespaces to
drop privileges, and mapping the existing uids and gids to 0.

I don't think it actually makes sense to have a knob.  From a practical
standpoint entering any subordinate ids into the subgid file for a user
is a permission to use groups in such a way that can not use them as a
negative acl (because we allow them to be dropped).

Certainly it has been that way for quite a while now.

Except for the negative acl aspect there are no issues with dropping
groups, as setgroups will limit you to the groups allowed in your user
namespace.

Eric

Re: Unprivileged containers and co-ordinating user namespaces

[Phil Estes]

Eric W. Biederman wrote:
> James Bottomley<James.Bottomley@HansenPartnership.com>  writes:
>
>> On Thu, 2016-04-28 at 16:00 -0700, W. Trevor King wrote:
>>> On Thu, Apr 28, 2016 at 03:02:08PM -0700, James Bottomley wrote:
>>>> /etc/usernamespaces
>>>>
>>>> and the format be :::
>>>>
>>>> …
>>>>
>>>> If this sounds OK to people, I can code up a utility that does this,
>>>> which should probably belong in util-linux.
>>> This sounds a lot like shadow's newuidmap and newgidmap [1,2,3].
>>>
>>> Cheers,
>>> Trevor
>>>
>>> [1]: https://github.com/shadow-maint/shadow/commit/673c2a6f9aa6c69588f4c1be08589b8d3475a520
>>> [2]: http://man7.org/linux/man-pages/man1/newuidmap.1.html
>>> [3]: http://man7.org/linux/man-pages/man5/subuid.5.html
>> I think that mostly works.  No-one's packaging it yet, which is why I
>> didn't notice.  It also looks like the build dependencies have vastly
>> expanded, so I can't get it to build in the build service yet.
>
> Both Fedora and Ubuntu should be packaging it.  Further Docker should
> already be using these files.
Yes, based on our discussion in the PRs when user namespaces 
capabilities were added to Docker, we respect the /etc/sub{u,g}id files 
for sourcing mappings for userns-confined processes.

- Phil

Re: Unprivileged containers and co-ordinating user namespaces

[James Bottomley]

On Wed, 2016-05-04 at 10:02 -0500, Eric W. Biederman wrote:
> James Bottomley <James.Bottomley@HansenPartnership.com> writes:
> 
> > On Thu, 2016-04-28 at 16:00 -0700, W. Trevor King wrote:
> > > On Thu, Apr 28, 2016 at 03:02:08PM -0700, James Bottomley wrote:
> > > > /etc/usernamespaces
> > > > 
> > > > and the format be :::
> > > > 
> > > > …
> > > > 
> > > > If this sounds OK to people, I can code up a utility that does 
> > > > this, which should probably belong in util-linux.
> > > 
> > > This sounds a lot like shadow's newuidmap and newgidmap [1,2,3].
> > > 
> > > Cheers,
> > > Trevor
> > > 
> > > [1]: https://github.com/shadow-maint/shadow/commit/673c2a6f9aa6c6
> > > 9588f4c1be08589b8d3475a520
> > > [2]: http://man7.org/linux/man-pages/man1/newuidmap.1.html
> > > [3]: http://man7.org/linux/man-pages/man5/subuid.5.html
> > 
> > I think that mostly works.  No-one's packaging it yet, which is why 
> > I didn't notice.  It also looks like the build dependencies have
> > vastly expanded, so I can't get it to build in the build service
> > yet.
> 
> Both Fedora and Ubuntu should be packaging it.  Further Docker should
> already be using these files.
> 
> > It looks like the only addition it needs is the setgroups flag for
> > newgidmap, which the security people will need, so I can patch 
> > that.  Plus it's trying to install newgidmap/newuidmap as setuid 
> > root rather than cap_setuid/cap_setgid, but that's fixable in the 
> > spec file.
> 
> I read the rest of this thread and I don't understand the setgroups 
> flag that you desire.  It sounds like someone with an incomplete 
> grasp on the situtation being cautious.
> 
> As far as I can tell the use cases for containers not supporting
> setgroups is very limited.  Basically just using user namespaces to
> drop privileges, and mapping the existing uids and gids to 0.
> 
> I don't think it actually makes sense to have a knob.  From a 
> practical standpoint entering any subordinate ids into the subgid 
> file for a user is a permission to use groups in such a way that can 
> not use them as a negative acl (because we allow them to be dropped).
> 
> Certainly it has been that way for quite a while now.

I don't quite get this.  If setgroups is set to deny and I have a set
of group mappings, I still can't get rid of the negative acl group.  I
can map it to a different gid inside my container, or I can not map it
at all, but in either case I still can't get access to anything with
the negative acl group marker because the group permission checks
occurs with the kguid_t set which includes my mapped or unmapped group.
 The only way I can lose it is to call sys_setgroups().

It's a bit ugly because I have to enter the container with --preserve
-credentials and I can't su to myself if I enter as root (-S 0), I have
to re-enter as myself instead, but it works.

> Except for the negative acl aspect there are no issues with dropping
> groups, as setgroups will limit you to the groups allowed in your 
> user namespace.

Well, notwithstanding the merits of negative acls, which I don't want
to debate because I don't think they're that useful, the use case might
be that a user possessing a negative acl still wants to use an
architecture emulation container for building.  Installing such a
container requires being able to set a set of groups and uids (required
by the installer), but it doesn't require the sys_setgroups() system
call, so they could reasonably be given the ability to set one up with
the nosetgroups flag and a range of gids allocated in subgid to ensure
they still can't get access to resources denied by the negative acl
group.

James

Re: Unprivileged containers and co-ordinating user namespaces

[James Bottomley]

On Wed, 2016-05-04 at 14:17 -0400, James Bottomley wrote:
> > Certainly it has been that way for quite a while now.
> 
> I don't quite get this.  If setgroups is set to deny and I have a set
> of group mappings, I still can't get rid of the negative acl group. 
>  I can map it to a different gid inside my container, or I can not 
> map  it at all, but in either case I still can't get access to 
> anything with the negative acl group marker because the group 
> permission checks occurs with the kguid_t set which includes my 
> mapped or unmapped group.  The only way I can lose it is to call
> sys_setgroups().

Sorry, this next bit should be at the end of the email (I was playing
and typing at the same time):

> It's a bit ugly because I have to enter the container with --preserve
> -credentials and I can't su to myself if I enter as root (-S 0), I 
> have to re-enter as myself instead, but it works.
> 
> > Except for the negative acl aspect there are no issues with 
> > dropping groups, as setgroups will limit you to the groups allowed 
> > in your user namespace.
> 
> Well, notwithstanding the merits of negative acls, which I don't want
> to debate because I don't think they're that useful, the use case 
> might be that a user possessing a negative acl still wants to use an
> architecture emulation container for building.  Installing such a
> container requires being able to set a set of groups and uids 
> (required by the installer), but it doesn't require the 
> sys_setgroups() system call, so they could reasonably be given the 
> ability to set one up with the nosetgroups flag and a range of gids 
> allocated in subgid to ensure they still can't get access to 
> resources denied by the negative acl group.

It's a bit ugly because I have to enter the container with --preserve
-credentials and I can't su to myself if I enter as root (-S 0), I have
to re-enter as myself instead, but it works.

James

Re: Unprivileged containers and co-ordinating user namespaces

[James Bottomley]

On Wed, 2016-05-04 at 14:17 -0400, James Bottomley wrote:
> > Certainly it has been that way for quite a while now.
> 
> I don't quite get this.  If setgroups is set to deny and I have a set
> of group mappings, I still can't get rid of the negative acl group. 
>  I can map it to a different gid inside my container, or I can not 
> map  it at all, but in either case I still can't get access to 
> anything with the negative acl group marker because the group 
> permission checks occurs with the kguid_t set which includes my 
> mapped or unmapped group.  The only way I can lose it is to call
> sys_setgroups().

Sorry, this next bit should be at the end of the email (I was playing
and typing at the same time):

> It's a bit ugly because I have to enter the container with --preserve
> -credentials and I can't su to myself if I enter as root (-S 0), I 
> have to re-enter as myself instead, but it works.
> 
> > Except for the negative acl aspect there are no issues with 
> > dropping groups, as setgroups will limit you to the groups allowed 
> > in your user namespace.
> 
> Well, notwithstanding the merits of negative acls, which I don't want
> to debate because I don't think they're that useful, the use case 
> might be that a user possessing a negative acl still wants to use an
> architecture emulation container for building.  Installing such a
> container requires being able to set a set of groups and uids 
> (required by the installer), but it doesn't require the 
> sys_setgroups() system call, so they could reasonably be given the 
> ability to set one up with the nosetgroups flag and a range of gids 
> allocated in subgid to ensure they still can't get access to 
> resources denied by the negative acl group.

It's a bit ugly because I have to enter the container with --preserve
-credentials and I can't su to myself if I enter as root (-S 0), I have
to re-enter as myself instead, but it works.

James

Re: Unprivileged containers and co-ordinating user namespaces

[Phil Estes]

Eric W. Biederman wrote:
> James Bottomley<James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org>  writes:
>
>> On Thu, 2016-04-28 at 16:00 -0700, W. Trevor King wrote:
>>> On Thu, Apr 28, 2016 at 03:02:08PM -0700, James Bottomley wrote:
>>>> /etc/usernamespaces
>>>>
>>>> and the format be :::
>>>>
>>>> …
>>>>
>>>> If this sounds OK to people, I can code up a utility that does this,
>>>> which should probably belong in util-linux.
>>> This sounds a lot like shadow's newuidmap and newgidmap [1,2,3].
>>>
>>> Cheers,
>>> Trevor
>>>
>>> [1]: https://github.com/shadow-maint/shadow/commit/673c2a6f9aa6c69588f4c1be08589b8d3475a520
>>> [2]: http://man7.org/linux/man-pages/man1/newuidmap.1.html
>>> [3]: http://man7.org/linux/man-pages/man5/subuid.5.html
>> I think that mostly works.  No-one's packaging it yet, which is why I
>> didn't notice.  It also looks like the build dependencies have vastly
>> expanded, so I can't get it to build in the build service yet.
>
> Both Fedora and Ubuntu should be packaging it.  Further Docker should
> already be using these files.
Yes, based on our discussion in the PRs when user namespaces 
capabilities were added to Docker, we respect the /etc/sub{u,g}id files 
for sourcing mappings for userns-confined processes.

- Phil

Re: Unprivileged containers and co-ordinating user namespaces

[James Bottomley]

On Wed, 2016-05-04 at 10:02 -0500, Eric W. Biederman wrote:
> James Bottomley <James.Bottomley@HansenPartnership.com> writes:
> 
> > On Thu, 2016-04-28 at 16:00 -0700, W. Trevor King wrote:
> > > On Thu, Apr 28, 2016 at 03:02:08PM -0700, James Bottomley wrote:
> > > > /etc/usernamespaces
> > > > 
> > > > and the format be :::
> > > > 
> > > > …
> > > > 
> > > > If this sounds OK to people, I can code up a utility that does 
> > > > this, which should probably belong in util-linux.
> > > 
> > > This sounds a lot like shadow's newuidmap and newgidmap [1,2,3].
> > > 
> > > Cheers,
> > > Trevor
> > > 
> > > [1]: https://github.com/shadow-maint/shadow/commit/673c2a6f9aa6c6
> > > 9588f4c1be08589b8d3475a520
> > > [2]: http://man7.org/linux/man-pages/man1/newuidmap.1.html
> > > [3]: http://man7.org/linux/man-pages/man5/subuid.5.html
> > 
> > I think that mostly works.  No-one's packaging it yet, which is why 
> > I didn't notice.  It also looks like the build dependencies have
> > vastly expanded, so I can't get it to build in the build service
> > yet.
> 
> Both Fedora and Ubuntu should be packaging it.  Further Docker should
> already be using these files.
> 
> > It looks like the only addition it needs is the setgroups flag for
> > newgidmap, which the security people will need, so I can patch 
> > that.  Plus it's trying to install newgidmap/newuidmap as setuid 
> > root rather than cap_setuid/cap_setgid, but that's fixable in the 
> > spec file.
> 
> I read the rest of this thread and I don't understand the setgroups 
> flag that you desire.  It sounds like someone with an incomplete 
> grasp on the situtation being cautious.
> 
> As far as I can tell the use cases for containers not supporting
> setgroups is very limited.  Basically just using user namespaces to
> drop privileges, and mapping the existing uids and gids to 0.
> 
> I don't think it actually makes sense to have a knob.  From a 
> practical standpoint entering any subordinate ids into the subgid 
> file for a user is a permission to use groups in such a way that can 
> not use them as a negative acl (because we allow them to be dropped).
> 
> Certainly it has been that way for quite a while now.

I don't quite get this.  If setgroups is set to deny and I have a set
of group mappings, I still can't get rid of the negative acl group.  I
can map it to a different gid inside my container, or I can not map it
at all, but in either case I still can't get access to anything with
the negative acl group marker because the group permission checks
occurs with the kguid_t set which includes my mapped or unmapped group.
 The only way I can lose it is to call sys_setgroups().

It's a bit ugly because I have to enter the container with --preserve
-credentials and I can't su to myself if I enter as root (-S 0), I have
to re-enter as myself instead, but it works.

> Except for the negative acl aspect there are no issues with dropping
> groups, as setgroups will limit you to the groups allowed in your 
> user namespace.

Well, notwithstanding the merits of negative acls, which I don't want
to debate because I don't think they're that useful, the use case might
be that a user possessing a negative acl still wants to use an
architecture emulation container for building.  Installing such a
container requires being able to set a set of groups and uids (required
by the installer), but it doesn't require the sys_setgroups() system
call, so they could reasonably be given the ability to set one up with
the nosetgroups flag and a range of gids allocated in subgid to ensure
they still can't get access to resources denied by the negative acl
group.

James

_______________________________________________
Containers mailing list
Containers@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/containers