-
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
Update ConstantTimeUtility.cs #3472
Conversation
- Fix typo
Can you add a test? |
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 believe that the change is correct because this line is intended to compare corresponding bytes from a_bytes
and b_bytes
and accumulate any differences into the f
variable. The use of XOR (^)
will result in a non-zero value for any byte that differs between a_bytes
and b_bytes
, and the f
variable accumulates these differences. If f
ends up being 0
, it means the two byte arrays are equal.
In this sense, without this change we just compare each byte of a_bytes
with itself, which would always result in 0, effectively rendering the check meaningless and always producing a result of true for equality, which looks like to be incorrect.
Please double check before merging.
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.
During test it would be good to ensure that the ConstantTimeEq
method behaves as expected in all cases, particularly for various data types.
If we can design some UTs based on that it would be great.
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.
But why do we have ConstantTimeEq
at all in our code? Isn't something like this provided by standard libraries? Like https://learn.microsoft.com/en-us/dotnet/api/system.security.cryptography.cryptographicoperations.fixedtimeequals?view=net-8.0?
The change seems good for me |
it was converted from rust lib from file to file, but i agree with you, @cschuchardt88 @vncoelho @shargon do you think we should adopt dotnet lib method? |
Let's fix this, and create an issue for the native one |
At the beginning, I was thinking about how things like this passed tests, so I checked the context. :) So first, it is not a transfer from Rust. If we are talking about https://github.com/zkcrypto/bls12_381, the codes are different. Second, this method is checking two same-type inputs, so it skips the length check. Third, is this line really used? If neo/src/Neo.Cryptography.BLS12_381/ConstantTimeUtility.cs Lines 28 to 29 in 90fbbe1
If what we need is a universal helper, then we should use std lib if there is one. If what we need is a crypto thing, then the usage of |
It's true in case if
So technically it's possible that both
Agree, so my question isn't relevant anymore. And given this fact the fix presented in the PR is valid.
I think that somehow it just happened than no one tried to call this helper for a large tuple or struct. Whereas all int-like cases are handled correctly by this helper, thanks to the first part of this method. Though it would be good to add a unit-test that calls this method not only for int-ilke types, but also for big unmanaged tuple or struct. |
OK, I'm wrong about it, and @txhsl is right,
|
This PR patch is correct. Example: a_bytes = // is a struct of ulong(s) (Like UInt160, UInt256)
b_bytes = // is a struct of ulong(s) (Like UInt160, UInt256)
a_u64 = // is an array of ulong(s)
b_u64 = // is an array of ulong(s) |
Description
Typo in ConstantTimeEq meant method only worked as intended for inputs qword aligned (8 byte-aligned).
Type of change
Checklist: