The Samba-Bugzilla – Attachment 16414 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 (text/plain), 11.97 KB, created by
Jeremy Allison
on 2021-01-28 22:43:44 UTC
(
hide
)
Description:
git-am fix for master.
Filename:
MIME Type:
Creator:
Jeremy Allison
Created:
2021-01-28 22:43:44 UTC
Size:
11.97 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 870107c121d2e57793d8eaaf60dedf336d7c3271 Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Thu, 28 Jan 2021 11:08:48 -0800 >Subject: [PATCH 5/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 > > >From a162c1814245ab95b8f6bb9100bf5d7992d5a3d1 Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Thu, 28 Jan 2021 11:10:54 -0800 >Subject: [PATCH 6/6] s3: libcli: smbXcli_tcon_copy() is now unused. > >The previous commit removed the only user. > >https://bugzilla.samba.org/show_bug.cgi?id=13992 > >Signed-off-by: Jeremy Allison <jra@samba.org> >--- > libcli/smb/smbXcli_base.c | 32 -------------------------------- > libcli/smb/smbXcli_base.h | 2 -- > 2 files changed, 34 deletions(-) > >diff --git a/libcli/smb/smbXcli_base.c b/libcli/smb/smbXcli_base.c >index e4d495f9622..3b760ea5718 100644 >--- a/libcli/smb/smbXcli_base.c >+++ b/libcli/smb/smbXcli_base.c >@@ -6494,38 +6494,6 @@ struct smbXcli_tcon *smbXcli_tcon_create(TALLOC_CTX *mem_ctx) > return tcon; > } > >-/* >- * Return a deep structure copy of a struct smbXcli_tcon * >- */ >- >-struct smbXcli_tcon *smbXcli_tcon_copy(TALLOC_CTX *mem_ctx, >- const struct smbXcli_tcon *tcon_in) >-{ >- struct smbXcli_tcon *tcon; >- >- tcon = talloc_memdup(mem_ctx, tcon_in, sizeof(struct smbXcli_tcon)); >- if (tcon == NULL) { >- return NULL; >- } >- >- /* Deal with the SMB1 strings. */ >- if (tcon_in->smb1.service != NULL) { >- tcon->smb1.service = talloc_strdup(tcon, tcon_in->smb1.service); >- if (tcon->smb1.service == NULL) { >- TALLOC_FREE(tcon); >- return NULL; >- } >- } >- if (tcon->smb1.fs_type != NULL) { >- tcon->smb1.fs_type = talloc_strdup(tcon, tcon_in->smb1.fs_type); >- if (tcon->smb1.fs_type == NULL) { >- TALLOC_FREE(tcon); >- return NULL; >- } >- } >- return tcon; >-} >- > void smbXcli_tcon_set_fs_attributes(struct smbXcli_tcon *tcon, > uint32_t fs_attributes) > { >diff --git a/libcli/smb/smbXcli_base.h b/libcli/smb/smbXcli_base.h >index d9c3175bdf5..d908b4e31fd 100644 >--- a/libcli/smb/smbXcli_base.h >+++ b/libcli/smb/smbXcli_base.h >@@ -522,8 +522,6 @@ NTSTATUS smb2cli_session_encryption_on(struct smbXcli_session *session); > uint16_t smb2cli_session_get_encryption_cipher(struct smbXcli_session *session); > > struct smbXcli_tcon *smbXcli_tcon_create(TALLOC_CTX *mem_ctx); >-struct smbXcli_tcon *smbXcli_tcon_copy(TALLOC_CTX *mem_ctx, >- const struct smbXcli_tcon *tcon_in); > void smbXcli_tcon_set_fs_attributes(struct smbXcli_tcon *tcon, > uint32_t fs_attributes); > uint32_t smbXcli_tcon_get_fs_attributes(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=16414">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