On Mon, Apr 19, 2021 at 05:52:39PM +0200, Giuseppe Scrivano wrote:
> ebied...@xmission.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" <se...@hallyn.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 <gscri...@redhat.com>
Awesome - thanks for testing.
