Enhance `ecp` and `pk` module #344

Taowyoo · 2024-02-05T19:24:30Z

What's New

Add functionalities for EcPoint and Pk:

Add a function to access Z field of EcPoint.
Add a newer version of EcPoint::mul with RNG for blinding.
- Also marked old version one as deprecated.
Add a newer version of Pk::private_from_ec_components with RNG for blinding.
- Also marked old version one as deprecated.
Add a function for performing const time comparison of EcPoint.

Test

Update tests to use above new functions with RNG.
Add a test for fn eq_const_time

Add bench for ensure fn eq_const_time works, see mbedtls/benches/ecp_eq_test.rs:

EcpPoint X not equal    time:   [6.2940 ns 6.3282 ns 6.3741 ns]
Found 13 outliers among 100 measurements (13.00%)
  1 (1.00%) high mild
  12 (12.00%) high severe

EcpPoint Y not equal    time:   [9.3641 ns 9.4346 ns 9.5214 ns]
Found 32 outliers among 100 measurements (32.00%)
  15 (15.00%) low severe
  2 (2.00%) low mild
  6 (6.00%) high mild
  9 (9.00%) high severe

EcpPoint Z not equal    time:   [12.141 ns 12.289 ns 12.477 ns]
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high severe

EcpPoint X not equal const time
                        time:   [9.7337 ns 9.7690 ns 9.8196 ns]
Found 19 outliers among 100 measurements (19.00%)
  5 (5.00%) low mild
  2 (2.00%) high mild
  12 (12.00%) high severe

EcpPoint Y not equal const time
                        time:   [9.6957 ns 9.7007 ns 9.7072 ns]
Found 11 outliers among 100 measurements (11.00%)
  5 (5.00%) high mild
  6 (6.00%) high severe

EcpPoint Z not equal const time
                        time:   [9.7067 ns 9.7381 ns 9.7804 ns]
Found 16 outliers among 100 measurements (16.00%)
  14 (14.00%) high mild
  2 (2.00%) high severe

- Add a function to access `Z` field of `EcPoint`. - Add a newer version of `EcPoint::mul` with RNG for blinding. - Also marked old version one as deprecated. - Add a newer version of `Pk::private_from_ec_components` with RNG for blinding. - Also marked old version one as deprecated. - Update tests to use above new functions. - Add a function for performing const time comparison of `EcPoint`.

s-arash

Left some nitpicks, otherwise looks good.

mbedtls/src/ecp/mod.rs

mbedtls/src/pk/mod.rs

Taowyoo

Better rustdoc

zugzwang · 2024-02-05T22:44:36Z

mbedtls/benches/ecp_eq_test.rs

Are these benches conclusive?

I am confused. How to determine they are conclusive or not?

zugzwang · 2024-02-05T22:46:40Z

mbedtls/src/ecp/mod.rs

@@ -309,6 +309,10 @@ impl EcPoint {
        Mpi::copy(&self.inner.Y)
    }

+    pub fn z(&self) -> Result<Mpi> {


Why do we want to expose z? Use of Jacobian coordinates is normally internal for efficiency, and APIs should speak Cartesian

MbedTLS uses Z as an internal value and it ensures that every function accepts/returns Z=0 (infinity point) or Z=1. But we should not expose this function at all.

zugzwang · 2024-02-05T22:48:50Z

mbedtls/src/ecp/mod.rs

+    ///
+    /// This function will return an error if:
+    ///
+    /// * `k` is not a valid private key, or `self` is not a valid public key.


There is no notion of "private key" or "public key", just scalars and elliptic curve points. Also, documentation should be a bit more precise (what is "invalid"?)

zugzwang · 2024-02-05T22:50:16Z

mbedtls/src/ecp/mod.rs

@@ -379,6 +433,16 @@ impl EcPoint {
        }
    }

+    /// This function compares two points in const time.
+    pub fn eq_const_time(&self, other: &EcPoint) -> bool {


Can you confirm this is really constant time?

I addedbench for ensure fn eq_const_time works, see mbedtls/benches/ecp_eq_test.rs.
Is the result in the PR description enough?

You could checkout the result from GitHub Action: https://github.com/fortanix/rust-mbedtls/actions/runs/7823284568/job/21343954890

zugzwang · 2024-02-08T07:26:22Z

mbedtls/benches/ecp_eq_test.rs

are these conclusive? Timing difference can be as few as some cycles. Instead, it would be preferable to reference MbedTLS C demonstrating constant-time implementation

zugzwang · 2024-02-08T07:47:50Z

mbedtls/src/pk/mod.rs

+    /// * Fails to genearte `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> {


Should be called private_from_ec_scalar_with_rng .

Ok I will create another PR for update these

Taowyoo requested review from s-arash and zugzwang February 5, 2024 19:24

Taowyoo self-assigned this Feb 5, 2024

Taowyoo force-pushed the yx/enhance_ecp branch from bfc61d9 to 77909e5 Compare February 5, 2024 19:36

Taowyoo added 2 commits February 5, 2024 11:36

bump mbedtls version to 0.12.3

c7a596b

Taowyoo force-pushed the yx/enhance_ecp branch from 77909e5 to c7a596b Compare February 5, 2024 19:37

add test for fn eq_const_time

afeb644

Taowyoo force-pushed the yx/enhance_ecp branch from 07e92f3 to afeb644 Compare February 5, 2024 19:49

Taowyoo changed the title ~~Enhance ecp module~~ Enhance ecp and pk module Feb 5, 2024

Taowyoo added 4 commits February 5, 2024 13:00

add bench to ensure eq_const_time works

9f53058

add cargo bench to CI

3b1bff1

improve eq_const_time

8124a0b

better bench

4532fb5

s-arash approved these changes Feb 8, 2024

View reviewed changes

mbedtls/src/ecp/mod.rs Outdated Show resolved Hide resolved

mbedtls/src/pk/mod.rs Outdated Show resolved Hide resolved

mbedtls/src/pk/mod.rs Outdated Show resolved Hide resolved

Taowyoo commented Feb 8, 2024

View reviewed changes

Taowyoo added 2 commits February 7, 2024 16:51

style: better rustdoc

21bd24e

better rustdoc

6124e0e

Taowyoo enabled auto-merge (squash) February 8, 2024 00:57

Taowyoo merged commit 391b600 into main Feb 8, 2024
11 checks passed

Taowyoo deleted the yx/enhance_ecp branch February 8, 2024 01:06

zugzwang reviewed Feb 8, 2024

View reviewed changes

Taowyoo mentioned this pull request Feb 8, 2024

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance `ecp` and `pk` module #344

Enhance `ecp` and `pk` module #344

Taowyoo commented Feb 5, 2024 •

edited

Loading

s-arash left a comment

Taowyoo left a comment

zugzwang Feb 5, 2024

Taowyoo Feb 8, 2024

zugzwang Feb 5, 2024

zugzwang Feb 8, 2024

Taowyoo Feb 8, 2024

zugzwang Feb 5, 2024

zugzwang Feb 5, 2024

Taowyoo Feb 8, 2024

Taowyoo Feb 8, 2024

zugzwang Feb 8, 2024

zugzwang Feb 8, 2024

Taowyoo Feb 8, 2024

Enhance ecp and pk module #344

Enhance ecp and pk module #344

Conversation

Taowyoo commented Feb 5, 2024 • edited Loading

What's New

Test

s-arash left a comment

Choose a reason for hiding this comment

Taowyoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Enhance `ecp` and `pk` module #344

Enhance `ecp` and `pk` module #344

Taowyoo commented Feb 5, 2024 •

edited

Loading