From 5e94fe726e9af81374c697ce603b3728ccaaebf3 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra@samba.org>
Date: Fri, 12 Jul 2019 12:10:35 -0700
Subject: [PATCH 1/6] CVE-2019-10197: smbd: separate out impersonation debug
info into a new function.
Will be called on elsewhere on successful impersonation.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=14035
Signed-off-by: Jeremy Allison <jra@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
Reviewed-by: Stefan Metzmacher <metze@samba.org>
---
source3/smbd/uid.c | 37 +++++++++++++++++++++++--------------
1 file changed, 23 insertions(+), 14 deletions(-)
diff --git a/source3/smbd/uid.c b/source3/smbd/uid.c
index a4bcb747d37e..ce8e8d92131c 100644
--- a/source3/smbd/uid.c
+++ b/source3/smbd/uid.c
@@ -279,6 +279,28 @@ static bool check_user_ok(connection_struct *conn,
return(True);
}
+static void print_impersonation_info(connection_struct *conn)
+{
+ struct smb_filename *cwdfname = NULL;
+
+ if (!CHECK_DEBUGLVL(DBGLVL_INFO)) {
+ return;
+ }
+
+ cwdfname = vfs_GetWd(talloc_tos(), conn);
+ if (cwdfname == NULL) {
+ return;
+ }
+
+ DBG_INFO("Impersonated user: uid=(%d,%d), gid=(%d,%d), cwd=[%s]\n",
+ (int)getuid(),
+ (int)geteuid(),
+ (int)getgid(),
+ (int)getegid(),
+ cwdfname->base_name);
+ TALLOC_FREE(cwdfname);
+}
+
/****************************************************************************
Become the user of a connection number without changing the security context
stack, but modify the current_user entries.
@@ -415,20 +437,7 @@ static bool change_to_user_internal(connection_struct *conn,
current_user.done_chdir = true;
}
- if (CHECK_DEBUGLVL(DBGLVL_INFO)) {
- struct smb_filename *cwdfname = vfs_GetWd(talloc_tos(), conn);
- if (cwdfname == NULL) {
- return false;
- }
- DBG_INFO("Impersonated user: uid=(%d,%d), gid=(%d,%d), cwd=[%s]\n",
- (int)getuid(),
- (int)geteuid(),
- (int)getgid(),
- (int)getegid(),
- cwdfname->base_name);
- TALLOC_FREE(cwdfname);
- }
-
+ print_impersonation_info(conn);
return true;
}
--
2.17.1
From b4cd0dcbc38ae61cfb075e5f659384df889e99f7 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze@samba.org>
Date: Thu, 11 Jul 2019 17:01:29 +0200
Subject: [PATCH 2/6] CVE-2019-10197: smbd: make sure that
change_to_user_internal() always resets current_user.done_chdir
We should not leave current_user.done_chdir as true if we didn't call
chdir_current_service() with success.
This caused problems in when calling vfs_ChDir() in pop_conn_ctx() when
chdir_current_service() worked once on one share but later failed on another
share.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=14035
Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
---
source3/smbd/uid.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/source3/smbd/uid.c b/source3/smbd/uid.c
index ce8e8d92131c..77a81f602988 100644
--- a/source3/smbd/uid.c
+++ b/source3/smbd/uid.c
@@ -427,6 +427,7 @@ static bool change_to_user_internal(connection_struct *conn,
current_user.conn = conn;
current_user.vuid = vuid;
current_user.need_chdir = conn->tcon_done;
+ current_user.done_chdir = false;
if (current_user.need_chdir) {
ok = chdir_current_service(conn);
--
2.17.1
From b1496ce793129302c9959ebc6330219c6a3143f0 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze@samba.org>
Date: Tue, 18 Jun 2019 14:04:08 +0200
Subject: [PATCH 3/6] CVE-2019-10197: smbd: make sure we reset
current_user.{need,done}_chdir in become_root()
BUG: https://bugzilla.samba.org/show_bug.cgi?id=14035
Signed-off-by: Stefan Metzmacher <metze@samba.org>
---
source3/smbd/uid.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/source3/smbd/uid.c b/source3/smbd/uid.c
index 77a81f602988..50868ba8572a 100644
--- a/source3/smbd/uid.c
+++ b/source3/smbd/uid.c
@@ -624,6 +624,9 @@ void smbd_become_root(void)
}
push_conn_ctx();
set_root_sec_ctx();
+
+ current_user.need_chdir = false;
+ current_user.done_chdir = false;
}
/* Unbecome the root user */
--
2.17.1
From 03a0719d6d5c1a81b44bc3cedc76563a1eb04491 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze@samba.org>
Date: Tue, 30 Jul 2019 17:16:59 +0200
Subject: [PATCH 4/6] CVE-2019-10197: selftest: make fsrvp_share its own
independent subdirectory
The next patch will otherwise break the fsrvp related tests.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=14035
Signed-off-by: Stefan Metzmacher <metze@samba.org>
---
selftest/target/Samba3.pm | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm
index 9d88253c9fe7..f7eb314138a0 100755
--- a/selftest/target/Samba3.pm
+++ b/selftest/target/Samba3.pm
@@ -1540,6 +1540,9 @@ sub provision($$$$$$$$$)
my $widelinks_linkdir="$shrdir/widelinks_foo";
push(@dirs,$widelinks_linkdir);
+ my $fsrvp_shrdir="$shrdir/fsrvp";
+ push(@dirs,$fsrvp_shrdir);
+
my $shadow_tstdir="$shrdir/shadow";
push(@dirs,$shadow_tstdir);
my $shadow_mntdir="$shadow_tstdir/mount";
@@ -2083,14 +2086,14 @@ sub provision($$$$$$$$$)
guest ok = yes
[fsrvp_share]
- path = $shrdir
+ path = $fsrvp_shrdir
comment = fake shapshots using rsync
vfs objects = shell_snap shadow_copy2
shell_snap:check path command = $fake_snap_pl --check
shell_snap:create command = $fake_snap_pl --create
shell_snap:delete command = $fake_snap_pl --delete
# a relative path here fails, the snapshot dir is no longer found
- shadow:snapdir = $shrdir/.snapshots
+ shadow:snapdir = $fsrvp_shrdir/.snapshots
[shadow1]
path = $shadow_shrdir
--
2.17.1
From 409447f3258b87745a2248570278b1c6da8991f4 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze@samba.org>
Date: Tue, 16 Jul 2019 15:40:38 +0200
Subject: [PATCH 5/6] CVE-2019-10197: test_smbclient_s3.sh: add regression test
for the no permission on share root problem
BUG: https://bugzilla.samba.org/show_bug.cgi?id=14035
Signed-off-by: Stefan Metzmacher <metze@samba.org>
---
selftest/knownfail.d/CVE-2019-10197 | 1 +
selftest/target/Samba3.pm | 12 +++++++++
source3/script/tests/test_smbclient_s3.sh | 30 +++++++++++++++++++++++
3 files changed, 43 insertions(+)
create mode 100644 selftest/knownfail.d/CVE-2019-10197
diff --git a/selftest/knownfail.d/CVE-2019-10197 b/selftest/knownfail.d/CVE-2019-10197
new file mode 100644
index 000000000000..f7056bbf3ad4
--- /dev/null
+++ b/selftest/knownfail.d/CVE-2019-10197
@@ -0,0 +1 @@
+^samba3.blackbox.smbclient_s3.*.noperm.share.regression
diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm
index f7eb314138a0..2f491441815f 100755
--- a/selftest/target/Samba3.pm
+++ b/selftest/target/Samba3.pm
@@ -1516,6 +1516,9 @@ sub provision($$$$$$$$$)
my $ro_shrdir="$shrdir/root-tmp";
push(@dirs,$ro_shrdir);
+ my $noperm_shrdir="$shrdir/noperm-tmp";
+ push(@dirs,$noperm_shrdir);
+
my $msdfs_shrdir="$shrdir/msdfsshare";
push(@dirs,$msdfs_shrdir);
@@ -1586,6 +1589,11 @@ sub provision($$$$$$$$$)
chmod 0755, $piddir;
+ ##
+ ## Create a directory without permissions to enter
+ ##
+ chmod 0000, $noperm_shrdir;
+
##
## create ro and msdfs share layout
##
@@ -1902,6 +1910,10 @@ sub provision($$$$$$$$$)
[ro-tmp]
path = $ro_shrdir
guest ok = yes
+[noperm]
+ path = $noperm_shrdir
+ wide links = yes
+ guest ok = yes
[write-list-tmp]
path = $shrdir
read only = yes
diff --git a/source3/script/tests/test_smbclient_s3.sh b/source3/script/tests/test_smbclient_s3.sh
index bf033ccd2fbf..0bae1d78fac9 100755
--- a/source3/script/tests/test_smbclient_s3.sh
+++ b/source3/script/tests/test_smbclient_s3.sh
@@ -1329,6 +1329,32 @@ EOF
fi
}
+#
+# Regression test for CVE-2019-10197
+# we should always get ACCESS_DENIED
+#
+test_noperm_share_regression()
+{
+ cmd='$SMBCLIENT -U$USERNAME%$PASSWORD //$SERVER/noperm -I $SERVER_IP $LOCAL_ADDARGS -c "ls;ls" 2>&1'
+ eval echo "$cmd"
+ out=`eval $cmd`
+ ret=$?
+ if [ $ret -eq 0 ] ; then
+ echo "$out"
+ echo "failed accessing no perm share should not work"
+ return 1
+ fi
+
+ num=`echo "$out" | grep 'NT_STATUS_ACCESS_DENIED' | wc -l`
+ if [ "$num" -ne "2" ] ; then
+ echo "$out"
+ echo "failed num[$num] - two NT_STATUS_ACCESS_DENIED lines expected"
+ return 1
+ fi
+
+ return 0
+}
+
# Test smbclient deltree command
test_deltree()
{
@@ -1857,6 +1883,10 @@ testit "follow local symlinks" \
test_local_symlinks || \
failed=`expr $failed + 1`
+testit "noperm share regression" \
+ test_noperm_share_regression || \
+ failed=`expr $failed + 1`
+
testit "smbclient deltree command" \
test_deltree || \
failed=`expr $failed + 1`
--
2.17.1
From 501e034aa5b6ba50bf14e41c59674fbbc28a2e9c Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze@samba.org>
Date: Thu, 11 Jul 2019 17:02:15 +0200
Subject: [PATCH 6/6] CVE-2019-10197: smbd: split change_to_user_impersonate()
out of change_to_user_internal()
This makes sure we always call chdir_current_service() even
when we still impersonated the user. Which is important
in order to run the SMB* request within the correct working directory
and only if the user has permissions to enter that directory.
It makes sure we always update conn->lastused_count
in chdir_current_service() for each request.
Note that vfs_ChDir() (called from chdir_current_service())
maintains its own cache and avoids calling SMB_VFS_CHDIR()
if possible.
It means we still avoid syscalls if we get a multiple requests
for the same session/tcon tuple.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=14035
Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Ralph Boehme <slow@samba.org>
---
selftest/knownfail.d/CVE-2019-10197 | 1 -
source3/smbd/uid.c | 21 +++++++++++++++++----
2 files changed, 17 insertions(+), 5 deletions(-)
delete mode 100644 selftest/knownfail.d/CVE-2019-10197
diff --git a/selftest/knownfail.d/CVE-2019-10197 b/selftest/knownfail.d/CVE-2019-10197
deleted file mode 100644
index f7056bbf3ad4..000000000000
--- a/selftest/knownfail.d/CVE-2019-10197
+++ /dev/null
@@ -1 +0,0 @@
-^samba3.blackbox.smbclient_s3.*.noperm.share.regression
diff --git a/source3/smbd/uid.c b/source3/smbd/uid.c
index 50868ba8572a..5c39baade5cf 100644
--- a/source3/smbd/uid.c
+++ b/source3/smbd/uid.c
@@ -306,9 +306,9 @@ static void print_impersonation_info(connection_struct *conn)
stack, but modify the current_user entries.
****************************************************************************/
-static bool change_to_user_internal(connection_struct *conn,
- const struct auth_session_info *session_info,
- uint64_t vuid)
+static bool change_to_user_impersonate(connection_struct *conn,
+ const struct auth_session_info *session_info,
+ uint64_t vuid)
{
int snum;
gid_t gid;
@@ -321,7 +321,6 @@ static bool change_to_user_internal(connection_struct *conn,
if ((current_user.conn == conn) &&
(current_user.vuid == vuid) &&
- (current_user.need_chdir == conn->tcon_done) &&
(current_user.ut.uid == session_info->unix_token->uid))
{
DBG_INFO("Skipping user change - already user\n");
@@ -426,6 +425,20 @@ static bool change_to_user_internal(connection_struct *conn,
current_user.conn = conn;
current_user.vuid = vuid;
+ return true;
+}
+
+static bool change_to_user_internal(connection_struct *conn,
+ const struct auth_session_info *session_info,
+ uint64_t vuid)
+{
+ bool ok;
+
+ ok = change_to_user_impersonate(conn, session_info, vuid);
+ if (!ok) {
+ return false;
+ }
+
current_user.need_chdir = conn->tcon_done;
current_user.done_chdir = false;
--
2.17.1