From c30b6bef458e5c789bc537cfd87e81286b21ec0b Mon Sep 17 00:00:00 2001 From: j-berman Date: Wed, 3 Jan 2024 01:46:48 -0800 Subject: [PATCH 1/4] monero: only mask user features on new polyseed, not on decode - This commit ensures a polyseed string that has unsupported features correctly errors on decode (rather than panic in debug build or return an incorrect successful response in prod build) - Also avoids panicking when checksum calculation is unexpectedly wrong Polyseed reference impl for feature masking: - polyseed_create: https://github.com/tevador/polyseed/blob/b7c35bb3c6b91e481ecb04fc235eaff69c507fa1/src/polyseed.c#L61 - polyseed_decode: https://github.com/tevador/polyseed/blob/b7c35bb3c6b91e481ecb04fc235eaff69c507fa1/src/polyseed.c#L212 --- coins/monero/src/tests/seed.rs | 11 +++++- coins/monero/src/wallet/seed/polyseed.rs | 47 ++++++++++++++++-------- 2 files changed, 41 insertions(+), 17 deletions(-) diff --git a/coins/monero/src/tests/seed.rs b/coins/monero/src/tests/seed.rs index 646441184..7518d9dfd 100644 --- a/coins/monero/src/tests/seed.rs +++ b/coins/monero/src/tests/seed.rs @@ -7,7 +7,7 @@ use curve25519_dalek::scalar::Scalar; use crate::{ hash, wallet::seed::{ - Seed, SeedType, + Seed, SeedType, SeedError, classic::{self, trim_by_lang}, polyseed, }, @@ -405,3 +405,12 @@ fn test_polyseed() { } } } + +#[test] +fn test_invalid_polyseed() { + let seed = "include domain claim resemble urban hire lunch bird \ + crucial fire best wife ring warm ignore model" + .into(); + let res = Seed::from_string(Zeroizing::new(seed)); + assert_eq!(res, Err(SeedError::UnsupportedFeatures)); +} diff --git a/coins/monero/src/wallet/seed/polyseed.rs b/coins/monero/src/wallet/seed/polyseed.rs index a4f62506b..c7a5baa2b 100644 --- a/coins/monero/src/wallet/seed/polyseed.rs +++ b/coins/monero/src/wallet/seed/polyseed.rs @@ -179,6 +179,31 @@ fn valid_entropy(entropy: &Zeroizing<[u8; 32]>) -> bool { res.into() } +fn from_internal( + language: Language, + masked_features: u8, + encoded_birthday: u16, + entropy: Zeroizing<[u8; 32]>, +) -> Result { + if !polyseed_features_supported(masked_features) { + Err(SeedError::UnsupportedFeatures)?; + } + + if !valid_entropy(&entropy) { + Err(SeedError::InvalidEntropy)?; + } + + let mut res = Polyseed { + language, + birthday: encoded_birthday, + features: masked_features, + entropy, + checksum: 0, + }; + res.checksum = poly_eval(&res.to_poly()); + Ok(res) +} + impl Polyseed { // TODO: Clean this fn to_poly(&self) -> Poly { @@ -226,20 +251,7 @@ impl Polyseed { birthday: u64, entropy: Zeroizing<[u8; 32]>, ) -> Result { - let features = user_features(features); - if !polyseed_features_supported(features) { - Err(SeedError::UnsupportedFeatures)?; - } - - let birthday = birthday_encode(birthday); - - if !valid_entropy(&entropy) { - Err(SeedError::InvalidEntropy)?; - } - - let mut res = Polyseed { language, birthday, features, entropy, checksum: 0 }; - res.checksum = poly_eval(&res.to_poly()); - Ok(res) + from_internal(language, user_features(features), birthday_encode(birthday), entropy) } /// Create a new `Polyseed`. @@ -375,9 +387,12 @@ impl Polyseed { let features = u8::try_from(extra >> DATE_BITS).expect("couldn't convert extra >> DATE_BITS to u8"); - let res = Polyseed::from(lang, features, birthday_decode(birthday), entropy); + let res = from_internal(lang, features, birthday, entropy); if let Ok(res) = res.as_ref() { - debug_assert_eq!(res.checksum, checksum); + if res.checksum != checksum { + // This should never trigger + Err(SeedError::InvalidSeed)?; + } } res } From c92c1fc0d99e728e4b9c9e3ae0a552b9e62a036c Mon Sep 17 00:00:00 2001 From: j-berman Date: Mon, 19 Feb 2024 17:19:26 -0800 Subject: [PATCH 2/4] PR comments --- coins/monero/src/tests/seed.rs | 1 + coins/monero/src/wallet/seed/polyseed.rs | 5 +---- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/coins/monero/src/tests/seed.rs b/coins/monero/src/tests/seed.rs index 7518d9dfd..0358bd89b 100644 --- a/coins/monero/src/tests/seed.rs +++ b/coins/monero/src/tests/seed.rs @@ -408,6 +408,7 @@ fn test_polyseed() { #[test] fn test_invalid_polyseed() { + // This seed includes unsupported features bits and should error on decode let seed = "include domain claim resemble urban hire lunch bird \ crucial fire best wife ring warm ignore model" .into(); diff --git a/coins/monero/src/wallet/seed/polyseed.rs b/coins/monero/src/wallet/seed/polyseed.rs index c7a5baa2b..d8877797a 100644 --- a/coins/monero/src/wallet/seed/polyseed.rs +++ b/coins/monero/src/wallet/seed/polyseed.rs @@ -389,10 +389,7 @@ impl Polyseed { let res = from_internal(lang, features, birthday, entropy); if let Ok(res) = res.as_ref() { - if res.checksum != checksum { - // This should never trigger - Err(SeedError::InvalidSeed)?; - } + debug_assert_eq!(res.checksum, checksum); } res } From d8950022edfe163efb6cc38b7fbebcb4ded7f6b9 Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Mon, 19 Feb 2024 21:40:29 -0500 Subject: [PATCH 3/4] Make from_internal a member of Polyseed --- coins/monero/src/wallet/seed/polyseed.rs | 55 ++++++++++++------------ 1 file changed, 27 insertions(+), 28 deletions(-) diff --git a/coins/monero/src/wallet/seed/polyseed.rs b/coins/monero/src/wallet/seed/polyseed.rs index d8877797a..66bb1edaa 100644 --- a/coins/monero/src/wallet/seed/polyseed.rs +++ b/coins/monero/src/wallet/seed/polyseed.rs @@ -178,32 +178,6 @@ fn valid_entropy(entropy: &Zeroizing<[u8; 32]>) -> bool { } res.into() } - -fn from_internal( - language: Language, - masked_features: u8, - encoded_birthday: u16, - entropy: Zeroizing<[u8; 32]>, -) -> Result { - if !polyseed_features_supported(masked_features) { - Err(SeedError::UnsupportedFeatures)?; - } - - if !valid_entropy(&entropy) { - Err(SeedError::InvalidEntropy)?; - } - - let mut res = Polyseed { - language, - birthday: encoded_birthday, - features: masked_features, - entropy, - checksum: 0, - }; - res.checksum = poly_eval(&res.to_poly()); - Ok(res) -} - impl Polyseed { // TODO: Clean this fn to_poly(&self) -> Poly { @@ -242,6 +216,31 @@ impl Polyseed { poly } + fn from_internal( + language: Language, + masked_features: u8, + encoded_birthday: u16, + entropy: Zeroizing<[u8; 32]>, + ) -> Result { + if !polyseed_features_supported(masked_features) { + Err(SeedError::UnsupportedFeatures)?; + } + + if !valid_entropy(&entropy) { + Err(SeedError::InvalidEntropy)?; + } + + let mut res = Polyseed { + language, + birthday: encoded_birthday, + features: masked_features, + entropy, + checksum: 0, + }; + res.checksum = poly_eval(&res.to_poly()); + Ok(res) + } + /// Create a new `Polyseed` with specific internals. /// /// `birthday` is defined in seconds since the Unix epoch. @@ -251,7 +250,7 @@ impl Polyseed { birthday: u64, entropy: Zeroizing<[u8; 32]>, ) -> Result { - from_internal(language, user_features(features), birthday_encode(birthday), entropy) + Self::from_internal(language, user_features(features), birthday_encode(birthday), entropy) } /// Create a new `Polyseed`. @@ -387,7 +386,7 @@ impl Polyseed { let features = u8::try_from(extra >> DATE_BITS).expect("couldn't convert extra >> DATE_BITS to u8"); - let res = from_internal(lang, features, birthday, entropy); + let res = Self::from_internal(lang, features, birthday, entropy); if let Ok(res) = res.as_ref() { debug_assert_eq!(res.checksum, checksum); } From 850abab7f96f48bbd32c771d5778b648af7b14fb Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Mon, 19 Feb 2024 22:02:00 -0500 Subject: [PATCH 4/4] Add accidentally removed newline --- coins/monero/src/wallet/seed/polyseed.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/coins/monero/src/wallet/seed/polyseed.rs b/coins/monero/src/wallet/seed/polyseed.rs index 66bb1edaa..69b60d984 100644 --- a/coins/monero/src/wallet/seed/polyseed.rs +++ b/coins/monero/src/wallet/seed/polyseed.rs @@ -178,6 +178,7 @@ fn valid_entropy(entropy: &Zeroizing<[u8; 32]>) -> bool { } res.into() } + impl Polyseed { // TODO: Clean this fn to_poly(&self) -> Poly {