linux-kernel - Re: [PATCH 01/12] Miscellaneous functions and defines needed by AppArmor, including the base path resolution routines.

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: <4B7FD2F0.6090602@canonical.com>
Date:	Sat, 20 Feb 2010 04:17:52 -0800
From:	John Johansen <john.johansen@...onical.com>
To:	Al Viro <viro@...IV.linux.org.uk>
CC:	linux-kernel@...r.kernel.org, linux-security-module@...r.kernel.org
Subject: Re: [PATCH 01/12] Miscellaneous functions and defines needed by AppArmor,
 including the base path resolution routines.

Al Viro wrote:
> On Fri, Feb 19, 2010 at 01:36:17AM -0800, john.johansen@...onical.com wrote:
> 
>> +static int d_namespace_path(struct path *path, char *buf, int buflen,
>> +			    char **name, int flags)
>> +{
>> +	struct path root, tmp, ns_root = { };
>> +	char *res;
>> +	int deleted, connected;
>> +	int error = 0;
>> +
>> +	read_lock(&current->fs->lock);
>> +	root = current->fs->root;
>> +	/* released below */
>> +	path_get(&root);
>> +	read_unlock(&current->fs->lock);
>> +
>> +	spin_lock(&vfsmount_lock);
>> +	if (root.mnt && root.mnt->mnt_ns)
>> +		/* released below */
>> +		ns_root.mnt = mntget(root.mnt->mnt_ns->root);
>> +	if (ns_root.mnt)
>> +		/* released below */
>> +		ns_root.dentry = dget(ns_root.mnt->mnt_root);
>> +	spin_unlock(&vfsmount_lock);
> 
> Junk.  You might as well leave ns_root {NULL, NULL} instead of that crap.
> 
Right, though the ns_root.mnt and ns_root.dentry are needed below to detect
disconnected paths.

<< snip >>

> 
>> +	if (flags & PATH_CHROOT_REL)
>> +		connected = tmp.dentry == root.dentry && tmp.mnt == root.mnt;
>> +	else
>> +		connected = tmp.dentry == ns_root.dentry &&
>> +			tmp.mnt == ns_root.mnt;
>> +
>> +	if (!connected && 
>> +	    !(flags & PATH_CONNECT_PATH) &&
>> +	    !((flags & PATH_CHROOT_REL) && (flags & PATH_CHROOT_NSCONNECT) &&
>> +	      (tmp.dentry == ns_root.dentry && tmp.mnt == ns_root.mnt))) {
>> +		/* disconnected path, don't return pathname starting with '/' */
>> +		error = -ESTALE;
>> +		if (*res == '/')
>> +			*name = res + 1;
> 
> Explanations, please.

Right this needs to have a very good comment attached.

Basically AppArmor wants to know if the path is connected to the expected root
(either the chroot, or the namespace depending on how the profile is created).

When the file object isn't actually connected, and the profile is setup to use
file delegation instead of artificially connecting it to /, we stip the leading /
returned by __d_path disconnecting the path.

Objects disconnected from the root in this way can not be matched via regular path
matching rules but need to be delegated.

Previously we were doing this by patching __d_path to return disconnected paths
and then reattaching them in d_path, and getcwd.  In many ways that was cleaner
as then we wouldn't need to grab to the ns_root to find if the path was connected
or not.

But for this submission I split any proposed changes to __d_path from AppArmor
so that they can be discussed separately.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/