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 <rob.hague00@gmail.com>

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)

src/Renci.SshNet/Channels/IChannel.cs

@@ -59,6 +59,11 @@ namespace Renci.SshNet.Channels
         /// </remarks>
         uint LocalPacketSize { get; }
 
+        /// <summary>
+        /// Gets the remote channel number.
+        /// </summary>
+        uint RemoteChannelNumber { get; }
+
         /// <summary>
         /// Gets the maximum size of a data packet that can be sent using the channel.
         /// </summary>

src/Renci.SshNet/Messages/Connection/ChannelRequest/UnknownRequestInfo.cs

@@ -0,0 +1,22 @@
+namespace Renci.SshNet.Messages.Connection
+{
+    /// <summary>
+    /// Represents an unknown request information that we can't handle.
+    /// </summary>
+    internal sealed class UnknownRequestInfo : RequestInfo
+    {
+        /// <summary>
+        /// Gets the name of the request.
+        /// </summary>
+        public override string RequestName { get; }
+
+        /// <summary>
+        /// Initializes a new instance of the <see cref="UnknownRequestInfo"/> class.
+        /// <paramref name="requestName">The name of the unknown request.</paramref>
+        /// </summary>
+        internal UnknownRequestInfo(string requestName)
+        {
+            RequestName = requestName;
+        }
+    }
+}

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);
                 }
             }

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<ExceptionEventArgs> _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<ExceptionEventArgs>();
+            _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<Message>()));
+        }
+
+        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<ChannelRequestMessage>(new ChannelRequestMessage(_localChannelNumber, _requestInfo)));
+        }
+
+        [TestMethod]
+        public void FailureMessageWasSent()
+        {
+            SessionMock.Verify(p => p.SendMessage(It.Is<ChannelFailureMessage>(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;
+        }
+    }
+}

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();