Handle unknown channel messages correctly

[mus65]

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.

The test is mostly copy&paste from ChannelTest_OnSessionChannelRequestReceived_OnRequest_Exception. I also reproduced the issue and tested the fix with an actual ProFTPD server locally.

[Rob-Hague]

The whole paragraph is (emphasis mine):

   If 'want reply' is FALSE, no response will be sent to the request.
   **Otherwise**, the recipient responds with either
   SSH_MSG_CHANNEL_SUCCESS, SSH_MSG_CHANNEL_FAILURE, or request-specific
   continuation messages.  If the request is not recognized or is not
   supported for the channel, SSH_MSG_CHANNEL_FAILURE is returned.

To me, that says we should only send SSH_MSG_CHANNEL_FAILURE if 'want reply' is true.

i.e.

                         // Raise request specific event
                         OnRequest(requestInfo);
                     }
-                    else
+                    else if (e.Message.WantReply)
                     {
                         var reply = new ChannelFailureMessage(LocalChannelNumber);
                         SendMessage(reply);

Unfortunately, WantReply does not exist on ChannelRequestMessage, but instead on RequestInfo which is a wrapper for the "type-specific data" in the SSH_MSG_CHANNEL_REQUEST. I guess this was done because 'want reply' often depends on the type of the request when sending one. I see two options:

1. Plumb WantReply onto ChannelRequestMessage instead of RequestInfo
2. Add your UnknownRequestInfo into the production code in order to load the 'want reply' value; or 2.2 Un-abstract RequestInfo for the same purpose

2 is probably easiest. What do you think?

[mus65]

2. Add your UnknownRequestInfo into the production code in order to load the 'want reply' value;

@Rob-Hague Pushed. Moved UnknownRequestInfo to production code and it checks WantReply now. The test still needs its own class to set WantReply=true .

[zybexXL]

This RFC clarification (draft) may be relevant here, though it's a slightly different issue arising from the same RFC paragraph: http://www.watersprings.org/pub/id/draft-sgtatham-secsh-closure-race-00.html

This issue has been discussed before in other SSH packages: https://libssh2.org/mail/libssh2-devel-archive-2012-01/0000.shtml

And this implementation also agrees with your interpretation of WantReply: https://github.com/hudson/ganymed-ssh-2/blob/master/src/main/java/ch/ethz/ssh2/channel/ChannelManager.java#L1315

[Rob-Hague]

This RFC clarification (draft) may be relevant here

Thanks. The relevant text is:

Specifically, users of [RFC4254] should replace the first sentence of the third paragraph of section 5.4:

If 'want reply' is FALSE, no response will be sent to the request.

with the text:

If 'want reply' is FALSE, or if the sender has already sent SSH_MSG_CHANNEL_CLOSE for the channel, then the sender MUST NOT send a reply.

I think we are covered here because SendMessage has a check on IsOpen which is false when we have sent SSH_MSG_CHANNEL_CLOSE:

https://github.com/sshnet/SSH.NET/blob/db3d7e8d03d0a852cd7d16e1dee95a72939de317/src/Renci.SshNet/Channels/Channel.cs#L479-L488

https://github.com/sshnet/SSH.NET/blob/db3d7e8d03d0a852cd7d16e1dee95a72939de317/src/Renci.SshNet/Channels/Channel.cs#L547-L575

[anandgmenon]

@Rob-Hague/ @WojciechNagorski Can we have a release with this fix please?

[Rob-Hague]

Probably some time in May

[anandgmenon]

@Rob-Hague can we please have a release for this?

[Rob-Hague]

Yes, soon