From ce0efa7d1fcfde14b1af0788f83b369a06a200f2 Mon Sep 17 00:00:00 2001 From: Jeremy Allison 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 --- 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 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 --- 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 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 --- 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 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 --- 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 < $tmpfile <