From 70a0a08dae410df014fe33267087821026ccc27e Mon Sep 17 00:00:00 2001 From: mus65 Date: Thu, 4 Apr 2024 16:31:04 +0200 Subject: [PATCH] Handle unknown channel messages correctly (#1363) * 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. * consider WantReply before sending failure reply * Use RemoteChannelNumber for failure message * fix wrong ChannelNumber in SshCommand Channel Response not directly related to the PR, was noticed during Code Review. --------- Co-authored-by: Rob Hague --- src/Renci.SshNet/Channels/Channel.cs | 11 ++- src/Renci.SshNet/Channels/IChannel.cs | 5 + .../ChannelRequest/UnknownRequestInfo.cs | 22 +++++ src/Renci.SshNet/SshCommand.cs | 4 +- ...nelRequestReceived_HandleUnknownMessage.cs | 92 +++++++++++++++++++ .../Classes/ShellStreamTest_ReadExpect.cs | 2 + 6 files changed, 131 insertions(+), 5 deletions(-) create mode 100644 src/Renci.SshNet/Messages/Connection/ChannelRequest/UnknownRequestInfo.cs 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 25975872..8ab9736d 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,14 @@ namespace Renci.SshNet.Channels } else { - // TODO: we should also send a SSH_MSG_CHANNEL_FAILURE message - throw new NotSupportedException(string.Format(CultureInfo.CurrentCulture, "Request '{0}' is not supported.", e.Message.RequestName)); + var unknownRequestInfo = new UnknownRequestInfo(e.Message.RequestName); + unknownRequestInfo.Load(e.Message.RequestData); + + if (unknownRequestInfo.WantReply) + { + var reply = new ChannelFailureMessage(RemoteChannelNumber); + SendMessage(reply); + } } } catch (Exception ex) diff --git a/src/Renci.SshNet/Channels/IChannel.cs b/src/Renci.SshNet/Channels/IChannel.cs index 77a75730..7ec3eb8e 100644 --- a/src/Renci.SshNet/Channels/IChannel.cs +++ b/src/Renci.SshNet/Channels/IChannel.cs @@ -59,6 +59,11 @@ namespace Renci.SshNet.Channels /// 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/Messages/Connection/ChannelRequest/UnknownRequestInfo.cs b/src/Renci.SshNet/Messages/Connection/ChannelRequest/UnknownRequestInfo.cs new file mode 100644 index 00000000..6cd19b5a --- /dev/null +++ b/src/Renci.SshNet/Messages/Connection/ChannelRequest/UnknownRequestInfo.cs @@ -0,0 +1,22 @@ +namespace Renci.SshNet.Messages.Connection +{ + /// + /// 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/src/Renci.SshNet/SshCommand.cs b/src/Renci.SshNet/SshCommand.cs index 647b041b..df61cda7 100644 --- a/src/Renci.SshNet/SshCommand.cs +++ b/src/Renci.SshNet/SshCommand.cs @@ -464,7 +464,7 @@ namespace Renci.SshNet if (exitStatusInfo.WantReply) { - var replyMessage = new ChannelSuccessMessage(_channel.LocalChannelNumber); + var replyMessage = new ChannelSuccessMessage(_channel.RemoteChannelNumber); _session.SendMessage(replyMessage); } } @@ -472,7 +472,7 @@ namespace Renci.SshNet { 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/Channels/ChannelTest_OnSessionChannelRequestReceived_HandleUnknownMessage.cs b/test/Renci.SshNet.Tests/Classes/Channels/ChannelTest_OnSessionChannelRequestReceived_HandleUnknownMessage.cs new file mode 100644 index 00000000..426b2463 --- /dev/null +++ b/test/Renci.SshNet.Tests/Classes/Channels/ChannelTest_OnSessionChannelRequestReceived_HandleUnknownMessage.cs @@ -0,0 +1,92 @@ +using System; +using System.Collections.Generic; + +using Microsoft.VisualStudio.TestTools.UnitTesting; + +using Moq; + +using Renci.SshNet.Common; +using Renci.SshNet.Messages; +using Renci.SshNet.Messages.Connection; + +namespace Renci.SshNet.Tests.Classes.Channels +{ + [TestClass] + public class ChannelTest_OnSessionChannelRequestReceived_HandleUnknownMessage : ChannelTestBase + { + private uint _localWindowSize; + private uint _localPacketSize; + private uint _localChannelNumber; + private uint _remoteChannelNumber; + private uint _remoteWindowSize; + private uint _remotePacketSize; + private ChannelStub _channel; + private IList _channelExceptionRegister; + private UnknownRequestInfoWithWantReply _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); + _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(); + } + + 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.InitializeRemoteChannelInfo(_remoteChannelNumber, _remoteWindowSize, _remotePacketSize); + _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 == _channel.RemoteChannelNumber)), Times.Once); + } + + [TestMethod] + public void NoExceptionShouldHaveFired() + { + Assert.AreEqual(0, _channelExceptionRegister.Count); + } + } + + internal class UnknownRequestInfoWithWantReply : RequestInfo + { + public override string RequestName + { + get + { + return nameof(UnknownRequestInfoWithWantReply); + } + } + + internal UnknownRequestInfoWithWantReply() + { + WantReply = true; + } + } +} diff --git a/test/Renci.SshNet.Tests/Classes/ShellStreamTest_ReadExpect.cs b/test/Renci.SshNet.Tests/Classes/ShellStreamTest_ReadExpect.cs index 1f116f79..e4ec77f3 100644 --- a/test/Renci.SshNet.Tests/Classes/ShellStreamTest_ReadExpect.cs +++ b/test/Renci.SshNet.Tests/Classes/ShellStreamTest_ReadExpect.cs @@ -370,6 +370,8 @@ namespace Renci.SshNet.Tests.Classes public uint LocalPacketSize => throw new NotImplementedException(); + public uint RemoteChannelNumber => throw new NotImplementedException(); + public uint RemotePacketSize => throw new NotImplementedException(); public bool IsOpen => throw new NotImplementedException();