lkml.org 
[lkml]   [2021]   [Apr]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v3.4] capabilities: require CAP_SETFCAP to map uid 0
"Serge E. Hallyn" <serge@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 requests mapping parent uid 0 into the new ns, 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++) {
> + 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;
> + }
> +
> + if (!extent0)
> + return true;
> +
> + if (map_ns == file_ns) {
> + /* The process unshared its ns and is writing to its own
> + * /proc/self/uid_map. User already has full capabilites in
> + * the new namespace. Verify that the parent had CAP_SETFCAP
> + * when it unshared.
> + * */
> + if (!file_ns->parent_could_setfcap)
> + return false;
> + } else {
> + /* Process p1 is writing to uid_map of p2, who is in a child
> + * user namespace to p1's. Verify that the opener of the map
> + * file has CAP_SETFCAP against the parent of the new map
> + * namespace */
> + if (!file_ns_capable(file, map_ns->parent, CAP_SETFCAP))
> + return false;
> + }

Is there any reason this permission check is not simply:

return map_ns->parent_could_setfcap ||
file_ns_capable(file, map_ns->parent, CAP_SETFCAP);

That is why don't we allow any mapping (that is otherwise valid) in user
namespaces whose creator had the permission to call CAP_SETFCAP?

Why limit the case of using the creators permissions to only the case of
mapping just a single uid (that happens to be the current euid) in the
user namespace?

I don't see any safety reasons for the map_ns == file_ns test.



Is the file_ns_capable check for CAP_SETFCAP actually needed? AKA could
the permission check be simplified to:

return map_ns->parent_could_setfcap;

That would be a much easier rule to explain to people.

I seem to remember distributions at least trying to make newuidmap have
just CAP_SETUID and newgidmap have just CAP_SETGID. Such a simplified
check would facilitate that.

Eric

\
 
 \ /
  Last update: 2021-04-21 21:19    [W:0.084 / U:0.636 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site