Re: Unprivileged containers and co-ordinating user namespaces

[James Bottomley <James.Bottomley@HansenPartnership.com>]

[-- 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 --]