Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle unknown channel messages correctly #1363

Merged
merged 5 commits into from
Apr 4, 2024

Conversation

mus65
Copy link
Contributor

@mus65 mus65 commented Mar 24, 2024

See discussion #1218 . Some servers send custom channel messages like '[email protected]' 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.

See discussion sshnet#1218 . Some servers send custom channel messages
like '[email protected]' 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.
@Rob-Hague
Copy link
Collaborator

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
Copy link
Contributor Author

mus65 commented Apr 1, 2024

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
Copy link
Contributor

zybexXL commented Apr 1, 2024

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
Copy link
Collaborator

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:

protected void SendMessage(Message message)
{
// Send channel messages only while channel is open
if (!IsOpen)
{
return;
}
_session.SendMessage(message);
}

if (!_closeMessageSent && IsOpen && IsConnected)
{
if (TrySendMessage(new ChannelCloseMessage(RemoteChannelNumber)))
{
_closeMessageSent = true;
// only wait for the channel to be closed by the server if we didn't send a
// SSH_MSG_CHANNEL_CLOSE as response to a SSH_MSG_CHANNEL_CLOSE sent by the
// server
var closeWaitResult = _session.TryWait(_channelClosedWaitHandle, ConnectionInfo.ChannelCloseTimeout);
if (closeWaitResult != WaitResult.Success)
{
DiagnosticAbstraction.Log(string.Format("Wait for channel close not successful: {0:G}.", closeWaitResult));
}
}
}
if (IsOpen)
{
// mark sure the channel is marked closed before we raise the Closed event
// this also ensures don't raise the Closed event more than once
IsOpen = false;
if (_closeMessageReceived)
{
// raise event signaling that both ends of the channel have been closed
Closed?.Invoke(this, new ChannelEventArgs(LocalChannelNumber));
}
}

Copy link
Collaborator

@Rob-Hague Rob-Hague left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@WojciechNagorski WojciechNagorski merged commit 70a0a08 into sshnet:develop Apr 4, 2024
1 check passed
@anandgmenon
Copy link

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

@Rob-Hague
Copy link
Collaborator

Probably some time in May

@anandgmenon
Copy link

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

@Rob-Hague
Copy link
Collaborator

Yes, soon

@anandgmenon
Copy link

anandgmenon commented Jun 28, 2024

Hey @Rob-Hague can you provide a tentative ETA on when you're planning to do the next release?

@mus65 mus65 deleted the unknownrequestmessage branch September 14, 2024 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants