From 5070ca970ee9545bbc6ba6f83a821c32ddbb9e94 Mon Sep 17 00:00:00 2001 From: Scott Xu Date: Sat, 30 Mar 2024 21:52:34 +0800 Subject: [PATCH 01/14] Implement OpenSSH strict key exchange extension --- src/Renci.SshNet/ConnectionInfo.cs | 1 + src/Renci.SshNet/Session.cs | 34 ++++++++++++++++++++++++------ 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/src/Renci.SshNet/ConnectionInfo.cs b/src/Renci.SshNet/ConnectionInfo.cs index 7584816ff..66e7632b7 100644 --- a/src/Renci.SshNet/ConnectionInfo.cs +++ b/src/Renci.SshNet/ConnectionInfo.cs @@ -378,6 +378,7 @@ public ConnectionInfo(string host, int port, string username, ProxyTypes proxyTy { "diffie-hellman-group14-sha256", () => new KeyExchangeDiffieHellmanGroup14Sha256() }, { "diffie-hellman-group14-sha1", () => new KeyExchangeDiffieHellmanGroup14Sha1() }, { "diffie-hellman-group1-sha1", () => new KeyExchangeDiffieHellmanGroup1Sha1() }, + { "kex-strict-c-v00@openssh.com", null }, }; Encryptions = new Dictionary diff --git a/src/Renci.SshNet/Session.cs b/src/Renci.SshNet/Session.cs index 0c067ec2d..62438a109 100644 --- a/src/Renci.SshNet/Session.cs +++ b/src/Renci.SshNet/Session.cs @@ -154,6 +154,12 @@ public class Session : ISession /// private bool _isDisconnecting; + /// + /// Indicates whether server supports strict key exchange. + /// 1.10. + /// + private bool _isStrictKex; + private IKeyExchange _keyExchange; private HashAlgorithm _serverMac; @@ -1106,13 +1112,20 @@ internal void SendMessage(Message message) SendPacket(data, 0, data.Length); } - // increment the packet sequence number only after we're sure the packet has - // been sent; even though it's only used for the MAC, it needs to be incremented - // for each package sent. - // - // the server will use it to verify the data integrity, and as such the order in - // which messages are sent must follow the outbound packet sequence number - _outboundPacketSequence++; + if (_isStrictKex && message is NewKeysMessage) + { + _outboundPacketSequence = 0; + } + else + { + // increment the packet sequence number only after we're sure the packet has + // been sent; even though it's only used for the MAC, it needs to be incremented + // for each package sent. + // + // the server will use it to verify the data integrity, and as such the order in + // which messages are sent must follow the outbound packet sequence number + _outboundPacketSequence++; + } } } @@ -1446,6 +1459,8 @@ internal void OnKeyExchangeInitReceived(KeyExchangeInitMessage message) _keyExchangeCompletedWaitHandle.Reset(); + _isStrictKex = message.KeyExchangeAlgorithms.Contains("kex-strict-s-v00@openssh.com"); + // Disable messages that are not key exchange related _sshMessageFactory.DisableNonKeyExchangeMessages(); @@ -1522,6 +1537,11 @@ internal void OnNewKeysReceived(NewKeysMessage message) // Enable activated messages that are not key exchange related _sshMessageFactory.EnableActivatedMessages(); + if (_isStrictKex) + { + _inboundPacketSequence = 0; + } + NewKeysReceived?.Invoke(this, new MessageEventArgs(message)); // Signal that key exchange completed From 5357fee57bb6d142cadad9181a6cc97d7a500682 Mon Sep 17 00:00:00 2001 From: Scott Xu Date: Tue, 2 Apr 2024 19:48:48 +0800 Subject: [PATCH 02/14] The pseudo-algorithm is only valid in the initial SSH2_MSG_KEXINIT and MUST be ignored if they are present in subsequent SSH2_MSG_KEXINIT packets. --- src/Renci.SshNet/Session.cs | 47 +++++++++++++++++++++++++------------ 1 file changed, 32 insertions(+), 15 deletions(-) diff --git a/src/Renci.SshNet/Session.cs b/src/Renci.SshNet/Session.cs index 62438a109..95cc9bd02 100644 --- a/src/Renci.SshNet/Session.cs +++ b/src/Renci.SshNet/Session.cs @@ -154,6 +154,11 @@ public class Session : ISession /// private bool _isDisconnecting; + /// + /// Indicates whether it is the init kex. + /// + private bool _isInitialKex = true; + /// /// Indicates whether server supports strict key exchange. /// 1.10. @@ -294,20 +299,20 @@ public Message ClientInitMessage get { _clientInitMessage ??= new KeyExchangeInitMessage - { - KeyExchangeAlgorithms = ConnectionInfo.KeyExchangeAlgorithms.Keys.ToArray(), - ServerHostKeyAlgorithms = ConnectionInfo.HostKeyAlgorithms.Keys.ToArray(), - EncryptionAlgorithmsClientToServer = ConnectionInfo.Encryptions.Keys.ToArray(), - EncryptionAlgorithmsServerToClient = ConnectionInfo.Encryptions.Keys.ToArray(), - MacAlgorithmsClientToServer = ConnectionInfo.HmacAlgorithms.Keys.ToArray(), - MacAlgorithmsServerToClient = ConnectionInfo.HmacAlgorithms.Keys.ToArray(), - CompressionAlgorithmsClientToServer = ConnectionInfo.CompressionAlgorithms.Keys.ToArray(), - CompressionAlgorithmsServerToClient = ConnectionInfo.CompressionAlgorithms.Keys.ToArray(), - LanguagesClientToServer = new[] { string.Empty }, - LanguagesServerToClient = new[] { string.Empty }, - FirstKexPacketFollows = false, - Reserved = 0 - }; + { + KeyExchangeAlgorithms = ConnectionInfo.KeyExchangeAlgorithms.Keys.ToArray(), + ServerHostKeyAlgorithms = ConnectionInfo.HostKeyAlgorithms.Keys.ToArray(), + EncryptionAlgorithmsClientToServer = ConnectionInfo.Encryptions.Keys.ToArray(), + EncryptionAlgorithmsServerToClient = ConnectionInfo.Encryptions.Keys.ToArray(), + MacAlgorithmsClientToServer = ConnectionInfo.HmacAlgorithms.Keys.ToArray(), + MacAlgorithmsServerToClient = ConnectionInfo.HmacAlgorithms.Keys.ToArray(), + CompressionAlgorithmsClientToServer = ConnectionInfo.CompressionAlgorithms.Keys.ToArray(), + CompressionAlgorithmsServerToClient = ConnectionInfo.CompressionAlgorithms.Keys.ToArray(), + LanguagesClientToServer = new[] { string.Empty }, + LanguagesServerToClient = new[] { string.Empty }, + FirstKexPacketFollows = false, + Reserved = 0 + }; return _clientInitMessage; } @@ -1459,7 +1464,17 @@ internal void OnKeyExchangeInitReceived(KeyExchangeInitMessage message) _keyExchangeCompletedWaitHandle.Reset(); - _isStrictKex = message.KeyExchangeAlgorithms.Contains("kex-strict-s-v00@openssh.com"); + if (_isInitialKex && message.KeyExchangeAlgorithms.Contains("kex-strict-s-v00@openssh.com")) + { + _isStrictKex = true; + + DiagnosticAbstraction.Log(string.Format("[{0}] Enabling strict key exchange extension.", ToHex(SessionId))); + + if (_inboundPacketSequence != 1) + { + throw new SshConnectionException("KEXINIT was not the first packet during strict key exchange", DisconnectReason.KeyExchangeFailed); + } + } // Disable messages that are not key exchange related _sshMessageFactory.DisableNonKeyExchangeMessages(); @@ -1537,6 +1552,8 @@ internal void OnNewKeysReceived(NewKeysMessage message) // Enable activated messages that are not key exchange related _sshMessageFactory.EnableActivatedMessages(); + _isInitialKex = false; + if (_isStrictKex) { _inboundPacketSequence = 0; From afc4bb298eeb3aa3fdba924b04b64dbbc8ba1904 Mon Sep 17 00:00:00 2001 From: Scott Xu Date: Wed, 10 Apr 2024 23:24:17 +0800 Subject: [PATCH 03/14] Only send strict kex pseudo algorithm for the first kex. Strictly disable non-kex massages in strict kex mode. --- src/Renci.SshNet/ConnectionInfo.cs | 1 - src/Renci.SshNet/Session.cs | 63 +++++++++++++++------------ src/Renci.SshNet/SshMessageFactory.cs | 31 +++++++++++-- 3 files changed, 62 insertions(+), 33 deletions(-) diff --git a/src/Renci.SshNet/ConnectionInfo.cs b/src/Renci.SshNet/ConnectionInfo.cs index 66e7632b7..7584816ff 100644 --- a/src/Renci.SshNet/ConnectionInfo.cs +++ b/src/Renci.SshNet/ConnectionInfo.cs @@ -378,7 +378,6 @@ public ConnectionInfo(string host, int port, string username, ProxyTypes proxyTy { "diffie-hellman-group14-sha256", () => new KeyExchangeDiffieHellmanGroup14Sha256() }, { "diffie-hellman-group14-sha1", () => new KeyExchangeDiffieHellmanGroup14Sha1() }, { "diffie-hellman-group1-sha1", () => new KeyExchangeDiffieHellmanGroup1Sha1() }, - { "kex-strict-c-v00@openssh.com", null }, }; Encryptions = new Dictionary diff --git a/src/Renci.SshNet/Session.cs b/src/Renci.SshNet/Session.cs index 95cc9bd02..ebd8a6380 100644 --- a/src/Renci.SshNet/Session.cs +++ b/src/Renci.SshNet/Session.cs @@ -157,7 +157,7 @@ public class Session : ISession /// /// Indicates whether it is the init kex. /// - private bool _isInitialKex = true; + private bool _isInitialKex; /// /// Indicates whether server supports strict key exchange. @@ -288,35 +288,11 @@ public bool IsConnected /// public byte[] SessionId { get; private set; } - private Message _clientInitMessage; - /// /// Gets the client init message. /// /// The client init message. - public Message ClientInitMessage - { - get - { - _clientInitMessage ??= new KeyExchangeInitMessage - { - KeyExchangeAlgorithms = ConnectionInfo.KeyExchangeAlgorithms.Keys.ToArray(), - ServerHostKeyAlgorithms = ConnectionInfo.HostKeyAlgorithms.Keys.ToArray(), - EncryptionAlgorithmsClientToServer = ConnectionInfo.Encryptions.Keys.ToArray(), - EncryptionAlgorithmsServerToClient = ConnectionInfo.Encryptions.Keys.ToArray(), - MacAlgorithmsClientToServer = ConnectionInfo.HmacAlgorithms.Keys.ToArray(), - MacAlgorithmsServerToClient = ConnectionInfo.HmacAlgorithms.Keys.ToArray(), - CompressionAlgorithmsClientToServer = ConnectionInfo.CompressionAlgorithms.Keys.ToArray(), - CompressionAlgorithmsServerToClient = ConnectionInfo.CompressionAlgorithms.Keys.ToArray(), - LanguagesClientToServer = new[] { string.Empty }, - LanguagesServerToClient = new[] { string.Empty }, - FirstKexPacketFollows = false, - Reserved = 0 - }; - - return _clientInitMessage; - } - } + public Message ClientInitMessage { get; private set; } /// /// Gets the server version string. @@ -624,6 +600,8 @@ public void Connect() // Send our key exchange init. // We need to do this before starting the message listener to avoid the case where we receive the server // key exchange init and we continue the key exchange before having sent our own init. + _isInitialKex = true; + ClientInitMessage = BuildClientInitMessage(includeStrictKexPseudoAlgorithm: true); SendMessage(ClientInitMessage); // Mark the message listener threads as started @@ -748,6 +726,8 @@ public async Task ConnectAsync(CancellationToken cancellationToken) // Send our key exchange init. // We need to do this before starting the message listener to avoid the case where we receive the server // key exchange init and we continue the key exchange before having sent our own init. + _isInitialKex = true; + ClientInitMessage = BuildClientInitMessage(includeStrictKexPseudoAlgorithm: true); SendMessage(ClientInitMessage); // Mark the message listener threads as started @@ -1477,7 +1457,7 @@ internal void OnKeyExchangeInitReceived(KeyExchangeInitMessage message) } // Disable messages that are not key exchange related - _sshMessageFactory.DisableNonKeyExchangeMessages(); + _sshMessageFactory.DisableNonKeyExchangeMessages(_isStrictKex); _keyExchange = _serviceFactory.CreateKeyExchange(ConnectionInfo.KeyExchangeAlgorithms, message.KeyExchangeAlgorithms); @@ -1552,7 +1532,11 @@ internal void OnNewKeysReceived(NewKeysMessage message) // Enable activated messages that are not key exchange related _sshMessageFactory.EnableActivatedMessages(); - _isInitialKex = false; + if (_isInitialKex) + { + _isInitialKex = false; + ClientInitMessage = BuildClientInitMessage(includeStrictKexPseudoAlgorithm: false); + } if (_isStrictKex) { @@ -2093,7 +2077,28 @@ private void Reset() private static SshConnectionException CreateConnectionAbortedByServerException() { return new SshConnectionException("An established connection was aborted by the server.", - DisconnectReason.ConnectionLost); + DisconnectReason.ConnectionLost); + } + + private KeyExchangeInitMessage BuildClientInitMessage(bool includeStrictKexPseudoAlgorithm) + { + return new KeyExchangeInitMessage + { + KeyExchangeAlgorithms = includeStrictKexPseudoAlgorithm ? + ConnectionInfo.KeyExchangeAlgorithms.Keys.Concat(["kex-strict-c-v00@openssh.com"]).ToArray() : + ConnectionInfo.KeyExchangeAlgorithms.Keys.ToArray(), + ServerHostKeyAlgorithms = ConnectionInfo.HostKeyAlgorithms.Keys.ToArray(), + EncryptionAlgorithmsClientToServer = ConnectionInfo.Encryptions.Keys.ToArray(), + EncryptionAlgorithmsServerToClient = ConnectionInfo.Encryptions.Keys.ToArray(), + MacAlgorithmsClientToServer = ConnectionInfo.HmacAlgorithms.Keys.ToArray(), + MacAlgorithmsServerToClient = ConnectionInfo.HmacAlgorithms.Keys.ToArray(), + CompressionAlgorithmsClientToServer = ConnectionInfo.CompressionAlgorithms.Keys.ToArray(), + CompressionAlgorithmsServerToClient = ConnectionInfo.CompressionAlgorithms.Keys.ToArray(), + LanguagesClientToServer = new[] { string.Empty }, + LanguagesServerToClient = new[] { string.Empty }, + FirstKexPacketFollows = false, + Reserved = 0, + }; } private bool _disposed; diff --git a/src/Renci.SshNet/SshMessageFactory.cs b/src/Renci.SshNet/SshMessageFactory.cs index efa861256..2887559a6 100644 --- a/src/Renci.SshNet/SshMessageFactory.cs +++ b/src/Renci.SshNet/SshMessageFactory.cs @@ -115,16 +115,41 @@ public Message Create(byte messageNumber) return enabledMessageMetadata.Create(); } - public void DisableNonKeyExchangeMessages() + /// + /// Disables non-KeyExchange messages. + /// + /// + /// to indicate the strict key exchange mode; otherwise . + /// In strict key exchange mode, only below messages are allowed: + /// + /// SSH_MSG_KEXINIT -> 20 + /// SSH_MSG_NEWKEYS -> 21 + /// SSH_MSG_DISCONNECT -> 1 + /// + /// Note: + /// The relevant KEX Reply MSG will be allowed from a sub class of KeyExchange class. + /// For example, it calls Session.RegisterMessage("SSH_MSG_KEX_ECDH_REPLY"); if the curve25519-sha256 KEX algorithm is selected per negotiation. + /// + public void DisableNonKeyExchangeMessages(bool strict) { for (var i = 0; i < AllMessages.Length; i++) { var messageMetadata = AllMessages[i]; var messageNumber = messageMetadata.Number; - if (messageNumber is (> 2 and < 20) or > 30) + if (strict) + { + if (messageNumber is not 20 and not 21 and not 1) + { + _enabledMessagesByNumber[messageNumber] = null; + } + } + else { - _enabledMessagesByNumber[messageNumber] = null; + if (messageNumber is (> 2 and < 20) or > 30) + { + _enabledMessagesByNumber[messageNumber] = null; + } } } } From 63503509863946224f04627d7586cb41588db382 Mon Sep 17 00:00:00 2001 From: Scott Xu Date: Thu, 11 Apr 2024 21:16:35 +0800 Subject: [PATCH 04/14] Unit tests for strict kex --- .../SessionTest_ConnectToServerFails.cs | 4 +- .../Classes/SessionTest_ConnectedBase.cs | 12 +- .../Classes/SessionTest_ConnectingBase.cs | 251 ++++++++++++++++++ ...onnecting_ServerIdentificationReceived.cs} | 8 +- ...erKexInit_ServerDoesNotSupportStrictKex.cs | 25 ++ ...ageAfterKexInit_ServerSupportsStrictKex.cs | 34 +++ .../Classes/SessionTest_NotConnected.cs | 4 +- 7 files changed, 318 insertions(+), 20 deletions(-) create mode 100644 test/Renci.SshNet.Tests/Classes/SessionTest_ConnectingBase.cs rename test/Renci.SshNet.Tests/Classes/{SessionTest_Connected_ServerIdentificationReceived.cs => SessionTest_Connecting_ServerIdentificationReceived.cs} (91%) create mode 100644 test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_ServerDoesNotSupportStrictKex.cs create mode 100644 test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_ServerSupportsStrictKex.cs diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectToServerFails.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectToServerFails.cs index 1950f2759..326bae645 100644 --- a/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectToServerFails.cs +++ b/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectToServerFails.cs @@ -87,7 +87,7 @@ public void IsConnectedShouldReturnFalse() } [TestMethod] - public void SendMessageShouldThrowShhConnectionException() + public void SendMessageShouldThrowSshConnectionException() { try { @@ -189,7 +189,7 @@ public void ISession_MessageListenerCompletedShouldBeSignaled() } [TestMethod] - public void ISession_SendMessageShouldThrowShhConnectionException() + public void ISession_SendMessageShouldThrowSshConnectionException() { var session = (ISession)_session; diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectedBase.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectedBase.cs index 5ce0d4675..0279205f7 100644 --- a/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectedBase.cs +++ b/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectedBase.cs @@ -46,8 +46,7 @@ public abstract class SessionTest_ConnectedBase protected Session Session { get; private set; } protected Socket ClientSocket { get; private set; } protected Socket ServerSocket { get; private set; } - internal SshIdentification ServerIdentification { get; set; } - protected bool CallSessionConnectWhenArrange { get; set; } + internal SshIdentification ServerIdentification { get; private set; } /// /// Should the "server" wait for the client kexinit before sending its own. @@ -163,8 +162,6 @@ protected virtual void SetupData() ClientSocket = new DirectConnector(_socketFactory).Connect(ConnectionInfo); - CallSessionConnectWhenArrange = true; - void SendKeyExchangeInit() { var keyExchangeInitMessage = new KeyExchangeInitMessage @@ -204,7 +201,7 @@ private void SetupMocks() _ = ServiceFactoryMock.Setup(p => p.CreateProtocolVersionExchange()) .Returns(_protocolVersionExchangeMock.Object); _ = _protocolVersionExchangeMock.Setup(p => p.Start(Session.ClientVersion, ClientSocket, ConnectionInfo.Timeout)) - .Returns(() => ServerIdentification); + .Returns(ServerIdentification); _ = ServiceFactoryMock.Setup(p => p.CreateKeyExchange(ConnectionInfo.KeyExchangeAlgorithms, new[] { _keyExchangeAlgorithm })).Returns(_keyExchangeMock.Object); _ = _keyExchangeMock.Setup(p => p.Name) .Returns(_keyExchangeAlgorithm); @@ -244,10 +241,7 @@ protected void Arrange() SetupData(); SetupMocks(); - if (CallSessionConnectWhenArrange) - { - Session.Connect(); - } + Session.Connect(); } protected virtual void ClientAuthentication_Callback() diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectingBase.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectingBase.cs new file mode 100644 index 000000000..7673f82e2 --- /dev/null +++ b/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectingBase.cs @@ -0,0 +1,251 @@ +using System; +using System.Collections.Generic; +using System.Globalization; +using System.Net; +using System.Net.Sockets; +using System.Security.Cryptography; + +using Microsoft.VisualStudio.TestTools.UnitTesting; + +using Moq; + +using Renci.SshNet.Common; +using Renci.SshNet.Compression; +using Renci.SshNet.Connection; +using Renci.SshNet.Messages; +using Renci.SshNet.Messages.Transport; +using Renci.SshNet.Security; +using Renci.SshNet.Security.Cryptography; +using Renci.SshNet.Tests.Common; + +namespace Renci.SshNet.Tests.Classes +{ + [TestClass] + public abstract class SessionTest_ConnectingBase + { + internal Mock ServiceFactoryMock { get; private set; } + internal Mock SocketFactoryMock { get; private set; } + internal Mock ConnectorMock { get; private set; } + + private Mock _protocolVersionExchangeMock; + private Mock _keyExchangeMock; + private Mock _clientAuthenticationMock; + private IPEndPoint _serverEndPoint; + private string[] _keyExchangeAlgorithms; + private bool _authenticationStarted; + private SocketFactory _socketFactory; + + protected Random Random { get; private set; } + protected byte[] SessionId { get; private set; } + protected ConnectionInfo ConnectionInfo { get; private set; } + protected IList DisconnectedRegister { get; private set; } + protected IList> DisconnectReceivedRegister { get; private set; } + protected IList ErrorOccurredRegister { get; private set; } + protected AsyncSocketListener ServerListener { get; private set; } + protected IList ServerBytesReceivedRegister { get; private set; } + protected Session Session { get; private set; } + protected Socket ClientSocket { get; private set; } + protected Socket ServerSocket { get; private set; } + internal SshIdentification ServerIdentification { get; set; } + protected virtual bool IsStrictKex { get; } + + [TestInitialize] + public void Setup() + { + CreateMocks(); + SetupData(); + SetupMocks(); + } + + protected virtual void MITMAttack() + { + } + + [TestCleanup] + public void TearDown() + { + if (ServerListener != null) + { + ServerListener.Dispose(); + ServerListener = null; + } + + if (ServerSocket != null) + { + ServerSocket.Dispose(); + ServerSocket = null; + } + + if (Session != null) + { + Session.Dispose(); + Session = null; + } + + if (ClientSocket != null && ClientSocket.Connected) + { + ClientSocket.Shutdown(SocketShutdown.Both); + ClientSocket.Dispose(); + } + } + + protected virtual void SetupData() + { + Random = new Random(); + + _serverEndPoint = new IPEndPoint(IPAddress.Loopback, 8122); + ConnectionInfo = new ConnectionInfo( + _serverEndPoint.Address.ToString(), + _serverEndPoint.Port, + "user", + new PasswordAuthenticationMethod("user", "password")) + { Timeout = TimeSpan.FromSeconds(20) }; + _keyExchangeAlgorithms = IsStrictKex ? + [Random.Next().ToString(CultureInfo.InvariantCulture), "kex-strict-s-v00@openssh.com"] : + [Random.Next().ToString(CultureInfo.InvariantCulture)]; + SessionId = new byte[10]; + Random.NextBytes(SessionId); + DisconnectedRegister = new List(); + DisconnectReceivedRegister = new List>(); + ErrorOccurredRegister = new List(); + ServerBytesReceivedRegister = new List(); + ServerIdentification = new SshIdentification("2.0", "OurServerStub"); + _authenticationStarted = false; + _socketFactory = new SocketFactory(); + + Session = new Session(ConnectionInfo, ServiceFactoryMock.Object, SocketFactoryMock.Object); + Session.Disconnected += (sender, args) => DisconnectedRegister.Add(args); + Session.DisconnectReceived += (sender, args) => DisconnectReceivedRegister.Add(args); + Session.ErrorOccured += (sender, args) => ErrorOccurredRegister.Add(args); + + ServerListener = new AsyncSocketListener(_serverEndPoint) + { + ShutdownRemoteCommunicationSocket = false + }; + ServerListener.Connected += socket => + { + ServerSocket = socket; + var keyExchangeInitMessage = new KeyExchangeInitMessage + { + CompressionAlgorithmsClientToServer = new string[0], + CompressionAlgorithmsServerToClient = new string[0], + EncryptionAlgorithmsClientToServer = new string[0], + EncryptionAlgorithmsServerToClient = new string[0], + KeyExchangeAlgorithms = _keyExchangeAlgorithms, + LanguagesClientToServer = new string[0], + LanguagesServerToClient = new string[0], + MacAlgorithmsClientToServer = new string[0], + MacAlgorithmsServerToClient = new string[0], + ServerHostKeyAlgorithms = new string[0] + }; + var keyExchangeInit = keyExchangeInitMessage.GetPacket(8, null); + _ = ServerSocket.Send(keyExchangeInit, 4, keyExchangeInit.Length - 4, SocketFlags.None); + }; + ServerListener.BytesReceived += (received, socket) => + { + ServerBytesReceivedRegister.Add(received); + + if (received.Length > 5 && received[5] == 20) + { + MITMAttack(); + var newKeysMessage = new NewKeysMessage(); + var newKeys = newKeysMessage.GetPacket(8, null); + _ = ServerSocket.Send(newKeys, 4, newKeys.Length - 4, SocketFlags.None); + + if (!_authenticationStarted) + { + var serviceAcceptMessage = ServiceAcceptMessageBuilder.Create(ServiceName.UserAuthentication) + .Build(); + _ = ServerSocket.Send(serviceAcceptMessage, 0, serviceAcceptMessage.Length, SocketFlags.None); + + _authenticationStarted = true; + } + } + }; + ServerListener.Start(); + + ClientSocket = new DirectConnector(_socketFactory).Connect(ConnectionInfo); + } + + private void CreateMocks() + { + ServiceFactoryMock = new Mock(MockBehavior.Strict); + SocketFactoryMock = new Mock(MockBehavior.Strict); + ConnectorMock = new Mock(MockBehavior.Strict); + _protocolVersionExchangeMock = new Mock(MockBehavior.Strict); + _keyExchangeMock = new Mock(MockBehavior.Strict); + _clientAuthenticationMock = new Mock(MockBehavior.Strict); + } + + private void SetupMocks() + { + _ = ServiceFactoryMock.Setup(p => p.CreateConnector(ConnectionInfo, SocketFactoryMock.Object)) + .Returns(ConnectorMock.Object); + _ = ConnectorMock.Setup(p => p.Connect(ConnectionInfo)) + .Returns(ClientSocket); + _ = ServiceFactoryMock.Setup(p => p.CreateProtocolVersionExchange()) + .Returns(_protocolVersionExchangeMock.Object); + _ = _protocolVersionExchangeMock.Setup(p => p.Start(Session.ClientVersion, ClientSocket, ConnectionInfo.Timeout)) + .Returns(() => ServerIdentification); + _ = ServiceFactoryMock.Setup(p => p.CreateKeyExchange(ConnectionInfo.KeyExchangeAlgorithms, _keyExchangeAlgorithms)).Returns(_keyExchangeMock.Object); + + _ = _keyExchangeMock.Setup(p => p.Name) + .Returns(_keyExchangeAlgorithms[0]); + _ = _keyExchangeMock.Setup(p => p.Start(Session, It.IsAny(), false)); + _ = _keyExchangeMock.Setup(p => p.ExchangeHash) + .Returns(SessionId); + _ = _keyExchangeMock.Setup(p => p.CreateServerCipher()) + .Returns((Cipher) null); + _ = _keyExchangeMock.Setup(p => p.CreateClientCipher()) + .Returns((Cipher) null); + _ = _keyExchangeMock.Setup(p => p.CreateServerHash(out It.Ref.IsAny)) + .Returns((ref bool serverEtm) => + { + serverEtm = false; + return (HashAlgorithm) null; + }); + _ = _keyExchangeMock.Setup(p => p.CreateClientHash(out It.Ref.IsAny)) + .Returns((ref bool clientEtm) => + { + clientEtm = false; + return (HashAlgorithm) null; + }); + _ = _keyExchangeMock.Setup(p => p.CreateCompressor()) + .Returns((Compressor) null); + _ = _keyExchangeMock.Setup(p => p.CreateDecompressor()) + .Returns((Compressor) null); + _ = _keyExchangeMock.Setup(p => p.Dispose()); + _ = ServiceFactoryMock.Setup(p => p.CreateClientAuthentication()) + .Returns(_clientAuthenticationMock.Object); + _ = _clientAuthenticationMock.Setup(p => p.Authenticate(ConnectionInfo, Session)); + } + + private class ServiceAcceptMessageBuilder + { + private readonly ServiceName _serviceName; + + private ServiceAcceptMessageBuilder(ServiceName serviceName) + { + _serviceName = serviceName; + } + + public static ServiceAcceptMessageBuilder Create(ServiceName serviceName) + { + return new ServiceAcceptMessageBuilder(serviceName); + } + + public byte[] Build() + { + var serviceName = _serviceName.ToArray(); + var target = new ServiceAcceptMessage(); + + var sshDataStream = new SshDataStream(4 + 1 + 1 + 4 + serviceName.Length); + sshDataStream.Write((uint) (sshDataStream.Capacity - 4)); // packet length + sshDataStream.WriteByte(0); // padding length + sshDataStream.WriteByte(target.MessageNumber); + sshDataStream.WriteBinary(serviceName); + return sshDataStream.ToArray(); + } + } + } +} diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_Connected_ServerIdentificationReceived.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerIdentificationReceived.cs similarity index 91% rename from test/Renci.SshNet.Tests/Classes/SessionTest_Connected_ServerIdentificationReceived.cs rename to test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerIdentificationReceived.cs index 7b5ff1d86..fdb7cc79f 100644 --- a/test/Renci.SshNet.Tests/Classes/SessionTest_Connected_ServerIdentificationReceived.cs +++ b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerIdentificationReceived.cs @@ -5,14 +5,12 @@ namespace Renci.SshNet.Tests.Classes { [TestClass] - public class SessionTest_Connected_ServerIdentificationReceived : SessionTest_ConnectedBase + public class SessionTest_Connecting_ServerIdentificationReceived : SessionTest_ConnectingBase { protected override void SetupData() { base.SetupData(); - CallSessionConnectWhenArrange = false; - Session.ServerIdentificationReceived += (s, e) => { if ((e.SshIdentification.SoftwareVersion.StartsWith("OpenSSH_6.5", System.StringComparison.Ordinal) || e.SshIdentification.SoftwareVersion.StartsWith("OpenSSH_6.6", System.StringComparison.Ordinal)) @@ -24,10 +22,6 @@ protected override void SetupData() }; } - protected override void Act() - { - } - [TestMethod] [DataRow("OpenSSH_6.5")] [DataRow("OpenSSH_6.5p1")] diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_ServerDoesNotSupportStrictKex.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_ServerDoesNotSupportStrictKex.cs new file mode 100644 index 000000000..40f2db412 --- /dev/null +++ b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_ServerDoesNotSupportStrictKex.cs @@ -0,0 +1,25 @@ +using System.Net.Sockets; + +using Microsoft.VisualStudio.TestTools.UnitTesting; + +using Renci.SshNet.Messages.Transport; + +namespace Renci.SshNet.Tests.Classes +{ + [TestClass] + public class SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_ServerDoesNotSupportStrictKex : SessionTest_ConnectingBase + { + protected override void MITMAttack() + { + var ignoreMessage = new IgnoreMessage(); + var ignore = ignoreMessage.GetPacket(8, null); + _ = ServerSocket.Send(ignore, 4, ignore.Length - 4, SocketFlags.None); + } + + [TestMethod] + public void DoesNotThrowException() + { + Session.Connect(); + } + } +} diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_ServerSupportsStrictKex.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_ServerSupportsStrictKex.cs new file mode 100644 index 000000000..08a8eb7e2 --- /dev/null +++ b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_ServerSupportsStrictKex.cs @@ -0,0 +1,34 @@ +using System.Net.Sockets; + +using Microsoft.VisualStudio.TestTools.UnitTesting; + +using Renci.SshNet.Common; +using Renci.SshNet.Messages.Transport; + +namespace Renci.SshNet.Tests.Classes +{ + [TestClass] + public class SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_ServerSupportsStrictKex : SessionTest_ConnectingBase + { + protected override bool IsStrictKex + { + get + { + return true; + } + } + + protected override void MITMAttack() + { + var ignoreMessage = new IgnoreMessage(); + var ignore = ignoreMessage.GetPacket(8, null); + _ = ServerSocket.Send(ignore, 4, ignore.Length - 4, SocketFlags.None); + } + + [TestMethod] + public void ShouldThrowSshConnectionException() + { + Assert.ThrowsException(Session.Connect); + } + } +} diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_NotConnected.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_NotConnected.cs index 4bd134348..c493b6df1 100644 --- a/test/Renci.SshNet.Tests/Classes/SessionTest_NotConnected.cs +++ b/test/Renci.SshNet.Tests/Classes/SessionTest_NotConnected.cs @@ -57,7 +57,7 @@ public void IsConnectedShouldReturnFalse() } [TestMethod] - public void SendMessageShouldThrowShhConnectionException() + public void SendMessageShouldThrowSshConnectionException() { try { @@ -159,7 +159,7 @@ public void ISession_MessageListenerCompletedShouldBeSignaled() } [TestMethod] - public void ISession_SendMessageShouldThrowShhConnectionException() + public void ISession_SendMessageShouldThrowSshConnectionException() { var session = (ISession) _session; From 9097805ca2fe489fa87789444404faf00e2418f7 Mon Sep 17 00:00:00 2001 From: Scott Xu Date: Sat, 13 Apr 2024 20:04:17 +0800 Subject: [PATCH 05/14] More unit tests --- .../Classes/SessionTest_ConnectedBase.cs | 2 +- .../Classes/SessionTest_ConnectingBase.cs | 2 +- ...erKexInit_ServerDoesNotSupportStrictKex.cs | 37 ++++++++++++++++ ...ageAfterKexInit_ServerSupportsStrictKex.cs | 43 +++++++++++++++++++ 4 files changed, 82 insertions(+), 2 deletions(-) create mode 100644 test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_ServerDoesNotSupportStrictKex.cs create mode 100644 test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_ServerSupportsStrictKex.cs diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectedBase.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectedBase.cs index 0279205f7..e8e328e91 100644 --- a/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectedBase.cs +++ b/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectedBase.cs @@ -46,7 +46,7 @@ public abstract class SessionTest_ConnectedBase protected Session Session { get; private set; } protected Socket ClientSocket { get; private set; } protected Socket ServerSocket { get; private set; } - internal SshIdentification ServerIdentification { get; private set; } + protected SshIdentification ServerIdentification { get; private set; } /// /// Should the "server" wait for the client kexinit before sending its own. diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectingBase.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectingBase.cs index 7673f82e2..6627fa9e8 100644 --- a/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectingBase.cs +++ b/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectingBase.cs @@ -46,7 +46,7 @@ public abstract class SessionTest_ConnectingBase protected Session Session { get; private set; } protected Socket ClientSocket { get; private set; } protected Socket ServerSocket { get; private set; } - internal SshIdentification ServerIdentification { get; set; } + protected SshIdentification ServerIdentification { get; set; } protected virtual bool IsStrictKex { get; } [TestInitialize] diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_ServerDoesNotSupportStrictKex.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_ServerDoesNotSupportStrictKex.cs new file mode 100644 index 000000000..f6092c61d --- /dev/null +++ b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_ServerDoesNotSupportStrictKex.cs @@ -0,0 +1,37 @@ +using System.Globalization; +using System.Net.Sockets; +using System.Text; + +using Microsoft.VisualStudio.TestTools.UnitTesting; + +using Renci.SshNet.Common; +using Renci.SshNet.Messages.Transport; + +namespace Renci.SshNet.Tests.Classes +{ + [TestClass] + public class SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_ServerDoesNotSupportStrictKex : SessionTest_ConnectingBase + { + protected override void MITMAttack() + { + using var stream = new SshDataStream(0); + stream.WriteByte(1); + stream.Write("This is a debug message", Encoding.UTF8); + stream.Write(CultureInfo.CurrentCulture.Name, Encoding.UTF8); + + var debugMessage = new DebugMessage(); + debugMessage.Load(stream.ToArray()); + + var debug = debugMessage.GetPacket(8, null); + _ = ServerSocket.Send(debug, 4, debug.Length - 4, SocketFlags.None); + } + + [TestMethod] + public void ThrowsSshConnectionException() + { + // Should we allow debug message during kex in non-strict-kex mode? + // Probably better to keep this behavior as is, unless someone strongly disagree. + Assert.ThrowsException(Session.Connect); + } + } +} diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_ServerSupportsStrictKex.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_ServerSupportsStrictKex.cs new file mode 100644 index 000000000..409100d8e --- /dev/null +++ b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_ServerSupportsStrictKex.cs @@ -0,0 +1,43 @@ +using System.Globalization; +using System.Net.Sockets; +using System.Text; + +using Microsoft.VisualStudio.TestTools.UnitTesting; + +using Renci.SshNet.Common; +using Renci.SshNet.Messages.Transport; + +namespace Renci.SshNet.Tests.Classes +{ + [TestClass] + public class SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_ServerSupportsStrictKex : SessionTest_ConnectingBase + { + protected override bool IsStrictKex + { + get + { + return true; + } + } + + protected override void MITMAttack() + { + using var stream = new SshDataStream(0); + stream.WriteByte(1); + stream.Write("This is a debug message", Encoding.UTF8); + stream.Write(CultureInfo.CurrentCulture.Name, Encoding.UTF8); + + var debugMessage = new DebugMessage(); + debugMessage.Load(stream.ToArray()); + + var debug = debugMessage.GetPacket(8, null); + _ = ServerSocket.Send(debug, 4, debug.Length - 4, SocketFlags.None); + } + + [TestMethod] + public void ShouldThrowSshConnectionException() + { + Assert.ThrowsException(Session.Connect); + } + } +} From c5da652c2b7dad74e27aa142a72d81d6e80855ef Mon Sep 17 00:00:00 2001 From: Scott Xu Date: Sat, 13 Apr 2024 22:14:58 +0800 Subject: [PATCH 06/14] More unit tests --- src/Renci.SshNet/Session.cs | 2 +- .../Classes/SessionTest_ConnectingBase.cs | 57 +++++++++++++++---- ...setSequenceNumberAfterNewKeys_StrictKex.cs | 35 ++++++++++++ ...sSequenceNumberAfterNewKeys_NoStrictKex.cs | 31 ++++++++++ ...dsDebugMessageAfterKexInit_NoStrictKex.cs} | 23 ++++++-- ...endsDebugMessageAfterKexInit_StrictKex.cs} | 17 ++++-- ...sIgnoreMessageAfterKexInit_NoStrictKex.cs} | 17 +++++- ...endsIgnoreMessageAfterKexInit_StrictKex.cs | 40 +++++++++++++ ...IgnoreMessageBeforeKexInit_NoStrictKex.cs} | 18 +++--- ...ndsIgnoreMessageBeforeKexInit_StrictKex.cs | 41 +++++++++++++ 10 files changed, 249 insertions(+), 32 deletions(-) create mode 100644 test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerNotResetSequenceNumberAfterNewKeys_StrictKex.cs create mode 100644 test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerResetsSequenceNumberAfterNewKeys_NoStrictKex.cs rename test/Renci.SshNet.Tests/Classes/{SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_ServerDoesNotSupportStrictKex.cs => SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_NoStrictKex.cs} (62%) rename test/Renci.SshNet.Tests/Classes/{SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_ServerSupportsStrictKex.cs => SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_StrictKex.cs} (64%) rename test/Renci.SshNet.Tests/Classes/{SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_ServerDoesNotSupportStrictKex.cs => SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_NoStrictKex.cs} (59%) create mode 100644 test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_StrictKex.cs rename test/Renci.SshNet.Tests/Classes/{SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_ServerSupportsStrictKex.cs => SessionTest_Connecting_ServerSendsIgnoreMessageBeforeKexInit_NoStrictKex.cs} (58%) create mode 100644 test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageBeforeKexInit_StrictKex.cs diff --git a/src/Renci.SshNet/Session.cs b/src/Renci.SshNet/Session.cs index ebd8a6380..255763a7f 100644 --- a/src/Renci.SshNet/Session.cs +++ b/src/Renci.SshNet/Session.cs @@ -1452,7 +1452,7 @@ internal void OnKeyExchangeInitReceived(KeyExchangeInitMessage message) if (_inboundPacketSequence != 1) { - throw new SshConnectionException("KEXINIT was not the first packet during strict key exchange", DisconnectReason.KeyExchangeFailed); + throw new SshConnectionException("KEXINIT was not the first packet during strict key exchange.", DisconnectReason.KeyExchangeFailed); } } diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectingBase.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectingBase.cs index 6627fa9e8..51a7b9258 100644 --- a/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectingBase.cs +++ b/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectingBase.cs @@ -47,7 +47,17 @@ public abstract class SessionTest_ConnectingBase protected Socket ClientSocket { get; private set; } protected Socket ServerSocket { get; private set; } protected SshIdentification ServerIdentification { get; set; } - protected virtual bool IsStrictKex { get; } + protected virtual bool ServerSupportsStrictKex { get; } + + protected virtual bool ServerResetsSequenceAfterSendingNewKeys + { + get + { + return ServerSupportsStrictKex; + } + } + + protected uint ServerOutboundPacketSequence { get; set; } [TestInitialize] public void Setup() @@ -57,7 +67,11 @@ public void Setup() SetupMocks(); } - protected virtual void MITMAttack() + protected virtual void MITMAttackBeforeKexInit() + { + } + + protected virtual void MITMAttackAfterKexInit() { } @@ -99,8 +113,8 @@ protected virtual void SetupData() _serverEndPoint.Port, "user", new PasswordAuthenticationMethod("user", "password")) - { Timeout = TimeSpan.FromSeconds(20) }; - _keyExchangeAlgorithms = IsStrictKex ? + { Timeout = TimeSpan.FromSeconds(2000) }; + _keyExchangeAlgorithms = ServerSupportsStrictKex ? [Random.Next().ToString(CultureInfo.InvariantCulture), "kex-strict-s-v00@openssh.com"] : [Random.Next().ToString(CultureInfo.InvariantCulture)]; SessionId = new byte[10]; @@ -125,6 +139,7 @@ protected virtual void SetupData() ServerListener.Connected += socket => { ServerSocket = socket; + MITMAttackBeforeKexInit(); var keyExchangeInitMessage = new KeyExchangeInitMessage { CompressionAlgorithmsClientToServer = new string[0], @@ -140,6 +155,7 @@ protected virtual void SetupData() }; var keyExchangeInit = keyExchangeInitMessage.GetPacket(8, null); _ = ServerSocket.Send(keyExchangeInit, 4, keyExchangeInit.Length - 4, SocketFlags.None); + ServerOutboundPacketSequence++; }; ServerListener.BytesReceived += (received, socket) => { @@ -147,16 +163,34 @@ protected virtual void SetupData() if (received.Length > 5 && received[5] == 20) { - MITMAttack(); + MITMAttackAfterKexInit(); var newKeysMessage = new NewKeysMessage(); var newKeys = newKeysMessage.GetPacket(8, null); _ = ServerSocket.Send(newKeys, 4, newKeys.Length - 4, SocketFlags.None); + if (ServerResetsSequenceAfterSendingNewKeys) + { + ServerOutboundPacketSequence = 0; + } + else + { + ServerOutboundPacketSequence++; + } + if (!_authenticationStarted) { var serviceAcceptMessage = ServiceAcceptMessageBuilder.Create(ServiceName.UserAuthentication) - .Build(); - _ = ServerSocket.Send(serviceAcceptMessage, 0, serviceAcceptMessage.Length, SocketFlags.None); + .Build(ServerOutboundPacketSequence); + var hash = Abstractions.CryptoAbstraction.CreateSHA256().ComputeHash(serviceAcceptMessage); + + var packet = new byte[serviceAcceptMessage.Length - 4 + hash.Length]; + + Array.Copy(serviceAcceptMessage, 4, packet, 0, serviceAcceptMessage.Length - 4); + Array.Copy(hash, 0, packet, serviceAcceptMessage.Length - 4, hash.Length); + + _ = ServerSocket.Send(packet, 0, packet.Length, SocketFlags.None); + + ServerOutboundPacketSequence++; _authenticationStarted = true; } @@ -202,7 +236,7 @@ private void SetupMocks() .Returns((ref bool serverEtm) => { serverEtm = false; - return (HashAlgorithm) null; + return SHA256.Create(); }); _ = _keyExchangeMock.Setup(p => p.CreateClientHash(out It.Ref.IsAny)) .Returns((ref bool clientEtm) => @@ -234,13 +268,14 @@ public static ServiceAcceptMessageBuilder Create(ServiceName serviceName) return new ServiceAcceptMessageBuilder(serviceName); } - public byte[] Build() + public byte[] Build(uint sequence) { var serviceName = _serviceName.ToArray(); var target = new ServiceAcceptMessage(); - var sshDataStream = new SshDataStream(4 + 1 + 1 + 4 + serviceName.Length); - sshDataStream.Write((uint) (sshDataStream.Capacity - 4)); // packet length + var sshDataStream = new SshDataStream(4 + 4 + 1 + 1 + 4 + serviceName.Length); + sshDataStream.Write(sequence); + sshDataStream.Write((uint) (sshDataStream.Capacity - 8)); //sequence and packet length sshDataStream.WriteByte(0); // padding length sshDataStream.WriteByte(target.MessageNumber); sshDataStream.WriteBinary(serviceName); diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerNotResetSequenceNumberAfterNewKeys_StrictKex.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerNotResetSequenceNumberAfterNewKeys_StrictKex.cs new file mode 100644 index 000000000..339f9df6c --- /dev/null +++ b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerNotResetSequenceNumberAfterNewKeys_StrictKex.cs @@ -0,0 +1,35 @@ +using Microsoft.VisualStudio.TestTools.UnitTesting; + +using Renci.SshNet.Common; +using Renci.SshNet.Messages.Transport; + +namespace Renci.SshNet.Tests.Classes +{ + [TestClass] + public class SessionTest_Connecting_ServerNotResetSequenceNumberAfterNewKeys_StrictKex : SessionTest_ConnectingBase + { + protected override bool ServerSupportsStrictKex + { + get + { + return true; + } + } + + protected override bool ServerResetsSequenceAfterSendingNewKeys + { + get + { + return false; + } + } + + + [TestMethod] + public void ShouldThrowSshConnectionException() + { + var reason = Assert.ThrowsException(Session.Connect).DisconnectReason; + Assert.AreEqual(DisconnectReason.MacError, reason); + } + } +} diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerResetsSequenceNumberAfterNewKeys_NoStrictKex.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerResetsSequenceNumberAfterNewKeys_NoStrictKex.cs new file mode 100644 index 000000000..270572560 --- /dev/null +++ b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerResetsSequenceNumberAfterNewKeys_NoStrictKex.cs @@ -0,0 +1,31 @@ +using Microsoft.VisualStudio.TestTools.UnitTesting; + +namespace Renci.SshNet.Tests.Classes +{ + [TestClass] + public class SessionTest_Connecting_ServerResetsSequenceNumberAfterNewKeys_NoStrictKex : SessionTest_ConnectingBase + { + protected override bool ServerSupportsStrictKex + { + get + { + return true; + } + } + + protected override bool ServerResetsSequenceAfterSendingNewKeys + { + get + { + return true; + } + } + + + [TestMethod] + public void ShouldNotThrowException() + { + Session.Connect(); + } + } +} diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_ServerDoesNotSupportStrictKex.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_NoStrictKex.cs similarity index 62% rename from test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_ServerDoesNotSupportStrictKex.cs rename to test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_NoStrictKex.cs index f6092c61d..38d2b5efc 100644 --- a/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_ServerDoesNotSupportStrictKex.cs +++ b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_NoStrictKex.cs @@ -10,9 +10,17 @@ namespace Renci.SshNet.Tests.Classes { [TestClass] - public class SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_ServerDoesNotSupportStrictKex : SessionTest_ConnectingBase + public class SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_NoStrictKex : SessionTest_ConnectingBase { - protected override void MITMAttack() + protected override bool ServerSupportsStrictKex + { + get + { + return false; + } + } + + protected override void MITMAttackAfterKexInit() { using var stream = new SshDataStream(0); stream.WriteByte(1); @@ -21,17 +29,22 @@ protected override void MITMAttack() var debugMessage = new DebugMessage(); debugMessage.Load(stream.ToArray()); - var debug = debugMessage.GetPacket(8, null); + + // MitM sends debug message to client _ = ServerSocket.Send(debug, 4, debug.Length - 4, SocketFlags.None); + + // MitM drops server message + ServerOutboundPacketSequence++; } [TestMethod] - public void ThrowsSshConnectionException() + public void ThrowsSshException() { // Should we allow debug message during kex in non-strict-kex mode? // Probably better to keep this behavior as is, unless someone strongly disagree. - Assert.ThrowsException(Session.Connect); + var message = Assert.ThrowsException(Session.Connect).Message; + Assert.AreEqual("Message type 4 is not valid in the current context.", message); } } } diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_ServerSupportsStrictKex.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_StrictKex.cs similarity index 64% rename from test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_ServerSupportsStrictKex.cs rename to test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_StrictKex.cs index 409100d8e..8e2876270 100644 --- a/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_ServerSupportsStrictKex.cs +++ b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_StrictKex.cs @@ -10,9 +10,9 @@ namespace Renci.SshNet.Tests.Classes { [TestClass] - public class SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_ServerSupportsStrictKex : SessionTest_ConnectingBase + public class SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_StrictKex : SessionTest_ConnectingBase { - protected override bool IsStrictKex + protected override bool ServerSupportsStrictKex { get { @@ -20,7 +20,7 @@ protected override bool IsStrictKex } } - protected override void MITMAttack() + protected override void MITMAttackAfterKexInit() { using var stream = new SshDataStream(0); stream.WriteByte(1); @@ -29,15 +29,20 @@ protected override void MITMAttack() var debugMessage = new DebugMessage(); debugMessage.Load(stream.ToArray()); - var debug = debugMessage.GetPacket(8, null); + + // MitM sends debug message to client _ = ServerSocket.Send(debug, 4, debug.Length - 4, SocketFlags.None); + + // MitM drops server message + ServerOutboundPacketSequence++; } [TestMethod] - public void ShouldThrowSshConnectionException() + public void ShouldThrowSshException() { - Assert.ThrowsException(Session.Connect); + var message = Assert.ThrowsException(Session.Connect).Message; + Assert.AreEqual("Message type 4 is not valid in the current context.", message); } } } diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_ServerDoesNotSupportStrictKex.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_NoStrictKex.cs similarity index 59% rename from test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_ServerDoesNotSupportStrictKex.cs rename to test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_NoStrictKex.cs index 40f2db412..c68ffd01f 100644 --- a/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_ServerDoesNotSupportStrictKex.cs +++ b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_NoStrictKex.cs @@ -7,13 +7,26 @@ namespace Renci.SshNet.Tests.Classes { [TestClass] - public class SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_ServerDoesNotSupportStrictKex : SessionTest_ConnectingBase + public class SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_NoStrictKex : SessionTest_ConnectingBase { - protected override void MITMAttack() + protected override bool ServerSupportsStrictKex + { + get + { + return false; + } + } + + protected override void MITMAttackAfterKexInit() { var ignoreMessage = new IgnoreMessage(); var ignore = ignoreMessage.GetPacket(8, null); + + // MitM sends ignore message to client _ = ServerSocket.Send(ignore, 4, ignore.Length - 4, SocketFlags.None); + + // MitM drops server message + ServerOutboundPacketSequence++; } [TestMethod] diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_StrictKex.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_StrictKex.cs new file mode 100644 index 000000000..3b9e96bff --- /dev/null +++ b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_StrictKex.cs @@ -0,0 +1,40 @@ +using System.Net.Sockets; + +using Microsoft.VisualStudio.TestTools.UnitTesting; + +using Renci.SshNet.Common; +using Renci.SshNet.Messages.Transport; + +namespace Renci.SshNet.Tests.Classes +{ + [TestClass] + public class SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_StrictKex : SessionTest_ConnectingBase + { + protected override bool ServerSupportsStrictKex + { + get + { + return true; + } + } + + protected override void MITMAttackAfterKexInit() + { + var ignoreMessage = new IgnoreMessage(); + var ignore = ignoreMessage.GetPacket(8, null); + + // MitM sends ignore message to client + _ = ServerSocket.Send(ignore, 4, ignore.Length - 4, SocketFlags.None); + + // MitM drops server message + ServerOutboundPacketSequence++; + } + + [TestMethod] + public void ShouldThrowSshException() + { + var message = Assert.ThrowsException(Session.Connect).Message; + Assert.AreEqual("Message type 2 is not valid in the current context.", message); + } + } +} diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_ServerSupportsStrictKex.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageBeforeKexInit_NoStrictKex.cs similarity index 58% rename from test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_ServerSupportsStrictKex.cs rename to test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageBeforeKexInit_NoStrictKex.cs index 08a8eb7e2..cc92b17e5 100644 --- a/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_ServerSupportsStrictKex.cs +++ b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageBeforeKexInit_NoStrictKex.cs @@ -2,33 +2,37 @@ using Microsoft.VisualStudio.TestTools.UnitTesting; -using Renci.SshNet.Common; using Renci.SshNet.Messages.Transport; namespace Renci.SshNet.Tests.Classes { [TestClass] - public class SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_ServerSupportsStrictKex : SessionTest_ConnectingBase + public class SessionTest_Connecting_ServerSendsIgnoreMessageBeforeKexInit_NoStrictKex : SessionTest_ConnectingBase { - protected override bool IsStrictKex + protected override bool ServerSupportsStrictKex { get { - return true; + return false; } } - protected override void MITMAttack() + protected override void MITMAttackBeforeKexInit() { var ignoreMessage = new IgnoreMessage(); var ignore = ignoreMessage.GetPacket(8, null); + + // MitM sends ignore message to client _ = ServerSocket.Send(ignore, 4, ignore.Length - 4, SocketFlags.None); + + // MitM drops server message + ServerOutboundPacketSequence++; } [TestMethod] - public void ShouldThrowSshConnectionException() + public void DoesNotThrowException() { - Assert.ThrowsException(Session.Connect); + Session.Connect(); } } } diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageBeforeKexInit_StrictKex.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageBeforeKexInit_StrictKex.cs new file mode 100644 index 000000000..d8666cf1a --- /dev/null +++ b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageBeforeKexInit_StrictKex.cs @@ -0,0 +1,41 @@ +using System.Net.Sockets; + +using Microsoft.VisualStudio.TestTools.UnitTesting; + +using Renci.SshNet.Common; +using Renci.SshNet.Messages.Transport; + +namespace Renci.SshNet.Tests.Classes +{ + [TestClass] + public class SessionTest_Connecting_ServerSendsIgnoreMessageBeforeKexInit_StrictKex : SessionTest_ConnectingBase + { + protected override bool ServerSupportsStrictKex + { + get + { + return true; + } + } + + protected override void MITMAttackBeforeKexInit() + { + var ignoreMessage = new IgnoreMessage(); + var ignore = ignoreMessage.GetPacket(8, null); + + // MitM sends ignore message to client + _ = ServerSocket.Send(ignore, 4, ignore.Length - 4, SocketFlags.None); + + // MitM drops server message + ServerOutboundPacketSequence++; + } + + [TestMethod] + public void ShouldThrowSshConnectionException() + { + var exception = Assert.ThrowsException(Session.Connect); + Assert.AreEqual(DisconnectReason.KeyExchangeFailed, exception.DisconnectReason); + Assert.AreEqual("KEXINIT was not the first packet during strict key exchange.", exception.Message); + } + } +} From d0b0a084d506d4540e323c2eefccd707243475b8 Mon Sep 17 00:00:00 2001 From: Scott Xu Date: Sat, 13 Apr 2024 22:20:41 +0800 Subject: [PATCH 07/14] Correct file name --- ...necting_ServerResetsSequenceNumberAfterNewKeys_StrictKex.cs} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename test/Renci.SshNet.Tests/Classes/{SessionTest_Connecting_ServerResetsSequenceNumberAfterNewKeys_NoStrictKex.cs => SessionTest_Connecting_ServerResetsSequenceNumberAfterNewKeys_StrictKex.cs} (91%) diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerResetsSequenceNumberAfterNewKeys_NoStrictKex.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerResetsSequenceNumberAfterNewKeys_StrictKex.cs similarity index 91% rename from test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerResetsSequenceNumberAfterNewKeys_NoStrictKex.cs rename to test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerResetsSequenceNumberAfterNewKeys_StrictKex.cs index 270572560..a8f0680ba 100644 --- a/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerResetsSequenceNumberAfterNewKeys_NoStrictKex.cs +++ b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerResetsSequenceNumberAfterNewKeys_StrictKex.cs @@ -3,7 +3,7 @@ namespace Renci.SshNet.Tests.Classes { [TestClass] - public class SessionTest_Connecting_ServerResetsSequenceNumberAfterNewKeys_NoStrictKex : SessionTest_ConnectingBase + public class SessionTest_Connecting_ServerResetsSequenceNumberAfterNewKeys_StrictKex : SessionTest_ConnectingBase { protected override bool ServerSupportsStrictKex { From 35f0b9c60c7a45b2097538d721085e15eb2702ca Mon Sep 17 00:00:00 2001 From: Scott Xu Date: Sat, 13 Apr 2024 22:40:32 +0800 Subject: [PATCH 08/14] Update SessionTest_ConnectingBase.cs --- test/Renci.SshNet.Tests/Classes/SessionTest_ConnectingBase.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectingBase.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectingBase.cs index 51a7b9258..3b6e3147c 100644 --- a/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectingBase.cs +++ b/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectingBase.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Globalization; using System.Net; @@ -113,7 +113,7 @@ protected virtual void SetupData() _serverEndPoint.Port, "user", new PasswordAuthenticationMethod("user", "password")) - { Timeout = TimeSpan.FromSeconds(2000) }; + { Timeout = TimeSpan.FromSeconds(20) }; _keyExchangeAlgorithms = ServerSupportsStrictKex ? [Random.Next().ToString(CultureInfo.InvariantCulture), "kex-strict-s-v00@openssh.com"] : [Random.Next().ToString(CultureInfo.InvariantCulture)]; From f0cb43407d74d0ac318feb9a0b730042122f6a87 Mon Sep 17 00:00:00 2001 From: Scott Xu Date: Sun, 14 Apr 2024 19:36:44 +0800 Subject: [PATCH 09/14] More unit tests --- src/Renci.SshNet/Session.cs | 5 +++ .../Classes/SessionTest_Connected.cs | 26 +++++++++++++ .../Classes/SessionTest_ConnectingBase.cs | 8 ++-- ...ndsDebugMessageAfterKexInit_NoStrictKex.cs | 2 +- ...SendsDebugMessageAfterKexInit_StrictKex.cs | 2 +- ...DisconnectMessageAfterKexInit_StrictKex.cs | 39 +++++++++++++++++++ ...dsIgnoreMessageAfterKexInit_NoStrictKex.cs | 2 +- ...endsIgnoreMessageAfterKexInit_StrictKex.cs | 2 +- ...sIgnoreMessageBeforeKexInit_NoStrictKex.cs | 2 +- ...ndsIgnoreMessageBeforeKexInit_StrictKex.cs | 2 +- ...rverSendsMaxIgnoreMessagesBeforeKexInit.cs | 33 ++++++++++++++++ .../Common/AsyncSocketListener.cs | 2 +- 12 files changed, 114 insertions(+), 11 deletions(-) create mode 100644 test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsDisconnectMessageAfterKexInit_StrictKex.cs create mode 100644 test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsMaxIgnoreMessagesBeforeKexInit.cs diff --git a/src/Renci.SshNet/Session.cs b/src/Renci.SshNet/Session.cs index 255763a7f..92d6f8268 100644 --- a/src/Renci.SshNet/Session.cs +++ b/src/Renci.SshNet/Session.cs @@ -1333,6 +1333,11 @@ private Message ReceiveMessage(Socket socket) _inboundPacketSequence++; + if (_inboundPacketSequence == uint.MaxValue && _isInitialKex) + { + throw new SshConnectionException("Inbound packet sequence number is about to wrap during initial key exchange.", DisconnectReason.KeyExchangeFailed); + } + return LoadMessage(data, messagePayloadOffset, messagePayloadLength); } diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_Connected.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_Connected.cs index cdc95c12e..e15a45396 100644 --- a/test/Renci.SshNet.Tests/Classes/SessionTest_Connected.cs +++ b/test/Renci.SshNet.Tests/Classes/SessionTest_Connected.cs @@ -1,4 +1,5 @@ using System; +using System.Linq; using System.Threading; using Microsoft.VisualStudio.TestTools.UnitTesting; using Moq; @@ -30,6 +31,31 @@ public void ClientVersionIsRenciSshNet() Assert.AreEqual("SSH-2.0-Renci.SshNet.SshClient.0.0.1", Session.ClientVersion); } + [TestMethod] + public void IncludeStrictKexPseudoAlgorithmInInitKex() + { + Assert.IsTrue(ServerBytesReceivedRegister.Count > 0); + + var kexInitMessage = new KeyExchangeInitMessage(); + kexInitMessage.Load(ServerBytesReceivedRegister[0], 4 + 1 + 1, ServerBytesReceivedRegister[0].Length - 4 - 1 - 1); + Assert.IsTrue(kexInitMessage.KeyExchangeAlgorithms.Contains("kex-strict-c-v00@openssh.com")); + } + + [TestMethod] + public void ShouldNotIncludeStrictKexPseudoAlgorithmInSubsequenceKex() + { + ServerBytesReceivedRegister.Clear(); + Session.SendMessage(Session.ClientInitMessage); + + Thread.Sleep(100); + + Assert.IsTrue(ServerBytesReceivedRegister.Count > 0); + + var kexInitMessage = new KeyExchangeInitMessage(); + kexInitMessage.Load(ServerBytesReceivedRegister[0], 4 + 1 + 1, ServerBytesReceivedRegister[0].Length - 4 - 1 - 1); + Assert.IsFalse(kexInitMessage.KeyExchangeAlgorithms.Contains("kex-strict-c-v00@openssh.com")); + } + [TestMethod] public void ConnectionInfoShouldReturnConnectionInfoPassedThroughConstructor() { diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectingBase.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectingBase.cs index 3b6e3147c..c9a83068b 100644 --- a/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectingBase.cs +++ b/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectingBase.cs @@ -67,11 +67,11 @@ public void Setup() SetupMocks(); } - protected virtual void MITMAttackBeforeKexInit() + protected virtual void ActionBeforeKexInit() { } - protected virtual void MITMAttackAfterKexInit() + protected virtual void ActionAfterKexInit() { } @@ -139,7 +139,7 @@ protected virtual void SetupData() ServerListener.Connected += socket => { ServerSocket = socket; - MITMAttackBeforeKexInit(); + ActionBeforeKexInit(); var keyExchangeInitMessage = new KeyExchangeInitMessage { CompressionAlgorithmsClientToServer = new string[0], @@ -163,7 +163,7 @@ protected virtual void SetupData() if (received.Length > 5 && received[5] == 20) { - MITMAttackAfterKexInit(); + ActionAfterKexInit(); var newKeysMessage = new NewKeysMessage(); var newKeys = newKeysMessage.GetPacket(8, null); _ = ServerSocket.Send(newKeys, 4, newKeys.Length - 4, SocketFlags.None); diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_NoStrictKex.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_NoStrictKex.cs index 38d2b5efc..4056d49ad 100644 --- a/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_NoStrictKex.cs +++ b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_NoStrictKex.cs @@ -20,7 +20,7 @@ protected override bool ServerSupportsStrictKex } } - protected override void MITMAttackAfterKexInit() + protected override void ActionAfterKexInit() { using var stream = new SshDataStream(0); stream.WriteByte(1); diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_StrictKex.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_StrictKex.cs index 8e2876270..6fbdcceaa 100644 --- a/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_StrictKex.cs +++ b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_StrictKex.cs @@ -20,7 +20,7 @@ protected override bool ServerSupportsStrictKex } } - protected override void MITMAttackAfterKexInit() + protected override void ActionAfterKexInit() { using var stream = new SshDataStream(0); stream.WriteByte(1); diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsDisconnectMessageAfterKexInit_StrictKex.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsDisconnectMessageAfterKexInit_StrictKex.cs new file mode 100644 index 000000000..989d43e56 --- /dev/null +++ b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsDisconnectMessageAfterKexInit_StrictKex.cs @@ -0,0 +1,39 @@ +using System.Net.Sockets; + +using Microsoft.VisualStudio.TestTools.UnitTesting; + +using Renci.SshNet.Common; +using Renci.SshNet.Messages.Transport; + +namespace Renci.SshNet.Tests.Classes +{ + [TestClass] + public class SessionTest_Connecting_ServerSendsDisconnectMessageAfterKexInit_StrictKex : SessionTest_ConnectingBase + { + protected override bool ServerSupportsStrictKex + { + get + { + return true; + } + } + + protected override void ActionAfterKexInit() + { + var disconnectMessage = new DisconnectMessage(DisconnectReason.TooManyConnections, "too many connections"); + var disconnect = disconnectMessage.GetPacket(8, null); + + // Server sends disconnect message to client + _ = ServerSocket.Send(disconnect, 4, disconnect.Length - 4, SocketFlags.None); + + ServerOutboundPacketSequence++; + } + + [TestMethod] + public void DisconnectIsAllowedDuringStrictKex() + { + var exception = Assert.ThrowsException(Session.Connect); + Assert.AreEqual(DisconnectReason.TooManyConnections, exception.DisconnectReason); + } + } +} diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_NoStrictKex.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_NoStrictKex.cs index c68ffd01f..f20d81d8a 100644 --- a/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_NoStrictKex.cs +++ b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_NoStrictKex.cs @@ -17,7 +17,7 @@ protected override bool ServerSupportsStrictKex } } - protected override void MITMAttackAfterKexInit() + protected override void ActionAfterKexInit() { var ignoreMessage = new IgnoreMessage(); var ignore = ignoreMessage.GetPacket(8, null); diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_StrictKex.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_StrictKex.cs index 3b9e96bff..0179c0eb0 100644 --- a/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_StrictKex.cs +++ b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageAfterKexInit_StrictKex.cs @@ -18,7 +18,7 @@ protected override bool ServerSupportsStrictKex } } - protected override void MITMAttackAfterKexInit() + protected override void ActionAfterKexInit() { var ignoreMessage = new IgnoreMessage(); var ignore = ignoreMessage.GetPacket(8, null); diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageBeforeKexInit_NoStrictKex.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageBeforeKexInit_NoStrictKex.cs index cc92b17e5..c85d925b7 100644 --- a/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageBeforeKexInit_NoStrictKex.cs +++ b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageBeforeKexInit_NoStrictKex.cs @@ -17,7 +17,7 @@ protected override bool ServerSupportsStrictKex } } - protected override void MITMAttackBeforeKexInit() + protected override void ActionBeforeKexInit() { var ignoreMessage = new IgnoreMessage(); var ignore = ignoreMessage.GetPacket(8, null); diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageBeforeKexInit_StrictKex.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageBeforeKexInit_StrictKex.cs index d8666cf1a..53dde0b3c 100644 --- a/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageBeforeKexInit_StrictKex.cs +++ b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsIgnoreMessageBeforeKexInit_StrictKex.cs @@ -18,7 +18,7 @@ protected override bool ServerSupportsStrictKex } } - protected override void MITMAttackBeforeKexInit() + protected override void ActionBeforeKexInit() { var ignoreMessage = new IgnoreMessage(); var ignore = ignoreMessage.GetPacket(8, null); diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsMaxIgnoreMessagesBeforeKexInit.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsMaxIgnoreMessagesBeforeKexInit.cs new file mode 100644 index 000000000..8c6cfd60d --- /dev/null +++ b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsMaxIgnoreMessagesBeforeKexInit.cs @@ -0,0 +1,33 @@ +using System.Net.Sockets; + +using Microsoft.VisualStudio.TestTools.UnitTesting; + +using Renci.SshNet.Common; +using Renci.SshNet.Messages.Transport; + +namespace Renci.SshNet.Tests.Classes +{ + [TestClass] + public class SessionTest_Connecting_ServerSendsMaxIgnoreMessagesBeforeKexInit : SessionTest_ConnectingBase + { + protected override void ActionBeforeKexInit() + { + var ignoreMessage = new IgnoreMessage(); + var ignore = ignoreMessage.GetPacket(8, null); + for (uint i = 0; i < uint.MaxValue; i++) + { + // MitM sends ignore message to client + _ = ServerSocket.Send(ignore, 4, ignore.Length - 4, SocketFlags.None); + } + } + + [TestMethod] + [Ignore("It takes hours to send 4294967295 ignore messages.")] + public void ShouldThrowSshConnectionException() + { + var exception = Assert.ThrowsException(Session.Connect); + Assert.AreEqual(DisconnectReason.KeyExchangeFailed, exception.DisconnectReason); + Assert.AreEqual("Inbound packet sequence number is about to wrap during initial key exchange.", exception.Message); + } + } +} diff --git a/test/Renci.SshNet.Tests/Common/AsyncSocketListener.cs b/test/Renci.SshNet.Tests/Common/AsyncSocketListener.cs index 0f7dac81f..59317f44f 100644 --- a/test/Renci.SshNet.Tests/Common/AsyncSocketListener.cs +++ b/test/Renci.SshNet.Tests/Common/AsyncSocketListener.cs @@ -385,7 +385,7 @@ private class SocketStateObject { public Socket Socket { get; private set; } - public readonly byte[] Buffer = new byte[1024]; + public readonly byte[] Buffer = new byte[2048]; public SocketStateObject(Socket handler) { From 38b5b9fb53e114f2c849b1cd1a419de73b450ae8 Mon Sep 17 00:00:00 2001 From: Scott Xu Date: Wed, 17 Apr 2024 20:58:27 +0800 Subject: [PATCH 10/14] Delete SessionTest_Connecting_ServerSendsMaxIgnoreMessagesBeforeKexInit.cs --- ...rverSendsMaxIgnoreMessagesBeforeKexInit.cs | 33 ------------------- 1 file changed, 33 deletions(-) delete mode 100644 test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsMaxIgnoreMessagesBeforeKexInit.cs diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsMaxIgnoreMessagesBeforeKexInit.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsMaxIgnoreMessagesBeforeKexInit.cs deleted file mode 100644 index 8c6cfd60d..000000000 --- a/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsMaxIgnoreMessagesBeforeKexInit.cs +++ /dev/null @@ -1,33 +0,0 @@ -using System.Net.Sockets; - -using Microsoft.VisualStudio.TestTools.UnitTesting; - -using Renci.SshNet.Common; -using Renci.SshNet.Messages.Transport; - -namespace Renci.SshNet.Tests.Classes -{ - [TestClass] - public class SessionTest_Connecting_ServerSendsMaxIgnoreMessagesBeforeKexInit : SessionTest_ConnectingBase - { - protected override void ActionBeforeKexInit() - { - var ignoreMessage = new IgnoreMessage(); - var ignore = ignoreMessage.GetPacket(8, null); - for (uint i = 0; i < uint.MaxValue; i++) - { - // MitM sends ignore message to client - _ = ServerSocket.Send(ignore, 4, ignore.Length - 4, SocketFlags.None); - } - } - - [TestMethod] - [Ignore("It takes hours to send 4294967295 ignore messages.")] - public void ShouldThrowSshConnectionException() - { - var exception = Assert.ThrowsException(Session.Connect); - Assert.AreEqual(DisconnectReason.KeyExchangeFailed, exception.DisconnectReason); - Assert.AreEqual("Inbound packet sequence number is about to wrap during initial key exchange.", exception.Message); - } - } -} From 5b1421e0102273f0b3980409381c464a366bc972 Mon Sep 17 00:00:00 2001 From: Scott Xu Date: Wed, 17 Apr 2024 21:09:49 +0800 Subject: [PATCH 11/14] Add a comment about throwing exception when inbound sequence number is about to wrap during init kex. --- src/Renci.SshNet/Session.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/Renci.SshNet/Session.cs b/src/Renci.SshNet/Session.cs index 92d6f8268..3bba61cc3 100644 --- a/src/Renci.SshNet/Session.cs +++ b/src/Renci.SshNet/Session.cs @@ -1333,6 +1333,8 @@ private Message ReceiveMessage(Socket socket) _inboundPacketSequence++; + // The below code mirrors from https://github.com/openssh/openssh-portable/commit/1edb00c58f8a6875fad6a497aa2bacf37f9e6cd5 + // It ensures the integrity of key exchange process. if (_inboundPacketSequence == uint.MaxValue && _isInitialKex) { throw new SshConnectionException("Inbound packet sequence number is about to wrap during initial key exchange.", DisconnectReason.KeyExchangeFailed); From 9cd2653f95835c7fbade33a5003062fd3144ac26 Mon Sep 17 00:00:00 2001 From: Scott Xu Date: Fri, 19 Apr 2024 09:32:09 +0800 Subject: [PATCH 12/14] Delete SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_NoStrictKex.cs --- ...ndsDebugMessageAfterKexInit_NoStrictKex.cs | 50 ------------------- 1 file changed, 50 deletions(-) delete mode 100644 test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_NoStrictKex.cs diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_NoStrictKex.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_NoStrictKex.cs deleted file mode 100644 index 4056d49ad..000000000 --- a/test/Renci.SshNet.Tests/Classes/SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_NoStrictKex.cs +++ /dev/null @@ -1,50 +0,0 @@ -using System.Globalization; -using System.Net.Sockets; -using System.Text; - -using Microsoft.VisualStudio.TestTools.UnitTesting; - -using Renci.SshNet.Common; -using Renci.SshNet.Messages.Transport; - -namespace Renci.SshNet.Tests.Classes -{ - [TestClass] - public class SessionTest_Connecting_ServerSendsDebugMessageAfterKexInit_NoStrictKex : SessionTest_ConnectingBase - { - protected override bool ServerSupportsStrictKex - { - get - { - return false; - } - } - - protected override void ActionAfterKexInit() - { - using var stream = new SshDataStream(0); - stream.WriteByte(1); - stream.Write("This is a debug message", Encoding.UTF8); - stream.Write(CultureInfo.CurrentCulture.Name, Encoding.UTF8); - - var debugMessage = new DebugMessage(); - debugMessage.Load(stream.ToArray()); - var debug = debugMessage.GetPacket(8, null); - - // MitM sends debug message to client - _ = ServerSocket.Send(debug, 4, debug.Length - 4, SocketFlags.None); - - // MitM drops server message - ServerOutboundPacketSequence++; - } - - [TestMethod] - public void ThrowsSshException() - { - // Should we allow debug message during kex in non-strict-kex mode? - // Probably better to keep this behavior as is, unless someone strongly disagree. - var message = Assert.ThrowsException(Session.Connect).Message; - Assert.AreEqual("Message type 4 is not valid in the current context.", message); - } - } -} From ebc555e2f61d4fb1bef51bf224eca0f81f3d8c18 Mon Sep 17 00:00:00 2001 From: Scott Xu Date: Fri, 19 Apr 2024 19:18:20 +0800 Subject: [PATCH 13/14] Fix build --- .../Classes/SessionTest_ConnectingBase.cs | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectingBase.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectingBase.cs index c9a83068b..f34634d7b 100644 --- a/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectingBase.cs +++ b/test/Renci.SshNet.Tests/Classes/SessionTest_ConnectingBase.cs @@ -228,10 +228,18 @@ private void SetupMocks() _ = _keyExchangeMock.Setup(p => p.Start(Session, It.IsAny(), false)); _ = _keyExchangeMock.Setup(p => p.ExchangeHash) .Returns(SessionId); - _ = _keyExchangeMock.Setup(p => p.CreateServerCipher()) - .Returns((Cipher) null); - _ = _keyExchangeMock.Setup(p => p.CreateClientCipher()) - .Returns((Cipher) null); + _ = _keyExchangeMock.Setup(p => p.CreateServerCipher(out It.Ref.IsAny)) + .Returns((ref bool serverAead) => + { + serverAead = false; + return (Cipher) null; + }); + _ = _keyExchangeMock.Setup(p => p.CreateClientCipher(out It.Ref.IsAny)) + .Returns((ref bool clientAead) => + { + clientAead = false; + return (Cipher) null; + }); _ = _keyExchangeMock.Setup(p => p.CreateServerHash(out It.Ref.IsAny)) .Returns((ref bool serverEtm) => { From a5fdc4f4c47d7b25e1261262ec4ecafb9a6f9264 Mon Sep 17 00:00:00 2001 From: Rob Hague Date: Wed, 24 Apr 2024 22:36:08 +0200 Subject: [PATCH 14/14] Update test/Renci.SshNet.Tests/Classes/SessionTest_Connected.cs --- test/Renci.SshNet.Tests/Classes/SessionTest_Connected.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/Renci.SshNet.Tests/Classes/SessionTest_Connected.cs b/test/Renci.SshNet.Tests/Classes/SessionTest_Connected.cs index e15a45396..de7a89d6b 100644 --- a/test/Renci.SshNet.Tests/Classes/SessionTest_Connected.cs +++ b/test/Renci.SshNet.Tests/Classes/SessionTest_Connected.cs @@ -42,7 +42,7 @@ public void IncludeStrictKexPseudoAlgorithmInInitKex() } [TestMethod] - public void ShouldNotIncludeStrictKexPseudoAlgorithmInSubsequenceKex() + public void ShouldNotIncludeStrictKexPseudoAlgorithmInSubsequentKex() { ServerBytesReceivedRegister.Clear(); Session.SendMessage(Session.ClientInitMessage);