diff --git a/Cargo.lock b/Cargo.lock index 79f9d98f..6dfdc5e2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -67,6 +67,21 @@ dependencies = [ "memchr", ] +[[package]] +name = "android-tzdata" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e999941b234f3131b00bc13c22d06e8c5ff726d1b6318ac7eb276997bbb4fef0" + +[[package]] +name = "android_system_properties" +version = "0.1.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "819e7219dbd41043ac279b19830f2efc897156490d7fd6ea916720117ee66311" +dependencies = [ + "libc", +] + [[package]] name = "anstream" version = "0.6.15" @@ -443,6 +458,21 @@ version = "0.22.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "72b3254f16251a8381aa12e40e3c4d2f0199f8c6508fbecb9d91f575e0fbb8c6" +[[package]] +name = "bit-set" +version = "0.5.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0700ddab506f33b20a03b13996eccd309a48e5ff77d0d95926aa0210fb4e95f1" +dependencies = [ + "bit-vec", +] + +[[package]] +name = "bit-vec" +version = "0.6.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "349f9b6a179ed607305526ca489b34ad0a41aed5f7980fa90eb03160b69598fb" + [[package]] name = "bitflags" version = "1.3.2" @@ -592,6 +622,20 @@ version = "1.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "17cc5e6b5ab06331c33589842070416baa137e8b0eb912b008cfd4a78ada7919" +[[package]] +name = "chrono" +version = "0.4.38" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a21f936df1771bf62b77f047b726c4625ff2e8aa607c01ec06e5a05bd8463401" +dependencies = [ + "android-tzdata", + "iana-time-zone", + "js-sys", + "num-traits", + "wasm-bindgen", + "windows-targets 0.52.6", +] + [[package]] name = "cipher" version = "0.4.4" @@ -920,6 +964,41 @@ dependencies = [ "syn 2.0.76", ] +[[package]] +name = "darling" +version = "0.20.10" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6f63b86c8a8826a49b8c21f08a2d07338eec8d900540f8630dc76284be802989" +dependencies = [ + "darling_core", + "darling_macro", +] + +[[package]] +name = "darling_core" +version = "0.20.10" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "95133861a8032aaea082871032f5815eb9e98cef03fa916ab4500513994df9e5" +dependencies = [ + "fnv", + "ident_case", + "proc-macro2", + "quote", + "strsim", + "syn 2.0.76", +] + +[[package]] +name = "darling_macro" +version = "0.20.10" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d336a2a514f6ccccaa3e09b02d41d35330c07ddf03a62165fcec10bb561c7806" +dependencies = [ + "darling_core", + "quote", + "syn 2.0.76", +] + [[package]] name = "data-encoding" version = "2.6.0" @@ -946,6 +1025,37 @@ dependencies = [ "syn 2.0.76", ] +[[package]] +name = "derive_builder" +version = "0.20.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0350b5cb0331628a5916d6c5c0b72e97393b8b6b03b47a9284f4e7f5a405ffd7" +dependencies = [ + "derive_builder_macro", +] + +[[package]] +name = "derive_builder_core" +version = "0.20.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d48cda787f839151732d396ac69e3473923d54312c070ee21e9effcaa8ca0b1d" +dependencies = [ + "darling", + "proc-macro2", + "quote", + "syn 2.0.76", +] + +[[package]] +name = "derive_builder_macro" +version = "0.20.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "206868b8242f27cecce124c19fd88157fbd0dd334df2587f36417bafbc85097b" +dependencies = [ + "derive_builder_core", + "syn 2.0.76", +] + [[package]] name = "derive_more" version = "1.0.0" @@ -1117,6 +1227,17 @@ dependencies = [ "once_cell", ] +[[package]] +name = "fancy-regex" +version = "0.13.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "531e46835a22af56d1e3b66f04844bed63158bc094a628bec1d321d9b4c44bf2" +dependencies = [ + "bit-set", + "regex-automata", + "regex-syntax", +] + [[package]] name = "fastrand" version = "1.9.0" @@ -1510,6 +1631,35 @@ dependencies = [ "serde", ] +[[package]] +name = "iana-time-zone" +version = "0.1.60" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e7ffbb5a1b541ea2561f8c41c087286cc091e21e556a4f09a8f6cbf17b69b141" +dependencies = [ + "android_system_properties", + "core-foundation-sys", + "iana-time-zone-haiku", + "js-sys", + "wasm-bindgen", + "windows-core", +] + +[[package]] +name = "iana-time-zone-haiku" +version = "0.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f31827a206f56af32e590ba56d5d2d085f558508192593743f16b2306495269f" +dependencies = [ + "cc", +] + +[[package]] +name = "ident_case" +version = "1.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b9e0384b61958566e926dc50660321d12159025e767c18e043daf26b70104c39" + [[package]] name = "idna" version = "0.5.0" @@ -1607,6 +1757,15 @@ version = "1.70.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7943c866cc5cd64cbc25b2e01621d07fa8eb2a1a23160ee81ce38704e97b8ecf" +[[package]] +name = "itertools" +version = "0.13.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "413ee7dfc52ee1a4949ceeb7dbc8a33f2d6c088194d9f922fb8318faf1f01186" +dependencies = [ + "either", +] + [[package]] name = "itoa" version = "1.0.11" @@ -1741,6 +1900,7 @@ dependencies = [ "url", "wasm-timer", "ws_stream_wasm", + "zxcvbn", ] [[package]] @@ -3525,6 +3685,15 @@ version = "0.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "712e227841d057c1ee1cd2fb22fa7e5a5461ae8e48fa2ca79ec42cfc1931183f" +[[package]] +name = "windows-core" +version = "0.52.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "33ab640c8d7e35bf8ba19b884ba838ceb4fba93a4e8c65a9059d08afcfc683d9" +dependencies = [ + "windows-targets 0.52.6", +] + [[package]] name = "windows-sys" version = "0.48.0" @@ -3800,3 +3969,20 @@ dependencies = [ "quote", "syn 2.0.76", ] + +[[package]] +name = "zxcvbn" +version = "3.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ad76e35b00ad53688d6b90c431cabe3cbf51f7a4a154739e04b63004ab1c736c" +dependencies = [ + "chrono", + "derive_builder", + "fancy-regex", + "itertools", + "lazy_static", + "regex", + "time", + "wasm-bindgen", + "web-sys", +] diff --git a/Cargo.toml b/Cargo.toml index 5a414160..341bf92c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -58,6 +58,7 @@ thiserror = "1.0.24" time = "0.3.7" trycmd = "0.15" url = "2.2.2" +zxcvbn = "3.1.0" [package] name = "magic-wormhole" @@ -100,6 +101,7 @@ thiserror = { workspace = true } futures = { workspace = true } url = { workspace = true, features = ["serde"] } percent-encoding = { workspace = true } +zxcvbn = { workspace = true, optional = true } # Transit dependencies @@ -143,6 +145,8 @@ eyre = { workspace = true } [features] +# Check the entropy of custom codes. Will fail for any weak passwords. +entropy = ["zxcvbn"] transfer = ["transit", "dep:tar", "dep:rmp-serde"] transit = [ "dep:noise-rust-crypto", diff --git a/cli/Cargo.toml b/cli/Cargo.toml index 9835b8a3..a22654a8 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -28,7 +28,7 @@ async-std = { workspace = true, features = ["attributes", "unstable"] } rand = { workspace = true } # CLI specific dependencies -magic-wormhole = { path = "..", version = "0.7", features = ["all"] } +magic-wormhole = { path = "..", version = "0.7", features = ["all", "entropy"] } clap = { workspace = true, features = ["cargo", "derive", "help"] } clap_complete = { workspace = true } env_logger = { workspace = true } @@ -39,7 +39,9 @@ color-eyre = { workspace = true } number_prefix = { workspace = true } ctrlc = { workspace = true } qr2term = { workspace = true } -arboard = { workspace = true, features = ["wayland-data-control"] } # Wayland by default, fallback to X11. +arboard = { workspace = true, features = [ + "wayland-data-control", +] } # Wayland by default, fallback to X11. [dev-dependencies] trycmd = { workspace = true } diff --git a/cli/src/main.rs b/cli/src/main.rs index 0ae66b90..e31b09b8 100644 --- a/cli/src/main.rs +++ b/cli/src/main.rs @@ -603,13 +603,26 @@ async fn parse_and_connect( } // TODO: Apply this change to all usages after an API break - // Check if an interactive terminal is connected - let code: Option = if std::io::stdin().is_terminal() { - // We accept a little breakage in non-interactive use, because this is a security issue - code.map(|c| c.parse()).transpose()? - } else { - // We run as a script. Only output an error - code.map(|c| c.into()) + // https://github.com/magic-wormhole/magic-wormhole.rs/issues/193 + // We accept a little breakage in non-interactive use, because this is a security issue + // 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 = { + match res { + Some(Ok(code)) => Some(code), + // Check if an interactive terminal is connected + Some(Err(err)) if std::io::stdin().is_terminal() => { + // Only fail for the case where the password is < 4 characters. + // Anything else will just print an error for now. + return Err(err.into()); + }, + Some(Err(_)) => { + // The library crate already emits an error log for this. + code.map(|c| c.into()) + }, + None => None, + } }; /* We need to track that information for when we generate a QR code */ diff --git a/src/core.rs b/src/core.rs index 2b3075d3..525a8d16 100644 --- a/src/core.rs +++ b/src/core.rs @@ -827,17 +827,53 @@ impl AsRef for Nameplate { } } +/// This is a compromise. Generally we want to be wordlist-agnostic here. But +/// we can't ignore that the PGP wordlist is the most common wordlist in use. +/// +/// - The shortest word in the PGP wordlist is 4 characters long. The longest +/// word is 9 characters. This means we can't limit to more than 9 bytes here, +/// 'solo-orca' is 9 bytes, and we want to allow 2-word codes. +/// - A 9 character random password is very strong. If it is only comprised of +/// uniformly distributed lowercase ASCII characters, we have an entropy of +/// 26^9 >= 40 bits. This is much more than the default 16 bits we get from two +/// words from the PGP wordlist. +/// - An emoji contains at least 2 bytes of data. So two emoji are actually +/// about the same security as two words from the PGP wordlist. +/// +/// We ultimately can't protect people from making bad choices, as entropy is a +/// very difficult thing. What we can do instead is offer guidance, by printing +/// warnings in case of rather short passwords, and making people choose for +/// themselves whether their privacy is worth it to them choosing longer codes. #[derive(PartialEq, Eq, PartialOrd, Ord, Clone, Debug, Copy, derive_more::Display, Error)] -#[display("Password too short. Must be at least 4 bytes")] #[non_exhaustive] -pub struct ParsePasswordError {} +pub enum ParsePasswordError { + #[display("Password too short. It is only {value} bytes, but must be at least {required}")] + TooShort { value: usize, required: usize }, + #[display( + "Password too weak. It can be guessed with an average of {value} tries, but must be at least {required}" + )] + LittleEntropy { value: u64, required: u64 }, +} /// Wormhole codes look like 4-purple-sausages, consisting of a number followed by some random words. /// This number is called a "Nameplate", the rest is called the `Password` -#[derive(PartialEq, Eq, Clone, Debug, Deserialize, Serialize, derive_more::Display)] +#[derive(Clone, Debug, Serialize, derive_more::Display)] #[serde(transparent)] -#[display("{}", _0)] -pub struct Password(String); +#[display("{password}")] +pub struct Password { + password: String, + #[serde(skip)] + #[cfg(feature = "entropy")] + entropy: zxcvbn::Entropy, +} + +impl PartialEq for Password { + fn eq(&self, other: &Self) -> bool { + self.password == other.password + } +} + +impl Eq for Password {} impl Password { /// Create a new password from a string. Does not check the entropy of the password. @@ -845,30 +881,70 @@ impl Password { /// 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 { - Password(n.into()) + let password = n.into(); + #[cfg(feature = "entropy")] + let entropy = Self::calculate_entropy(&password); + + Password { + password, + #[cfg(feature = "entropy")] + entropy, + } + } + + #[cfg(feature = "entropy")] + fn calculate_entropy(password: &str) -> zxcvbn::Entropy { + static PGP_WORDLIST: std::sync::OnceLock> = std::sync::OnceLock::new(); + let words = PGP_WORDLIST.get_or_init(|| { + // TODO: We leak the str: https://github.com/shssoichiro/zxcvbn-rs/issues/87 + crate::core::wordlist::default_wordlist(2) + .into_words() + .map(|s| &*s.leak()) + .collect::>() + }); + + zxcvbn::zxcvbn(password, &words[..]) } } impl From for String { fn from(value: Password) -> Self { - value.0 + value.password } } impl AsRef for Password { fn as_ref(&self) -> &str { - &self.0 + &self.password } } impl FromStr for Password { type Err = ParsePasswordError; - fn from_str(pass: &str) -> Result { - if pass.len() >= 4 { - Ok(Self(pass.to_string())) + fn from_str(password: &str) -> Result { + let password = password.to_string(); + + if password.len() < 4 { + Err(ParsePasswordError::TooShort { + value: password.len(), + required: 4, + }) } else { - Err(ParsePasswordError {}) + #[cfg(feature = "entropy")] + return { + let entropy = Self::calculate_entropy(&password); + if entropy.guesses() < 2_u64.pow(16) { + return Err(ParsePasswordError::LittleEntropy { + value: entropy.guesses(), + required: 2_u64.pow(16), + }); + } + Ok(Self { password, entropy }) + }; + + #[cfg(not(feature = "entropy"))] + Ok(Self { password }) } } } diff --git a/src/core/wordlist.rs b/src/core/wordlist.rs index 966d82ef..d20e6acf 100644 --- a/src/core/wordlist.rs +++ b/src/core/wordlist.rs @@ -66,6 +66,11 @@ impl Wordlist { .collect(); components.join("-") } + + #[cfg(feature = "entropy")] + pub(crate) fn into_words(self) -> impl Iterator { + self.words.into_iter().flatten() + } } fn load_pgpwords() -> Vec> {