sin/samba-mirror
1
0
Fork 0
mirror of https://github.com/samba-team/samba.git synced 2025-01-22 22:04:08 +03:00
Code

s3:smbd: fix multichannel connection passing race

Browse Source 
If a client opens multiple connection with the same
client guid in parallel, our connection passing is likely
to hit a race.

Assume we have 3 processes:

smbdA: This process already handles all connections for
       a given client guid
smbdB: This just received a new connection with an
       SMB2 neprot for the same client guid
smbdC: This also received a new connection with an
       SMB2 neprot for the same client guid

Now both smbdB and smbdC send a MSG_SMBXSRV_CONNECTION_PASS
message to smbdA. These messages contain the socket fd
for each connection.

While waiting for a MSG_SMBXSRV_CONNECTION_PASSED message
from smbdA, both smbdB and smbdC watch the smbXcli_client.tdb
record for changes (that also verifies smbdA stays alive).

Once one of them say smbdB received the MSG_SMBXSRV_CONNECTION_PASSED
message, the dbwrap_watch logic will wakeup smbdC in order to
let it recheck the smbXcli_client.tdb record in order to
handle the case where smbdA died or deleted its record.

Now smbdC rechecks the smbXcli_client.tdb record, but it
was not woken because of a problem with smbdA. It meant
that smbdC sends a MSG_SMBXSRV_CONNECTION_PASS message
including the socket fd again.

As a result smbdA got the socket fd from smbdC twice (or even more),
and creates two (or more) smbXsrv_connection structures for the
same low level tcp connection. And it also sends more than one
SMB2 negprot response. Depending on the tevent logic, it will
use different smbXsrv_connection structures to process incoming
requests. And this will almost immediately result in errors.

The typicall error is:
 smb2_validate_sequence_number: smb2_validate_sequence_number: bad message_id 2 (sequence id 2) (granted = 1, low = 1, range = 1)

But other errors would also be possible.

The detail that leads to the long delays on the client side is
that our smbd_server_connection_terminate_ex() code will close
only the fd of a single smbXsrv_connection, but the refcount
on the socket fd in the kernel is still not 0, so the tcp
connection is still alive...

Now we remember the server_id of the process that we send
the MSG_SMBXSRV_CONNECTION_PASS message to. And just keep
watching the smbXcli_client.tdb record if the server_id
don't change. As we just need more patience to wait for
the MSG_SMBXSRV_CONNECTION_PASSED message.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15346

Signed-off-by: Stefan Metzmacher <metze@samba.org>
Reviewed-by: Andreas Schneider <asn@samba.org>

Autobuild-User(master): Andreas Schneider <asn@cryptomilk.org>
Autobuild-Date(master): Tue Aug  8 13:59:58 UTC 2023 on atb-devel-224

(cherry picked from commit f348b84fbcf203ab1ba92840cf7aecd55dbf9aa0)

Autobuild-User(v4-17-test): Jule Anger <janger@samba.org>
Autobuild-Date(v4-17-test): Tue Aug 15 09:00:14 UTC 2023 on sn-devel-184
This commit is contained in:
Stefan Metzmacher 2023-08-03 15:45:45 +02:00 committed by Jule Anger
parent f3d5e3add5
commit 8738efc404
2 changed files with 31 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
selftest/knownfail.d/samba3.smb2.multichannel
View File

@ -1 +0,0 @@
^samba3.smb2.multichannel.bugs.bug_15346

31
source3/smbd/smbXsrv_client.c
View File

@ -487,6 +487,7 @@ struct smb2srv_client_mc_negprot_state {
struct tevent_context *ev;
struct smbd_smb2_request *smb2req;
struct db_record *db_rec;
struct server_id sent_server_id;
uint64_t watch_instance;
uint32_t last_seqnum;
struct tevent_req *filter_subreq;
@ -529,6 +530,8 @@ struct tevent_req *smb2srv_client_mc_negprot_send(TALLOC_CTX *mem_ctx,
tevent_req_set_cleanup_fn(req, smb2srv_client_mc_negprot_cleanup);
server_id_set_disconnected(&state->sent_server_id);
smb2srv_client_mc_negprot_next(req);
if (!tevent_req_is_in_progress(req)) {
@ -624,6 +627,30 @@ verify_again:
return;
}
if (server_id_equal(&state->sent_server_id, &global->server_id)) {
/*
* We hit a race with other concurrent connections,
* which have woken us.
*
* We already sent the pass or drop message to
* the process, so we need to wait for a
* response and not pass the connection
* again! Otherwise the process would
* receive the same tcp connection via
* more than one file descriptor and
* create more than one smbXsrv_connection
* structure for the same tcp connection,
* which means the client would see more
* than one SMB2 negprot response to its
* single SMB2 netprot request and we
* as server get the session keys and
* message id validation wrong
*/
goto watch_again;
}
server_id_set_disconnected(&state->sent_server_id);
/*
* If last_server_id is set, we expect
* smbXsrv_client_global_verify_record()
@ -659,6 +686,7 @@ verify_again:
*/
goto verify_again;
}
state->sent_server_id = global->server_id;
if (tevent_req_nterror(req, status)) {
return;
}
@ -673,11 +701,14 @@ verify_again:
*/
goto verify_again;
}
state->sent_server_id = global->server_id;
if (tevent_req_nterror(req, status)) {
return;
}
}
watch_again:
/*
* If the record changed, but we are not happy with the change yet,
* we better remove ourself from the waiter list