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 <rob.hague00@gmail.com>
This commit is contained in:
Scott Xu
2024-04-25 05:49:20 +08:00
committed by GitHub
parent 94397d47ed
commit ce45129308
2 changed files with 15 additions and 11 deletions
@@ -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
}
/// <summary>
+10 -10
View File
@@ -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<byte>(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<byte>(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);
}