-
Notifications
You must be signed in to change notification settings - Fork 229
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
base: master
Are you sure you want to change the base?
Conversation
bbdc6cf
to
f6f6335
Compare
This is currently still work-in-progress (even though it all works fine now). The issue is that I cannot get |
* Free a list. | ||
* The output list isn't modified, but the memory is freed. | ||
*/ | ||
void free_list (char *const *list) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: const char **list
?
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how is this "quad"?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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; | ||
|
There was a problem hiding this comment.
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
There was a problem hiding this 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
@AkihiroSuda Working on it, I'm trying to figure out how to fix AutoMake first. |
f6f6335
to
35c6e43
Compare
Alright the AutoMake issue has been resolved, so I've removed There aren't any |
35c6e43
to
8603235
Compare
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]>
8603235
to
626af91
Compare
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).
We can fix that
|
Oh, I also haven't added any way for people to set |
for (i = 0; NULL != list[i]; i++) | ||
commalen += strlen(list[i]) + 1; | ||
|
||
comma = xmalloc(commalen + 1); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?: ""); |
There was a problem hiding this comment.
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), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 usenewgidmap
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?
There was a problem hiding this comment.
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?)
Alright, I'm going to work on the |
Thanks @cyphar - should I close this one then? |
Yup, I'll close it. |
Hey @cyphar did this topic continue elsewhere? |
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? :) |
😅 I will take a look at this again this week. |
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 |
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? |
Hi @cyphar - no pressure, just wondering - have you given this any more thought? :) |
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]