Messages in this thread | | | From | (Eric W. Biederman) | Date | Wed, 21 Apr 2021 14:16:34 -0500 | Subject | Re: [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
| |