monero: only mask user features on new polyseed, not on decode #503
Conversation
- 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
This is a feature, not a bug. |
| @@ -405,3 +405,12 @@ fn test_polyseed() { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| #[test] | |||
| fn test_invalid_polyseed() { | |||
There was a problem hiding this comment.
I'd clarify how it's invalid.
| @@ -179,6 +179,31 @@ fn valid_entropy(entropy: &Zeroizing<[u8; 32]>) -> bool { | |||
| res.into() | |||
| } | |||
|
|
|||
| fn from_internal( | |||
There was a problem hiding this comment.
Is there a reason to have this outside the Polyseed impl?
There was a problem hiding this comment.
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).
| if let Ok(res) = res.as_ref() { | ||
| debug_assert_eq!(res.checksum, checksum); | ||
| if res.checksum != checksum { | ||
| // This should never trigger |
There was a problem hiding this comment.
If this should never trigger, the debug_assert is valid.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I guess debug assert in one line, then return the invalid seed error in the next could make sense.
There was a problem hiding this comment.
Because it should be impossible, and is accordingly redundant, so it should be optimized out on release.
* 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 * PR comments * Make from_internal a member of Polyseed * Add accidentally removed newline --------- Co-authored-by: Luke Parker <[email protected]>
Polyseed::from_stringcould return an incorrect successful response)Polyseed reference impl for feature masking: