-
Notifications
You must be signed in to change notification settings - Fork 84
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
Correct EcPoint
const time comparison and better comments and namings
#345
Conversation
Taowyoo
commented
Feb 8, 2024
- Better comments
- Better naming
- Better comments - Better naming
/// The implementation is based on C mbedtls function [`mbedtls_ecp_point_cmp`]. | ||
/// This new implementation ensures there is no shortcut when any of `x, y ,z` fields of two points is not equal. | ||
/// | ||
/// [`mbedtls_ecp_point_cmp`]: https://github.com/fortanix/rust-mbedtls/blob/main/mbedtls-sys/vendor/library/ecp.c#L809-L825 |
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.
One TBD here:
The mpi_cmp_mpi
is also not const time.
Should I use other mpi const time function to implement this?
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.
Such as this:
/**
* \brief Check if an MPI is less than the other in constant time.
*
* \param X The left-hand MPI. This must point to an initialized MPI
* with the same allocated length as Y.
* \param Y The right-hand MPI. This must point to an initialized MPI
* with the same allocated length as X.
* \param ret The result of the comparison:
* \c 1 if \p X is less than \p Y.
* \c 0 if \p X is greater than or equal to \p Y.
*
* \return 0 on success.
* \return MBEDTLS_ERR_MPI_BAD_INPUT_DATA if the allocated length of
* the two input MPIs is not the same.
*/
int mbedtls_mpi_lt_mpi_ct(const mbedtls_mpi *X, const mbedtls_mpi *Y,
unsigned *ret);
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.
Yes, was going to suggest this.
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.
Sure! Reimplemented it with calling mbedtls_mpi_lt_mpi_ct
twice on x,y,z.
3a1684f
to
5a3daa1
Compare
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.
LGTM but requesting changes so that we use the proper const-time compare. Thanks!
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.
Will approve after changes addressed. Thanks for the PR
- Better comments - Better naming
- Add `Mpi` const time compare. - Refactor `EcPoint` const time compare.
5a3daa1
to
dcc0d62
Compare
dcc0d62
to
eb2cf13
Compare
EcPoint
const time comparison and better comments and namings
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.
Thanks!