-
Notifications
You must be signed in to change notification settings - Fork 1k
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.
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
fix: sensitive data compare should use constant time compare to avoid timing attack #3508
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand this timing attack. It looks correct and the XOR is quite efficient.
Although it does not look much critical to me in this specific case.
side-channel attack is sort of hot in the research area~~~~ but not sure how industry see it. |
return false; | ||
|
||
if (left.Length != right.Length) | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is no constant, maybe we should do a dummy for for the maxLength string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is no constant, maybe we should do a dummy for for the maxLength string
Implementations in Rust subtle
and Golang crypto/subtle
are similar to this one. Length is not sensitive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is no constant, maybe we should do a dummy for for the maxLength string
Implementations in Rust
subtle
and Golangcrypto/subtle
are similar to this one. Length is not sensitive.
Then these implementations are wrong 🙃
/// <param name="left">The left <see cref="string"/>.</param> | ||
/// <param name="right">The right <see cref="string"/>.</param> | ||
/// <returns>True if the two <see cref="string"/>s are equal, false otherwise.</returns> | ||
public static bool ConstantTimeEquals(this string left, string right) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we please not introduce yet another constant time comparison function? This was discussed already in #3472 (review), there are standard solutions to this that should be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we please not introduce yet another constant time comparison function? This was discussed already in #3472 (review), there are standard solutions to this that should be used.
The implementation in ConstantTimeUtility
has some limits.
For example:
public static bool ConstantTimeEq<T>(in T a, in T b) **where T : unmanaged**
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
string string1 = "hello";
string string2 = "hello";
// Convert to bytes
byte[] bytes1 = Encoding.UTF8.GetBytes(string1);
byte[] bytes2 = Encoding.UTF8.GetBytes(string2);
// Use FixedTimeEquals
bool result = CryptographicOperations.FixedTimeEquals(bytes1, bytes2);
That's not working?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
string string1 = "hello"; string string2 = "hello"; // Convert to bytes byte[] bytes1 = Encoding.UTF8.GetBytes(string1); byte[] bytes2 = Encoding.UTF8.GetBytes(string2); // Use FixedTimeEquals bool result = CryptographicOperations.FixedTimeEquals(bytes1, bytes2);That's not working?
This needs extra data copies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First rule of cryptography, don't write your own cryptography. I'm with @roman-khimov, better to use the official one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First rule of cryptography, don't write your own cryptography. I'm with @roman-khimov, better to use the official one
Has changed to CryptographicOperations.FixedTimeEquals
, and also eliminated the two memory allocations(Encoding.UTF8.GetString(Convert.FromBase64String(reqauth.Replace("Basic ", "").Trim()));
and authstring.Split
), so it should have better performance.
Description
RpcServer.CheckAuth
use direct compare to determine twostring
s equal or not, but sensitive data compare should use constant time compare to avoid timing attackType of change
Checklist: