From b7d9500a6d1f54d29338929dd8d0f0863ad995b4 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 ced2d450f8e7..89f539ed4300 100644 --- a/source3/smbd/uid.c +++ b/source3/smbd/uid.c @@ -284,6 +284,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. @@ -420,20 +442,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 632fdf8947285c70650ce5a849349a86c4dc294e 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 89f539ed4300..b6ef02a36b3a 100644 --- a/source3/smbd/uid.c +++ b/source3/smbd/uid.c @@ -432,6 +432,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 830dfe13ccdf5d12e3f04a2f6068ae2d40fb71cf 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 b6ef02a36b3a..9cf09c11a622 100644 --- a/source3/smbd/uid.c +++ b/source3/smbd/uid.c @@ -629,6 +629,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 6ff1e7677208774e0b973457c50f48965871f117 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 142523441751..5e9daf3eec46 100755 --- a/selftest/target/Samba3.pm +++ b/selftest/target/Samba3.pm @@ -1474,6 +1474,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"; @@ -2009,14 +2012,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 99352a814b086fef25af184a357cb027cabc7b59 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 5e9daf3eec46..22e5035b0794 100755 --- a/selftest/target/Samba3.pm +++ b/selftest/target/Samba3.pm @@ -1450,6 +1450,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); @@ -1520,6 +1523,11 @@ sub provision($$$$$$$$$) chmod 0755, $piddir; + ## + ## Create a directory without permissions to enter + ## + chmod 0000, $noperm_shrdir; + ## ## create ro and msdfs share layout ## @@ -1828,6 +1836,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 9c8ebfb8488a3583a3a42b49d6cdadb923f35c3e 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 9cf09c11a622..f65e3091dfd9 100644 --- a/source3/smbd/uid.c +++ b/source3/smbd/uid.c @@ -311,9 +311,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; @@ -326,7 +326,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"); @@ -431,6 +430,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