From 76ab5032d41a6d5d8d669e94a825f3332d4a8db2 Mon Sep 17 00:00:00 2001 From: Rob Hague Date: Sun, 1 Nov 2026 15:41:47 +0100 Subject: [PATCH] Check _keyExchangeCompletedWaitHandle in write lock Fixes a race condition during key re-exchange where the message listener thread receives and responds to KEXINIT between the time that another thread sending a (non-KEX/transport) message checks _keyExchangeCompletedWaitHandle.IsSet and enters the write lock. This is a protocol error since neither party may send such messages between KEXINIT and NEWKEYS (RFC 4253 section 7.1) Fix by checking the handle state within the lock. Because sending the KEXINIT takes the same path with the same lock, it cannot preempt the other message and either it will be sent afterwards or it is sent first and the other message will correctly wait for the kex to complete. We also relax the restriction on non-KEX messages during re-exchange under strict kex, since the spec intends this just for the initial exchange. This is not strictly necessary for this fix. --- src/Renci.SshNet/Common/Lock.cs | 7 +++++ src/Renci.SshNet/Session.cs | 48 ++++++++++++++++++++------------- 2 files changed, 36 insertions(+), 19 deletions(-) diff --git a/src/Renci.SshNet/Common/Lock.cs b/src/Renci.SshNet/Common/Lock.cs index b402390ab..8d9f4e836 100644 --- a/src/Renci.SshNet/Common/Lock.cs +++ b/src/Renci.SshNet/Common/Lock.cs @@ -12,6 +12,13 @@ public bool TryEnter() #pragma warning restore CA2002 // Do not lock on objects with weak identity } + public void Enter() + { +#pragma warning disable CA2002 // Do not lock on objects with weak identity + Monitor.Enter(this); +#pragma warning restore CA2002 // Do not lock on objects with weak identity + } + public void Exit() { Monitor.Exit(this); diff --git a/src/Renci.SshNet/Session.cs b/src/Renci.SshNet/Session.cs index f01756e10..ac548b27d 100644 --- a/src/Renci.SshNet/Session.cs +++ b/src/Renci.SshNet/Session.cs @@ -1015,34 +1015,40 @@ internal void SendMessage(Message message) throw new SshConnectionException("Client not connected."); } - if (!_keyExchangeCompletedWaitHandle.IsSet && message is not IKeyExchangedAllowed) + while (true) { + _socketWriteLock.Enter(); + + if (_keyExchangeCompletedWaitHandle.IsSet || message is IKeyExchangedAllowed) + { + break; + } + // Wait for key exchange to be completed + _socketWriteLock.Exit(); WaitOnHandle(_keyExchangeCompletedWaitHandle.WaitHandle); } - if (_logger.IsEnabled(LogLevel.Trace)) + try { - _logger.LogTrace("[{SessionId}] Sending message {MessageName}({MessageNumber}) to server: '{Message}'.", SessionIdHex, message.MessageName, message.MessageNumber, message.ToString()); - } + if (_logger.IsEnabled(LogLevel.Trace)) + { + _logger.LogTrace("[{SessionId}] Sending message {MessageName}({MessageNumber}) to server: '{Message}'.", SessionIdHex, message.MessageName, message.MessageNumber, message.ToString()); + } - var paddingMultiplier = _clientCipher is null ? (byte)8 : Math.Max((byte)8, _clientCipher.MinimumSize); + var paddingMultiplier = _clientCipher is null ? (byte)8 : Math.Max((byte)8, _clientCipher.MinimumSize); - var macLength = 0; + var macLength = 0; - if (_clientAead) - { - macLength = _clientCipher.TagSize; - } - else if (_clientMac != null) - { - macLength = _clientMac.HashSize / 8; - } + if (_clientAead) + { + macLength = _clientCipher.TagSize; + } + else if (_clientMac != null) + { + macLength = _clientMac.HashSize / 8; + } - // take a write lock to ensure the outbound packet sequence number is incremented - // atomically, and only after the packet has actually been sent - lock (_socketWriteLock) - { var activeBufferLength = message.GetPacket( ref _sendBuffer, paddingMultiplier, @@ -1116,6 +1122,10 @@ internal void SendMessage(Message message) _outboundPacketSequence++; } } + finally + { + _socketWriteLock.Exit(); + } } /// @@ -1500,7 +1510,7 @@ internal void OnKeyExchangeInitReceived(KeyExchangeInitMessage message) } // Disable messages that are not key exchange related - _sshMessageFactory.DisableNonKeyExchangeMessages(_isStrictKex); + _sshMessageFactory.DisableNonKeyExchangeMessages(strict: _isStrictKex && _isInitialKex); _keyExchange = _serviceFactory.CreateKeyExchange(ConnectionInfo.KeyExchangeAlgorithms, message.KeyExchangeAlgorithms);