Check the subgroup of points decoded through Serde

core/CHANGELOG.md

@@ -50,6 +50,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- Reject points outside the prime-order subgroup when deserializing a `Note`
+  or `Sender` with Serde [#318]
 - Ignore a small-order component of `R` in `ViewKey::owns`, `SecretKey::owns`
   and `SecretKey::gen_note_sk` [#317]
 - Reject a stealth address whose `R` is the identity or of small order in
@@ -499,6 +501,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Canonical implementation shielded by feature.
 
 <!-- ISSUES -->
+[#318]: https://github.com/dusk-network/phoenix/issues/318
 [#319]: https://github.com/dusk-network/phoenix/issues/319
 [#317]: https://github.com/dusk-network/phoenix/issues/317
 [#316]: https://github.com/dusk-network/phoenix/issues/316

core/src/note.rs

@@ -77,6 +77,10 @@ impl TryFrom<i32> for NoteType {
 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
 pub struct Note {
     pub(crate) note_type: NoteType,
+    #[cfg_attr(
+        feature = "serde",
+        serde(deserialize_with = "crate::serde_support::deserialize_point")
+    )]
     pub(crate) value_commitment: JubJubAffine,
     pub(crate) stealth_address: StealthAddress,
     #[cfg_attr(
@@ -562,7 +566,15 @@ impl Serializable<SIZE> for Note {
 pub enum Sender {
     /// The sender's [`PublicKey`], encrypted using the note_pk of the
     /// stealth-address.
-    Encryption([(JubJubAffine, JubJubAffine); 2]),
+    Encryption(
+        #[cfg_attr(
+            feature = "serde",
+            serde(
+                deserialize_with = "crate::serde_support::deserialize_sender_enc"
+            )
+        )]
+        [(JubJubAffine, JubJubAffine); 2],
+    ),
     /// Information to identify the origin of a `Note`, if it wasn't created as
     /// a phoenix-transaction output-note.
     ContractInfo(

core/src/serde_support.rs

@@ -10,11 +10,46 @@ use alloc::format;
 use alloc::string::{String, ToString};
 
 use dusk_bytes::Serializable;
+use dusk_jubjub::JubJubAffine;
 use serde::de::Error as SerdeError;
 use serde::{Deserialize, Deserializer, Serialize, Serializer};
+use serde_with::As;
+use serde_with::hex::Hex;
 
 use crate::{PublicKey, SecretKey, StealthAddress, ViewKey};
 
+/// Decodes a point of exactly 32 bytes through [`Serializable::from_bytes`],
+/// as the `dusk-bytes` decoders do, which requires it to lie in the
+/// prime-order subgroup. The `Deserialize` of `JubJubAffine` skips that check
+/// (dusk-network/jubjub#192).
+pub(crate) fn deserialize_point<'de, D: Deserializer<'de>>(
+    deserializer: D,
+) -> Result<JubJubAffine, D::Error> {
+    let bytes: [u8; JubJubAffine::SIZE] = As::<Hex>::deserialize(deserializer)?;
+    // `JubJubAffine::from_bytes` resolves to the inherent decoder, which skips
+    // the subgroup check.
+    <JubJubAffine as Serializable<32>>::from_bytes(&bytes)
+        .map_err(|err| SerdeError::custom(format!("{err:?}")))
+}
+
+/// Decodes the points of a sender encryption as [`deserialize_point`] does.
+pub(crate) fn deserialize_sender_enc<'de, D: Deserializer<'de>>(
+    deserializer: D,
+) -> Result<[(JubJubAffine, JubJubAffine); 2], D::Error> {
+    struct Point(JubJubAffine);
+
+    impl<'de> Deserialize<'de> for Point {
+        fn deserialize<D: Deserializer<'de>>(
+            deserializer: D,
+        ) -> Result<Self, D::Error> {
+            deserialize_point(deserializer).map(Self)
+        }
+    }
+
+    let enc = <[(Point, Point); 2]>::deserialize(deserializer)?;
+    Ok(enc.map(|(c1, c2)| (c1.0, c2.0)))
+}
+
 impl Serialize for PublicKey {
     fn serialize<S: Serializer>(
         &self,

core/tests/serde.rs

@@ -8,7 +8,9 @@
 
 use dusk_bls12_381::BlsScalar;
 use dusk_bytes::Serializable;
-use dusk_jubjub::{JubJubAffine, JubJubScalar};
+use dusk_jubjub::{
+    GENERATOR_EXTENDED, JubJubAffine, JubJubExtended, JubJubScalar,
+};
 use ff::Field;
 use jubjub_schnorr::{PublicKey as NotePublicKey, SecretKey as NoteSecretKey};
 use phoenix_core::{
@@ -160,6 +162,78 @@ fn serde_note() -> Result<(), Box<dyn std::error::Error>> {
     Ok(())
 }
 
+#[test]
+fn serde_rejects_points_outside_the_subgroup()
+-> Result<(), Box<dyn std::error::Error>> {
+    // The order-two point `(0, -1)`, and the generator plus it.
+    let order_two =
+        JubJubAffine::from_raw_unchecked(BlsScalar::zero(), -BlsScalar::one());
+    let mixed = JubJubAffine::from(
+        GENERATOR_EXTENDED + JubJubExtended::from(order_two),
+    );
+
+    let mut rng = StdRng::seed_from_u64(0xc0b);
+    let note = create_note(&mut rng);
+    let honest = serde_json::to_value(&note)?;
+    assert_eq!(serde_json::from_value::<Note>(honest.clone())?, note);
+
+    for point in [order_two, mixed] {
+        let point = serde_json::to_value(point)?;
+
+        let mut json = honest.clone();
+        json["value_commitment"] = point.clone();
+        assert!(serde_json::from_value::<Note>(json).is_err());
+
+        for (i, j) in [(0, 0), (0, 1), (1, 0), (1, 1)] {
+            let mut json = honest.clone();
+            json["sender"]["Encryption"][i][j] = point.clone();
+            let sender = json["sender"].clone();
+            assert!(serde_json::from_value::<Sender>(sender).is_err());
+            assert!(serde_json::from_value::<Note>(json).is_err());
+        }
+    }
+
+    // The identity lies in the subgroup, as in the empty note.
+    let empty = Note::empty();
+    let json = serde_json::to_string(&empty)?;
+    assert_eq!(serde_json::from_str::<Note>(&json)?, empty);
+
+    Ok(())
+}
+
+#[test]
+fn serde_rejects_points_of_wrong_length()
+-> Result<(), Box<dyn std::error::Error>> {
+    let mut rng = StdRng::seed_from_u64(0xc0b);
+    let note = create_note(&mut rng);
+    let honest = serde_json::to_value(&note)?;
+
+    // An honest point with one byte more, and with one byte less.
+    let resized = |point: &serde_json::Value| {
+        let hex = point.as_str().expect("a point is encoded in hex");
+        [format!("{hex}00"), hex[..hex.len() - 2].to_string()]
+            .map(serde_json::Value::from)
+    };
+
+    for point in resized(&honest["value_commitment"]) {
+        let mut json = honest.clone();
+        json["value_commitment"] = point;
+        assert!(serde_json::from_value::<Note>(json).is_err());
+    }
+
+    for (i, j) in [(0, 0), (0, 1), (1, 0), (1, 1)] {
+        for point in resized(&honest["sender"]["Encryption"][i][j]) {
+            let mut json = honest.clone();
+            json["sender"]["Encryption"][i][j] = point;
+            let sender = json["sender"].clone();
+            assert!(serde_json::from_value::<Sender>(sender).is_err());
+            assert!(serde_json::from_value::<Note>(json).is_err());
+        }
+    }
+
+    Ok(())
+}
+
 #[test]
 fn serde_tx_skeleton() -> Result<(), Box<dyn std::error::Error>> {
     let mut rng = StdRng::seed_from_u64(0xc0b);