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

[Stefan Bader]

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

-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/d3b1b55a/attachment.sig>