From 8ee095c20e7414d04cd6d819f418acb11165e76f Mon Sep 17 00:00:00 2001 From: Hein Dauven Date: Sat, 10 Oct 2026 19:51:19 +0200 Subject: [PATCH] core: Check the subgroup of points decoded through Serde The value commitment of a `Note` and the points of a `Sender` encryption derive their Serde implementation from dusk-jubjub, whose `Deserialize` checks that a point is on the curve, but not, as `Serializable::from_bytes` does, that it lies in the prime-order subgroup (dusk-network/jubjub#192). A note or sender decoded from JSON could carry a point its dusk-bytes decoders reject. Decode those points through `Serializable::from_bytes`, as the Serde implementations of the keys and the stealth address already do through their dusk-bytes decoders. A point must still be exactly 32 bytes. The encoding is unchanged, and the existing JSON vectors still decode. The test replaces the value commitment and each sender point of an honest note with the order-two point, then with the generator plus it, and checks that neither the note nor the sender decodes, while the honest note, and the empty note, whose points are the identity, round-trip. Another test adds a byte to each point, then removes one, and checks that neither the note nor the sender decodes. --- core/CHANGELOG.md | 3 ++ core/src/note.rs | 14 +++++++- core/src/serde_support.rs | 35 ++++++++++++++++++ core/tests/serde.rs | 76 ++++++++++++++++++++++++++++++++++++++- 4 files changed, 126 insertions(+), 2 deletions(-) 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);