From 1ab13008ebbcbb64fce10f04fbaeeb88b6d0cba7 Mon Sep 17 00:00:00 2001 From: Marius Thesing Date: Sun, 24 Mar 2024 11:19:39 +0100 Subject: [PATCH 1/4] Handle unknown channel messages correctly See discussion #1218 . Some servers send custom channel messages like 'keepalive@proftpd.org' as keep alive messages. This currently causes a NotSupportedException. According to the spec https://datatracker.ietf.org/doc/html/rfc4254#section-5.4 : "If the request is not recognized or is not supported for the channel, SSH_MSG_CHANNEL_FAILURE is returned." Send a failure message back instead of throwing an exception. --- src/Renci.SshNet/Channels/Channel.cs | 5 +- ...nelRequestReceived_HandleUnknownMessage.cs | 81 +++++++++++++++++++ 2 files changed, 83 insertions(+), 3 deletions(-) create mode 100644 test/Renci.SshNet.Tests/Classes/Channels/ChannelTest_OnSessionChannelRequestReceived_HandleUnknownMessage.cs diff --git a/src/Renci.SshNet/Channels/Channel.cs b/src/Renci.SshNet/Channels/Channel.cs index 25975872d..312cc801b 100644 --- a/src/Renci.SshNet/Channels/Channel.cs +++ b/src/Renci.SshNet/Channels/Channel.cs @@ -1,5 +1,4 @@ using System; -using System.Globalization; using System.Net.Sockets; using System.Threading; @@ -715,8 +714,8 @@ private void OnChannelRequest(object sender, MessageEventArgs _channelExceptionRegister; + private UnknownRequestInfo _requestInfo; + + protected override void SetupData() + { + var random = new Random(); + + _localWindowSize = (uint) random.Next(1000, int.MaxValue); + _localPacketSize = _localWindowSize - 1; + _localChannelNumber = (uint) random.Next(0, int.MaxValue); + _channelExceptionRegister = new List(); + _requestInfo = new UnknownRequestInfo(); + } + + protected override void SetupMocks() + { + _ = SessionMock.Setup(p => p.ConnectionInfo) + .Returns(new ConnectionInfo("host", "user", new PasswordAuthenticationMethod("user", "password"))); + _ = SessionMock.Setup(p => p.SendMessage(It.IsAny())); + } + + protected override void Arrange() + { + base.Arrange(); + + _channel = new ChannelStub(SessionMock.Object, _localChannelNumber, _localWindowSize, _localPacketSize); + _channel.SetIsOpen(true); + _channel.Exception += (sender, args) => _channelExceptionRegister.Add(args); + } + + protected override void Act() + { + SessionMock.Raise(s => s.ChannelRequestReceived += null, + new MessageEventArgs(new ChannelRequestMessage(_localChannelNumber, _requestInfo))); + } + + [TestMethod] + public void FailureMessageWasSent() + { + SessionMock.Verify(p => p.SendMessage(It.Is(m => m.LocalChannelNumber == _localChannelNumber)), Times.Once); + } + + [TestMethod] + public void NoExceptionShouldHaveFired() + { + Assert.AreEqual(0, _channelExceptionRegister.Count); + } + } + + internal class UnknownRequestInfo : RequestInfo + { + public override string RequestName + { + get + { + return nameof(UnknownRequestInfo); + } + } + + } +} From c2ea1d2341591d85cafec40a2b96d28d15a2fe4d Mon Sep 17 00:00:00 2001 From: Marius Thesing Date: Mon, 1 Apr 2024 09:39:07 +0200 Subject: [PATCH 2/4] consider WantReply before sending failure reply --- src/Renci.SshNet/Channels/Channel.cs | 10 +++++++-- .../ChannelRequest/UnknownRequestInfo.cs | 22 +++++++++++++++++++ ...nelRequestReceived_HandleUnknownMessage.cs | 12 ++++++---- 3 files changed, 38 insertions(+), 6 deletions(-) create mode 100644 src/Renci.SshNet/Messages/Connection/ChannelRequest/UnknownRequestInfo.cs diff --git a/src/Renci.SshNet/Channels/Channel.cs b/src/Renci.SshNet/Channels/Channel.cs index 312cc801b..870573f1a 100644 --- a/src/Renci.SshNet/Channels/Channel.cs +++ b/src/Renci.SshNet/Channels/Channel.cs @@ -714,8 +714,14 @@ private void OnChannelRequest(object sender, MessageEventArgs + /// Represents an unknown request information that we can't handle. + /// + internal sealed class UnknownRequestInfo : RequestInfo + { + /// + /// Gets the name of the request. + /// + public override string RequestName { get; } + + /// + /// Initializes a new instance of the class. + /// The name of the unknown request. + /// + internal UnknownRequestInfo(string requestName) + { + RequestName = requestName; + } + } +} diff --git a/test/Renci.SshNet.Tests/Classes/Channels/ChannelTest_OnSessionChannelRequestReceived_HandleUnknownMessage.cs b/test/Renci.SshNet.Tests/Classes/Channels/ChannelTest_OnSessionChannelRequestReceived_HandleUnknownMessage.cs index 88641c0dd..807a62f9f 100644 --- a/test/Renci.SshNet.Tests/Classes/Channels/ChannelTest_OnSessionChannelRequestReceived_HandleUnknownMessage.cs +++ b/test/Renci.SshNet.Tests/Classes/Channels/ChannelTest_OnSessionChannelRequestReceived_HandleUnknownMessage.cs @@ -19,7 +19,7 @@ public class ChannelTest_OnSessionChannelRequestReceived_HandleUnknownMessage : private uint _localChannelNumber; private ChannelStub _channel; private IList _channelExceptionRegister; - private UnknownRequestInfo _requestInfo; + private UnknownRequestInfoWithWantReply _requestInfo; protected override void SetupData() { @@ -29,7 +29,7 @@ protected override void SetupData() _localPacketSize = _localWindowSize - 1; _localChannelNumber = (uint) random.Next(0, int.MaxValue); _channelExceptionRegister = new List(); - _requestInfo = new UnknownRequestInfo(); + _requestInfo = new UnknownRequestInfoWithWantReply(); } protected override void SetupMocks() @@ -67,15 +67,19 @@ public void NoExceptionShouldHaveFired() } } - internal class UnknownRequestInfo : RequestInfo + internal class UnknownRequestInfoWithWantReply : RequestInfo { public override string RequestName { get { - return nameof(UnknownRequestInfo); + return nameof(UnknownRequestInfoWithWantReply); } } + internal UnknownRequestInfoWithWantReply() + { + WantReply = true; + } } } From 88583b4077696f348af53991ba2738b2fe082d72 Mon Sep 17 00:00:00 2001 From: Marius Thesing Date: Mon, 1 Apr 2024 14:59:45 +0200 Subject: [PATCH 3/4] Use RemoteChannelNumber for failure message --- src/Renci.SshNet/Channels/Channel.cs | 2 +- ...SessionChannelRequestReceived_HandleUnknownMessage.cs | 9 ++++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/Renci.SshNet/Channels/Channel.cs b/src/Renci.SshNet/Channels/Channel.cs index 870573f1a..8ab9736d8 100644 --- a/src/Renci.SshNet/Channels/Channel.cs +++ b/src/Renci.SshNet/Channels/Channel.cs @@ -719,7 +719,7 @@ private void OnChannelRequest(object sender, MessageEventArgs _channelExceptionRegister; private UnknownRequestInfoWithWantReply _requestInfo; @@ -28,6 +31,9 @@ protected override void SetupData() _localWindowSize = (uint) random.Next(1000, int.MaxValue); _localPacketSize = _localWindowSize - 1; _localChannelNumber = (uint) random.Next(0, int.MaxValue); + _remoteChannelNumber = (uint) random.Next(0, int.MaxValue); + _remoteWindowSize = (uint) random.Next(0, int.MaxValue); + _remotePacketSize = (uint) random.Next(0, int.MaxValue); _channelExceptionRegister = new List(); _requestInfo = new UnknownRequestInfoWithWantReply(); } @@ -44,6 +50,7 @@ protected override void Arrange() base.Arrange(); _channel = new ChannelStub(SessionMock.Object, _localChannelNumber, _localWindowSize, _localPacketSize); + _channel.InitializeRemoteChannelInfo(_remoteChannelNumber, _remoteWindowSize, _remotePacketSize); _channel.SetIsOpen(true); _channel.Exception += (sender, args) => _channelExceptionRegister.Add(args); } @@ -57,7 +64,7 @@ protected override void Act() [TestMethod] public void FailureMessageWasSent() { - SessionMock.Verify(p => p.SendMessage(It.Is(m => m.LocalChannelNumber == _localChannelNumber)), Times.Once); + SessionMock.Verify(p => p.SendMessage(It.Is(m => m.LocalChannelNumber == _channel.RemoteChannelNumber)), Times.Once); } [TestMethod] From e9f7ed594270e4c5a771d6e1fac55de0cf6235fe Mon Sep 17 00:00:00 2001 From: Marius Thesing Date: Mon, 1 Apr 2024 15:55:18 +0200 Subject: [PATCH 4/4] fix wrong ChannelNumber in SshCommand Channel Response not directly related to the PR, was noticed during Code Review. --- src/Renci.SshNet/Channels/IChannel.cs | 5 +++++ src/Renci.SshNet/SshCommand.cs | 4 ++-- .../Renci.SshNet.Tests/Classes/ShellStreamTest_ReadExpect.cs | 2 ++ 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/Renci.SshNet/Channels/IChannel.cs b/src/Renci.SshNet/Channels/IChannel.cs index 77a757307..7ec3eb8e2 100644 --- a/src/Renci.SshNet/Channels/IChannel.cs +++ b/src/Renci.SshNet/Channels/IChannel.cs @@ -59,6 +59,11 @@ internal interface IChannel : IDisposable /// uint LocalPacketSize { get; } + /// + /// Gets the remote channel number. + /// + uint RemoteChannelNumber { get; } + /// /// Gets the maximum size of a data packet that can be sent using the channel. /// diff --git a/src/Renci.SshNet/SshCommand.cs b/src/Renci.SshNet/SshCommand.cs index 58b1942cc..7395ec257 100644 --- a/src/Renci.SshNet/SshCommand.cs +++ b/src/Renci.SshNet/SshCommand.cs @@ -454,7 +454,7 @@ private void Channel_RequestReceived(object sender, ChannelRequestEventArgs e) if (exitStatusInfo.WantReply) { - var replyMessage = new ChannelSuccessMessage(_channel.LocalChannelNumber); + var replyMessage = new ChannelSuccessMessage(_channel.RemoteChannelNumber); _session.SendMessage(replyMessage); } } @@ -462,7 +462,7 @@ private void Channel_RequestReceived(object sender, ChannelRequestEventArgs e) { if (e.Info.WantReply) { - var replyMessage = new ChannelFailureMessage(_channel.LocalChannelNumber); + var replyMessage = new ChannelFailureMessage(_channel.RemoteChannelNumber); _session.SendMessage(replyMessage); } } diff --git a/test/Renci.SshNet.Tests/Classes/ShellStreamTest_ReadExpect.cs b/test/Renci.SshNet.Tests/Classes/ShellStreamTest_ReadExpect.cs index 3752dfb52..a226f47b6 100644 --- a/test/Renci.SshNet.Tests/Classes/ShellStreamTest_ReadExpect.cs +++ b/test/Renci.SshNet.Tests/Classes/ShellStreamTest_ReadExpect.cs @@ -368,6 +368,8 @@ public void Open() public uint LocalPacketSize => throw new NotImplementedException(); + public uint RemoteChannelNumber => throw new NotImplementedException(); + public uint RemotePacketSize => throw new NotImplementedException(); public bool IsOpen => throw new NotImplementedException();