From ca8c46220b3410ced03e7f190b8c50584531aa7d Mon Sep 17 00:00:00 2001 From: Henry de Valence Date: Thu, 12 Jul 2018 12:44:56 -0700 Subject: [PATCH 1/7] Add safety notes to the README --- README.md | 58 +++++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 50 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 4c7353c..050c976 100644 --- a/README.md +++ b/README.md @@ -66,13 +66,14 @@ Curve arithmetic is implemented using one of the following backends: * a `u32` backend using `u64` products; * a `u64` backend using `u128` products; -* an `avx2` backend using parallel formulas, available when compiling for a - target with `target_feature=+avx2`. +* an `avx2` backend using [parallel formulas][parallel_doc], available + when compiling for a target with `target_feature=+avx2`. By default the `u64` backend is selected. To select a specific backend, use: ```sh cargo build --no-default-features --features "std u32_backend" cargo build --no-default-features --features "std u64_backend" +# Requires RUSTFLAGS="-C target_feature=+avx2" cargo build --no-default-features --features "std avx2_backend" ``` Crates using `curve25519-dalek` can either select a backend on behalf of their @@ -83,6 +84,50 @@ builds using `--no-default-features`. Note that this requires explicitly selecting an arithmetic backend using one of the `_backend` features. If no backend is selected, compilation will fail. +The `yolocrypto` feature enables experimental features. The name `yolocrypto` +is meant to indicate that it is not considered production-ready, and we do not +consider `yolocrypto` features to be covered by semver guarantees. +This is designed to make it easier to test intended new features +without having to stabilise them first. Use `yolocrypto` at your own, +obvious, risk. + +# Safety + +The `curve25519-dalek` types are designed to make illegal states +unrepresentable. For example, any instance of an `EdwardsPoint` is +guaranteed to hold a point on the Edwards curve, and any instance of a +`RistrettoPoint` is guaranteed to hold a valid point in the Ristretto +group. + +All operations are implemented using constant-time logic (no +secret-dependent branches, no secret-dependent memory accesses), +unless specifically marked as being variable-time code. +When using the `nightly` feature, we also insert an optimization +barrier before every conditional move or assignment. + +Some functionality (e.g., multiscalar multiplication or batch +inversion) requires heap allocation for temporary buffers. **All +heap-allocated buffers of potentially secret data are explicitly +zeroed before release**. + +However, we do not attempt to zero stack data, for two reasons. +First, it's not possible to do so correctly: we don't have control +over stack allocations, so there's no way to know how much data to +wipe. Second, because `curve25519-dalek` provides a mid-level API, +the correct place to start zeroing stack data is likely not at the +entrypoints of `curve25519-dalek` functions, but at the entrypoints of +functions in other crates. + +The implementation is memory-safe, and contains no significant +`unsafe` code. The AVX2 backend uses `unsafe` internally to call AVX2 +intrinsics. These are marked `unsafe` because invoking them on a +non-AVX2 target would cause `SIGILL`, but the entire backend is only +compiled for `target_feature=+avx2`. Some types implement an `unsafe +trait` to mark them as zeroable (for heap allocations), but this does +not affect memory safety. + +# Performance + Benchmarks are run using [`criterion.rs`][criterion]: ```sh @@ -93,12 +138,8 @@ cargo bench --no-default-features --features "std u64_backend" cargo bench --no-default-features --features "std avx2_backend" ``` -The `yolocrypto` feature enables experimental features. The name `yolocrypto` -is meant to indicate that it is not considered production-ready, and we do not -consider `yolocrypto` features to be covered by semver guarantees. -This is designed to make it easier to test intended new features -without having to stabilise them first. Use `yolocrypto` at your own, -obvious, risk. +Performance is a secondary goal behind correctness, safety, and +clarity, but we aim to be competitive with other implementations. # Contributing @@ -144,3 +185,4 @@ contributions. [docs-external]: https://doc.dalek.rs/curve25519_dalek/ [docs-internal]: https://doc-internal.dalek.rs/curve25519_dalek/ [criterion]: https://github.com/japaric/criterion.rs +[parallel_doc]: https://doc-internal.dalek.rs/curve25519_dalek/backend/avx2/index.html From 8d46eacd2cea9f248ee603d67237c9d1f96257c2 Mon Sep 17 00:00:00 2001 From: Henry de Valence Date: Mon, 16 Jul 2018 14:30:14 -0700 Subject: [PATCH 2/7] Clarify wording on the nightly feature and CT Closes #147 --- README.md | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 050c976..f241f81 100644 --- a/README.md +++ b/README.md @@ -102,13 +102,19 @@ group. All operations are implemented using constant-time logic (no secret-dependent branches, no secret-dependent memory accesses), unless specifically marked as being variable-time code. -When using the `nightly` feature, we also insert an optimization -barrier before every conditional move or assignment. +We believe that our constant-time logic is lowered to constant-time +assembly, at least on `x86_64` targets. + +As an additional guard against possible future compiler optimizations, the +`nightly` feature places an optimization barrier before every +conditional move or assignment. More details can be found in [the +documentation for the `subtle` crate][subtle_doc]. This is +recommended, but not required. Some functionality (e.g., multiscalar multiplication or batch -inversion) requires heap allocation for temporary buffers. **All +inversion) requires heap allocation for temporary buffers. All heap-allocated buffers of potentially secret data are explicitly -zeroed before release**. +zeroed before release. However, we do not attempt to zero stack data, for two reasons. First, it's not possible to do so correctly: we don't have control @@ -186,3 +192,4 @@ contributions. [docs-internal]: https://doc-internal.dalek.rs/curve25519_dalek/ [criterion]: https://github.com/japaric/criterion.rs [parallel_doc]: https://doc-internal.dalek.rs/curve25519_dalek/backend/avx2/index.html +[subtle_doc]: https://doc.dalek.rs/subtle/ From bc731f9d79100743b1995286b111564ec8df2f9f Mon Sep 17 00:00:00 2001 From: Henry de Valence Date: Mon, 16 Jul 2018 22:11:48 -0700 Subject: [PATCH 3/7] Remove fixme notes from FieldElement code --- src/field.rs | 21 +++++---------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/src/field.rs b/src/field.rs index 2241c13..9d8d041 100644 --- a/src/field.rs +++ b/src/field.rs @@ -96,17 +96,10 @@ impl FieldElement { /// Compute (self^(2^250-1), self^11), used as a helper function /// within invert() and pow22523(). - /// - /// XXX This returns an extra intermediate to save computation in - /// finding inverses, at the cost of an extra copy when it's not - /// used (e.g., when raising to (p-1)/2 or (p-5)/8). Good idea? fn pow22501(&self) -> (FieldElement, FieldElement) { // Instead of managing which temporary variables are used - // for what, we define as many as we need and trust the - // compiler to reuse stack space as appropriate. - // - // XXX testing some examples suggests that this does happen, - // but it would be good to check asm for this function. + // for what, we define as many as we need and leave stack + // allocation to the compiler // // Each temporary variable t_i is of the form (self)^e_i. // Squaring t_i corresponds to multiplying e_i by 2, @@ -177,12 +170,9 @@ impl FieldElement { /// /// The inverse is computed as self^(p-2), since /// x^(p-2)x = x^(p-1) = 1 (mod p). - // - // XXX do we want the debug assertion to check for zero? it breaks behaviour - // such as that such as in curve25519_dalek::montgomery::test::identity_to_monty. + /// + /// This function returns zero on input zero. pub fn invert(&self) -> FieldElement { - // debug_assert!(*self != FieldElement::zero()); - // The bits of p-2 = 2^255 -19 -2 are 11010111111...11. // // nonzero bits of exponent @@ -194,8 +184,7 @@ impl FieldElement { } /// Raise this field element to the power (p-5)/8 = 2^252 -3. - /// Used in decoding. - pub fn pow_p58(&self) -> FieldElement { + fn pow_p58(&self) -> FieldElement { // The bits of (p-5)/8 are 101111.....11. // // nonzero bits of exponent From 5bb6cd42a21f40979e84238cf185c458531bd30b Mon Sep 17 00:00:00 2001 From: Henry de Valence Date: Mon, 16 Jul 2018 22:13:40 -0700 Subject: [PATCH 4/7] we won't remove this function --- src/edwards.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/edwards.rs b/src/edwards.rs index 7e15fce..63c8689 100644 --- a/src/edwards.rs +++ b/src/edwards.rs @@ -583,8 +583,6 @@ impl VartimeMultiscalarMul for EdwardsPoint { impl EdwardsPoint { /// Compute \\(aA + bB\\) in variable time, where \\(B\\) is the Ed25519 basepoint. - /// - /// XXX eliminate this function when we have the precomputation API #[cfg(feature = "stage2_build")] pub fn vartime_double_scalar_mul_basepoint(a: &Scalar, A: &EdwardsPoint, b: &Scalar) -> EdwardsPoint { // If we built with AVX2, use the AVX2 backend. From f7f3f79da89ec8b08d58718b4df860a4c3314db8 Mon Sep 17 00:00:00 2001 From: Henry de Valence Date: Mon, 16 Jul 2018 22:22:21 -0700 Subject: [PATCH 5/7] Add missing Ristretto vartime-double-base fn --- src/edwards.rs | 14 +++++--------- src/ristretto.rs | 15 +++++++++++++++ 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/src/edwards.rs b/src/edwards.rs index 63c8689..9930d84 100644 --- a/src/edwards.rs +++ b/src/edwards.rs @@ -587,16 +587,12 @@ impl EdwardsPoint { pub fn vartime_double_scalar_mul_basepoint(a: &Scalar, A: &EdwardsPoint, b: &Scalar) -> EdwardsPoint { // If we built with AVX2, use the AVX2 backend. #[cfg(all(feature="avx2_backend", target_feature="avx2"))] - { - use backend::avx2::scalar_mul::vartime_double_base::mul; - mul(a, A, b) - } - // Otherwise, proceed as normal: + use backend::avx2::scalar_mul::vartime_double_base; + // Otherwise, use the serial backend: #[cfg(not(all(feature="avx2_backend", target_feature="avx2")))] - { - use scalar_mul::vartime_double_base::mul; - mul(a, A, b) - } + use scalar_mul::vartime_double_base; + + vartime_double_base::mul(a, A, b) } } diff --git a/src/ristretto.rs b/src/ristretto.rs index 5581a19..cb4e8e0 100644 --- a/src/ristretto.rs +++ b/src/ristretto.rs @@ -830,6 +830,21 @@ impl VartimeMultiscalarMul for RistrettoPoint { } } +impl RistrettoPoint { + /// Compute \\(aA + bB\\) in variable time, where \\(B\\) is the + /// Ristretto basepoint. + #[cfg(feature = "stage2_build")] + pub fn vartime_double_scalar_mul_basepoint( + a: &Scalar, + A: &RistrettoPoint, + b: &Scalar, + ) -> RistrettoPoint { + RistrettoPoint( + EdwardsPoint::vartime_double_scalar_mul_basepoint(a, &A.0, b) + ) + } +} + /// A precomputed table of multiples of a basepoint, used to accelerate /// scalar multiplication. /// From 0c58de036787a34c1db791ee3c84fd8e292ada3f Mon Sep 17 00:00:00 2001 From: Henry de Valence Date: Mon, 16 Jul 2018 22:22:58 -0700 Subject: [PATCH 6/7] it wouldn't be --- src/edwards.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/edwards.rs b/src/edwards.rs index 9930d84..df2f8c8 100644 --- a/src/edwards.rs +++ b/src/edwards.rs @@ -689,8 +689,6 @@ impl EdwardsBasepointTable { } /// Get the basepoint for this table as an `EdwardsPoint`. - /// - /// XXX maybe this would be better as a `From` impl pub fn basepoint(&self) -> EdwardsPoint { // self.0[0].select(1) = 1*(16^2)^0*B // but as an `AffineNielsPoint`, so add identity to convert to extended. From dfc9e7c0b7391e47580fa1f9f24093c8cbb4f5c8 Mon Sep 17 00:00:00 2001 From: Henry de Valence Date: Mon, 16 Jul 2018 22:28:22 -0700 Subject: [PATCH 7/7] fixup extendedpoint validity check --- src/edwards.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/edwards.rs b/src/edwards.rs index df2f8c8..08eff32 100644 --- a/src/edwards.rs +++ b/src/edwards.rs @@ -271,9 +271,11 @@ impl Identity for EdwardsPoint { // ------------------------------------------------------------------------ impl ValidityCheck for EdwardsPoint { - // XXX this should also check that T is correct fn is_valid(&self) -> bool { - self.to_projective().is_valid() + let point_on_curve = self.to_projective().is_valid(); + let on_segre_image = (&self.X * &self.Y) == (&self.Z * &self.T); + + point_on_curve && on_segre_image } }