[PATCH] for bug 11647 - fix access denied when path = "/"

Wed Dec 23 20:27:27 UTC 2015

On Wed, Dec 23, 2015 at 08:01:16PM +0100, Michael Adam wrote:
> attachment...
> 
> (See the patch with git show -w)
> 
> On 2015-12-23 at 19:55 +0100, Michael Adam wrote:
> > Hi,
> > 
> > please see attached patch for
> > 
> > BUG: https://bugzilla.samba.org/show_bug.cgi?id=11647
> > 
> > José and I are in the process of verifying that it fixes
> > all the issues we have seen.
> > 
> > Thanks - Michael

Reviewed-by: Jeremy Allison <jra at samba.org>

Feel free to push to master !

Thanks,

Jeremy.

> From 18be79b6e7192350d26f2abdc5f27c95563d7c9f Mon Sep 17 00:00:00 2001
> From: Michael Adam <obnox at samba.org>
> Date: Wed, 23 Dec 2015 18:01:23 +0100
> Subject: [PATCH] s3:smbd: fix a corner case of the symlink verification
> 
> Commit 7606c0db257b3f9d84da5b2bf5fbb4034cc8d77d fixes the
> path checks in check_reduced_name[_with_privilege]() to
> prevent unintended access via wide links.
> 
> The fix fails to correctly treat a corner case where the share
> path is "/". This case is important for some real world
> scenarios, notably the use of the glusterfs VFS module:
> 
> For the share path "/", the newly introduced checks deny all
> operations in the share.
> 
> This change fixes the checks for the corner case.
> The point is that the assumptions on which the original
> checks are based are not true for the rootdir "/" case.
> This is the case where the rootdir starts _and ends_ with
> a slash. Hence a subdirectory does not continue with a
> slash after the rootdir, since the candidate path has
> been normalized.
> 
> This fix just omits the string comparison and the
> next character checks in the case of rootdir "/",
> which is correct because we know that the candidate
> path is normalized and hence starts with a '/'.
> 
> The patch is fairly minimal, but changes indentation,
> hence best viewed with 'git show -w'.
> 
> A side effect is that the rootdir="/" case needs
> one strncmp less.
> 
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=11647
> 
> Pair-Programmed-With: Jose A. Rivera <jarrpa at samba.org>
> 
> Signed-off-by: Michael Adam <obnox at samba.org>
> Signed-off-by: Jose A. Rivera <jarrpa at samba.org>
> ---
>  source3/smbd/vfs.c | 78 +++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 53 insertions(+), 25 deletions(-)
> 
> diff --git a/source3/smbd/vfs.c b/source3/smbd/vfs.c
> index 27b38d6..1a2ee3d 100644
> --- a/source3/smbd/vfs.c
> +++ b/source3/smbd/vfs.c
> @@ -982,7 +982,6 @@ NTSTATUS check_reduced_name_with_privilege(connection_struct *conn,
>  	struct smb_filename *smb_fname_cwd = NULL;
>  	struct privilege_paths *priv_paths = NULL;
>  	int ret;
> -	bool matched;
>  
>  	DEBUG(3,("check_reduced_name_with_privilege [%s] [%s]\n",
>  			fname,
> @@ -1077,18 +1076,32 @@ NTSTATUS check_reduced_name_with_privilege(connection_struct *conn,
>  	}
>  
>  	rootdir_len = strlen(conn_rootdir);
> -	matched = (strncmp(conn_rootdir, resolved_name, rootdir_len) == 0);
> -
> -	if (!matched || (resolved_name[rootdir_len] != '/' &&
> -			 resolved_name[rootdir_len] != '\0')) {
> -		DEBUG(2, ("check_reduced_name_with_privilege: Bad access "
> -			"attempt: %s is a symlink outside the "
> -			"share path\n",
> -			dir_name));
> -		DEBUGADD(2, ("conn_rootdir =%s\n", conn_rootdir));
> -		DEBUGADD(2, ("resolved_name=%s\n", resolved_name));
> -		status = NT_STATUS_ACCESS_DENIED;
> -		goto err;
> +
> +	/*
> +	 * In the case of rootdir_len == 1, we know that conn_rootdir is
> +	 * "/", and we also know that resolved_name starts with a slash.
> +	 * So, in this corner case, resolved_name is automatically a
> +	 * sub-directory of the conn_rootdir. Thus we can skip the string
> +	 * comparison and the next character checks (which are even
> +	 * wrong in this case).
> +	 */
> +	if (rootdir_len != 1) {
> +		bool matched;
> +
> +		matched = (strncmp(conn_rootdir, resolved_name,
> +				rootdir_len) == 0);
> +
> +		if (!matched || (resolved_name[rootdir_len] != '/' &&
> +				 resolved_name[rootdir_len] != '\0')) {
> +			DEBUG(2, ("check_reduced_name_with_privilege: Bad "
> +				"access attempt: %s is a symlink outside the "
> +				"share path\n",
> +				dir_name));
> +			DEBUGADD(2, ("conn_rootdir =%s\n", conn_rootdir));
> +			DEBUGADD(2, ("resolved_name=%s\n", resolved_name));
> +			status = NT_STATUS_ACCESS_DENIED;
> +			goto err;
> +		}
>  	}
>  
>  	/* Now ensure that the last component either doesn't
> @@ -1220,7 +1233,6 @@ NTSTATUS check_reduced_name(connection_struct *conn, const char *fname)
>  	if (!allow_widelinks || !allow_symlinks) {
>  		const char *conn_rootdir;
>  		size_t rootdir_len;
> -		bool matched;
>  
>  		conn_rootdir = SMB_VFS_CONNECTPATH(conn, fname);
>  		if (conn_rootdir == NULL) {
> @@ -1231,17 +1243,33 @@ NTSTATUS check_reduced_name(connection_struct *conn, const char *fname)
>  		}
>  
>  		rootdir_len = strlen(conn_rootdir);
> -		matched = (strncmp(conn_rootdir, resolved_name,
> -				rootdir_len) == 0);
> -		if (!matched || (resolved_name[rootdir_len] != '/' &&
> -				 resolved_name[rootdir_len] != '\0')) {
> -			DEBUG(2, ("check_reduced_name: Bad access "
> -				"attempt: %s is a symlink outside the "
> -				"share path\n", fname));
> -			DEBUGADD(2, ("conn_rootdir =%s\n", conn_rootdir));
> -			DEBUGADD(2, ("resolved_name=%s\n", resolved_name));
> -			SAFE_FREE(resolved_name);
> -			return NT_STATUS_ACCESS_DENIED;
> +
> +		/*
> +		 * In the case of rootdir_len == 1, we know that
> +		 * conn_rootdir is "/", and we also know that
> +		 * resolved_name starts with a slash.  So, in this
> +		 * corner case, resolved_name is automatically a
> +		 * sub-directory of the conn_rootdir. Thus we can skip
> +		 * the string comparison and the next character checks
> +		 * (which are even wrong in this case).
> +		 */
> +		if (rootdir_len != 1) {
> +			bool matched;
> +
> +			matched = (strncmp(conn_rootdir, resolved_name,
> +					rootdir_len) == 0);
> +			if (!matched || (resolved_name[rootdir_len] != '/' &&
> +					 resolved_name[rootdir_len] != '\0')) {
> +				DEBUG(2, ("check_reduced_name: Bad access "
> +					"attempt: %s is a symlink outside the "
> +					"share path\n", fname));
> +				DEBUGADD(2, ("conn_rootdir =%s\n",
> +					     conn_rootdir));
> +				DEBUGADD(2, ("resolved_name=%s\n",
> +					     resolved_name));
> +				SAFE_FREE(resolved_name);
> +				return NT_STATUS_ACCESS_DENIED;
> +			}
>  		}
>  
>  		/* Extra checks if all symlinks are disallowed. */
> -- 
> 2.5.0
> 