Skip to content

Fix potential side-channel timing attack issue #1375

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Apr 24, 2024
Merged

Fix potential side-channel timing attack issue #1375

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7d869d3
Fix potential side-channel timing attack issue
scott-xu Apr 16, 2024
0c49466
Eliminate the extra allocation
scott-xu Apr 16, 2024
908f281
Remove serverHash variable
scott-xu Apr 17, 2024
872c8c4
Merge branch 'develop' into timing-attack
Rob-Hague Apr 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/Renci.SshNet/Security/Cryptography/CipherDigitalSignature.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

/// <summary>
Expand Down
20 changes: 10 additions & 10 deletions src/Renci.SshNet/Session.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1291,11 +1291,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);

// 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);
}
Expand All @@ -1319,11 +1319,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);

// 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);
}
Expand Down