Shift from_bytes into the Point impl.

Looks like we were also computing our test cases in a slightly sketchy
way, and just testing that we failed in exactly the same way. We do, but
now we generate better test data.

src/ed25519/mod.rs

@@ -5,7 +5,6 @@ mod point;
 use digest::Digest;
 use rand::Rng;
 use sha2::Sha512;
-use self::fe::*;
 use self::point::*;
 #[cfg(test)]
 use testing::run_test;
@@ -134,8 +133,7 @@ impl ED25519Public {
             return false;
         }
 
-        let mut a = Point::new();
-        x25519_ge_frombytes_vartime(&mut a, &self.public);
+        let mut a = Point::from_bytes(&self.public).unwrap(); // FIXME!!
         a.invert();
         let h_digest = eddsa_digest(signature_r, &self.public, msg);
         let h = digest_scalar(&h_digest);

src/ed25519/point.rs

@@ -35,31 +35,11 @@ impl Point {
     }
   }
 
-  pub fn encode_to(&self, target: &mut [u8])
-  {
-    into_encoded_point(target, &self.x, &self.y, &self.z);
-  }
-
-  pub fn invert(&mut self)
-  {
-      let tmp = self.clone();
-      fe_neg(&mut self.x, &tmp.x);
-      fe_neg(&mut self.t, &tmp.t);
-  }
-}
-
-const D: FieldElement = FieldElement {
-    value: [-10913610, 13857413, -15372611, 6949391,   114729,
-            -8787816,  -6275908, -3247719,  -18696448, -12055116]
-};
-
-const SQRTM1: FieldElement = FieldElement {
-    value: [-32595792, -7943725,  9377950,  3500415, 12389472,
-            -272473,   -25146209, -2005654, 326686,  11406482]
-};
-
-pub fn x25519_ge_frombytes_vartime(h: &mut Point, s: &[u8]) -> bool
+  /// Convert 32 bytes into an ED25519 point. This routine is not
+  /// statically timed, so don't use it if that's important to you.
+  pub fn from_bytes(s: &[u8]) -> Option<Point>
   {
+      let mut h = Point::new();
       let mut u = FieldElement::new();
       let mut v = FieldElement::new();
       let mut v3 = FieldElement::new();
@@ -99,7 +79,7 @@ pub fn x25519_ge_frombytes_vartime(h: &mut Point, s: &[u8]) -> bool
       if fe_isnonzero(&check) {
         fe_add(&mut check, &vxx, &u); /* vx^2+u */
         if fe_isnonzero(&check) {
-      return false;
+          return None;
         }
         temp = h.x.clone();
         fe_mul(&mut h.x, &temp, &SQRTM1);
@@ -111,22 +91,49 @@ pub fn x25519_ge_frombytes_vartime(h: &mut Point, s: &[u8]) -> bool
       }
 
       fe_mul(&mut h.t, &h.x, &h.y);
-  return true;
+      return Some(h);
   }
 
+  pub fn encode_to(&self, target: &mut [u8])
+  {
+    into_encoded_point(target, &self.x, &self.y, &self.z);
+  }
+
+  pub fn invert(&mut self)
+  {
+      let tmp = self.clone();
+      fe_neg(&mut self.x, &tmp.x);
+      fe_neg(&mut self.t, &tmp.t);
+  }
+}
+
+const D: FieldElement = FieldElement {
+    value: [-10913610, 13857413, -15372611, 6949391,   114729,
+            -8787816,  -6275908, -3247719,  -18696448, -12055116]
+};
+
+const SQRTM1: FieldElement = FieldElement {
+    value: [-32595792, -7943725,  9377950,  3500415, 12389472,
+            -272473,   -25146209, -2005654, 326686,  11406482]
+};
+
 #[cfg(test)]
 #[test]
 fn from_bytes_vartime() {
     let fname = "testdata/ed25519/fbv.test";
-    run_test(fname.to_string(), 2, |case| {
+    run_test(fname.to_string(), 3, |case| {
         let (nega, abytes) = case.get("a").unwrap();
+        let (negb, bbytes) = case.get("b").unwrap();
         let (negc, cbytes) = case.get("c").unwrap();
 
-        assert!(!nega && !negc);
+        assert!(!nega && !negb && !negc);
         let target = Point::load_test_value(&cbytes);
-        let mut mine = Point::new();
-        x25519_ge_frombytes_vartime(&mut mine, &abytes);
-        assert_eq!(target, mine);
+        let mine = Point::from_bytes(&abytes);
+        if bbytes.len() < cbytes.len() {
+            assert!(mine.is_none());
+        } else {
+            assert_eq!(target, mine.unwrap());
+        }
     });
 }
 
@@ -1789,49 +1796,49 @@ pub fn curve25519_scalar_mask(a: &mut [u8])
 //  fe_mul(&mut x2, &tmp2, &z2);
 //  fe_tobytes(out, &x2);
 //}
-
-pub fn x25519_public_from_private(public: &mut [u8], private: &[u8])
-{
-  assert_eq!(private.len(), 32);
-  assert_eq!(public.len(), 32);
-
-  let mut e = [0; 32];
-  e.copy_from_slice(private);
-  curve25519_scalar_mask(&mut e);
-
-  #[allow(non_snake_case)]
-  let mut A = Point::new();
-  x25519_ge_scalarmult_base(&mut A, &e);
-
-  /* We only need the u-coordinate of the curve25519 point. The map is
-   * u=(y+1)/(1-y). Since y=Y/Z, this gives u=(Z+Y)/(Z-Y). */
-  let mut zplusy = FieldElement::new();
-  let mut zminusy = FieldElement::new();
-  fe_add(&mut zplusy,  &A.z, &A.y);
-  fe_sub(&mut zminusy, &A.z, &A.y);
-  let zminusy_inv = fe_invert(&zminusy);
-  let copy = zplusy.clone();
-  fe_mul(&mut zplusy, &copy, &zminusy_inv);
-  fe_tobytes(public, &zplusy);
-}
-
-#[cfg(test)]
-#[test]
-fn public_from_private() {
-    let fname = "testdata/ed25519/pubfrompriv.test";
-    run_test(fname.to_string(), 2, |case| {
-        let (nega, abytes) = case.get("a").unwrap();
-        let (negb, bbytes) = case.get("b").unwrap();
-
-        assert!(!nega && !negb);
-        let mut mine = [0; 32];
-        x25519_public_from_private(&mut mine, abytes);
-        for i in 0..32 {
-          assert_eq!(mine[i], bbytes[i]);
-        }
-    });
-}
-
+//
+//pub fn x25519_public_from_private(public: &mut [u8], private: &[u8])
+//{
+//  assert_eq!(private.len(), 32);
+//  assert_eq!(public.len(), 32);
+//
+//  let mut e = [0; 32];
+//  e.copy_from_slice(private);
+//  curve25519_scalar_mask(&mut e);
+//
+//  #[allow(non_snake_case)]
+//  let mut A = Point::new();
+//  x25519_ge_scalarmult_base(&mut A, &e);
+//
+//  /* We only need the u-coordinate of the curve25519 point. The map is
+//   * u=(y+1)/(1-y). Since y=Y/Z, this gives u=(Z+Y)/(Z-Y). */
+//  let mut zplusy = FieldElement::new();
+//  let mut zminusy = FieldElement::new();
+//  fe_add(&mut zplusy,  &A.z, &A.y);
+//  fe_sub(&mut zminusy, &A.z, &A.y);
+//  let zminusy_inv = fe_invert(&zminusy);
+//  let copy = zplusy.clone();
+//  fe_mul(&mut zplusy, &copy, &zminusy_inv);
+//  fe_tobytes(public, &zplusy);
+//}
+//
+//#[cfg(test)]
+//#[test]
+//fn public_from_private() {
+//    let fname = "testdata/ed25519/pubfrompriv.test";
+//    run_test(fname.to_string(), 2, |case| {
+//        let (nega, abytes) = case.get("a").unwrap();
+//        let (negb, bbytes) = case.get("b").unwrap();
+//
+//        assert!(!nega && !negb);
+//        let mut mine = [0; 32];
+//        x25519_public_from_private(&mut mine, abytes);
+//        for i in 0..32 {
+//          assert_eq!(mine[i], bbytes[i]);
+//        }
+//    });
+//}
+//
 fn into_encoded_point(bytes: &mut [u8], x: &FieldElement, y: &FieldElement, z: &FieldElement)
 {
     let mut x_over_z = FieldElement::new();

test-generator/ED25519.hs

@@ -263,11 +263,15 @@ fbvTests = Task {
             curve25519_scalar_mask aptr'
             alloca $ \ dest ->
               do clearSpace dest
-                 point_frombytes dest aptr'
+                 ok <- point_frombytes dest aptr'
                  a <- pack `fmap` peekArray 32 (castPtr aptr)
                  c <- pack `fmap` peekArray (4 * 10 * 4) (castPtr dest)
-                 let res = Map.fromList [("a", showBin a), ("c", showBin c)]
-                 return (res, toNumber abytes, (memory0, drg1))
+                 let c' | ok        = c
+                        | otherwise = BS.empty
+                 let res = Map.fromList [("a", showBin a),
+                                         ("b", showBin c'),
+                                         ("c", showBin c)]
+                 return (res, if ok then (toNumber abytes) else 0, (memory0, drg1))
 
 conversionTests :: Task
 conversionTests = Task {
@@ -566,12 +570,18 @@ instance Storable Point3 where
   poke      p (P3 v) = pokeArray (castPtr p) v
 
 randomPoint3 :: SystemRandom -> (Ptr Point3 -> SystemRandom -> IO a) -> IO a
-randomPoint3 drg action =
-  randomPackedBytes drg $ \ aptr drg' ->
-    allocaArray (4 * 10) $ \ dest ->
+randomPoint3 drg0 action = allocaArray (4 * 10) (go drg0)
+ where
+  go drg dest =
+    do mres <- randomPackedBytes drg $ \ aptr drg' ->
                  do clearSpace dest
-         point_frombytes dest aptr
-         action (castPtr dest) drg'
+                    worked <- point_frombytes dest aptr
+                    if worked
+                       then Right `fmap` action (castPtr dest) drg'
+                       else return (Left drg')
+       case mres of
+         Right x -> return x
+         Left drg' -> go drg' dest
 
 data PointCached = PC [Element]
 
@@ -690,7 +700,7 @@ foreign import ccall unsafe "fe_sq2"
 foreign import ccall unsafe "fe_pow22523"
   fe_pow22523 :: Ptr Element -> Ptr Element -> IO ()
 foreign import ccall unsafe "GFp_x25519_ge_frombytes_vartime"
-  point_frombytes :: Ptr Point3 -> Ptr PackedBytes -> IO ()
+  point_frombytes :: Ptr Point3 -> Ptr PackedBytes -> IO Bool
 foreign import ccall unsafe "x25519_ge_p3_to_cached"
   p3_to_cached :: Ptr PointCached -> Ptr Point3 -> IO ()
 foreign import ccall unsafe "x25519_ge_p1p1_to_p2"