From 7d869d3633ee8986669468330c220a3c03c521a0 Mon Sep 17 00:00:00 2001 From: Scott Xu Date: Tue, 16 Apr 2024 22:49:34 +0800 Subject: [PATCH 1/3] Fix potential side-channel timing attack issue --- .../Cryptography/CipherDigitalSignature.cs | 6 +++++- src/Renci.SshNet/Session.cs | 16 ++++++++++------ 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/src/Renci.SshNet/Security/Cryptography/CipherDigitalSignature.cs b/src/Renci.SshNet/Security/Cryptography/CipherDigitalSignature.cs index e61b5d655..0a83610d5 100644 --- a/src/Renci.SshNet/Security/Cryptography/CipherDigitalSignature.cs +++ b/src/Renci.SshNet/Security/Cryptography/CipherDigitalSignature.cs @@ -40,7 +40,11 @@ public override bool Verify(byte[] input, byte[] signature) var encryptedSignature = _cipher.Decrypt(signature); var hashData = Hash(input); var expected = DerEncode(hashData); - return expected.IsEqualTo(encryptedSignature); +#if NETSTANDARD2_1_OR_GREATER || NETCOREAPP2_1_OR_GREATER + return System.Security.Cryptography.CryptographicOperations.FixedTimeEquals(expected, encryptedSignature); +#else + return Chaos.NaCl.CryptoBytes.ConstantTimeEquals(expected, encryptedSignature); +#endif } /// diff --git a/src/Renci.SshNet/Session.cs b/src/Renci.SshNet/Session.cs index 0c067ec2d..760bc7ece 100644 --- a/src/Renci.SshNet/Session.cs +++ b/src/Renci.SshNet/Session.cs @@ -1286,9 +1286,11 @@ private Message ReceiveMessage(Socket socket) var clientHash = _serverMac.ComputeHash(data, 0, data.Length - serverMacLength); var serverHash = data.Take(data.Length - serverMacLength, serverMacLength); - // TODO Add IsEqualTo overload that takes left+right index and number of bytes to compare. - // TODO That way we can eliminate the extra allocation of the Take above. - if (!serverHash.IsEqualTo(clientHash)) +#if NETSTANDARD2_1_OR_GREATER || NETCOREAPP2_1_OR_GREATER + if (!CryptographicOperations.FixedTimeEquals(clientHash, serverHash)) +#else + if (!Security.Chaos.NaCl.CryptoBytes.ConstantTimeEquals(clientHash, serverHash)) +#endif { throw new SshConnectionException("MAC error", DisconnectReason.MacError); } @@ -1314,9 +1316,11 @@ private Message ReceiveMessage(Socket socket) var clientHash = _serverMac.ComputeHash(data, 0, data.Length - serverMacLength); var serverHash = data.Take(data.Length - serverMacLength, serverMacLength); - // TODO Add IsEqualTo overload that takes left+right index and number of bytes to compare. - // TODO That way we can eliminate the extra allocation of the Take above. - if (!serverHash.IsEqualTo(clientHash)) +#if NETSTANDARD2_1_OR_GREATER || NETCOREAPP2_1_OR_GREATER + if (!CryptographicOperations.FixedTimeEquals(clientHash, serverHash)) +#else + if (!Security.Chaos.NaCl.CryptoBytes.ConstantTimeEquals(clientHash, serverHash)) +#endif { throw new SshConnectionException("MAC error", DisconnectReason.MacError); } From 0c4946623e68ab49576aafa993aff09abb804b7f Mon Sep 17 00:00:00 2001 From: Scott Xu Date: Tue, 16 Apr 2024 23:30:26 +0800 Subject: [PATCH 2/3] Eliminate the extra allocation --- src/Renci.SshNet/Session.cs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/Renci.SshNet/Session.cs b/src/Renci.SshNet/Session.cs index 760bc7ece..691b08203 100644 --- a/src/Renci.SshNet/Session.cs +++ b/src/Renci.SshNet/Session.cs @@ -1284,12 +1284,11 @@ private Message ReceiveMessage(Socket socket) if (_serverMac != null && _serverEtm) { var clientHash = _serverMac.ComputeHash(data, 0, data.Length - serverMacLength); - var serverHash = data.Take(data.Length - serverMacLength, serverMacLength); #if NETSTANDARD2_1_OR_GREATER || NETCOREAPP2_1_OR_GREATER - if (!CryptographicOperations.FixedTimeEquals(clientHash, serverHash)) + if (!CryptographicOperations.FixedTimeEquals(clientHash, new ReadOnlySpan(data, data.Length - serverMacLength, serverMacLength))) #else - if (!Security.Chaos.NaCl.CryptoBytes.ConstantTimeEquals(clientHash, serverHash)) + if (!Security.Chaos.NaCl.CryptoBytes.ConstantTimeEquals(clientHash, 0, data, data.Length - serverMacLength, serverMacLength)) #endif { throw new SshConnectionException("MAC error", DisconnectReason.MacError); @@ -1317,9 +1316,9 @@ private Message ReceiveMessage(Socket socket) var serverHash = data.Take(data.Length - serverMacLength, serverMacLength); #if NETSTANDARD2_1_OR_GREATER || NETCOREAPP2_1_OR_GREATER - if (!CryptographicOperations.FixedTimeEquals(clientHash, serverHash)) + if (!CryptographicOperations.FixedTimeEquals(clientHash, new ReadOnlySpan(data, data.Length - serverMacLength, serverMacLength))) #else - if (!Security.Chaos.NaCl.CryptoBytes.ConstantTimeEquals(clientHash, serverHash)) + if (!Security.Chaos.NaCl.CryptoBytes.ConstantTimeEquals(clientHash, 0, data, data.Length - serverMacLength, serverMacLength)) #endif { throw new SshConnectionException("MAC error", DisconnectReason.MacError); From 908f28144a5049e77bc9a4ac7ac4963ba1b3f5cb Mon Sep 17 00:00:00 2001 From: Scott Xu Date: Wed, 17 Apr 2024 21:15:59 +0800 Subject: [PATCH 3/3] Remove serverHash variable --- src/Renci.SshNet/Session.cs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/Renci.SshNet/Session.cs b/src/Renci.SshNet/Session.cs index 691b08203..a62f090c0 100644 --- a/src/Renci.SshNet/Session.cs +++ b/src/Renci.SshNet/Session.cs @@ -1284,7 +1284,6 @@ private Message ReceiveMessage(Socket socket) if (_serverMac != null && _serverEtm) { var clientHash = _serverMac.ComputeHash(data, 0, data.Length - serverMacLength); - #if NETSTANDARD2_1_OR_GREATER || NETCOREAPP2_1_OR_GREATER if (!CryptographicOperations.FixedTimeEquals(clientHash, new ReadOnlySpan(data, data.Length - serverMacLength, serverMacLength))) #else @@ -1313,8 +1312,6 @@ private Message ReceiveMessage(Socket socket) if (_serverMac != null && !_serverEtm) { var clientHash = _serverMac.ComputeHash(data, 0, data.Length - serverMacLength); - var serverHash = data.Take(data.Length - serverMacLength, serverMacLength); - #if NETSTANDARD2_1_OR_GREATER || NETCOREAPP2_1_OR_GREATER if (!CryptographicOperations.FixedTimeEquals(clientHash, new ReadOnlySpan(data, data.Length - serverMacLength, serverMacLength))) #else