From 4c04dcf3dbeb02233b04a129fee4b1b9a51ae24a Mon Sep 17 00:00:00 2001
From: David <dvdplm@gmail.com>
Date: Fri, 13 Dec 2024 07:05:08 +0100
Subject: [PATCH] Be explicit about byte order expectations (#167)

---
 synedrion/src/cggmp21/params.rs   | 10 ++++----
 synedrion/src/curve/arithmetic.rs | 40 +++++++++++++++++++++++++++----
 synedrion/src/curve/ecdsa.rs      |  2 +-
 3 files changed, 42 insertions(+), 10 deletions(-)

diff --git a/synedrion/src/cggmp21/params.rs b/synedrion/src/cggmp21/params.rs
index 4c8e4d6..18f5cf9 100644
--- a/synedrion/src/cggmp21/params.rs
+++ b/synedrion/src/cggmp21/params.rs
@@ -130,7 +130,7 @@ pub trait SchemeParams: Debug + Clone + Send + PartialEq + Eq + Send + Sync + 's
 
 /// Converts a curve scalar to the associated integer type.
 pub(crate) fn uint_from_scalar<P: SchemeParams>(value: &Scalar) -> <P::Paillier as PaillierParams>::Uint {
-    let scalar_bytes = value.to_bytes();
+    let scalar_bytes = value.to_be_bytes();
     let mut repr = <P::Paillier as PaillierParams>::Uint::zero().to_be_bytes();
 
     let uint_len = repr.as_ref().len();
@@ -166,7 +166,7 @@ pub(crate) fn scalar_from_uint<P: SchemeParams>(value: &<P::Paillier as Paillier
     let scalar_len = Scalar::repr_len();
 
     // Can unwrap here since the value is within the Scalar range
-    Scalar::try_from_bytes(&repr.as_ref()[uint_len - scalar_len..])
+    Scalar::try_from_be_bytes(&repr.as_ref()[uint_len - scalar_len..])
         .expect("the value was reduced modulo `CURVE_ORDER`, so it's a valid curve scalar")
 }
 
@@ -185,7 +185,7 @@ pub(crate) fn scalar_from_wide_uint<P: SchemeParams>(value: &<P::Paillier as Pai
     let scalar_len = Scalar::repr_len();
 
     // Can unwrap here since the value is within the Scalar range
-    Scalar::try_from_bytes(&repr.as_ref()[uint_len - scalar_len..])
+    Scalar::try_from_be_bytes(&repr.as_ref()[uint_len - scalar_len..])
         .expect("the value was reduced modulo `CURVE_ORDER`, so it's a valid curve scalar")
 }
 
@@ -208,7 +208,7 @@ pub(crate) fn secret_scalar_from_uint<P: SchemeParams>(
 
     // Can unwrap here since the value is within the Scalar range
     Secret::init_with(|| {
-        Scalar::try_from_bytes(&repr.expose_secret().as_ref()[uint_len - scalar_len..])
+        Scalar::try_from_be_bytes(&repr.expose_secret().as_ref()[uint_len - scalar_len..])
             .expect("the value was reduced modulo `CURVE_ORDER`, so it's a valid curve scalar")
     })
 }
@@ -216,7 +216,7 @@ pub(crate) fn secret_scalar_from_uint<P: SchemeParams>(
 pub(crate) fn secret_uint_from_scalar<P: SchemeParams>(
     value: &Secret<Scalar>,
 ) -> Secret<<P::Paillier as PaillierParams>::Uint> {
-    let scalar_bytes = Secret::init_with(|| value.expose_secret().to_bytes());
+    let scalar_bytes = Secret::init_with(|| value.expose_secret().to_be_bytes());
     let mut repr = Secret::init_with(|| <P::Paillier as PaillierParams>::Uint::zero().to_be_bytes());
 
     let uint_len = repr.expose_secret().as_ref().len();
diff --git a/synedrion/src/curve/arithmetic.rs b/synedrion/src/curve/arithmetic.rs
index 4101706..edf31d7 100644
--- a/synedrion/src/curve/arithmetic.rs
+++ b/synedrion/src/curve/arithmetic.rs
@@ -98,7 +98,8 @@ impl Scalar {
         Self(<BackendScalar as Reduce<U256>>::reduce_bytes(&arr))
     }
 
-    pub fn to_bytes(self) -> k256::FieldBytes {
+    /// Returns the SEC1 encoding of this scalar (big endian order).
+    pub fn to_be_bytes(self) -> k256::FieldBytes {
         self.0.to_bytes()
     }
 
@@ -114,7 +115,8 @@ impl Scalar {
         Secret::init_with(|| Self(*sk.as_nonzero_scalar().as_ref()))
     }
 
-    pub(crate) fn try_from_bytes(bytes: &[u8]) -> Result<Self, String> {
+    /// Attempts to instantiate a `Scalar` from a slice of bytes. Assumes big-endian order.
+    pub(crate) fn try_from_be_bytes(bytes: &[u8]) -> Result<Self, String> {
         let arr = GenericArray::<u8, FieldBytesSize<Secp256k1>>::from_exact_iter(bytes.iter().cloned())
             .ok_or("Invalid length of a curve scalar")?;
 
@@ -151,7 +153,7 @@ pub(crate) fn secret_split(rng: &mut impl CryptoRngCore, scalar: Secret<Scalar>,
 impl<'a> TryFrom<&'a [u8]> for Scalar {
     type Error = String;
     fn try_from(val: &'a [u8]) -> Result<Self, Self::Error> {
-        Self::try_from_bytes(val)
+        Self::try_from_be_bytes(val)
     }
 }
 
@@ -169,7 +171,7 @@ impl ConditionallySelectable for Scalar {
 
 impl Serialize for Scalar {
     fn serialize<S: Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {
-        SliceLike::<Hex>::serialize(&self.to_bytes(), serializer)
+        SliceLike::<Hex>::serialize(&self.to_be_bytes(), serializer)
     }
 }
 
@@ -374,3 +376,33 @@ impl<'a> core::iter::Sum<&'a Self> for Point {
         iter.cloned().sum()
     }
 }
+
+#[cfg(test)]
+mod test {
+    use super::Scalar;
+    use rand::SeedableRng;
+    use rand_chacha::ChaChaRng;
+    #[test]
+    fn to_and_from_bytes() {
+        let mut rng = ChaChaRng::from_seed([7u8; 32]);
+        let s = Scalar::random(&mut rng);
+
+        // Round trip works
+        let bytes = s.to_be_bytes();
+        let s_from_bytes = Scalar::try_from_be_bytes(&bytes).expect("bytes are valid");
+        assert_eq!(s, s_from_bytes);
+
+        // …but building a `Scalar` from LE bytes does not.
+        let mut bytes = bytes;
+        let le_bytes = bytes
+            .chunks_exact_mut(8)
+            .flat_map(|word_bytes| {
+                word_bytes.reverse();
+                word_bytes.to_vec()
+            })
+            .collect::<Vec<u8>>();
+
+        let s_from_le_bytes = Scalar::try_from_be_bytes(&le_bytes).expect("bytes are valid-ish");
+        assert_ne!(s, s_from_le_bytes, "Using LE bytes should not work")
+    }
+}
diff --git a/synedrion/src/curve/ecdsa.rs b/synedrion/src/curve/ecdsa.rs
index e03fb0a..6a51c37 100644
--- a/synedrion/src/curve/ecdsa.rs
+++ b/synedrion/src/curve/ecdsa.rs
@@ -18,7 +18,7 @@ impl RecoverableSignature {
         // but consequent usage of it may fail otherwise.
         let signature = signature.normalize_s().unwrap_or(signature);
 
-        let message_bytes = message.to_bytes();
+        let message_bytes = message.to_be_bytes();
         let recovery_id = RecoveryId::trial_recovery_from_prehash(
             &VerifyingKey::from_affine(vkey.to_backend().to_affine()).ok()?,
             &message_bytes,