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

[Ralph Böhme]

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.

Cheerio!
-slow
-------------- next part --------------
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