[PATCH] Fix bug 12721 - CVE-2017-2619 regression with "follow symlinks = no"
Jeremy Allison
jra at samba.org
Mon Mar 27 19:23:38 UTC 2017
This one was really interesting. It actually isn't
related (directly) to the CVE-2017-2619 fix. That
fix exposed an incorrect assumption that Samba has
had for a few years. Quite simply, on a POSIX
filesystem the directory entries "." and ".."
by design can *NEVER* be symlinks - and we were treating
them as if they could be.
Fix and regression test attached.
Please review and push if happy, and I'll back-port
for the production releases.
Jeremy.
-------------- next part --------------
From 9ed3c05b23e4315992e9a5a6554b2869ef9dc8e6 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Mon, 27 Mar 2017 10:46:47 -0700
Subject: [PATCH 1/2] 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 at samba.org>
---
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 35f560b8676..5133fe5c2fd 100644
--- a/source3/smbd/vfs.c
+++ b/source3/smbd/vfs.c
@@ -1307,8 +1307,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.1.578.ge9c3154ca4-goog
From 87c7b8a440be9a3fcab829c8e6e8c0a63c20a2f8 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Mon, 27 Mar 2017 11:48:25 -0700
Subject: [PATCH 2/2] 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 at samba.org>
---
selftest/target/Samba3.pm | 8 ++++
source3/script/tests/test_smbclient_s3.sh | 73 +++++++++++++++++++++++++++++++
2 files changed, 81 insertions(+)
diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm
index d754b5f9ac1..354f1527a70 100755
--- a/selftest/target/Samba3.pm
+++ b/selftest/target/Samba3.pm
@@ -1245,6 +1245,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";
@@ -1861,6 +1864,11 @@ sub provision($$$$$$$$)
copy = tmp
mangled names = illegal
+[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 22849bd5031..7d86a6134ae 100755
--- a/source3/script/tests/test_smbclient_s3.sh
+++ b/source3/script/tests/test_smbclient_s3.sh
@@ -1096,6 +1096,75 @@ EOF
fi
}
+# 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
+}
LOGDIR_PREFIX=test_smbclient_s3
@@ -1196,6 +1265,10 @@ testit "streams_depot can delete correctly" \
test_streams_depot_delete || \
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.1.578.ge9c3154ca4-goog
More information about the samba-technical
mailing list