diff --git a/core/CHANGELOG.md b/core/CHANGELOG.md index 10d5220..7777a1c 100644 --- a/core/CHANGELOG.md +++ b/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. +[#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 diff --git a/core/src/note.rs b/core/src/note.rs index ebb6db5..f1bb3b1 100644 --- a/core/src/note.rs +++ b/core/src/note.rs @@ -77,6 +77,10 @@ impl TryFrom 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 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( diff --git a/core/src/serde_support.rs b/core/src/serde_support.rs index 473cd96..695cb0f 100644 --- a/core/src/serde_support.rs +++ b/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 { + let bytes: [u8; JubJubAffine::SIZE] = As::::deserialize(deserializer)?; + // `JubJubAffine::from_bytes` resolves to the inherent decoder, which skips + // the subgroup check. + >::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>( + deserializer: D, + ) -> Result { + 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( &self, diff --git a/core/tests/serde.rs b/core/tests/serde.rs index a1f0cf2..01e4dc1 100644 --- a/core/tests/serde.rs +++ b/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> { Ok(()) } +#[test] +fn serde_rejects_points_outside_the_subgroup() +-> Result<(), Box> { + // 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(¬e)?; + assert_eq!(serde_json::from_value::(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::(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).is_err()); + assert!(serde_json::from_value::(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::(&json)?, empty); + + Ok(()) +} + +#[test] +fn serde_rejects_points_of_wrong_length() +-> Result<(), Box> { + let mut rng = StdRng::seed_from_u64(0xc0b); + let note = create_note(&mut rng); + let honest = serde_json::to_value(¬e)?; + + // 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::(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).is_err()); + assert!(serde_json::from_value::(json).is_err()); + } + } + + Ok(()) +} + #[test] fn serde_tx_skeleton() -> Result<(), Box> { let mut rng = StdRng::seed_from_u64(0xc0b);