Skip to content

Commit ce45129

Browse files
scott-xuRob-Hague
andauthored
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 <[email protected]>
1 parent 94397d4 commit ce45129

File tree

2 files changed

+15
-11
lines changed

2 files changed

+15
-11
lines changed

src/Renci.SshNet/Security/Cryptography/CipherDigitalSignature.cs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,11 @@ public override bool Verify(byte[] input, byte[] signature)
4040
var encryptedSignature = _cipher.Decrypt(signature);
4141
var hashData = Hash(input);
4242
var expected = DerEncode(hashData);
43-
return expected.IsEqualTo(encryptedSignature);
43+
#if NETSTANDARD2_1_OR_GREATER || NETCOREAPP2_1_OR_GREATER
44+
return System.Security.Cryptography.CryptographicOperations.FixedTimeEquals(expected, encryptedSignature);
45+
#else
46+
return Chaos.NaCl.CryptoBytes.ConstantTimeEquals(expected, encryptedSignature);
47+
#endif
4448
}
4549

4650
/// <summary>

src/Renci.SshNet/Session.cs

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1291,11 +1291,11 @@ private Message ReceiveMessage(Socket socket)
12911291
if (_serverMac != null && _serverEtm)
12921292
{
12931293
var clientHash = _serverMac.ComputeHash(data, 0, data.Length - serverMacLength);
1294-
var serverHash = data.Take(data.Length - serverMacLength, serverMacLength);
1295-
1296-
// TODO Add IsEqualTo overload that takes left+right index and number of bytes to compare.
1297-
// TODO That way we can eliminate the extra allocation of the Take above.
1298-
if (!serverHash.IsEqualTo(clientHash))
1294+
#if NETSTANDARD2_1_OR_GREATER || NETCOREAPP2_1_OR_GREATER
1295+
if (!CryptographicOperations.FixedTimeEquals(clientHash, new ReadOnlySpan<byte>(data, data.Length - serverMacLength, serverMacLength)))
1296+
#else
1297+
if (!Security.Chaos.NaCl.CryptoBytes.ConstantTimeEquals(clientHash, 0, data, data.Length - serverMacLength, serverMacLength))
1298+
#endif
12991299
{
13001300
throw new SshConnectionException("MAC error", DisconnectReason.MacError);
13011301
}
@@ -1319,11 +1319,11 @@ private Message ReceiveMessage(Socket socket)
13191319
if (_serverMac != null && !_serverEtm)
13201320
{
13211321
var clientHash = _serverMac.ComputeHash(data, 0, data.Length - serverMacLength);
1322-
var serverHash = data.Take(data.Length - serverMacLength, serverMacLength);
1323-
1324-
// TODO Add IsEqualTo overload that takes left+right index and number of bytes to compare.
1325-
// TODO That way we can eliminate the extra allocation of the Take above.
1326-
if (!serverHash.IsEqualTo(clientHash))
1322+
#if NETSTANDARD2_1_OR_GREATER || NETCOREAPP2_1_OR_GREATER
1323+
if (!CryptographicOperations.FixedTimeEquals(clientHash, new ReadOnlySpan<byte>(data, data.Length - serverMacLength, serverMacLength)))
1324+
#else
1325+
if (!Security.Chaos.NaCl.CryptoBytes.ConstantTimeEquals(clientHash, 0, data, data.Length - serverMacLength, serverMacLength))
1326+
#endif
13271327
{
13281328
throw new SshConnectionException("MAC error", DisconnectReason.MacError);
13291329
}

0 commit comments

Comments
 (0)