From ce451293083ce4689f16e1bc88f282fc27bf36f5 Mon Sep 17 00:00:00 2001 From: Scott Xu Date: Thu, 25 Apr 2024 05:49:20 +0800 Subject: [PATCH] Fix potential side-channel timing attack issue (#1375) * Fix potential side-channel timing attack issue * Eliminate the extra allocation * Remove serverHash variable --------- Co-authored-by: Rob Hague --- .../Cryptography/CipherDigitalSignature.cs | 6 +++++- src/Renci.SshNet/Session.cs | 20 +++++++++---------- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/src/Renci.SshNet/Security/Cryptography/CipherDigitalSignature.cs b/src/Renci.SshNet/Security/Cryptography/CipherDigitalSignature.cs index e61b5d65..0a83610d 100644 --- a/src/Renci.SshNet/Security/Cryptography/CipherDigitalSignature.cs +++ b/src/Renci.SshNet/Security/Cryptography/CipherDigitalSignature.cs @@ -40,7 +40,11 @@ namespace Renci.SshNet.Security.Cryptography 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 3712ea76..6a2c81a4 100644 --- a/src/Renci.SshNet/Session.cs +++ b/src/Renci.SshNet/Session.cs @@ -1291,11 +1291,11 @@ namespace Renci.SshNet if (_serverMac != null && _serverEtm) { 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, new ReadOnlySpan(data, data.Length - serverMacLength, serverMacLength))) +#else + if (!Security.Chaos.NaCl.CryptoBytes.ConstantTimeEquals(clientHash, 0, data, data.Length - serverMacLength, serverMacLength)) +#endif { throw new SshConnectionException("MAC error", DisconnectReason.MacError); } @@ -1319,11 +1319,11 @@ namespace Renci.SshNet if (_serverMac != null && !_serverEtm) { 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, new ReadOnlySpan(data, data.Length - serverMacLength, serverMacLength))) +#else + if (!Security.Chaos.NaCl.CryptoBytes.ConstantTimeEquals(clientHash, 0, data, data.Length - serverMacLength, serverMacLength)) +#endif { throw new SshConnectionException("MAC error", DisconnectReason.MacError); }