[CVE-2015-2925][Vivid][PATCH 2/2] vfs: Test for and handle paths that are unreachable from their mnt_root

Tue Oct 6 10:21:47 UTC 2015

On 06.10.2015 10:05, Stefan Bader wrote:
> On 05.10.2015 14:58, Luis Henriques wrote:
>> From: "Eric W. Biederman" <ebiederm at xmission.com>
>>
>> commit 397d425dc26da728396e66d392d5dcb8dac30c37 upstream.
>>
>> In rare cases a directory can be renamed out from under a bind mount.
>> In those cases without special handling it becomes possible to walk up
>> the directory tree to the root dentry of the filesystem and down
>> from the root dentry to every other file or directory on the filesystem.
>>
>> Like division by zero .. from an unconnected path can not be given
>> a useful semantic as there is no predicting at which path component
>> the code will realize it is unconnected.  We certainly can not match
>> the current behavior as the current behavior is a security hole.
>>
>> Therefore when encounting .. when following an unconnected path
>> return -ENOENT.
>>
>> - Add a function path_connected to verify path->dentry is reachable
>>   from path->mnt.mnt_root.  AKA to validate that rename did not do
>>   something nasty to the bind mount.
>>
>>   To avoid races path_connected must be called after following a path
>>   component to it's next path component.
>>
>> Signed-off-by: "Eric W. Biederman" <ebiederm at xmission.com>
>> Signed-off-by: Al Viro <viro at zeniv.linux.org.uk>
>> CVE-2015-2925
>> BugLink: https://bugs.launchpad.net/bugs/1441108
>> Signed-off-by: Luis Henriques <luis.henriques at canonical.com>
>> ---
>>  fs/namei.c | 31 ++++++++++++++++++++++++++++---
>>  1 file changed, 28 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/namei.c b/fs/namei.c
>> index 27007985ba76..1db59ccbae7d 100644
>> --- a/fs/namei.c
>> +++ b/fs/namei.c
>> @@ -500,6 +500,24 @@ struct nameidata {
>>  	char *saved_names[MAX_NESTED_LINKS + 1];
>>  };
>>  
>> +/**
>> + * path_connected - Verify that a path->dentry is below path->mnt.mnt_root
>> + * @path: nameidate to verify
>> + *
>> + * Rename can sometimes move a file or directory outside of a bind
>> + * mount, path_connected allows those cases to be detected.
>> + */
>> +static bool path_connected(const struct path *path)
>> +{
>> +	struct vfsmount *mnt = path->mnt;
>> +
>> +	/* Only bind mounts can have disconnected paths */
>> +	if (mnt->mnt_root == mnt->mnt_sb->s_root)
>> +		return true;
>> +
>> +	return is_subdir(path->dentry, mnt->mnt_root);
>> +}
>> +
>>  /*
>>   * Path walking has 2 modes, rcu-walk and ref-walk (see
>>   * Documentation/filesystems/path-lookup.txt).  In situations when we can't
>> @@ -1189,6 +1207,8 @@ static int follow_dotdot_rcu(struct nameidata *nd)
>>  				goto failed;
>>  			nd->path.dentry = parent;
>>  			nd->seq = seq;
>> +			if (unlikely(!path_connected(&nd->path)))
>> +				goto failed;
>>  			break;
>>  		}
>>  		if (!follow_up_rcu(&nd->path))
>> @@ -1285,7 +1305,7 @@ static void follow_mount(struct path *path)
>>  	}
>>  }
>>  
>> -static void follow_dotdot(struct nameidata *nd)
>> +static int follow_dotdot(struct nameidata *nd)
>>  {
>>  	if (!nd->root.mnt)
>>  		set_root(nd);
>> @@ -1301,6 +1321,10 @@ static void follow_dotdot(struct nameidata *nd)
>>  			/* rare case of legitimate dget_parent()... */
>>  			nd->path.dentry = dget_parent(nd->path.dentry);
>>  			dput(old);
> 
>> +			if (unlikely(!path_connected(&nd->path))) {
>> +				path_put(&nd->path);
>> +				return -ENOENT;
> 
> This being a backport by upstream for a cve I should probably have more
> confidence in what they did. But here I worry a bit. The additional path_put
> here looks to be a result (together with the replacement of goto below for
> mountpoint_last) of working around changes done by:
> 
> commit deb106c632d73c96b6b2b5ca71bacb8aef38fc7b
> Author: Al Viro <viro at zeniv.linux.org.uk>
> Date:   Fri May 8 18:05:21 2015 -0400
> 
>     namei: lift terminate_walk() all the way up
> 
> which moved the call to terminate_walk() up to higher level in the call stack.
> And one thing terminate_walk() does is to do the path_pt (for non rcu case).
> 
>> +			}
>>  			break;
>>  		}
>>  		if (!follow_up(&nd->path))
>> @@ -1308,6 +1332,7 @@ static void follow_dotdot(struct nameidata *nd)
>>  	}
>>  	follow_mount(&nd->path);
>>  	nd->inode = nd->path.dentry->d_inode;
>> +	return 0;
>>  }
>>  
>>  /*
>> @@ -1528,7 +1553,7 @@ static inline int handle_dots(struct nameidata *nd, int type)
>>  			if (follow_dotdot_rcu(nd))
>>  				return -ECHILD;
>>  		} else
>> -			follow_dotdot(nd);
>> +			return follow_dotdot(nd);
>>  	}
>>  	return 0;
>>  }
>> @@ -2263,7 +2288,7 @@ mountpoint_last(struct nameidata *nd, struct path *path)
>>  	if (unlikely(nd->last_type != LAST_NORM)) {
>>  		error = handle_dots(nd, nd->last_type);
>>  		if (error)
>> -			goto out;
>> +			return error;
>>  		dentry = dget(nd->path.dentry);
>>  		goto done;
> 
> So here either follow_dotdot_rcu() or follow dotdot() are called and the "goto
> out" in the error case would call terminate_walk() which I assume has not been
> added to the higher level function in Vivid. So I wonder a bit whether the right
> change would be to not add this hunk and neither add the path_put...

Hm ok, Luis pointed me to http://article.gmane.org/gmane.linux.kernel.stable/151103

which does not exactly explain why this works but apparently it got tried the
other way and that did not. So its hopefully ok...

> -Stefan
> 
>>  	}
>>
> 
> 
> 
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20151006/60380d5e/attachment.sig>