Skip to content

newsubgid: add deny_setgroups option to /etc/subgid #99

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cyphar wants to merge 5 commits into
base: master
Choose a base branch
from
Open

newsubgid: add deny_setgroups option to /etc/subgid #99

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fdfb3b9
libmisc: list: update helpers
cyphar Feb 16, 2018
4a43149
lib: subordinateio: add options field to /etc/sub{uid,gid}
cyphar Feb 18, 2018
0d368da
src: new{uid,gid}map: add basic (noop) support for options
cyphar Feb 18, 2018
b1a1af3
newgidmap: add deny_setgroups option to /etc/subgid
cyphar Feb 18, 2018
626af91
man: add documentation for new setgroups(2) semantics
cyphar Feb 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@

AUTOMAKE_OPTIONS = 1.0 foreign

DEFS =
DEFS =

noinst_LTLIBRARIES = libshadow.la

libshadow_la_LIBADD = ../libmisc/libmisc.la
libshadow_la_LDFLAGS = -version-info 0:0:0

libshadow_la_SOURCES = \
Expand Down
2 changes: 2 additions & 0 deletions lib/prototypes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,8 +229,10 @@ extern void setup_limits (const struct passwd *);
extern /*@only@*/ /*@out@*/char **add_list (/*@returned@*/ /*@only@*/char **, const char *);
extern /*@only@*/ /*@out@*/char **del_list (/*@returned@*/ /*@only@*/char **, const char *);
extern /*@only@*/ /*@out@*/char **dup_list (char *const *);
extern /*@only@*/ /*@out@*/void free_list (char *const *);
extern bool is_on_list (char *const *list, const char *member);
extern /*@only@*/char **comma_to_list (const char *);
extern /*@only@*/char *comma_from_list (char *const *);

/* log.c */
extern void dolastlog (
Expand Down
76 changes: 52 additions & 24 deletions lib/subordinateio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,11 @@ struct subordinate_range {
const char *owner;
unsigned long start;
unsigned long count;
char **options;
};

#define NFIELDS 3
#define NFIELDS_MIN 3
#define NFIELDS_MAX 4

/*
* subordinate_dup: create a duplicate range
Expand All @@ -46,6 +48,7 @@ static /*@null@*/ /*@only@*/void *subordinate_dup (const void *ent)
}
range->start = rangeent->start;
range->count = rangeent->count;
range->options = dup_list(rangeent->options);

return range;
}
Expand All @@ -58,8 +61,9 @@ static /*@null@*/ /*@only@*/void *subordinate_dup (const void *ent)
static void subordinate_free (/*@out@*/ /*@only@*/void *ent)
{
struct subordinate_range *rangeent = ent;

free ((void *)(rangeent->owner));
free_list(rangeent->options);
free (rangeent);
}

Expand All @@ -76,9 +80,9 @@ static void *subordinate_parse (const char *line)
{
static struct subordinate_range range;
static char rangebuf[1024];
int i;
int i, j;
char *cp;
char *fields[NFIELDS];
char *fields[NFIELDS_MAX];

/*
* Copy the string to a temporary buffer so the substrings can
Expand All @@ -93,7 +97,7 @@ static void *subordinate_parse (const char *line)
* field. The fields are converted into NUL terminated strings.
*/

for (cp = rangebuf, i = 0; (i < NFIELDS) && (NULL != cp); i++) {
for (cp = rangebuf, i = 0; (i < NFIELDS_MAX) && (NULL != cp); i++) {
fields[i] = cp;
while (('\0' != *cp) && (':' != *cp)) {
cp++;
Expand All @@ -108,16 +112,23 @@ 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.
* There must be at least NFIELDS_MIN and at most NFIELDS_MAX colon
* separated fields or the entry is invalid. The first NFIELDS_MIN fields
* must be non-blank.
*/
if (i != NFIELDS || *fields[0] == '\0' || *fields[1] == '\0' || *fields[2] == '\0')
if (i < NFIELDS_MIN || i > NFIELDS_MAX)
return NULL;
for (j = 0; j < NFIELDS_MIN; j++)
if (*fields[j] == '\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 >= 4)
range.options = comma_to_list(fields[3]);

return &range;
}
Expand All @@ -133,11 +144,15 @@ static void *subordinate_parse (const char *line)
static int subordinate_put (const void *ent, FILE * file)
{
const struct subordinate_range *range = ent;
char *options = comma_from_list(range->options);

return fprintf(file, "%s:%lu:%lu\n",
range->owner,
range->start,
range->count) < 0 ? -1 : 0;
int n = fprintf(file, "%s:%lu:%lu:%s\n",
range->owner,
range->start,
range->count,
options ?: "");
Copy link
Member

@hallyn hallyn Feb 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iiuc this is a gnu extension, probably not safe for shadow to use, so please make it options ? options : ""

Copy link
Collaborator

@alejandro-colomar alejandro-colomar Dec 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have recently embraced this GNU extension.

free(options);
return n < 0 ? -1 : 0;
}

static struct commonio_ops subordinate_ops = {
Expand Down Expand Up @@ -285,38 +300,41 @@ static const struct subordinate_range *find_range(struct commonio_db *db,
}

/*
* have_range: check whether @owner is authorized to use the range
* (@start .. @start+@count-1).
* get_range: check whether @owner is authorized to use the range
* (@start .. @start+@count-1) and return the first matching range.
* @db: database to check
* @owner: owning uid being queried
* @start: start of range
* @count: number of uids in range
*
* Returns true if @owner is authorized to use the range, false otherwise.
* Returns the corresponding subordinate_range if @owner is authorized to use
* the range, NULL otherwise.
*/
static bool have_range(struct commonio_db *db,
const char *owner, unsigned long start, unsigned long count)
static const struct subordinate_range *get_range(struct commonio_db *db,
const char *owner,
unsigned long start,
unsigned long count)
{
const struct subordinate_range *range;
unsigned long end;

if (count == 0)
return false;
return NULL;

end = start + count - 1;
range = find_range (db, owner, start);
while (range) {
unsigned long last;
unsigned long last;

last = range->start + range->count - 1;
if (last >= (start + count - 1))
return true;
return range;

count = end - last;
start = last + 1;
range = find_range(db, owner, start);
}
return false;
return NULL;
}

/*
Expand Down Expand Up @@ -430,7 +448,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 (get_range(db, owner, start, count))
return 1;

/* Otherwise append the range */
Expand Down Expand Up @@ -585,7 +603,12 @@ 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 NULL != get_range (&subordinate_uid_db, owner, start, count);
}

char **sub_uid_options(const char *owner, uid_t start, unsigned long count)
{
return dup_list(get_range(&subordinate_uid_db, owner, start, count)->options);
}

int sub_uid_add (const char *owner, uid_t start, unsigned long count)
Expand Down Expand Up @@ -661,7 +684,12 @@ int sub_gid_open (int mode)

bool have_sub_gids(const char *owner, gid_t start, unsigned long count)
{
return have_range(&subordinate_gid_db, owner, start, count);
return NULL != get_range (&subordinate_gid_db, owner, start, count);
}

char **sub_gid_options(const char *owner, gid_t start, unsigned long count)
{
return dup_list(get_range(&subordinate_gid_db, owner, start, count)->options);
}

bool sub_gid_assigned(const char *owner)
Expand Down
2 changes: 2 additions & 0 deletions lib/subordinateio.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

extern int sub_uid_close(void);
extern bool have_sub_uids(const char *owner, uid_t start, unsigned long count);
extern char **sub_uid_options(const char *owner, uid_t start, unsigned long count);
extern bool sub_uid_file_present (void);
extern bool sub_uid_assigned(const char *owner);
extern int sub_uid_lock (void);
Expand All @@ -26,6 +27,7 @@ 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 char **sub_gid_options(const char *owner, uid_t start, unsigned long count);
extern bool sub_gid_file_present (void);
extern bool sub_gid_assigned(const char *owner);
extern int sub_gid_lock (void);
Expand Down
4 changes: 2 additions & 2 deletions libmisc/Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ EXTRA_DIST = .indent.pro xgetXXbyYY.c

AM_CPPFLAGS = -I$(top_srcdir)/lib

noinst_LIBRARIES = libmisc.a
noinst_LTLIBRARIES = libmisc.la

libmisc_a_SOURCES = \
libmisc_la_SOURCES = \
addgrps.c \
age.c \
audit_help.c \
Expand Down
47 changes: 46 additions & 1 deletion libmisc/list.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,8 @@
int i;
char **tmp;

assert (NULL != list);
if (NULL == list)
return NULL;

for (i = 0; NULL != list[i]; i++);

Expand All @@ -169,6 +170,23 @@
return tmp;
}

/*
* Free a list.
* The output list isn't modified, but the memory is freed.
*/
void free_list (char *const *list)
Copy link

@AkihiroSuda AkihiroSuda Feb 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: const char **list ?

Copy link
Contributor Author

@cyphar cyphar Feb 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this to match the signature of dup_list, but I don't really mind either way.

{
int i;

if (NULL == list)
return;

for (i = 0; NULL != list[i]; i++)
free(list[i]);
free((void *)list);
}


/*
* Check if member is part of the input list
* The input list is not modified, but in order to allow the use of this
Expand Down Expand Up @@ -269,3 +287,30 @@ bool is_on_list (char *const *list, const char *member)
return array;
}

/*
* comma_from_list - inverse of comma_to_list
*/

/*@only@*/char *comma_from_list (char *const *list)
{
char *comma;
int i, commalen = 0;

if (NULL == list)
return NULL;

for (i = 0; NULL != list[i]; i++)
commalen += strlen(list[i]) + 1;

comma = xmalloc(commalen + 1);
Copy link
Member

@hallyn hallyn Feb 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically +1 shouldn't be needed here right? Each entry is followed by either comma or \0, and you add +1 to the length of each entry as you add up.

Copy link
Collaborator

@alejandro-colomar alejandro-colomar Dec 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly, the loops are simpler if we leave a trailing comma, so that a CSV would look like a,b,c,\0. It is technically incorrect as a CSV --as you have a trailing empty element--, but we do support such CSVs elsewhere, so we could keep the +1.

memset(comma, '\0', commalen + 1);

for (i = 0; NULL != list[i]; i++) {
int j = strlen(comma);
Copy link
Member

@hallyn hallyn Feb 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't really performance critical, but re-calculating j at each iteration here is unnecessary.

Copy link
Member

@hallyn hallyn Feb 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, but you're using strncat which doesn't give you the printed length, I see :)

Copy link
Collaborator

@alejandro-colomar alejandro-colomar Dec 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now have stpecpy(), which is designed precisely with this in mind.

if (j > 0)
comma[j++] = ',';
strncat(comma, list[i], commalen - j);
}

return comma;
}
Comment on lines +308 to +316
Copy link
Collaborator

@alejandro-colomar alejandro-colomar Dec 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use stpecpy() instead. We have forbidden strncat() with string input; we only allow it with [[gnu::nonstring]] input --that is, utmp(5) members--.

With stpecpy() --which is defined in lib/string/--, it would be:

char  *p, *e;

[...]

p = comma;
e = comma + commalen + 1;
for (int i = 0; NULL != list[i]; i++) {
	p = stpecpy(p, e, list[i]);
	p = stpecpy(p, e, ",");
}

if (p == NULL) {
	free(comma);
	return NULL;
}

return comma;

Copy link
Collaborator

@alejandro-colomar alejandro-colomar Dec 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, since we know there won't be truncation, we can just use stpcpy(3) from libc.

char  *p;

[...]

p = comma;
for (int i = 0; NULL != list[i]; i++) {
	p = stpecpy(p, list[i]);
	p = stpecpy(p, ",");
}

return comma;

9 changes: 6 additions & 3 deletions man/groupmod.8.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
Copyright (c) 1991 , Julianne Frances Haugh
Copyright (c) 2007 - 2011, Nicolas François
All rights reserved.

Redistribution and use in source and binary forms, with or without
modification, are permitted provided that the following conditions
are met:
Expand All @@ -15,7 +15,7 @@
3. The name of the copyright holders or contributors may not be used to
endorse or promote products derived from this software without
specific prior written permission.

THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
``AS IS'' AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
Expand Down Expand Up @@ -136,7 +136,7 @@
<option>-n</option>, <option>--new-name</option>&nbsp;<replaceable>NEW_GROUP</replaceable>
</term>
<listitem>
<para>
<para>
The name of the group will be changed from <replaceable>GROUP</replaceable>
to <replaceable>NEW_GROUP</replaceable> name.
</para>
Expand Down Expand Up @@ -278,16 +278,19 @@
<para>E_GRP_UPDATE: can't update group file</para>
</listitem>
</varlistentry>
<varlistentry>
<term><replaceable>11</replaceable></term>
<listitem>
<para>E_CLEANUP_SERVICE: can't setup cleanup service</para>
</listitem>
</varlistentry>
<varlistentry>
<term><replaceable>12</replaceable></term>
<listitem>
<para>E_PAM_USERNAME: can't determine your username for use with pam</para>
</listitem>
</varlistentry>
<varlistentry>
<term><replaceable>13</replaceable></term>
<listitem>
<para>E_PAM_ERROR: pam returned an error, see syslog facility id groupmod for the PAM error message</para>
Expand Down
21 changes: 19 additions & 2 deletions man/newgidmap.1.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
<!--
Copyright (c) 2013 Eric W. Biederman
All rights reserved.

Redistribution and use in source and binary forms, with or without
modification, are permitted provided that the following conditions
are met:
Expand All @@ -14,7 +14,7 @@
3. The name of the copyright holders or contributors may not be used to
endorse or promote products derived from this software without
specific prior written permission.

THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
``AS IS'' AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
Expand Down Expand Up @@ -122,12 +122,29 @@
of the above sets, each of the GIDs in the range [lowergid,
lowergid+count] is allowed to the caller according to
<filename>/etc/subgid</filename> before setting
<filename>/proc/[pid]/setgroups</filename> and
<filename>/proc/[pid]/gid_map</filename>.
</para>

<para>
Note that newgidmap may be used only once for a given process.
</para>

<para>
<command>newgidmap</command> also allows you to map a user's own
effective group ID without it being specified in
<filename>/etc/subgid</filename> (in order to match the "unprivileged
user namespaces" feature in Linux 3.8). If this is the only mapping
requested (in order to match the security protections from Linux 3.19),
<command>newgidmap</command> will ensure that
<filename>/proc/[pid]/setgroups</filename> is set to "deny" (either by
writing "deny" itself or seeing that it is already set to "deny"), and
will fail if writing "deny" failed. This restriction is also applied if
any of the mappings given to <command>newgidmap</command> has the
<option>deny_setgroups</option> option set in
<filename>/etc/subgid</filename>.
</para>

</refsect1>

<refsect1 id='options'>
Expand Down
Loading