From 665f9b5dc7760c5cfae76fa7fd99af3feadf6cc3 Mon Sep 17 00:00:00 2001 From: tchuna Date: Wed, 12 Aug 2026 15:19:23 +0200 Subject: [PATCH 1/3] Fix race between SendMessage and server-initiated key re-exchange SendMessage checked the key exchange wait handle before acquiring the socket write lock. When a server-initiated re-exchange started in that window, the client's SSH_MSG_KEXINIT could be sent first, after which the already in-flight data message violated RFC 4253 section 7.1. Strict servers (e.g. ProFTPD mod_sftp) then fail the exchange or drop the connection. SendMessage now re-checks the wait handle while holding the write lock and goes back to waiting when a re-exchange has started in the meantime. The packet is also built entirely under the write lock, so a completing re-exchange can no longer swap the client cipher, MAC or compression state in the middle of building a packet. The race does not reproduce against the OpenSSH test server (which is why the attempt in #1774 stayed green): OpenSSH queues non key exchange output while a re-exchange is in progress and tolerates the client data that slips in. ProFTPD mod_sftp does not, so this adds an integration test which reproduces the failure with concurrent SFTP uploads against a ProFTPD server configured to re-key every 1 MB. Without the fix the test failed 7 out of 8 runs; with it, it passes consistently. Fixes #1764. --- src/Renci.SshNet/Session.cs | 159 ++++++++++-------- .../.dockerignore | 1 + .../Logging/TextWriterLogger.cs | 45 +++++ .../Logging/TextWriterLoggerProvider.cs | 18 ++ .../ProFtpdRekeyTests.cs | 154 +++++++++++++++++ .../proftpd/Dockerfile | 11 ++ .../proftpd/proftpd.conf | 26 +++ 7 files changed, 344 insertions(+), 70 deletions(-) create mode 100644 test/Renci.SshNet.IntegrationTests/Logging/TextWriterLogger.cs create mode 100644 test/Renci.SshNet.IntegrationTests/Logging/TextWriterLoggerProvider.cs create mode 100644 test/Renci.SshNet.IntegrationTests/ProFtpdRekeyTests.cs create mode 100644 test/Renci.SshNet.IntegrationTests/proftpd/Dockerfile create mode 100644 test/Renci.SshNet.IntegrationTests/proftpd/proftpd.conf diff --git a/src/Renci.SshNet/Session.cs b/src/Renci.SshNet/Session.cs index 5468b53c8..a5d662779 100644 --- a/src/Renci.SshNet/Session.cs +++ b/src/Renci.SshNet/Session.cs @@ -1045,17 +1045,41 @@ internal void WaitOnHandle(WaitHandle waitHandle, TimeSpan timeout) /// The size of the packet exceeds the maximum size defined by the protocol. internal void SendMessage(Message message) { - if (!_socket.IsConnected()) + while (true) { - throw new SshConnectionException("Client not connected."); - } + if (!_socket.IsConnected()) + { + throw new SshConnectionException("Client not connected."); + } - if (!_keyExchangeCompletedWaitHandle.IsSet && message is not IKeyExchangedAllowed) - { - // Wait for key exchange to be completed - WaitOnHandle(_keyExchangeCompletedWaitHandle.WaitHandle); + if (!_keyExchangeCompletedWaitHandle.IsSet && message is not IKeyExchangedAllowed) + { + // Wait for key exchange to be completed + WaitOnHandle(_keyExchangeCompletedWaitHandle.WaitHandle); + } + + // 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) + { + if (!_keyExchangeCompletedWaitHandle.IsSet && message is not IKeyExchangedAllowed) + { + // A key re-exchange started between the check above and acquiring the + // write lock. Our SSH_MSG_KEXINIT may already have been sent, in which + // case sending this message now would violate RFC 4253 section 7.1 and + // cause the server to drop the connection. Go back to waiting for the + // key exchange to complete. + continue; + } + + SendMessageWithinWriteLock(message); + return; + } } + } + private void SendMessageWithinWriteLock(Message message) + { if (_logger.IsEnabled(LogLevel.Trace)) { _logger.LogTrace("[{SessionId}] Sending message {MessageName}({MessageNumber}) to server: '{Message}'.", SessionIdHex, message.MessageName, message.MessageNumber, message.ToString()); @@ -1074,82 +1098,77 @@ internal void SendMessage(Message message) 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, - _clientCompression, - _clientEtm || _clientAead, - macLength); + var activeBufferLength = message.GetPacket( + ref _sendBuffer, + paddingMultiplier, + _clientCompression, + _clientEtm || _clientAead, + macLength); - // write outbound packet sequence to start of packet data - BinaryPrimitives.WriteUInt32BigEndian(_sendBuffer, _outboundPacketSequence); + // write outbound packet sequence to start of packet data + BinaryPrimitives.WriteUInt32BigEndian(_sendBuffer, _outboundPacketSequence); - if (_clientMac != null && !_clientEtm) - { - // non-ETM mac = MAC(key, sequence_number || unencrypted_packet) + if (_clientMac != null && !_clientEtm) + { + // non-ETM mac = MAC(key, sequence_number || unencrypted_packet) - var hashSuccess = _clientMac.TryComputeHash( - buffer: _sendBuffer, - offset: 0, - count: activeBufferLength - macLength, - destination: _sendBuffer.AsSpan(activeBufferLength - macLength), - bytesWritten: out var bytesWritten); + var hashSuccess = _clientMac.TryComputeHash( + buffer: _sendBuffer, + offset: 0, + count: activeBufferLength - macLength, + destination: _sendBuffer.AsSpan(activeBufferLength - macLength), + bytesWritten: out var bytesWritten); - Debug.Assert(hashSuccess && bytesWritten == macLength); - } + Debug.Assert(hashSuccess && bytesWritten == macLength); + } - if (_clientCipher != null) - { - _clientCipher.SetSequenceNumber(_outboundPacketSequence); + if (_clientCipher != null) + { + _clientCipher.SetSequenceNumber(_outboundPacketSequence); - // Not encrypting the sequence number (it is not part of the packet), - // nor the packet length for ETM. - var offset = _clientEtm ? 8 : 4; + // Not encrypting the sequence number (it is not part of the packet), + // nor the packet length for ETM. + var offset = _clientEtm ? 8 : 4; - var numberOfBytesEncrypted = _clientCipher.Encrypt( - input: _sendBuffer, - offset, - length: activeBufferLength - offset - macLength, - output: _sendBuffer, - outputOffset: offset); + var numberOfBytesEncrypted = _clientCipher.Encrypt( + input: _sendBuffer, + offset, + length: activeBufferLength - offset - macLength, + output: _sendBuffer, + outputOffset: offset); - Debug.Assert(numberOfBytesEncrypted == activeBufferLength - offset - macLength + (_clientAead ? macLength : 0)); - } + Debug.Assert(numberOfBytesEncrypted == activeBufferLength - offset - macLength + (_clientAead ? macLength : 0)); + } - if (_clientMac != null && _clientEtm) - { - // ETM mac = MAC(key, sequence_number || packet_length || encrypted_packet) + if (_clientMac != null && _clientEtm) + { + // ETM mac = MAC(key, sequence_number || packet_length || encrypted_packet) - var hashSuccess = _clientMac.TryComputeHash( - buffer: _sendBuffer, - offset: 0, - count: activeBufferLength - macLength, - destination: _sendBuffer.AsSpan(activeBufferLength - macLength), - bytesWritten: out var bytesWritten); + var hashSuccess = _clientMac.TryComputeHash( + buffer: _sendBuffer, + offset: 0, + count: activeBufferLength - macLength, + destination: _sendBuffer.AsSpan(activeBufferLength - macLength), + bytesWritten: out var bytesWritten); - Debug.Assert(hashSuccess && bytesWritten == macLength); - } + Debug.Assert(hashSuccess && bytesWritten == macLength); + } - SendPacket(_sendBuffer, 4, activeBufferLength - 4); + SendPacket(_sendBuffer, 4, activeBufferLength - 4); - if (_isStrictKex && message is NewKeysMessage) - { - _outboundPacketSequence = 0; - } - else - { - // increment the packet sequence number only after we're sure the packet has - // been sent; even though it's only used for the MAC, it needs to be incremented - // for each package sent. - // - // the server will use it to verify the data integrity, and as such the order in - // which messages are sent must follow the outbound packet sequence number - _outboundPacketSequence++; - } + if (_isStrictKex && message is NewKeysMessage) + { + _outboundPacketSequence = 0; + } + else + { + // increment the packet sequence number only after we're sure the packet has + // been sent; even though it's only used for the MAC, it needs to be incremented + // for each package sent. + // + // the server will use it to verify the data integrity, and as such the order in + // which messages are sent must follow the outbound packet sequence number + _outboundPacketSequence++; } } diff --git a/test/Renci.SshNet.IntegrationTests/.dockerignore b/test/Renci.SshNet.IntegrationTests/.dockerignore index a8eb4de7e..5f3d7f5e8 100644 --- a/test/Renci.SshNet.IntegrationTests/.dockerignore +++ b/test/Renci.SshNet.IntegrationTests/.dockerignore @@ -1,3 +1,4 @@ * !server/* !user/* +!proftpd/* diff --git a/test/Renci.SshNet.IntegrationTests/Logging/TextWriterLogger.cs b/test/Renci.SshNet.IntegrationTests/Logging/TextWriterLogger.cs new file mode 100644 index 000000000..43694f89d --- /dev/null +++ b/test/Renci.SshNet.IntegrationTests/Logging/TextWriterLogger.cs @@ -0,0 +1,45 @@ +#nullable enable + +using Microsoft.Extensions.Logging; + +namespace Renci.SshNet.IntegrationTests.Logging +{ + internal class TextWriterLogger(TextWriter writer, string categoryName) : ILogger + { + public IDisposable? BeginScope(TState state) + where TState : notnull + { + return null; + } + + public bool IsEnabled(LogLevel logLevel) + { + return true; + } + + public void Log(LogLevel logLevel, EventId eventId, TState state, Exception? exception, Func formatter) + { + StringBuilder sb = new StringBuilder(); + sb.Append(logLevel); + sb.Append(": "); + sb.Append(categoryName); + sb.Append(": "); + + string message = formatter(state, exception); + sb.Append(message); + + if (exception != null) + { + sb.Append(": "); + sb.Append(exception); + } + + string line = sb.ToString(); + + lock (writer) + { + writer.WriteLine(line); + } + } + } +} diff --git a/test/Renci.SshNet.IntegrationTests/Logging/TextWriterLoggerProvider.cs b/test/Renci.SshNet.IntegrationTests/Logging/TextWriterLoggerProvider.cs new file mode 100644 index 000000000..ea1715b0b --- /dev/null +++ b/test/Renci.SshNet.IntegrationTests/Logging/TextWriterLoggerProvider.cs @@ -0,0 +1,18 @@ +#nullable enable + +using Microsoft.Extensions.Logging; + +namespace Renci.SshNet.IntegrationTests.Logging +{ + internal class TextWriterLoggerProvider(TextWriter writer) : ILoggerProvider + { + public ILogger CreateLogger(string categoryName) + { + return new TextWriterLogger(writer, categoryName); + } + + public void Dispose() + { + } + } +} diff --git a/test/Renci.SshNet.IntegrationTests/ProFtpdRekeyTests.cs b/test/Renci.SshNet.IntegrationTests/ProFtpdRekeyTests.cs new file mode 100644 index 000000000..93b265d38 --- /dev/null +++ b/test/Renci.SshNet.IntegrationTests/ProFtpdRekeyTests.cs @@ -0,0 +1,154 @@ +#if NET // The test uses Parallel.ForEachAsync, which is not available on .NET Framework. + +using DotNet.Testcontainers.Builders; +using DotNet.Testcontainers.Containers; +using DotNet.Testcontainers.Images; + +using Microsoft.Extensions.Logging; + +using Renci.SshNet.IntegrationTests.Logging; + +namespace Renci.SshNet.IntegrationTests +{ + /// + /// Reproduces https://github.com/sshnet/SSH.NET/issues/1764: connection failures during + /// SFTP transfers when the server initiates a key re-exchange. + /// + /// Unlike OpenSSH, which queues non key exchange output while a re-exchange is in progress, + /// ProFTPD mod_sftp keeps sending channel messages (SSH_MSG_CHANNEL_WINDOW_ADJUST, + /// SSH_MSG_CHANNEL_DATA) after it has sent its SSH_MSG_KEXINIT. The same load which passes + /// against the OpenSSH test server therefore fails against ProFTPD with either + /// "Message type 93 is not valid in the current context." or a connection drop + /// ("Key exchange failed"). These tests run against a ProFTPD container configured to + /// re-key every 1 MB (see proftpd/proftpd.conf) to give the race many trials per upload. + /// + /// + /// Trace-level logging to a file is enabled for the duration of these tests: the small + /// per-message overhead on the message listener thread widens the window between the + /// arrival of the server's SSH_MSG_KEXINIT and its processing, during which concurrent + /// uploaders keep sending data. This mirrors real-world conditions (applications with + /// trace logging enabled, or slower links) and makes the race fail reliably on loopback. + /// + /// + [TestClass] + public sealed class ProFtpdRekeyTests : TestBase + { + private static IFutureDockerImage _proFtpdImage; + private static IContainer _proFtpdServer; + private static string _proFtpdHostName; + private static ushort _proFtpdPort; + private static StreamWriter _traceLogWriter; + private static ILoggerFactory _traceLoggerFactory; + + [ClassInitialize] + [System.Diagnostics.CodeAnalysis.SuppressMessage("Style", "IDE0060:Remove unused parameter", Justification = "MSTests requires context parameter")] + public static async Task ClassInitialize(TestContext context) + { + _traceLogWriter = new StreamWriter(Path.GetTempFileName()) { AutoFlush = true }; + _traceLoggerFactory = LoggerFactory.Create(builder => + { + builder.SetMinimumLevel(LogLevel.Trace); + builder.AddProvider(new TextWriterLoggerProvider(_traceLogWriter)); + }); + + SshNetLoggingConfiguration.InitializeLogging(_traceLoggerFactory); + + _proFtpdImage = new ImageFromDockerfileBuilder() + .WithName("renci-ssh-tests-proftpd-image") + .WithDockerfileDirectory(CommonDirectoryPath.GetSolutionDirectory(), Path.Combine("test", "Renci.SshNet.IntegrationTests")) + .WithDockerfile("proftpd/Dockerfile") + .WithDeleteIfExists(true) + .Build(); + + await _proFtpdImage.CreateAsync(context.CancellationToken); + + _proFtpdServer = new ContainerBuilder(_proFtpdImage) + .WithHostname("renci-ssh-tests-proftpd") + .WithPortBinding(22, true) + .Build(); + + await _proFtpdServer.StartAsync(context.CancellationToken); + + _proFtpdPort = _proFtpdServer.GetMappedPublicPort(22); + _proFtpdHostName = _proFtpdServer.Hostname; + } + + [ClassCleanup] + public static async Task ClassCleanup() + { + if (_proFtpdServer != null) + { + await _proFtpdServer.DisposeAsync(); + } + + if (_proFtpdImage != null) + { + await _proFtpdImage.DisposeAsync(); + } + + // Restore the assembly-wide logging configuration set up by InfrastructureFixture. + var defaultLoggerFactory = LoggerFactory.Create(builder => + { + builder.SetMinimumLevel(LogLevel.Debug); + builder.AddTestConsoleLogger(); + }); + + SshNetLoggingConfiguration.InitializeLogging(defaultLoggerFactory); + + _traceLoggerFactory?.Dispose(); + _traceLogWriter?.Dispose(); + } + + [TestMethod] + public async Task Sftp_ConcurrentUploads_WithServerRekey() + { + const int fileSize = 128 * 1024 * 1024; + const int concurrentUploads = 4; + const int attempts = 3; + + using (var sftp = new SftpClient(_proFtpdHostName, _proFtpdPort, "sshnet", "ssh4ever")) + { + await sftp.ConnectAsync(CancellationToken.None); + + for (var attempt = 0; attempt < attempts; attempt++) + { + await Parallel.ForEachAsync(Enumerable.Range(0, concurrentUploads), async (i, ct) => + { + var localFile = CreateZeroFilledTempFile(fileSize); + + try + { + var remoteFile = $"rekey-test-{i}"; + + using (var fileStream = File.OpenRead(localFile)) + { + await sftp.UploadFileAsync(fileStream, remoteFile, ct); + } + + var remoteLength = (await sftp.GetAsync(remoteFile, ct)).Attributes.Size; + + Assert.AreEqual(fileSize, remoteLength); + } + finally + { + File.Delete(localFile); + } + }); + } + } + } + + private static string CreateZeroFilledTempFile(long size) + { + var file = Path.GetTempFileName(); + + using (var fs = File.OpenWrite(file)) + { + fs.SetLength(size); + } + + return file; + } + } +} +#endif diff --git a/test/Renci.SshNet.IntegrationTests/proftpd/Dockerfile b/test/Renci.SshNet.IntegrationTests/proftpd/Dockerfile new file mode 100644 index 000000000..138e7c358 --- /dev/null +++ b/test/Renci.SshNet.IntegrationTests/proftpd/Dockerfile @@ -0,0 +1,11 @@ +# ProFTPD with mod_sftp, used by ProFtpdRekeyTests to reproduce +# https://github.com/sshnet/SSH.NET/issues/1764 (alpine:3.24 ships ProFTPD 1.3.9c). +FROM alpine:3.24 +RUN apk add --no-cache proftpd proftpd-mod_sftp openssh-keygen +RUN ssh-keygen -t rsa -b 3072 -N "" -m PEM -f /etc/proftpd/host_rsa +RUN chmod 400 /etc/proftpd/host_rsa +RUN adduser -D sshnet +RUN echo 'sshnet:ssh4ever' | chpasswd +COPY proftpd/proftpd.conf /etc/proftpd/proftpd.conf +EXPOSE 22 +CMD ["proftpd", "--nodaemon", "--config", "/etc/proftpd/proftpd.conf"] diff --git a/test/Renci.SshNet.IntegrationTests/proftpd/proftpd.conf b/test/Renci.SshNet.IntegrationTests/proftpd/proftpd.conf new file mode 100644 index 000000000..8bd8ee6d7 --- /dev/null +++ b/test/Renci.SshNet.IntegrationTests/proftpd/proftpd.conf @@ -0,0 +1,26 @@ +ServerName "renci-ssh-tests-proftpd" +ServerType standalone +DefaultServer on +# SFTPEngine converts this server to SSH/SFTP entirely; no plain FTP is served. +Port 22 +User nobody +Group nobody +DefaultRoot ~ +AllowOverwrite on +PidFile /var/run/proftpd.pid +ScoreboardFile /var/run/proftpd.scoreboard + +LoadModule mod_sftp.c + + + SFTPEngine on + SFTPHostKey /etc/proftpd/host_rsa + SFTPAuthMethods password + SFTPLog /var/log/sftp.log + # Re-key every 1 MB so that the key re-exchange races of + # https://github.com/sshnet/SSH.NET/issues/1764 get many trials per upload. + # Unlike OpenSSH, ProFTPD mod_sftp keeps sending channel messages + # (SSH_MSG_CHANNEL_WINDOW_ADJUST / SSH_MSG_CHANNEL_DATA) after it has + # initiated a re-exchange, which is what triggers the client-side failures. + SFTPRekey required 3600 1 + From a1cc0a824810b57b98eceea5793e16f0d9118613 Mon Sep 17 00:00:00 2001 From: tchuna Date: Wed, 12 Aug 2026 16:05:55 +0200 Subject: [PATCH 2/3] Make ProFTPD rekey test pass on CI - Wait until the container's SSH port is actually listening before connecting. On the Linux CI runner the first connection attempt arrived before proftpd bound the port, failing the protocol version exchange with "Connection reset by peer". This is the same behavior InfrastructureFixture works around with a Task.Delay(300) on Unix; use an explicit wait strategy instead. - Skip the test on the Windows CI runners: Docker there is in Windows containers mode and cannot run the Linux ProFTPD image ("no matching manifest for windows/amd64"), mirroring the Windows CI condition used by InfrastructureFixture. --- .../Renci.SshNet.IntegrationTests/ProFtpdRekeyTests.cs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test/Renci.SshNet.IntegrationTests/ProFtpdRekeyTests.cs b/test/Renci.SshNet.IntegrationTests/ProFtpdRekeyTests.cs index 93b265d38..a66e9f70a 100644 --- a/test/Renci.SshNet.IntegrationTests/ProFtpdRekeyTests.cs +++ b/test/Renci.SshNet.IntegrationTests/ProFtpdRekeyTests.cs @@ -44,6 +44,15 @@ public sealed class ProFtpdRekeyTests : TestBase [System.Diagnostics.CodeAnalysis.SuppressMessage("Style", "IDE0060:Remove unused parameter", Justification = "MSTests requires context parameter")] public static async Task ClassInitialize(TestContext context) { + // The Windows Tests in CI cannot run the ProFTPD container: Docker on the Windows + // runners is in Windows containers mode ("no matching manifest for windows/amd64"), + // which is why the OpenSSH server for the other integration tests is set up in + // WSL2 with Podman instead (see InfrastructureFixture). + if (OperatingSystem.IsWindows() && Environment.GetEnvironmentVariable("CI") == "true") + { + Assert.Inconclusive("Requires a container runtime able to run Linux containers."); + } + _traceLogWriter = new StreamWriter(Path.GetTempFileName()) { AutoFlush = true }; _traceLoggerFactory = LoggerFactory.Create(builder => { @@ -65,6 +74,7 @@ public static async Task ClassInitialize(TestContext context) _proFtpdServer = new ContainerBuilder(_proFtpdImage) .WithHostname("renci-ssh-tests-proftpd") .WithPortBinding(22, true) + .WithWaitStrategy(Wait.ForUnixContainer().UntilInternalTcpPortIsAvailable(22)) .Build(); await _proFtpdServer.StartAsync(context.CancellationToken); From e93bc7586447e6aabb6af1e2268ac08b54eaf192 Mon Sep 17 00:00:00 2001 From: Volodymyr Vlasenko Date: Fri, 25 Sep 2026 12:36:46 -0400 Subject: [PATCH 3/3] Accept and defer connection protocol messages received during a key re-exchange A server may send connection protocol messages (e.g. SSH_MSG_CHANNEL_DATA) after its SSH_MSG_KEXINIT and before its SSH_MSG_NEWKEYS, for instance the response to a request that crossed the KEXINIT on the wire. The session rejected these with "Message type 94 is not valid in the current context", which tears down the session. For a key re-exchange: - keep the connection protocol messages (80-100) enabled in the message factory; the strict key exchange restriction applies to the initial key exchange only - don't process them until the key exchange has completed, since processing may send a message (e.g. SSH_MSG_CHANNEL_WINDOW_ADJUST) that is held back until then, which only the message listener thread can bring about - give them their own copy of the payload, as the receive buffer they would otherwise reference is reused before they are processed This is the receive-side counterpart of the send-side fix for #1764. --- src/Renci.SshNet/Session.cs | 94 ++++++++++- src/Renci.SshNet/SshMessageFactory.cs | 24 ++- .../Classes/SessionTest_ConnectedBase.cs | 11 ++ ...rRekeysAndSendsChannelDataBeforeNewKeys.cs | 155 ++++++++++++++++++ .../Classes/SshMessageFactoryTest.cs | 59 +++++++ 5 files changed, 336 insertions(+), 7 deletions(-) create mode 100644 test/Renci.SshNet.Tests/Classes/SessionTest_Connected_ServerRekeysAndSendsChannelDataBeforeNewKeys.cs diff --git a/src/Renci.SshNet/Session.cs b/src/Renci.SshNet/Session.cs index eaa8c47c5..f0b3b757d 100644 --- a/src/Renci.SshNet/Session.cs +++ b/src/Renci.SshNet/Session.cs @@ -1,5 +1,6 @@ using System; using System.Buffers.Binary; +using System.Collections.Generic; using System.Diagnostics; using System.Globalization; using System.Linq; @@ -89,6 +90,15 @@ public sealed class Session : ISession /// private readonly Lock _socketWriteLock = new Lock(); + /// + /// The connection protocol messages that were received while a key re-exchange was in progress. They + /// are processed once the key exchange has completed, see . + /// + /// + /// Only accessed by the message listener thread. + /// + private readonly Queue _messagesReceivedDuringKeyExchange = new Queue(); + /// /// Holds an object that is used to ensure only a single thread can dispose /// at any given time. @@ -1201,14 +1211,21 @@ private bool TrySendMessage(Message message) /// /// Receives the message from the server. /// + /// The to read from. + /// + /// if the message is a connection protocol message that was received during a key + /// re-exchange, and is to be processed once that has completed; otherwise, . + /// /// /// The incoming SSH message, or if the connection with the SSH server was closed. /// /// /// We need no locking here since all messages are read by a single thread. /// - private Message ReceiveMessage(Socket socket) + private Message ReceiveMessage(Socket socket, out bool processAfterKeyExchange) { + processAfterKeyExchange = false; + // The length of the "packet length" field in bytes const int packetLengthFieldLength = 4; @@ -1426,12 +1443,22 @@ private Message ReceiveMessage(Socket socket) throw new SshConnectionException("Inbound packet sequence number is about to wrap during initial key exchange.", DisconnectReason.KeyExchangeFailed); } + if (payload.Count > 0 && IsToBeProcessedAfterKeyExchange(payload.Array[payload.Offset])) + { + // Unlike any other message, this one is not going to be processed before the next call to + // ReceiveMessage, which reuses the receive buffer that the deserialised message would + // otherwise reference (e.g. the data of an SSH_MSG_CHANNEL_DATA). So give it its own copy. + payload = new ArraySegment(payload.AsSpan().ToArray()); + processAfterKeyExchange = true; + } + var message = LoadMessage(payload.Array, payload.Offset, payload.Count); // The deserialised message may still reference data in the buffer, so calling Discard // here might seem misguided. It is OK because Discard does not mutate the buffer // and it will not be touched again until the next call to ReceiveMessage, which will - // only occur after the message has been fully processed. + // only occur after the message has been fully processed (or has been given its own copy + // of the data, see above). _receiveBuffer.Discard(totalPacketLength); return message; @@ -1518,8 +1545,15 @@ internal void OnKeyExchangeInitReceived(KeyExchangeInitMessage message) } } - // Disable messages that are not key exchange related - _sshMessageFactory.DisableNonKeyExchangeMessages(_isStrictKex); + // Disable messages that are not key exchange related. + // + // The strict key exchange restrictions apply to the initial key exchange only. In a key + // re-exchange of an established session we still accept connection protocol messages (e.g. data + // that the server had already queued): they are protected by the old keys until the server's + // SSH_MSG_NEWKEYS. They are not processed until the key exchange has completed, see MessageListener. + _sshMessageFactory.DisableNonKeyExchangeMessages( + strict: _isStrictKex && _isInitialKex, + allowConnectionProtocolMessages: !_isInitialKex); _keyExchange = _serviceFactory.CreateKeyExchange(ConnectionInfo.KeyExchangeAlgorithms, message.KeyExchangeAlgorithms); @@ -1944,15 +1978,33 @@ private void MessageListener() // remain in message loop until socket is shut down or until we're disconnecting while (true) { - var message = ReceiveMessage(_socket); + var message = ReceiveMessage(_socket, out var processAfterKeyExchange); if (message is null) { // Connection with SSH server was closed, so break out of the message loop break; } + if (processAfterKeyExchange) + { + // A connection protocol message received during a key re-exchange. Processing it now could + // make it send a message (e.g. SSH_MSG_CHANNEL_WINDOW_ADJUST) which is held back until the + // key exchange completes, which only this thread can bring about by reading the + // rest of the key exchange. + _messagesReceivedDuringKeyExchange.Enqueue(message); + continue; + } + // process message message.Process(this); + + if (message is NewKeysMessage) + { + // The key exchange is now complete (and sending is allowed again), so we can process the + // messages that were received during it - in the order in which they were received, and + // before reading the next packet. + ProcessMessagesReceivedDuringKeyExchange(); + } } // connection with SSH server was closed or socket was disposed @@ -1973,6 +2025,37 @@ private void MessageListener() } } + /// + /// Determines whether a received message has to wait until the key re-exchange that is in progress has completed. + /// + /// The number of the received message. + /// + /// if the message is a connection protocol message and a key + /// re-exchange is in progress; otherwise, . + /// + private bool IsToBeProcessedAfterKeyExchange(byte messageNumber) + { + if (messageNumber < SshMessageFactory.FirstConnectionProtocolMessageNumber || _isInitialKex) + { + return false; + } + + var keyExchangeCompletedWaitHandle = _keyExchangeCompletedWaitHandle; + + return keyExchangeCompletedWaitHandle is { IsSet: false }; + } + + /// + /// Processes the messages that were received while a key re-exchange was in progress. + /// + private void ProcessMessagesReceivedDuringKeyExchange() + { + while (_messagesReceivedDuringKeyExchange.Count > 0) + { + _messagesReceivedDuringKeyExchange.Dequeue().Process(this); + } + } + /// /// Raises the event. /// @@ -2009,6 +2092,7 @@ private void Reset() _ = _exceptionWaitHandle?.Reset(); _keyExchangeCompletedWaitHandle?.Reset(); _ = _messageListenerCompleted?.Set(); + _messagesReceivedDuringKeyExchange.Clear(); SessionId = null; _isDisconnectMessageSent = false; diff --git a/src/Renci.SshNet/SshMessageFactory.cs b/src/Renci.SshNet/SshMessageFactory.cs index 9985e1611..c41aa56e0 100644 --- a/src/Renci.SshNet/SshMessageFactory.cs +++ b/src/Renci.SshNet/SshMessageFactory.cs @@ -66,6 +66,12 @@ internal sealed class SshMessageFactory /// internal const byte HighestMessageNumber = 100; + /// + /// Defines the lowest message number of the connection protocol (RFC 4254), which is + /// SSH_MSG_GLOBAL_REQUEST. The connection protocol messages run up to . + /// + internal const byte FirstConnectionProtocolMessageNumber = 80; + /// /// Initializes a new instance of the class. /// @@ -126,7 +132,18 @@ public Message Create(byte messageNumber) /// The relevant KEX Reply MSG will be allowed from a sub class of KeyExchange class. /// For example, it calls Session.RegisterMessage("SSH_MSG_KEX_ECDH_REPLY"); if the curve25519-sha256 KEX algorithm is selected per negotiation. /// - public void DisableNonKeyExchangeMessages(bool strict) + /// + /// to leave the connection protocol messages (80 - 100, e.g. SSH_MSG_CHANNEL_DATA) enabled; + /// otherwise . Ignored in strict key exchange mode. + /// + /// This is meant for a key re-exchange in an established session. The sender of a message is not + /// supposed to send connection protocol messages between its SSH_MSG_KEXINIT and its SSH_MSG_NEWKEYS, + /// but the messages the peer had already queued when it decided to re-exchange keys, or that a + /// less strict peer sends anyway, are still protected by the old keys and are safe to accept. + /// The caller is responsible for not processing them until the key exchange has completed. + /// + /// + public void DisableNonKeyExchangeMessages(bool strict, bool allowConnectionProtocolMessages = false) { for (var i = 0; i < AllMessages.Length; i++) { @@ -142,7 +159,10 @@ public void DisableNonKeyExchangeMessages(bool strict) } else { - if (messageNumber is (> 2 and < 20) or > 30) + var isNonKeyExchangeMessage = messageNumber is (> 2 and < 20) or > 30; + var isRetained = allowConnectionProtocolMessages && messageNumber >= FirstConnectionProtocolMessageNumber; + + if (isNonKeyExchangeMessage && !isRetained) { _enabledMessagesByNumber[messageNumber] = null; } diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectedBase.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectedBase.cs index c8ff510e9..79c8e7207 100644 --- a/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectedBase.cs +++ b/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectedBase.cs @@ -38,6 +38,14 @@ public abstract class SessionTest_ConnectedBase protected Random Random { get; private set; } protected byte[] SessionId { get; private set; } + + /// + /// The name of the (mocked) key exchange algorithm that the "server" offers in its KEXINIT. + /// + protected string KeyExchangeAlgorithm + { + get { return _keyExchangeAlgorithm; } + } protected ConnectionInfo ConnectionInfo { get; private set; } protected IList DisconnectedRegister { get; private set; } protected IList> DisconnectReceivedRegister { get; private set; } @@ -208,6 +216,9 @@ private void SetupMocks() _ = _keyExchangeMock.Setup(p => p.Name) .Returns(_keyExchangeAlgorithm); _ = _keyExchangeMock.Setup(p => p.Start(Session, It.IsAny(), false)); + + // a key re-exchange initiated by the server: the client is expected to send its own KEXINIT + _ = _keyExchangeMock.Setup(p => p.Start(Session, It.IsAny(), true)); _ = _keyExchangeMock.Setup(p => p.ExchangeHash) .Returns(SessionId); _ = _keyExchangeMock.Setup(p => p.CreateServerCipher(out It.Ref.IsAny)) diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_Connected_ServerRekeysAndSendsChannelDataBeforeNewKeys.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_Connected_ServerRekeysAndSendsChannelDataBeforeNewKeys.cs new file mode 100644 index 000000000..59dbbfa14 --- /dev/null +++ b/test/Renci.SshNet.Tests/Classes/SessionTest_Connected_ServerRekeysAndSendsChannelDataBeforeNewKeys.cs @@ -0,0 +1,155 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Net.Sockets; +using System.Threading; + +using Microsoft.VisualStudio.TestTools.UnitTesting; + +using Renci.SshNet.Messages.Connection; +using Renci.SshNet.Messages.Transport; +using Renci.SshNet.Tests.Common; + +namespace Renci.SshNet.Tests.Classes +{ + /// + /// The server starts a key re-exchange (SSH_MSG_KEXINIT) and then, before it sends its SSH_MSG_NEWKEYS, + /// sends an SSH_MSG_CHANNEL_DATA. This is what the client sees when the server has an SFTP response + /// to a request that crossed the KEXINIT on the wire (RFC 4253 section 7.1 asks a sender not to do that, + /// but not every server is that strict, and the data is still protected by the old keys at this point). + /// + /// + /// The client must neither fail the session ("Message type 94 is not valid in the current context") + /// nor dispatch the channel data while the key exchange is still in progress: a handler that answers + /// with an SSH_MSG_CHANNEL_WINDOW_ADJUST (as Channel.OnData does) would otherwise block on the + /// key exchange gate, which can only be opened by the very thread that is running the handler. + /// + [TestClass] + public class SessionTest_Connected_ServerRekeysAndSendsChannelDataBeforeNewKeys : SessionTest_ConnectedBase + { + private const uint LocalChannelNumber = 0; + + private byte[] _channelData; + private List _channelDataReceived; + private ManualResetEventSlim _windowAdjustReceivedByServer; + + protected override void SetupData() + { + base.SetupData(); + + _channelData = new byte[] { 1, 2, 3, 4, 5 }; + _channelDataReceived = new List(); + _windowAdjustReceivedByServer = new ManualResetEventSlim(); + + Session.ChannelDataReceived += (sender, args) => + { + lock (_channelDataReceived) + { + // the message may reference a larger buffer, only Offset and Size delimit the data + _channelDataReceived.Add(args.Message.Data.Skip(args.Message.Offset).Take(args.Message.Size).ToArray()); + } + + // Answer the way Channel.OnData does when the receive window needs to be adjusted. + Session.SendMessage(new ChannelWindowAdjustMessage(LocalChannelNumber, 1000)); + }; + + ServerListener.BytesReceived += (received, socket) => + { + if (GetMessageNumbers(received).Contains((byte)93)) + { + _windowAdjustReceivedByServer.Set(); + } + }; + } + + protected override void Act() + { + var keyExchangeInit = new KeyExchangeInitMessage + { + CompressionAlgorithmsClientToServer = new string[0], + CompressionAlgorithmsServerToClient = new string[0], + EncryptionAlgorithmsClientToServer = new string[0], + EncryptionAlgorithmsServerToClient = new string[0], + KeyExchangeAlgorithms = new[] { KeyExchangeAlgorithm }, + LanguagesClientToServer = new string[0], + LanguagesServerToClient = new string[0], + MacAlgorithmsClientToServer = new string[0], + MacAlgorithmsServerToClient = new string[0], + ServerHostKeyAlgorithms = new string[0] + }.GetPacket(8, null); + + var channelData = new ChannelDataMessage(LocalChannelNumber, _channelData).GetPacket(8, null); + + // The receive buffer of the session is 4 KB. These packets that follow the channel data (SSH_MSG_IGNORE is + // allowed during a key exchange) fill it up, so that the session has to reuse the part of the buffer that + // holds the channel data while that is still waiting for the key exchange to complete. + var filler = new List(); + for (var i = 0; i < 4; i++) + { + filler.Add(new IgnoreMessage(Enumerable.Repeat((byte)0xEE, 1500).ToArray()).GetPacket(8, null)); + } + + // Send all packets at once, so that the channel data is guaranteed to follow the KEXINIT on the wire. The + // server's SSH_MSG_NEWKEYS is sent by the KeyExchangeInitReceived handler in the base class, and therefore + // arrives after the channel data. + var packets = Concatenate(new[] { keyExchangeInit, channelData }.Concat(filler)); + + _ = ServerSocket.Send(packets, 0, packets.Length, SocketFlags.None); + + _ = _windowAdjustReceivedByServer.Wait(TimeSpan.FromSeconds(5)); + } + + [TestMethod] + public void ErrorOccurredIsNeverRaised() + { + Assert.IsEmpty(ErrorOccurredRegister, ErrorOccurredRegister.AsString()); + } + + [TestMethod] + public void IsConnectedShouldReturnTrue() + { + Assert.IsTrue(Session.IsConnected); + } + + [TestMethod] + public void ChannelDataIsDeliveredExactlyOnce() + { + lock (_channelDataReceived) + { + Assert.HasCount(1, _channelDataReceived); + CollectionAssert.AreEqual(_channelData, _channelDataReceived[0]); + } + } + + [TestMethod] + public void WindowAdjustSentFromChannelDataHandlerReachesServer() + { + Assert.IsTrue(_windowAdjustReceivedByServer.IsSet); + } + + /// + /// Concatenates the packets, without the 4 bytes at the start of each of them that + /// are reserved for the packet sequence number (which is not sent over the wire). + /// + private static byte[] Concatenate(IEnumerable packets) + { + return packets.SelectMany(p => p.Skip(4)).ToArray(); + } + + /// + /// Returns the message numbers of the (unencrypted) packets in . + /// + private static IEnumerable GetMessageNumbers(byte[] received) + { + var offset = 0; + while (offset + 6 <= received.Length) + { + var packetLength = (received[offset] << 24) | (received[offset + 1] << 16) | (received[offset + 2] << 8) | received[offset + 3]; + + yield return received[offset + 5]; + + offset += 4 + packetLength; + } + } + } +} diff --git a/test/Renci.SshNet.Tests/Classes/SshMessageFactoryTest.cs b/test/Renci.SshNet.Tests/Classes/SshMessageFactoryTest.cs index 74eb6f84c..eba6edecf 100644 --- a/test/Renci.SshNet.Tests/Classes/SshMessageFactoryTest.cs +++ b/test/Renci.SshNet.Tests/Classes/SshMessageFactoryTest.cs @@ -5,6 +5,7 @@ using Renci.SshNet.Common; using Renci.SshNet.Messages.Authentication; +using Renci.SshNet.Messages.Connection; using Renci.SshNet.Messages.Transport; namespace Renci.SshNet.Tests.Classes @@ -20,6 +21,20 @@ public void SetUp() _sshMessageFactory = new SshMessageFactory(); } + private void AssertMessageIsNotValidInCurrentContext(byte messageNumber) + { + try + { + _sshMessageFactory.Create(messageNumber); + Assert.Fail(); + } + catch (SshException ex) + { + Assert.IsNull(ex.InnerException); + Assert.AreEqual(string.Format(CultureInfo.CurrentCulture, "Message type {0} is not valid in the current context.", messageNumber), ex.Message); + } + } + [TestMethod] public void CreateShouldThrowSshExceptionWhenMessageIsNotEnabled() { @@ -314,6 +329,50 @@ public void DisableNonKeyExchangeMessagesShouldNotDisableKeyExchangeMessages() Assert.AreEqual(typeof(NewKeysMessage), actual.GetType()); } + [TestMethod] + public void DisableNonKeyExchangeMessagesShouldDisableConnectionProtocolMessagesByDefault() + { + const byte messageNumber = 94; + + _sshMessageFactory.EnableAndActivateMessage("SSH_MSG_CHANNEL_DATA"); + _sshMessageFactory.DisableNonKeyExchangeMessages(strict: false); + + AssertMessageIsNotValidInCurrentContext(messageNumber); + } + + [TestMethod] + public void DisableNonKeyExchangeMessagesShouldNotDisableConnectionProtocolMessagesWhenAllowed() + { + const byte messageNumber = 94; + + _sshMessageFactory.EnableAndActivateMessage("SSH_MSG_CHANNEL_DATA"); + _sshMessageFactory.DisableNonKeyExchangeMessages(strict: false, allowConnectionProtocolMessages: true); + + var actual = _sshMessageFactory.Create(messageNumber); + Assert.IsNotNull(actual); + Assert.AreEqual(typeof(ChannelDataMessage), actual.GetType()); + } + + [TestMethod] + public void DisableNonKeyExchangeMessagesShouldStillDisableOtherNonKeyExchangeMessagesWhenConnectionProtocolMessagesAreAllowed() + { + _sshMessageFactory.EnableAndActivateMessage("SSH_MSG_USERAUTH_PASSWD_CHANGEREQ"); + _sshMessageFactory.DisableNonKeyExchangeMessages(strict: false, allowConnectionProtocolMessages: true); + + AssertMessageIsNotValidInCurrentContext(60); + } + + [TestMethod] + public void DisableNonKeyExchangeMessagesShouldIgnoreAllowConnectionProtocolMessagesInStrictMode() + { + const byte messageNumber = 94; + + _sshMessageFactory.EnableAndActivateMessage("SSH_MSG_CHANNEL_DATA"); + _sshMessageFactory.DisableNonKeyExchangeMessages(strict: true, allowConnectionProtocolMessages: true); + + AssertMessageIsNotValidInCurrentContext(messageNumber); + } + [TestMethod] public void EnableActivatedMessagesShouldEnableMessagesThatWereEnabledPriorToInvokingDisableNonKeyExchangeMessages() {