From 3c887bb66ffaef5582972b4274a5d6fd09788439 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Wed, 31 Aug 2022 13:55:19 +0200 Subject: [PATCH 1/4] s3:tests: let test_smbXsrv_client_dead_rec.sh cleanup the correct files BUG: https://bugzilla.samba.org/show_bug.cgi?id=15159 Signed-off-by: Stefan Metzmacher Reviewed-by: Jeremy Allison (cherry picked from commit 3fd18a0d5b77a9f78c595852c342d4c8c33fac61) --- source3/script/tests/test_smbXsrv_client_dead_rec.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source3/script/tests/test_smbXsrv_client_dead_rec.sh b/source3/script/tests/test_smbXsrv_client_dead_rec.sh index a29350878bd4..0a287370944e 100755 --- a/source3/script/tests/test_smbXsrv_client_dead_rec.sh +++ b/source3/script/tests/test_smbXsrv_client_dead_rec.sh @@ -62,7 +62,7 @@ ${SMBCLIENT} //"${SERVER}"/"${SHARE}" -U"${USER}"%"${PASSWORD}" \ --option="libsmb:client_guid=6112f7d3-9528-4a2a-8861-0ca129aae6c4" \ -c exit -rm -f smbclient-stdin smbclient-stdout aio_outstanding_testfile +rm -f smbclient-stdin smbclient-stdout smbclient-stderr # # Ensure the panic count didn't change. -- 2.34.1 From e1b36df6686456c43e02db3a6de18a74a4132807 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Wed, 31 Aug 2022 14:04:10 +0200 Subject: [PATCH 2/4] s3:tests: add test_smbXsrv_client_cross_node.sh This demonstrates that a client-guid connected to ctdb node 0 caused a connection with the same client-guid to be rejected by ctdb node 1. Node 1 rejects the SMB2 Negotiate with NT_STATUS_NOT_SUPPORTED, because passing the multi-channel connection to a different node is not supported. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15159 Signed-off-by: Stefan Metzmacher Reviewed-by: Jeremy Allison (cherry picked from commit 0efcfaa49c3d61f2c8116ebafd55b72d3277d0d8) --- selftest/knownfail.d/bug_15159 | 1 + .../tests/test_smbXsrv_client_cross_node.sh | 95 +++++++++++++++++++ source3/selftest/tests.py | 7 ++ 3 files changed, 103 insertions(+) create mode 100644 selftest/knownfail.d/bug_15159 create mode 100755 source3/script/tests/test_smbXsrv_client_cross_node.sh diff --git a/selftest/knownfail.d/bug_15159 b/selftest/knownfail.d/bug_15159 new file mode 100644 index 000000000000..5ecd8b1a66e5 --- /dev/null +++ b/selftest/knownfail.d/bug_15159 @@ -0,0 +1 @@ +samba3.blackbox.smbXsrv_client_cross_node.smbclient.against.node1 diff --git a/source3/script/tests/test_smbXsrv_client_cross_node.sh b/source3/script/tests/test_smbXsrv_client_cross_node.sh new file mode 100755 index 000000000000..ff826924b493 --- /dev/null +++ b/source3/script/tests/test_smbXsrv_client_cross_node.sh @@ -0,0 +1,95 @@ +#!/bin/bash +# +# Test smbd let cluster node 0 destroy the connection, +# if the client with a specific client-guid connections to node 1 +# + +if [ $# -lt 4 ]; then + echo Usage: test_smbXsrv_client_cross_node.sh SERVERCONFFILE NODE0 NODE1 SHARENAME + exit 1 +fi + +CONF=$1 +NODE0=$2 +NODE1=$3 +SHARE=$4 + +SMBCLIENT="$BINDIR/smbclient" +SMBSTATUS="$BINDIR/smbstatus" + +incdir=$(dirname "$0")/../../../testprogs/blackbox +. "$incdir"/subunit.sh + +failed=0 + +test_smbclient() +{ + name="$1" + server="$2" + share="$3" + cmd="$4" + shift + shift + subunit_start_test "$name" + output=$($VALGRIND $SMBCLIENT //$server/$share -c "$cmd" "$@" 2>&1) + status=$? + if [ x$status = x0 ]; then + subunit_pass_test "$name" + else + echo "$output" | subunit_fail_test "$name" + fi + return $status +} + +cd "$SELFTEST_TMPDIR" || exit 1 + +# Create the smbclient communication pipes. +rm -f smbclient-stdin smbclient-stdout smbclient-stderr +mkfifo smbclient-stdin smbclient-stdout smbclient-stderr + +UID_WRAPPER_ROOT=1 +export UID_WRAPPER_ROOT + +smbstatus_num_sessions() +{ + UID_WRAPPER_INITIAL_RUID=0 UID_WRAPPER_INITIAL_EUID=0 "$SMBSTATUS" "$CONF" --json | jq -M '.sessions | length' +} + +testit_grep "step1: smbstatus 0 sessions" '^0$' smbstatus_num_sessions || failed=$(expr $failed + 1) + +test_smbclient "smbclient against node0[${NODE0}]" "${NODE0}" "${SHARE}" "ls" -U"${DC_USERNAME}"%"${DC_PASSWORD}" \ + --option="libsmb:client_guid=6112f7d3-9528-4a2a-8861-0ca129aae6c4" \ + || failed=$(expr $failed + 1) + +testit_grep "step2: smbstatus 0 sessions" '^0$' smbstatus_num_sessions || failed=$(expr $failed + 1) + +CLI_FORCE_INTERACTIVE=1 +export CLI_FORCE_INTERACTIVE + +testit "start backgroup smbclient against node0[${NODE0}]" true || failed=$(expr $failed + 1) + +# Connect a first time +${SMBCLIENT} //"${NODE0}"/"${SHARE}" -U"${DC_USERNAME}"%"${DC_PASSWORD}" \ + --option="libsmb:client_guid=6112f7d3-9528-4a2a-8861-0ca129aae6c4" \ + smbclient-stdout 2>smbclient-stderr & +CLIENT_PID=$! + +exec 100>smbclient-stdin 101 Date: Tue, 30 Aug 2022 16:56:12 +0200 Subject: [PATCH 3/4] smbXsrv_client: correctly check in negotiate_request.length smbXsrv_client_connection_pass[ed]_* BUG: https://bugzilla.samba.org/show_bug.cgi?id=15159 Signed-off-by: Stefan Metzmacher Reviewed-by: Jeremy Allison (cherry picked from commit 21ef01e7b8368caa050ed82b9d787d1679220b2b) --- source3/smbd/smbXsrv_client.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/source3/smbd/smbXsrv_client.c b/source3/smbd/smbXsrv_client.c index 079ca80ad121..7cf51b2d0226 100644 --- a/source3/smbd/smbXsrv_client.c +++ b/source3/smbd/smbXsrv_client.c @@ -614,10 +614,6 @@ static bool smb2srv_client_mc_negprot_filter(struct messaging_rec *rec, void *pr return false; } - if (rec->buf.length < SMB2_HDR_BODY) { - return false; - } - return true; } @@ -707,6 +703,14 @@ static void smb2srv_client_mc_negprot_done(struct tevent_req *subreq) return; } + if (passed_info0->negotiate_request.length != 0) { + DBG_ERR("negotiate_request.length[%zu]\n", + passed_info0->negotiate_request.length); + NDR_PRINT_DEBUG(smbXsrv_connection_passB, &passed_blob); + tevent_req_nterror(req, NT_STATUS_INTERNAL_ERROR); + return; + } + tevent_req_nterror(req, NT_STATUS_MESSAGE_RETRIEVED); } @@ -931,12 +935,6 @@ static bool smbXsrv_client_connection_pass_filter(struct messaging_rec *rec, voi return false; } - if (rec->buf.length < SMB2_HDR_BODY) { - return false; - } - - /* TODO: verify client_guid...? */ - return true; } @@ -1029,6 +1027,15 @@ static void smbXsrv_client_connection_pass_loop(struct tevent_req *subreq) goto next; } + if (pass_info0->negotiate_request.length < SMB2_HDR_BODY) { + DBG_WARNING("negotiate_request.length[%zu]\n", + pass_info0->negotiate_request.length); + if (DEBUGLVL(DBGLVL_WARNING)) { + NDR_PRINT_DEBUG(smbXsrv_connection_passB, &pass_blob); + } + goto next; + } + status = smb2srv_client_connection_passed(client, pass_info0); if (!NT_STATUS_IS_OK(status)) { const char *r = "smb2srv_client_connection_passed() failed"; -- 2.34.1 From e9a19d49f94df268d3c532d8f0392f4243c45931 Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Tue, 30 Aug 2022 20:45:50 +0200 Subject: [PATCH 4/4] smbXsrv_client: notify a different node to drop a connection by client guid. If a client disconnected all its interfaces and reconnects when the come back, it will likely start from any ip address returned dns, which means it can try to connect to a different ctdb node. The old node may not have noticed the disconnect and still holds the client_guid based smbd. Up unil now the new node returned NT_STATUS_NOT_SUPPORTED to the SMB2 Negotiate request, as messaging_send_iov[_from]() will return -1/ENOSYS if a file descriptor os passed to a process on a different node. Now we tell the other node to teardown all client connections belonging to the client-guid. Note that this is not authenticated, but if an attacker can capture the client-guid, he can also inject TCP resets anyway, to get the same effect. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15159 Signed-off-by: Stefan Metzmacher Reviewed-by: Jeremy Allison Autobuild-User(master): Jeremy Allison Autobuild-Date(master): Fri Sep 2 20:59:15 UTC 2022 on sn-devel-184 (cherry picked from commit 8591d9424371e173b079d5c8a267ea4c2cb266ad) --- librpc/idl/messaging.idl | 1 + selftest/knownfail.d/bug_15159 | 1 - source3/librpc/idl/smbXsrv.idl | 28 ++++ source3/smbd/smbXsrv_client.c | 239 +++++++++++++++++++++++++++++++-- 4 files changed, 255 insertions(+), 14 deletions(-) delete mode 100644 selftest/knownfail.d/bug_15159 diff --git a/librpc/idl/messaging.idl b/librpc/idl/messaging.idl index d6929c799adf..5d217c03f5be 100644 --- a/librpc/idl/messaging.idl +++ b/librpc/idl/messaging.idl @@ -138,6 +138,7 @@ interface messaging MSG_SMBXSRV_SESSION_CLOSE = 0x0600, MSG_SMBXSRV_CONNECTION_PASS = 0x0601, MSG_SMBXSRV_CONNECTION_PASSED = 0x0602, + MSG_SMBXSRV_CONNECTION_DROP = 0x0603, /* source4 and NTVFS smb server messages */ MSG_BRL_RETRY = 0x0700, diff --git a/selftest/knownfail.d/bug_15159 b/selftest/knownfail.d/bug_15159 deleted file mode 100644 index 5ecd8b1a66e5..000000000000 --- a/selftest/knownfail.d/bug_15159 +++ /dev/null @@ -1 +0,0 @@ -samba3.blackbox.smbXsrv_client_cross_node.smbclient.against.node1 diff --git a/source3/librpc/idl/smbXsrv.idl b/source3/librpc/idl/smbXsrv.idl index fc502009b3bd..ec65a5c1a613 100644 --- a/source3/librpc/idl/smbXsrv.idl +++ b/source3/librpc/idl/smbXsrv.idl @@ -143,6 +143,7 @@ interface smbXsrv boolean8 server_multi_channel_enabled; hyper next_channel_id; [ignore] struct tevent_req *connection_pass_subreq; + [ignore] struct tevent_req *connection_drop_subreq; /* * A List of pending breaks. @@ -194,6 +195,33 @@ interface smbXsrv [in] smbXsrv_connection_passB blob ); + /* + * smbXsrv_connection_drop is used in the MSG_SMBXSRV_CONNECTION_DROP + * message as reaction the record is deleted. + */ + typedef struct { + GUID client_guid; + server_id src_server_id; + NTTIME xconn_connect_time; + server_id dst_server_id; + NTTIME client_connect_time; + } smbXsrv_connection_drop0; + + typedef union { + [case(0)] smbXsrv_connection_drop0 *info0; + [default] hyper *dummy; + } smbXsrv_connection_dropU; + + typedef [public] struct { + smbXsrv_version_values version; + [value(0)] uint32 reserved; + [switch_is(version)] smbXsrv_connection_dropU info; + } smbXsrv_connection_dropB; + + void smbXsrv_connection_drop_decode( + [in] smbXsrv_connection_dropB blob + ); + /* sessions */ typedef [public,bitmap8bit] bitmap { diff --git a/source3/smbd/smbXsrv_client.c b/source3/smbd/smbXsrv_client.c index 7cf51b2d0226..d7a6fa35bf07 100644 --- a/source3/smbd/smbXsrv_client.c +++ b/source3/smbd/smbXsrv_client.c @@ -346,6 +346,55 @@ static NTSTATUS smb2srv_client_connection_pass(struct smbd_smb2_request *smb2req return NT_STATUS_OK; } +static NTSTATUS smb2srv_client_connection_drop(struct smbd_smb2_request *smb2req, + struct smbXsrv_client_global0 *global) +{ + DATA_BLOB blob; + enum ndr_err_code ndr_err; + NTSTATUS status; + struct smbXsrv_connection_drop0 drop_info0; + struct smbXsrv_connection_dropB drop_blob; + struct iovec iov; + + drop_info0 = (struct smbXsrv_connection_drop0) { + .client_guid = global->client_guid, + .src_server_id = smb2req->xconn->client->global->server_id, + .xconn_connect_time = smb2req->xconn->client->global->initial_connect_time, + .dst_server_id = global->server_id, + .client_connect_time = global->initial_connect_time, + }; + + ZERO_STRUCT(drop_blob); + drop_blob.version = smbXsrv_version_global_current(); + drop_blob.info.info0 = &drop_info0; + + if (DEBUGLVL(DBGLVL_DEBUG)) { + NDR_PRINT_DEBUG(smbXsrv_connection_dropB, &drop_blob); + } + + ndr_err = ndr_push_struct_blob(&blob, talloc_tos(), &drop_blob, + (ndr_push_flags_fn_t)ndr_push_smbXsrv_connection_dropB); + if (!NDR_ERR_CODE_IS_SUCCESS(ndr_err)) { + status = ndr_map_error2ntstatus(ndr_err); + return status; + } + + iov.iov_base = blob.data; + iov.iov_len = blob.length; + + status = messaging_send_iov(smb2req->xconn->client->msg_ctx, + global->server_id, + MSG_SMBXSRV_CONNECTION_DROP, + &iov, 1, + NULL, 0); + data_blob_free(&blob); + if (!NT_STATUS_IS_OK(status)) { + return status; + } + + return NT_STATUS_OK; +} + static NTSTATUS smbXsrv_client_global_store(struct smbXsrv_client_global0 *global) { struct smbXsrv_client_globalB global_blob; @@ -552,15 +601,17 @@ static void smb2srv_client_mc_negprot_next(struct tevent_req *req) return; } - subreq = messaging_filtered_read_send(state, - state->ev, - client->msg_ctx, - smb2srv_client_mc_negprot_filter, - NULL); - if (tevent_req_nomem(subreq, req)) { - return; + if (procid_is_local(&global->server_id)) { + subreq = messaging_filtered_read_send(state, + state->ev, + client->msg_ctx, + smb2srv_client_mc_negprot_filter, + NULL); + if (tevent_req_nomem(subreq, req)) { + return; + } + tevent_req_set_callback(subreq, smb2srv_client_mc_negprot_done, req); } - tevent_req_set_callback(subreq, smb2srv_client_mc_negprot_done, req); /* * If the record changed, but we are not happy with the change yet, @@ -593,11 +644,20 @@ static void smb2srv_client_mc_negprot_next(struct tevent_req *req) } tevent_req_set_callback(subreq, smb2srv_client_mc_negprot_watched, req); - status = smb2srv_client_connection_pass(state->smb2req, - global); - TALLOC_FREE(global); - if (tevent_req_nterror(req, status)) { - return; + if (procid_is_local(&global->server_id)) { + status = smb2srv_client_connection_pass(state->smb2req, + global); + TALLOC_FREE(global); + if (tevent_req_nterror(req, status)) { + return; + } + } else { + status = smb2srv_client_connection_drop(state->smb2req, + global); + TALLOC_FREE(global); + if (tevent_req_nterror(req, status)) { + return; + } } TALLOC_FREE(state->db_rec); @@ -792,6 +852,8 @@ static int smbXsrv_client_destructor(struct smbXsrv_client *client) static bool smbXsrv_client_connection_pass_filter(struct messaging_rec *rec, void *private_data); static void smbXsrv_client_connection_pass_loop(struct tevent_req *subreq); +static bool smbXsrv_client_connection_drop_filter(struct messaging_rec *rec, void *private_data); +static void smbXsrv_client_connection_drop_loop(struct tevent_req *subreq); NTSTATUS smbXsrv_client_create(TALLOC_CTX *mem_ctx, struct tevent_context *ev_ctx, @@ -874,6 +936,18 @@ NTSTATUS smbXsrv_client_create(TALLOC_CTX *mem_ctx, tevent_req_set_callback(subreq, smbXsrv_client_connection_pass_loop, client); client->connection_pass_subreq = subreq; + subreq = messaging_filtered_read_send(client, + client->raw_ev_ctx, + client->msg_ctx, + smbXsrv_client_connection_drop_filter, + client); + if (subreq == NULL) { + TALLOC_FREE(client); + return NT_STATUS_NO_MEMORY; + } + tevent_req_set_callback(subreq, smbXsrv_client_connection_drop_loop, client); + client->connection_drop_subreq = subreq; + *_client = client; return NT_STATUS_OK; } @@ -1099,6 +1173,144 @@ next: client->connection_pass_subreq = subreq; } +static bool smbXsrv_client_connection_drop_filter(struct messaging_rec *rec, void *private_data) +{ + if (rec->msg_type != MSG_SMBXSRV_CONNECTION_DROP) { + return false; + } + + if (rec->num_fds != 0) { + return false; + } + + return true; +} + +static void smbXsrv_client_connection_drop_loop(struct tevent_req *subreq) +{ + struct smbXsrv_client *client = + tevent_req_callback_data(subreq, + struct smbXsrv_client); + int ret; + struct messaging_rec *rec = NULL; + struct smbXsrv_connection_dropB drop_blob; + enum ndr_err_code ndr_err; + struct smbXsrv_connection_drop0 *drop_info0 = NULL; + struct server_id_buf src_server_id_buf = {}; + NTSTATUS status; + + client->connection_drop_subreq = NULL; + + ret = messaging_filtered_read_recv(subreq, talloc_tos(), &rec); + TALLOC_FREE(subreq); + if (ret != 0) { + goto next; + } + + if (rec->num_fds != 0) { + DBG_ERR("MSG_SMBXSRV_CONNECTION_DROP: num_fds[%u]\n", + rec->num_fds); + goto next; + } + + ndr_err = ndr_pull_struct_blob(&rec->buf, rec, &drop_blob, + (ndr_pull_flags_fn_t)ndr_pull_smbXsrv_connection_dropB); + if (!NDR_ERR_CODE_IS_SUCCESS(ndr_err)) { + status = ndr_map_error2ntstatus(ndr_err); + DBG_WARNING("ndr_pull_struct_blob - %s\n", nt_errstr(status)); + goto next; + } + + if (DEBUGLVL(DBGLVL_DEBUG)) { + NDR_PRINT_DEBUG(smbXsrv_connection_dropB, &drop_blob); + } + + if (drop_blob.version != SMBXSRV_VERSION_0) { + DBG_ERR("ignore invalid version %u\n", drop_blob.version); + NDR_PRINT_DEBUG(smbXsrv_connection_dropB, &drop_blob); + goto next; + } + + drop_info0 = drop_blob.info.info0; + if (drop_info0 == NULL) { + DBG_ERR("ignore NULL info %u\n", drop_blob.version); + NDR_PRINT_DEBUG(smbXsrv_connection_dropB, &drop_blob); + goto next; + } + + if (!GUID_equal(&client->global->client_guid, &drop_info0->client_guid)) + { + struct GUID_txt_buf buf1, buf2; + + DBG_WARNING("client's client_guid [%s] != droped guid [%s]\n", + GUID_buf_string(&client->global->client_guid, + &buf1), + GUID_buf_string(&drop_info0->client_guid, + &buf2)); + if (DEBUGLVL(DBGLVL_WARNING)) { + NDR_PRINT_DEBUG(smbXsrv_connection_dropB, &drop_blob); + } + goto next; + } + + if (client->global->initial_connect_time != + drop_info0->client_connect_time) + { + DBG_WARNING("client's initial connect time [%s] (%llu) != " + "droped initial connect time [%s] (%llu)\n", + nt_time_string(talloc_tos(), + client->global->initial_connect_time), + (unsigned long long)client->global->initial_connect_time, + nt_time_string(talloc_tos(), + drop_info0->client_connect_time), + (unsigned long long)drop_info0->client_connect_time); + if (DEBUGLVL(DBGLVL_WARNING)) { + NDR_PRINT_DEBUG(smbXsrv_connection_dropB, &drop_blob); + } + goto next; + } + + /* + * Disconnect all client connections, which means we will tear down all + * sessions, tcons and non-durable opens. At the end we will remove our + * smbXsrv_client_global.tdb record, which will wake up the watcher on + * the other node in order to let it take over the client. + * + * The client will have to reopen all sessions, tcons and durable opens. + */ + smbd_server_disconnect_client(client, + server_id_str_buf(drop_info0->src_server_id, &src_server_id_buf)); + return; + +next: + if (rec != NULL) { + int sock_fd; + uint8_t fd_idx; + + for (fd_idx = 0; fd_idx < rec->num_fds; fd_idx++) { + sock_fd = rec->fds[fd_idx]; + close(sock_fd); + } + rec->num_fds = 0; + + TALLOC_FREE(rec); + } + + subreq = messaging_filtered_read_send(client, + client->raw_ev_ctx, + client->msg_ctx, + smbXsrv_client_connection_drop_filter, + client); + if (subreq == NULL) { + const char *r; + r = "messaging_read_send(MSG_SMBXSRV_CONNECTION_DROP failed"; + exit_server_cleanly(r); + return; + } + tevent_req_set_callback(subreq, smbXsrv_client_connection_drop_loop, client); + client->connection_drop_subreq = subreq; +} + NTSTATUS smbXsrv_client_remove(struct smbXsrv_client *client) { struct smbXsrv_client_table *table = client->table; @@ -1117,6 +1329,7 @@ NTSTATUS smbXsrv_client_remove(struct smbXsrv_client *client) } TALLOC_FREE(client->connection_pass_subreq); + TALLOC_FREE(client->connection_drop_subreq); client->global->db_rec = smbXsrv_client_global_fetch_locked( table->global.db_ctx, -- 2.34.1