From de5e2960d57849a5efd04f83905a5f0e64602933 Mon Sep 17 00:00:00 2001 From: Fina Wilke Date: Sat, 7 Sep 2024 18:49:04 +0200 Subject: [PATCH] core: Remove unsafe methods when entropy feature is enabled --- CHANGELOG.md | 1 + cli/src/main.rs | 50 +++++++++++++++++++++++++----------------- src/core.rs | 46 +++++++++++++++++++++++++++++++++----- src/core/rendezvous.rs | 3 +-- src/core/test.rs | 7 +++--- src/lib.rs | 2 +- src/uri.rs | 23 +++++++++---------- 7 files changed, 88 insertions(+), 44 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ce5f553e..795944a8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - \[all\] Code words with a weak password section or a non-integer nameplate will throw an error in the long. This error can be upgraded to a hard error by enabling the "entropy" feature. This feature will become the default in the next major release. - \[lib\] Implemented FromStr for `Code` and `Nameplate` - \[lib\] Added new checked type for the `Password` section of a wormhole code +- \[lib\] Added new `entropy` feature. When enabled, the entropy of the passed password will be checked on creation. This will change the signature of `MailboxConnection::create_with_password` to require the password to be passed via the new `Password` wrapper type. - \[lib\]\[deprecated\] Deprecated the `Code` and `Nameplate` `From>` implementations and `new()` methods. They are unchecked and will print a warning for now. These will be removed in the next breaking release. ## [0.7.1] - 2024-07-25 diff --git a/cli/src/main.rs b/cli/src/main.rs index 4b80c8a8..a79de164 100644 --- a/cli/src/main.rs +++ b/cli/src/main.rs @@ -608,27 +608,37 @@ async fn parse_and_connect( // Split the nameplate parsing from the code parsing to ensure we allow non-integer nameplates // until the next breaking release let res: Option> = code.as_ref().map(|c| c.parse()); - let code: Option = + let code: Option = match res { + Some(Ok(code)) => Some(code), + // Check if an interactive terminal is connected + Some(Err( + err @ (ParseCodeError::SeparatorMissing + | ParseCodeError::Password(ParsePasswordError::TooShort { .. })), + )) => { + // Only fail for the case where the password is < 4 characters. + // Anything else will just print an error for now. + return Err(err.into()); + }, + // If we have an interactive terminal connected, also fail for low entropy + Some(Err(err @ ParseCodeError::Password(ParsePasswordError::LittleEntropy { .. }))) + if std::io::stdin().is_terminal() => { - match res { - Some(Ok(code)) => Some(code), - // Check if an interactive terminal is connected - Some(Err(err @ ParseCodeError::Password(ParsePasswordError::TooShort { .. }))) => { - // Only fail for the case where the password is < 4 characters. - // Anything else will just print an error for now. - return Err(err.into()); - }, - // If we have an interactive terminal connected, also fail for low entropy - Some(Err( - err @ ParseCodeError::Password(ParsePasswordError::LittleEntropy { .. }), - )) if std::io::stdin().is_terminal() => return Err(err.into()), - Some(Err(_)) => { - // The library crate already emits an error log for this. - code.map(|c| c.into()) - }, - None => None, - } - }; + return Err(err.into()) + }, + Some(Err(err)) => { + tracing::error!("{} This will fail in the next release.", err); + code.map(|c| { + let (nameplate, password) = c.split_once("-").unwrap(); + unsafe { + magic_wormhole::Code::from_components( + magic_wormhole::Nameplate::new_unchecked(nameplate), + magic_wormhole::Password::new_unchecked(password), + ) + } + }) + }, + None => None, + }; /* We need to track that information for when we generate a QR code */ let mut uri_rendezvous = None; diff --git a/src/core.rs b/src/core.rs index ffccc1e0..ca73dd12 100644 --- a/src/core.rs +++ b/src/core.rs @@ -162,9 +162,12 @@ impl MailboxConnection { /// TODO: Replace this with create_with_validated_password pub async fn create_with_password( config: AppConfig, - password: &str, + #[cfg(not(feature = "entropy"))] password: &str, + #[cfg(feature = "entropy")] password: Password, ) -> Result { + #[cfg(not(feature = "entropy"))] let password = password.parse().map_err(ParseCodeError::from)?; + Self::create_with_validated_password(config, password).await } @@ -790,10 +793,19 @@ pub struct ParseNameplateError {} #[serde(transparent)] #[deref(forward)] #[display("{}", _0)] +#[cfg(not(feature = "entropy"))] pub struct Nameplate( #[deprecated(since = "0.7.0", note = "use the AsRef implementation")] pub String, ); +/// Wormhole codes look like 4-purple-sausages, consisting of a number followed by some random words. +/// This number is called a "Nameplate". +#[derive(PartialEq, Eq, Clone, Debug, Deserialize, Serialize, derive_more::Display)] +#[serde(transparent)] +#[display("{}", _0)] +#[cfg(feature = "entropy")] +pub struct Nameplate(String); + #[allow(deprecated)] impl Nameplate { /// Create a new nameplate from a string. @@ -803,6 +815,7 @@ impl Nameplate { since = "0.7.2", note = "Nameplates will be required to be numbers soon. Use the [std::str::FromStr] implementation" )] + #[cfg(not(feature = "entropy"))] pub fn new(n: impl Into) -> Self { let nameplate = n.into(); if let Err(err) = Nameplate::from_str(&nameplate) { @@ -815,8 +828,12 @@ impl Nameplate { } } + /// Create a new nameplate from a string. + /// + /// Safety: Nameplate will be [required to be numbers](https://github.com/magic-wormhole/magic-wormhole-mailbox-server/issues/45) soon. #[allow(unsafe_code)] - unsafe fn new_unchecked(n: impl Into) -> Self { + #[doc(hidden)] + pub unsafe fn new_unchecked(n: &str) -> Self { Nameplate(n.into()) } } @@ -845,6 +862,7 @@ impl From for String { /// /// Does not check if the nameplate is a number. This may be incompatible. #[allow(deprecated)] +#[cfg(not(feature = "entropy"))] impl From for Nameplate { fn from(value: String) -> Self { tracing::debug!( @@ -930,9 +948,12 @@ impl Eq for Password {} impl Password { /// Create a new password from a string. Does not check the entropy of the password. /// + /// You should use [`Password::from_str`] / [`String::parse`] instead. + /// /// Safety: Does not check the entropy of the password, or if one exists at all. This can be a security risk. #[allow(unsafe_code)] - unsafe fn new_unchecked(n: impl Into) -> Self { + #[doc(hidden)] + pub unsafe fn new_unchecked(n: impl Into) -> Self { let password = n.into(); #[cfg(feature = "entropy")] let entropy = Self::calculate_entropy(&password); @@ -1024,10 +1045,22 @@ pub enum ParseCodeError { */ #[derive(PartialEq, Eq, Clone, Debug, derive_more::Display, derive_more::Deref)] #[display("{}", _0)] +#[cfg(not(feature = "entropy"))] pub struct Code( #[deprecated(since = "0.7.0", note = "use the std::fmt::Display implementation")] pub String, ); +/** A wormhole code à la 15-foo-bar + * + * The part until the first dash is called the "nameplate" and is purely numeric. + * The rest is the password and may be arbitrary, although dash-joining words from + * a wordlist is a common convention. + */ +#[derive(PartialEq, Eq, Clone, Debug, derive_more::Display)] +#[display("{}", _0)] +#[cfg(feature = "entropy")] +pub struct Code(String); + #[allow(deprecated)] impl Code { /// Create a new code, comprised of a [`Nameplate`] and a password. @@ -1037,6 +1070,7 @@ impl Code { since = "0.7.2", note = "Use [`from_components`] or the [std::str::FromStr] implementation" )] + #[cfg(not(feature = "entropy"))] pub fn new(nameplate: &Nameplate, password: &str) -> Self { if let Err(err) = Password::from_str(password) { tracing::error!("{err}"); @@ -1057,7 +1091,8 @@ impl Code { #[deprecated(since = "0.7.2", note = "Use [Self::nameplate] and [Self::password]")] pub fn split(&self) -> (Nameplate, String) { let mut iter = self.0.splitn(2, '-'); - let nameplate = Nameplate::new(iter.next().unwrap()); + #[allow(unsafe_code)] + let nameplate = unsafe { Nameplate::new_unchecked(iter.next().unwrap()) }; let password = iter.next().unwrap(); (nameplate, password.to_string()) } @@ -1092,6 +1127,7 @@ impl From for String { /// Deprecated: Use the [`std::str::FromStr`] implementation /// /// Safety: Does not check the entropy of the password, or if one exists at all. This can be a security risk. +#[cfg(not(feature = "entropy"))] impl From for Code { fn from(value: String) -> Self { tracing::debug!( @@ -1112,8 +1148,8 @@ impl FromStr for Code { fn from_str(s: &str) -> Result { match s.split_once('-') { Some((n, p)) => { - let nameplate: Nameplate = n.parse()?; let password: Password = p.parse()?; + let nameplate: Nameplate = n.parse()?; Ok(Self(format!("{}-{}", nameplate, password))) }, diff --git a/src/core/rendezvous.rs b/src/core/rendezvous.rs index d8ecbc47..ac372f96 100644 --- a/src/core/rendezvous.rs +++ b/src/core/rendezvous.rs @@ -572,8 +572,7 @@ impl RendezvousServer { .and_then(|state| state.nameplate.clone()) .expect("Can only release an allocated nameplate, and only once"); - use std::ops::Deref; - self.send_message(&OutboundMessage::release(nameplate.deref().deref())) + self.send_message(&OutboundMessage::release(nameplate.as_ref())) .await?; match self.receive_reply().await? { RendezvousReply::Released => (), diff --git a/src/core/test.rs b/src/core/test.rs index d884cfac..22991264 100644 --- a/src/core/test.rs +++ b/src/core/test.rs @@ -1,6 +1,9 @@ #![allow(irrefutable_let_patterns)] use super::{Mood, Phase}; +use rand::Rng; +use std::{borrow::Cow, str::FromStr, time::Duration}; + #[cfg(feature = "transfer")] use crate::transfer; use crate::{ @@ -8,8 +11,6 @@ use crate::{ core::{MailboxConnection, Nameplate}, transit, AppConfig, AppID, Code, Wormhole, WormholeError, }; -use rand::Rng; -use std::{borrow::Cow, time::Duration}; use test_log::test; pub const TEST_APPID: AppID = AppID(std::borrow::Cow::Borrowed( @@ -576,7 +577,7 @@ pub async fn test_connect_with_code_expecting_nameplate() -> eyre::Result<()> { fn generate_random_code() -> Code { let mut rng = rand::thread_rng(); let nameplate_string = format!("{}-guitarist-revenge", rng.gen_range(1000..10000)); - let nameplate = Nameplate::new(&nameplate_string); + let nameplate = Nameplate::from_str(&nameplate_string).unwrap(); Code::from_components(nameplate, "guitarist-revenge".parse().unwrap()) } diff --git a/src/lib.rs b/src/lib.rs index b8e35bfb..bd0438e1 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -41,5 +41,5 @@ pub mod uri; pub use crate::core::{ key::{GenericKey, Key, KeyPurpose, WormholeKey}, rendezvous, AppConfig, AppID, Code, MailboxConnection, Mood, Nameplate, ParseCodeError, - ParseNameplateError, ParsePasswordError, Wormhole, WormholeError, WormholeWelcome, + ParseNameplateError, ParsePasswordError, Password, Wormhole, WormholeError, WormholeWelcome, }; diff --git a/src/uri.rs b/src/uri.rs index acca9b61..6f5e9f83 100644 --- a/src/uri.rs +++ b/src/uri.rs @@ -39,6 +39,9 @@ pub enum ParseError { #[source] std::str::Utf8Error, ), + /// Error parsing the mailbox code + #[error("Error parsing the mailbox code")] + ParseCodeError(#[from] ParseCodeError), } /// The wormhole-transfer URI Scheme is used to encode a wormhole code for file transfer as a URI. @@ -100,15 +103,9 @@ impl TryFrom<&url::Url> for WormholeTransferUri { "follower" => false, invalid => return Err(ParseError::InvalidRole(invalid.into())), }; - let code = Code( - percent_encoding::percent_decode_str(url.path()) - .decode_utf8()? - .into(), - ); - // TODO move the code validation to somewhere else and also add more checks - if code.is_empty() { - return Err(ParseError::MissingCode); - } + let code: Code = percent_encoding::percent_decode_str(url.path()) + .decode_utf8()? + .parse()?; Ok(WormholeTransferUri { code, @@ -137,7 +134,7 @@ impl std::str::FromStr for WormholeTransferUri { impl From<&WormholeTransferUri> for url::Url { fn from(val: &WormholeTransferUri) -> Self { let mut url = url::Url::parse("wormhole-transfer:").unwrap(); - url.set_path(&val.code); + url.set_path(val.code.as_ref()); /* Only do this if there are any query parameteres at all, otherwise the URL will have an ugly trailing '?'. */ if val.rendezvous_server.is_some() || val.is_leader { let mut query = url.query_pairs_mut(); @@ -171,18 +168,18 @@ mod test { #[test] fn test_uri() { test_eq( - WormholeTransferUri::new(Code("4-hurricane-equipment".to_owned())), + WormholeTransferUri::new("4-hurricane-equipment".parse().unwrap()), "wormhole-transfer:4-hurricane-equipment", ); test_eq( - WormholeTransferUri::new(Code("8-🙈-🙉-🙊".to_owned())), + WormholeTransferUri::new("8-🙈-🙉-🙊".parse().unwrap()), "wormhole-transfer:8-%F0%9F%99%88-%F0%9F%99%89-%F0%9F%99%8A", ); test_eq( WormholeTransferUri { - code: Code("8-🙈-🙉-🙊".to_owned()), + code: "8-🙈-🙉-🙊".parse().unwrap(), rendezvous_server: Some(url::Url::parse("ws://localhost:4000").unwrap()), is_leader: true, },