Re: [PATCH] capabilities: require CAP_SETFCAP to map uid 0 (v3.2)

[Giuseppe Scrivano]

ebiederm@xxxxxxxxxxxx (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@xxxxxxxxxx> 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?

>> +
>> +		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@xxxxxxxxxx>

Regards,
Giuseppe