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

From: "Eric W. Biederman" <ebiederm@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@xmission.com>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
CVE-2015-2925
BugLink: https://bugs.launchpad.net/bugs/1441108
Signed-off-by: Luis Henriques <luis.henriques@canonical.com>
---
 fs/namei.c | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

On 05.10.2015 14:58, Luis Henriques wrote:
> From: "Eric W. Biederman" <ebiederm@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@xmission.com>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> CVE-2015-2925
> BugLink: https://bugs.launchpad.net/bugs/1441108
> Signed-off-by: Luis Henriques <luis.henriques@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@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...

-Stefan

>  	}
>

On 06.10.2015 10:05, Stefan Bader wrote:
> On 05.10.2015 14:58, Luis Henriques wrote:
>> From: "Eric W. Biederman" <ebiederm@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@xmission.com>
>> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
>> CVE-2015-2925
>> BugLink: https://bugs.launchpad.net/bugs/1441108
>> Signed-off-by: Luis Henriques <luis.henriques@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@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
> 
>>  	}
>>
> 
> 
> 
>