From cc67ddd01c2a06498182556e9d73d2bfc07d8c5c Mon Sep 17 00:00:00 2001 From: tomresno312 <251623035+tomresno312@users.noreply.github.com> Date: Mon, 27 Jul 2026 07:32:48 +0200 Subject: [PATCH 1/2] Send the SOCKS reply before any data from the target ForwardedPortDynamic hands the client's socket to the channel in Open() and only then writes its SOCKS reply. From the moment Open() returns, ChannelDirectTcpip.OnData - running on the session's message-receive thread - writes arriving channel data to that same socket. Two threads, one socket, no ordering between them. RFC 1928 section 6 requires the CONNECT reply to precede any byte from the target, so a target that speaks first (SSH, SMTP, IMAP, POP3, FTP, MySQL, PostgreSQL, Redis) can land its greeting where the reply belonged. A SOCKS client reading the reply positionally then fails or misparses, and the stream is corrupt from that point on. HTTP largely escapes it because the client speaks first, which is why this hides in the common case. Measured against a real sshd through a dynamic tunnel, 12 rounds per level, counting responses whose first byte was not 0x05: concurrency=1 correct=12 earlyData=0 (before: 4 / 8) concurrency=2 correct=24 earlyData=0 (before: 19 / 5) concurrency=8 correct=96 earlyData=0 (before: 49 / 47) concurrency=16 correct=192 earlyData=0 (before: 119 / 73) Fix by holding the relay shut until the port says otherwise. OnData buffers into a queue while _relaying is false; Bind() calls StartRelay(), which flushes the queue in order and opens the gate. Both forwarded ports already call Bind() immediately after writing their reply, so no API changes. ShutdownSocket and CloseSocket defer while the gate is shut, for the same reason: a target that speaks and hangs up at once (sshd answering "Not allowed at this time.") otherwise shuts the send side down before the reply is written, which surfaced as SocketException: A request to send or receive data was disallowed because the socket had already been shut down in that direction with a previous shutdown call on the port's Exception event - 24 of them in the run above, none after. ForwardedPort_Closing deliberately does not defer: it is the abort path whose job is to interrupt a blocking receive. Data is copied on the way into the queue, as the array belongs to the message being processed. This covers SOCKS4 too, whose reply goes out through the same window. Co-Authored-By: Claude Opus 5 (1M context) --- .../Channels/ChannelDirectTcpip.cs | 263 ++++++++++--- .../ChannelDirectTcpipTest_ReplyOrdering.cs | 357 ++++++++++++++++++ 2 files changed, 574 insertions(+), 46 deletions(-) create mode 100644 test/Renci.SshNet.Tests/Classes/Channels/ChannelDirectTcpipTest_ReplyOrdering.cs diff --git a/src/Renci.SshNet/Channels/ChannelDirectTcpip.cs b/src/Renci.SshNet/Channels/ChannelDirectTcpip.cs index 2e2c7527e..94cac4d98 100644 --- a/src/Renci.SshNet/Channels/ChannelDirectTcpip.cs +++ b/src/Renci.SshNet/Channels/ChannelDirectTcpip.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; using System.Net; using System.Net.Sockets; using System.Threading; @@ -23,6 +24,34 @@ internal sealed class ChannelDirectTcpip : ClientChannel, IChannelDirectTcpip private IForwardedPort _forwardedPort; private Socket _socket; + /// + /// Holds a value indicating whether data received on the channel may be written to + /// . + /// + /// + /// until releases the relay; see + /// for why it is held back. + /// + private bool _relaying; + + /// + /// Holds the data received on the channel before the relay was released, or + /// when there is none. + /// + private Queue _pendingData; + + /// + /// Holds the shutdown that was requested before the relay was released, or + /// when none was. + /// + private SocketShutdown? _pendingShutdown; + + /// + /// Holds a value indicating whether must be closed as soon as the relay + /// is released. + /// + private bool _pendingClose; + /// /// Initializes a new instance of the class. /// @@ -47,6 +76,18 @@ public override ChannelTypes ChannelType get { return ChannelTypes.DirectTcpip; } } + /// + /// Opens the channel for the specified remote host and port. + /// + /// The name of the remote host to forward to. + /// The port of the remote host to forward to. + /// The forwarded port for which the channel is opened. + /// The socket to receive requests from, and send responses from the remote host to. + /// + /// The channel takes ownership of here, but does not yet write to it: + /// the caller may still have its own reply to send first. Data received before + /// is buffered rather than written. See . + /// public void Open(string remoteHost, uint port, IForwardedPort forwardedPort, Socket socket) { if (IsOpen) @@ -80,13 +121,20 @@ public void Open(string remoteHost, uint port, IForwardedPort forwardedPort, Soc /// private void ForwardedPort_Closing(object sender, EventArgs eventArgs) { - // signal to the client that we will not send anything anymore; this should also interrupt the - // blocking receive in Bind if the client sends FIN/ACK in time - ShutdownSocket(SocketShutdown.Send); + // The port is going away, so this is an abort rather than an orderly close: act on the + // socket right away even when the relay has not been released, discarding anything that + // is still buffered. There is nobody left to relay it for, and the whole point of this + // handler is to interrupt a blocking receive. + lock (_socketLock) + { + // signal to the client that we will not send anything anymore; this should also interrupt the + // blocking receive in Bind if the client sends FIN/ACK in time + ShutdownSocketCore(SocketShutdown.Send); - // if the FIN/ACK is not sent in time by the remote client, then interrupt the blocking receive - // by closing the socket - CloseSocket(); + // if the FIN/ACK is not sent in time by the remote client, then interrupt the blocking receive + // by closing the socket + CloseSocketCore(); + } } /// @@ -94,15 +142,25 @@ private void ForwardedPort_Closing(object sender, EventArgs eventArgs) /// public void Bind() { + // Release the data that arrived while the forwarded port was still writing its own reply + // to the client, and only then start pumping in the other direction. + StartRelay(); + // Cannot bind if channel is not open if (!IsOpen) { return; } + var socket = _socket; + if (socket is null) + { + return; + } + var buffer = new byte[RemotePacketSize]; - SocketAbstraction.ReadContinuous(_socket, buffer, 0, buffer.Length, SendData); + SocketAbstraction.ReadContinuous(socket, buffer, 0, buffer.Length, SendData); // even though the client has disconnected, we still want to properly close the // channel @@ -113,55 +171,163 @@ public void Bind() } /// - /// Closes the socket, hereby interrupting the blocking receive in . + /// Starts writing data received on the channel to the socket, first flushing whatever arrived + /// while the relay was held back. /// - private void CloseSocket() + /// + /// + /// A forwarded port writes its own reply to the client between and + /// - for that is the SOCKS reply, which + /// RFC 1928 s6 requires to precede any byte from the target. runs on the + /// session's message-receive thread, so without this gate the two would write to the same + /// socket from two threads with no ordering between them, and a target that speaks first - SSH, + /// SMTP, IMAP, POP3, FTP, MySQL, PostgreSQL, Redis - would land its greeting where the reply + /// belonged. + /// + /// + /// A shutdown or close requested while the relay was closed is applied here too, after the + /// buffered data, so that a target which speaks and then immediately hangs up still gets its + /// bytes to the client in the right order. + /// + /// + private void StartRelay() { - if (_socket is null) + lock (_socketLock) { - return; + if (_relaying) + { + return; + } + + _relaying = true; + + var pending = _pendingData; + _pendingData = null; + + if (pending is not null) + { + while (pending.Count > 0) + { + var data = pending.Dequeue(); + SendToSocket(data, 0, data.Length); + } + } + + if (_pendingShutdown is SocketShutdown pendingShutdown) + { + _pendingShutdown = null; + ShutdownSocketCore(pendingShutdown); + } + + if (_pendingClose) + { + _pendingClose = false; + CloseSocketCore(); + } } + } + + /// + /// Writes data to the socket, if there still is one and it is connected. + /// + /// An array of containing the data to write. + /// The zero-based offset in at which to begin taking data from. + /// The number of bytes of to write. + /// + /// The caller must hold . + /// + private void SendToSocket(byte[] data, int offset, int count) + { + var socket = _socket; + if (socket.IsConnected()) + { + SocketAbstraction.Send(socket, data, offset, count); + } + } + + /// + /// Closes the socket, hereby interrupting the blocking receive in . + /// + private void CloseSocket() + { lock (_socketLock) { - if (_socket is null) + if (!_relaying) { + // Disposing the socket now would strand both the reply the forwarded port is about + // to write and anything OnData has buffered. StartRelay applies this afterwards. + _pendingClose = true; return; } - // closing a socket actually disposes the socket, so we can safely dereference - // the field to avoid entering the lock again later - _socket.Dispose(); - _socket = null; + CloseSocketCore(); } } + /// + /// Disposes the socket. + /// + /// + /// The caller must hold . + /// + private void CloseSocketCore() + { + // closing a socket actually disposes the socket, so we can safely dereference + // the field to avoid entering the lock again later + _socket?.Dispose(); + _socket = null; + + // there is no longer anywhere to write buffered data to + _pendingData = null; + } + /// /// Shuts down the socket. /// /// One of the values that specifies the operation that will no longer be allowed. private void ShutdownSocket(SocketShutdown how) { - if (_socket is null) - { - return; - } - lock (_socketLock) { - if (!_socket.IsConnected()) + if (!_relaying) { + // Shutting the send side down now would break the reply the forwarded port is + // about to write, and would discard whatever OnData has buffered. StartRelay + // applies this after the buffer has been drained. + _pendingShutdown = _pendingShutdown is null || _pendingShutdown == how + ? how + : SocketShutdown.Both; return; } - try - { - _socket.Shutdown(how); - } - catch (SocketException ex) - { - _logger.LogInformation(ex, "Failure shutting down socket"); - } + ShutdownSocketCore(how); + } + } + + /// + /// Shuts down the socket, without regard for the relay. + /// + /// One of the values that specifies the operation that will no longer be allowed. + /// + /// The caller must hold . + /// + private void ShutdownSocketCore(SocketShutdown how) + { + var socket = _socket; + + if (!socket.IsConnected()) + { + return; + } + + try + { + socket.Shutdown(how); + } + catch (SocketException ex) + { + _logger.LogInformation(ex, "Failure shutting down socket"); } } @@ -198,15 +364,28 @@ protected override void OnData(ArraySegment data) { base.OnData(data); - if (_socket != null) + lock (_socketLock) { - lock (_socketLock) + if (_socket is null) { - if (_socket.IsConnected()) - { - SocketAbstraction.Send(_socket, data.Array, data.Offset, data.Count); - } + return; + } + + if (!_relaying) + { + // We are on the session's message-receive thread and the forwarded port has not + // finished writing its own reply to the client yet. Hold on to the data instead of + // racing that thread for the socket; StartRelay writes it out in order. The array + // belongs to the message being processed, so it has to be copied. + var buffered = new byte[data.Count]; + Buffer.BlockCopy(data.Array, data.Offset, buffered, 0, data.Count); + + _pendingData ??= new Queue(); + _pendingData.Enqueue(buffered); + return; } + + SendToSocket(data.Array, data.Offset, data.Count); } } @@ -288,17 +467,9 @@ protected override void Dispose(bool disposing) if (disposing) { - if (_socket != null) + lock (_socketLock) { - lock (_socketLock) - { - var socket = _socket; - if (socket != null) - { - _socket = null; - socket.Dispose(); - } - } + CloseSocketCore(); } var channelOpen = _channelOpen; diff --git a/test/Renci.SshNet.Tests/Classes/Channels/ChannelDirectTcpipTest_ReplyOrdering.cs b/test/Renci.SshNet.Tests/Classes/Channels/ChannelDirectTcpipTest_ReplyOrdering.cs new file mode 100644 index 000000000..c364d010f --- /dev/null +++ b/test/Renci.SshNet.Tests/Classes/Channels/ChannelDirectTcpipTest_ReplyOrdering.cs @@ -0,0 +1,357 @@ +using System; +using System.Net; +using System.Net.Sockets; +using System.Text; +using System.Threading; +using System.Threading.Tasks; + +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.VisualStudio.TestTools.UnitTesting; + +using Moq; + +using Renci.SshNet.Channels; +using Renci.SshNet.Messages; +using Renci.SshNet.Messages.Connection; +using Renci.SshNet.Tests.Common; + +namespace Renci.SshNet.Tests.Classes.Channels +{ + /// + /// A forwarded port owns the client socket between and + /// so that it can write its own reply first — + /// writes the SOCKS reply there, and RFC 1928 §6 requires that + /// reply to precede any byte from the target. + /// + /// runs on the session's message-receive thread, so if it + /// wrote to the socket during that window the two threads would race and a target that speaks + /// first — SSH, SMTP, IMAP, POP3, FTP, MySQL, PostgreSQL, Redis — could land its greeting where + /// the reply belonged. These tests pin the ordering, and the disposal of the data and of the + /// socket itself when the target speaks and then immediately hangs up. + /// + [TestClass] + public class ChannelDirectTcpipTest_ReplyOrdering : TestBase + { + private const string Reply = ""; + private const string TargetGreeting = "SSH-2.0-OpenSSH_10.2p1\r\n"; + + private Mock _sessionMock; + private Mock _forwardedPortMock; + private Mock _connectionInfoMock; + private uint _localChannelNumber; + private uint _remoteChannelNumber; + private uint _remoteWindowSize; + private uint _remotePacketSize; + + /// + /// The socket the channel owns and writes to, i.e. the one the forwarded port accepted. + /// + private Socket _serverSide; + + /// + /// The socket the SOCKS client would be holding. + /// + private Socket _clientSide; + + private Socket _listener; + + protected override void OnInit() + { + base.OnInit(); + + var random = new Random(); + _localChannelNumber = (uint)random.Next(0, int.MaxValue); + _remoteChannelNumber = (uint)random.Next(0, int.MaxValue); + _remoteWindowSize = 0x100000; + _remotePacketSize = 0x8000; + + _sessionMock = new Mock(MockBehavior.Strict); + _forwardedPortMock = new Mock(MockBehavior.Strict); + _connectionInfoMock = new Mock(MockBehavior.Strict); + + _ = _sessionMock.Setup(p => p.SessionLoggerFactory) + .Returns(NullLoggerFactory.Instance); + _ = _sessionMock.Setup(p => p.IsConnected) + .Returns(true); + _ = _sessionMock.Setup(p => p.ConnectionInfo) + .Returns(_connectionInfoMock.Object); + _ = _connectionInfoMock.Setup(p => p.ChannelCloseTimeout) + .Returns(TimeSpan.FromSeconds(10)); + _ = _sessionMock.Setup(p => p.WaitOnHandle(It.IsAny())) + .Callback(handle => handle.WaitOne()); + _ = _sessionMock.Setup(p => p.TryWait(It.IsAny(), It.IsAny())) + .Returns(WaitResult.Success); + _ = _sessionMock.Setup(p => p.TrySendMessage(It.IsAny())) + .Returns(true); + + // Confirm the channel open as soon as it is requested, the way a server would. Any other + // message the channel sends (a window adjust, say) is simply accepted. + _ = _sessionMock.Setup(p => p.SendMessage(It.IsAny())) + .Callback(message => + { + if (message is ChannelOpenMessage open) + { + _sessionMock.Raise(p => p.ChannelOpenConfirmationReceived += null, + new MessageEventArgs( + new ChannelOpenConfirmationMessage(open.LocalChannelNumber, + _remoteWindowSize, + _remotePacketSize, + _remoteChannelNumber))); + } + }); + + _listener = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp); + _listener.Bind(new IPEndPoint(IPAddress.Loopback, 0)); + _listener.Listen(1); + + _clientSide = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp) { NoDelay = true }; + _clientSide.Connect(_listener.LocalEndPoint); + _serverSide = _listener.Accept(); + _serverSide.NoDelay = true; + } + + protected override void OnCleanup() + { + _clientSide?.Dispose(); + _serverSide?.Dispose(); + _listener?.Dispose(); + + base.OnCleanup(); + } + + /// + /// The defect itself: data that arrives while the port is still composing its reply must not + /// be written to the client ahead of that reply. + /// + [TestMethod] + public void TargetDataArrivingBeforeBindIsWrittenAfterThePortsOwnReply() + { + using (var channel = CreateOpenChannel()) + { + RaiseData(TargetGreeting); + + // Nothing may reach the client yet: the port has not written its reply. + Assert.AreEqual(0, BytesWaitingForClient(), "the target's greeting was written to the client before the port's reply"); + + // The forwarded port writes its reply, then hands the socket over. + _ = _serverSide.Send(Encoding.ASCII.GetBytes(Reply)); + + var bind = RunBind(channel); + + Assert.AreEqual(Reply + TargetGreeting, ReadFromClient(Reply.Length + TargetGreeting.Length)); + + FinishBind(bind); + } + } + + /// + /// Everything buffered has to come out, once, in the order it arrived. + /// + [TestMethod] + public void EveryChunkBufferedBeforeBindIsDeliveredOnceAndInOrder() + { + using (var channel = CreateOpenChannel()) + { + var expected = new StringBuilder(Reply); + + for (var i = 0; i < 25; i++) + { + var chunk = "chunk-" + i.ToString("D2") + ";"; + RaiseData(chunk); + _ = expected.Append(chunk); + } + + Assert.AreEqual(0, BytesWaitingForClient()); + + _ = _serverSide.Send(Encoding.ASCII.GetBytes(Reply)); + + var bind = RunBind(channel); + + Assert.AreEqual(expected.ToString(), ReadFromClient(expected.Length)); + + FinishBind(bind); + } + } + + /// + /// A target that speaks and hangs up at once — sshd refusing a connection, say — must still + /// get its bytes to the client, after the reply, before the socket is shut down. + /// + [TestMethod] + public void EofArrivingBeforeBindDoesNotDiscardBufferedDataOrPreEmptTheReply() + { + using (var channel = CreateOpenChannel()) + { + RaiseData(TargetGreeting); + _sessionMock.Raise(p => p.ChannelEofReceived += null, + new MessageEventArgs(new ChannelEofMessage(_localChannelNumber))); + + // The reply must still be writable: the send side may not have been shut down yet. + _ = _serverSide.Send(Encoding.ASCII.GetBytes(Reply)); + + var bind = RunBind(channel); + + Assert.AreEqual(Reply + TargetGreeting, ReadFromClient(Reply.Length + TargetGreeting.Length)); + + // and only then the end of the stream + Assert.AreEqual(0, _clientSide.Receive(new byte[16], 0, 16, SocketFlags.None)); + + FinishBind(bind); + } + } + + /// + /// Same again, but with the whole channel closed before the port got its reply out. The socket + /// may not be disposed from under the reply. + /// + [TestMethod] + public void ChannelClosedBeforeBindDoesNotDiscardBufferedDataOrPreEmptTheReply() + { + using (var channel = CreateOpenChannel()) + { + RaiseData(TargetGreeting); + _sessionMock.Raise(p => p.ChannelEofReceived += null, + new MessageEventArgs(new ChannelEofMessage(_localChannelNumber))); + _sessionMock.Raise(p => p.ChannelCloseReceived += null, + new MessageEventArgs(new ChannelCloseMessage(_localChannelNumber))); + + Assert.IsFalse(channel.IsOpen, "the channel should have been closed"); + + _ = _serverSide.Send(Encoding.ASCII.GetBytes(Reply)); + + channel.Bind(); + + Assert.AreEqual(Reply + TargetGreeting, ReadFromClient(Reply.Length + TargetGreeting.Length)); + Assert.AreEqual(0, _clientSide.Receive(new byte[16], 0, 16, SocketFlags.None)); + } + } + + /// + /// Once the relay is running, data goes straight to the socket — no buffering, no delay until + /// some later event. + /// + [TestMethod] + public void DataArrivingAfterBindIsWrittenStraightToTheClient() + { + using (var channel = CreateOpenChannel()) + { + _ = _serverSide.Send(Encoding.ASCII.GetBytes(Reply)); + + var bind = RunBind(channel); + + Assert.AreEqual(Reply, ReadFromClient(Reply.Length)); + + RaiseData(TargetGreeting); + + Assert.AreEqual(TargetGreeting, ReadFromClient(TargetGreeting.Length)); + + FinishBind(bind); + } + } + + /// + /// The port going away is an abort, not an orderly close: it must still take the socket down + /// immediately, which is what unblocks a forwarded port that is stopping. + /// + [TestMethod] + public void ForwardedPortClosingBeforeBindStillTakesTheSocketDown() + { + using (var channel = CreateOpenChannel()) + { + RaiseData(TargetGreeting); + + _forwardedPortMock.Raise(p => p.Closing += null, EventArgs.Empty); + + // FIN, not the buffered greeting + Assert.AreEqual(0, _clientSide.Receive(new byte[64], 0, 64, SocketFlags.None)); + + // and Bind must not block on a socket that is gone + channel.Bind(); + } + } + + private ChannelDirectTcpip CreateOpenChannel() + { + var channel = new ChannelDirectTcpip(_sessionMock.Object, + _localChannelNumber, + localWindowSize: 0x100000, + localPacketSize: 0x8000); + channel.Open("target.example.com", 22, _forwardedPortMock.Object, _serverSide); + Assert.IsTrue(channel.IsOpen); + return channel; + } + + private void RaiseData(string text) + { + // The array belongs to the message and is reused by the session, so hand over a distinct + // one each time — a channel that kept a reference rather than a copy must not pass. + var payload = Encoding.ASCII.GetBytes(text); + _sessionMock.Raise(p => p.ChannelDataReceived += null, + new MessageEventArgs(new ChannelDataMessage(_localChannelNumber, payload))); + Array.Clear(payload, 0, payload.Length); + } + + /// + /// Bind blocks receiving from the client, so it runs on its own thread. + /// + private static Task RunBind(ChannelDirectTcpip channel) + { + var started = new ManualResetEventSlim(false); + var task = Task.Factory.StartNew(() => + { + started.Set(); + channel.Bind(); + }, TaskCreationOptions.LongRunning); + _ = started.Wait(TimeSpan.FromSeconds(5)); + return task; + } + + private void FinishBind(Task bind) + { + _clientSide.Shutdown(SocketShutdown.Send); + Assert.IsTrue(bind.Wait(TimeSpan.FromSeconds(10)), "Bind did not return after the client shut down its send side"); + } + + /// + /// Returns how many bytes are readable by the client, after giving a wrongly-ordered write + /// long enough to show up. Loopback delivery is immediate, so this does not race the fault it + /// is looking for; it only gives it time. + /// + private int BytesWaitingForClient() + { + var deadline = DateTime.UtcNow.AddMilliseconds(250); + while (DateTime.UtcNow < deadline) + { + if (_clientSide.Available > 0) + { + return _clientSide.Available; + } + + Thread.Sleep(10); + } + + return _clientSide.Available; + } + + private string ReadFromClient(int count) + { + var buffer = new byte[count]; + var read = 0; + + _clientSide.ReceiveTimeout = 10000; + + while (read < count) + { + var bytes = _clientSide.Receive(buffer, read, count - read, SocketFlags.None); + if (bytes == 0) + { + break; + } + + read += bytes; + } + + return Encoding.ASCII.GetString(buffer, 0, read); + } + } +} From 072c9035f520b38e797c32ee23e0fd34a819e0ce Mon Sep 17 00:00:00 2001 From: tomresno312 <251623035+tomresno312@users.noreply.github.com> Date: Mon, 27 Jul 2026 07:32:49 +0200 Subject: [PATCH 2/2] Reply to a rejected SOCKS4 request with the full eight bytes A SOCKS4 reply is a fixed-width record - version, code, destination port, destination address - and is eight bytes whether or not the request was granted. HandleSocks4 sent only two on the rejection path: SocketAbstraction.SendByte(socket, 0x00); if (channel.IsOpen) { ... eight bytes ... } SocketAbstraction.SendByte(socket, 0x5b); The last six fields are defined as ignored by the client, but reading them is not optional. A client that reads the record as a record blocks waiting for bytes that never arrive, instead of seeing the refusal; this library's own Socks4Connector reads them on the granted path. Against a real server, a CONNECT to a port with nothing listening on it left the client unable to read a reply at all. It now gets 00-5B-00-01-7F-00-00-01 Both paths now compose the reply through CreateSocks4Reply, mirroring CreateSocks5Reply, and send it once rather than in four writes. Co-Authored-By: Claude Opus 5 (1M context) --- src/Renci.SshNet/ForwardedPortDynamic.cs | 62 ++++-- ...rdedPortDynamicTest_Started_Socks4Reply.cs | 183 ++++++++++++++++++ 2 files changed, 234 insertions(+), 11 deletions(-) create mode 100644 test/Renci.SshNet.Tests/Classes/ForwardedPortDynamicTest_Started_Socks4Reply.cs diff --git a/src/Renci.SshNet/ForwardedPortDynamic.cs b/src/Renci.SshNet/ForwardedPortDynamic.cs index 7210b403a..b4db9cb30 100644 --- a/src/Renci.SshNet/ForwardedPortDynamic.cs +++ b/src/Renci.SshNet/ForwardedPortDynamic.cs @@ -504,19 +504,13 @@ private bool HandleSocks4(Socket socket, IChannelDirectTcpip channel, TimeSpan t channel.Open(host, port, this, socket); - SocketAbstraction.SendByte(socket, 0x00); + var channelOpen = channel.IsOpen; - if (channel.IsOpen) - { - SocketAbstraction.SendByte(socket, 0x5a); - SocketAbstraction.Send(socket, portBuffer, 0, portBuffer.Length); - SocketAbstraction.Send(socket, ipBuffer, 0, ipBuffer.Length); - return true; - } + var socksReply = CreateSocks4Reply(channelOpen, portBuffer, ipBuffer); + + SocketAbstraction.Send(socket, socksReply, 0, socksReply.Length); - // signal that request was rejected or failed - SocketAbstraction.SendByte(socket, 0x5b); - return false; + return channelOpen; } private bool HandleSocks5(Socket socket, IChannelDirectTcpip channel, TimeSpan timeout) @@ -671,6 +665,52 @@ private static string GetSocks5Host(int addressType, Socket socket, TimeSpan tim } } + /// + /// Creates the reply to a SOCKS4 request. + /// + /// if the channel was opened; otherwise, . + /// The destination port from the request. + /// The destination address from the request. + /// + /// An array of containing the reply. + /// + /// + /// The reply is eight bytes whether or not the request was granted. The destination port and + /// address are defined to be ignored by the client, but they are not optional: a client that + /// reads the reply as the fixed-width record it is - which includes the SOCKS4 client in this + /// library - would block waiting for the remaining six bytes of a rejection. + /// + private static byte[] CreateSocks4Reply(bool channelOpen, byte[] portBuffer, byte[] ipBuffer) + { + var socksReply = new byte[// Reply version; fixed: 0x00 + 1 + + + // Reply code + 1 + + + // Destination port; ignored by the client + 2 + + + // Destination IPv4 address; ignored by the client + 4]; + + socksReply[0] = 0x00; + + if (channelOpen) + { + socksReply[1] = 0x5a; // request granted + } + else + { + socksReply[1] = 0x5b; // request rejected or failed + } + + Buffer.BlockCopy(portBuffer, 0, socksReply, 2, portBuffer.Length); + Buffer.BlockCopy(ipBuffer, 0, socksReply, 4, ipBuffer.Length); + + return socksReply; + } + private static byte[] CreateSocks5Reply(bool channelOpen) { var socksReply = new byte[// SOCKS version diff --git a/test/Renci.SshNet.Tests/Classes/ForwardedPortDynamicTest_Started_Socks4Reply.cs b/test/Renci.SshNet.Tests/Classes/ForwardedPortDynamicTest_Started_Socks4Reply.cs new file mode 100644 index 000000000..17d46b624 --- /dev/null +++ b/test/Renci.SshNet.Tests/Classes/ForwardedPortDynamicTest_Started_Socks4Reply.cs @@ -0,0 +1,183 @@ +using System; +using System.Collections.Generic; +using System.Globalization; +using System.Net; +using System.Net.Sockets; +using System.Text; + +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.VisualStudio.TestTools.UnitTesting; + +using Moq; + +using Renci.SshNet.Channels; +using Renci.SshNet.Common; +using Renci.SshNet.Tests.Common; + +namespace Renci.SshNet.Tests.Classes +{ + /// + /// A SOCKS4 reply is a fixed-width eight byte record - version, code, destination port, + /// destination address - whether or not the request was granted. The last six fields are defined + /// as ignored by the client, but reading them is not optional: a client that reads the record as + /// a record, which SSH.NET's own does on the + /// granted path, blocks waiting for bytes that never come if a rejection is short. + /// + [TestClass] + public class ForwardedPortDynamicTest_Started_Socks4Reply + { + private const string UserName = "socks-user"; + + private Mock _sessionMock; + private Mock _channelMock; + private Mock _connectionInfoMock; + private ForwardedPortDynamic _forwardedPort; + private Socket _client; + private IPEndPoint _remoteEndpoint; + private IList _exceptionRegister; + + [TestCleanup] + public void Cleanup() + { + if (_forwardedPort is not null && _forwardedPort.IsStarted) + { + _forwardedPort.Stop(); + } + + _client?.Dispose(); + _client = null; + _forwardedPort?.Dispose(); + _forwardedPort = null; + } + + /// + /// The case that was short: the channel could not be opened, so the request is rejected. + /// + [TestMethod] + public void RejectedRequestShouldBeAnsweredWithAFullEightByteReply() + { + Arrange(channelOpen: false); + + var reply = RequestConnect(); + + Assert.AreEqual(8, reply.Length, "a SOCKS4 reply is eight bytes, granted or not"); + Assert.AreEqual(0x00, reply[0], "reply version"); + Assert.AreEqual(0x5b, reply[1], "request rejected or failed"); + AssertEchoesTheRequest(reply); + } + + /// + /// And the granted case still is what it was. + /// + [TestMethod] + public void GrantedRequestShouldBeAnsweredWithAFullEightByteReply() + { + Arrange(channelOpen: true); + + var reply = RequestConnect(); + + Assert.AreEqual(8, reply.Length); + Assert.AreEqual(0x00, reply[0], "reply version"); + Assert.AreEqual(0x5a, reply[1], "request granted"); + AssertEchoesTheRequest(reply); + } + + private void AssertEchoesTheRequest(byte[] reply) + { + var address = _remoteEndpoint.Address.GetAddressBytes(); + + Assert.AreEqual((byte)(_remoteEndpoint.Port >> 8), reply[2], "destination port, high byte"); + Assert.AreEqual((byte)(_remoteEndpoint.Port & 0xFF), reply[3], "destination port, low byte"); + CollectionAssert.AreEqual(address, new[] { reply[4], reply[5], reply[6], reply[7] }, "destination address"); + } + + private void Arrange(bool channelOpen) + { + _exceptionRegister = new List(); + _remoteEndpoint = new IPEndPoint(IPAddress.Parse("193.168.1.5"), 8122); + + _sessionMock = new Mock(MockBehavior.Strict); + _channelMock = new Mock(MockBehavior.Strict); + _connectionInfoMock = new Mock(MockBehavior.Strict); + + _ = _sessionMock.Setup(p => p.SessionLoggerFactory).Returns(NullLoggerFactory.Instance); + _ = _sessionMock.Setup(p => p.IsConnected).Returns(true); + _ = _sessionMock.Setup(p => p.ConnectionInfo).Returns(_connectionInfoMock.Object); + _ = _sessionMock.Setup(p => p.CreateChannelDirectTcpip()).Returns(_channelMock.Object); + _ = _connectionInfoMock.Setup(p => p.Timeout).Returns(TimeSpan.FromSeconds(5)); + _ = _channelMock.Setup(p => p.Open(_remoteEndpoint.Address.ToString(), + (uint)_remoteEndpoint.Port, + It.IsAny(), + It.IsAny())); + _ = _channelMock.Setup(p => p.IsOpen).Returns(channelOpen); + _ = _channelMock.Setup(p => p.Bind()); + _ = _channelMock.Setup(p => p.Dispose()); + + // Bind to an ephemeral port rather than a fixed one, so that the test does not collide + // with anything else that happens to be listening. + _forwardedPort = new ForwardedPortDynamic("127.0.0.1", 0); + _forwardedPort.Exception += (sender, args) => _exceptionRegister.Add(args); + _forwardedPort.Session = _sessionMock.Object; + _forwardedPort.Start(); + + _client = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp) + { + ReceiveTimeout = 10000 + }; + _client.Connect(new IPEndPoint(IPAddress.Loopback, (int)_forwardedPort.BoundPort)); + } + + /// + /// Sends a SOCKS4 CONNECT and reads back exactly what the protocol says the reply is, without + /// giving up early. A reply that is short shows up here as a receive timeout, which is what a + /// real client would see too. + /// + private byte[] RequestConnect() + { + var user = Encoding.ASCII.GetBytes(UserName); + var address = _remoteEndpoint.Address.GetAddressBytes(); + var request = new byte[8 + user.Length + 1]; + + request[0] = 0x04; // SOCKS version + request[1] = 0x01; // CONNECT + request[2] = (byte)(_remoteEndpoint.Port >> 8); + request[3] = (byte)(_remoteEndpoint.Port & 0xFF); + Buffer.BlockCopy(address, 0, request, 4, address.Length); + Buffer.BlockCopy(user, 0, request, 8, user.Length); + request[request.Length - 1] = 0x00; + + _ = _client.Send(request, 0, request.Length, SocketFlags.None); + + var reply = new byte[8]; + var received = 0; + + try + { + while (received < reply.Length) + { + var bytes = _client.Receive(reply, received, reply.Length - received, SocketFlags.None); + if (bytes == 0) + { + break; + } + + received += bytes; + } + } + catch (SocketException ex) when (ex.SocketErrorCode == SocketError.TimedOut) + { + Assert.Fail(string.Format(CultureInfo.InvariantCulture, + "timed out after {0} of {1} reply bytes: {2}", + received, + reply.Length, + BitConverter.ToString(reply, 0, received))); + } + + Assert.AreEqual(0, _exceptionRegister.Count, _exceptionRegister.AsString()); + + var actual = new byte[received]; + Buffer.BlockCopy(reply, 0, actual, 0, received); + return actual; + } + } +}