> > 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@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/containers
