From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 00609C433ED for ; Tue, 18 May 2021 09:32:48 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id D00E860E09 for ; Tue, 18 May 2021 09:32:48 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1347929AbhERJeF (ORCPT ); Tue, 18 May 2021 05:34:05 -0400 Received: from mail.kernel.org ([198.145.29.99]:45210 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S239643AbhERJeE (ORCPT ); Tue, 18 May 2021 05:34:04 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id 2D24461073; Tue, 18 May 2021 09:32:44 +0000 (UTC) Date: Tue, 18 May 2021 11:32:41 +0200 From: Christian Brauner To: "Serge E. Hallyn" Cc: Giuseppe Scrivano , linux-kernel@vger.kernel.org, dwalsh@redhat.com, ebiederm@xmission.com Subject: Re: [RFC PATCH 1/3] setgroups: new mode 'shadow' for /proc/PID/setgroups Message-ID: <20210518093241.7hqs2hnlqeywvhp5@wittgenstein> References: <20210510130011.1441834-1-gscrivan@redhat.com> <20210510130011.1441834-2-gscrivan@redhat.com> <20210515015157.GB2845@mail.hallyn.com> <87y2cdqyhj.fsf@redhat.com> <20210517143321.en2jy2gaxrhdhvub@wittgenstein> <20210517161713.GB25644@mail.hallyn.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20210517161713.GB25644@mail.hallyn.com> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, May 17, 2021 at 11:17:13AM -0500, Serge Hallyn wrote: > On Mon, May 17, 2021 at 04:33:21PM +0200, Christian Brauner wrote: > > On Mon, May 17, 2021 at 03:30:16PM +0200, Giuseppe Scrivano wrote: > > > >> diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c > > > >> index 8d62863721b0..b1940b63f7ac 100644 > > > >> --- a/kernel/user_namespace.c > > > >> +++ b/kernel/user_namespace.c > > > >> @@ -123,6 +123,7 @@ int create_user_ns(struct cred *new) > > > >> ns->ucount_max[i] = INT_MAX; > > > >> } > > > >> ns->ucounts = ucounts; > > > >> + ns->shadow_group_info = get_current_groups(); > > > > > > > > If userns u1 unshares u2 with shadow set, then when u2 unshares > > > > u3, should u3 get the same shadowed set that u2 has, or should it > > > > get all of u2's groups as u3's initial shadow set? > > > > > > good question. Thinking more of it, I think a reasonable interface is > > > to expect a child userns to inherit the same shadow groups as its parent > > > userns. If "shadow" is written again to the /proc/PID/setgroups file > > > then it grows shadow groups set to include the ones the userns had at > > > creation time (which includes the parent shadow groups). What do you > > > think of it? I'll play more with this idea and see if it works. > > > > So when I initially looked at that proposal I was neither "yay" or "nay" > > since it seemed useful to people and it looked somewhat straightforward > > to implement. > > > > But I do have concerns now after seeing this. The whole > > /proc//setgroups API is terrible in the first place and causes even > > more special-casing in container runtimes then there already is. But it > > fixes a security issue so ok we'll live with it. > > > > But I'm not happy about extending its format to include more options. I > > really don't want the precedent of adding magic keywords into this file. > > > > Which brings me to my second concern. I think starting to magically > > inherit group ids isn't a great idea. It's got a lot of potential for > > confusion. > > > > The point Serge here made makes this pretty obvious imho. I don't think > > introducing the complexities of magic group inheritance is something we > > should do. > > > > Alternative proposal, can we solve this in userspace instead? > > > > As has been pointed out there is a solution to this problem already > > which is to explicitly map those groups through, i.e. punch holes for > > the groups to be inherited. > > > > So can't we introduce a new mode for newgidmap by e.g. introducing > > another /etc/setgroups file or something similar that can be configured > > by the administrator. It could take options, e.g. "shadow=always" which > > could mean "everyone must inherit their groups" so newgidmap will punch > > holes for the caller's groups when writing the gid mapping. We could > > also extend this by making newgidmap take a command line switch so it's > > on a case-by-case basis. > > > > This is even more flexible since you could extend the new /etc/setgroups > > file to specify a list of groups that must always be preserved. I'd > > rather see something like this rather than a magic inheritance switch. > > > > Christian > > That sounds reasonable, but my concern is that admins currently using > groups to deny file access will need to take extra steps to maintain > that guarantee. I think that falls under the category of a regression. > Unless we make 'shadow=always' the default. But then *that* will regress > users who currently do not want that feature :) I think this should simply be up to the administrator. > > Anyway, if we do go this route - or maybe a login.defs option > "ALLOW_UNPRIV_GROUPS_DROP" - perhaps we can also add a new /etc/subauxgroups > file (TODO find a better name) which admins who are in the know can use to say > "hallyn 2000" meaning "user hallyn cannot drop group 2000" That sounds like something useful. Ideally integrated with the newly added libsubid. Christian