Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

monero: only mask user features on new polyseed, not on decode #503

Merged
merged 4 commits into from
Feb 20, 2024
Merged

monero: only mask user features on new polyseed, not on decode #503

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c30b6be
monero: only mask user features on new polyseed, not on decode
j-berman Jan 3, 2024
c92c1fc
PR comments
j-berman Feb 20, 2024
d895002
Make from_internal a member of Polyseed
kayabaNerve Feb 20, 2024
850abab
Add accidentally removed newline
kayabaNerve Feb 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion coins/monero/src/tests/seed.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
Expand Down Expand Up @@ -405,3 +405,12 @@ fn test_polyseed() {
}
}
}

#[test]
fn test_invalid_polyseed() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd clarify how it's invalid.

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));
}
47 changes: 31 additions & 16 deletions coins/monero/src/wallet/seed/polyseed.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,31 @@ fn valid_entropy(entropy: &Zeroizing<[u8; 32]>) -> bool {
res.into()
}

fn from_internal(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to have this outside the Polyseed impl?

Copy link
Contributor Author

@j-berman j-berman Jan 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from_string will first decode the string and yield the already masked features before passing it over to from_internal. The features shouldn't be masked again (the primary bug this PR is fixing).

from expects an unmasked features, which it then masks for the caller. I matched polyseed_create from the polyseed spec for this behavior, which also takes in unmasked features and masks it for the caller. Source: https://github.com/tevador/polyseed/blob/b7c35bb3c6b91e481ecb04fc235eaff69c507fa1/src/polyseed.c#L61 . I figured it would make sense that monero-serai's from would match the polyseed spec for this behavior since it's the only exposed function that a caller can pass features into.

Because of the above 2 distinct call paths, I pulled out the from_internal and had it take in the already masked features.

Sort of similar line of thinking for the birthday btw (from_string yields the encoded birthday so it's unnecessary to decode it, whereas from expects the decoded birthday which needs to be encoded).

language: Language,
masked_features: u8,
encoded_birthday: u16,
entropy: Zeroizing<[u8; 32]>,
) -> Result<Polyseed, SeedError> {
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 {
Expand Down Expand Up @@ -226,20 +251,7 @@ impl Polyseed {
birthday: u64,
entropy: Zeroizing<[u8; 32]>,
) -> Result<Polyseed, SeedError> {
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`.
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this should never trigger, the debug_assert is valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, why should it be a debug assert and not a normal assert? I see no reason the function should return a normal response in a non-debug build if this statement is true. The seed would be invalid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess debug assert in one line, then return the invalid seed error in the next could make sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it should be impossible, and is accordingly redundant, so it should be optimized out on release.

Err(SeedError::InvalidSeed)?;
}
}
res
}
Expand Down