The Samba-Bugzilla – Attachment 13118 Details for
Bug 12721
CVE-2017-2619 regression with "follow symlinks = no"
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
git-am fix for 4.4.next.
bug-12721-v4.4 (text/plain), 15.38 KB, created by
Jeremy Allison
on 2017-03-28 16:35:28 UTC
(
hide
)
Description:
git-am fix for 4.4.next.
Filename:
MIME Type:
Creator:
Jeremy Allison
Created:
2017-03-28 16:35:28 UTC
Size:
15.38 KB
patch
obsolete
>From ba78ec3ccbc26b26ae15de5c1a03d61f58756256 Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Mon, 27 Mar 2017 10:46:47 -0700 >Subject: [PATCH 1/6] s3: smbd: Fix incorrect logic exposed by fix for the > security bug 12496 (CVE-2017-2619). > >In a UNIX filesystem, the names "." and ".." by definition can *never* >be symlinks - they are already reserved names. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=12721 > >Signed-off-by: Jeremy Allison <jra@samba.org> >Reviewed-by: Uri Simchoni <uri@samba.org> >(cherry picked from commit ae17bebd250bdde5614b2ac17e53512f19fe9b68) >--- > source3/smbd/vfs.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > >diff --git a/source3/smbd/vfs.c b/source3/smbd/vfs.c >index 93726bd0f67..c358f78408f 100644 >--- a/source3/smbd/vfs.c >+++ b/source3/smbd/vfs.c >@@ -1277,8 +1277,11 @@ NTSTATUS check_reduced_name(connection_struct *conn, const char *fname) > /* fname can't have changed in resolved_path. */ > const char *p = &resolved_name[rootdir_len]; > >- /* *p can be '\0' if fname was "." */ >- if (*p == '\0' && ISDOT(fname)) { >+ /* >+ * UNIX filesystem semantics, names consisting >+ * only of "." or ".." CANNOT be symlinks. >+ */ >+ if (ISDOT(fname) || ISDOTDOT(fname)) { > goto out; > } > >-- >2.12.2.564.g063fe858b8-goog > > >From d6e8710cf25e77b25f51b75fcee7807c576ca8bd Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Mon, 27 Mar 2017 11:48:25 -0700 >Subject: [PATCH 2/6] s3: Test for CVE-2017-2619 regression with "follow > symlinks = no". > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=12721 > >Signed-off-by: Jeremy Allison <jra@samba.org> >Reviewed-by: Uri Simchoni <uri@samba.org> > >Back-ported from commit 782172a9bef0040981d20e49519b13dd744df6a0 >--- > selftest/target/Samba3.pm | 7 +++ > source3/script/tests/test_smbclient_s3.sh | 73 +++++++++++++++++++++++++++++++ > 2 files changed, 80 insertions(+) > >diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm >index 619ae1eafaa..938c4595552 100755 >--- a/selftest/target/Samba3.pm >+++ b/selftest/target/Samba3.pm >@@ -1191,6 +1191,9 @@ sub provision($$$$$$$$) > my $shadow_shrdir="$shadow_basedir/share"; > push(@dirs,$shadow_shrdir); > >+ my $nosymlinks_shrdir="$shrdir/nosymlinks"; >+ push(@dirs,$nosymlinks_shrdir); >+ > # this gets autocreated by winbindd > my $wbsockdir="$prefix_abs/winbindd"; > my $wbsockprivdir="$lockdir/winbindd_privileged"; >@@ -1717,6 +1720,10 @@ sub provision($$$$$$$$) > copy = tmp > acl_xattr:ignore system acls = yes > acl_xattr:default acl style = windows >+[nosymlinks] >+ copy = tmp >+ path = $nosymlinks_shrdir >+ follow symlinks = no > [kernel_oplocks] > copy = tmp > kernel oplocks = yes >diff --git a/source3/script/tests/test_smbclient_s3.sh b/source3/script/tests/test_smbclient_s3.sh >index 5e3db5d365b..f13fd348fe5 100755 >--- a/source3/script/tests/test_smbclient_s3.sh >+++ b/source3/script/tests/test_smbclient_s3.sh >@@ -1071,6 +1071,75 @@ done > > LOGDIR=$(mktemp -d ${PREFIX}/${LOGDIR_PREFIX}_XXXXXX) > >+# Test follow symlinks can't access symlinks >+test_nosymlinks() >+{ >+# Setup test dirs. >+ slink_name="$LOCAL_PATH/nosymlinks/source" >+ slink_target="$LOCAL_PATH/nosymlinks/target" >+ mkdir_target="$LOCAL_PATH/nosymlinks/a" >+ >+ rm -f $slink_target >+ rm -f $slink_name >+ rm -rf $mkdir_target >+ >+ touch $slink_target >+ ln -s $slink_target $slink_name >+ >+# Getting a file through a symlink name should fail. >+ tmpfile=$PREFIX/smbclient_interactive_prompt_commands >+ cat > $tmpfile <<EOF >+get source >+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 != 0 ] ; then >+ echo "$out" >+ echo "failed accessing nosymlinks with error $ret" >+ false >+ return >+ fi >+ >+ echo "$out" | grep 'NT_STATUS_ACCESS_DENIED' >+ ret=$? >+ if [ $ret != 0 ] ; then >+ echo "$out" >+ echo "failed - should get NT_STATUS_ACCESS_DENIED getting \\nosymlinks\\source" >+ false >+ fi >+ >+# But we should be able to create and delete directories. >+ cat > $tmpfile <<EOF >+mkdir a >+mkdir a\\b >+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 != 0 ] ; then >+ echo "$out" >+ echo "failed accessing nosymlinks with error $ret" >+ false >+ return >+ fi >+ >+ echo "$out" | grep 'NT_STATUS' >+ ret=$? >+ if [ $ret == 0 ] ; then >+ echo "$out" >+ echo "failed - NT_STATUS_XXXX doing mkdir a; mkdir a\\b on \\nosymlinks" >+ false >+ fi >+} > > testit "smbclient -L $SERVER_IP" $SMBCLIENT -L $SERVER_IP -N -p 139 || failed=`expr $failed + 1` > testit "smbclient -L $SERVER -I $SERVER_IP" $SMBCLIENT -L $SERVER -I $SERVER_IP -N -p 139 -c quit || failed=`expr $failed + 1` >@@ -1155,6 +1224,10 @@ testit "Ensure widelinks are restricted" \ > test_widelinks || \ > failed=`expr $failed + 1` > >+testit "follow symlinks = no" \ >+ test_nosymlinks || \ >+ failed=`expr $failed + 1` >+ > testit "rm -rf $LOGDIR" \ > rm -rf $LOGDIR || \ > failed=`expr $failed + 1` >-- >2.12.2.564.g063fe858b8-goog > > >From 05aff624687c7d1b316b6bbf6535d20eba34bc1c Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Mon, 27 Mar 2017 22:07:50 -0700 >Subject: [PATCH 3/6] 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@samba.org> >Reviewed-by: Ralph Boehme <slow@samba.org> >(cherry picked from commit 037297a1c50e90a0092e3b94f472623f41ccc015) >--- > 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 f13fd348fe5..bf55d0207d6 100755 >--- a/source3/script/tests/test_smbclient_s3.sh >+++ b/source3/script/tests/test_smbclient_s3.sh >@@ -1098,7 +1098,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 >@@ -1107,10 +1107,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. >@@ -1125,7 +1126,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 >@@ -1134,7 +1135,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.12.2.564.g063fe858b8-goog > > >From ee09803f70b6a9826c95db6fb2da253278d079b3 Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Mon, 27 Mar 2017 17:04:58 -0700 >Subject: [PATCH 4/6] 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@samba.org> >Reviewed-by: Ralph Boehme <slow@samba.org> >(cherry picked from commit 83e30cb48859b412b76572b6a3ba84d8fde167af) >--- > 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 046ce9c8391..9f72f99896a 100644 >--- a/source3/smbd/filename.c >+++ b/source3/smbd/filename.c >@@ -1240,7 +1240,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 0b6648739fb..f19af87103b 100644 >--- a/source3/smbd/open.c >+++ b/source3/smbd/open.c >@@ -546,7 +546,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 ebccdb94d02..9103e6eb1bf 100644 >--- a/source3/smbd/proto.h >+++ b/source3/smbd/proto.h >@@ -1197,7 +1197,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 c358f78408f..22054674435 100644 >--- a/source3/smbd/vfs.c >+++ b/source3/smbd/vfs.c >@@ -1149,9 +1149,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.12.2.564.g063fe858b8-goog > > >From 2e20206fd8b9715989a461bb14653e114d6b601f Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Mon, 27 Mar 2017 17:09:38 -0700 >Subject: [PATCH 5/6] 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@samba.org> >Reviewed-by: Ralph Boehme <slow@samba.org> >(cherry picked from commit e182a4d39e86c9694e255efdf6ee2ea3ccb9af4a) >--- > source3/smbd/vfs.c | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > >diff --git a/source3/smbd/vfs.c b/source3/smbd/vfs.c >index 22054674435..254337f05b8 100644 >--- a/source3/smbd/vfs.c >+++ b/source3/smbd/vfs.c >@@ -1162,6 +1162,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; > >@@ -1303,11 +1304,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; > } > } >@@ -1317,6 +1339,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.12.2.564.g063fe858b8-goog > > >From ccf2dffd0856b57a461b29bba3a3f3e204ae069e Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Mon, 27 Mar 2017 22:10:29 -0700 >Subject: [PATCH 6/6] s3: Test for CVE-2017-2619 regression with "follow > symlinks = no" - part 2 >MIME-Version: 1.0 >Content-Type: text/plain; charset=UTF-8 >Content-Transfer-Encoding: 8bit > >Add tests for regular access. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=12721 > >Signed-off-by: Jeremy Allison <jra@samba.org> >Reviewed-by: Ralph Boehme <slow@samba.org> > >Autobuild-User(master): Ralph Böhme <slow@samba.org> >Autobuild-Date(master): Tue Mar 28 17:05:27 CEST 2017 on sn-devel-144 > >(cherry picked from commit 4e734fcd1bf82c08aa303ce44e9735acccffcf06) >--- > 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 bf55d0207d6..0694e1b1d98 100755 >--- a/source3/script/tests/test_smbclient_s3.sh >+++ b/source3/script/tests/test_smbclient_s3.sh >@@ -1078,14 +1078,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 >@@ -1140,6 +1148,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 > } > > testit "smbclient -L $SERVER_IP" $SMBCLIENT -L $SERVER_IP -N -p 139 || failed=`expr $failed + 1` >-- >2.12.2.564.g063fe858b8-goog >
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=13118">View the attachment on a separate page</a>.</b>
View Attachment As Raw
Flags:
slow
:
review+
Actions:
View
Attachments on
bug 12721
:
13109
|
13110
|
13111
|
13112
|
13117
| 13118 |
13121
|
13128
|
13130