Skip to content

Fix race between SendMessage and server-initiated key re-exchange #1823

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Rob-Hague merged 1 commit into from
Nov 3, 2026
Merged

Fix race between SendMessage and server-initiated key re-exchange #1823

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/Renci.SshNet/Common/Lock.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
48 changes: 29 additions & 19 deletions src/Renci.SshNet/Session.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -1116,6 +1122,10 @@ internal void SendMessage(Message message)
_outboundPacketSequence++;
}
}
finally
{
_socketWriteLock.Exit();
}
}

/// <summary>
Expand Down Expand Up @@ -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);
Expand Down