The Samba-Bugzilla – Attachment 16415 Details for
Bug 13992
SAMBA RPC share error in SAMBA Stretch 4.5.16 and Buster 4.9.5
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
git-am fix for master.
bug-13992-master-1 (text/plain), 12.34 KB, created by
Jeremy Allison
on 2021-01-29 02:05:16 UTC
(
hide
)
Description:
git-am fix for master.
Filename:
MIME Type:
Creator:
Jeremy Allison
Created:
2021-01-29 02:05:16 UTC
Size:
12.34 KB
patch
obsolete
>From 5b50c027378b03157d3a44d39b10a84c9bb26726 Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Thu, 28 Jan 2021 14:07:23 -0800 >Subject: [PATCH 1/6] 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 <jra@samba.org> >--- > 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 39662ceab2dff65510685d0f4369c312c42ce26e Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Thu, 28 Jan 2021 14:32:53 -0800 >Subject: [PATCH 2/6] s3: libsmb: Ensure we disconnect the temporary SMB1 tcon > pointer on failure to set up encryption. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=13992 > >Signed-off-by: Jeremy Allison <jra@samba.org> >--- > source3/libsmb/clidfs.c | 7 +++++++ > 1 file changed, 7 insertions(+) > >diff --git a/source3/libsmb/clidfs.c b/source3/libsmb/clidfs.c >index 26b5499cf73..040b957e6f8 100644 >--- a/source3/libsmb/clidfs.c >+++ b/source3/libsmb/clidfs.c >@@ -1203,6 +1203,13 @@ bool cli_check_msdfs_proxy(TALLOC_CTX *ctx, > break; > case SMB_ENCRYPTION_REQUIRED: > default: >+ /* >+ * Failed to set up encryption. >+ * Disconnect the temporary IPC$ >+ * tcon before restoring the original >+ * tcon so we don't leak it. >+ */ >+ cli_tdis(cli); > cli_state_restore_tcon(cli, orig_tcon); > return false; > } >-- >2.27.0 > > >From 9e43ac6164b2256dfa9f6fc9968de4fc1aa3ee9c Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Thu, 28 Jan 2021 10:46:33 -0800 >Subject: [PATCH 3/6] 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 <jra@samba.org> >--- > 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 2d02db3b108..a81e40568e8 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 7c7cd13614afd80c20374b5cb15a4984a7ba0837 Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Thu, 28 Jan 2021 10:56:18 -0800 >Subject: [PATCH 4/6] 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 <jra@samba.org> >--- > source3/torture/torture.c | 5 +++++ > 1 file changed, 5 insertions(+) > >diff --git a/source3/torture/torture.c b/source3/torture/torture.c >index cdf5d5ca3aa..4f572902494 100644 >--- a/source3/torture/torture.c >+++ b/source3/torture/torture.c >@@ -1347,6 +1347,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; > } >@@ -1399,6 +1400,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; > } > >@@ -1407,6 +1410,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 3f153bcb1b65929a57dc483f7182482ed9ee839c Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Thu, 28 Jan 2021 17:35:55 -0800 >Subject: [PATCH 5/6] 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 <jra@samba.org> >--- > 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 4f572902494..d1ea9b85a72 100644 >--- a/source3/torture/torture.c >+++ b/source3/torture/torture.c >@@ -11690,7 +11690,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"); >@@ -11731,8 +11731,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; > } >@@ -11748,11 +11760,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 10a16142998fd226216cf0d8c3574810953cb7cd Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Thu, 28 Jan 2021 11:08:48 -0800 >Subject: [PATCH 6/6] 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 <jra@samba.org> >--- > 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 d117885b8f7..e86f52dac0d 100644 >--- a/source3/libsmb/clientgen.c >+++ b/source3/libsmb/clientgen.c >@@ -348,11 +348,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 >
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=16415">View the attachment on a separate page</a>.</b>
View Attachment As Raw
Actions:
View
Attachments on
bug 13992
:
15249
|
15266
|
15267
|
16412
|
16414
|
16415
|
16416
|
16422
|
16423
|
16424
|
16426