Skip to content
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.

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

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

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

Conversation

cyphar
Copy link
Contributor

@cyphar cyphar commented Feb 18, 2018

Add a new deny_setgroups (and corresponding allow_setgroups) flag to
/etc/subgid. The purpose of this flag is to extend the security
protections against CVE-2018-7169, so that even group mapping configured
in /etc/subgid by an administrator can still disable setgroups.

However, rather than the fairly lenient semantics for self-mapping, the
semantics of /etc/subgid are stronger. If a mapping is encountered where
"deny_setgroups" is set, then no other mapping can "undo" this
restriction. The reason for this is that "deny_setgroups" indicates that
(according to the administrator) the mapping is unsafe to allow setgroups
in, and adding more mappings will not change this fact. "allow_setgroups"
is the default, and setting it is a noop. The logic used when applying
setgroups policies is unchanged (only denies are written, and we don't
write anything if it's already denied).

Signed-off-by: Aleksa Sarai [email protected]

@cyphar cyphar force-pushed the subgid-flag-deny_setgroups branch 2 times, most recently from bbdc6cf to f6f6335 Compare February 18, 2018 20:07
@cyphar
Copy link
Contributor Author

cyphar commented Feb 18, 2018

This is currently still work-in-progress (even though it all works fine now). The issue is that I cannot get lib/ to use libmisc/list.c because automake really doesn't like me trying to add an LDADD for libmisc.la to lib_la_LDADD. So currently I just copied list.c to lib/ (which is ... bad).

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

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

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.

src/newgidmap.c Outdated
@@ -46,16 +46,83 @@
*/
const char *Prog;

enum quadopt_t {

Choose a reason for hiding this comment

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

how is this "quad"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm using the term quadoption from Mutt, but you're right that this might not be the best name.

Choose a reason for hiding this comment

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

Actually this looks triopt, as it has three values.

In github.com/moby/buildkit we call this BoolOrAuto.

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 know it has three values, I was referring to "quadoption" as a term in Mutt (see §26.1 of the Mutt manual). But maybe defaultbool_t is a better name. I'll fix it once I can fix the lib/ build issues.


if (strlen(flag) < 1)
continue;

Choose a reason for hiding this comment

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

Please add a comment about that no uid flag is valid atm

Copy link

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

could you update man pages as well

@cyphar
Copy link
Contributor Author

cyphar commented Feb 19, 2018

@AkihiroSuda Working on it, I'm trying to figure out how to fix AutoMake first.

@cyphar cyphar changed the title newsubgid: add deny_setgroups flag to /etc/subgid newsubgid: add deny_setgroups option to /etc/subgid Feb 19, 2018
@cyphar
Copy link
Contributor Author

cyphar commented Feb 19, 2018

Alright the AutoMake issue has been resolved, so I've removed lib/list.c. I've also now included manpage updates for this change as well as #97 (to clarify when and how newgidmap will write deny), as well as changed quadoption to autobool (and added helpers for autobool_t).

There aren't any newgidmap tests, but I have a feeling we should probably add some (though the tests aren't run as part of Travis -- restricting their usefulness).

Add a free_list helper so that we can duplicate list to callers and
require them to do the freeing for us, as well as a comma_from_list
helper for the /etc/sub{uid,gid} writing functions. In addition, fix up
the duplication code to just ignore NULL lists rather than aborting (for
no good reason).

As an aside, we really should switch to sharing the list code from
somewhere else rather than maintaining a separate version within
shadow-utils...

Signed-off-by: Aleksa Sarai <[email protected]>
Add support for an optional options field in /etc/sub{uid,gid}. We treat
this like other optional fields in /etc/passwd -- by ignoring its
non-existence and providing some functions to access the options.

We need these in order to be able to have the "allow_setgroups" and
"deny_setgroups" options in /etc/sub{uid,gid}.

This also required making libmisc a dependency of libshadow, which in
turn required converting libmisc to a libtool library so that AutoMake
didn't complain. It appears this mostly doesn't change any aspect of the
build other than allowing us to use libmisc symbols in libshadow.

Signed-off-by: Aleksa Sarai <[email protected]>
Add the most basic support for the new /etc/sub{uid,gid} options
possible (parse them and if any options are present then output an
error). We ignore empty-string options to avoid cases where an empty
field breaks things.

Signed-off-by: Aleksa Sarai <[email protected]>
Add a new deny_setgroups (and corresponding allow_setgroups) option to
/etc/subgid. The purpose of this option is to extend the security
protections against CVE-2018-7169, so that even group mapping configured
in /etc/subgid by an administrator can still disable setgroups.

However, rather than the fairly lenient semantics for self-mapping, the
semantics of /etc/subgid are stronger. If a mapping is encountered where
"deny_setgroups" is set, then no other mapping can "undo" this
restriction. The reason for this is that "deny_setgroups" indicates that
(according to the administrator) the mapping is unsafe to allow
setgroups in, and adding more mappings will not change this fact.
"allow_setgroups" is the default, and setting it is a noop. The logic
used when applying setgroups policies is unchanged (only denies are
written, and we don't write anything if it's already denied).

Signed-off-by: Aleksa Sarai <[email protected]>
Add documentation for allow_setgroups, deny_setgroups, the new option
format of /etc/sub{uid,gid}, and fix some errors in the groupmod(8) man
page that stopped it from building properly on my machine.

Signed-off-by: Aleksa Sarai <[email protected]>
@hallyn
Copy link
Member

hallyn commented Feb 19, 2018 via email

@cyphar
Copy link
Contributor Author

cyphar commented Feb 19, 2018

Oh, I also haven't added any way for people to set deny_setgroups (or other options in the future) within usermod or useradd -- I couldn't figure out what the interface should look like. Any suggestions, or should we leave that feature for later?

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

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

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.

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

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

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

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 :)

range->owner,
range->start,
range->count,
options ?: "");
Copy link
Member

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 : ""


<variablelist>
<varlistentry>
<term><option>allow_setgroups</option> (default),
Copy link
Member

Choose a reason for hiding this comment

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

Hm, is this the right place to put this kind of thing though? Maybe it is, but:

The only case I know of for deny-setgroups is when an administrator has placed a user in a group with a negative acl, to prevent that user from accessing some resource.

In that case, does it make more sense to place a sort of "group_locked" option on the group itself in /etc/group? Then when newgidmap runs, it checks whether the target pid is in any locked group, and if so sets deny_setgroup.

Placing the deny/allow_setgroup variable in the gid mapping seems a step removed, and might cause an admin to not notice that "hey, the user being allowd to grab those gids should have been locked into group nos3critdocs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I hadn't though of this point.

You're quite right that most users of negative ACLs are done with groups (and I think it does make intuitive sense to be able to say "if you are in a bad group then you cannot use setgroups"), but I have a couple of questions about edge cases:

  • Should an administrator be able to override this decision for particular users or groups? Should that also be in /etc/groups (or /etc/passwd)?

  • What if it's a numeric group (so not one registered in /etc/groups)? Is the solution to just tell people "tough luck, you should register your group"?

  • This is a bit of an edge condition (and is an edge condition that actually isn't really easy to handle with the current implementation either), but how will this affect containers on the system that are also using group blacklisting? While /etc/sub* are global, if you have containers using allocated portions of the UID/GID space then you would expect that a user shouldn't be able to use newgidmap to drop the negative ACLs for a different group set than the one you are joining? Now, this is quite a weird case because it would require you to have a quite strange ACL setup -- so maybe this is a use case we don't care about?

Copy link
Member

Choose a reason for hiding this comment

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

Should an administrator be able to override this decision for particular users or groups? Should that also be in /etc/groups (or /etc/passwd)?

I don't really think so, as you either want to deny the group certain access or you don't, but my gut says there are more subtle cases. So let's say yes.

What if it's a numeric group (so not one registered in /etc/groups)? Is the solution to just tell people "tough luck, you should register your group"?

I think requiring a name makes sense. Do you?

how will this affect containers on the system that are also using group blacklisting?

Hmm - that applies to your approach too right?

It really seems like this requires a new kernel functionality to make the most sense - a new per-process 'lockedgrp' from which groups can never be removed. Otherwise all we can really do in the parent container is say "never let the child container do a setgroup" (right? or have i confused myself on this topic once again?)

@cyphar
Copy link
Contributor Author

cyphar commented Mar 28, 2018

Alright, I'm going to work on the /etc/group configuration method in a separate branch and I'll make a separate PR once it's ready.

@hallyn
Copy link
Member

hallyn commented Mar 30, 2018

Thanks @cyphar - should I close this one then?

@cyphar
Copy link
Contributor Author

cyphar commented Mar 30, 2018

Yup, I'll close it.

@cyphar cyphar closed this Mar 30, 2018
@hallyn
Copy link
Member

hallyn commented Aug 29, 2023

Hey @cyphar did this topic continue elsewhere?

@hallyn hallyn reopened this Aug 29, 2023
@cyphar
Copy link
Contributor Author

cyphar commented Aug 30, 2023

No, I started working on it and ran into some roadbump and then moved on to something else. I can take a look at reviving this when I get back from vacation in ~2 weeks.

@hallyn
Copy link
Member

hallyn commented Nov 26, 2023

No, I started working on it and ran into some roadbump and then moved on to something else. I can take a look at reviving this when I get back from vacation in ~2 weeks.

ping? :)

@cyphar
Copy link
Contributor Author

cyphar commented Nov 27, 2023

😅 I will take a look at this again this week.

@cyphar
Copy link
Contributor Author

cyphar commented Nov 28, 2023

Okay, I've started rewriting at this again, and I've remembered the roadblock I ran into last time.

@hallyn How should I handle the fact that struct group comes from libc and thus we can't add fields to it? Adding an argument to pass back the flags would be a little invasive, should I just wrap struct group into struct group_ext (or similar)? Are there users of our internal libraries, or can we change stuff like this without issue?

@hallyn
Copy link
Member

hallyn commented Dec 27, 2023

Okay, I've started rewriting at this again, and I've remembered the roadblock I ran into last time.

@hallyn How should I handle the fact that struct group comes from libc and thus we can't add fields to it? Adding an argument to pass back the flags would be a little invasive, should I just wrap struct group into struct group_ext (or similar)? Are there users of our internal libraries, or can we change stuff like this without issue?

D'oh, sorry, I never replied to this...

There are no outside users of our internal libraries that I know of. Only libsubid. Introducing struct group_ext should be fine. I can't visualize right now where all you need this, so not quite sure whether there would be a better way. Could you post a link to your current branch so I can take a look?

@hallyn
Copy link
Member

hallyn commented Aug 2, 2024

Hi @cyphar - no pressure, just wondering - have you given this any more thought? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants