From 2a75d84965be70690da084c818c0778fe0e9330f Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 28 Jan 2021 14:07:23 -0800 Subject: [PATCH 1/5] s3: tests: Add regression test for bug 13992. Subtle extra test. Mark as knownfail for now. '^ user1$' must appear MORE THAN ONCE, as it can read more than one share. The previous test found user1, but only once as the bug only allows reading the security descriptor for one share, and we were unlucky that the first share security descriptor returned allows user1 to read from it. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13992 Signed-off-by: Jeremy Allison Reviewed-by: Andreas Schneider (cherry picked from commit 068f4a977f0539f790809d580bf22d2362032e3d) --- selftest/knownfail.d/bug-13992 | 2 ++ .../tests/test_net_rpc_share_allowedusers.sh | 20 +++++++++++++++++++ 2 files changed, 22 insertions(+) create mode 100644 selftest/knownfail.d/bug-13992 diff --git a/selftest/knownfail.d/bug-13992 b/selftest/knownfail.d/bug-13992 new file mode 100644 index 00000000000..76365f09303 --- /dev/null +++ b/selftest/knownfail.d/bug-13992 @@ -0,0 +1,2 @@ +# bug 13992 +^samba3.blackbox.net_rpc_share_allowedusers.net_rpc_share_allowedusers\(nt4_dc\) diff --git a/source3/script/tests/test_net_rpc_share_allowedusers.sh b/source3/script/tests/test_net_rpc_share_allowedusers.sh index 5dd382d4c51..d22c7580681 100755 --- a/source3/script/tests/test_net_rpc_share_allowedusers.sh +++ b/source3/script/tests/test_net_rpc_share_allowedusers.sh @@ -26,5 +26,25 @@ testit_grep "net_rpc_share_allowedusers" '^print\$$' $net usersidlist | $VALGRIN testit_grep "net_rpc_share_allowedusers" '^print\$$' $net usersidlist | $VALGRIND $net rpc share allowedusers -S$SERVER -U$USERNAME%$PASSWORD $ADDARGS - 'print$' || failed=`expr $failed + 1` # Check user "user1" is allowed to read share "tmp". testit_grep "net_rpc_share_allowedusers" '^ user1$' $net usersidlist | $VALGRIND $net rpc share allowedusers -S$SERVER -U$USERNAME%$PASSWORD $ADDARGS || failed=`expr $failed + 1` +# +# Subtle extra test for bug https://bugzilla.samba.org/show_bug.cgi?id=13992 +# +# '^ user1$' must appear MORE THAN ONCE, as it can read more than one +# share. The previous test found user1, but only once as the bug only +# allows reading the security descriptor for one share, and we were +# unlucky that the first share security descriptor returned allows +# user1 to read from it. +# +subunit_start_test "net_rpc_share_allowedusers" +multi_userout=`$net usersidlist | $VALGRIND $net rpc share allowedusers -S$SERVER -U$USERNAME%$PASSWORD $ADDARGS` +num_matches=`echo "$multi_userout" | grep -c '^ user1$'` +if [ "$num_matches" -gt "1" ] +then + subunit_pass_test "net_rpc_share_allowedusers" +else + echo "net_rpc_share_allowedusers only found $num_matches shares readable by user1. Should be greater than one.\n" + failed=`expr $failed + 1` + echo "$multi_userout" | subunit_fail_test "net_rpc_share_allowedusers" +fi testok $0 $failed -- 2.27.0 From 8e9db89642cba20c4393cd794508b1333cc106fb Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 28 Jan 2021 10:46:33 -0800 Subject: [PATCH 2/5] s3: smbtorture3: Ensure we *always* replace the saved saved_tcon even in an error condition. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13992 Signed-off-by: Jeremy Allison Reviewed-by: Andreas Schneider (cherry picked from commit dc701959cad7bf15aa47cad6451212606520f67f) --- source3/torture/test_smb2.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source3/torture/test_smb2.c b/source3/torture/test_smb2.c index d5023d88cfd..3e25a562bd6 100644 --- a/source3/torture/test_smb2.c +++ b/source3/torture/test_smb2.c @@ -188,11 +188,11 @@ bool run_smb2_basic(int dummy) cli->timeout, cli->smb2.session, cli->smb2.tcon); + cli_state_restore_tcon(cli, saved_tcon); if (!NT_STATUS_IS_OK(status)) { printf("smb2cli_tdis returned %s\n", nt_errstr(status)); return false; } - cli_state_restore_tcon(cli, saved_tcon); status = smb2cli_tdis(cli->conn, cli->timeout, -- 2.27.0 From 78da9875cb84104a18cd2739b501b3527739bc9a Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 28 Jan 2021 10:56:18 -0800 Subject: [PATCH 3/5] s3: smbtorture3: Ensure run_tcon_test() always replaces any saved tcon and shuts down correctly even in error paths. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13992 Signed-off-by: Jeremy Allison Reviewed-by: Andreas Schneider (cherry picked from commit f9ca91bd293e9f2710c4449c5d4f5d016a066049) --- source3/torture/torture.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/source3/torture/torture.c b/source3/torture/torture.c index 2a3133373e9..6160adb98cc 100644 --- a/source3/torture/torture.c +++ b/source3/torture/torture.c @@ -1343,6 +1343,7 @@ static bool run_tcon_test(int dummy) if (!NT_STATUS_IS_OK(status)) { printf("%s refused 2nd tree connect (%s)\n", host, nt_errstr(status)); + cli_state_restore_tcon(cli, orig_tcon); cli_shutdown(cli); return False; } @@ -1395,6 +1396,8 @@ static bool run_tcon_test(int dummy) status = cli_close(cli, fnum1); if (!NT_STATUS_IS_OK(status)) { printf("close failed (%s)\n", nt_errstr(status)); + cli_state_restore_tcon(cli, orig_tcon); + cli_shutdown(cli); return False; } @@ -1403,6 +1406,8 @@ static bool run_tcon_test(int dummy) status = cli_tdis(cli); if (!NT_STATUS_IS_OK(status)) { printf("secondary tdis failed (%s)\n", nt_errstr(status)); + cli_state_restore_tcon(cli, orig_tcon); + cli_shutdown(cli); return False; } -- 2.27.0 From 06d5f2bbd536dab3e5d7395b6f84ccf037ba7457 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 28 Jan 2021 17:35:55 -0800 Subject: [PATCH 4/5] s3: torture: Change the SMB1-only UID-REGRESSION-TEST to do an explicit copy of the tcon struct in use. For this test only, explicitly copy the SMB1 tcon struct, don't use cli_state_save_tcon()//cli_state_restore_tcon() as these calls will soon change to just manipulate the pointer to avoid TALLOC_FREE() on the tcon struct which calls destructors on child pipe data. In SMB1 this test calls cli_tdis() twice with an invalid vuid and expects the SMB1 tcon struct to be preserved across the calls. SMB1 cli_tdis() frees cli->smb1.tcon so we must put back a deep copy into cli->smb1.tcon to be able to safely call cli_tdis() again. This is a test-only hack. Real client code uses cli_state_save_tcon()/cli_state_restore_tcon() if it needs to temporarily swap out the active tcon on a client connection. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13992 Signed-off-by: Jeremy Allison Reviewed-by: Andreas Schneider (cherry picked from commit e93e6108837eff0cebad8dc26d055c0e1386093a) --- source3/torture/torture.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/source3/torture/torture.c b/source3/torture/torture.c index 6160adb98cc..5e263797730 100644 --- a/source3/torture/torture.c +++ b/source3/torture/torture.c @@ -11719,7 +11719,7 @@ static bool run_uid_regression_test(int dummy) int16_t old_vuid; int32_t old_cnum; bool correct = True; - struct smbXcli_tcon *orig_tcon = NULL; + struct smbXcli_tcon *tcon_copy = NULL; NTSTATUS status; printf("starting uid regression test\n"); @@ -11760,8 +11760,20 @@ static bool run_uid_regression_test(int dummy) } old_cnum = cli_state_get_tid(cli); - orig_tcon = cli_state_save_tcon(cli); - if (orig_tcon == NULL) { + /* + * This is an SMB1-only test. + * Copy the tcon, not "save/restore". + * + * In SMB1 the cli_tdis() below frees + * cli->smb1.tcon so we need a copy + * of the struct to put back for the + * second tdis call with invalid vuid. + * + * This is a test-only hack. Real client code + * uses cli_state_save_tcon()/cli_state_restore_tcon(). + */ + tcon_copy = smbXcli_tcon_copy(cli, cli->smb1.tcon); + if (tcon_copy == NULL) { correct = false; goto out; } @@ -11777,11 +11789,11 @@ static bool run_uid_regression_test(int dummy) } else { d_printf("First tdis failed (%s)\n", nt_errstr(status)); correct = false; - cli_state_restore_tcon(cli, orig_tcon); + cli->smb1.tcon = tcon_copy; goto out; } - cli_state_restore_tcon(cli, orig_tcon); + cli->smb1.tcon = tcon_copy; cli_state_set_uid(cli, old_vuid); cli_state_set_tid(cli, old_cnum); -- 2.27.0 From 5bcf3d93c29b1d4d54c31167497db8de3a4a4ba7 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 28 Jan 2021 11:08:48 -0800 Subject: [PATCH 5/5] s3: libsmb: cli_state_save_tcon(). Don't deepcopy tcon struct when temporarily swapping out a connection on a cli_state. This used to make a deep copy of either cli->smb2.tcon or cli->smb1.tcon, but this leaves the original tcon pointer in place which will then get TALLOC_FREE()'d when the new tree connection is made on this cli_state. As there may be pipes open on the old tree connection with talloc'ed state allocated using the original tcon pointer as a talloc parent we can't deep copy and then free this pointer as that will fire the destructors on the pipe memory and mark them as not connected. This call is used to temporarily swap out a tcon pointer (whilst keeping existing pipes open) to allow a new tcon on the same cli_state and all users correctly call cli_state_restore_tcon() once they are finished with the new tree connection. Just return the existing pointer and set the old value to NULL. We know we MUST be calling cli_state_restore_tcon() below to restore the original tcon tree connection pointer before closing the session. Remove the knownfail.d entry. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13992 Signed-off-by: Jeremy Allison Reviewed-by: Andreas Schneider Autobuild-User(master): Jeremy Allison Autobuild-Date(master): Tue Feb 2 21:05:25 UTC 2021 on sn-devel-184 (cherry picked from commit 4f80f5f9046b64a9e5e0503b1cb54f1492c4faec) --- selftest/knownfail.d/bug-13992 | 2 -- source3/libsmb/clientgen.c | 30 ++++++++++++++++++++++++++++-- 2 files changed, 28 insertions(+), 4 deletions(-) delete mode 100644 selftest/knownfail.d/bug-13992 diff --git a/selftest/knownfail.d/bug-13992 b/selftest/knownfail.d/bug-13992 deleted file mode 100644 index 76365f09303..00000000000 --- a/selftest/knownfail.d/bug-13992 +++ /dev/null @@ -1,2 +0,0 @@ -# bug 13992 -^samba3.blackbox.net_rpc_share_allowedusers.net_rpc_share_allowedusers\(nt4_dc\) diff --git a/source3/libsmb/clientgen.c b/source3/libsmb/clientgen.c index 4412651f505..8ded372d0ba 100644 --- a/source3/libsmb/clientgen.c +++ b/source3/libsmb/clientgen.c @@ -352,11 +352,37 @@ uint32_t cli_state_set_tid(struct cli_state *cli, uint32_t tid) struct smbXcli_tcon *cli_state_save_tcon(struct cli_state *cli) { + /* + * Note. This used to make a deep copy of either + * cli->smb2.tcon or cli->smb1.tcon, but this leaves + * the original pointer in place which will then get + * TALLOC_FREE()'d when the new connection is made on + * this cli_state. + * + * As there may be pipes open on the old connection with + * talloc'ed state allocated using the tcon pointer as a + * parent we can't deep copy and then free this as that + * closes the open pipes. + * + * This call is used to temporarily swap out a tcon pointer + * to allow a new tcon on the same cli_state. + * + * Just return the raw pointer and set the old value to NULL. + * We know we MUST be calling cli_state_restore_tcon() below + * to restore before closing the session. + * + * See BUG: https://bugzilla.samba.org/show_bug.cgi?id=13992 + */ + struct smbXcli_tcon *tcon_ret = NULL; + if (smbXcli_conn_protocol(cli->conn) >= PROTOCOL_SMB2_02) { - return smbXcli_tcon_copy(cli, cli->smb2.tcon); + tcon_ret = cli->smb2.tcon; + cli->smb2.tcon = NULL; /* *Not* TALLOC_FREE(). */ } else { - return smbXcli_tcon_copy(cli, cli->smb1.tcon); + tcon_ret = cli->smb1.tcon; + cli->smb1.tcon = NULL; /* *Not* TALLOC_FREE(). */ } + return tcon_ret; } void cli_state_restore_tcon(struct cli_state *cli, struct smbXcli_tcon *tcon) -- 2.27.0