From 5e94fe726e9af81374c697ce603b3728ccaaebf3 Mon Sep 17 00:00:00 2001 From: Jeremy Allison 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 Reviewed-by: Ralph Boehme Reviewed-by: Stefan Metzmacher --- 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 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 Reviewed-by: Ralph Boehme --- 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 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 --- 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 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 --- 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 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 --- 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 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 Reviewed-by: Ralph Boehme --- 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