* [PATCH 1/2] groups: Factor out a function to set a pre-sorted group list @ 2014-11-15 9:00 ` Josh Triplett 0 siblings, 0 replies; 73+ messages in thread From: Josh Triplett @ 2014-11-15 9:00 UTC (permalink / raw) To: Andrew Morton, Eric W. Biederman, Kees Cook, mtk.manpages, linux-api, linux-man, linux-kernel This way, functions that already need to sort the group list need not do so twice. The new set_groups_sorted is intentionally not exported. Signed-off-by: Josh Triplett <josh@joshtriplett.org> --- kernel/groups.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/kernel/groups.c b/kernel/groups.c index 451698f..f0667e7 100644 --- a/kernel/groups.c +++ b/kernel/groups.c @@ -154,16 +154,26 @@ int groups_search(const struct group_info *group_info, kgid_t grp) } /** + * set_groups_sorted - Change a group subscription in a set of credentials + * @new: The newly prepared set of credentials to alter + * @group_info: The group list to install; must be sorted + */ +static void set_groups_sorted(struct cred *new, struct group_info *group_info) +{ + put_group_info(new->group_info); + get_group_info(group_info); + new->group_info = group_info; +} + +/** * set_groups - Change a group subscription in a set of credentials * @new: The newly prepared set of credentials to alter * @group_info: The group list to install */ void set_groups(struct cred *new, struct group_info *group_info) { - put_group_info(new->group_info); groups_sort(group_info); - get_group_info(group_info); - new->group_info = group_info; + set_groups_sorted(new, group_info); } EXPORT_SYMBOL(set_groups); -- 2.1.3 ^ permalink raw reply related [flat|nested] 73+ messages in thread
* [PATCH 1/2] groups: Factor out a function to set a pre-sorted group list @ 2014-11-15 9:00 ` Josh Triplett 0 siblings, 0 replies; 73+ messages in thread From: Josh Triplett @ 2014-11-15 9:00 UTC (permalink / raw) To: Andrew Morton, Eric W. Biederman, Kees Cook, mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w, linux-api-u79uwXL29TY76Z2rM5mHXA, linux-man-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA This way, functions that already need to sort the group list need not do so twice. The new set_groups_sorted is intentionally not exported. Signed-off-by: Josh Triplett <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org> --- kernel/groups.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/kernel/groups.c b/kernel/groups.c index 451698f..f0667e7 100644 --- a/kernel/groups.c +++ b/kernel/groups.c @@ -154,16 +154,26 @@ int groups_search(const struct group_info *group_info, kgid_t grp) } /** + * set_groups_sorted - Change a group subscription in a set of credentials + * @new: The newly prepared set of credentials to alter + * @group_info: The group list to install; must be sorted + */ +static void set_groups_sorted(struct cred *new, struct group_info *group_info) +{ + put_group_info(new->group_info); + get_group_info(group_info); + new->group_info = group_info; +} + +/** * set_groups - Change a group subscription in a set of credentials * @new: The newly prepared set of credentials to alter * @group_info: The group list to install */ void set_groups(struct cred *new, struct group_info *group_info) { - put_group_info(new->group_info); groups_sort(group_info); - get_group_info(group_info); - new->group_info = group_info; + set_groups_sorted(new, group_info); } EXPORT_SYMBOL(set_groups); -- 2.1.3 -- To unsubscribe from this list: send the line "unsubscribe linux-man" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 73+ messages in thread
* [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups 2014-11-15 9:00 ` Josh Triplett (?) @ 2014-11-15 9:01 ` Josh Triplett 2014-11-15 15:37 ` Eric W. Biederman 2014-11-17 18:06 ` Casey Schaufler -1 siblings, 2 replies; 73+ messages in thread From: Josh Triplett @ 2014-11-15 9:01 UTC (permalink / raw) To: Andrew Morton, Eric W. Biederman, Kees Cook, mtk.manpages, linux-api, linux-man, linux-kernel Currently, unprivileged processes (without CAP_SETGID) cannot call setgroups at all. In particular, processes with a set of supplementary groups cannot further drop permissions without obtaining elevated permissions first. Allow unprivileged processes to call setgroups with a subset of their current groups; only require CAP_SETGID to add a group the process does not currently have. The kernel already maintains the list of supplementary group IDs in sorted order, and setgroups already needs to sort the new list, so this just requires a linear comparison of the two sorted lists. This moves the CAP_SETGID test from setgroups into set_current_groups. Tested via the following test program: #include <err.h> #include <grp.h> #include <stdio.h> #include <sys/types.h> #include <unistd.h> void run_id(void) { pid_t p = fork(); switch (p) { case -1: err(1, "fork"); case 0: execl("/usr/bin/id", "id", NULL); err(1, "exec"); default: if (waitpid(p, NULL, 0) < 0) err(1, "waitpid"); } } int main(void) { gid_t list1[] = { 1, 2, 3, 4, 5 }; gid_t list2[] = { 2, 3, 4 }; run_id(); if (setgroups(5, list1) < 0) err(1, "setgroups 1"); run_id(); if (setresgid(1, 1, 1) < 0) err(1, "setresgid"); if (setresuid(1, 1, 1) < 0) err(1, "setresuid"); run_id(); if (setgroups(3, list2) < 0) err(1, "setgroups 2"); run_id(); if (setgroups(5, list1) < 0) err(1, "setgroups 3"); run_id(); return 0; } Without this patch, the test program gets EPERM from the second setgroups call, after dropping root privileges. With this patch, the test program successfully drops groups 1 and 5, but then gets EPERM from the third setgroups call, since that call attempts to add groups the process does not currently have. Signed-off-by: Josh Triplett <josh@joshtriplett.org> --- kernel/groups.c | 33 ++++++++++++++++++++++++++++++--- kernel/uid16.c | 2 -- 2 files changed, 30 insertions(+), 5 deletions(-) diff --git a/kernel/groups.c b/kernel/groups.c index f0667e7..fe7367d 100644 --- a/kernel/groups.c +++ b/kernel/groups.c @@ -153,6 +153,29 @@ int groups_search(const struct group_info *group_info, kgid_t grp) return 0; } +/* Compare two sorted group lists; return true if the first is a subset of the + * second. + */ +static bool is_subset(const struct group_info *g1, const struct group_info *g2) +{ + unsigned int i, j; + + for (i = 0, j = 0; i < g1->ngroups; i++) { + kgid_t gid1 = GROUP_AT(g1, i); + kgid_t gid2; + for (; j < g2->ngroups; j++) { + gid2 = GROUP_AT(g2, j); + if (gid_lte(gid1, gid2)) + break; + } + if (j >= g2->ngroups || !gid_eq(gid1, gid2)) + return false; + j++; + } + + return true; +} + /** * set_groups_sorted - Change a group subscription in a set of credentials * @new: The newly prepared set of credentials to alter @@ -189,11 +212,17 @@ int set_current_groups(struct group_info *group_info) { struct cred *new; + groups_sort(group_info); new = prepare_creds(); if (!new) return -ENOMEM; + if (!ns_capable(current_user_ns(), CAP_SETGID) + && !is_subset(group_info, new->group_info)) { + abort_creds(new); + return -EPERM; + } - set_groups(new, group_info); + set_groups_sorted(new, group_info); return commit_creds(new); } @@ -233,8 +262,6 @@ SYSCALL_DEFINE2(setgroups, int, gidsetsize, gid_t __user *, grouplist) struct group_info *group_info; int retval; - if (!ns_capable(current_user_ns(), CAP_SETGID)) - return -EPERM; if ((unsigned)gidsetsize > NGROUPS_MAX) return -EINVAL; diff --git a/kernel/uid16.c b/kernel/uid16.c index 602e5bb..b27e167 100644 --- a/kernel/uid16.c +++ b/kernel/uid16.c @@ -176,8 +176,6 @@ SYSCALL_DEFINE2(setgroups16, int, gidsetsize, old_gid_t __user *, grouplist) struct group_info *group_info; int retval; - if (!ns_capable(current_user_ns(), CAP_SETGID)) - return -EPERM; if ((unsigned)gidsetsize > NGROUPS_MAX) return -EINVAL; -- 2.1.3 ^ permalink raw reply related [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-15 15:37 ` Eric W. Biederman 0 siblings, 0 replies; 73+ messages in thread From: Eric W. Biederman @ 2014-11-15 15:37 UTC (permalink / raw) To: Josh Triplett Cc: Andrew Morton, Kees Cook, mtk.manpages, linux-api, linux-man, linux-kernel Josh Triplett <josh@joshtriplett.org> writes: > Currently, unprivileged processes (without CAP_SETGID) cannot call > setgroups at all. In particular, processes with a set of supplementary > groups cannot further drop permissions without obtaining elevated > permissions first. > > Allow unprivileged processes to call setgroups with a subset of their > current groups; only require CAP_SETGID to add a group the process does > not currently have. A couple of questions. - Is there precedence in other unix flavors for this? - What motiviates this change? - Have you looked to see if anything might for bug compatibilty require applications not to be able to drop supplementary groups? > The kernel already maintains the list of supplementary group IDs in > sorted order, and setgroups already needs to sort the new list, so this > just requires a linear comparison of the two sorted lists. > > This moves the CAP_SETGID test from setgroups into set_current_groups. > > Tested via the following test program: > > #include <err.h> > #include <grp.h> > #include <stdio.h> > #include <sys/types.h> > #include <unistd.h> > > void run_id(void) > { > pid_t p = fork(); > switch (p) { > case -1: > err(1, "fork"); > case 0: > execl("/usr/bin/id", "id", NULL); > err(1, "exec"); > default: > if (waitpid(p, NULL, 0) < 0) > err(1, "waitpid"); > } > } > > int main(void) > { > gid_t list1[] = { 1, 2, 3, 4, 5 }; > gid_t list2[] = { 2, 3, 4 }; > run_id(); > if (setgroups(5, list1) < 0) > err(1, "setgroups 1"); > run_id(); > if (setresgid(1, 1, 1) < 0) > err(1, "setresgid"); > if (setresuid(1, 1, 1) < 0) > err(1, "setresuid"); > run_id(); > if (setgroups(3, list2) < 0) > err(1, "setgroups 2"); > run_id(); > if (setgroups(5, list1) < 0) > err(1, "setgroups 3"); > run_id(); > > return 0; > } > > Without this patch, the test program gets EPERM from the second > setgroups call, after dropping root privileges. With this patch, the > test program successfully drops groups 1 and 5, but then gets EPERM from > the third setgroups call, since that call attempts to add groups the > process does not currently have. > > Signed-off-by: Josh Triplett <josh@joshtriplett.org> > --- > kernel/groups.c | 33 ++++++++++++++++++++++++++++++--- > kernel/uid16.c | 2 -- > 2 files changed, 30 insertions(+), 5 deletions(-) > > diff --git a/kernel/groups.c b/kernel/groups.c > index f0667e7..fe7367d 100644 > --- a/kernel/groups.c > +++ b/kernel/groups.c > @@ -153,6 +153,29 @@ int groups_search(const struct group_info *group_info, kgid_t grp) > return 0; > } > > +/* Compare two sorted group lists; return true if the first is a subset of the > + * second. > + */ > +static bool is_subset(const struct group_info *g1, const struct group_info *g2) > +{ > + unsigned int i, j; > + > + for (i = 0, j = 0; i < g1->ngroups; i++) { > + kgid_t gid1 = GROUP_AT(g1, i); > + kgid_t gid2; > + for (; j < g2->ngroups; j++) { > + gid2 = GROUP_AT(g2, j); > + if (gid_lte(gid1, gid2)) > + break; > + } > + if (j >= g2->ngroups || !gid_eq(gid1, gid2)) > + return false; > + j++; > + } > + > + return true; > +} > + > /** > * set_groups_sorted - Change a group subscription in a set of credentials > * @new: The newly prepared set of credentials to alter > @@ -189,11 +212,17 @@ int set_current_groups(struct group_info *group_info) > { > struct cred *new; > > + groups_sort(group_info); > new = prepare_creds(); > if (!new) > return -ENOMEM; > + if (!ns_capable(current_user_ns(), CAP_SETGID) > + && !is_subset(group_info, new->group_info)) { > + abort_creds(new); > + return -EPERM; > + } > > - set_groups(new, group_info); > + set_groups_sorted(new, group_info); > return commit_creds(new); > } > > @@ -233,8 +262,6 @@ SYSCALL_DEFINE2(setgroups, int, gidsetsize, gid_t __user *, grouplist) > struct group_info *group_info; > int retval; > > - if (!ns_capable(current_user_ns(), CAP_SETGID)) > - return -EPERM; > if ((unsigned)gidsetsize > NGROUPS_MAX) > return -EINVAL; > > diff --git a/kernel/uid16.c b/kernel/uid16.c > index 602e5bb..b27e167 100644 > --- a/kernel/uid16.c > +++ b/kernel/uid16.c > @@ -176,8 +176,6 @@ SYSCALL_DEFINE2(setgroups16, int, gidsetsize, old_gid_t __user *, grouplist) > struct group_info *group_info; > int retval; > > - if (!ns_capable(current_user_ns(), CAP_SETGID)) > - return -EPERM; > if ((unsigned)gidsetsize > NGROUPS_MAX) > return -EINVAL; ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-15 15:37 ` Eric W. Biederman 0 siblings, 0 replies; 73+ messages in thread From: Eric W. Biederman @ 2014-11-15 15:37 UTC (permalink / raw) To: Josh Triplett Cc: Andrew Morton, Kees Cook, mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w, linux-api-u79uwXL29TY76Z2rM5mHXA, linux-man-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA Josh Triplett <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org> writes: > Currently, unprivileged processes (without CAP_SETGID) cannot call > setgroups at all. In particular, processes with a set of supplementary > groups cannot further drop permissions without obtaining elevated > permissions first. > > Allow unprivileged processes to call setgroups with a subset of their > current groups; only require CAP_SETGID to add a group the process does > not currently have. A couple of questions. - Is there precedence in other unix flavors for this? - What motiviates this change? - Have you looked to see if anything might for bug compatibilty require applications not to be able to drop supplementary groups? > The kernel already maintains the list of supplementary group IDs in > sorted order, and setgroups already needs to sort the new list, so this > just requires a linear comparison of the two sorted lists. > > This moves the CAP_SETGID test from setgroups into set_current_groups. > > Tested via the following test program: > > #include <err.h> > #include <grp.h> > #include <stdio.h> > #include <sys/types.h> > #include <unistd.h> > > void run_id(void) > { > pid_t p = fork(); > switch (p) { > case -1: > err(1, "fork"); > case 0: > execl("/usr/bin/id", "id", NULL); > err(1, "exec"); > default: > if (waitpid(p, NULL, 0) < 0) > err(1, "waitpid"); > } > } > > int main(void) > { > gid_t list1[] = { 1, 2, 3, 4, 5 }; > gid_t list2[] = { 2, 3, 4 }; > run_id(); > if (setgroups(5, list1) < 0) > err(1, "setgroups 1"); > run_id(); > if (setresgid(1, 1, 1) < 0) > err(1, "setresgid"); > if (setresuid(1, 1, 1) < 0) > err(1, "setresuid"); > run_id(); > if (setgroups(3, list2) < 0) > err(1, "setgroups 2"); > run_id(); > if (setgroups(5, list1) < 0) > err(1, "setgroups 3"); > run_id(); > > return 0; > } > > Without this patch, the test program gets EPERM from the second > setgroups call, after dropping root privileges. With this patch, the > test program successfully drops groups 1 and 5, but then gets EPERM from > the third setgroups call, since that call attempts to add groups the > process does not currently have. > > Signed-off-by: Josh Triplett <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org> > --- > kernel/groups.c | 33 ++++++++++++++++++++++++++++++--- > kernel/uid16.c | 2 -- > 2 files changed, 30 insertions(+), 5 deletions(-) > > diff --git a/kernel/groups.c b/kernel/groups.c > index f0667e7..fe7367d 100644 > --- a/kernel/groups.c > +++ b/kernel/groups.c > @@ -153,6 +153,29 @@ int groups_search(const struct group_info *group_info, kgid_t grp) > return 0; > } > > +/* Compare two sorted group lists; return true if the first is a subset of the > + * second. > + */ > +static bool is_subset(const struct group_info *g1, const struct group_info *g2) > +{ > + unsigned int i, j; > + > + for (i = 0, j = 0; i < g1->ngroups; i++) { > + kgid_t gid1 = GROUP_AT(g1, i); > + kgid_t gid2; > + for (; j < g2->ngroups; j++) { > + gid2 = GROUP_AT(g2, j); > + if (gid_lte(gid1, gid2)) > + break; > + } > + if (j >= g2->ngroups || !gid_eq(gid1, gid2)) > + return false; > + j++; > + } > + > + return true; > +} > + > /** > * set_groups_sorted - Change a group subscription in a set of credentials > * @new: The newly prepared set of credentials to alter > @@ -189,11 +212,17 @@ int set_current_groups(struct group_info *group_info) > { > struct cred *new; > > + groups_sort(group_info); > new = prepare_creds(); > if (!new) > return -ENOMEM; > + if (!ns_capable(current_user_ns(), CAP_SETGID) > + && !is_subset(group_info, new->group_info)) { > + abort_creds(new); > + return -EPERM; > + } > > - set_groups(new, group_info); > + set_groups_sorted(new, group_info); > return commit_creds(new); > } > > @@ -233,8 +262,6 @@ SYSCALL_DEFINE2(setgroups, int, gidsetsize, gid_t __user *, grouplist) > struct group_info *group_info; > int retval; > > - if (!ns_capable(current_user_ns(), CAP_SETGID)) > - return -EPERM; > if ((unsigned)gidsetsize > NGROUPS_MAX) > return -EINVAL; > > diff --git a/kernel/uid16.c b/kernel/uid16.c > index 602e5bb..b27e167 100644 > --- a/kernel/uid16.c > +++ b/kernel/uid16.c > @@ -176,8 +176,6 @@ SYSCALL_DEFINE2(setgroups16, int, gidsetsize, old_gid_t __user *, grouplist) > struct group_info *group_info; > int retval; > > - if (!ns_capable(current_user_ns(), CAP_SETGID)) > - return -EPERM; > if ((unsigned)gidsetsize > NGROUPS_MAX) > return -EINVAL; ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-15 19:29 ` Josh Triplett 0 siblings, 0 replies; 73+ messages in thread From: Josh Triplett @ 2014-11-15 19:29 UTC (permalink / raw) To: Eric W. Biederman Cc: Andrew Morton, Kees Cook, mtk.manpages, linux-api, linux-man, linux-kernel On Sat, Nov 15, 2014 at 09:37:27AM -0600, Eric W. Biederman wrote: > Josh Triplett <josh@joshtriplett.org> writes: > > > Currently, unprivileged processes (without CAP_SETGID) cannot call > > setgroups at all. In particular, processes with a set of supplementary > > groups cannot further drop permissions without obtaining elevated > > permissions first. > > > > Allow unprivileged processes to call setgroups with a subset of their > > current groups; only require CAP_SETGID to add a group the process does > > not currently have. > > A couple of questions. > - Is there precedence in other unix flavors for this? I found a few references to now-nonexistent pages at MIT about a system with this property, but other than that no. I've also found more than a few references to people wanting this functionality. > - What motiviates this change? I have a series of patches planned to add more ways to drop elevated privileges without requiring a transition through root to do so. That would improve the ability for unprivileged users to run programs sandboxed with even *less* privileges. (Among other things, that would also allow programs running with no_new_privs to further *reduce* their privileges, which they can't currently do in this case.) > - Have you looked to see if anything might for bug compatibilty > require applications not to be able to drop supplementary groups? I haven't found any such case; that doesn't mean such a case does not exist. Feedback welcome. The only case I can think of (and I don't know of any examples of such a system): some kind of quota system that limits the members of a group to a certain amount of storage, but places no such limit on non-members. However, the idea of *holding* a credential (a supplementary group ID) giving *less* privileges, and *dropping* a credential giving *more* privileges, would completely invert normal security models. (The sane way to design such a system would be to have a privileged group for "users who can exceed the quota".) If it turns out that a real case exists that people care about, I could easily make this configurable, either at compile time or via a sysctl. - Josh Triplett ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-15 19:29 ` Josh Triplett 0 siblings, 0 replies; 73+ messages in thread From: Josh Triplett @ 2014-11-15 19:29 UTC (permalink / raw) To: Eric W. Biederman Cc: Andrew Morton, Kees Cook, mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w, linux-api-u79uwXL29TY76Z2rM5mHXA, linux-man-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA On Sat, Nov 15, 2014 at 09:37:27AM -0600, Eric W. Biederman wrote: > Josh Triplett <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org> writes: > > > Currently, unprivileged processes (without CAP_SETGID) cannot call > > setgroups at all. In particular, processes with a set of supplementary > > groups cannot further drop permissions without obtaining elevated > > permissions first. > > > > Allow unprivileged processes to call setgroups with a subset of their > > current groups; only require CAP_SETGID to add a group the process does > > not currently have. > > A couple of questions. > - Is there precedence in other unix flavors for this? I found a few references to now-nonexistent pages at MIT about a system with this property, but other than that no. I've also found more than a few references to people wanting this functionality. > - What motiviates this change? I have a series of patches planned to add more ways to drop elevated privileges without requiring a transition through root to do so. That would improve the ability for unprivileged users to run programs sandboxed with even *less* privileges. (Among other things, that would also allow programs running with no_new_privs to further *reduce* their privileges, which they can't currently do in this case.) > - Have you looked to see if anything might for bug compatibilty > require applications not to be able to drop supplementary groups? I haven't found any such case; that doesn't mean such a case does not exist. Feedback welcome. The only case I can think of (and I don't know of any examples of such a system): some kind of quota system that limits the members of a group to a certain amount of storage, but places no such limit on non-members. However, the idea of *holding* a credential (a supplementary group ID) giving *less* privileges, and *dropping* a credential giving *more* privileges, would completely invert normal security models. (The sane way to design such a system would be to have a privileged group for "users who can exceed the quota".) If it turns out that a real case exists that people care about, I could easily make this configurable, either at compile time or via a sysctl. - Josh Triplett ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups 2014-11-15 19:29 ` Josh Triplett @ 2014-11-15 20:06 ` Andy Lutomirski -1 siblings, 0 replies; 73+ messages in thread From: Andy Lutomirski @ 2014-11-15 20:06 UTC (permalink / raw) To: Josh Triplett Cc: Eric W. Biederman, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel On Sat, Nov 15, 2014 at 11:29 AM, Josh Triplett <josh@joshtriplett.org> wrote: > On Sat, Nov 15, 2014 at 09:37:27AM -0600, Eric W. Biederman wrote: >> Josh Triplett <josh@joshtriplett.org> writes: >> >> > Currently, unprivileged processes (without CAP_SETGID) cannot call >> > setgroups at all. In particular, processes with a set of supplementary >> > groups cannot further drop permissions without obtaining elevated >> > permissions first. >> > >> > Allow unprivileged processes to call setgroups with a subset of their >> > current groups; only require CAP_SETGID to add a group the process does >> > not currently have. >> >> A couple of questions. >> - Is there precedence in other unix flavors for this? > > I found a few references to now-nonexistent pages at MIT about a system > with this property, but other than that no. > > I've also found more than a few references to people wanting this > functionality. > >> - What motiviates this change? > > I have a series of patches planned to add more ways to drop elevated > privileges without requiring a transition through root to do so. That > would improve the ability for unprivileged users to run programs > sandboxed with even *less* privileges. (Among other things, that would > also allow programs running with no_new_privs to further *reduce* their > privileges, which they can't currently do in this case.) > >> - Have you looked to see if anything might for bug compatibilty >> require applications not to be able to drop supplementary groups? > > I haven't found any such case; that doesn't mean such a case does not > exist. Feedback welcome. > > The only case I can think of (and I don't know of any examples of such a > system): some kind of quota system that limits the members of a group to > a certain amount of storage, but places no such limit on non-members. > > However, the idea of *holding* a credential (a supplementary group ID) > giving *less* privileges, and *dropping* a credential giving *more* > privileges, would completely invert normal security models. (The sane > way to design such a system would be to have a privileged group for > "users who can exceed the quota".) Agreed. And, if you want to bypass quotas by dropping a supplementary group, you already can by unsharing your user namespace. However, sudoers seems to allow negative group matches. So maybe allowing this only with no_new_privs already set would make sense. --Andy ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-15 20:06 ` Andy Lutomirski 0 siblings, 0 replies; 73+ messages in thread From: Andy Lutomirski @ 2014-11-15 20:06 UTC (permalink / raw) To: Josh Triplett Cc: Eric W. Biederman, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel-u79uwXL29TY76Z2rM5mHXA On Sat, Nov 15, 2014 at 11:29 AM, Josh Triplett <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org> wrote: > On Sat, Nov 15, 2014 at 09:37:27AM -0600, Eric W. Biederman wrote: >> Josh Triplett <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org> writes: >> >> > Currently, unprivileged processes (without CAP_SETGID) cannot call >> > setgroups at all. In particular, processes with a set of supplementary >> > groups cannot further drop permissions without obtaining elevated >> > permissions first. >> > >> > Allow unprivileged processes to call setgroups with a subset of their >> > current groups; only require CAP_SETGID to add a group the process does >> > not currently have. >> >> A couple of questions. >> - Is there precedence in other unix flavors for this? > > I found a few references to now-nonexistent pages at MIT about a system > with this property, but other than that no. > > I've also found more than a few references to people wanting this > functionality. > >> - What motiviates this change? > > I have a series of patches planned to add more ways to drop elevated > privileges without requiring a transition through root to do so. That > would improve the ability for unprivileged users to run programs > sandboxed with even *less* privileges. (Among other things, that would > also allow programs running with no_new_privs to further *reduce* their > privileges, which they can't currently do in this case.) > >> - Have you looked to see if anything might for bug compatibilty >> require applications not to be able to drop supplementary groups? > > I haven't found any such case; that doesn't mean such a case does not > exist. Feedback welcome. > > The only case I can think of (and I don't know of any examples of such a > system): some kind of quota system that limits the members of a group to > a certain amount of storage, but places no such limit on non-members. > > However, the idea of *holding* a credential (a supplementary group ID) > giving *less* privileges, and *dropping* a credential giving *more* > privileges, would completely invert normal security models. (The sane > way to design such a system would be to have a privileged group for > "users who can exceed the quota".) Agreed. And, if you want to bypass quotas by dropping a supplementary group, you already can by unsharing your user namespace. However, sudoers seems to allow negative group matches. So maybe allowing this only with no_new_privs already set would make sense. --Andy ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-15 20:20 ` Josh Triplett 0 siblings, 0 replies; 73+ messages in thread From: Josh Triplett @ 2014-11-15 20:20 UTC (permalink / raw) To: Andy Lutomirski Cc: Eric W. Biederman, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel On Sat, Nov 15, 2014 at 12:06:20PM -0800, Andy Lutomirski wrote: > On Sat, Nov 15, 2014 at 11:29 AM, Josh Triplett <josh@joshtriplett.org> wrote: > > On Sat, Nov 15, 2014 at 09:37:27AM -0600, Eric W. Biederman wrote: > >> Josh Triplett <josh@joshtriplett.org> writes: > >> > >> > Currently, unprivileged processes (without CAP_SETGID) cannot call > >> > setgroups at all. In particular, processes with a set of supplementary > >> > groups cannot further drop permissions without obtaining elevated > >> > permissions first. > >> > > >> > Allow unprivileged processes to call setgroups with a subset of their > >> > current groups; only require CAP_SETGID to add a group the process does > >> > not currently have. > >> > >> A couple of questions. > >> - Is there precedence in other unix flavors for this? > > > > I found a few references to now-nonexistent pages at MIT about a system > > with this property, but other than that no. > > > > I've also found more than a few references to people wanting this > > functionality. > > > >> - What motiviates this change? > > > > I have a series of patches planned to add more ways to drop elevated > > privileges without requiring a transition through root to do so. That > > would improve the ability for unprivileged users to run programs > > sandboxed with even *less* privileges. (Among other things, that would > > also allow programs running with no_new_privs to further *reduce* their > > privileges, which they can't currently do in this case.) > > > >> - Have you looked to see if anything might for bug compatibilty > >> require applications not to be able to drop supplementary groups? > > > > I haven't found any such case; that doesn't mean such a case does not > > exist. Feedback welcome. > > > > The only case I can think of (and I don't know of any examples of such a > > system): some kind of quota system that limits the members of a group to > > a certain amount of storage, but places no such limit on non-members. > > > > However, the idea of *holding* a credential (a supplementary group ID) > > giving *less* privileges, and *dropping* a credential giving *more* > > privileges, would completely invert normal security models. (The sane > > way to design such a system would be to have a privileged group for > > "users who can exceed the quota".) > > Agreed. And, if you want to bypass quotas by dropping a supplementary > group, you already can by unsharing your user namespace. Good point! Given that a process can run with a new user namespace and no other namespaces, and then drop all its other privileges that way, the ability to drop privileges without using a user namespace seems completely harmless, with one exception that you noted: > However, sudoers seems to allow negative group matches. So maybe > allowing this only with no_new_privs already set would make sense. Sigh, bad sudo. Sure, restricting this to no_new_privs only seems fine. I'll do that in v2, and document that in the manpage. - Josh Triplett ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-15 20:20 ` Josh Triplett 0 siblings, 0 replies; 73+ messages in thread From: Josh Triplett @ 2014-11-15 20:20 UTC (permalink / raw) To: Andy Lutomirski Cc: Eric W. Biederman, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel-u79uwXL29TY76Z2rM5mHXA On Sat, Nov 15, 2014 at 12:06:20PM -0800, Andy Lutomirski wrote: > On Sat, Nov 15, 2014 at 11:29 AM, Josh Triplett <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org> wrote: > > On Sat, Nov 15, 2014 at 09:37:27AM -0600, Eric W. Biederman wrote: > >> Josh Triplett <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org> writes: > >> > >> > Currently, unprivileged processes (without CAP_SETGID) cannot call > >> > setgroups at all. In particular, processes with a set of supplementary > >> > groups cannot further drop permissions without obtaining elevated > >> > permissions first. > >> > > >> > Allow unprivileged processes to call setgroups with a subset of their > >> > current groups; only require CAP_SETGID to add a group the process does > >> > not currently have. > >> > >> A couple of questions. > >> - Is there precedence in other unix flavors for this? > > > > I found a few references to now-nonexistent pages at MIT about a system > > with this property, but other than that no. > > > > I've also found more than a few references to people wanting this > > functionality. > > > >> - What motiviates this change? > > > > I have a series of patches planned to add more ways to drop elevated > > privileges without requiring a transition through root to do so. That > > would improve the ability for unprivileged users to run programs > > sandboxed with even *less* privileges. (Among other things, that would > > also allow programs running with no_new_privs to further *reduce* their > > privileges, which they can't currently do in this case.) > > > >> - Have you looked to see if anything might for bug compatibilty > >> require applications not to be able to drop supplementary groups? > > > > I haven't found any such case; that doesn't mean such a case does not > > exist. Feedback welcome. > > > > The only case I can think of (and I don't know of any examples of such a > > system): some kind of quota system that limits the members of a group to > > a certain amount of storage, but places no such limit on non-members. > > > > However, the idea of *holding* a credential (a supplementary group ID) > > giving *less* privileges, and *dropping* a credential giving *more* > > privileges, would completely invert normal security models. (The sane > > way to design such a system would be to have a privileged group for > > "users who can exceed the quota".) > > Agreed. And, if you want to bypass quotas by dropping a supplementary > group, you already can by unsharing your user namespace. Good point! Given that a process can run with a new user namespace and no other namespaces, and then drop all its other privileges that way, the ability to drop privileges without using a user namespace seems completely harmless, with one exception that you noted: > However, sudoers seems to allow negative group matches. So maybe > allowing this only with no_new_privs already set would make sense. Sigh, bad sudo. Sure, restricting this to no_new_privs only seems fine. I'll do that in v2, and document that in the manpage. - Josh Triplett ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups 2014-11-15 20:20 ` Josh Triplett @ 2014-11-16 2:05 ` Theodore Ts'o -1 siblings, 0 replies; 73+ messages in thread From: Theodore Ts'o @ 2014-11-16 2:05 UTC (permalink / raw) To: Josh Triplett Cc: Andy Lutomirski, Eric W. Biederman, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel On Sat, Nov 15, 2014 at 12:20:42PM -0800, Josh Triplett wrote: > > However, sudoers seems to allow negative group matches. So maybe > > allowing this only with no_new_privs already set would make sense. > > Sigh, bad sudo. Sure, restricting this to no_new_privs only seems fine. > I'll do that in v2, and document that in the manpage. I've also seen use cases (generally back in the bad old days of big timesharing VAX 750's :-) where the system admin might assign someone to the "games-abusers" group, and then set /usr/games to mode 705 root:games-abusers --- presumably because it's easier to add a few people to the deny list rather than having to add all of the EECS department to the games group minus the abusers. So arbitrarily anyone to drop groups from their supplemental group list will result in a change from both existing practice and legacy Unix systems, and it could potentially lead to a security exposure. Cheers, - Ted ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-16 2:05 ` Theodore Ts'o 0 siblings, 0 replies; 73+ messages in thread From: Theodore Ts'o @ 2014-11-16 2:05 UTC (permalink / raw) To: Josh Triplett Cc: Andy Lutomirski, Eric W. Biederman, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel-u79uwXL29TY76Z2rM5mHXA On Sat, Nov 15, 2014 at 12:20:42PM -0800, Josh Triplett wrote: > > However, sudoers seems to allow negative group matches. So maybe > > allowing this only with no_new_privs already set would make sense. > > Sigh, bad sudo. Sure, restricting this to no_new_privs only seems fine. > I'll do that in v2, and document that in the manpage. I've also seen use cases (generally back in the bad old days of big timesharing VAX 750's :-) where the system admin might assign someone to the "games-abusers" group, and then set /usr/games to mode 705 root:games-abusers --- presumably because it's easier to add a few people to the deny list rather than having to add all of the EECS department to the games group minus the abusers. So arbitrarily anyone to drop groups from their supplemental group list will result in a change from both existing practice and legacy Unix systems, and it could potentially lead to a security exposure. Cheers, - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-man" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-16 2:35 ` Josh Triplett 0 siblings, 0 replies; 73+ messages in thread From: Josh Triplett @ 2014-11-16 2:35 UTC (permalink / raw) To: Theodore Ts'o Cc: Andy Lutomirski, Eric W. Biederman, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel On November 15, 2014 6:05:11 PM PST, Theodore Ts'o <tytso@mit.edu> wrote: >On Sat, Nov 15, 2014 at 12:20:42PM -0800, Josh Triplett wrote: >> > However, sudoers seems to allow negative group matches. So maybe >> > allowing this only with no_new_privs already set would make sense. >> >> Sigh, bad sudo. Sure, restricting this to no_new_privs only seems >fine. >> I'll do that in v2, and document that in the manpage. > >I've also seen use cases (generally back in the bad old days of big >timesharing VAX 750's :-) where the system admin might assign someone >to the "games-abusers" group, and then set /usr/games to mode 705 >root:games-abusers --- presumably because it's easier to add a few >people to the deny list rather than having to add all of the EECS >department to the games group minus the abusers. > >So arbitrarily anyone to drop groups from their supplemental group >list will result in a change from both existing practice and legacy >Unix systems, and it could potentially lead to a security exposure. As Andy pointed out, you can already do that with a user namespace, for any case not involving a setuid or setgid (or otherwise privilege-gaining) program. And requiring no_new_privs handles that. Given the combination of those two things, do you still see any problematic cases? - Josh Triplett ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-16 2:35 ` Josh Triplett 0 siblings, 0 replies; 73+ messages in thread From: Josh Triplett @ 2014-11-16 2:35 UTC (permalink / raw) To: Theodore Ts'o Cc: Andy Lutomirski, Eric W. Biederman, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel-u79uwXL29TY76Z2rM5mHXA On November 15, 2014 6:05:11 PM PST, Theodore Ts'o <tytso-3s7WtUTddSA@public.gmane.org> wrote: >On Sat, Nov 15, 2014 at 12:20:42PM -0800, Josh Triplett wrote: >> > However, sudoers seems to allow negative group matches. So maybe >> > allowing this only with no_new_privs already set would make sense. >> >> Sigh, bad sudo. Sure, restricting this to no_new_privs only seems >fine. >> I'll do that in v2, and document that in the manpage. > >I've also seen use cases (generally back in the bad old days of big >timesharing VAX 750's :-) where the system admin might assign someone >to the "games-abusers" group, and then set /usr/games to mode 705 >root:games-abusers --- presumably because it's easier to add a few >people to the deny list rather than having to add all of the EECS >department to the games group minus the abusers. > >So arbitrarily anyone to drop groups from their supplemental group >list will result in a change from both existing practice and legacy >Unix systems, and it could potentially lead to a security exposure. As Andy pointed out, you can already do that with a user namespace, for any case not involving a setuid or setgid (or otherwise privilege-gaining) program. And requiring no_new_privs handles that. Given the combination of those two things, do you still see any problematic cases? - Josh Triplett -- To unsubscribe from this list: send the line "unsubscribe linux-man" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-16 3:08 ` Eric W. Biederman 0 siblings, 0 replies; 73+ messages in thread From: Eric W. Biederman @ 2014-11-16 3:08 UTC (permalink / raw) To: Josh Triplett Cc: Theodore Ts'o, Andy Lutomirski, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel Josh Triplett <josh@joshtriplett.org> writes: > On November 15, 2014 6:05:11 PM PST, Theodore Ts'o <tytso@mit.edu> wrote: >>On Sat, Nov 15, 2014 at 12:20:42PM -0800, Josh Triplett wrote: >>> > However, sudoers seems to allow negative group matches. So maybe >>> > allowing this only with no_new_privs already set would make sense. >>> >>> Sigh, bad sudo. Sure, restricting this to no_new_privs only seems >>fine. >>> I'll do that in v2, and document that in the manpage. >> >>I've also seen use cases (generally back in the bad old days of big >>timesharing VAX 750's :-) where the system admin might assign someone >>to the "games-abusers" group, and then set /usr/games to mode 705 >>root:games-abusers --- presumably because it's easier to add a few >>people to the deny list rather than having to add all of the EECS >>department to the games group minus the abusers. >> >>So arbitrarily anyone to drop groups from their supplemental group >>list will result in a change from both existing practice and legacy >>Unix systems, and it could potentially lead to a security exposure. > > As Andy pointed out, you can already do that with a user namespace, That may be a bug with the user namespace permission check. Perhaps we shouldn't allow dropping groups that aren't mapped in the user namespace. To add to the discussion there are also sg and newgroup, though I don't think they touch supplementary groups. > for any case not involving a setuid or setgid (or otherwise > privilege-gaining) program. And requiring no_new_privs handles that. Frankly adding a no_new_privs check to allow something/anything scares me. That converts no_new_privs from something simple and easy to understand to something much more complicated. > Given the combination of those two things, do you still see any > problematic cases? Josh I think it is time to stop and make certain you understand how unix groups work in the large. It seems to me that either supplementary groups can be dropped or supplementary groups can not be dropped. If they can be dropped we shouldn't need complicated checks to allow them to be dropped in some circumstances but not others. Unix groups are an old interface and they have been used in a lot of ways over the years. We need to be careful any change we make is good and truly backwards compatible and that we do our darndest not to introduce security holes. Eric ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-16 3:08 ` Eric W. Biederman 0 siblings, 0 replies; 73+ messages in thread From: Eric W. Biederman @ 2014-11-16 3:08 UTC (permalink / raw) To: Josh Triplett Cc: Theodore Ts'o, Andy Lutomirski, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel@vger.kernel.org Josh Triplett <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org> writes: > On November 15, 2014 6:05:11 PM PST, Theodore Ts'o <tytso-3s7WtUTddSA@public.gmane.org> wrote: >>On Sat, Nov 15, 2014 at 12:20:42PM -0800, Josh Triplett wrote: >>> > However, sudoers seems to allow negative group matches. So maybe >>> > allowing this only with no_new_privs already set would make sense. >>> >>> Sigh, bad sudo. Sure, restricting this to no_new_privs only seems >>fine. >>> I'll do that in v2, and document that in the manpage. >> >>I've also seen use cases (generally back in the bad old days of big >>timesharing VAX 750's :-) where the system admin might assign someone >>to the "games-abusers" group, and then set /usr/games to mode 705 >>root:games-abusers --- presumably because it's easier to add a few >>people to the deny list rather than having to add all of the EECS >>department to the games group minus the abusers. >> >>So arbitrarily anyone to drop groups from their supplemental group >>list will result in a change from both existing practice and legacy >>Unix systems, and it could potentially lead to a security exposure. > > As Andy pointed out, you can already do that with a user namespace, That may be a bug with the user namespace permission check. Perhaps we shouldn't allow dropping groups that aren't mapped in the user namespace. To add to the discussion there are also sg and newgroup, though I don't think they touch supplementary groups. > for any case not involving a setuid or setgid (or otherwise > privilege-gaining) program. And requiring no_new_privs handles that. Frankly adding a no_new_privs check to allow something/anything scares me. That converts no_new_privs from something simple and easy to understand to something much more complicated. > Given the combination of those two things, do you still see any > problematic cases? Josh I think it is time to stop and make certain you understand how unix groups work in the large. It seems to me that either supplementary groups can be dropped or supplementary groups can not be dropped. If they can be dropped we shouldn't need complicated checks to allow them to be dropped in some circumstances but not others. Unix groups are an old interface and they have been used in a lot of ways over the years. We need to be careful any change we make is good and truly backwards compatible and that we do our darndest not to introduce security holes. Eric ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-16 5:07 ` Josh Triplett 0 siblings, 0 replies; 73+ messages in thread From: Josh Triplett @ 2014-11-16 5:07 UTC (permalink / raw) To: Eric W. Biederman Cc: Theodore Ts'o, Andy Lutomirski, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel On Sat, Nov 15, 2014 at 09:08:07PM -0600, Eric W. Biederman wrote: > Josh Triplett <josh@joshtriplett.org> writes: > > On November 15, 2014 6:05:11 PM PST, Theodore Ts'o <tytso@mit.edu> wrote: > >>On Sat, Nov 15, 2014 at 12:20:42PM -0800, Josh Triplett wrote: > >>> > However, sudoers seems to allow negative group matches. So maybe > >>> > allowing this only with no_new_privs already set would make sense. > >>> > >>> Sigh, bad sudo. Sure, restricting this to no_new_privs only seems > >>fine. > >>> I'll do that in v2, and document that in the manpage. > >> > >>I've also seen use cases (generally back in the bad old days of big > >>timesharing VAX 750's :-) where the system admin might assign someone > >>to the "games-abusers" group, and then set /usr/games to mode 705 > >>root:games-abusers --- presumably because it's easier to add a few > >>people to the deny list rather than having to add all of the EECS > >>department to the games group minus the abusers. > >> > >>So arbitrarily anyone to drop groups from their supplemental group > >>list will result in a change from both existing practice and legacy > >>Unix systems, and it could potentially lead to a security exposure. > > > > As Andy pointed out, you can already do that with a user namespace, > > That may be a bug with the user namespace permission check. Perhaps we > shouldn't allow dropping groups that aren't mapped in the user > namespace. Changing that would break containers for users on many common Linux distributions, which put users in various supplementary groups by default. I do actually have another patch planned, to allow users to set up gid maps for groups they currently have permission for, not just for their primary group. But even with that change, breaking the ability for container-root to use setgroups to drop all its supplementary groups seems very wrong, and seems highly likely to break real-world software running in containers. > To add to the discussion there are also sg and newgroup, though I don't > think they touch supplementary groups. Both sg and newgrp do prove a different point, though: an unprivileged user should be able to change their gid to one of their supplementary groups. That's another patch I planned to submit after this one: allow setgid/setresgid/etc to a supplementary group ID. > > for any case not involving a setuid or setgid (or otherwise > > privilege-gaining) program. And requiring no_new_privs handles that. > > Frankly adding a no_new_privs check to allow something/anything scares > me. That converts no_new_privs from something simple and easy to > understand to something much more complicated. no_new_privs already exists in large part to support such use cases, such as setting seccomp filters, which would be dangerous for privilege-escalating programs to inherit. > > Given the combination of those two things, do you still see any > > problematic cases? > > Josh I think it is time to stop and make certain you understand how > unix groups work in the large. > > It seems to me that either supplementary groups can be dropped or > supplementary groups can not be dropped. If they can be dropped we > shouldn't need complicated checks to allow them to be dropped in some > circumstances but not others. > > Unix groups are an old interface and they have been used in a lot of > ways over the years. We need to be careful any change we make is good > and truly backwards compatible and that we do our darndest not to > introduce security holes. Seeking the decades-old precedents and corner cases that might contradict the most obvious semantics is a large part of what I'm looking for out of this thread. However, I don't necessarily think that we must permit unprivileged setgroups in *all* cases if we permit it in *any*; it may well make sense to reduce the surface area of the change by limiting the circumstances in which you can do this. Whether it does end up making sense to do so depends on the outcome of this discussion. I added the no_new_privs check at Andy's suggestion, since the use case I had in mind will work just fine with that restriction. I'm also open to just allowing setgroups in all cases, if that semantic seems preferable, or to adding some separate check specific to this case if that really seems necessary. - Josh Triplett ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-16 5:07 ` Josh Triplett 0 siblings, 0 replies; 73+ messages in thread From: Josh Triplett @ 2014-11-16 5:07 UTC (permalink / raw) To: Eric W. Biederman Cc: Theodore Ts'o, Andy Lutomirski, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel-u79uwXL29TY76Z2rM5mHXA On Sat, Nov 15, 2014 at 09:08:07PM -0600, Eric W. Biederman wrote: > Josh Triplett <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org> writes: > > On November 15, 2014 6:05:11 PM PST, Theodore Ts'o <tytso-3s7WtUTddSA@public.gmane.org> wrote: > >>On Sat, Nov 15, 2014 at 12:20:42PM -0800, Josh Triplett wrote: > >>> > However, sudoers seems to allow negative group matches. So maybe > >>> > allowing this only with no_new_privs already set would make sense. > >>> > >>> Sigh, bad sudo. Sure, restricting this to no_new_privs only seems > >>fine. > >>> I'll do that in v2, and document that in the manpage. > >> > >>I've also seen use cases (generally back in the bad old days of big > >>timesharing VAX 750's :-) where the system admin might assign someone > >>to the "games-abusers" group, and then set /usr/games to mode 705 > >>root:games-abusers --- presumably because it's easier to add a few > >>people to the deny list rather than having to add all of the EECS > >>department to the games group minus the abusers. > >> > >>So arbitrarily anyone to drop groups from their supplemental group > >>list will result in a change from both existing practice and legacy > >>Unix systems, and it could potentially lead to a security exposure. > > > > As Andy pointed out, you can already do that with a user namespace, > > That may be a bug with the user namespace permission check. Perhaps we > shouldn't allow dropping groups that aren't mapped in the user > namespace. Changing that would break containers for users on many common Linux distributions, which put users in various supplementary groups by default. I do actually have another patch planned, to allow users to set up gid maps for groups they currently have permission for, not just for their primary group. But even with that change, breaking the ability for container-root to use setgroups to drop all its supplementary groups seems very wrong, and seems highly likely to break real-world software running in containers. > To add to the discussion there are also sg and newgroup, though I don't > think they touch supplementary groups. Both sg and newgrp do prove a different point, though: an unprivileged user should be able to change their gid to one of their supplementary groups. That's another patch I planned to submit after this one: allow setgid/setresgid/etc to a supplementary group ID. > > for any case not involving a setuid or setgid (or otherwise > > privilege-gaining) program. And requiring no_new_privs handles that. > > Frankly adding a no_new_privs check to allow something/anything scares > me. That converts no_new_privs from something simple and easy to > understand to something much more complicated. no_new_privs already exists in large part to support such use cases, such as setting seccomp filters, which would be dangerous for privilege-escalating programs to inherit. > > Given the combination of those two things, do you still see any > > problematic cases? > > Josh I think it is time to stop and make certain you understand how > unix groups work in the large. > > It seems to me that either supplementary groups can be dropped or > supplementary groups can not be dropped. If they can be dropped we > shouldn't need complicated checks to allow them to be dropped in some > circumstances but not others. > > Unix groups are an old interface and they have been used in a lot of > ways over the years. We need to be careful any change we make is good > and truly backwards compatible and that we do our darndest not to > introduce security holes. Seeking the decades-old precedents and corner cases that might contradict the most obvious semantics is a large part of what I'm looking for out of this thread. However, I don't necessarily think that we must permit unprivileged setgroups in *all* cases if we permit it in *any*; it may well make sense to reduce the surface area of the change by limiting the circumstances in which you can do this. Whether it does end up making sense to do so depends on the outcome of this discussion. I added the no_new_privs check at Andy's suggestion, since the use case I had in mind will work just fine with that restriction. I'm also open to just allowing setgroups in all cases, if that semantic seems preferable, or to adding some separate check specific to this case if that really seems necessary. - Josh Triplett ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-16 13:32 ` Theodore Ts'o 0 siblings, 0 replies; 73+ messages in thread From: Theodore Ts'o @ 2014-11-16 13:32 UTC (permalink / raw) To: Eric W. Biederman Cc: Josh Triplett, Andy Lutomirski, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel On Sat, Nov 15, 2014 at 09:08:07PM -0600, Eric W. Biederman wrote: > > That may be a bug with the user namespace permission check. Perhaps we > shouldn't allow dropping groups that aren't mapped in the user > namespace. I'm not saying that we can't change the behavior of whether or not a user can drop a group permission. I'm just saying that we need to do so consciously. The setgroups()/getgroups() ABI isn't part of POSIX/SuSv3 so we wouldn't be breaking POSIX compatibility, for those people who care about that. The bigger deal is that it's very different from how BSD 4.x has handled things, which means there is two decades of history that we're looking at here. And there are times when taking away permissions in an expected fashion can cause security problems. (As a silly example; some architect at Digital wrote a spec that said that setuid must return EINVAL for values greater than 32k --- back in the days when uid's were a signed short. The junior programmer who implemented this for Ultrix made the check for 32,000 decimal. Guess what happened when /bin/login got a failure with setuid when it wasn't expecting one --- since root could never get an error with that system call, right? And MIT Project Athena started ran out of lower numbered uid's and froshlings started getting assigned uid's > 32,000....) In this particular case, the change is probably a little less likely to cause serious problems, although the fact that sudo does allow negative group assignments is an example of another potential breakage. OTOH, I'm aware of how this could cause major problems to the concept of allowing an untrusted user to set up their own containers to constrain what program with a possibly untrusted provinance might be able to do. I can see times when I might want to run in a container where the user didn't have access to groups that I have access to by default --- including groups such as disk, sudo, lpadmin, etc. If we do want to make such a change, my suggestion is to keep things *very* simple. Let it be a boot-time option whether or not users are allowed to drop group permissions, and let it affect all possible ways that users can drop groups. And we can create a shell script that will search for the obvious ways that a user could get screwed by enabling this, which we can encourage distributions to package up for their end users. And then we document the heck out of the fact that this option exists, and when/if we want to make it the default, so it's perfectly clear and transparent to all what is happening. One of the things that scare me about the addition of the forced capability "setuid" binary was that the feature was just silently slid in, and we didn't do a good job reaching out to the tripwire and rootkit and other such programs out there, such that several years after we enabled capability support, most sysadmins and security scanning programs are still apparently obivious to the this very convenient feature that we gave to rootkit and malware authors. Now we can say that we're just adding new features, and we owe no debt other parts of the ecosystem --- this is the attitude used by upower and other freedesktop.org components when they made incompatible changes made available by new features provided by systemd[1]. [1] http://m.memegen.com/u7o1tk.jpg But as kernel developers, who pride ourselves on not breaking userspace, I think we should try for a higher standard than this. And perhaps this change with allowing groups to be dropped --- which is admittedly a useful thing to be able to allow --- is a good place to start trying to model a better way of doing things. We should try to be responsible about how we add new features, and think of all of the potential downstream consequences to the _all_ of the ecosystem. - Ted P.S. And we really should try reaching out to the security scanners about capabilities, too.... ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-16 13:32 ` Theodore Ts'o 0 siblings, 0 replies; 73+ messages in thread From: Theodore Ts'o @ 2014-11-16 13:32 UTC (permalink / raw) To: Eric W. Biederman Cc: Josh Triplett, Andy Lutomirski, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel-u79uwXL29TY76Z2rM5mHXA On Sat, Nov 15, 2014 at 09:08:07PM -0600, Eric W. Biederman wrote: > > That may be a bug with the user namespace permission check. Perhaps we > shouldn't allow dropping groups that aren't mapped in the user > namespace. I'm not saying that we can't change the behavior of whether or not a user can drop a group permission. I'm just saying that we need to do so consciously. The setgroups()/getgroups() ABI isn't part of POSIX/SuSv3 so we wouldn't be breaking POSIX compatibility, for those people who care about that. The bigger deal is that it's very different from how BSD 4.x has handled things, which means there is two decades of history that we're looking at here. And there are times when taking away permissions in an expected fashion can cause security problems. (As a silly example; some architect at Digital wrote a spec that said that setuid must return EINVAL for values greater than 32k --- back in the days when uid's were a signed short. The junior programmer who implemented this for Ultrix made the check for 32,000 decimal. Guess what happened when /bin/login got a failure with setuid when it wasn't expecting one --- since root could never get an error with that system call, right? And MIT Project Athena started ran out of lower numbered uid's and froshlings started getting assigned uid's > 32,000....) In this particular case, the change is probably a little less likely to cause serious problems, although the fact that sudo does allow negative group assignments is an example of another potential breakage. OTOH, I'm aware of how this could cause major problems to the concept of allowing an untrusted user to set up their own containers to constrain what program with a possibly untrusted provinance might be able to do. I can see times when I might want to run in a container where the user didn't have access to groups that I have access to by default --- including groups such as disk, sudo, lpadmin, etc. If we do want to make such a change, my suggestion is to keep things *very* simple. Let it be a boot-time option whether or not users are allowed to drop group permissions, and let it affect all possible ways that users can drop groups. And we can create a shell script that will search for the obvious ways that a user could get screwed by enabling this, which we can encourage distributions to package up for their end users. And then we document the heck out of the fact that this option exists, and when/if we want to make it the default, so it's perfectly clear and transparent to all what is happening. One of the things that scare me about the addition of the forced capability "setuid" binary was that the feature was just silently slid in, and we didn't do a good job reaching out to the tripwire and rootkit and other such programs out there, such that several years after we enabled capability support, most sysadmins and security scanning programs are still apparently obivious to the this very convenient feature that we gave to rootkit and malware authors. Now we can say that we're just adding new features, and we owe no debt other parts of the ecosystem --- this is the attitude used by upower and other freedesktop.org components when they made incompatible changes made available by new features provided by systemd[1]. [1] http://m.memegen.com/u7o1tk.jpg But as kernel developers, who pride ourselves on not breaking userspace, I think we should try for a higher standard than this. And perhaps this change with allowing groups to be dropped --- which is admittedly a useful thing to be able to allow --- is a good place to start trying to model a better way of doing things. We should try to be responsible about how we add new features, and think of all of the potential downstream consequences to the _all_ of the ecosystem. - Ted P.S. And we really should try reaching out to the security scanners about capabilities, too.... ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-16 15:42 ` Andy Lutomirski 0 siblings, 0 replies; 73+ messages in thread From: Andy Lutomirski @ 2014-11-16 15:42 UTC (permalink / raw) To: Theodore Ts'o, Eric W. Biederman, Josh Triplett, Andy Lutomirski, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel On Sun, Nov 16, 2014 at 5:32 AM, Theodore Ts'o <tytso@mit.edu> wrote: > On Sat, Nov 15, 2014 at 09:08:07PM -0600, Eric W. Biederman wrote: >> >> That may be a bug with the user namespace permission check. Perhaps we >> shouldn't allow dropping groups that aren't mapped in the user >> namespace. > > I'm not saying that we can't change the behavior of whether or not a > user can drop a group permission. I'm just saying that we need to do > so consciously. The setgroups()/getgroups() ABI isn't part of > POSIX/SuSv3 so we wouldn't be breaking POSIX compatibility, for those > people who care about that. It may make sense to reach out to some place like oss-security. FWIW, I think we should ask, at the same time, about: - Dropping supplementary groups. - Switching gid/egid/sgid to a supplementary group. - Denying ptrace of a process with supplementary groups that the tracer doesn't have. Also, I much prefer a sysctl to a boot option. Boot options are nasty to configure in many distributions. --Andy > > The bigger deal is that it's very different from how BSD 4.x has > handled things, which means there is two decades of history that we're > looking at here. And there are times when taking away permissions in > an expected fashion can cause security problems. (As a silly example; > some architect at Digital wrote a spec that said that setuid must > return EINVAL for values greater than 32k --- back in the days when > uid's were a signed short. The junior programmer who implemented this > for Ultrix made the check for 32,000 decimal. Guess what happened > when /bin/login got a failure with setuid when it wasn't expecting one > --- since root could never get an error with that system call, right? > And MIT Project Athena started ran out of lower numbered uid's and > froshlings started getting assigned uid's > 32,000....) > > In this particular case, the change is probably a little less likely > to cause serious problems, although the fact that sudo does allow > negative group assignments is an example of another potential > breakage. > > OTOH, I'm aware of how this could cause major problems to the concept > of allowing an untrusted user to set up their own containers to > constrain what program with a possibly untrusted provinance might be > able to do. I can see times when I might want to run in a container > where the user didn't have access to groups that I have access to by > default --- including groups such as disk, sudo, lpadmin, etc. > > If we do want to make such a change, my suggestion is to keep things > *very* simple. Let it be a boot-time option whether or not users are > allowed to drop group permissions, and let it affect all possible ways > that users can drop groups. And we can create a shell script that > will search for the obvious ways that a user could get screwed by > enabling this, which we can encourage distributions to package up for > their end users. And then we document the heck out of the fact that > this option exists, and when/if we want to make it the default, so > it's perfectly clear and transparent to all what is happening. > > One of the things that scare me about the addition of the forced > capability "setuid" binary was that the feature was just silently slid > in, and we didn't do a good job reaching out to the tripwire and > rootkit and other such programs out there, such that several years > after we enabled capability support, most sysadmins and security > scanning programs are still apparently obivious to the this very > convenient feature that we gave to rootkit and malware authors. Now > we can say that we're just adding new features, and we owe no debt > other parts of the ecosystem --- this is the attitude used by upower > and other freedesktop.org components when they made incompatible > changes made available by new features provided by systemd[1]. > > [1] http://m.memegen.com/u7o1tk.jpg > > But as kernel developers, who pride ourselves on not breaking > userspace, I think we should try for a higher standard than this. And > perhaps this change with allowing groups to be dropped --- which is > admittedly a useful thing to be able to allow --- is a good place to > start trying to model a better way of doing things. We should try to > be responsible about how we add new features, and think of all of the > potential downstream consequences to the _all_ of the ecosystem. > > - Ted > > P.S. And we really should try reaching out to the security scanners > about capabilities, too.... -- Andy Lutomirski AMA Capital Management, LLC ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-16 15:42 ` Andy Lutomirski 0 siblings, 0 replies; 73+ messages in thread From: Andy Lutomirski @ 2014-11-16 15:42 UTC (permalink / raw) To: Theodore Ts'o, Eric W. Biederman, Josh Triplett, Andy Lutomirski, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel-u79uwXL29TY76Z2rM5mHXA On Sun, Nov 16, 2014 at 5:32 AM, Theodore Ts'o <tytso-3s7WtUTddSA@public.gmane.org> wrote: > On Sat, Nov 15, 2014 at 09:08:07PM -0600, Eric W. Biederman wrote: >> >> That may be a bug with the user namespace permission check. Perhaps we >> shouldn't allow dropping groups that aren't mapped in the user >> namespace. > > I'm not saying that we can't change the behavior of whether or not a > user can drop a group permission. I'm just saying that we need to do > so consciously. The setgroups()/getgroups() ABI isn't part of > POSIX/SuSv3 so we wouldn't be breaking POSIX compatibility, for those > people who care about that. It may make sense to reach out to some place like oss-security. FWIW, I think we should ask, at the same time, about: - Dropping supplementary groups. - Switching gid/egid/sgid to a supplementary group. - Denying ptrace of a process with supplementary groups that the tracer doesn't have. Also, I much prefer a sysctl to a boot option. Boot options are nasty to configure in many distributions. --Andy > > The bigger deal is that it's very different from how BSD 4.x has > handled things, which means there is two decades of history that we're > looking at here. And there are times when taking away permissions in > an expected fashion can cause security problems. (As a silly example; > some architect at Digital wrote a spec that said that setuid must > return EINVAL for values greater than 32k --- back in the days when > uid's were a signed short. The junior programmer who implemented this > for Ultrix made the check for 32,000 decimal. Guess what happened > when /bin/login got a failure with setuid when it wasn't expecting one > --- since root could never get an error with that system call, right? > And MIT Project Athena started ran out of lower numbered uid's and > froshlings started getting assigned uid's > 32,000....) > > In this particular case, the change is probably a little less likely > to cause serious problems, although the fact that sudo does allow > negative group assignments is an example of another potential > breakage. > > OTOH, I'm aware of how this could cause major problems to the concept > of allowing an untrusted user to set up their own containers to > constrain what program with a possibly untrusted provinance might be > able to do. I can see times when I might want to run in a container > where the user didn't have access to groups that I have access to by > default --- including groups such as disk, sudo, lpadmin, etc. > > If we do want to make such a change, my suggestion is to keep things > *very* simple. Let it be a boot-time option whether or not users are > allowed to drop group permissions, and let it affect all possible ways > that users can drop groups. And we can create a shell script that > will search for the obvious ways that a user could get screwed by > enabling this, which we can encourage distributions to package up for > their end users. And then we document the heck out of the fact that > this option exists, and when/if we want to make it the default, so > it's perfectly clear and transparent to all what is happening. > > One of the things that scare me about the addition of the forced > capability "setuid" binary was that the feature was just silently slid > in, and we didn't do a good job reaching out to the tripwire and > rootkit and other such programs out there, such that several years > after we enabled capability support, most sysadmins and security > scanning programs are still apparently obivious to the this very > convenient feature that we gave to rootkit and malware authors. Now > we can say that we're just adding new features, and we owe no debt > other parts of the ecosystem --- this is the attitude used by upower > and other freedesktop.org components when they made incompatible > changes made available by new features provided by systemd[1]. > > [1] http://m.memegen.com/u7o1tk.jpg > > But as kernel developers, who pride ourselves on not breaking > userspace, I think we should try for a higher standard than this. And > perhaps this change with allowing groups to be dropped --- which is > admittedly a useful thing to be able to allow --- is a good place to > start trying to model a better way of doing things. We should try to > be responsible about how we add new features, and think of all of the > potential downstream consequences to the _all_ of the ecosystem. > > - Ted > > P.S. And we really should try reaching out to the security scanners > about capabilities, too.... -- Andy Lutomirski AMA Capital Management, LLC ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-16 19:12 ` Josh Triplett 0 siblings, 0 replies; 73+ messages in thread From: Josh Triplett @ 2014-11-16 19:12 UTC (permalink / raw) To: Andy Lutomirski Cc: Theodore Ts'o, Eric W. Biederman, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel On Sun, Nov 16, 2014 at 07:42:30AM -0800, Andy Lutomirski wrote: > On Sun, Nov 16, 2014 at 5:32 AM, Theodore Ts'o <tytso@mit.edu> wrote: > > On Sat, Nov 15, 2014 at 09:08:07PM -0600, Eric W. Biederman wrote: > >> > >> That may be a bug with the user namespace permission check. Perhaps we > >> shouldn't allow dropping groups that aren't mapped in the user > >> namespace. > > > > I'm not saying that we can't change the behavior of whether or not a > > user can drop a group permission. I'm just saying that we need to do > > so consciously. The setgroups()/getgroups() ABI isn't part of > > POSIX/SuSv3 so we wouldn't be breaking POSIX compatibility, for those > > people who care about that. > > It may make sense to reach out to some place like oss-security. > > FWIW, I think we should ask, at the same time, about: > > - Dropping supplementary groups. > - Switching gid/egid/sgid to a supplementary group. > - Denying ptrace of a process with supplementary groups that the > tracer doesn't have. I wonder how crazy it would be to just require either CAP_SYS_PTRACE or cred1 == cred2 (as in, you have *exactly* the same credentials as the target)? > Also, I much prefer a sysctl to a boot option. Boot options are nasty > to configure in many distributions. Agreed. - Josh Triplett ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-16 19:12 ` Josh Triplett 0 siblings, 0 replies; 73+ messages in thread From: Josh Triplett @ 2014-11-16 19:12 UTC (permalink / raw) To: Andy Lutomirski Cc: Theodore Ts'o, Eric W. Biederman, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel-u79uwXL29TY76Z2rM5mHXA On Sun, Nov 16, 2014 at 07:42:30AM -0800, Andy Lutomirski wrote: > On Sun, Nov 16, 2014 at 5:32 AM, Theodore Ts'o <tytso-3s7WtUTddSA@public.gmane.org> wrote: > > On Sat, Nov 15, 2014 at 09:08:07PM -0600, Eric W. Biederman wrote: > >> > >> That may be a bug with the user namespace permission check. Perhaps we > >> shouldn't allow dropping groups that aren't mapped in the user > >> namespace. > > > > I'm not saying that we can't change the behavior of whether or not a > > user can drop a group permission. I'm just saying that we need to do > > so consciously. The setgroups()/getgroups() ABI isn't part of > > POSIX/SuSv3 so we wouldn't be breaking POSIX compatibility, for those > > people who care about that. > > It may make sense to reach out to some place like oss-security. > > FWIW, I think we should ask, at the same time, about: > > - Dropping supplementary groups. > - Switching gid/egid/sgid to a supplementary group. > - Denying ptrace of a process with supplementary groups that the > tracer doesn't have. I wonder how crazy it would be to just require either CAP_SYS_PTRACE or cred1 == cred2 (as in, you have *exactly* the same credentials as the target)? > Also, I much prefer a sysctl to a boot option. Boot options are nasty > to configure in many distributions. Agreed. - Josh Triplett ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-16 19:09 ` Josh Triplett 0 siblings, 0 replies; 73+ messages in thread From: Josh Triplett @ 2014-11-16 19:09 UTC (permalink / raw) To: Theodore Ts'o, Eric W. Biederman, Andy Lutomirski, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel On Sun, Nov 16, 2014 at 08:32:30AM -0500, Theodore Ts'o wrote: > On Sat, Nov 15, 2014 at 09:08:07PM -0600, Eric W. Biederman wrote: > > That may be a bug with the user namespace permission check. Perhaps we > > shouldn't allow dropping groups that aren't mapped in the user > > namespace. > > I'm not saying that we can't change the behavior of whether or not a > user can drop a group permission. I'm just saying that we need to do > so consciously. Agreed. > The setgroups()/getgroups() ABI isn't part of > POSIX/SuSv3 so we wouldn't be breaking POSIX compatibility, for those > people who care about that. POSIX.1-2001 actually specifies getgroups, but not setgroups. In any case, yes, POSIX doesn't say anything about this behavior. > The bigger deal is that it's very different from how BSD 4.x has > handled things, which means there is two decades of history that we're > looking at here. And there are times when taking away permissions in > an expected fashion can cause security problems. (As a silly example; > some architect at Digital wrote a spec that said that setuid must > return EINVAL for values greater than 32k --- back in the days when > uid's were a signed short. The junior programmer who implemented this > for Ultrix made the check for 32,000 decimal. Guess what happened > when /bin/login got a failure with setuid when it wasn't expecting one > --- since root could never get an error with that system call, right? Ignored it and kept going, starting the user's shell as root? I'd guess that a similar story motivated the note in the Linux manpages for setuid, setresuid, and similar, saying "Note: there are cases where setuid() can fail even when the caller is UID 0; it is a grave security error to omit checking for a failure return from setuid().". (Also, these days, glibc marks setuid and similar with the warn_unused_result attribute.) > And MIT Project Athena started ran out of lower numbered uid's and > froshlings started getting assigned uid's > 32,000....) > > In this particular case, the change is probably a little less likely > to cause serious problems, although the fact that sudo does allow > negative group assignments is an example of another potential > breakage. > > OTOH, I'm aware of how this could cause major problems to the concept > of allowing an untrusted user to set up their own containers to > constrain what program with a possibly untrusted provinance might be > able to do. I can see times when I might want to run in a container > where the user didn't have access to groups that I have access to by > default --- including groups such as disk, sudo, lpadmin, etc. > > If we do want to make such a change, my suggestion is to keep things > *very* simple. Let it be a boot-time option whether or not users are > allowed to drop group permissions, and let it affect all possible ways > that users can drop groups. And we can create a shell script that > will search for the obvious ways that a user could get screwed by > enabling this, which we can encourage distributions to package up for > their end users. And then we document the heck out of the fact that > this option exists, and when/if we want to make it the default, so > it's perfectly clear and transparent to all what is happening. An option sounds sensible to me. I think a sysctl makes more sense, though. I'll add one in v4. What did you have in mind about the shell script? Something like: grep -r !% /etc/sudoers /etc/sudoers.d ? - Josh Triplett ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-16 19:09 ` Josh Triplett 0 siblings, 0 replies; 73+ messages in thread From: Josh Triplett @ 2014-11-16 19:09 UTC (permalink / raw) To: Theodore Ts'o, Eric W. Biederman, Andy Lutomirski, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel-u79uwXL29TY76Z2rM5mHXA On Sun, Nov 16, 2014 at 08:32:30AM -0500, Theodore Ts'o wrote: > On Sat, Nov 15, 2014 at 09:08:07PM -0600, Eric W. Biederman wrote: > > That may be a bug with the user namespace permission check. Perhaps we > > shouldn't allow dropping groups that aren't mapped in the user > > namespace. > > I'm not saying that we can't change the behavior of whether or not a > user can drop a group permission. I'm just saying that we need to do > so consciously. Agreed. > The setgroups()/getgroups() ABI isn't part of > POSIX/SuSv3 so we wouldn't be breaking POSIX compatibility, for those > people who care about that. POSIX.1-2001 actually specifies getgroups, but not setgroups. In any case, yes, POSIX doesn't say anything about this behavior. > The bigger deal is that it's very different from how BSD 4.x has > handled things, which means there is two decades of history that we're > looking at here. And there are times when taking away permissions in > an expected fashion can cause security problems. (As a silly example; > some architect at Digital wrote a spec that said that setuid must > return EINVAL for values greater than 32k --- back in the days when > uid's were a signed short. The junior programmer who implemented this > for Ultrix made the check for 32,000 decimal. Guess what happened > when /bin/login got a failure with setuid when it wasn't expecting one > --- since root could never get an error with that system call, right? Ignored it and kept going, starting the user's shell as root? I'd guess that a similar story motivated the note in the Linux manpages for setuid, setresuid, and similar, saying "Note: there are cases where setuid() can fail even when the caller is UID 0; it is a grave security error to omit checking for a failure return from setuid().". (Also, these days, glibc marks setuid and similar with the warn_unused_result attribute.) > And MIT Project Athena started ran out of lower numbered uid's and > froshlings started getting assigned uid's > 32,000....) > > In this particular case, the change is probably a little less likely > to cause serious problems, although the fact that sudo does allow > negative group assignments is an example of another potential > breakage. > > OTOH, I'm aware of how this could cause major problems to the concept > of allowing an untrusted user to set up their own containers to > constrain what program with a possibly untrusted provinance might be > able to do. I can see times when I might want to run in a container > where the user didn't have access to groups that I have access to by > default --- including groups such as disk, sudo, lpadmin, etc. > > If we do want to make such a change, my suggestion is to keep things > *very* simple. Let it be a boot-time option whether or not users are > allowed to drop group permissions, and let it affect all possible ways > that users can drop groups. And we can create a shell script that > will search for the obvious ways that a user could get screwed by > enabling this, which we can encourage distributions to package up for > their end users. And then we document the heck out of the fact that > this option exists, and when/if we want to make it the default, so > it's perfectly clear and transparent to all what is happening. An option sounds sensible to me. I think a sysctl makes more sense, though. I'll add one in v4. What did you have in mind about the shell script? Something like: grep -r !% /etc/sudoers /etc/sudoers.d ? - Josh Triplett ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-16 3:40 ` Theodore Ts'o 0 siblings, 0 replies; 73+ messages in thread From: Theodore Ts'o @ 2014-11-16 3:40 UTC (permalink / raw) To: Josh Triplett Cc: Andy Lutomirski, Eric W. Biederman, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel On Sat, Nov 15, 2014 at 06:35:05PM -0800, Josh Triplett wrote: > >So arbitrarily anyone to drop groups from their supplemental group > >list will result in a change from both existing practice and legacy > >Unix systems, and it could potentially lead to a security exposure. > > As Andy pointed out, you can already do that with a user namespace, > for any case not involving a setuid or setgid (or otherwise > privilege-gaining) program. And requiring no_new_privs handles > that. Well, it's no worse than what we can do already with the user namespace, yes. I'm still worried it's going to come as a surprise for some configurations because it's a change from what was allowed historically. Then again, pretty much all of the tripwire and rootkit scanners won't notice a "setuid" program that uses capabilities instead of the traditional setuid bit, and most sysadmins won't think to check for an executable with a forced capability mask, so this isn't exactly a new problem.... - Ted ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-16 3:40 ` Theodore Ts'o 0 siblings, 0 replies; 73+ messages in thread From: Theodore Ts'o @ 2014-11-16 3:40 UTC (permalink / raw) To: Josh Triplett Cc: Andy Lutomirski, Eric W. Biederman, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel-u79uwXL29TY76Z2rM5mHXA On Sat, Nov 15, 2014 at 06:35:05PM -0800, Josh Triplett wrote: > >So arbitrarily anyone to drop groups from their supplemental group > >list will result in a change from both existing practice and legacy > >Unix systems, and it could potentially lead to a security exposure. > > As Andy pointed out, you can already do that with a user namespace, > for any case not involving a setuid or setgid (or otherwise > privilege-gaining) program. And requiring no_new_privs handles > that. Well, it's no worse than what we can do already with the user namespace, yes. I'm still worried it's going to come as a surprise for some configurations because it's a change from what was allowed historically. Then again, pretty much all of the tripwire and rootkit scanners won't notice a "setuid" program that uses capabilities instead of the traditional setuid bit, and most sysadmins won't think to check for an executable with a forced capability mask, so this isn't exactly a new problem.... - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-man" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-16 4:52 ` Josh Triplett 0 siblings, 0 replies; 73+ messages in thread From: Josh Triplett @ 2014-11-16 4:52 UTC (permalink / raw) To: Theodore Ts'o, Andy Lutomirski, Eric W. Biederman, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel On Sat, Nov 15, 2014 at 10:40:06PM -0500, Theodore Ts'o wrote: > On Sat, Nov 15, 2014 at 06:35:05PM -0800, Josh Triplett wrote: > > >So arbitrarily anyone to drop groups from their supplemental group > > >list will result in a change from both existing practice and legacy > > >Unix systems, and it could potentially lead to a security exposure. > > > > As Andy pointed out, you can already do that with a user namespace, > > for any case not involving a setuid or setgid (or otherwise > > privilege-gaining) program. And requiring no_new_privs handles > > that. > > Well, it's no worse than what we can do already with the user > namespace, yes. I'm still worried it's going to come as a surprise > for some configurations because it's a change from what was allowed > historically. Then again, pretty much all of the tripwire and rootkit > scanners won't notice a "setuid" program that uses capabilities > instead of the traditional setuid bit, and most sysadmins won't think > to check for an executable with a forced capability mask, so this > isn't exactly a new problem.... We certainly have introduced orthogonal APIs in various areas before, such that applications written prior to those APIs may interact interestingly with them; we don't allow *breaking* those applications, or introducing security holes, but the existence of applications designed to block one particular way to do something doesn't *automatically* rule out the possibility of adding another way to do it. It does require some careful thought, though. When we introduced seccomp filters for syscalls, we tied them to no_new_privs to prevent any possible security holes caused by selective syscall denial/filtration. In this case, I'm indifferent about whether unprivileged setgroups works without no_new_privs; if people are comfortable with that, fine, and if people would prefer no_new_privs (or for that matter a sysctl, a compile-time option, or any other means of making the behavior optional), I can do that too. The security model of "having a group gives you less privilege than not having it" seems crazy, but nonetheless I can see a couple of easy ways that we can avoid breaking that pattern, no_new_privs being one of them. I'd like to make sure that nobody sees any other real-world corner case that unprivileged setgroups would break. - Josh Triplett ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-16 4:52 ` Josh Triplett 0 siblings, 0 replies; 73+ messages in thread From: Josh Triplett @ 2014-11-16 4:52 UTC (permalink / raw) To: Theodore Ts'o, Andy Lutomirski, Eric W. Biederman, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel-u79uwXL29TY76Z2rM5mHXA On Sat, Nov 15, 2014 at 10:40:06PM -0500, Theodore Ts'o wrote: > On Sat, Nov 15, 2014 at 06:35:05PM -0800, Josh Triplett wrote: > > >So arbitrarily anyone to drop groups from their supplemental group > > >list will result in a change from both existing practice and legacy > > >Unix systems, and it could potentially lead to a security exposure. > > > > As Andy pointed out, you can already do that with a user namespace, > > for any case not involving a setuid or setgid (or otherwise > > privilege-gaining) program. And requiring no_new_privs handles > > that. > > Well, it's no worse than what we can do already with the user > namespace, yes. I'm still worried it's going to come as a surprise > for some configurations because it's a change from what was allowed > historically. Then again, pretty much all of the tripwire and rootkit > scanners won't notice a "setuid" program that uses capabilities > instead of the traditional setuid bit, and most sysadmins won't think > to check for an executable with a forced capability mask, so this > isn't exactly a new problem.... We certainly have introduced orthogonal APIs in various areas before, such that applications written prior to those APIs may interact interestingly with them; we don't allow *breaking* those applications, or introducing security holes, but the existence of applications designed to block one particular way to do something doesn't *automatically* rule out the possibility of adding another way to do it. It does require some careful thought, though. When we introduced seccomp filters for syscalls, we tied them to no_new_privs to prevent any possible security holes caused by selective syscall denial/filtration. In this case, I'm indifferent about whether unprivileged setgroups works without no_new_privs; if people are comfortable with that, fine, and if people would prefer no_new_privs (or for that matter a sysctl, a compile-time option, or any other means of making the behavior optional), I can do that too. The security model of "having a group gives you less privilege than not having it" seems crazy, but nonetheless I can see a couple of easy ways that we can avoid breaking that pattern, no_new_privs being one of them. I'd like to make sure that nobody sees any other real-world corner case that unprivileged setgroups would break. - Josh Triplett ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups 2014-11-16 4:52 ` Josh Triplett @ 2014-11-17 11:37 ` One Thousand Gnomes -1 siblings, 0 replies; 73+ messages in thread From: One Thousand Gnomes @ 2014-11-17 11:37 UTC (permalink / raw) To: Josh Triplett Cc: Theodore Ts'o, Andy Lutomirski, Eric W. Biederman, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel > optional), I can do that too. The security model of "having a group > gives you less privilege than not having it" seems crazy, but > nonetheless I can see a couple of easy ways that we can avoid breaking It's an old pattern of use that makes complete sense in a traditional Unix permission world because it's the only way to do "exclude {list}" nicely. Our default IMHO shouldn't break this. > that pattern, no_new_privs being one of them. I'd like to make sure > that nobody sees any other real-world corner case that unprivileged > setgroups would break. Barring the usual risk of people doing improper error checking I don't see one immediately. For containers I think it actually makes sense that the sysctl can be applied per container anyway. Alan ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-17 11:37 ` One Thousand Gnomes 0 siblings, 0 replies; 73+ messages in thread From: One Thousand Gnomes @ 2014-11-17 11:37 UTC (permalink / raw) To: Josh Triplett Cc: Theodore Ts'o, Andy Lutomirski, Eric W. Biederman, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel-u79uwXL29TY76Z2rM5mHXA > optional), I can do that too. The security model of "having a group > gives you less privilege than not having it" seems crazy, but > nonetheless I can see a couple of easy ways that we can avoid breaking It's an old pattern of use that makes complete sense in a traditional Unix permission world because it's the only way to do "exclude {list}" nicely. Our default IMHO shouldn't break this. > that pattern, no_new_privs being one of them. I'd like to make sure > that nobody sees any other real-world corner case that unprivileged > setgroups would break. Barring the usual risk of people doing improper error checking I don't see one immediately. For containers I think it actually makes sense that the sysctl can be applied per container anyway. Alan -- To unsubscribe from this list: send the line "unsubscribe linux-man" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-17 18:07 ` Andy Lutomirski 0 siblings, 0 replies; 73+ messages in thread From: Andy Lutomirski @ 2014-11-17 18:07 UTC (permalink / raw) To: One Thousand Gnomes Cc: linux-man, Ted Ts'o, Michael Kerrisk-manpages, Josh Triplett, linux-kernel, Andrew Morton, Eric W. Biederman, Linux API, Kees Cook On Nov 17, 2014 3:37 AM, "One Thousand Gnomes" <gnomes@lxorguk.ukuu.org.uk> wrote: > > > optional), I can do that too. The security model of "having a group > > gives you less privilege than not having it" seems crazy, but > > nonetheless I can see a couple of easy ways that we can avoid breaking > > It's an old pattern of use that makes complete sense in a traditional > Unix permission world because it's the only way to do "exclude {list}" > nicely. Our default IMHO shouldn't break this. > > > that pattern, no_new_privs being one of them. I'd like to make sure > > that nobody sees any other real-world corner case that unprivileged > > setgroups would break. > > Barring the usual risk of people doing improper error checking I don't > see one immediately. > > For containers I think it actually makes sense that the sysctl can be > applied per container anyway. We'll probably need per container sysctls some day. > > Alan ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-17 18:07 ` Andy Lutomirski 0 siblings, 0 replies; 73+ messages in thread From: Andy Lutomirski @ 2014-11-17 18:07 UTC (permalink / raw) To: One Thousand Gnomes Cc: linux-man, Ted Ts'o, Michael Kerrisk-manpages, Josh Triplett, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Andrew Morton, Eric W. Biederman, Linux API, Kees Cook On Nov 17, 2014 3:37 AM, "One Thousand Gnomes" <gnomes-qBU/x9rampVanCEyBjwyrvXRex20P6io@public.gmane.org> wrote: > > > optional), I can do that too. The security model of "having a group > > gives you less privilege than not having it" seems crazy, but > > nonetheless I can see a couple of easy ways that we can avoid breaking > > It's an old pattern of use that makes complete sense in a traditional > Unix permission world because it's the only way to do "exclude {list}" > nicely. Our default IMHO shouldn't break this. > > > that pattern, no_new_privs being one of them. I'd like to make sure > > that nobody sees any other real-world corner case that unprivileged > > setgroups would break. > > Barring the usual risk of people doing improper error checking I don't > see one immediately. > > For containers I think it actually makes sense that the sysctl can be > applied per container anyway. We'll probably need per container sysctls some day. > > Alan -- To unsubscribe from this list: send the line "unsubscribe linux-man" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-17 22:11 ` Eric W.Biederman 0 siblings, 0 replies; 73+ messages in thread From: Eric W.Biederman @ 2014-11-17 22:11 UTC (permalink / raw) To: Andy Lutomirski, One Thousand Gnomes Cc: linux-man, Ted Ts'o, Michael Kerrisk-manpages, Josh Triplett, linux-kernel, Andrew Morton, Linux API, Kees Cook On November 17, 2014 1:07:30 PM EST, Andy Lutomirski <luto@amacapital.net> wrote: >On Nov 17, 2014 3:37 AM, "One Thousand Gnomes" ><gnomes@lxorguk.ukuu.org.uk> wrote: >> >> > optional), I can do that too. The security model of "having a >group >> > gives you less privilege than not having it" seems crazy, but >> > nonetheless I can see a couple of easy ways that we can avoid >breaking >> >> It's an old pattern of use that makes complete sense in a traditional >> Unix permission world because it's the only way to do "exclude >{list}" >> nicely. Our default IMHO shouldn't break this. >> >> > that pattern, no_new_privs being one of them. I'd like to make >sure >> > that nobody sees any other real-world corner case that unprivileged >> > setgroups would break. >> >> Barring the usual risk of people doing improper error checking I >don't >> see one immediately. >> >> For containers I think it actually makes sense that the sysctl can be >> applied per container anyway. > >We'll probably need per container sysctls some day. We already have a mess of per network namespace sysctls, as well as few for other namespaces. We have the infrastructure it is just a matter of using it for whatever purpose we need. Eric ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-17 22:11 ` Eric W.Biederman 0 siblings, 0 replies; 73+ messages in thread From: Eric W.Biederman @ 2014-11-17 22:11 UTC (permalink / raw) To: Andy Lutomirski, One Thousand Gnomes Cc: linux-man, Ted Ts'o, Michael Kerrisk-manpages, Josh Triplett, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Andrew Morton, Linux API, Kees Cook On November 17, 2014 1:07:30 PM EST, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote: >On Nov 17, 2014 3:37 AM, "One Thousand Gnomes" ><gnomes-qBU/x9rampVanCEyBjwyrvXRex20P6io@public.gmane.org> wrote: >> >> > optional), I can do that too. The security model of "having a >group >> > gives you less privilege than not having it" seems crazy, but >> > nonetheless I can see a couple of easy ways that we can avoid >breaking >> >> It's an old pattern of use that makes complete sense in a traditional >> Unix permission world because it's the only way to do "exclude >{list}" >> nicely. Our default IMHO shouldn't break this. >> >> > that pattern, no_new_privs being one of them. I'd like to make >sure >> > that nobody sees any other real-world corner case that unprivileged >> > setgroups would break. >> >> Barring the usual risk of people doing improper error checking I >don't >> see one immediately. >> >> For containers I think it actually makes sense that the sysctl can be >> applied per container anyway. > >We'll probably need per container sysctls some day. We already have a mess of per network namespace sysctls, as well as few for other namespaces. We have the infrastructure it is just a matter of using it for whatever purpose we need. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-man" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-17 22:22 ` Andy Lutomirski 0 siblings, 0 replies; 73+ messages in thread From: Andy Lutomirski @ 2014-11-17 22:22 UTC (permalink / raw) To: Eric W.Biederman Cc: One Thousand Gnomes, linux-man, Ted Ts'o, Michael Kerrisk-manpages, Josh Triplett, linux-kernel, Andrew Morton, Linux API, Kees Cook On Mon, Nov 17, 2014 at 2:11 PM, Eric W.Biederman <ebiederm@xmission.com> wrote: > > > On November 17, 2014 1:07:30 PM EST, Andy Lutomirski <luto@amacapital.net> wrote: >>On Nov 17, 2014 3:37 AM, "One Thousand Gnomes" >><gnomes@lxorguk.ukuu.org.uk> wrote: >>> >>> > optional), I can do that too. The security model of "having a >>group >>> > gives you less privilege than not having it" seems crazy, but >>> > nonetheless I can see a couple of easy ways that we can avoid >>breaking >>> >>> It's an old pattern of use that makes complete sense in a traditional >>> Unix permission world because it's the only way to do "exclude >>{list}" >>> nicely. Our default IMHO shouldn't break this. >>> >>> > that pattern, no_new_privs being one of them. I'd like to make >>sure >>> > that nobody sees any other real-world corner case that unprivileged >>> > setgroups would break. >>> >>> Barring the usual risk of people doing improper error checking I >>don't >>> see one immediately. >>> >>> For containers I think it actually makes sense that the sysctl can be >>> applied per container anyway. >> >>We'll probably need per container sysctls some day. > > We already have a mess of per network namespace sysctls, > as well as few for other namespaces. > > We have the infrastructure it is just a matter of using it for whatever purpose we need. > A list of group id ranges that it's permissible to drop would do the trick, both for setgroups and for unshare. The downside would be that users in those groups (i.e. everyone by default) would not be able to unshare their user ns. Better ideas welcome. --Andy ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-17 22:22 ` Andy Lutomirski 0 siblings, 0 replies; 73+ messages in thread From: Andy Lutomirski @ 2014-11-17 22:22 UTC (permalink / raw) To: Eric W.Biederman Cc: One Thousand Gnomes, linux-man, Ted Ts'o, Michael Kerrisk-manpages, Josh Triplett, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Andrew Morton, Linux API, Kees Cook On Mon, Nov 17, 2014 at 2:11 PM, Eric W.Biederman <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> wrote: > > > On November 17, 2014 1:07:30 PM EST, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote: >>On Nov 17, 2014 3:37 AM, "One Thousand Gnomes" >><gnomes-qBU/x9rampVanCEyBjwyrvXRex20P6io@public.gmane.org> wrote: >>> >>> > optional), I can do that too. The security model of "having a >>group >>> > gives you less privilege than not having it" seems crazy, but >>> > nonetheless I can see a couple of easy ways that we can avoid >>breaking >>> >>> It's an old pattern of use that makes complete sense in a traditional >>> Unix permission world because it's the only way to do "exclude >>{list}" >>> nicely. Our default IMHO shouldn't break this. >>> >>> > that pattern, no_new_privs being one of them. I'd like to make >>sure >>> > that nobody sees any other real-world corner case that unprivileged >>> > setgroups would break. >>> >>> Barring the usual risk of people doing improper error checking I >>don't >>> see one immediately. >>> >>> For containers I think it actually makes sense that the sysctl can be >>> applied per container anyway. >> >>We'll probably need per container sysctls some day. > > We already have a mess of per network namespace sysctls, > as well as few for other namespaces. > > We have the infrastructure it is just a matter of using it for whatever purpose we need. > A list of group id ranges that it's permissible to drop would do the trick, both for setgroups and for unshare. The downside would be that users in those groups (i.e. everyone by default) would not be able to unshare their user ns. Better ideas welcome. --Andy -- To unsubscribe from this list: send the line "unsubscribe linux-man" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-17 22:37 ` josh-iaAMLnmF4UmaiuxdJuQwMA 0 siblings, 0 replies; 73+ messages in thread From: josh @ 2014-11-17 22:37 UTC (permalink / raw) To: Andy Lutomirski Cc: Eric W.Biederman, One Thousand Gnomes, linux-man, Ted Ts'o, Michael Kerrisk-manpages, linux-kernel, Andrew Morton, Linux API, Kees Cook On Mon, Nov 17, 2014 at 02:22:59PM -0800, Andy Lutomirski wrote: > On Mon, Nov 17, 2014 at 2:11 PM, Eric W.Biederman <ebiederm@xmission.com> wrote: > > > > > > On November 17, 2014 1:07:30 PM EST, Andy Lutomirski <luto@amacapital.net> wrote: > >>On Nov 17, 2014 3:37 AM, "One Thousand Gnomes" > >><gnomes@lxorguk.ukuu.org.uk> wrote: > >>> > >>> > optional), I can do that too. The security model of "having a > >>group > >>> > gives you less privilege than not having it" seems crazy, but > >>> > nonetheless I can see a couple of easy ways that we can avoid > >>breaking > >>> > >>> It's an old pattern of use that makes complete sense in a traditional > >>> Unix permission world because it's the only way to do "exclude > >>{list}" > >>> nicely. Our default IMHO shouldn't break this. > >>> > >>> > that pattern, no_new_privs being one of them. I'd like to make > >>sure > >>> > that nobody sees any other real-world corner case that unprivileged > >>> > setgroups would break. > >>> > >>> Barring the usual risk of people doing improper error checking I > >>don't > >>> see one immediately. > >>> > >>> For containers I think it actually makes sense that the sysctl can be > >>> applied per container anyway. > >> > >>We'll probably need per container sysctls some day. > > > > We already have a mess of per network namespace sysctls, > > as well as few for other namespaces. > > > > We have the infrastructure it is just a matter of using it for whatever purpose we need. > > > > A list of group id ranges that it's permissible to drop would do the > trick, both for setgroups and for unshare. The downside would be that > users in those groups (i.e. everyone by default) would not be able to > unshare their user ns. > > Better ideas welcome. Personally, I think that seems like more flexibility than necessary to achieve the goal. I think a sysctl turning group-dropping on and off would suffice; systems that know they don't use groups to exclude specific users can enable that sysctl. - Josh Triplett ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-17 22:37 ` josh-iaAMLnmF4UmaiuxdJuQwMA 0 siblings, 0 replies; 73+ messages in thread From: josh-iaAMLnmF4UmaiuxdJuQwMA @ 2014-11-17 22:37 UTC (permalink / raw) To: Andy Lutomirski Cc: Eric W.Biederman, One Thousand Gnomes, linux-man, Ted Ts'o, Michael Kerrisk-manpages, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Andrew Morton, Linux API, Kees Cook On Mon, Nov 17, 2014 at 02:22:59PM -0800, Andy Lutomirski wrote: > On Mon, Nov 17, 2014 at 2:11 PM, Eric W.Biederman <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> wrote: > > > > > > On November 17, 2014 1:07:30 PM EST, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote: > >>On Nov 17, 2014 3:37 AM, "One Thousand Gnomes" > >><gnomes-qBU/x9rampVanCEyBjwyrvXRex20P6io@public.gmane.org> wrote: > >>> > >>> > optional), I can do that too. The security model of "having a > >>group > >>> > gives you less privilege than not having it" seems crazy, but > >>> > nonetheless I can see a couple of easy ways that we can avoid > >>breaking > >>> > >>> It's an old pattern of use that makes complete sense in a traditional > >>> Unix permission world because it's the only way to do "exclude > >>{list}" > >>> nicely. Our default IMHO shouldn't break this. > >>> > >>> > that pattern, no_new_privs being one of them. I'd like to make > >>sure > >>> > that nobody sees any other real-world corner case that unprivileged > >>> > setgroups would break. > >>> > >>> Barring the usual risk of people doing improper error checking I > >>don't > >>> see one immediately. > >>> > >>> For containers I think it actually makes sense that the sysctl can be > >>> applied per container anyway. > >> > >>We'll probably need per container sysctls some day. > > > > We already have a mess of per network namespace sysctls, > > as well as few for other namespaces. > > > > We have the infrastructure it is just a matter of using it for whatever purpose we need. > > > > A list of group id ranges that it's permissible to drop would do the > trick, both for setgroups and for unshare. The downside would be that > users in those groups (i.e. everyone by default) would not be able to > unshare their user ns. > > Better ideas welcome. Personally, I think that seems like more flexibility than necessary to achieve the goal. I think a sysctl turning group-dropping on and off would suffice; systems that know they don't use groups to exclude specific users can enable that sysctl. - Josh Triplett ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups 2014-11-17 22:37 ` josh-iaAMLnmF4UmaiuxdJuQwMA (?) @ 2014-11-18 0:56 ` Casey Schaufler -1 siblings, 0 replies; 73+ messages in thread From: Casey Schaufler @ 2014-11-18 0:56 UTC (permalink / raw) To: josh, Andy Lutomirski Cc: Eric W.Biederman, One Thousand Gnomes, linux-man, Ted Ts'o, Michael Kerrisk-manpages, linux-kernel, Andrew Morton, Linux API, Kees Cook, LSM On 11/17/2014 2:37 PM, josh@joshtriplett.org wrote: > On Mon, Nov 17, 2014 at 02:22:59PM -0800, Andy Lutomirski wrote: >> On Mon, Nov 17, 2014 at 2:11 PM, Eric W.Biederman <ebiederm@xmission.com> wrote: >>> >>> On November 17, 2014 1:07:30 PM EST, Andy Lutomirski <luto@amacapital.net> wrote: >>>> On Nov 17, 2014 3:37 AM, "One Thousand Gnomes" >>>> <gnomes@lxorguk.ukuu.org.uk> wrote: >>>>>> optional), I can do that too. The security model of "having a >>>> group >>>>>> gives you less privilege than not having it" seems crazy, but >>>>>> nonetheless I can see a couple of easy ways that we can avoid >>>> breaking >>>>> It's an old pattern of use that makes complete sense in a traditional >>>>> Unix permission world because it's the only way to do "exclude >>>> {list}" >>>>> nicely. Our default IMHO shouldn't break this. >>>>> >>>>>> that pattern, no_new_privs being one of them. I'd like to make >>>> sure >>>>>> that nobody sees any other real-world corner case that unprivileged >>>>>> setgroups would break. >>>>> Barring the usual risk of people doing improper error checking I >>>> don't >>>>> see one immediately. >>>>> >>>>> For containers I think it actually makes sense that the sysctl can be >>>>> applied per container anyway. >>>> We'll probably need per container sysctls some day. >>> We already have a mess of per network namespace sysctls, >>> as well as few for other namespaces. >>> >>> We have the infrastructure it is just a matter of using it for whatever purpose we need. >>> >> A list of group id ranges that it's permissible to drop would do the >> trick, both for setgroups and for unshare. The downside would be that >> users in those groups (i.e. everyone by default) would not be able to >> unshare their user ns. >> >> Better ideas welcome. > Personally, I think that seems like more flexibility than necessary to > achieve the goal. I think a sysctl turning group-dropping on and off > would suffice; systems that know they don't use groups to exclude > specific users can enable that sysctl. Right. Until someone comes along and installs a package that uses groups in this particular way. You can't count on the fact that someone isn't using it in that particular way today as an indicator that they won't tomorrow. Are you thinking about providing a tool that will tell sysadmins whether or not their system is safe to use this option? Certainly you are going to suggest that most sysadmins would know how to figure out if it is safe to use this option. The developers of user namespaces didn't notice it might be a problem. You can't count on sysadmins or distro developers to do better. > > - Josh Triplett > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups 2014-11-15 9:01 ` [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups Josh Triplett 2014-11-15 15:37 ` Eric W. Biederman @ 2014-11-17 18:06 ` Casey Schaufler 2014-11-17 18:31 ` Andy Lutomirski 1 sibling, 1 reply; 73+ messages in thread From: Casey Schaufler @ 2014-11-17 18:06 UTC (permalink / raw) To: Josh Triplett, Andrew Morton, Eric W. Biederman, Kees Cook, mtk.manpages, linux-api, linux-man, linux-kernel, Casey Schaufler On 11/15/2014 1:01 AM, Josh Triplett wrote: > Currently, unprivileged processes (without CAP_SETGID) cannot call > setgroups at all. In particular, processes with a set of supplementary > groups cannot further drop permissions without obtaining elevated > permissions first. Has anyone put any thought into how this will interact with POSIX ACLs? I don't see that anywhere in the discussion. Tizen takes advantage of the fact you can't drop groups. If a process can drop itself out of groups without privilege it can circumvent the system security policy. Back when the LSM was introduced a choice was made between authoritative hooks (which would have allowed this sort of thing) and restrictive hooks (which would not). Authoritative hooks were rejected because they would have "broken Linux". I hope that the people who spoke up then will speak up now. This is going to introduce a whole class of vulnerabilities. Don't even think of arguing that the reduction in use of privilege will make up for that. Developers have enough trouble with the difference between setuid() and seteuid() to expect them to use group dropping properly. > > Allow unprivileged processes to call setgroups with a subset of their > current groups; only require CAP_SETGID to add a group the process does > not currently have. > > The kernel already maintains the list of supplementary group IDs in > sorted order, and setgroups already needs to sort the new list, so this > just requires a linear comparison of the two sorted lists. > > This moves the CAP_SETGID test from setgroups into set_current_groups. > > Tested via the following test program: > > #include <err.h> > #include <grp.h> > #include <stdio.h> > #include <sys/types.h> > #include <unistd.h> > > void run_id(void) > { > pid_t p = fork(); > switch (p) { > case -1: > err(1, "fork"); > case 0: > execl("/usr/bin/id", "id", NULL); > err(1, "exec"); > default: > if (waitpid(p, NULL, 0) < 0) > err(1, "waitpid"); > } > } > > int main(void) > { > gid_t list1[] = { 1, 2, 3, 4, 5 }; > gid_t list2[] = { 2, 3, 4 }; > run_id(); > if (setgroups(5, list1) < 0) > err(1, "setgroups 1"); > run_id(); > if (setresgid(1, 1, 1) < 0) > err(1, "setresgid"); > if (setresuid(1, 1, 1) < 0) > err(1, "setresuid"); > run_id(); > if (setgroups(3, list2) < 0) > err(1, "setgroups 2"); > run_id(); > if (setgroups(5, list1) < 0) > err(1, "setgroups 3"); > run_id(); > > return 0; > } > > Without this patch, the test program gets EPERM from the second > setgroups call, after dropping root privileges. With this patch, the > test program successfully drops groups 1 and 5, but then gets EPERM from > the third setgroups call, since that call attempts to add groups the > process does not currently have. > > Signed-off-by: Josh Triplett <josh@joshtriplett.org> > --- > kernel/groups.c | 33 ++++++++++++++++++++++++++++++--- > kernel/uid16.c | 2 -- > 2 files changed, 30 insertions(+), 5 deletions(-) > > diff --git a/kernel/groups.c b/kernel/groups.c > index f0667e7..fe7367d 100644 > --- a/kernel/groups.c > +++ b/kernel/groups.c > @@ -153,6 +153,29 @@ int groups_search(const struct group_info *group_info, kgid_t grp) > return 0; > } > > +/* Compare two sorted group lists; return true if the first is a subset of the > + * second. > + */ > +static bool is_subset(const struct group_info *g1, const struct group_info *g2) > +{ > + unsigned int i, j; > + > + for (i = 0, j = 0; i < g1->ngroups; i++) { > + kgid_t gid1 = GROUP_AT(g1, i); > + kgid_t gid2; > + for (; j < g2->ngroups; j++) { > + gid2 = GROUP_AT(g2, j); > + if (gid_lte(gid1, gid2)) > + break; > + } > + if (j >= g2->ngroups || !gid_eq(gid1, gid2)) > + return false; > + j++; > + } > + > + return true; > +} > + > /** > * set_groups_sorted - Change a group subscription in a set of credentials > * @new: The newly prepared set of credentials to alter > @@ -189,11 +212,17 @@ int set_current_groups(struct group_info *group_info) > { > struct cred *new; > > + groups_sort(group_info); > new = prepare_creds(); > if (!new) > return -ENOMEM; > + if (!ns_capable(current_user_ns(), CAP_SETGID) > + && !is_subset(group_info, new->group_info)) { > + abort_creds(new); > + return -EPERM; > + } > > - set_groups(new, group_info); > + set_groups_sorted(new, group_info); > return commit_creds(new); > } > > @@ -233,8 +262,6 @@ SYSCALL_DEFINE2(setgroups, int, gidsetsize, gid_t __user *, grouplist) > struct group_info *group_info; > int retval; > > - if (!ns_capable(current_user_ns(), CAP_SETGID)) > - return -EPERM; > if ((unsigned)gidsetsize > NGROUPS_MAX) > return -EINVAL; > > diff --git a/kernel/uid16.c b/kernel/uid16.c > index 602e5bb..b27e167 100644 > --- a/kernel/uid16.c > +++ b/kernel/uid16.c > @@ -176,8 +176,6 @@ SYSCALL_DEFINE2(setgroups16, int, gidsetsize, old_gid_t __user *, grouplist) > struct group_info *group_info; > int retval; > > - if (!ns_capable(current_user_ns(), CAP_SETGID)) > - return -EPERM; > if ((unsigned)gidsetsize > NGROUPS_MAX) > return -EINVAL; > ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-17 18:31 ` Andy Lutomirski 0 siblings, 0 replies; 73+ messages in thread From: Andy Lutomirski @ 2014-11-17 18:31 UTC (permalink / raw) To: Casey Schaufler Cc: Josh Triplett, Andrew Morton, Eric W. Biederman, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel On Mon, Nov 17, 2014 at 10:06 AM, Casey Schaufler <casey@schaufler-ca.com> wrote: > On 11/15/2014 1:01 AM, Josh Triplett wrote: >> Currently, unprivileged processes (without CAP_SETGID) cannot call >> setgroups at all. In particular, processes with a set of supplementary >> groups cannot further drop permissions without obtaining elevated >> permissions first. > > Has anyone put any thought into how this will interact with > POSIX ACLs? I don't see that anywhere in the discussion. That means that user namespaces are a problem, too, and we need to fix it. Or we should add some control to turn unprivileged user namespace creation on and off and document that turning it on defeats POSIX ACLs with a group entry that is more restrictive than the other entry. --Andy > > Tizen takes advantage of the fact you can't drop groups. If > a process can drop itself out of groups without privilege > it can circumvent the system security policy. > > Back when the LSM was introduced a choice was made between > authoritative hooks (which would have allowed this sort of thing) > and restrictive hooks (which would not). Authoritative hooks were > rejected because they would have "broken Linux". I hope that the > people who spoke up then will speak up now. > > This is going to introduce a whole class of vulnerabilities. > Don't even think of arguing that the reduction in use of privilege > will make up for that. Developers have enough trouble with the > difference between setuid() and seteuid() to expect them to use > group dropping properly. > >> >> Allow unprivileged processes to call setgroups with a subset of their >> current groups; only require CAP_SETGID to add a group the process does >> not currently have. >> >> The kernel already maintains the list of supplementary group IDs in >> sorted order, and setgroups already needs to sort the new list, so this >> just requires a linear comparison of the two sorted lists. >> >> This moves the CAP_SETGID test from setgroups into set_current_groups. >> >> Tested via the following test program: >> >> #include <err.h> >> #include <grp.h> >> #include <stdio.h> >> #include <sys/types.h> >> #include <unistd.h> >> >> void run_id(void) >> { >> pid_t p = fork(); >> switch (p) { >> case -1: >> err(1, "fork"); >> case 0: >> execl("/usr/bin/id", "id", NULL); >> err(1, "exec"); >> default: >> if (waitpid(p, NULL, 0) < 0) >> err(1, "waitpid"); >> } >> } >> >> int main(void) >> { >> gid_t list1[] = { 1, 2, 3, 4, 5 }; >> gid_t list2[] = { 2, 3, 4 }; >> run_id(); >> if (setgroups(5, list1) < 0) >> err(1, "setgroups 1"); >> run_id(); >> if (setresgid(1, 1, 1) < 0) >> err(1, "setresgid"); >> if (setresuid(1, 1, 1) < 0) >> err(1, "setresuid"); >> run_id(); >> if (setgroups(3, list2) < 0) >> err(1, "setgroups 2"); >> run_id(); >> if (setgroups(5, list1) < 0) >> err(1, "setgroups 3"); >> run_id(); >> >> return 0; >> } >> >> Without this patch, the test program gets EPERM from the second >> setgroups call, after dropping root privileges. With this patch, the >> test program successfully drops groups 1 and 5, but then gets EPERM from >> the third setgroups call, since that call attempts to add groups the >> process does not currently have. >> >> Signed-off-by: Josh Triplett <josh@joshtriplett.org> >> --- >> kernel/groups.c | 33 ++++++++++++++++++++++++++++++--- >> kernel/uid16.c | 2 -- >> 2 files changed, 30 insertions(+), 5 deletions(-) >> >> diff --git a/kernel/groups.c b/kernel/groups.c >> index f0667e7..fe7367d 100644 >> --- a/kernel/groups.c >> +++ b/kernel/groups.c >> @@ -153,6 +153,29 @@ int groups_search(const struct group_info *group_info, kgid_t grp) >> return 0; >> } >> >> +/* Compare two sorted group lists; return true if the first is a subset of the >> + * second. >> + */ >> +static bool is_subset(const struct group_info *g1, const struct group_info *g2) >> +{ >> + unsigned int i, j; >> + >> + for (i = 0, j = 0; i < g1->ngroups; i++) { >> + kgid_t gid1 = GROUP_AT(g1, i); >> + kgid_t gid2; >> + for (; j < g2->ngroups; j++) { >> + gid2 = GROUP_AT(g2, j); >> + if (gid_lte(gid1, gid2)) >> + break; >> + } >> + if (j >= g2->ngroups || !gid_eq(gid1, gid2)) >> + return false; >> + j++; >> + } >> + >> + return true; >> +} >> + >> /** >> * set_groups_sorted - Change a group subscription in a set of credentials >> * @new: The newly prepared set of credentials to alter >> @@ -189,11 +212,17 @@ int set_current_groups(struct group_info *group_info) >> { >> struct cred *new; >> >> + groups_sort(group_info); >> new = prepare_creds(); >> if (!new) >> return -ENOMEM; >> + if (!ns_capable(current_user_ns(), CAP_SETGID) >> + && !is_subset(group_info, new->group_info)) { >> + abort_creds(new); >> + return -EPERM; >> + } >> >> - set_groups(new, group_info); >> + set_groups_sorted(new, group_info); >> return commit_creds(new); >> } >> >> @@ -233,8 +262,6 @@ SYSCALL_DEFINE2(setgroups, int, gidsetsize, gid_t __user *, grouplist) >> struct group_info *group_info; >> int retval; >> >> - if (!ns_capable(current_user_ns(), CAP_SETGID)) >> - return -EPERM; >> if ((unsigned)gidsetsize > NGROUPS_MAX) >> return -EINVAL; >> >> diff --git a/kernel/uid16.c b/kernel/uid16.c >> index 602e5bb..b27e167 100644 >> --- a/kernel/uid16.c >> +++ b/kernel/uid16.c >> @@ -176,8 +176,6 @@ SYSCALL_DEFINE2(setgroups16, int, gidsetsize, old_gid_t __user *, grouplist) >> struct group_info *group_info; >> int retval; >> >> - if (!ns_capable(current_user_ns(), CAP_SETGID)) >> - return -EPERM; >> if ((unsigned)gidsetsize > NGROUPS_MAX) >> return -EINVAL; >> > > -- > To unsubscribe from this list: send the line "unsubscribe linux-api" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Andy Lutomirski AMA Capital Management, LLC ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-17 18:31 ` Andy Lutomirski 0 siblings, 0 replies; 73+ messages in thread From: Andy Lutomirski @ 2014-11-17 18:31 UTC (permalink / raw) To: Casey Schaufler Cc: Josh Triplett, Andrew Morton, Eric W. Biederman, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel-u79uwXL29TY76Z2rM5mHXA On Mon, Nov 17, 2014 at 10:06 AM, Casey Schaufler <casey-iSGtlc1asvQWG2LlvL+J4A@public.gmane.org> wrote: > On 11/15/2014 1:01 AM, Josh Triplett wrote: >> Currently, unprivileged processes (without CAP_SETGID) cannot call >> setgroups at all. In particular, processes with a set of supplementary >> groups cannot further drop permissions without obtaining elevated >> permissions first. > > Has anyone put any thought into how this will interact with > POSIX ACLs? I don't see that anywhere in the discussion. That means that user namespaces are a problem, too, and we need to fix it. Or we should add some control to turn unprivileged user namespace creation on and off and document that turning it on defeats POSIX ACLs with a group entry that is more restrictive than the other entry. --Andy > > Tizen takes advantage of the fact you can't drop groups. If > a process can drop itself out of groups without privilege > it can circumvent the system security policy. > > Back when the LSM was introduced a choice was made between > authoritative hooks (which would have allowed this sort of thing) > and restrictive hooks (which would not). Authoritative hooks were > rejected because they would have "broken Linux". I hope that the > people who spoke up then will speak up now. > > This is going to introduce a whole class of vulnerabilities. > Don't even think of arguing that the reduction in use of privilege > will make up for that. Developers have enough trouble with the > difference between setuid() and seteuid() to expect them to use > group dropping properly. > >> >> Allow unprivileged processes to call setgroups with a subset of their >> current groups; only require CAP_SETGID to add a group the process does >> not currently have. >> >> The kernel already maintains the list of supplementary group IDs in >> sorted order, and setgroups already needs to sort the new list, so this >> just requires a linear comparison of the two sorted lists. >> >> This moves the CAP_SETGID test from setgroups into set_current_groups. >> >> Tested via the following test program: >> >> #include <err.h> >> #include <grp.h> >> #include <stdio.h> >> #include <sys/types.h> >> #include <unistd.h> >> >> void run_id(void) >> { >> pid_t p = fork(); >> switch (p) { >> case -1: >> err(1, "fork"); >> case 0: >> execl("/usr/bin/id", "id", NULL); >> err(1, "exec"); >> default: >> if (waitpid(p, NULL, 0) < 0) >> err(1, "waitpid"); >> } >> } >> >> int main(void) >> { >> gid_t list1[] = { 1, 2, 3, 4, 5 }; >> gid_t list2[] = { 2, 3, 4 }; >> run_id(); >> if (setgroups(5, list1) < 0) >> err(1, "setgroups 1"); >> run_id(); >> if (setresgid(1, 1, 1) < 0) >> err(1, "setresgid"); >> if (setresuid(1, 1, 1) < 0) >> err(1, "setresuid"); >> run_id(); >> if (setgroups(3, list2) < 0) >> err(1, "setgroups 2"); >> run_id(); >> if (setgroups(5, list1) < 0) >> err(1, "setgroups 3"); >> run_id(); >> >> return 0; >> } >> >> Without this patch, the test program gets EPERM from the second >> setgroups call, after dropping root privileges. With this patch, the >> test program successfully drops groups 1 and 5, but then gets EPERM from >> the third setgroups call, since that call attempts to add groups the >> process does not currently have. >> >> Signed-off-by: Josh Triplett <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org> >> --- >> kernel/groups.c | 33 ++++++++++++++++++++++++++++++--- >> kernel/uid16.c | 2 -- >> 2 files changed, 30 insertions(+), 5 deletions(-) >> >> diff --git a/kernel/groups.c b/kernel/groups.c >> index f0667e7..fe7367d 100644 >> --- a/kernel/groups.c >> +++ b/kernel/groups.c >> @@ -153,6 +153,29 @@ int groups_search(const struct group_info *group_info, kgid_t grp) >> return 0; >> } >> >> +/* Compare two sorted group lists; return true if the first is a subset of the >> + * second. >> + */ >> +static bool is_subset(const struct group_info *g1, const struct group_info *g2) >> +{ >> + unsigned int i, j; >> + >> + for (i = 0, j = 0; i < g1->ngroups; i++) { >> + kgid_t gid1 = GROUP_AT(g1, i); >> + kgid_t gid2; >> + for (; j < g2->ngroups; j++) { >> + gid2 = GROUP_AT(g2, j); >> + if (gid_lte(gid1, gid2)) >> + break; >> + } >> + if (j >= g2->ngroups || !gid_eq(gid1, gid2)) >> + return false; >> + j++; >> + } >> + >> + return true; >> +} >> + >> /** >> * set_groups_sorted - Change a group subscription in a set of credentials >> * @new: The newly prepared set of credentials to alter >> @@ -189,11 +212,17 @@ int set_current_groups(struct group_info *group_info) >> { >> struct cred *new; >> >> + groups_sort(group_info); >> new = prepare_creds(); >> if (!new) >> return -ENOMEM; >> + if (!ns_capable(current_user_ns(), CAP_SETGID) >> + && !is_subset(group_info, new->group_info)) { >> + abort_creds(new); >> + return -EPERM; >> + } >> >> - set_groups(new, group_info); >> + set_groups_sorted(new, group_info); >> return commit_creds(new); >> } >> >> @@ -233,8 +262,6 @@ SYSCALL_DEFINE2(setgroups, int, gidsetsize, gid_t __user *, grouplist) >> struct group_info *group_info; >> int retval; >> >> - if (!ns_capable(current_user_ns(), CAP_SETGID)) >> - return -EPERM; >> if ((unsigned)gidsetsize > NGROUPS_MAX) >> return -EINVAL; >> >> diff --git a/kernel/uid16.c b/kernel/uid16.c >> index 602e5bb..b27e167 100644 >> --- a/kernel/uid16.c >> +++ b/kernel/uid16.c >> @@ -176,8 +176,6 @@ SYSCALL_DEFINE2(setgroups16, int, gidsetsize, old_gid_t __user *, grouplist) >> struct group_info *group_info; >> int retval; >> >> - if (!ns_capable(current_user_ns(), CAP_SETGID)) >> - return -EPERM; >> if ((unsigned)gidsetsize > NGROUPS_MAX) >> return -EINVAL; >> > > -- > To unsubscribe from this list: send the line "unsubscribe linux-api" in > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Andy Lutomirski AMA Capital Management, LLC ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups 2014-11-17 18:31 ` Andy Lutomirski (?) @ 2014-11-17 18:46 ` Andy Lutomirski 2014-11-17 18:51 ` Casey Schaufler 2014-11-17 22:41 ` Eric W.Biederman -1 siblings, 2 replies; 73+ messages in thread From: Andy Lutomirski @ 2014-11-17 18:46 UTC (permalink / raw) To: Casey Schaufler Cc: Josh Triplett, Andrew Morton, Eric W. Biederman, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel On Mon, Nov 17, 2014 at 10:31 AM, Andy Lutomirski <luto@amacapital.net> wrote: > On Mon, Nov 17, 2014 at 10:06 AM, Casey Schaufler > <casey@schaufler-ca.com> wrote: >> On 11/15/2014 1:01 AM, Josh Triplett wrote: >>> Currently, unprivileged processes (without CAP_SETGID) cannot call >>> setgroups at all. In particular, processes with a set of supplementary >>> groups cannot further drop permissions without obtaining elevated >>> permissions first. >> >> Has anyone put any thought into how this will interact with >> POSIX ACLs? I don't see that anywhere in the discussion. > > That means that user namespaces are a problem, too, and we need to fix > it. Or we should add some control to turn unprivileged user namespace > creation on and off and document that turning it on defeats POSIX ACLs > with a group entry that is more restrictive than the other entry. > This is a significant enough issue that I posted it to oss-security: http://www.openwall.com/lists/oss-security/2014/11/17/19 It's not at all obvious to me how to fix it. We could disallow userns creation of any supplementary groups don't match fsuid, or we could keep negative-only groups around in the userns. It may be worth adding a sysctl to change the behavior, too. IMO it's absurd to use groups to deny permissions that are otherwise available. --Andy > --Andy > >> >> Tizen takes advantage of the fact you can't drop groups. If >> a process can drop itself out of groups without privilege >> it can circumvent the system security policy. >> >> Back when the LSM was introduced a choice was made between >> authoritative hooks (which would have allowed this sort of thing) >> and restrictive hooks (which would not). Authoritative hooks were >> rejected because they would have "broken Linux". I hope that the >> people who spoke up then will speak up now. >> >> This is going to introduce a whole class of vulnerabilities. >> Don't even think of arguing that the reduction in use of privilege >> will make up for that. Developers have enough trouble with the >> difference between setuid() and seteuid() to expect them to use >> group dropping properly. >> >>> >>> Allow unprivileged processes to call setgroups with a subset of their >>> current groups; only require CAP_SETGID to add a group the process does >>> not currently have. >>> >>> The kernel already maintains the list of supplementary group IDs in >>> sorted order, and setgroups already needs to sort the new list, so this >>> just requires a linear comparison of the two sorted lists. >>> >>> This moves the CAP_SETGID test from setgroups into set_current_groups. >>> >>> Tested via the following test program: >>> >>> #include <err.h> >>> #include <grp.h> >>> #include <stdio.h> >>> #include <sys/types.h> >>> #include <unistd.h> >>> >>> void run_id(void) >>> { >>> pid_t p = fork(); >>> switch (p) { >>> case -1: >>> err(1, "fork"); >>> case 0: >>> execl("/usr/bin/id", "id", NULL); >>> err(1, "exec"); >>> default: >>> if (waitpid(p, NULL, 0) < 0) >>> err(1, "waitpid"); >>> } >>> } >>> >>> int main(void) >>> { >>> gid_t list1[] = { 1, 2, 3, 4, 5 }; >>> gid_t list2[] = { 2, 3, 4 }; >>> run_id(); >>> if (setgroups(5, list1) < 0) >>> err(1, "setgroups 1"); >>> run_id(); >>> if (setresgid(1, 1, 1) < 0) >>> err(1, "setresgid"); >>> if (setresuid(1, 1, 1) < 0) >>> err(1, "setresuid"); >>> run_id(); >>> if (setgroups(3, list2) < 0) >>> err(1, "setgroups 2"); >>> run_id(); >>> if (setgroups(5, list1) < 0) >>> err(1, "setgroups 3"); >>> run_id(); >>> >>> return 0; >>> } >>> >>> Without this patch, the test program gets EPERM from the second >>> setgroups call, after dropping root privileges. With this patch, the >>> test program successfully drops groups 1 and 5, but then gets EPERM from >>> the third setgroups call, since that call attempts to add groups the >>> process does not currently have. >>> >>> Signed-off-by: Josh Triplett <josh@joshtriplett.org> >>> --- >>> kernel/groups.c | 33 ++++++++++++++++++++++++++++++--- >>> kernel/uid16.c | 2 -- >>> 2 files changed, 30 insertions(+), 5 deletions(-) >>> >>> diff --git a/kernel/groups.c b/kernel/groups.c >>> index f0667e7..fe7367d 100644 >>> --- a/kernel/groups.c >>> +++ b/kernel/groups.c >>> @@ -153,6 +153,29 @@ int groups_search(const struct group_info *group_info, kgid_t grp) >>> return 0; >>> } >>> >>> +/* Compare two sorted group lists; return true if the first is a subset of the >>> + * second. >>> + */ >>> +static bool is_subset(const struct group_info *g1, const struct group_info *g2) >>> +{ >>> + unsigned int i, j; >>> + >>> + for (i = 0, j = 0; i < g1->ngroups; i++) { >>> + kgid_t gid1 = GROUP_AT(g1, i); >>> + kgid_t gid2; >>> + for (; j < g2->ngroups; j++) { >>> + gid2 = GROUP_AT(g2, j); >>> + if (gid_lte(gid1, gid2)) >>> + break; >>> + } >>> + if (j >= g2->ngroups || !gid_eq(gid1, gid2)) >>> + return false; >>> + j++; >>> + } >>> + >>> + return true; >>> +} >>> + >>> /** >>> * set_groups_sorted - Change a group subscription in a set of credentials >>> * @new: The newly prepared set of credentials to alter >>> @@ -189,11 +212,17 @@ int set_current_groups(struct group_info *group_info) >>> { >>> struct cred *new; >>> >>> + groups_sort(group_info); >>> new = prepare_creds(); >>> if (!new) >>> return -ENOMEM; >>> + if (!ns_capable(current_user_ns(), CAP_SETGID) >>> + && !is_subset(group_info, new->group_info)) { >>> + abort_creds(new); >>> + return -EPERM; >>> + } >>> >>> - set_groups(new, group_info); >>> + set_groups_sorted(new, group_info); >>> return commit_creds(new); >>> } >>> >>> @@ -233,8 +262,6 @@ SYSCALL_DEFINE2(setgroups, int, gidsetsize, gid_t __user *, grouplist) >>> struct group_info *group_info; >>> int retval; >>> >>> - if (!ns_capable(current_user_ns(), CAP_SETGID)) >>> - return -EPERM; >>> if ((unsigned)gidsetsize > NGROUPS_MAX) >>> return -EINVAL; >>> >>> diff --git a/kernel/uid16.c b/kernel/uid16.c >>> index 602e5bb..b27e167 100644 >>> --- a/kernel/uid16.c >>> +++ b/kernel/uid16.c >>> @@ -176,8 +176,6 @@ SYSCALL_DEFINE2(setgroups16, int, gidsetsize, old_gid_t __user *, grouplist) >>> struct group_info *group_info; >>> int retval; >>> >>> - if (!ns_capable(current_user_ns(), CAP_SETGID)) >>> - return -EPERM; >>> if ((unsigned)gidsetsize > NGROUPS_MAX) >>> return -EINVAL; >>> >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-api" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > -- > Andy Lutomirski > AMA Capital Management, LLC -- Andy Lutomirski AMA Capital Management, LLC ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups 2014-11-17 18:46 ` Andy Lutomirski @ 2014-11-17 18:51 ` Casey Schaufler [not found] ` <546A43CE.2030706-iSGtlc1asvQWG2LlvL+J4A@public.gmane.org> 2014-11-17 22:41 ` Eric W.Biederman 1 sibling, 1 reply; 73+ messages in thread From: Casey Schaufler @ 2014-11-17 18:51 UTC (permalink / raw) To: Andy Lutomirski Cc: Josh Triplett, Andrew Morton, Eric W. Biederman, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel, LSM On 11/17/2014 10:46 AM, Andy Lutomirski wrote: > On Mon, Nov 17, 2014 at 10:31 AM, Andy Lutomirski <luto@amacapital.net> wrote: >> On Mon, Nov 17, 2014 at 10:06 AM, Casey Schaufler >> <casey@schaufler-ca.com> wrote: >>> On 11/15/2014 1:01 AM, Josh Triplett wrote: >>>> Currently, unprivileged processes (without CAP_SETGID) cannot call >>>> setgroups at all. In particular, processes with a set of supplementary >>>> groups cannot further drop permissions without obtaining elevated >>>> permissions first. >>> Has anyone put any thought into how this will interact with >>> POSIX ACLs? I don't see that anywhere in the discussion. >> That means that user namespaces are a problem, too, and we need to fix >> it. Or we should add some control to turn unprivileged user namespace >> creation on and off and document that turning it on defeats POSIX ACLs >> with a group entry that is more restrictive than the other entry. >> > This is a significant enough issue that I posted it to oss-security: > > http://www.openwall.com/lists/oss-security/2014/11/17/19 > > It's not at all obvious to me how to fix it. We could disallow userns > creation of any supplementary groups don't match fsuid, or we could > keep negative-only groups around in the userns. > > It may be worth adding a sysctl to change the behavior, too. IMO it's > absurd to use groups to deny permissions that are otherwise available. Absurd or not, it's traditional behavior, and if you don't have ACLs it is the best way to accomplish the security goal. > > --Andy > >> --Andy >> >>> Tizen takes advantage of the fact you can't drop groups. If >>> a process can drop itself out of groups without privilege >>> it can circumvent the system security policy. >>> >>> Back when the LSM was introduced a choice was made between >>> authoritative hooks (which would have allowed this sort of thing) >>> and restrictive hooks (which would not). Authoritative hooks were >>> rejected because they would have "broken Linux". I hope that the >>> people who spoke up then will speak up now. >>> >>> This is going to introduce a whole class of vulnerabilities. >>> Don't even think of arguing that the reduction in use of privilege >>> will make up for that. Developers have enough trouble with the >>> difference between setuid() and seteuid() to expect them to use >>> group dropping properly. >>> >>>> Allow unprivileged processes to call setgroups with a subset of their >>>> current groups; only require CAP_SETGID to add a group the process does >>>> not currently have. >>>> >>>> The kernel already maintains the list of supplementary group IDs in >>>> sorted order, and setgroups already needs to sort the new list, so this >>>> just requires a linear comparison of the two sorted lists. >>>> >>>> This moves the CAP_SETGID test from setgroups into set_current_groups. >>>> >>>> Tested via the following test program: >>>> >>>> #include <err.h> >>>> #include <grp.h> >>>> #include <stdio.h> >>>> #include <sys/types.h> >>>> #include <unistd.h> >>>> >>>> void run_id(void) >>>> { >>>> pid_t p = fork(); >>>> switch (p) { >>>> case -1: >>>> err(1, "fork"); >>>> case 0: >>>> execl("/usr/bin/id", "id", NULL); >>>> err(1, "exec"); >>>> default: >>>> if (waitpid(p, NULL, 0) < 0) >>>> err(1, "waitpid"); >>>> } >>>> } >>>> >>>> int main(void) >>>> { >>>> gid_t list1[] = { 1, 2, 3, 4, 5 }; >>>> gid_t list2[] = { 2, 3, 4 }; >>>> run_id(); >>>> if (setgroups(5, list1) < 0) >>>> err(1, "setgroups 1"); >>>> run_id(); >>>> if (setresgid(1, 1, 1) < 0) >>>> err(1, "setresgid"); >>>> if (setresuid(1, 1, 1) < 0) >>>> err(1, "setresuid"); >>>> run_id(); >>>> if (setgroups(3, list2) < 0) >>>> err(1, "setgroups 2"); >>>> run_id(); >>>> if (setgroups(5, list1) < 0) >>>> err(1, "setgroups 3"); >>>> run_id(); >>>> >>>> return 0; >>>> } >>>> >>>> Without this patch, the test program gets EPERM from the second >>>> setgroups call, after dropping root privileges. With this patch, the >>>> test program successfully drops groups 1 and 5, but then gets EPERM from >>>> the third setgroups call, since that call attempts to add groups the >>>> process does not currently have. >>>> >>>> Signed-off-by: Josh Triplett <josh@joshtriplett.org> >>>> --- >>>> kernel/groups.c | 33 ++++++++++++++++++++++++++++++--- >>>> kernel/uid16.c | 2 -- >>>> 2 files changed, 30 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/kernel/groups.c b/kernel/groups.c >>>> index f0667e7..fe7367d 100644 >>>> --- a/kernel/groups.c >>>> +++ b/kernel/groups.c >>>> @@ -153,6 +153,29 @@ int groups_search(const struct group_info *group_info, kgid_t grp) >>>> return 0; >>>> } >>>> >>>> +/* Compare two sorted group lists; return true if the first is a subset of the >>>> + * second. >>>> + */ >>>> +static bool is_subset(const struct group_info *g1, const struct group_info *g2) >>>> +{ >>>> + unsigned int i, j; >>>> + >>>> + for (i = 0, j = 0; i < g1->ngroups; i++) { >>>> + kgid_t gid1 = GROUP_AT(g1, i); >>>> + kgid_t gid2; >>>> + for (; j < g2->ngroups; j++) { >>>> + gid2 = GROUP_AT(g2, j); >>>> + if (gid_lte(gid1, gid2)) >>>> + break; >>>> + } >>>> + if (j >= g2->ngroups || !gid_eq(gid1, gid2)) >>>> + return false; >>>> + j++; >>>> + } >>>> + >>>> + return true; >>>> +} >>>> + >>>> /** >>>> * set_groups_sorted - Change a group subscription in a set of credentials >>>> * @new: The newly prepared set of credentials to alter >>>> @@ -189,11 +212,17 @@ int set_current_groups(struct group_info *group_info) >>>> { >>>> struct cred *new; >>>> >>>> + groups_sort(group_info); >>>> new = prepare_creds(); >>>> if (!new) >>>> return -ENOMEM; >>>> + if (!ns_capable(current_user_ns(), CAP_SETGID) >>>> + && !is_subset(group_info, new->group_info)) { >>>> + abort_creds(new); >>>> + return -EPERM; >>>> + } >>>> >>>> - set_groups(new, group_info); >>>> + set_groups_sorted(new, group_info); >>>> return commit_creds(new); >>>> } >>>> >>>> @@ -233,8 +262,6 @@ SYSCALL_DEFINE2(setgroups, int, gidsetsize, gid_t __user *, grouplist) >>>> struct group_info *group_info; >>>> int retval; >>>> >>>> - if (!ns_capable(current_user_ns(), CAP_SETGID)) >>>> - return -EPERM; >>>> if ((unsigned)gidsetsize > NGROUPS_MAX) >>>> return -EINVAL; >>>> >>>> diff --git a/kernel/uid16.c b/kernel/uid16.c >>>> index 602e5bb..b27e167 100644 >>>> --- a/kernel/uid16.c >>>> +++ b/kernel/uid16.c >>>> @@ -176,8 +176,6 @@ SYSCALL_DEFINE2(setgroups16, int, gidsetsize, old_gid_t __user *, grouplist) >>>> struct group_info *group_info; >>>> int retval; >>>> >>>> - if (!ns_capable(current_user_ns(), CAP_SETGID)) >>>> - return -EPERM; >>>> if ((unsigned)gidsetsize > NGROUPS_MAX) >>>> return -EINVAL; >>>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-api" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> >> -- >> Andy Lutomirski >> AMA Capital Management, LLC > > ^ permalink raw reply [flat|nested] 73+ messages in thread
[parent not found: <546A43CE.2030706-iSGtlc1asvQWG2LlvL+J4A@public.gmane.org>]
* [CFT][PATCH] userns: Avoid problems with negative groups 2014-11-17 18:51 ` Casey Schaufler @ 2014-11-27 16:59 ` Eric W. Biederman 0 siblings, 0 replies; 73+ messages in thread From: Eric W. Biederman @ 2014-11-27 16:59 UTC (permalink / raw) To: Linux Containers Cc: linux-man, Kees Cook, Linux API, Josh Triplett, Andy Lutomirski, LSM, Michael Kerrisk-manpages, Richard Weinberger, Casey Schaufler, Andrew Morton, linux-kernel-u79uwXL29TY76Z2rM5mHXA Classic unix permission checks have an interesting feature. The group permissions for a file can be set to less than the other permissions on a file. Occassionally this is used deliberately to give a certain group of users fewer permissions than the default. Overlooking negative groups has resulted in the permission checks for setting up a group mapping in a user namespace to be too lax. Tighten the permission checks in new_idmap_permitted to ensure that mapping uids and gids into user namespaces without privilege will not result in new combinations of credentials being available to the users. When setting mappings without privilege only the creator of the user namespace is interesting as all other users that have CAP_SETUID over the user namespace will also have CAP_SETUID over the user namespaces parent. So the scope of the unprivileged check is reduced to just the case where cred->euid is the namespace creator. For setting a uid mapping without privilege only euid is considered as setresuid can set uid, suid and fsuid from euid without privielege making any combination of uids possible with user namespaces already possible without them. For setting a gid mapping without privilege only egid on a credential without supplementary groups is condsidered, as setresgid can set gid, sgid and fsgid from egid without privilege. The requirement for no supplementary groups is because CAP_SETUID in a user namespace allows supplementary groups to be cleared, which unfortunately means allowing a credential with supplementary groups would allow new combinations of credentials to exist, and thus would allow defeating negative groups without permission. This change should break userspace by the minimal amount needed to fix this issue. This should fix CVE-2014-8989. Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> --- If people can test and review this change and verify this and verify it doesn't break anything I can't help breaking I will appreciate it. kernel/user_namespace.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c index aa312b0dc3ec..b338c42d04fd 100644 --- a/kernel/user_namespace.c +++ b/kernel/user_namespace.c @@ -812,16 +812,24 @@ static bool new_idmap_permitted(const struct file *file, struct user_namespace *ns, int cap_setid, struct uid_gid_map *new_map) { - /* Allow mapping to your own filesystem ids */ - if ((new_map->nr_extents == 1) && (new_map->extent[0].count == 1)) { + const struct cred *cred = file->f_cred; + + /* Allow mapping an id without CAP_SETUID and CAP_SETGID when + * allowing the root of the user namespace CAP_SETUID or + * CAP_SETGID restricted to just that id will not change the + * set of credentials available that user. + */ + if ((new_map->nr_extents == 1) && (new_map->extent[0].count == 1) && + uid_eq(ns->owner, cred->euid)) { u32 id = new_map->extent[0].lower_first; if (cap_setid == CAP_SETUID) { kuid_t uid = make_kuid(ns->parent, id); - if (uid_eq(uid, file->f_cred->fsuid)) + if (uid_eq(uid, cred->euid)) return true; } else if (cap_setid == CAP_SETGID) { kgid_t gid = make_kgid(ns->parent, id); - if (gid_eq(gid, file->f_cred->fsgid)) + if (gid_eq(gid, cred->egid) && + (cred->group_info->ngroups == 0)) return true; } } -- 1.9.1 ^ permalink raw reply related [flat|nested] 73+ messages in thread
* [CFT][PATCH] userns: Avoid problems with negative groups @ 2014-11-27 16:59 ` Eric W. Biederman 0 siblings, 0 replies; 73+ messages in thread From: Eric W. Biederman @ 2014-11-27 16:59 UTC (permalink / raw) To: Linux Containers Cc: Andy Lutomirski, Josh Triplett, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel, LSM, Casey Schaufler, Serge E. Hallyn, Richard Weinberger Classic unix permission checks have an interesting feature. The group permissions for a file can be set to less than the other permissions on a file. Occassionally this is used deliberately to give a certain group of users fewer permissions than the default. Overlooking negative groups has resulted in the permission checks for setting up a group mapping in a user namespace to be too lax. Tighten the permission checks in new_idmap_permitted to ensure that mapping uids and gids into user namespaces without privilege will not result in new combinations of credentials being available to the users. When setting mappings without privilege only the creator of the user namespace is interesting as all other users that have CAP_SETUID over the user namespace will also have CAP_SETUID over the user namespaces parent. So the scope of the unprivileged check is reduced to just the case where cred->euid is the namespace creator. For setting a uid mapping without privilege only euid is considered as setresuid can set uid, suid and fsuid from euid without privielege making any combination of uids possible with user namespaces already possible without them. For setting a gid mapping without privilege only egid on a credential without supplementary groups is condsidered, as setresgid can set gid, sgid and fsgid from egid without privilege. The requirement for no supplementary groups is because CAP_SETUID in a user namespace allows supplementary groups to be cleared, which unfortunately means allowing a credential with supplementary groups would allow new combinations of credentials to exist, and thus would allow defeating negative groups without permission. This change should break userspace by the minimal amount needed to fix this issue. This should fix CVE-2014-8989. Cc: stable@vger.kernel.org Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- If people can test and review this change and verify this and verify it doesn't break anything I can't help breaking I will appreciate it. kernel/user_namespace.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c index aa312b0dc3ec..b338c42d04fd 100644 --- a/kernel/user_namespace.c +++ b/kernel/user_namespace.c @@ -812,16 +812,24 @@ static bool new_idmap_permitted(const struct file *file, struct user_namespace *ns, int cap_setid, struct uid_gid_map *new_map) { - /* Allow mapping to your own filesystem ids */ - if ((new_map->nr_extents == 1) && (new_map->extent[0].count == 1)) { + const struct cred *cred = file->f_cred; + + /* Allow mapping an id without CAP_SETUID and CAP_SETGID when + * allowing the root of the user namespace CAP_SETUID or + * CAP_SETGID restricted to just that id will not change the + * set of credentials available that user. + */ + if ((new_map->nr_extents == 1) && (new_map->extent[0].count == 1) && + uid_eq(ns->owner, cred->euid)) { u32 id = new_map->extent[0].lower_first; if (cap_setid == CAP_SETUID) { kuid_t uid = make_kuid(ns->parent, id); - if (uid_eq(uid, file->f_cred->fsuid)) + if (uid_eq(uid, cred->euid)) return true; } else if (cap_setid == CAP_SETGID) { kgid_t gid = make_kgid(ns->parent, id); - if (gid_eq(gid, file->f_cred->fsgid)) + if (gid_eq(gid, cred->egid) && + (cred->group_info->ngroups == 0)) return true; } } -- 1.9.1 ^ permalink raw reply related [flat|nested] 73+ messages in thread
* Re: [CFT][PATCH] userns: Avoid problems with negative groups [not found] ` <87lhmwwpey.fsf_-_-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org> @ 2014-11-27 20:52 ` Andy Lutomirski 0 siblings, 0 replies; 73+ messages in thread From: Andy Lutomirski @ 2014-11-27 20:52 UTC (permalink / raw) To: Eric W. Biederman Cc: Linux Containers, Josh Triplett, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel, LSM, Casey Schaufler, Serge E. Hallyn, Richard Weinberger On Thu, Nov 27, 2014 at 8:59 AM, Eric W. Biederman <ebiederm@xmission.com> wrote: > > Classic unix permission checks have an interesting feature. The group > permissions for a file can be set to less than the other permissions > on a file. Occassionally this is used deliberately to give a certain > group of users fewer permissions than the default. > > Overlooking negative groups has resulted in the permission checks for > setting up a group mapping in a user namespace to be too lax. Tighten > the permission checks in new_idmap_permitted to ensure that mapping > uids and gids into user namespaces without privilege will not result > in new combinations of credentials being available to the users. > > When setting mappings without privilege only the creator of the user > namespace is interesting as all other users that have CAP_SETUID over > the user namespace will also have CAP_SETUID over the user namespaces > parent. So the scope of the unprivileged check is reduced to just > the case where cred->euid is the namespace creator. > > For setting a uid mapping without privilege only euid is considered as > setresuid can set uid, suid and fsuid from euid without privielege > making any combination of uids possible with user namespaces already > possible without them. > > For setting a gid mapping without privilege only egid on a credential > without supplementary groups is condsidered, as setresgid can set gid, > sgid and fsgid from egid without privilege. The requirement for no > supplementary groups is because CAP_SETUID in a user namespace allows > supplementary groups to be cleared, which unfortunately means allowing > a credential with supplementary groups would allow new combinations > of credentials to exist, and thus would allow defeating negative > groups without permission. > > This change should break userspace by the minimal amount needed > to fix this issue. > > This should fix CVE-2014-8989. I think this is both unnecessarily restrictive and that it doesn't fix the bug. For example, I can exploit CVE-2014-8989 without ever writing a uid map or a gid map. IIUC, the only real issue is that user namespaces allow groups to be dropped using setgroups that wouldn't otherwise be dropped. Can we get away with adding a per-user-ns flag that determines whether setgroups can be used? setgroups would be unusable until the gid_map has been written and then it would become usable if and only if the parent userns could use setgroups and the opener of gid_map was privileged. If we wanted to allow finer-grained control, we could allow writing control lines like: options +setgroups or options -setgroups in gid_map, or we could add user_ns_flags that can only be written once and only before either uid_map or gid_map is written. --Andy > > Cc: stable@vger.kernel.org > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> > --- > > If people can test and review this change and verify this and verify it > doesn't break anything I can't help breaking I will appreciate it. > > kernel/user_namespace.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c > index aa312b0dc3ec..b338c42d04fd 100644 > --- a/kernel/user_namespace.c > +++ b/kernel/user_namespace.c > @@ -812,16 +812,24 @@ static bool new_idmap_permitted(const struct file *file, > struct user_namespace *ns, int cap_setid, > struct uid_gid_map *new_map) > { > - /* Allow mapping to your own filesystem ids */ > - if ((new_map->nr_extents == 1) && (new_map->extent[0].count == 1)) { > + const struct cred *cred = file->f_cred; > + > + /* Allow mapping an id without CAP_SETUID and CAP_SETGID when > + * allowing the root of the user namespace CAP_SETUID or > + * CAP_SETGID restricted to just that id will not change the > + * set of credentials available that user. > + */ > + if ((new_map->nr_extents == 1) && (new_map->extent[0].count == 1) && > + uid_eq(ns->owner, cred->euid)) { > u32 id = new_map->extent[0].lower_first; > if (cap_setid == CAP_SETUID) { > kuid_t uid = make_kuid(ns->parent, id); > - if (uid_eq(uid, file->f_cred->fsuid)) > + if (uid_eq(uid, cred->euid)) > return true; > } else if (cap_setid == CAP_SETGID) { > kgid_t gid = make_kgid(ns->parent, id); > - if (gid_eq(gid, file->f_cred->fsgid)) > + if (gid_eq(gid, cred->egid) && > + (cred->group_info->ngroups == 0)) > return true; > } > } > -- > 1.9.1 > -- Andy Lutomirski AMA Capital Management, LLC ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [CFT][PATCH] userns: Avoid problems with negative groups @ 2014-11-27 20:52 ` Andy Lutomirski 0 siblings, 0 replies; 73+ messages in thread From: Andy Lutomirski @ 2014-11-27 20:52 UTC (permalink / raw) To: Eric W. Biederman Cc: Linux Containers, Josh Triplett, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel-u79uwXL29TY76Z2rM5mHXA, LSM, Casey Schaufler, Serge E. Hallyn, Richard Weinberger On Thu, Nov 27, 2014 at 8:59 AM, Eric W. Biederman <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> wrote: > > Classic unix permission checks have an interesting feature. The group > permissions for a file can be set to less than the other permissions > on a file. Occassionally this is used deliberately to give a certain > group of users fewer permissions than the default. > > Overlooking negative groups has resulted in the permission checks for > setting up a group mapping in a user namespace to be too lax. Tighten > the permission checks in new_idmap_permitted to ensure that mapping > uids and gids into user namespaces without privilege will not result > in new combinations of credentials being available to the users. > > When setting mappings without privilege only the creator of the user > namespace is interesting as all other users that have CAP_SETUID over > the user namespace will also have CAP_SETUID over the user namespaces > parent. So the scope of the unprivileged check is reduced to just > the case where cred->euid is the namespace creator. > > For setting a uid mapping without privilege only euid is considered as > setresuid can set uid, suid and fsuid from euid without privielege > making any combination of uids possible with user namespaces already > possible without them. > > For setting a gid mapping without privilege only egid on a credential > without supplementary groups is condsidered, as setresgid can set gid, > sgid and fsgid from egid without privilege. The requirement for no > supplementary groups is because CAP_SETUID in a user namespace allows > supplementary groups to be cleared, which unfortunately means allowing > a credential with supplementary groups would allow new combinations > of credentials to exist, and thus would allow defeating negative > groups without permission. > > This change should break userspace by the minimal amount needed > to fix this issue. > > This should fix CVE-2014-8989. I think this is both unnecessarily restrictive and that it doesn't fix the bug. For example, I can exploit CVE-2014-8989 without ever writing a uid map or a gid map. IIUC, the only real issue is that user namespaces allow groups to be dropped using setgroups that wouldn't otherwise be dropped. Can we get away with adding a per-user-ns flag that determines whether setgroups can be used? setgroups would be unusable until the gid_map has been written and then it would become usable if and only if the parent userns could use setgroups and the opener of gid_map was privileged. If we wanted to allow finer-grained control, we could allow writing control lines like: options +setgroups or options -setgroups in gid_map, or we could add user_ns_flags that can only be written once and only before either uid_map or gid_map is written. --Andy > > Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> > --- > > If people can test and review this change and verify this and verify it > doesn't break anything I can't help breaking I will appreciate it. > > kernel/user_namespace.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c > index aa312b0dc3ec..b338c42d04fd 100644 > --- a/kernel/user_namespace.c > +++ b/kernel/user_namespace.c > @@ -812,16 +812,24 @@ static bool new_idmap_permitted(const struct file *file, > struct user_namespace *ns, int cap_setid, > struct uid_gid_map *new_map) > { > - /* Allow mapping to your own filesystem ids */ > - if ((new_map->nr_extents == 1) && (new_map->extent[0].count == 1)) { > + const struct cred *cred = file->f_cred; > + > + /* Allow mapping an id without CAP_SETUID and CAP_SETGID when > + * allowing the root of the user namespace CAP_SETUID or > + * CAP_SETGID restricted to just that id will not change the > + * set of credentials available that user. > + */ > + if ((new_map->nr_extents == 1) && (new_map->extent[0].count == 1) && > + uid_eq(ns->owner, cred->euid)) { > u32 id = new_map->extent[0].lower_first; > if (cap_setid == CAP_SETUID) { > kuid_t uid = make_kuid(ns->parent, id); > - if (uid_eq(uid, file->f_cred->fsuid)) > + if (uid_eq(uid, cred->euid)) > return true; > } else if (cap_setid == CAP_SETGID) { > kgid_t gid = make_kgid(ns->parent, id); > - if (gid_eq(gid, file->f_cred->fsgid)) > + if (gid_eq(gid, cred->egid) && > + (cred->group_info->ngroups == 0)) > return true; > } > } > -- > 1.9.1 > -- Andy Lutomirski AMA Capital Management, LLC ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [CFT][PATCH] userns: Avoid problems with negative groups [not found] ` <CALCETrUuWDq2akKfb50AiPHeDDWzPW7ijz1QwnuNiskyZbBEfA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2014-11-28 5:21 ` Eric W. Biederman 0 siblings, 0 replies; 73+ messages in thread From: Eric W. Biederman @ 2014-11-28 5:21 UTC (permalink / raw) To: Andy Lutomirski Cc: Linux Containers, Josh Triplett, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel, LSM, Casey Schaufler, Serge E. Hallyn, Richard Weinberger Andy Lutomirski <luto@amacapital.net> writes: >> This change should break userspace by the minimal amount needed >> to fix this issue. >> >> This should fix CVE-2014-8989. > > I think this is both unnecessarily restrictive and that it doesn't fix > the bug. You are going to have to work very hard to convince me this is unnecessarily restrictive. >For example, I can exploit CVE-2014-8989 without ever > writing a uid map or a gid map. Yes. I realized just after I sent the patch that setgroups(0, NULL) would still work without a mapping set. That is a first glass grade a oversight that resulted in a bug. None of the other uid or gid changing syscalls without a mapping set, and setgroups was just overlooked because it was different. Oops. I will send an updated patch that stops setgroups from working without a mapping set shortly. > IIUC, the only real issue is that user namespaces allow groups to be > dropped using setgroups that wouldn't otherwise be dropped. Can we > get away with adding a per-user-ns flag that determines whether > setgroups can be used? Being able to call setgroups is fundamental to login programs, and login programs are one of the things user namespaces need to support. So adding an extra flag and an extra place where privilege is required is just noise, and will wind up breaking every user of user namespaces. Further being able to setup uid and gid mappings without privilege is primarily a nice to have. The original design did not have unprivileged setting of uid and gid maps and if it proves insecure I goofed and the feature isn't safe so it needs to be removed. This does mean that running a system with negative groups and users delegated subordinate gids in /etc/subuid is a bad idea and system administrators shouldn't do that as those negative groups won't prove effective in stopping their users. But this is all under system administrator control so shrug. There isn't a way to avoid that fundamental conflict. > setgroups would be unusable until the gid_map has been written and > then it would become usable if and only if the parent userns could use > setgroups and the opener of gid_map was privileged. That proposal sounds a lot more restrictive and a lot more of a pain to use than what I have implemented in my patch. > If we wanted to allow finer-grained control, we could allow writing > control lines like: > > options +setgroups > > or > > options -setgroups > > in gid_map, or we could add user_ns_flags that can only be written > once and only before either uid_map or gid_map is written. Definitely more complicated and I can't imagine a case where I need a gid map without needing to call setgroups. Eric ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [CFT][PATCH] userns: Avoid problems with negative groups @ 2014-11-28 5:21 ` Eric W. Biederman 0 siblings, 0 replies; 73+ messages in thread From: Eric W. Biederman @ 2014-11-28 5:21 UTC (permalink / raw) To: Andy Lutomirski Cc: Linux Containers, Josh Triplett, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel@vger.kernel.org, LSM, Casey Schaufler, Serge E. Hallyn, Richard Weinberger Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> writes: >> This change should break userspace by the minimal amount needed >> to fix this issue. >> >> This should fix CVE-2014-8989. > > I think this is both unnecessarily restrictive and that it doesn't fix > the bug. You are going to have to work very hard to convince me this is unnecessarily restrictive. >For example, I can exploit CVE-2014-8989 without ever > writing a uid map or a gid map. Yes. I realized just after I sent the patch that setgroups(0, NULL) would still work without a mapping set. That is a first glass grade a oversight that resulted in a bug. None of the other uid or gid changing syscalls without a mapping set, and setgroups was just overlooked because it was different. Oops. I will send an updated patch that stops setgroups from working without a mapping set shortly. > IIUC, the only real issue is that user namespaces allow groups to be > dropped using setgroups that wouldn't otherwise be dropped. Can we > get away with adding a per-user-ns flag that determines whether > setgroups can be used? Being able to call setgroups is fundamental to login programs, and login programs are one of the things user namespaces need to support. So adding an extra flag and an extra place where privilege is required is just noise, and will wind up breaking every user of user namespaces. Further being able to setup uid and gid mappings without privilege is primarily a nice to have. The original design did not have unprivileged setting of uid and gid maps and if it proves insecure I goofed and the feature isn't safe so it needs to be removed. This does mean that running a system with negative groups and users delegated subordinate gids in /etc/subuid is a bad idea and system administrators shouldn't do that as those negative groups won't prove effective in stopping their users. But this is all under system administrator control so shrug. There isn't a way to avoid that fundamental conflict. > setgroups would be unusable until the gid_map has been written and > then it would become usable if and only if the parent userns could use > setgroups and the opener of gid_map was privileged. That proposal sounds a lot more restrictive and a lot more of a pain to use than what I have implemented in my patch. > If we wanted to allow finer-grained control, we could allow writing > control lines like: > > options +setgroups > > or > > options -setgroups > > in gid_map, or we could add user_ns_flags that can only be written > once and only before either uid_map or gid_map is written. Definitely more complicated and I can't imagine a case where I need a gid map without needing to call setgroups. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-man" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 73+ messages in thread
[parent not found: <87wq6frjcw.fsf-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org>]
* [CFT][PATCH v2] userns: Avoid problems with negative groups 2014-11-28 5:21 ` Eric W. Biederman @ 2014-11-28 5:22 ` Eric W. Biederman -1 siblings, 0 replies; 73+ messages in thread From: Eric W. Biederman @ 2014-11-28 5:22 UTC (permalink / raw) To: Andy Lutomirski Cc: linux-man, Kees Cook, Linux API, Linux Containers, Josh Triplett, linux-kernel-u79uwXL29TY76Z2rM5mHXA, LSM, Michael Kerrisk-manpages, Richard Weinberger, Casey Schaufler, Andrew Morton Classic unix permission checks have an interesting feature. The group permissions for a file can be set to less than the other permissions on a file. Occassionally this is used deliberately to give a certain group of users fewer permissions than the default. Overlooking negative groups has resulted in the permission checks for setting up a group mapping in a user namespace to be too lax. Tighten the permission checks in new_idmap_permitted to ensure that mapping uids and gids into user namespaces without privilege will not result in new combinations of credentials being available to the users. When setting mappings without privilege only the creator of the user namespace is interesting as all other users that have CAP_SETUID over the user namespace will also have CAP_SETUID over the user namespaces parent. So the scope of the unprivileged check is reduced to just the case where cred->euid is the namespace creator. For setting a uid mapping without privilege only euid is considered as setresuid can set uid, suid and fsuid from euid without privielege making any combination of uids possible with user namespaces already possible without them. For setting a gid mapping without privilege only egid on a credential without supplementary groups is condsidered, as setresgid can set gid, sgid and fsgid from egid without privilege. The requirement for no supplementary groups is because CAP_SETUID in a user namespace allows supplementary groups to be cleared, which unfortunately means allowing a credential with supplementary groups would allow new combinations of credentials to exist, and thus would allow defeating negative groups without permission. setgroups is modified to fail not only when the group ids do not map but also when there are no gid mappings at all, preventing setgroups(0, NULL) from succeeding when gid mappings have not been established. This change should break userspace by the minimal amount needed to fix this issue. This should fix CVE-2014-8989. Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> --- include/linux/user_namespace.h | 10 ++++++++++ kernel/groups.c | 7 ++++++- kernel/uid16.c | 5 ++++- kernel/user_namespace.c | 16 ++++++++++++---- 4 files changed, 32 insertions(+), 6 deletions(-) diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h index e95372654f09..26d5e8f5db97 100644 --- a/include/linux/user_namespace.h +++ b/include/linux/user_namespace.h @@ -46,6 +46,11 @@ static inline struct user_namespace *get_user_ns(struct user_namespace *ns) return ns; } +static inline bool gid_mapping_possible(const struct user_namespace *ns) +{ + return ns->gid_map.nr_extents != 0; +} + extern int create_user_ns(struct cred *new); extern int unshare_userns(unsigned long unshare_flags, struct cred **new_cred); extern void free_user_ns(struct user_namespace *ns); @@ -70,6 +75,11 @@ static inline struct user_namespace *get_user_ns(struct user_namespace *ns) return &init_user_ns; } +static inline bool gid_mapping_possible(const struct user_namespace *ns) +{ + return true; +} + static inline int create_user_ns(struct cred *new) { return -EINVAL; diff --git a/kernel/groups.c b/kernel/groups.c index 451698f86cfa..302aa415158f 100644 --- a/kernel/groups.c +++ b/kernel/groups.c @@ -6,6 +6,7 @@ #include <linux/slab.h> #include <linux/security.h> #include <linux/syscalls.h> +#include <linux/user_namespace.h> #include <asm/uaccess.h> /* init to 2 - one for init_task, one to ensure it is never freed */ @@ -220,14 +221,18 @@ out: SYSCALL_DEFINE2(setgroups, int, gidsetsize, gid_t __user *, grouplist) { + struct user_namespace *user_ns = current_user_ns(); struct group_info *group_info; int retval; - if (!ns_capable(current_user_ns(), CAP_SETGID)) + if (!gid_mapping_possible(user_ns) || + !ns_capable(user_ns, CAP_SETGID)) return -EPERM; if ((unsigned)gidsetsize > NGROUPS_MAX) return -EINVAL; + /* How do I alloc a negative gidsetsize??? */ + group_info = groups_alloc(gidsetsize); if (!group_info) return -ENOMEM; diff --git a/kernel/uid16.c b/kernel/uid16.c index 602e5bbbceff..602c7de2aa11 100644 --- a/kernel/uid16.c +++ b/kernel/uid16.c @@ -13,6 +13,7 @@ #include <linux/highuid.h> #include <linux/security.h> #include <linux/syscalls.h> +#include <linux/user_namespace.h> #include <asm/uaccess.h> @@ -173,10 +174,12 @@ out: SYSCALL_DEFINE2(setgroups16, int, gidsetsize, old_gid_t __user *, grouplist) { + struct user_namespace *user_ns = current_user_ns(); struct group_info *group_info; int retval; - if (!ns_capable(current_user_ns(), CAP_SETGID)) + if (!gid_mapping_possible(user_ns) || + !ns_capable(user_ns, CAP_SETGID)) return -EPERM; if ((unsigned)gidsetsize > NGROUPS_MAX) return -EINVAL; diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c index aa312b0dc3ec..b338c42d04fd 100644 --- a/kernel/user_namespace.c +++ b/kernel/user_namespace.c @@ -812,16 +812,24 @@ static bool new_idmap_permitted(const struct file *file, struct user_namespace *ns, int cap_setid, struct uid_gid_map *new_map) { - /* Allow mapping to your own filesystem ids */ - if ((new_map->nr_extents == 1) && (new_map->extent[0].count == 1)) { + const struct cred *cred = file->f_cred; + + /* Allow mapping an id without CAP_SETUID and CAP_SETGID when + * allowing the root of the user namespace CAP_SETUID or + * CAP_SETGID restricted to just that id will not change the + * set of credentials available that user. + */ + if ((new_map->nr_extents == 1) && (new_map->extent[0].count == 1) && + uid_eq(ns->owner, cred->euid)) { u32 id = new_map->extent[0].lower_first; if (cap_setid == CAP_SETUID) { kuid_t uid = make_kuid(ns->parent, id); - if (uid_eq(uid, file->f_cred->fsuid)) + if (uid_eq(uid, cred->euid)) return true; } else if (cap_setid == CAP_SETGID) { kgid_t gid = make_kgid(ns->parent, id); - if (gid_eq(gid, file->f_cred->fsgid)) + if (gid_eq(gid, cred->egid) && + (cred->group_info->ngroups == 0)) return true; } } -- 1.9.1 ^ permalink raw reply related [flat|nested] 73+ messages in thread
* [CFT][PATCH v2] userns: Avoid problems with negative groups @ 2014-11-28 5:22 ` Eric W. Biederman 0 siblings, 0 replies; 73+ messages in thread From: Eric W. Biederman @ 2014-11-28 5:22 UTC (permalink / raw) To: Andy Lutomirski Cc: Linux Containers, Josh Triplett, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel, LSM, Casey Schaufler, Serge E. Hallyn, Richard Weinberger Classic unix permission checks have an interesting feature. The group permissions for a file can be set to less than the other permissions on a file. Occassionally this is used deliberately to give a certain group of users fewer permissions than the default. Overlooking negative groups has resulted in the permission checks for setting up a group mapping in a user namespace to be too lax. Tighten the permission checks in new_idmap_permitted to ensure that mapping uids and gids into user namespaces without privilege will not result in new combinations of credentials being available to the users. When setting mappings without privilege only the creator of the user namespace is interesting as all other users that have CAP_SETUID over the user namespace will also have CAP_SETUID over the user namespaces parent. So the scope of the unprivileged check is reduced to just the case where cred->euid is the namespace creator. For setting a uid mapping without privilege only euid is considered as setresuid can set uid, suid and fsuid from euid without privielege making any combination of uids possible with user namespaces already possible without them. For setting a gid mapping without privilege only egid on a credential without supplementary groups is condsidered, as setresgid can set gid, sgid and fsgid from egid without privilege. The requirement for no supplementary groups is because CAP_SETUID in a user namespace allows supplementary groups to be cleared, which unfortunately means allowing a credential with supplementary groups would allow new combinations of credentials to exist, and thus would allow defeating negative groups without permission. setgroups is modified to fail not only when the group ids do not map but also when there are no gid mappings at all, preventing setgroups(0, NULL) from succeeding when gid mappings have not been established. This change should break userspace by the minimal amount needed to fix this issue. This should fix CVE-2014-8989. Cc: stable@vger.kernel.org Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- include/linux/user_namespace.h | 10 ++++++++++ kernel/groups.c | 7 ++++++- kernel/uid16.c | 5 ++++- kernel/user_namespace.c | 16 ++++++++++++---- 4 files changed, 32 insertions(+), 6 deletions(-) diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h index e95372654f09..26d5e8f5db97 100644 --- a/include/linux/user_namespace.h +++ b/include/linux/user_namespace.h @@ -46,6 +46,11 @@ static inline struct user_namespace *get_user_ns(struct user_namespace *ns) return ns; } +static inline bool gid_mapping_possible(const struct user_namespace *ns) +{ + return ns->gid_map.nr_extents != 0; +} + extern int create_user_ns(struct cred *new); extern int unshare_userns(unsigned long unshare_flags, struct cred **new_cred); extern void free_user_ns(struct user_namespace *ns); @@ -70,6 +75,11 @@ static inline struct user_namespace *get_user_ns(struct user_namespace *ns) return &init_user_ns; } +static inline bool gid_mapping_possible(const struct user_namespace *ns) +{ + return true; +} + static inline int create_user_ns(struct cred *new) { return -EINVAL; diff --git a/kernel/groups.c b/kernel/groups.c index 451698f86cfa..302aa415158f 100644 --- a/kernel/groups.c +++ b/kernel/groups.c @@ -6,6 +6,7 @@ #include <linux/slab.h> #include <linux/security.h> #include <linux/syscalls.h> +#include <linux/user_namespace.h> #include <asm/uaccess.h> /* init to 2 - one for init_task, one to ensure it is never freed */ @@ -220,14 +221,18 @@ out: SYSCALL_DEFINE2(setgroups, int, gidsetsize, gid_t __user *, grouplist) { + struct user_namespace *user_ns = current_user_ns(); struct group_info *group_info; int retval; - if (!ns_capable(current_user_ns(), CAP_SETGID)) + if (!gid_mapping_possible(user_ns) || + !ns_capable(user_ns, CAP_SETGID)) return -EPERM; if ((unsigned)gidsetsize > NGROUPS_MAX) return -EINVAL; + /* How do I alloc a negative gidsetsize??? */ + group_info = groups_alloc(gidsetsize); if (!group_info) return -ENOMEM; diff --git a/kernel/uid16.c b/kernel/uid16.c index 602e5bbbceff..602c7de2aa11 100644 --- a/kernel/uid16.c +++ b/kernel/uid16.c @@ -13,6 +13,7 @@ #include <linux/highuid.h> #include <linux/security.h> #include <linux/syscalls.h> +#include <linux/user_namespace.h> #include <asm/uaccess.h> @@ -173,10 +174,12 @@ out: SYSCALL_DEFINE2(setgroups16, int, gidsetsize, old_gid_t __user *, grouplist) { + struct user_namespace *user_ns = current_user_ns(); struct group_info *group_info; int retval; - if (!ns_capable(current_user_ns(), CAP_SETGID)) + if (!gid_mapping_possible(user_ns) || + !ns_capable(user_ns, CAP_SETGID)) return -EPERM; if ((unsigned)gidsetsize > NGROUPS_MAX) return -EINVAL; diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c index aa312b0dc3ec..b338c42d04fd 100644 --- a/kernel/user_namespace.c +++ b/kernel/user_namespace.c @@ -812,16 +812,24 @@ static bool new_idmap_permitted(const struct file *file, struct user_namespace *ns, int cap_setid, struct uid_gid_map *new_map) { - /* Allow mapping to your own filesystem ids */ - if ((new_map->nr_extents == 1) && (new_map->extent[0].count == 1)) { + const struct cred *cred = file->f_cred; + + /* Allow mapping an id without CAP_SETUID and CAP_SETGID when + * allowing the root of the user namespace CAP_SETUID or + * CAP_SETGID restricted to just that id will not change the + * set of credentials available that user. + */ + if ((new_map->nr_extents == 1) && (new_map->extent[0].count == 1) && + uid_eq(ns->owner, cred->euid)) { u32 id = new_map->extent[0].lower_first; if (cap_setid == CAP_SETUID) { kuid_t uid = make_kuid(ns->parent, id); - if (uid_eq(uid, file->f_cred->fsuid)) + if (uid_eq(uid, cred->euid)) return true; } else if (cap_setid == CAP_SETGID) { kgid_t gid = make_kgid(ns->parent, id); - if (gid_eq(gid, file->f_cred->fsgid)) + if (gid_eq(gid, cred->egid) && + (cred->group_info->ngroups == 0)) return true; } } -- 1.9.1 ^ permalink raw reply related [flat|nested] 73+ messages in thread
* Re: [CFT][PATCH] userns: Avoid problems with negative groups [not found] ` <87wq6frjcw.fsf-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org> 2014-11-28 5:22 ` Eric W. Biederman @ 2014-11-28 15:11 ` Andy Lutomirski 1 sibling, 0 replies; 73+ messages in thread From: Andy Lutomirski @ 2014-11-28 15:11 UTC (permalink / raw) To: Eric W. Biederman Cc: linux-man, Kees Cook, Linux API, Linux Containers, Josh Triplett, linux-kernel-u79uwXL29TY76Z2rM5mHXA, LSM, Michael Kerrisk-manpages, Richard Weinberger, Casey Schaufler, Andrew Morton On Thu, Nov 27, 2014 at 9:21 PM, Eric W. Biederman <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> wrote: > Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> writes: > >>> This change should break userspace by the minimal amount needed >>> to fix this issue. >>> >>> This should fix CVE-2014-8989. >> >> I think this is both unnecessarily restrictive and that it doesn't fix >> the bug. > > You are going to have to work very hard to convince me this is > unnecessarily restrictive. > >>For example, I can exploit CVE-2014-8989 without ever >> writing a uid map or a gid map. > > Yes. I realized just after I sent the patch that setgroups(0, NULL) > would still work without a mapping set. That is a first glass grade a > oversight that resulted in a bug. None of the other uid or gid changing > syscalls without a mapping set, and setgroups was just overlooked > because it was different. Oops. > > I will send an updated patch that stops setgroups from working without > a mapping set shortly. > >> IIUC, the only real issue is that user namespaces allow groups to be >> dropped using setgroups that wouldn't otherwise be dropped. Can we >> get away with adding a per-user-ns flag that determines whether >> setgroups can be used? > > Being able to call setgroups is fundamental to login programs, and login > programs are one of the things user namespaces need to support. So > adding an extra flag and an extra place where privilege is required > is just noise, and will wind up breaking every user of user namespaces. > > Further being able to setup uid and gid mappings without privilege is > primarily a nice to have. The original design did not have unprivileged > setting of uid and gid maps and if it proves insecure I goofed and the > feature isn't safe so it needs to be removed. Being able to set a single-user uid map and gid map is very useful for sandboxing. This lets unprivileged users drop filesystem and network access and still run most normal programs. A surprising number of normal unprivileged programs fail if run without a mapping. > > This does mean that running a system with negative groups and users > delegated subordinate gids in /etc/subuid is a bad idea and system > administrators shouldn't do that as those negative groups won't prove > effective in stopping their users. But this is all under system > administrator control so shrug. There isn't a way to avoid that > fundamental conflict. > >> setgroups would be unusable until the gid_map has been written and >> then it would become usable if and only if the parent userns could use >> setgroups and the opener of gid_map was privileged. > > That proposal sounds a lot more restrictive and a lot more of a pain > to use than what I have implemented in my patch. > >> If we wanted to allow finer-grained control, we could allow writing >> control lines like: >> >> options +setgroups >> >> or >> >> options -setgroups >> >> in gid_map, or we could add user_ns_flags that can only be written >> once and only before either uid_map or gid_map is written. > > Definitely more complicated and I can't imagine a case where I need > a gid map without needing to call setgroups. I do it all the time. Unshare, set mappings (with no inner uid 0 at all), set no_new_privs, drop caps, and go. Can we try the intermediate approach? If you set gid_map without privilege and you have supplementary groups, then let the write to gid_map succeed but prevent setgroups from ever working? That should only be a couple of lines of code longer than your patch, and it will avoid breaking sandbox use cases. If we want to get really fancy in the future, we could have a concept of pinned groups. That is, if you're in a userns and you're a member of an unmapped group, then you can't drop that group. (Actually, that all by itself might be enough to fix this issue.) --Andy > > Eric -- Andy Lutomirski AMA Capital Management, LLC ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [CFT][PATCH] userns: Avoid problems with negative groups [not found] ` <87wq6frjcw.fsf-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org> @ 2014-11-28 15:11 ` Andy Lutomirski 2014-11-28 15:11 ` [CFT][PATCH] " Andy Lutomirski 1 sibling, 0 replies; 73+ messages in thread From: Andy Lutomirski @ 2014-11-28 15:11 UTC (permalink / raw) To: Eric W. Biederman Cc: Linux Containers, Josh Triplett, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel, LSM, Casey Schaufler, Serge E. Hallyn, Richard Weinberger On Thu, Nov 27, 2014 at 9:21 PM, Eric W. Biederman <ebiederm@xmission.com> wrote: > Andy Lutomirski <luto@amacapital.net> writes: > >>> This change should break userspace by the minimal amount needed >>> to fix this issue. >>> >>> This should fix CVE-2014-8989. >> >> I think this is both unnecessarily restrictive and that it doesn't fix >> the bug. > > You are going to have to work very hard to convince me this is > unnecessarily restrictive. > >>For example, I can exploit CVE-2014-8989 without ever >> writing a uid map or a gid map. > > Yes. I realized just after I sent the patch that setgroups(0, NULL) > would still work without a mapping set. That is a first glass grade a > oversight that resulted in a bug. None of the other uid or gid changing > syscalls without a mapping set, and setgroups was just overlooked > because it was different. Oops. > > I will send an updated patch that stops setgroups from working without > a mapping set shortly. > >> IIUC, the only real issue is that user namespaces allow groups to be >> dropped using setgroups that wouldn't otherwise be dropped. Can we >> get away with adding a per-user-ns flag that determines whether >> setgroups can be used? > > Being able to call setgroups is fundamental to login programs, and login > programs are one of the things user namespaces need to support. So > adding an extra flag and an extra place where privilege is required > is just noise, and will wind up breaking every user of user namespaces. > > Further being able to setup uid and gid mappings without privilege is > primarily a nice to have. The original design did not have unprivileged > setting of uid and gid maps and if it proves insecure I goofed and the > feature isn't safe so it needs to be removed. Being able to set a single-user uid map and gid map is very useful for sandboxing. This lets unprivileged users drop filesystem and network access and still run most normal programs. A surprising number of normal unprivileged programs fail if run without a mapping. > > This does mean that running a system with negative groups and users > delegated subordinate gids in /etc/subuid is a bad idea and system > administrators shouldn't do that as those negative groups won't prove > effective in stopping their users. But this is all under system > administrator control so shrug. There isn't a way to avoid that > fundamental conflict. > >> setgroups would be unusable until the gid_map has been written and >> then it would become usable if and only if the parent userns could use >> setgroups and the opener of gid_map was privileged. > > That proposal sounds a lot more restrictive and a lot more of a pain > to use than what I have implemented in my patch. > >> If we wanted to allow finer-grained control, we could allow writing >> control lines like: >> >> options +setgroups >> >> or >> >> options -setgroups >> >> in gid_map, or we could add user_ns_flags that can only be written >> once and only before either uid_map or gid_map is written. > > Definitely more complicated and I can't imagine a case where I need > a gid map without needing to call setgroups. I do it all the time. Unshare, set mappings (with no inner uid 0 at all), set no_new_privs, drop caps, and go. Can we try the intermediate approach? If you set gid_map without privilege and you have supplementary groups, then let the write to gid_map succeed but prevent setgroups from ever working? That should only be a couple of lines of code longer than your patch, and it will avoid breaking sandbox use cases. If we want to get really fancy in the future, we could have a concept of pinned groups. That is, if you're in a userns and you're a member of an unmapped group, then you can't drop that group. (Actually, that all by itself might be enough to fix this issue.) --Andy > > Eric -- Andy Lutomirski AMA Capital Management, LLC ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [CFT][PATCH] userns: Avoid problems with negative groups @ 2014-11-28 15:11 ` Andy Lutomirski 0 siblings, 0 replies; 73+ messages in thread From: Andy Lutomirski @ 2014-11-28 15:11 UTC (permalink / raw) To: Eric W. Biederman Cc: Linux Containers, Josh Triplett, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel-u79uwXL29TY76Z2rM5mHXA, LSM, Casey Schaufler, Serge E. Hallyn, Richard Weinberger On Thu, Nov 27, 2014 at 9:21 PM, Eric W. Biederman <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> wrote: > Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> writes: > >>> This change should break userspace by the minimal amount needed >>> to fix this issue. >>> >>> This should fix CVE-2014-8989. >> >> I think this is both unnecessarily restrictive and that it doesn't fix >> the bug. > > You are going to have to work very hard to convince me this is > unnecessarily restrictive. > >>For example, I can exploit CVE-2014-8989 without ever >> writing a uid map or a gid map. > > Yes. I realized just after I sent the patch that setgroups(0, NULL) > would still work without a mapping set. That is a first glass grade a > oversight that resulted in a bug. None of the other uid or gid changing > syscalls without a mapping set, and setgroups was just overlooked > because it was different. Oops. > > I will send an updated patch that stops setgroups from working without > a mapping set shortly. > >> IIUC, the only real issue is that user namespaces allow groups to be >> dropped using setgroups that wouldn't otherwise be dropped. Can we >> get away with adding a per-user-ns flag that determines whether >> setgroups can be used? > > Being able to call setgroups is fundamental to login programs, and login > programs are one of the things user namespaces need to support. So > adding an extra flag and an extra place where privilege is required > is just noise, and will wind up breaking every user of user namespaces. > > Further being able to setup uid and gid mappings without privilege is > primarily a nice to have. The original design did not have unprivileged > setting of uid and gid maps and if it proves insecure I goofed and the > feature isn't safe so it needs to be removed. Being able to set a single-user uid map and gid map is very useful for sandboxing. This lets unprivileged users drop filesystem and network access and still run most normal programs. A surprising number of normal unprivileged programs fail if run without a mapping. > > This does mean that running a system with negative groups and users > delegated subordinate gids in /etc/subuid is a bad idea and system > administrators shouldn't do that as those negative groups won't prove > effective in stopping their users. But this is all under system > administrator control so shrug. There isn't a way to avoid that > fundamental conflict. > >> setgroups would be unusable until the gid_map has been written and >> then it would become usable if and only if the parent userns could use >> setgroups and the opener of gid_map was privileged. > > That proposal sounds a lot more restrictive and a lot more of a pain > to use than what I have implemented in my patch. > >> If we wanted to allow finer-grained control, we could allow writing >> control lines like: >> >> options +setgroups >> >> or >> >> options -setgroups >> >> in gid_map, or we could add user_ns_flags that can only be written >> once and only before either uid_map or gid_map is written. > > Definitely more complicated and I can't imagine a case where I need > a gid map without needing to call setgroups. I do it all the time. Unshare, set mappings (with no inner uid 0 at all), set no_new_privs, drop caps, and go. Can we try the intermediate approach? If you set gid_map without privilege and you have supplementary groups, then let the write to gid_map succeed but prevent setgroups from ever working? That should only be a couple of lines of code longer than your patch, and it will avoid breaking sandbox use cases. If we want to get really fancy in the future, we could have a concept of pinned groups. That is, if you're in a userns and you're a member of an unmapped group, then you can't drop that group. (Actually, that all by itself might be enough to fix this issue.) --Andy > > Eric -- Andy Lutomirski AMA Capital Management, LLC -- To unsubscribe from this list: send the line "unsubscribe linux-man" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 73+ messages in thread
[parent not found: <CALCETrX2s-7iaLMEKLQsExTEp3JyoAPQG44p0v5wkeED3-6dQA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [CFT][PATCH] userns: Avoid problems with negative groups [not found] ` <CALCETrX2s-7iaLMEKLQsExTEp3JyoAPQG44p0v5wkeED3-6dQA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2014-11-28 16:34 ` Eric W. Biederman 0 siblings, 0 replies; 73+ messages in thread From: Eric W. Biederman @ 2014-11-28 16:34 UTC (permalink / raw) To: Andy Lutomirski Cc: linux-man, Kees Cook, Linux API, Linux Containers, Josh Triplett, linux-kernel-u79uwXL29TY76Z2rM5mHXA, LSM, Michael Kerrisk-manpages, Richard Weinberger, Casey Schaufler, Andrew Morton Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> writes: > On Thu, Nov 27, 2014 at 9:21 PM, Eric W. Biederman > <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> wrote: >> Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> writes: >> >>>> This change should break userspace by the minimal amount needed >>>> to fix this issue. >>>> >>>> This should fix CVE-2014-8989. >>> >>> I think this is both unnecessarily restrictive and that it doesn't fix >>> the bug. >> >> You are going to have to work very hard to convince me this is >> unnecessarily restrictive. >> >>>For example, I can exploit CVE-2014-8989 without ever >>> writing a uid map or a gid map. >> >> Yes. I realized just after I sent the patch that setgroups(0, NULL) >> would still work without a mapping set. That is a first glass grade a >> oversight that resulted in a bug. None of the other uid or gid changing >> syscalls without a mapping set, and setgroups was just overlooked >> because it was different. Oops. >> >> I will send an updated patch that stops setgroups from working without >> a mapping set shortly. >> >>> IIUC, the only real issue is that user namespaces allow groups to be >>> dropped using setgroups that wouldn't otherwise be dropped. Can we >>> get away with adding a per-user-ns flag that determines whether >>> setgroups can be used? >> >> Being able to call setgroups is fundamental to login programs, and login >> programs are one of the things user namespaces need to support. So >> adding an extra flag and an extra place where privilege is required >> is just noise, and will wind up breaking every user of user namespaces. >> >> Further being able to setup uid and gid mappings without privilege is >> primarily a nice to have. The original design did not have unprivileged >> setting of uid and gid maps and if it proves insecure I goofed and the >> feature isn't safe so it needs to be removed. > > Being able to set a single-user uid map and gid map is very useful for > sandboxing. This lets unprivileged users drop filesystem and network > access and still run most normal programs. A surprising number of > normal unprivileged programs fail if run without a mapping. You can still set a single uid map unprivileged. That should be enough to keep your capabilities inside the namespace as long as you need them. I am sad that in practice you can't set a single gid map, as everyone calls setgroups. Although I sort of think it might be worth scouring userspace for something that will call setgroups and drop all of your groups. If we can find something preexisting that will solve this entire mess in a much more elegant way. >> This does mean that running a system with negative groups and users >> delegated subordinate gids in /etc/subuid is a bad idea and system >> administrators shouldn't do that as those negative groups won't prove >> effective in stopping their users. But this is all under system >> administrator control so shrug. There isn't a way to avoid that >> fundamental conflict. >> >>> setgroups would be unusable until the gid_map has been written and >>> then it would become usable if and only if the parent userns could use >>> setgroups and the opener of gid_map was privileged. >> >> That proposal sounds a lot more restrictive and a lot more of a pain >> to use than what I have implemented in my patch. >> >>> If we wanted to allow finer-grained control, we could allow writing >>> control lines like: >>> >>> options +setgroups >>> >>> or >>> >>> options -setgroups >>> >>> in gid_map, or we could add user_ns_flags that can only be written >>> once and only before either uid_map or gid_map is written. >> >> Definitely more complicated and I can't imagine a case where I need >> a gid map without needing to call setgroups. > > I do it all the time. Unshare, set mappings (with no inner uid 0 at > all), set no_new_privs, drop caps, and go. > > Can we try the intermediate approach? If you set gid_map without > privilege and you have supplementary groups, then let the write to > gid_map succeed but prevent setgroups from ever working? That should > only be a couple of lines of code longer than your patch, and it will > avoid breaking sandbox use cases. I am torn. Send me an incremental patch and I will be happy to evaluate it and if all is good fold the change in. I hate breaking userspace but if I break it I would rather it be a clean break that is easy to spot and work around rather than something that almost works, and causes people a lot of difficulty debugging. My expectation is that systems that are serious will have /etc/subuid and /etc/subgid and newuidmap and newgidmap setup. Which is the other way to allow you to map your own gid. > If we want to get really fancy in the future, we could have a concept > of pinned groups. That is, if you're in a userns and you're a member > of an unmapped group, then you can't drop that group. (Actually, that > all by itself might be enough to fix this issue.) Not allowing you to drop groups really isn't enough. One of the first things applications like ssh do is call setgroups(0, NULL) to drop the privileges granted by supplementary groups. Further login program somewhere call setgroups(N, ....) and give you every group mapped by /etc/group. I don't think I want to run containers with every supplementary group I might want to drop mapped, and more than that, it would require changing a lot more userspace than just the userspace that just does unpriv containers with a single uid, and a single gid mapped. But please test and see if you really need to map your group, and send me an incremental patch if you see a way to do better. Breaking userspace sucks. Eric ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [CFT][PATCH] userns: Avoid problems with negative groups [not found] ` <CALCETrX2s-7iaLMEKLQsExTEp3JyoAPQG44p0v5wkeED3-6dQA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2014-11-28 16:34 ` Eric W. Biederman 0 siblings, 0 replies; 73+ messages in thread From: Eric W. Biederman @ 2014-11-28 16:34 UTC (permalink / raw) To: Andy Lutomirski Cc: Linux Containers, Josh Triplett, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel, LSM, Casey Schaufler, Serge E. Hallyn, Richard Weinberger Andy Lutomirski <luto@amacapital.net> writes: > On Thu, Nov 27, 2014 at 9:21 PM, Eric W. Biederman > <ebiederm@xmission.com> wrote: >> Andy Lutomirski <luto@amacapital.net> writes: >> >>>> This change should break userspace by the minimal amount needed >>>> to fix this issue. >>>> >>>> This should fix CVE-2014-8989. >>> >>> I think this is both unnecessarily restrictive and that it doesn't fix >>> the bug. >> >> You are going to have to work very hard to convince me this is >> unnecessarily restrictive. >> >>>For example, I can exploit CVE-2014-8989 without ever >>> writing a uid map or a gid map. >> >> Yes. I realized just after I sent the patch that setgroups(0, NULL) >> would still work without a mapping set. That is a first glass grade a >> oversight that resulted in a bug. None of the other uid or gid changing >> syscalls without a mapping set, and setgroups was just overlooked >> because it was different. Oops. >> >> I will send an updated patch that stops setgroups from working without >> a mapping set shortly. >> >>> IIUC, the only real issue is that user namespaces allow groups to be >>> dropped using setgroups that wouldn't otherwise be dropped. Can we >>> get away with adding a per-user-ns flag that determines whether >>> setgroups can be used? >> >> Being able to call setgroups is fundamental to login programs, and login >> programs are one of the things user namespaces need to support. So >> adding an extra flag and an extra place where privilege is required >> is just noise, and will wind up breaking every user of user namespaces. >> >> Further being able to setup uid and gid mappings without privilege is >> primarily a nice to have. The original design did not have unprivileged >> setting of uid and gid maps and if it proves insecure I goofed and the >> feature isn't safe so it needs to be removed. > > Being able to set a single-user uid map and gid map is very useful for > sandboxing. This lets unprivileged users drop filesystem and network > access and still run most normal programs. A surprising number of > normal unprivileged programs fail if run without a mapping. You can still set a single uid map unprivileged. That should be enough to keep your capabilities inside the namespace as long as you need them. I am sad that in practice you can't set a single gid map, as everyone calls setgroups. Although I sort of think it might be worth scouring userspace for something that will call setgroups and drop all of your groups. If we can find something preexisting that will solve this entire mess in a much more elegant way. >> This does mean that running a system with negative groups and users >> delegated subordinate gids in /etc/subuid is a bad idea and system >> administrators shouldn't do that as those negative groups won't prove >> effective in stopping their users. But this is all under system >> administrator control so shrug. There isn't a way to avoid that >> fundamental conflict. >> >>> setgroups would be unusable until the gid_map has been written and >>> then it would become usable if and only if the parent userns could use >>> setgroups and the opener of gid_map was privileged. >> >> That proposal sounds a lot more restrictive and a lot more of a pain >> to use than what I have implemented in my patch. >> >>> If we wanted to allow finer-grained control, we could allow writing >>> control lines like: >>> >>> options +setgroups >>> >>> or >>> >>> options -setgroups >>> >>> in gid_map, or we could add user_ns_flags that can only be written >>> once and only before either uid_map or gid_map is written. >> >> Definitely more complicated and I can't imagine a case where I need >> a gid map without needing to call setgroups. > > I do it all the time. Unshare, set mappings (with no inner uid 0 at > all), set no_new_privs, drop caps, and go. > > Can we try the intermediate approach? If you set gid_map without > privilege and you have supplementary groups, then let the write to > gid_map succeed but prevent setgroups from ever working? That should > only be a couple of lines of code longer than your patch, and it will > avoid breaking sandbox use cases. I am torn. Send me an incremental patch and I will be happy to evaluate it and if all is good fold the change in. I hate breaking userspace but if I break it I would rather it be a clean break that is easy to spot and work around rather than something that almost works, and causes people a lot of difficulty debugging. My expectation is that systems that are serious will have /etc/subuid and /etc/subgid and newuidmap and newgidmap setup. Which is the other way to allow you to map your own gid. > If we want to get really fancy in the future, we could have a concept > of pinned groups. That is, if you're in a userns and you're a member > of an unmapped group, then you can't drop that group. (Actually, that > all by itself might be enough to fix this issue.) Not allowing you to drop groups really isn't enough. One of the first things applications like ssh do is call setgroups(0, NULL) to drop the privileges granted by supplementary groups. Further login program somewhere call setgroups(N, ....) and give you every group mapped by /etc/group. I don't think I want to run containers with every supplementary group I might want to drop mapped, and more than that, it would require changing a lot more userspace than just the userspace that just does unpriv containers with a single uid, and a single gid mapped. But please test and see if you really need to map your group, and send me an incremental patch if you see a way to do better. Breaking userspace sucks. Eric ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [CFT][PATCH] userns: Avoid problems with negative groups @ 2014-11-28 16:34 ` Eric W. Biederman 0 siblings, 0 replies; 73+ messages in thread From: Eric W. Biederman @ 2014-11-28 16:34 UTC (permalink / raw) To: Andy Lutomirski Cc: Linux Containers, Josh Triplett, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel@vger.kernel.org, LSM, Casey Schaufler, Serge E. Hallyn, Richard Weinberger Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> writes: > On Thu, Nov 27, 2014 at 9:21 PM, Eric W. Biederman > <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> wrote: >> Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> writes: >> >>>> This change should break userspace by the minimal amount needed >>>> to fix this issue. >>>> >>>> This should fix CVE-2014-8989. >>> >>> I think this is both unnecessarily restrictive and that it doesn't fix >>> the bug. >> >> You are going to have to work very hard to convince me this is >> unnecessarily restrictive. >> >>>For example, I can exploit CVE-2014-8989 without ever >>> writing a uid map or a gid map. >> >> Yes. I realized just after I sent the patch that setgroups(0, NULL) >> would still work without a mapping set. That is a first glass grade a >> oversight that resulted in a bug. None of the other uid or gid changing >> syscalls without a mapping set, and setgroups was just overlooked >> because it was different. Oops. >> >> I will send an updated patch that stops setgroups from working without >> a mapping set shortly. >> >>> IIUC, the only real issue is that user namespaces allow groups to be >>> dropped using setgroups that wouldn't otherwise be dropped. Can we >>> get away with adding a per-user-ns flag that determines whether >>> setgroups can be used? >> >> Being able to call setgroups is fundamental to login programs, and login >> programs are one of the things user namespaces need to support. So >> adding an extra flag and an extra place where privilege is required >> is just noise, and will wind up breaking every user of user namespaces. >> >> Further being able to setup uid and gid mappings without privilege is >> primarily a nice to have. The original design did not have unprivileged >> setting of uid and gid maps and if it proves insecure I goofed and the >> feature isn't safe so it needs to be removed. > > Being able to set a single-user uid map and gid map is very useful for > sandboxing. This lets unprivileged users drop filesystem and network > access and still run most normal programs. A surprising number of > normal unprivileged programs fail if run without a mapping. You can still set a single uid map unprivileged. That should be enough to keep your capabilities inside the namespace as long as you need them. I am sad that in practice you can't set a single gid map, as everyone calls setgroups. Although I sort of think it might be worth scouring userspace for something that will call setgroups and drop all of your groups. If we can find something preexisting that will solve this entire mess in a much more elegant way. >> This does mean that running a system with negative groups and users >> delegated subordinate gids in /etc/subuid is a bad idea and system >> administrators shouldn't do that as those negative groups won't prove >> effective in stopping their users. But this is all under system >> administrator control so shrug. There isn't a way to avoid that >> fundamental conflict. >> >>> setgroups would be unusable until the gid_map has been written and >>> then it would become usable if and only if the parent userns could use >>> setgroups and the opener of gid_map was privileged. >> >> That proposal sounds a lot more restrictive and a lot more of a pain >> to use than what I have implemented in my patch. >> >>> If we wanted to allow finer-grained control, we could allow writing >>> control lines like: >>> >>> options +setgroups >>> >>> or >>> >>> options -setgroups >>> >>> in gid_map, or we could add user_ns_flags that can only be written >>> once and only before either uid_map or gid_map is written. >> >> Definitely more complicated and I can't imagine a case where I need >> a gid map without needing to call setgroups. > > I do it all the time. Unshare, set mappings (with no inner uid 0 at > all), set no_new_privs, drop caps, and go. > > Can we try the intermediate approach? If you set gid_map without > privilege and you have supplementary groups, then let the write to > gid_map succeed but prevent setgroups from ever working? That should > only be a couple of lines of code longer than your patch, and it will > avoid breaking sandbox use cases. I am torn. Send me an incremental patch and I will be happy to evaluate it and if all is good fold the change in. I hate breaking userspace but if I break it I would rather it be a clean break that is easy to spot and work around rather than something that almost works, and causes people a lot of difficulty debugging. My expectation is that systems that are serious will have /etc/subuid and /etc/subgid and newuidmap and newgidmap setup. Which is the other way to allow you to map your own gid. > If we want to get really fancy in the future, we could have a concept > of pinned groups. That is, if you're in a userns and you're a member > of an unmapped group, then you can't drop that group. (Actually, that > all by itself might be enough to fix this issue.) Not allowing you to drop groups really isn't enough. One of the first things applications like ssh do is call setgroups(0, NULL) to drop the privileges granted by supplementary groups. Further login program somewhere call setgroups(N, ....) and give you every group mapped by /etc/group. I don't think I want to run containers with every supplementary group I might want to drop mapped, and more than that, it would require changing a lot more userspace than just the userspace that just does unpriv containers with a single uid, and a single gid mapped. But please test and see if you really need to map your group, and send me an incremental patch if you see a way to do better. Breaking userspace sucks. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-man" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 73+ messages in thread
[parent not found: <874mtjp9m1.fsf-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org>]
* Re: [CFT][PATCH] userns: Avoid problems with negative groups 2014-11-28 16:34 ` Eric W. Biederman @ 2014-11-28 17:11 ` Andy Lutomirski -1 siblings, 0 replies; 73+ messages in thread From: Andy Lutomirski @ 2014-11-28 17:11 UTC (permalink / raw) To: Eric W. Biederman Cc: linux-man, Kees Cook, Linux API, Linux Containers, Serge Hallyn, Josh Triplett, linux-kernel-u79uwXL29TY76Z2rM5mHXA, LSM, Michael Kerrisk-manpages, Richard Weinberger, Casey Schaufler, Andrew Morton cc: Serge and Stephane, since this may end up affecting LXC. On Fri, Nov 28, 2014 at 8:34 AM, Eric W. Biederman <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> wrote: > Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> writes: > >> On Thu, Nov 27, 2014 at 9:21 PM, Eric W. Biederman >> <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> wrote: >>> Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> writes: >>> >>>>> This change should break userspace by the minimal amount needed >>>>> to fix this issue. >>>>> >>>>> This should fix CVE-2014-8989. >>>> >>>> I think this is both unnecessarily restrictive and that it doesn't fix >>>> the bug. >>> >>> You are going to have to work very hard to convince me this is >>> unnecessarily restrictive. >>> >>>>For example, I can exploit CVE-2014-8989 without ever >>>> writing a uid map or a gid map. >>> >>> Yes. I realized just after I sent the patch that setgroups(0, NULL) >>> would still work without a mapping set. That is a first glass grade a >>> oversight that resulted in a bug. None of the other uid or gid changing >>> syscalls without a mapping set, and setgroups was just overlooked >>> because it was different. Oops. >>> >>> I will send an updated patch that stops setgroups from working without >>> a mapping set shortly. >>> >>>> IIUC, the only real issue is that user namespaces allow groups to be >>>> dropped using setgroups that wouldn't otherwise be dropped. Can we >>>> get away with adding a per-user-ns flag that determines whether >>>> setgroups can be used? >>> >>> Being able to call setgroups is fundamental to login programs, and login >>> programs are one of the things user namespaces need to support. So >>> adding an extra flag and an extra place where privilege is required >>> is just noise, and will wind up breaking every user of user namespaces. >>> >>> Further being able to setup uid and gid mappings without privilege is >>> primarily a nice to have. The original design did not have unprivileged >>> setting of uid and gid maps and if it proves insecure I goofed and the >>> feature isn't safe so it needs to be removed. >> >> Being able to set a single-user uid map and gid map is very useful for >> sandboxing. This lets unprivileged users drop filesystem and network >> access and still run most normal programs. A surprising number of >> normal unprivileged programs fail if run without a mapping. > > You can still set a single uid map unprivileged. That should be enough > to keep your capabilities inside the namespace as long as you need them. > > I am sad that in practice you can't set a single gid map, as everyone > calls setgroups. > That's not the problem. The problem is that a surprising number of libraries expect getegid(), etc to return sensible values. > Although I sort of think it might be worth scouring userspace for > something that will call setgroups and drop all of your groups. If we > can find something preexisting that will solve this entire mess in a > much more elegant way. > >>> This does mean that running a system with negative groups and users >>> delegated subordinate gids in /etc/subuid is a bad idea and system >>> administrators shouldn't do that as those negative groups won't prove >>> effective in stopping their users. But this is all under system >>> administrator control so shrug. There isn't a way to avoid that >>> fundamental conflict. >>> >>>> setgroups would be unusable until the gid_map has been written and >>>> then it would become usable if and only if the parent userns could use >>>> setgroups and the opener of gid_map was privileged. >>> >>> That proposal sounds a lot more restrictive and a lot more of a pain >>> to use than what I have implemented in my patch. >>> >>>> If we wanted to allow finer-grained control, we could allow writing >>>> control lines like: >>>> >>>> options +setgroups >>>> >>>> or >>>> >>>> options -setgroups >>>> >>>> in gid_map, or we could add user_ns_flags that can only be written >>>> once and only before either uid_map or gid_map is written. >>> >>> Definitely more complicated and I can't imagine a case where I need >>> a gid map without needing to call setgroups. >> >> I do it all the time. Unshare, set mappings (with no inner uid 0 at >> all), set no_new_privs, drop caps, and go. >> >> Can we try the intermediate approach? If you set gid_map without >> privilege and you have supplementary groups, then let the write to >> gid_map succeed but prevent setgroups from ever working? That should >> only be a couple of lines of code longer than your patch, and it will >> avoid breaking sandbox use cases. > > I am torn. Send me an incremental patch and I will be happy to evaluate > it and if all is good fold the change in. I hate breaking userspace but > if I break it I would rather it be a clean break that is easy to spot > and work around rather than something that almost works, and causes > people a lot of difficulty debugging. I'll play with it this afternoon or over the weekend. I'm currently on vacation, so I'll be a little slow. > > My expectation is that systems that are serious will have /etc/subuid > and /etc/subgid and newuidmap and newgidmap setup. Which is the other > way to allow you to map your own gid. > >> If we want to get really fancy in the future, we could have a concept >> of pinned groups. That is, if you're in a userns and you're a member >> of an unmapped group, then you can't drop that group. (Actually, that >> all by itself might be enough to fix this issue.) > > Not allowing you to drop groups really isn't enough. One of the first > things applications like ssh do is call setgroups(0, NULL) to drop the > privileges granted by supplementary groups. Further login program > somewhere call setgroups(N, ....) and give you every group mapped > by /etc/group. > > I don't think I want to run containers with every supplementary group I > might want to drop mapped, and more than that, it would require changing > a lot more userspace than just the userspace that just does unpriv > containers with a single uid, and a single gid mapped. > > But please test and see if you really need to map your group, and send > me an incremental patch if you see a way to do better. Breaking > userspace sucks. I *know* I need the uid mapped, and I'm pretty sure I need the gid as well. FWIW, the code I care about won't object too strongly to a one-time break, but it will object to needing to use subuids, since it will have all kinds of problems if it starts to need to rely on setuid helpers. There's a third option: use your patch but require explicit userspace opt-in for code that wants the setgroups-not-allowed mode. I'll try implementing that. --Andy ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [CFT][PATCH] userns: Avoid problems with negative groups @ 2014-11-28 17:11 ` Andy Lutomirski 0 siblings, 0 replies; 73+ messages in thread From: Andy Lutomirski @ 2014-11-28 17:11 UTC (permalink / raw) To: Eric W. Biederman Cc: Linux Containers, Josh Triplett, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel, LSM, Casey Schaufler, Serge E. Hallyn, Richard Weinberger, Serge Hallyn, Stephane Graber cc: Serge and Stephane, since this may end up affecting LXC. On Fri, Nov 28, 2014 at 8:34 AM, Eric W. Biederman <ebiederm@xmission.com> wrote: > Andy Lutomirski <luto@amacapital.net> writes: > >> On Thu, Nov 27, 2014 at 9:21 PM, Eric W. Biederman >> <ebiederm@xmission.com> wrote: >>> Andy Lutomirski <luto@amacapital.net> writes: >>> >>>>> This change should break userspace by the minimal amount needed >>>>> to fix this issue. >>>>> >>>>> This should fix CVE-2014-8989. >>>> >>>> I think this is both unnecessarily restrictive and that it doesn't fix >>>> the bug. >>> >>> You are going to have to work very hard to convince me this is >>> unnecessarily restrictive. >>> >>>>For example, I can exploit CVE-2014-8989 without ever >>>> writing a uid map or a gid map. >>> >>> Yes. I realized just after I sent the patch that setgroups(0, NULL) >>> would still work without a mapping set. That is a first glass grade a >>> oversight that resulted in a bug. None of the other uid or gid changing >>> syscalls without a mapping set, and setgroups was just overlooked >>> because it was different. Oops. >>> >>> I will send an updated patch that stops setgroups from working without >>> a mapping set shortly. >>> >>>> IIUC, the only real issue is that user namespaces allow groups to be >>>> dropped using setgroups that wouldn't otherwise be dropped. Can we >>>> get away with adding a per-user-ns flag that determines whether >>>> setgroups can be used? >>> >>> Being able to call setgroups is fundamental to login programs, and login >>> programs are one of the things user namespaces need to support. So >>> adding an extra flag and an extra place where privilege is required >>> is just noise, and will wind up breaking every user of user namespaces. >>> >>> Further being able to setup uid and gid mappings without privilege is >>> primarily a nice to have. The original design did not have unprivileged >>> setting of uid and gid maps and if it proves insecure I goofed and the >>> feature isn't safe so it needs to be removed. >> >> Being able to set a single-user uid map and gid map is very useful for >> sandboxing. This lets unprivileged users drop filesystem and network >> access and still run most normal programs. A surprising number of >> normal unprivileged programs fail if run without a mapping. > > You can still set a single uid map unprivileged. That should be enough > to keep your capabilities inside the namespace as long as you need them. > > I am sad that in practice you can't set a single gid map, as everyone > calls setgroups. > That's not the problem. The problem is that a surprising number of libraries expect getegid(), etc to return sensible values. > Although I sort of think it might be worth scouring userspace for > something that will call setgroups and drop all of your groups. If we > can find something preexisting that will solve this entire mess in a > much more elegant way. > >>> This does mean that running a system with negative groups and users >>> delegated subordinate gids in /etc/subuid is a bad idea and system >>> administrators shouldn't do that as those negative groups won't prove >>> effective in stopping their users. But this is all under system >>> administrator control so shrug. There isn't a way to avoid that >>> fundamental conflict. >>> >>>> setgroups would be unusable until the gid_map has been written and >>>> then it would become usable if and only if the parent userns could use >>>> setgroups and the opener of gid_map was privileged. >>> >>> That proposal sounds a lot more restrictive and a lot more of a pain >>> to use than what I have implemented in my patch. >>> >>>> If we wanted to allow finer-grained control, we could allow writing >>>> control lines like: >>>> >>>> options +setgroups >>>> >>>> or >>>> >>>> options -setgroups >>>> >>>> in gid_map, or we could add user_ns_flags that can only be written >>>> once and only before either uid_map or gid_map is written. >>> >>> Definitely more complicated and I can't imagine a case where I need >>> a gid map without needing to call setgroups. >> >> I do it all the time. Unshare, set mappings (with no inner uid 0 at >> all), set no_new_privs, drop caps, and go. >> >> Can we try the intermediate approach? If you set gid_map without >> privilege and you have supplementary groups, then let the write to >> gid_map succeed but prevent setgroups from ever working? That should >> only be a couple of lines of code longer than your patch, and it will >> avoid breaking sandbox use cases. > > I am torn. Send me an incremental patch and I will be happy to evaluate > it and if all is good fold the change in. I hate breaking userspace but > if I break it I would rather it be a clean break that is easy to spot > and work around rather than something that almost works, and causes > people a lot of difficulty debugging. I'll play with it this afternoon or over the weekend. I'm currently on vacation, so I'll be a little slow. > > My expectation is that systems that are serious will have /etc/subuid > and /etc/subgid and newuidmap and newgidmap setup. Which is the other > way to allow you to map your own gid. > >> If we want to get really fancy in the future, we could have a concept >> of pinned groups. That is, if you're in a userns and you're a member >> of an unmapped group, then you can't drop that group. (Actually, that >> all by itself might be enough to fix this issue.) > > Not allowing you to drop groups really isn't enough. One of the first > things applications like ssh do is call setgroups(0, NULL) to drop the > privileges granted by supplementary groups. Further login program > somewhere call setgroups(N, ....) and give you every group mapped > by /etc/group. > > I don't think I want to run containers with every supplementary group I > might want to drop mapped, and more than that, it would require changing > a lot more userspace than just the userspace that just does unpriv > containers with a single uid, and a single gid mapped. > > But please test and see if you really need to map your group, and send > me an incremental patch if you see a way to do better. Breaking > userspace sucks. I *know* I need the uid mapped, and I'm pretty sure I need the gid as well. FWIW, the code I care about won't object too strongly to a one-time break, but it will object to needing to use subuids, since it will have all kinds of problems if it starts to need to rely on setuid helpers. There's a third option: use your patch but require explicit userspace opt-in for code that wants the setgroups-not-allowed mode. I'll try implementing that. --Andy ^ permalink raw reply [flat|nested] 73+ messages in thread
[parent not found: <CALCETrUuWDq2akKfb50AiPHeDDWzPW7ijz1QwnuNiskyZbBEfA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [CFT][PATCH] userns: Avoid problems with negative groups [not found] ` <CALCETrUuWDq2akKfb50AiPHeDDWzPW7ijz1QwnuNiskyZbBEfA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2014-11-28 5:21 ` Eric W. Biederman 0 siblings, 0 replies; 73+ messages in thread From: Eric W. Biederman @ 2014-11-28 5:21 UTC (permalink / raw) To: Andy Lutomirski Cc: linux-man, Kees Cook, Linux API, Linux Containers, Josh Triplett, linux-kernel-u79uwXL29TY76Z2rM5mHXA, LSM, Michael Kerrisk-manpages, Richard Weinberger, Casey Schaufler, Andrew Morton Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> writes: >> This change should break userspace by the minimal amount needed >> to fix this issue. >> >> This should fix CVE-2014-8989. > > I think this is both unnecessarily restrictive and that it doesn't fix > the bug. You are going to have to work very hard to convince me this is unnecessarily restrictive. >For example, I can exploit CVE-2014-8989 without ever > writing a uid map or a gid map. Yes. I realized just after I sent the patch that setgroups(0, NULL) would still work without a mapping set. That is a first glass grade a oversight that resulted in a bug. None of the other uid or gid changing syscalls without a mapping set, and setgroups was just overlooked because it was different. Oops. I will send an updated patch that stops setgroups from working without a mapping set shortly. > IIUC, the only real issue is that user namespaces allow groups to be > dropped using setgroups that wouldn't otherwise be dropped. Can we > get away with adding a per-user-ns flag that determines whether > setgroups can be used? Being able to call setgroups is fundamental to login programs, and login programs are one of the things user namespaces need to support. So adding an extra flag and an extra place where privilege is required is just noise, and will wind up breaking every user of user namespaces. Further being able to setup uid and gid mappings without privilege is primarily a nice to have. The original design did not have unprivileged setting of uid and gid maps and if it proves insecure I goofed and the feature isn't safe so it needs to be removed. This does mean that running a system with negative groups and users delegated subordinate gids in /etc/subuid is a bad idea and system administrators shouldn't do that as those negative groups won't prove effective in stopping their users. But this is all under system administrator control so shrug. There isn't a way to avoid that fundamental conflict. > setgroups would be unusable until the gid_map has been written and > then it would become usable if and only if the parent userns could use > setgroups and the opener of gid_map was privileged. That proposal sounds a lot more restrictive and a lot more of a pain to use than what I have implemented in my patch. > If we wanted to allow finer-grained control, we could allow writing > control lines like: > > options +setgroups > > or > > options -setgroups > > in gid_map, or we could add user_ns_flags that can only be written > once and only before either uid_map or gid_map is written. Definitely more complicated and I can't imagine a case where I need a gid map without needing to call setgroups. Eric ^ permalink raw reply [flat|nested] 73+ messages in thread
[parent not found: <87lhmwwpey.fsf_-_-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org>]
* Re: [CFT][PATCH] userns: Avoid problems with negative groups [not found] ` <87lhmwwpey.fsf_-_-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org> @ 2014-11-27 20:52 ` Andy Lutomirski 0 siblings, 0 replies; 73+ messages in thread From: Andy Lutomirski @ 2014-11-27 20:52 UTC (permalink / raw) To: Eric W. Biederman Cc: linux-man, Kees Cook, Linux API, Linux Containers, Josh Triplett, linux-kernel-u79uwXL29TY76Z2rM5mHXA, LSM, Michael Kerrisk-manpages, Richard Weinberger, Casey Schaufler, Andrew Morton On Thu, Nov 27, 2014 at 8:59 AM, Eric W. Biederman <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> wrote: > > Classic unix permission checks have an interesting feature. The group > permissions for a file can be set to less than the other permissions > on a file. Occassionally this is used deliberately to give a certain > group of users fewer permissions than the default. > > Overlooking negative groups has resulted in the permission checks for > setting up a group mapping in a user namespace to be too lax. Tighten > the permission checks in new_idmap_permitted to ensure that mapping > uids and gids into user namespaces without privilege will not result > in new combinations of credentials being available to the users. > > When setting mappings without privilege only the creator of the user > namespace is interesting as all other users that have CAP_SETUID over > the user namespace will also have CAP_SETUID over the user namespaces > parent. So the scope of the unprivileged check is reduced to just > the case where cred->euid is the namespace creator. > > For setting a uid mapping without privilege only euid is considered as > setresuid can set uid, suid and fsuid from euid without privielege > making any combination of uids possible with user namespaces already > possible without them. > > For setting a gid mapping without privilege only egid on a credential > without supplementary groups is condsidered, as setresgid can set gid, > sgid and fsgid from egid without privilege. The requirement for no > supplementary groups is because CAP_SETUID in a user namespace allows > supplementary groups to be cleared, which unfortunately means allowing > a credential with supplementary groups would allow new combinations > of credentials to exist, and thus would allow defeating negative > groups without permission. > > This change should break userspace by the minimal amount needed > to fix this issue. > > This should fix CVE-2014-8989. I think this is both unnecessarily restrictive and that it doesn't fix the bug. For example, I can exploit CVE-2014-8989 without ever writing a uid map or a gid map. IIUC, the only real issue is that user namespaces allow groups to be dropped using setgroups that wouldn't otherwise be dropped. Can we get away with adding a per-user-ns flag that determines whether setgroups can be used? setgroups would be unusable until the gid_map has been written and then it would become usable if and only if the parent userns could use setgroups and the opener of gid_map was privileged. If we wanted to allow finer-grained control, we could allow writing control lines like: options +setgroups or options -setgroups in gid_map, or we could add user_ns_flags that can only be written once and only before either uid_map or gid_map is written. --Andy > > Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> > --- > > If people can test and review this change and verify this and verify it > doesn't break anything I can't help breaking I will appreciate it. > > kernel/user_namespace.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c > index aa312b0dc3ec..b338c42d04fd 100644 > --- a/kernel/user_namespace.c > +++ b/kernel/user_namespace.c > @@ -812,16 +812,24 @@ static bool new_idmap_permitted(const struct file *file, > struct user_namespace *ns, int cap_setid, > struct uid_gid_map *new_map) > { > - /* Allow mapping to your own filesystem ids */ > - if ((new_map->nr_extents == 1) && (new_map->extent[0].count == 1)) { > + const struct cred *cred = file->f_cred; > + > + /* Allow mapping an id without CAP_SETUID and CAP_SETGID when > + * allowing the root of the user namespace CAP_SETUID or > + * CAP_SETGID restricted to just that id will not change the > + * set of credentials available that user. > + */ > + if ((new_map->nr_extents == 1) && (new_map->extent[0].count == 1) && > + uid_eq(ns->owner, cred->euid)) { > u32 id = new_map->extent[0].lower_first; > if (cap_setid == CAP_SETUID) { > kuid_t uid = make_kuid(ns->parent, id); > - if (uid_eq(uid, file->f_cred->fsuid)) > + if (uid_eq(uid, cred->euid)) > return true; > } else if (cap_setid == CAP_SETGID) { > kgid_t gid = make_kgid(ns->parent, id); > - if (gid_eq(gid, file->f_cred->fsgid)) > + if (gid_eq(gid, cred->egid) && > + (cred->group_info->ngroups == 0)) > return true; > } > } > -- > 1.9.1 > -- Andy Lutomirski AMA Capital Management, LLC ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-17 22:41 ` Eric W.Biederman 0 siblings, 0 replies; 73+ messages in thread From: Eric W.Biederman @ 2014-11-17 22:41 UTC (permalink / raw) To: Andy Lutomirski, Casey Schaufler Cc: Josh Triplett, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel On November 17, 2014 1:46:59 PM EST, Andy Lutomirski <luto@amacapital.net> wrote: >On Mon, Nov 17, 2014 at 10:31 AM, Andy Lutomirski <luto@amacapital.net> >wrote: >> On Mon, Nov 17, 2014 at 10:06 AM, Casey Schaufler >> <casey@schaufler-ca.com> wrote: >>> On 11/15/2014 1:01 AM, Josh Triplett wrote: >>>> Currently, unprivileged processes (without CAP_SETGID) cannot call >>>> setgroups at all. In particular, processes with a set of >supplementary >>>> groups cannot further drop permissions without obtaining elevated >>>> permissions first. >>> >>> Has anyone put any thought into how this will interact with >>> POSIX ACLs? I don't see that anywhere in the discussion. >> >> That means that user namespaces are a problem, too, and we need to >fix >> it. Or we should add some control to turn unprivileged user >namespace >> creation on and off and document that turning it on defeats POSIX >ACLs >> with a group entry that is more restrictive than the other entry. >> > >This is a significant enough issue that I posted it to oss-security: > >http://www.openwall.com/lists/oss-security/2014/11/17/19 > >It's not at all obvious to me how to fix it. We could disallow userns >creation of any supplementary groups don't match fsuid, or we could >keep negative-only groups around in the userns. > >It may be worth adding a sysctl to change the behavior, too. IMO it's >absurd to use groups to deny permissions that are otherwise available. There is an obvious user namespace fix. Don't allow dropping supplemental groups that are not mapped. That will require a little bit of fancy footwork if you want to play with supplemental groups in your unprivileged user namespace. I would like to get a grip on what hoops would be required before we add the additional restriction. Possibly something as simple as calling sg. I also want to look at what Tizen and any other concrete pieces of code I can find using this negative permission pattern are actually doing. Bugs definitely exist, but I have this erie feeling that the bugs may be in instances of userspace using this negative group permission pattern. I think we may have a hideous case of one setuid binary defeating a privilege check of another piece of code. This issue looks like it is worth a full scale investigation. Sigh. Eric ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-17 22:41 ` Eric W.Biederman 0 siblings, 0 replies; 73+ messages in thread From: Eric W.Biederman @ 2014-11-17 22:41 UTC (permalink / raw) To: Andy Lutomirski, Casey Schaufler Cc: Josh Triplett, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel-u79uwXL29TY76Z2rM5mHXA On November 17, 2014 1:46:59 PM EST, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote: >On Mon, Nov 17, 2014 at 10:31 AM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> >wrote: >> On Mon, Nov 17, 2014 at 10:06 AM, Casey Schaufler >> <casey-iSGtlc1asvQWG2LlvL+J4A@public.gmane.org> wrote: >>> On 11/15/2014 1:01 AM, Josh Triplett wrote: >>>> Currently, unprivileged processes (without CAP_SETGID) cannot call >>>> setgroups at all. In particular, processes with a set of >supplementary >>>> groups cannot further drop permissions without obtaining elevated >>>> permissions first. >>> >>> Has anyone put any thought into how this will interact with >>> POSIX ACLs? I don't see that anywhere in the discussion. >> >> That means that user namespaces are a problem, too, and we need to >fix >> it. Or we should add some control to turn unprivileged user >namespace >> creation on and off and document that turning it on defeats POSIX >ACLs >> with a group entry that is more restrictive than the other entry. >> > >This is a significant enough issue that I posted it to oss-security: > >http://www.openwall.com/lists/oss-security/2014/11/17/19 > >It's not at all obvious to me how to fix it. We could disallow userns >creation of any supplementary groups don't match fsuid, or we could >keep negative-only groups around in the userns. > >It may be worth adding a sysctl to change the behavior, too. IMO it's >absurd to use groups to deny permissions that are otherwise available. There is an obvious user namespace fix. Don't allow dropping supplemental groups that are not mapped. That will require a little bit of fancy footwork if you want to play with supplemental groups in your unprivileged user namespace. I would like to get a grip on what hoops would be required before we add the additional restriction. Possibly something as simple as calling sg. I also want to look at what Tizen and any other concrete pieces of code I can find using this negative permission pattern are actually doing. Bugs definitely exist, but I have this erie feeling that the bugs may be in instances of userspace using this negative group permission pattern. I think we may have a hideous case of one setuid binary defeating a privilege check of another piece of code. This issue looks like it is worth a full scale investigation. Sigh. Eric ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-17 22:50 ` Andy Lutomirski 0 siblings, 0 replies; 73+ messages in thread From: Andy Lutomirski @ 2014-11-17 22:50 UTC (permalink / raw) To: Eric W.Biederman Cc: Casey Schaufler, Josh Triplett, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel On Mon, Nov 17, 2014 at 2:41 PM, Eric W.Biederman <ebiederm@xmission.com> wrote: > > > On November 17, 2014 1:46:59 PM EST, Andy Lutomirski <luto@amacapital.net> wrote: >>On Mon, Nov 17, 2014 at 10:31 AM, Andy Lutomirski <luto@amacapital.net> >>wrote: >>> On Mon, Nov 17, 2014 at 10:06 AM, Casey Schaufler >>> <casey@schaufler-ca.com> wrote: >>>> On 11/15/2014 1:01 AM, Josh Triplett wrote: >>>>> Currently, unprivileged processes (without CAP_SETGID) cannot call >>>>> setgroups at all. In particular, processes with a set of >>supplementary >>>>> groups cannot further drop permissions without obtaining elevated >>>>> permissions first. >>>> >>>> Has anyone put any thought into how this will interact with >>>> POSIX ACLs? I don't see that anywhere in the discussion. >>> >>> That means that user namespaces are a problem, too, and we need to >>fix >>> it. Or we should add some control to turn unprivileged user >>namespace >>> creation on and off and document that turning it on defeats POSIX >>ACLs >>> with a group entry that is more restrictive than the other entry. >>> >> >>This is a significant enough issue that I posted it to oss-security: >> >>http://www.openwall.com/lists/oss-security/2014/11/17/19 >> >>It's not at all obvious to me how to fix it. We could disallow userns >>creation of any supplementary groups don't match fsuid, or we could >>keep negative-only groups around in the userns. >> >>It may be worth adding a sysctl to change the behavior, too. IMO it's >>absurd to use groups to deny permissions that are otherwise available. > > There is an obvious user namespace fix. Don't allow dropping supplemental groups that are not mapped. Why exactly does this fix it? I guess that, if a supplementary group is in your subgid list, then we can assume that dropping it is safe? > > That will require a little bit of fancy footwork if you want to play with supplemental groups in your unprivileged user namespace. I would like to get a grip on what hoops would be required before we add the additional restriction. Possibly something as simple as calling sg. The main hoop I can think of is that setgroups would be impossible to call if you have an unmapped supplementary group. This could break all kinds of things. > > I also want to look at what Tizen and any other concrete pieces of code I can find using this negative permission pattern are actually doing. Bugs definitely exist, but I have this erie feeling that the bugs may be in instances of userspace using this negative group permission pattern. I think we may have a hideous case of one setuid binary defeating a privilege check of another piece of code. > > This issue looks like it is worth a full scale investigation. Sigh. Agreed. > > Eric -- Andy Lutomirski AMA Capital Management, LLC ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-17 22:50 ` Andy Lutomirski 0 siblings, 0 replies; 73+ messages in thread From: Andy Lutomirski @ 2014-11-17 22:50 UTC (permalink / raw) To: Eric W.Biederman Cc: Casey Schaufler, Josh Triplett, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel-u79uwXL29TY76Z2rM5mHXA On Mon, Nov 17, 2014 at 2:41 PM, Eric W.Biederman <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> wrote: > > > On November 17, 2014 1:46:59 PM EST, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote: >>On Mon, Nov 17, 2014 at 10:31 AM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> >>wrote: >>> On Mon, Nov 17, 2014 at 10:06 AM, Casey Schaufler >>> <casey-iSGtlc1asvQWG2LlvL+J4A@public.gmane.org> wrote: >>>> On 11/15/2014 1:01 AM, Josh Triplett wrote: >>>>> Currently, unprivileged processes (without CAP_SETGID) cannot call >>>>> setgroups at all. In particular, processes with a set of >>supplementary >>>>> groups cannot further drop permissions without obtaining elevated >>>>> permissions first. >>>> >>>> Has anyone put any thought into how this will interact with >>>> POSIX ACLs? I don't see that anywhere in the discussion. >>> >>> That means that user namespaces are a problem, too, and we need to >>fix >>> it. Or we should add some control to turn unprivileged user >>namespace >>> creation on and off and document that turning it on defeats POSIX >>ACLs >>> with a group entry that is more restrictive than the other entry. >>> >> >>This is a significant enough issue that I posted it to oss-security: >> >>http://www.openwall.com/lists/oss-security/2014/11/17/19 >> >>It's not at all obvious to me how to fix it. We could disallow userns >>creation of any supplementary groups don't match fsuid, or we could >>keep negative-only groups around in the userns. >> >>It may be worth adding a sysctl to change the behavior, too. IMO it's >>absurd to use groups to deny permissions that are otherwise available. > > There is an obvious user namespace fix. Don't allow dropping supplemental groups that are not mapped. Why exactly does this fix it? I guess that, if a supplementary group is in your subgid list, then we can assume that dropping it is safe? > > That will require a little bit of fancy footwork if you want to play with supplemental groups in your unprivileged user namespace. I would like to get a grip on what hoops would be required before we add the additional restriction. Possibly something as simple as calling sg. The main hoop I can think of is that setgroups would be impossible to call if you have an unmapped supplementary group. This could break all kinds of things. > > I also want to look at what Tizen and any other concrete pieces of code I can find using this negative permission pattern are actually doing. Bugs definitely exist, but I have this erie feeling that the bugs may be in instances of userspace using this negative group permission pattern. I think we may have a hideous case of one setuid binary defeating a privilege check of another piece of code. > > This issue looks like it is worth a full scale investigation. Sigh. Agreed. > > Eric -- Andy Lutomirski AMA Capital Management, LLC -- To unsubscribe from this list: send the line "unsubscribe linux-man" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-17 23:13 ` josh-iaAMLnmF4UmaiuxdJuQwMA 0 siblings, 0 replies; 73+ messages in thread From: josh @ 2014-11-17 23:13 UTC (permalink / raw) To: Andy Lutomirski Cc: Eric W.Biederman, Casey Schaufler, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel On Mon, Nov 17, 2014 at 02:50:10PM -0800, Andy Lutomirski wrote: > On Mon, Nov 17, 2014 at 2:41 PM, Eric W.Biederman <ebiederm@xmission.com> wrote: > > > > > > On November 17, 2014 1:46:59 PM EST, Andy Lutomirski <luto@amacapital.net> wrote: > >>On Mon, Nov 17, 2014 at 10:31 AM, Andy Lutomirski <luto@amacapital.net> > >>wrote: > >>> On Mon, Nov 17, 2014 at 10:06 AM, Casey Schaufler > >>> <casey@schaufler-ca.com> wrote: > >>>> On 11/15/2014 1:01 AM, Josh Triplett wrote: > >>>>> Currently, unprivileged processes (without CAP_SETGID) cannot call > >>>>> setgroups at all. In particular, processes with a set of > >>supplementary > >>>>> groups cannot further drop permissions without obtaining elevated > >>>>> permissions first. > >>>> > >>>> Has anyone put any thought into how this will interact with > >>>> POSIX ACLs? I don't see that anywhere in the discussion. > >>> > >>> That means that user namespaces are a problem, too, and we need to > >>fix > >>> it. Or we should add some control to turn unprivileged user > >>namespace > >>> creation on and off and document that turning it on defeats POSIX > >>ACLs > >>> with a group entry that is more restrictive than the other entry. > >>> > >> > >>This is a significant enough issue that I posted it to oss-security: > >> > >>http://www.openwall.com/lists/oss-security/2014/11/17/19 > >> > >>It's not at all obvious to me how to fix it. We could disallow userns > >>creation of any supplementary groups don't match fsuid, or we could > >>keep negative-only groups around in the userns. > >> > >>It may be worth adding a sysctl to change the behavior, too. IMO it's > >>absurd to use groups to deny permissions that are otherwise available. > > > > There is an obvious user namespace fix. Don't allow dropping supplemental groups that are not mapped. > > Why exactly does this fix it? I guess that, if a supplementary group > is in your subgid list, then we can assume that dropping it is safe? Considering that one of the fixes I'd like to add is "allow mapping groups that you have in your supplemental group list", no, that fix doesn't suffice. :) > > That will require a little bit of fancy footwork if you want to play with supplemental groups in your unprivileged user namespace. I would like to get a grip on what hoops would be required before we add the additional restriction. Possibly something as simple as calling sg. > > The main hoop I can think of is that setgroups would be impossible to > call if you have an unmapped supplementary group. This could break > all kinds of things. Agreed. - Josh Triplett ^ permalink raw reply [flat|nested] 73+ messages in thread
* Re: [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups @ 2014-11-17 23:13 ` josh-iaAMLnmF4UmaiuxdJuQwMA 0 siblings, 0 replies; 73+ messages in thread From: josh-iaAMLnmF4UmaiuxdJuQwMA @ 2014-11-17 23:13 UTC (permalink / raw) To: Andy Lutomirski Cc: Eric W.Biederman, Casey Schaufler, Andrew Morton, Kees Cook, Michael Kerrisk-manpages, Linux API, linux-man, linux-kernel-u79uwXL29TY76Z2rM5mHXA On Mon, Nov 17, 2014 at 02:50:10PM -0800, Andy Lutomirski wrote: > On Mon, Nov 17, 2014 at 2:41 PM, Eric W.Biederman <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> wrote: > > > > > > On November 17, 2014 1:46:59 PM EST, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote: > >>On Mon, Nov 17, 2014 at 10:31 AM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> > >>wrote: > >>> On Mon, Nov 17, 2014 at 10:06 AM, Casey Schaufler > >>> <casey-iSGtlc1asvQWG2LlvL+J4A@public.gmane.org> wrote: > >>>> On 11/15/2014 1:01 AM, Josh Triplett wrote: > >>>>> Currently, unprivileged processes (without CAP_SETGID) cannot call > >>>>> setgroups at all. In particular, processes with a set of > >>supplementary > >>>>> groups cannot further drop permissions without obtaining elevated > >>>>> permissions first. > >>>> > >>>> Has anyone put any thought into how this will interact with > >>>> POSIX ACLs? I don't see that anywhere in the discussion. > >>> > >>> That means that user namespaces are a problem, too, and we need to > >>fix > >>> it. Or we should add some control to turn unprivileged user > >>namespace > >>> creation on and off and document that turning it on defeats POSIX > >>ACLs > >>> with a group entry that is more restrictive than the other entry. > >>> > >> > >>This is a significant enough issue that I posted it to oss-security: > >> > >>http://www.openwall.com/lists/oss-security/2014/11/17/19 > >> > >>It's not at all obvious to me how to fix it. We could disallow userns > >>creation of any supplementary groups don't match fsuid, or we could > >>keep negative-only groups around in the userns. > >> > >>It may be worth adding a sysctl to change the behavior, too. IMO it's > >>absurd to use groups to deny permissions that are otherwise available. > > > > There is an obvious user namespace fix. Don't allow dropping supplemental groups that are not mapped. > > Why exactly does this fix it? I guess that, if a supplementary group > is in your subgid list, then we can assume that dropping it is safe? Considering that one of the fixes I'd like to add is "allow mapping groups that you have in your supplemental group list", no, that fix doesn't suffice. :) > > That will require a little bit of fancy footwork if you want to play with supplemental groups in your unprivileged user namespace. I would like to get a grip on what hoops would be required before we add the additional restriction. Possibly something as simple as calling sg. > > The main hoop I can think of is that setgroups would be impossible to > call if you have an unmapped supplementary group. This could break > all kinds of things. Agreed. - Josh Triplett -- To unsubscribe from this list: send the line "unsubscribe linux-man" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 73+ messages in thread
* [PATCH manpages] getgroups.2: Document unprivileged setgroups calls @ 2014-11-15 9:01 ` Josh Triplett 0 siblings, 0 replies; 73+ messages in thread From: Josh Triplett @ 2014-11-15 9:01 UTC (permalink / raw) To: Andrew Morton, Eric W. Biederman, Kees Cook, mtk.manpages, linux-api, linux-man, linux-kernel Signed-off-by: Josh Triplett <josh@joshtriplett.org> --- This should probably also include appropriate documentation for what kernel introduces this behavior. man2/getgroups.2 | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/man2/getgroups.2 b/man2/getgroups.2 index 373c204..edca37c 100644 --- a/man2/getgroups.2 +++ b/man2/getgroups.2 @@ -81,9 +81,10 @@ to be used in a further call to .PP .BR setgroups () sets the supplementary group IDs for the calling process. -Appropriate privileges (Linux: the +Any process may drop groups from its list, but adding groups requires +appropriate privileges (Linux: the .B CAP_SETGID -capability) are required. +capability). The .I size argument specifies the number of supplementary group IDs -- 2.1.3 ^ permalink raw reply related [flat|nested] 73+ messages in thread
* [PATCH manpages] getgroups.2: Document unprivileged setgroups calls @ 2014-11-15 9:01 ` Josh Triplett 0 siblings, 0 replies; 73+ messages in thread From: Josh Triplett @ 2014-11-15 9:01 UTC (permalink / raw) To: Andrew Morton, Eric W. Biederman, Kees Cook, mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w, linux-api-u79uwXL29TY76Z2rM5mHXA, linux-man-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA Signed-off-by: Josh Triplett <josh-iaAMLnmF4UmaiuxdJuQwMA@public.gmane.org> --- This should probably also include appropriate documentation for what kernel introduces this behavior. man2/getgroups.2 | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/man2/getgroups.2 b/man2/getgroups.2 index 373c204..edca37c 100644 --- a/man2/getgroups.2 +++ b/man2/getgroups.2 @@ -81,9 +81,10 @@ to be used in a further call to .PP .BR setgroups () sets the supplementary group IDs for the calling process. -Appropriate privileges (Linux: the +Any process may drop groups from its list, but adding groups requires +appropriate privileges (Linux: the .B CAP_SETGID -capability) are required. +capability). The .I size argument specifies the number of supplementary group IDs -- 2.1.3 -- To unsubscribe from this list: send the line "unsubscribe linux-man" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 73+ messages in thread
end of thread, other threads:[~2014-11-28 17:12 UTC | newest] Thread overview: 73+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-11-15 9:00 [PATCH 1/2] groups: Factor out a function to set a pre-sorted group list Josh Triplett 2014-11-15 9:00 ` Josh Triplett 2014-11-15 9:01 ` [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups Josh Triplett 2014-11-15 15:37 ` Eric W. Biederman 2014-11-15 15:37 ` Eric W. Biederman 2014-11-15 19:29 ` Josh Triplett 2014-11-15 19:29 ` Josh Triplett 2014-11-15 20:06 ` Andy Lutomirski 2014-11-15 20:06 ` Andy Lutomirski 2014-11-15 20:20 ` Josh Triplett 2014-11-15 20:20 ` Josh Triplett 2014-11-16 2:05 ` Theodore Ts'o 2014-11-16 2:05 ` Theodore Ts'o 2014-11-16 2:35 ` Josh Triplett 2014-11-16 2:35 ` Josh Triplett 2014-11-16 3:08 ` Eric W. Biederman 2014-11-16 3:08 ` Eric W. Biederman 2014-11-16 5:07 ` Josh Triplett 2014-11-16 5:07 ` Josh Triplett 2014-11-16 13:32 ` Theodore Ts'o 2014-11-16 13:32 ` Theodore Ts'o 2014-11-16 15:42 ` Andy Lutomirski 2014-11-16 15:42 ` Andy Lutomirski 2014-11-16 19:12 ` Josh Triplett 2014-11-16 19:12 ` Josh Triplett 2014-11-16 19:09 ` Josh Triplett 2014-11-16 19:09 ` Josh Triplett 2014-11-16 3:40 ` Theodore Ts'o 2014-11-16 3:40 ` Theodore Ts'o 2014-11-16 4:52 ` Josh Triplett 2014-11-16 4:52 ` Josh Triplett 2014-11-17 11:37 ` One Thousand Gnomes 2014-11-17 11:37 ` One Thousand Gnomes 2014-11-17 18:07 ` Andy Lutomirski 2014-11-17 18:07 ` Andy Lutomirski 2014-11-17 22:11 ` Eric W.Biederman 2014-11-17 22:11 ` Eric W.Biederman 2014-11-17 22:22 ` Andy Lutomirski 2014-11-17 22:22 ` Andy Lutomirski 2014-11-17 22:37 ` josh 2014-11-17 22:37 ` josh-iaAMLnmF4UmaiuxdJuQwMA 2014-11-18 0:56 ` Casey Schaufler 2014-11-17 18:06 ` Casey Schaufler 2014-11-17 18:31 ` Andy Lutomirski 2014-11-17 18:31 ` Andy Lutomirski 2014-11-17 18:46 ` Andy Lutomirski 2014-11-17 18:51 ` Casey Schaufler [not found] ` <546A43CE.2030706-iSGtlc1asvQWG2LlvL+J4A@public.gmane.org> 2014-11-27 16:59 ` [CFT][PATCH] userns: Avoid problems with negative groups Eric W. Biederman 2014-11-27 16:59 ` Eric W. Biederman 2014-11-27 20:52 ` Andy Lutomirski 2014-11-27 20:52 ` Andy Lutomirski 2014-11-28 5:21 ` Eric W. Biederman 2014-11-28 5:21 ` Eric W. Biederman [not found] ` <87wq6frjcw.fsf-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org> 2014-11-28 5:22 ` [CFT][PATCH v2] " Eric W. Biederman 2014-11-28 5:22 ` Eric W. Biederman 2014-11-28 15:11 ` [CFT][PATCH] " Andy Lutomirski 2014-11-28 15:11 ` Andy Lutomirski 2014-11-28 15:11 ` Andy Lutomirski [not found] ` <CALCETrX2s-7iaLMEKLQsExTEp3JyoAPQG44p0v5wkeED3-6dQA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2014-11-28 16:34 ` Eric W. Biederman 2014-11-28 16:34 ` Eric W. Biederman 2014-11-28 16:34 ` Eric W. Biederman [not found] ` <874mtjp9m1.fsf-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org> 2014-11-28 17:11 ` Andy Lutomirski 2014-11-28 17:11 ` Andy Lutomirski [not found] ` <CALCETrUuWDq2akKfb50AiPHeDDWzPW7ijz1QwnuNiskyZbBEfA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2014-11-28 5:21 ` Eric W. Biederman [not found] ` <87lhmwwpey.fsf_-_-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org> 2014-11-27 20:52 ` Andy Lutomirski 2014-11-17 22:41 ` [PATCH 2/2] groups: Allow unprivileged processes to use setgroups to drop groups Eric W.Biederman 2014-11-17 22:41 ` Eric W.Biederman 2014-11-17 22:50 ` Andy Lutomirski 2014-11-17 22:50 ` Andy Lutomirski 2014-11-17 23:13 ` josh 2014-11-17 23:13 ` josh-iaAMLnmF4UmaiuxdJuQwMA 2014-11-15 9:01 ` [PATCH manpages] getgroups.2: Document unprivileged setgroups calls Josh Triplett 2014-11-15 9:01 ` Josh Triplett
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.