Skip to content
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

Correct EcPoint const time comparison and better comments and namings #345

Merged
merged 6 commits into from
Feb 13, 2024
Merged

Correct EcPoint const time comparison and better comments and namings #345

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e70de83
style improve
Taowyoo Feb 8, 2024
36543dd
another ct approach
Taowyoo Feb 8, 2024
114bbc9
fix dead_code
Taowyoo Feb 8, 2024
a0f2dd5
style improve
Taowyoo Feb 8, 2024
19cc8cd
const time compare
Taowyoo Feb 12, 2024
eb2cf13
refactor
Taowyoo Feb 12, 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
20 changes: 13 additions & 7 deletions mbedtls/src/ecp/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,10 +309,6 @@ impl EcPoint {
Mpi::copy(&self.inner.Y)
}

pub fn z(&self) -> Result<Mpi> {
Taowyoo marked this conversation as resolved.
Show resolved Hide resolved
Mpi::copy(&self.inner.Z)
}

pub fn is_zero(&self) -> Result<bool> {
/*
mbedtls_ecp_is_zero takes arg as non-const for no particular reason
Expand Down Expand Up @@ -373,9 +369,14 @@ Please use `mul_with_rng` instead."
///
/// This function will return an error if:
///
/// * `k` is not a valid private key, or `self` is not a valid public key.
/// * `k` is not a valid private key, determined by mbedtls function [`mbedtls_ecp_check_privkey`]
Taowyoo marked this conversation as resolved.
Show resolved Hide resolved
/// * `self` is not a valid public key, determined by mbedtls function [`mbedtls_ecp_check_pubkey`]
/// * Memory allocation fails.
/// * Any other kind of failure occurs during the execution of the underlying `mbedtls_ecp_mul` function.
/// * Any other kind of failure occurs during the execution of the underlying [`mbedtls_ecp_mul`] function.
///
/// [`mbedtls_ecp_check_pubkey`]: https://github.com/fortanix/rust-mbedtls/blob/main/mbedtls-sys/vendor/include/mbedtls/ecp.h#L1115-L1143
/// [`mbedtls_ecp_check_privkey`]: https://github.com/fortanix/rust-mbedtls/blob/main/mbedtls-sys/vendor/include/mbedtls/ecp.h#L1145-L1165
/// [`mbedtls_ecp_mul`]: https://github.com/fortanix/rust-mbedtls/blob/main/mbedtls-sys/vendor/include/mbedtls/ecp.h#L933-L971
pub fn mul_with_rng<F: crate::rng::Random>(&self, group: &mut EcGroup, k: &Mpi, rng: &mut F) -> Result<EcPoint> {
// Note: mbedtls_ecp_mul performs point validation itself so we skip that here

Expand Down Expand Up @@ -433,7 +434,12 @@ Please use `mul_with_rng` instead."
}
}

/// This function compares two points in const time.
/// This function checks equalness of two points in const time.
///
/// 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
Copy link
Collaborator Author

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?

Copy link
Collaborator Author

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);

Copy link
Contributor

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.

Copy link
Collaborator Author

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.

pub fn eq_const_time(&self, other: &EcPoint) -> bool {
unsafe {
let x = mpi_cmp_mpi(&self.inner.X, &other.inner.X) == 0;
Expand Down
10 changes: 5 additions & 5 deletions mbedtls/src/pk/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,7 @@ impl Pk {
#[deprecated(
since = "0.12.3",
note = "This function does not accept an RNG so it's vulnerable to side channel attacks.
Please use `private_from_ec_components_with_rng` instead."
Please use `private_from_ec_scalar_with_rng` instead."
)]
pub fn private_from_ec_components(mut curve: EcGroup, private_key: Mpi) -> Result<Pk> {
let mut ret = Self::init();
Expand Down Expand Up @@ -348,10 +348,10 @@ Please use `private_from_ec_components_with_rng` instead."
///
/// This function will return an error if:
///
/// * Fails to genearte `EcPoint` from given EcGroup in `curve`.
/// * Fails to generate `EcPoint` from given EcGroup in `curve`.
/// * The underlying C `mbedtls_pk_setup` function fails to set up the `Pk` context.
/// * The `EcPoint::mul` function fails to generate the public key point.
pub fn private_from_ec_components_with_rng<F: Random>(mut curve: EcGroup, private_key: Mpi, rng: &mut F) -> Result<Pk> {
/// * The `EcPoint::mul_with_rng` function fails to generate the public key point.
pub fn private_from_ec_scalar_with_rng<F: Random>(mut curve: EcGroup, private_key: Mpi, rng: &mut F) -> Result<Pk> {
let mut ret = Self::init();
let curve_generator = curve.generator()?;
let public_point = curve_generator.mul_with_rng(&mut curve, &private_key, rng)?;
Expand Down Expand Up @@ -1205,7 +1205,7 @@ iy6KC991zzvaWY/Ys+q/84Afqa+0qJKQnPuy/7F5GkVdQA/lfbhi

assert_eq!(pem1, pem2);

let mut key_from_components = Pk::private_from_ec_components_with_rng(
let mut key_from_components = Pk::private_from_ec_scalar_with_rng(
secp256r1.clone(),
key1.ec_private().unwrap(),
&mut crate::test_support::rand::test_rng(),
Expand Down
Loading