From 5b50c027378b03157d3a44d39b10a84c9bb26726 Mon Sep 17 00:00:00 2001 From: Jeremy Allison 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 --- 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 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 --- 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 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 --- 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 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 --- 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 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 --- 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 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 --- 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