Merge remote-tracking branch 'hdevalence/more-pre-1.0-cleanups' into develop

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,56 @@ 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.
+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
+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 +144,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 +191,5 @@ 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
+[subtle_doc]: https://doc.dalek.rs/subtle/

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
     }
 }
 
@@ -583,22 +585,16 @@ 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.
         #[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)
     }
 }
 
@@ -695,8 +691,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.

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

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.
 ///