diff --git a/src/Renci.SshNet/Session.cs b/src/Renci.SshNet/Session.cs
index f01756e10..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.
@@ -1010,17 +1020,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());
@@ -1039,82 +1073,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++;
}
}
@@ -1182,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;
@@ -1407,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;
@@ -1499,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);
@@ -1925,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
@@ -1954,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.
///
@@ -1990,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.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..a66e9f70a
--- /dev/null
+++ b/test/Renci.SshNet.IntegrationTests/ProFtpdRekeyTests.cs
@@ -0,0 +1,164 @@
+#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)
+ {
+ // 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 =>
+ {
+ 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)
+ .WithWaitStrategy(Wait.ForUnixContainer().UntilInternalTcpPortIsAvailable(22))
+ .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
+
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()
{