linux-kernel - Re: [PATCH v3.4] 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: <m15z0fphwt.fsf@fess.ebiederm.org>
Date:   Wed, 21 Apr 2021 14:16:34 -0500
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     "Serge E. Hallyn" <serge@...lyn.com>
Cc:     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>
Subject: Re: [PATCH v3.4] capabilities: require CAP_SETFCAP to map uid 0

"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 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