[PATCH] s3/smbd: use stat from smb_fname if valid in refuse_symlink()

[Jeremy Allison]

On Sun, Sep 11, 2016 at 03:53:16PM +0200, Ralph Böhme wrote:
> On Sun, Sep 11, 2016 at 01:42:54PM +0200, Ralph Böhme wrote:
> > On Sun, Sep 11, 2016 at 01:09:43PM +0200, Ralph Böhme wrote:
> > > I think we can safely save one stat call in refuse_symlink(). Please
> > > review carefully & push if ok.
> > > 
> > > refuse_symlink() was added as part of CVE-2015-7560, bug 11648 in
> > > commit b551cd83ef74340adaf88629a9ee9fa5c5215ec6 taking a char *path
> > > and an fsp, so obviously a stat optimisation could only be done for
> > > the case a valid fsp was passed.
> > > 
> > > A later change in 13dae2b46ed9a53b7eeed4ce125478b5bbb3e2b5 changed the
> > > function signature to take a struct smb_filename * instead of a char *.
> > 
> > it fails the POSIX-SYMLINK-EA test. :/ Glad we have it! Looking
> > into it.
> 
> got it, looks like we have a bug in call_trans2qfilepathinfo() for
> requests with POSIX paths: we're calling SMB_VFS_STAT() instead of
> lstat on the smb_filename, even though the request is in POSIX
> context. Is this correct? I don't think so.
> 
> Not sure how bad this is, but changing the check that switches between
> stat and lstat to do an lstat for UNIX info levels *and* if the
> request if flagged as using POSIX pathnames fixes my refuse_symlink()
> optimisation.
> 
> Please review & push if ok. Passes smbtorture_s3 tests, currently
> running a complete autobuild.

LGTM - we will need a bugid for this to backport to 4.5.1.

Good catch !

Cheers,

	Jeremy.


> From 399d53a63322af12c63136a90066d49f137ef97a Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Sun, 11 Sep 2016 15:35:37 +0200
> Subject: [PATCH 1/2] s3/smbd: in call_trans2qfilepathinfo call lstat when
>  dealing with posix pathnames
> 
> This might be an info level SMB_INFO_QUERY_ALL_EAS which is not covered
> by INFO_LEVEL_IS_UNIX(). If smb_fname is a symlink we would then stat it
> in POSIX context.
> 
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
>  source3/smbd/trans2.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/source3/smbd/trans2.c b/source3/smbd/trans2.c
> index 1775316..a01bb81 100644
> --- a/source3/smbd/trans2.c
> +++ b/source3/smbd/trans2.c
> @@ -5767,7 +5767,8 @@ static void call_trans2qfilepathinfo(connection_struct *conn,
>  			}
>  			if (info_level == SMB_QUERY_FILE_UNIX_BASIC ||
>  					info_level == SMB_QUERY_FILE_UNIX_INFO2 ||
> -					info_level == SMB_QUERY_FILE_UNIX_LINK) {
> +					info_level == SMB_QUERY_FILE_UNIX_LINK ||
> +					req->posix_pathnames) {
>  				ucf_flags |= UCF_UNIX_NAME_LOOKUP;
>  			}
>  		}
> @@ -5831,7 +5832,7 @@ static void call_trans2qfilepathinfo(connection_struct *conn,
>  				return;
>  			}
>  
> -			if (INFO_LEVEL_IS_UNIX(info_level)) {
> +			if (INFO_LEVEL_IS_UNIX(info_level) || req->posix_pathnames) {
>  				/* Always do lstat for UNIX calls. */
>  				if (SMB_VFS_LSTAT(conn, smb_fname_base) != 0) {
>  					DEBUG(3,("call_trans2qfilepathinfo: "
> @@ -5877,7 +5878,7 @@ static void call_trans2qfilepathinfo(connection_struct *conn,
>  			}
>  		}
>  
> -		if (INFO_LEVEL_IS_UNIX(info_level)) {
> +		if (INFO_LEVEL_IS_UNIX(info_level) || req->posix_pathnames) {
>  			/* Always do lstat for UNIX calls. */
>  			if (SMB_VFS_LSTAT(conn, smb_fname)) {
>  				DEBUG(3,("call_trans2qfilepathinfo: "
> -- 
> 2.7.4
> 
> 
> From bf752b5c85a132d29c256821e9671a3b74e99bd5 Mon Sep 17 00:00:00 2001
> From: Ralph Boehme <slow at samba.org>
> Date: Sat, 10 Sep 2016 14:43:07 +0200
> Subject: [PATCH 2/2] s3/smbd: use stat from smb_fname if valid in
>  refuse_symlink()
> 
> Now that refuse_symlink() gets passed in a smb_fname and not just a char
> buffer, we can try to reuse its stat info and save one stat call here.
> 
> Signed-off-by: Ralph Boehme <slow at samba.org>
> ---
>  source3/smbd/trans2.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/source3/smbd/trans2.c b/source3/smbd/trans2.c
> index a01bb81..6999b2d 100644
> --- a/source3/smbd/trans2.c
> +++ b/source3/smbd/trans2.c
> @@ -55,7 +55,7 @@ static char *store_file_unix_basic_info2(connection_struct *conn,
>  				const SMB_STRUCT_STAT *psbuf);
>  
>  /****************************************************************************
> - Check if an open file handle or pathname is a symlink.
> + Check if an open file handle or smb_fname is a symlink.
>  ****************************************************************************/
>  
>  static NTSTATUS refuse_symlink(connection_struct *conn,
> @@ -68,6 +68,10 @@ static NTSTATUS refuse_symlink(connection_struct *conn,
>  	if (fsp) {
>  		pst = &fsp->fsp_name->st;
>  	} else {
> +		pst = &smb_fname->st;
> +	}
> +
> +	if (!VALID_STAT(*pst)) {
>  		int ret = vfs_stat_smb_basename(conn,
>  				smb_fname,
>  				&sbuf);
> @@ -76,6 +80,7 @@ static NTSTATUS refuse_symlink(connection_struct *conn,
>  		}
>  		pst = &sbuf;
>  	}
> +
>  	if (S_ISLNK(pst->st_ex_mode)) {
>  		return NT_STATUS_ACCESS_DENIED;
>  	}
> -- 
> 2.7.4
>