Race condition on ProtocolExtensions in SshSession.cs

[VictorNicollet]

Hello, I found this race condition while investigating an unrelated matter.

Method SshSession.OpenChannelWithInitialRequestAsync computes a local variable isExtensionSupported based on whether Config.ProtocolExtensions and this.ProtocolExtensions both contain the extension identifier "open-channel-request@microsoft.com".

The computed property this.ProtocolExtensions returns this.Protocol.Extensions, which is:

• Initialized from Config in method SshSession.ConnectAsync.
• When the SSH_MSG_EXT_INFO is received from the server, method SshSession.HandleMessageAsync(ExtensionInfoMessage, CancellationToken) assigns a new, empty dictionary to this.Protocol.Extensions, then uses this.Protocol.Extensions.Add() to fill it with the extensions received from the server.

The sequence of events that leads to the race is the following:

1. Calling SshClient.OpenSessionAsync invokes SshSession.ConnectAsync, which starts the background message-processing loop in SshSession.ProcessMessages and returns immediately.
2. Calling SshClientSession.AuthenticateClientAsync sends the authentication messages, and also returns immediately without waiting for a response.
3. Caller does not wait for the TaskCompletionSource passed to SshClientSession.AuthenticateClientAsync to complete (a scenario recommended by the documentation of that function, for performance purposes).
4. Calling SshClientSession.OpenChannelAsync reads from this.ProtocolExtensions. At this point,:

• If the background message-processing loop has already processed the SSH_MSG_EXT_INFO message, the program will behave properly.
• If the background message-processing loop has not yet processed the SSH_MSG_EXT_INFO message, the program will instead see the extensions from the configuration of the session, which will lead it to assume that "open-channel-request@microsoft.com" is supported by the server, even if it isn't.
• If the background message-processing loop is currently processing SSH_MSG_EXT_INFO and has already assigned an empty dictionary to this.Protocol.Extensions but has not yet filled it, the program will assume that "open-channel-request@microsoft.com" is not supported by the server even if it actually is.
• If the background message-processing loop is currently processing SSH_MSG_EXT_INFO, and is in the process of executing this.Protocol.Extensions.Add, then a System.Collections.Dictionary<,> is being both read and written to at the same time by two different threads, which may lead to misbehavior (although given the current implementation, this would require the server to send enough extensions to cause problems).

Waiting for the for the TaskCompletionSource passed to SshClientSession.AuthenticateClientAsync to complete eliminates the race condition.

[jasongin]

Thanks for the detailed report. I'll investigate and fix if appropriate.

This issue should go to the SSH repo though: https://github.com/microsoft/dev-tunnels-ssh/issues

[VictorNicollet]

This issue should go to the SSH repo though: https://github.com/microsoft/dev-tunnels-ssh/issues

I apologize. Do we want me to close this and open it over there ?

[derekbekoe]

I've transferred the issue to the ssh repo.

[jasongin]

If the background message-processing loop has not yet processed the SSH_MSG_EXT_INFO message, the program will instead see the extensions from the configuration of the session, which will lead it to assume that "open-channel-request@microsoft.com" is supported by the server, even if it isn't.

This part of the analysis is incorrect. In that case, the code here detects when the extension is supported locally while the list of supported extensions is not yet received from the remote side. In that case it optimistically sends the extended request message, but it has a fallback in case the request is not supported (which will be barely slower than not trying the extension at all).

The confusion might be related to this expression, which is admittedly difficult to mentally parse:

bool? isExtensionSupported =
   Config.ProtocolExtensions.Contains(SshProtocolExtensionNames.OpenChannelRequest) ?
   ProtocolExtensions?.ContainsKey(SshProtocolExtensionNames.OpenChannelRequest) : false;

The result of that expression is:

• true if the extension is known to be supported by both sides.
• false if the extension is known to be unsupported by either side.
• null if the extension is supported locally but the other side's list of supported extensions have not yet been received, as indicated by null ProtocolExtensions.

The third case triggers the optimistic request + fallback behavior.

---

If the background message-processing loop has already assigned an empty dictionary to this.Protocol.Extensions but has not yet filled it

If the background message-processing loop is currently in the process of executing this.Protocol.Extensions.Add

These are valid potential threading issues, which I will fix.

[VictorNicollet]

You are correct, I was under the impression that new SshProtocol would assign SshProtocol.Extensions based on the incoming configuration, but I cannot find code that would do that, so I was mistaken.