[PATCH] Fix bug 12721 - CVE-2017-2619 regression with "follow symlinks = no" - Patchset #2

Jeremy Allison jra at samba.org
Tue Mar 28 05:32:04 UTC 2017


On Mon, Mar 27, 2017 at 05:13:44PM -0700, Jeremy Allison via samba-technical wrote:
> 
> There's a second part needed to this bugfix (see
> Ralph's comment on bug:
> 
> https://bugzilla.samba.org/show_bug.cgi?id=12721
> 
> Attached is my proposed patch. Works, but please
> don't push yet until I finish writing the test
> case for it).

OK, here is the version I'm proposing we use
that goes on top of the previous patch you
successfully pushed.

It fixes the second regression issue reported
by the Debian, Ubuntu and Fedora users who
have "follow symlinks = no" in their smb.conf.

See the bug report for Ralph's description of
the problem that allowed me to track this down
and fix it quickly.

The patch has 4 parts:

---------------------------------------------
#1 - Fixup a few minor issues in the previous
test (use correct bash numeric comparisons,
added missing "return").

#2 - Add new "cwd_name" parameter to function
check_reduced_name(). Not yet used.

#3 - The core of the fix. Use the cwd_name
parameter to reconstruct the original
client name passed in for "no symlink" comparison.

#4 - Additional test case that prevents us
from regressing on accessing files/directories
on a share with "follow symlinks = no" set.

Please review and push if happy !

Cheers,

	Jeremy.
-------------- next part --------------
>From ce0efa7d1fcfde14b1af0788f83b369a06a200f2 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Mon, 27 Mar 2017 22:07:50 -0700
Subject: [PATCH 1/4] s3: Fixup test for CVE-2017-2619 regression with "follow
 symlinks = no"

Use correct bash operators (not string operators).
Add missing "return".

BUG: https://bugzilla.samba.org/show_bug.cgi?id=12721

Signed-off-by: Jeremy Allison <jra at samba.org>
---
 source3/script/tests/test_smbclient_s3.sh | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/source3/script/tests/test_smbclient_s3.sh b/source3/script/tests/test_smbclient_s3.sh
index 7d86a61..330ceea 100755
--- a/source3/script/tests/test_smbclient_s3.sh
+++ b/source3/script/tests/test_smbclient_s3.sh
@@ -1123,7 +1123,7 @@ EOF
     ret=$?
     rm -f $tmpfile
 
-    if [ $ret != 0 ] ; then
+    if [ $ret -ne 0 ] ; then
        echo "$out"
        echo "failed accessing nosymlinks with error $ret"
        false
@@ -1132,10 +1132,11 @@ EOF
 
     echo "$out" | grep 'NT_STATUS_ACCESS_DENIED'
     ret=$?
-    if [ $ret != 0 ] ; then
+    if [ $ret -ne 0 ] ; then
        echo "$out"
        echo "failed - should get NT_STATUS_ACCESS_DENIED getting \\nosymlinks\\source"
        false
+       return
     fi
 
 # But we should be able to create and delete directories.
@@ -1150,7 +1151,7 @@ EOF
     ret=$?
     rm -f $tmpfile
 
-    if [ $ret != 0 ] ; then
+    if [ $ret -ne 0 ] ; then
        echo "$out"
        echo "failed accessing nosymlinks with error $ret"
        false
@@ -1159,7 +1160,7 @@ EOF
 
     echo "$out" | grep 'NT_STATUS'
     ret=$?
-    if [ $ret == 0 ] ; then
+    if [ $ret -eq 0 ] ; then
 	echo "$out"
 	echo "failed - NT_STATUS_XXXX doing mkdir a; mkdir a\\b on \\nosymlinks"
 	false
-- 
2.7.4


>From 28e641a86ce61bb01e068d9ad6b147e4a6d6d087 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Mon, 27 Mar 2017 17:04:58 -0700
Subject: [PATCH 2/4] s3: smbd: Fix "follow symlink = no" regression part 2.

Add an extra paramter to cwd_name to check_reduced_name().

If cwd_name == NULL then fname is a client given path relative
to the root path of the share.

If cwd_name != NULL then fname is a client given path relative
to cwd_name. cwd_name is relative to the root path of the share.

Not yet used, logic added in the next commit.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=12721

Signed-off-by: Jeremy Allison <jra at samba.org>
---
 source3/smbd/filename.c |  2 +-
 source3/smbd/open.c     |  2 +-
 source3/smbd/proto.h    |  4 +++-
 source3/smbd/vfs.c      | 10 +++++++++-
 4 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/source3/smbd/filename.c b/source3/smbd/filename.c
index efe52a0..2d85e8d 100644
--- a/source3/smbd/filename.c
+++ b/source3/smbd/filename.c
@@ -1242,7 +1242,7 @@ NTSTATUS check_name(connection_struct *conn, const char *name)
 	}
 
 	if (!lp_widelinks(SNUM(conn)) || !lp_follow_symlinks(SNUM(conn))) {
-		status = check_reduced_name(conn,name);
+		status = check_reduced_name(conn, NULL, name);
 		if (!NT_STATUS_IS_OK(status)) {
 			DEBUG(5,("check_name: name %s failed with %s\n",name,
 						nt_errstr(status)));
diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index a6e61e7..50f8a5e 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -558,7 +558,7 @@ static int non_widelink_open(struct connection_struct *conn,
 	}
 
 	/* Ensure the relative path is below the share. */
-	status = check_reduced_name(conn, final_component);
+	status = check_reduced_name(conn, parent_dir, final_component);
 	if (!NT_STATUS_IS_OK(status)) {
 		saved_errno = map_errno_from_nt_status(status);
 		goto out;
diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h
index 2e8df29..841a095 100644
--- a/source3/smbd/proto.h
+++ b/source3/smbd/proto.h
@@ -1226,7 +1226,9 @@ const char *vfs_readdirname(connection_struct *conn, void *p,
 			    SMB_STRUCT_STAT *sbuf, char **talloced);
 int vfs_ChDir(connection_struct *conn, const char *path);
 char *vfs_GetWd(TALLOC_CTX *ctx, connection_struct *conn);
-NTSTATUS check_reduced_name(connection_struct *conn, const char *fname);
+NTSTATUS check_reduced_name(connection_struct *conn,
+			const char *cwd_name,
+			const char *fname);
 NTSTATUS check_reduced_name_with_privilege(connection_struct *conn,
 			const char *fname,
 			struct smb_request *smbreq);
diff --git a/source3/smbd/vfs.c b/source3/smbd/vfs.c
index 5133fe5..95a8fc1 100644
--- a/source3/smbd/vfs.c
+++ b/source3/smbd/vfs.c
@@ -1179,9 +1179,17 @@ NTSTATUS check_reduced_name_with_privilege(connection_struct *conn,
 /*******************************************************************
  Reduce a file name, removing .. elements and checking that
  it is below dir in the heirachy. This uses realpath.
+
+ If cwd_name == NULL then fname is a client given path relative
+ to the root path of the share.
+
+ If cwd_name != NULL then fname is a client given path relative
+ to cwd_name. cwd_name is relative to the root path of the share.
 ********************************************************************/
 
-NTSTATUS check_reduced_name(connection_struct *conn, const char *fname)
+NTSTATUS check_reduced_name(connection_struct *conn,
+				const char *cwd_name,
+				const char *fname)
 {
 	char *resolved_name = NULL;
 	bool allow_symlinks = true;
-- 
2.7.4


>From c8179ae09c09ca498205d43c7fde02cf680a7871 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Mon, 27 Mar 2017 17:09:38 -0700
Subject: [PATCH 3/4] s3: smbd: Fix "follow symlink = no" regression part 2.

Use the cwd_name parameter to reconstruct the original
client name for symlink testing.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=12721

Signed-off-by: Jeremy Allison <jra at samba.org>
---
 source3/smbd/vfs.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/source3/smbd/vfs.c b/source3/smbd/vfs.c
index 95a8fc1..b7364b7 100644
--- a/source3/smbd/vfs.c
+++ b/source3/smbd/vfs.c
@@ -1192,6 +1192,7 @@ NTSTATUS check_reduced_name(connection_struct *conn,
 				const char *fname)
 {
 	char *resolved_name = NULL;
+	char *new_fname = NULL;
 	bool allow_symlinks = true;
 	bool allow_widelinks = false;
 
@@ -1333,11 +1334,32 @@ NTSTATUS check_reduced_name(connection_struct *conn,
 			}
 
 			p++;
+
+			/*
+			 * If cwd_name is present and not ".",
+			 * then fname is relative to that, not
+			 * the root of the share. Make sure the
+			 * path we check is the one the client
+			 * sent (cwd_name+fname).
+			 */
+			if (cwd_name != NULL && !ISDOT(cwd_name)) {
+				new_fname = talloc_asprintf(talloc_tos(),
+							"%s/%s",
+							cwd_name,
+							fname);
+				if (new_fname == NULL) {
+					SAFE_FREE(resolved_name);
+					return NT_STATUS_NO_MEMORY;
+				}
+				fname = new_fname;
+			}
+
 			if (strcmp(fname, p)!=0) {
 				DEBUG(2, ("check_reduced_name: Bad access "
 					"attempt: %s is a symlink to %s\n",
 					  fname, p));
 				SAFE_FREE(resolved_name);
+				TALLOC_FREE(new_fname);
 				return NT_STATUS_ACCESS_DENIED;
 			}
 		}
@@ -1347,6 +1369,7 @@ NTSTATUS check_reduced_name(connection_struct *conn,
 
 	DBG_INFO("%s reduced to %s\n", fname, resolved_name);
 	SAFE_FREE(resolved_name);
+	TALLOC_FREE(new_fname);
 	return NT_STATUS_OK;
 }
 
-- 
2.7.4


>From 3df3ac07363fa853a254aec4de5e864f528c3e39 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Mon, 27 Mar 2017 22:10:29 -0700
Subject: [PATCH 4/4] s3: Test for CVE-2017-2619 regression with "follow
 symlinks = no" - part 2

Add tests for regular access.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=12721

Signed-off-by: Jeremy Allison <jra at samba.org>
---
 source3/script/tests/test_smbclient_s3.sh | 37 +++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/source3/script/tests/test_smbclient_s3.sh b/source3/script/tests/test_smbclient_s3.sh
index 330ceea..9bff883 100755
--- a/source3/script/tests/test_smbclient_s3.sh
+++ b/source3/script/tests/test_smbclient_s3.sh
@@ -1103,14 +1103,22 @@ test_nosymlinks()
     slink_name="$LOCAL_PATH/nosymlinks/source"
     slink_target="$LOCAL_PATH/nosymlinks/target"
     mkdir_target="$LOCAL_PATH/nosymlinks/a"
+    dir1="$LOCAL_PATH/nosymlinks/foo"
+    dir2="$LOCAL_PATH/nosymlinks/foo/bar"
+    get_target="$LOCAL_PATH/nosymlinks/foo/bar/testfile"
 
     rm -f $slink_target
     rm -f $slink_name
     rm -rf $mkdir_target
+    rm -rf $dir1
 
     touch $slink_target
     ln -s $slink_target $slink_name
 
+    mkdir $dir1
+    mkdir $dir2
+    touch $get_target
+
 # Getting a file through a symlink name should fail.
     tmpfile=$PREFIX/smbclient_interactive_prompt_commands
     cat > $tmpfile <<EOF
@@ -1165,6 +1173,35 @@ EOF
 	echo "failed - NT_STATUS_XXXX doing mkdir a; mkdir a\\b on \\nosymlinks"
 	false
     fi
+
+# Ensure regular file/directory access also works.
+    cat > $tmpfile <<EOF
+cd foo\\bar
+ls
+get testfile -
+quit
+EOF
+    cmd='CLI_FORCE_INTERACTIVE=yes $SMBCLIENT "$@" -U$USERNAME%$PASSWORD //$SERVER/nosymlinks -I $SERVER_IP $ADDARGS < $tmpfile 2>&1'
+    eval echo "$cmd"
+    out=`eval $cmd`
+    ret=$?
+    rm -f $tmpfile
+
+    if [ $ret -ne 0 ] ; then
+       echo "$out"
+       echo "failed accessing nosymlinks with error $ret"
+       false
+       return
+    fi
+
+    echo "$out" | grep 'NT_STATUS'
+    ret=$?
+    if [ $ret -eq 0 ] ; then
+       echo "$out"
+       echo "failed - NT_STATUS_XXXX doing cd foo\\bar; get testfile on \\nosymlinks"
+       false
+       return
+    fi
 }
 
 LOGDIR_PREFIX=test_smbclient_s3
-- 
2.7.4



More information about the samba-technical mailing list