linux-kernel - Re: [PATCH] capabilities: require CAP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210420134047.GA11346@mail.hallyn.com>
Date:   Tue, 20 Apr 2021 08:40:47 -0500
From:   "Serge E. Hallyn" <serge@...lyn.com>
To:     Giuseppe Scrivano <gscrivan@...hat.com>
Cc:     "Eric W. Biederman" <ebiederm@...ssion.com>,
        Christian Brauner <christian.brauner@...ntu.com>,
        lkml <linux-kernel@...r.kernel.org>,
        Linus Torvalds <torvalds@...uxfoundation.org>,
        Kees Cook <keescook@...omium.org>,
        "Andrew G. Morgan" <morgan@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        security@...nel.org, Tycho Andersen <tycho@...ho.ws>,
        Andy Lutomirski <luto@...nel.org>,
        "Serge E. Hallyn" <serge@...lyn.com>
Subject: Re: [PATCH] capabilities: require CAP_SETFCAP to map uid 0 (v3.2)

On Mon, Apr 19, 2021 at 05:52:39PM +0200, Giuseppe Scrivano wrote:
> ebiederm@...ssion.com (Eric W. Biederman) writes:
> 
> > Guiseppe can you take a look at this?
> >
> > This is a second attempt at tightening up the semantics of writing to
> > file capabilities from a user namespace.
> >
> > The first attempt was reverted with 3b0c2d3eaa83 ("Revert 95ebabde382c
> > ("capabilities: Don't allow writing ambiguous v3 file capabilities")"),
> > which corrected the issue reported in:
> > https://github.com/containers/buildah/issues/3071
> >
> > There is a report the podman testsuite passes.  While different this
> > looks in many ways much more strict than the code that was reverted.  So
> > while I can imagine this change doesn't cause problems as is, I will be
> > surprised.
> 
> thanks for pulling me in the discussion.
> 
> I've tested the patch with several cases similar to the issue we had in
> the past and the patch seems to work well.  
> 
> Podman creates all the user namespaces within the same parent user
> namespace.  In the parent user namespace all the capabilities are kept
> and AFAIK Docker does the same.  I'd expect a change in behavior only
> for nested user namespaces in containers where CAP_SETFCAP is not
> granted, but that is not a common configuration given that CAP_SETFCAP
> is added by default.
> 
> 
> > "Serge E. Hallyn" <serge@...lyn.com> writes:
> >
> >> +/**
> >> + * verify_root_map() - check the uid 0 mapping
> >> + * @file: idmapping file
> >> + * @map_ns: user namespace of the target process
> >> + * @new_map: requested idmap
> >> + *
> >> + * If a process requested a mapping for uid 0 onto uid 0, verify that the
> >> + * process writing the map had the CAP_SETFCAP capability as the target process
> >> + * will be able to write fscaps that are valid in ancestor user namespaces.
> >> + *
> >> + * Return: true if the mapping is allowed, false if not.
> >> + */
> >> +static bool verify_root_map(const struct file *file,
> >> +			    struct user_namespace *map_ns,
> >> +			    struct uid_gid_map *new_map)
> >> +{
> >> +	int idx;
> >> +	const struct user_namespace *file_ns = file->f_cred->user_ns;
> >> +	struct uid_gid_extent *extent0 = NULL;
> >> +
> >> +	for (idx = 0; idx < new_map->nr_extents; idx++) {
> >> +		u32 lower_first;
> 
> nit: lower_first seems unused?

Drat - I noticed that Sunday or Monday and forgot to remove it, thanks.

> >> +
> >> +		if (new_map->nr_extents <= UID_GID_MAP_MAX_BASE_EXTENTS)
> >> +			extent0 = &new_map->extent[idx];
> >> +		else
> >> +			extent0 = &new_map->forward[idx];
> >> +		if (extent0->lower_first == 0)
> >> +			break;
> >> +
> >> +		extent0 = NULL;
> >> +	}
> 
> Tested-by: Giuseppe Scrivano <gscrivan@...hat.com>

Awesome - thanks for testing.