diff --git a/age/CHANGELOG.md b/age/CHANGELOG.md index 072dc72c..2267301f 100644 --- a/age/CHANGELOG.md +++ b/age/CHANGELOG.md @@ -10,6 +10,14 @@ to 1.0.0 are beta releases. ## [Unreleased] +## [0.6.1, 0.7.2, 0.8.2, 0.9.3] - 2024-11-18 +### Security +- The age plugin protocol previously allowed plugin names that could be + interpreted as file paths. Under certain conditions, this could lead to a + different binary being executed as an age plugin than intended. Plugin names + are now required to only contain alphanumeric characters or the four special + characters `+-._`. + ## [0.10.0] - 2024-02-04 ### Added - Russian translation! diff --git a/age/src/plugin.rs b/age/src/plugin.rs index 2c00203d..8e2e8873 100644 --- a/age/src/plugin.rs +++ b/age/src/plugin.rs @@ -41,6 +41,13 @@ const CMD_FILE_KEY: &str = "file-key"; const ONE_HUNDRED_MS: Duration = Duration::from_millis(100); const TEN_SECONDS: Duration = Duration::from_secs(10); +#[inline] +fn valid_plugin_name(plugin_name: &str) -> bool { + plugin_name + .bytes() + .all(|b| b.is_ascii_alphanumeric() | matches!(b, b'+' | b'-' | b'.' | b'_')) +} + fn binary_name(plugin_name: &str) -> String { format!("age-plugin-{}", plugin_name) } @@ -102,10 +109,15 @@ impl std::str::FromStr for Recipient { if hrp.len() > PLUGIN_RECIPIENT_PREFIX.len() && hrp.starts_with(PLUGIN_RECIPIENT_PREFIX) { - Ok(Recipient { - name: hrp.split_at(PLUGIN_RECIPIENT_PREFIX.len()).1.to_owned(), - recipient: s.to_owned(), - }) + let name = hrp.split_at(PLUGIN_RECIPIENT_PREFIX.len()).1.to_owned(); + if valid_plugin_name(&name) { + Ok(Recipient { + name, + recipient: s.to_owned(), + }) + } else { + Err("invalid plugin name") + } } else { Err("invalid HRP") } @@ -146,14 +158,20 @@ impl std::str::FromStr for Identity { if hrp.len() > PLUGIN_IDENTITY_PREFIX.len() && hrp.starts_with(PLUGIN_IDENTITY_PREFIX) { - Ok(Identity { - name: hrp - .split_at(PLUGIN_IDENTITY_PREFIX.len()) - .1 - .trim_end_matches('-') - .to_owned(), - identity: s.to_owned(), - }) + // TODO: Decide whether to allow plugin names to end in - + let name = hrp + .split_at(PLUGIN_IDENTITY_PREFIX.len()) + .1 + .trim_end_matches('-') + .to_owned(); + if valid_plugin_name(&name) { + Ok(Identity { + name, + identity: s.to_owned(), + }) + } else { + Err("invalid plugin name") + } } else { Err("invalid HRP") } @@ -169,16 +187,25 @@ impl fmt::Display for Identity { impl Identity { /// Returns the identity corresponding to the given plugin name in its default mode. + /// + /// # Panics + /// + /// Panics if `plugin_name` contains invalid characters. pub fn default_for_plugin(plugin_name: &str) -> Self { - bech32::encode( - &format!("{}{}-", PLUGIN_IDENTITY_PREFIX, plugin_name), - [], - Variant::Bech32, - ) - .expect("HRP is valid") - .to_uppercase() - .parse() - .unwrap() + if valid_plugin_name(plugin_name) { + bech32::encode( + &format!("{}{}-", PLUGIN_IDENTITY_PREFIX, plugin_name), + [], + Variant::Bech32, + ) + .expect("HRP is valid") + .to_uppercase() + .parse() + .unwrap() + } else { + // TODO: Change the API to be fallible. + panic!("invalid plugin name") + } } /// Returns the plugin name for this identity. @@ -357,22 +384,28 @@ impl RecipientPluginV1 { identities: &[Identity], callbacks: C, ) -> Result { - Plugin::new(plugin_name) - .map_err(|binary_name| EncryptError::MissingPlugin { binary_name }) - .map(|plugin| RecipientPluginV1 { - plugin, - recipients: recipients - .iter() - .filter(|r| r.name == plugin_name) - .cloned() - .collect(), - identities: identities - .iter() - .filter(|r| r.name == plugin_name) - .cloned() - .collect(), - callbacks, + if valid_plugin_name(plugin_name) { + Plugin::new(plugin_name) + .map_err(|binary_name| EncryptError::MissingPlugin { binary_name }) + .map(|plugin| RecipientPluginV1 { + plugin, + recipients: recipients + .iter() + .filter(|r| r.name == plugin_name) + .cloned() + .collect(), + identities: identities + .iter() + .filter(|r| r.name == plugin_name) + .cloned() + .collect(), + callbacks, + }) + } else { + Err(EncryptError::MissingPlugin { + binary_name: plugin_name.to_string(), }) + } } } @@ -527,17 +560,23 @@ impl IdentityPluginV1 { identities: &[Identity], callbacks: C, ) -> Result { - Plugin::new(plugin_name) - .map_err(|binary_name| DecryptError::MissingPlugin { binary_name }) - .map(|plugin| IdentityPluginV1 { - plugin, - identities: identities - .iter() - .filter(|r| r.name == plugin_name) - .cloned() - .collect(), - callbacks, + if valid_plugin_name(plugin_name) { + Plugin::new(plugin_name) + .map_err(|binary_name| DecryptError::MissingPlugin { binary_name }) + .map(|plugin| IdentityPluginV1 { + plugin, + identities: identities + .iter() + .filter(|r| r.name == plugin_name) + .cloned() + .collect(), + callbacks, + }) + } else { + Err(DecryptError::MissingPlugin { + binary_name: plugin_name.to_string(), }) + } } fn unwrap_stanzas<'a>( @@ -651,7 +690,29 @@ impl crate::Identity for IdentityPluginV1 { #[cfg(test)] mod tests { - use super::Identity; + use crate::{Callbacks, DecryptError, EncryptError}; + + use super::{ + Identity, IdentityPluginV1, Recipient, RecipientPluginV1, PLUGIN_IDENTITY_PREFIX, + PLUGIN_RECIPIENT_PREFIX, + }; + + const INVALID_PLUGIN_NAME: &str = "foobar/../../../../../../../usr/bin/echo"; + + #[derive(Clone)] + struct NoCallbacks; + impl Callbacks for NoCallbacks { + fn display_message(&self, _: &str) {} + fn confirm(&self, _: &str, _: &str, _: Option<&str>) -> Option { + None + } + fn request_public_string(&self, _: &str) -> Option { + None + } + fn request_passphrase(&self, _: &str) -> Option { + None + } + } #[test] fn default_for_plugin() { @@ -660,4 +721,49 @@ mod tests { "AGE-PLUGIN-FOOBAR-1QVHULF", ); } + + #[test] + fn recipient_rejects_invalid_chars() { + let invalid_recipient = bech32::encode( + &format!("{}{}", PLUGIN_RECIPIENT_PREFIX, INVALID_PLUGIN_NAME), + [], + bech32::Variant::Bech32, + ) + .unwrap(); + assert!(invalid_recipient.parse::().is_err()); + } + + #[test] + fn identity_rejects_invalid_chars() { + let invalid_identity = bech32::encode( + &format!("{}{}-", PLUGIN_IDENTITY_PREFIX, INVALID_PLUGIN_NAME), + [], + bech32::Variant::Bech32, + ) + .expect("HRP is valid") + .to_uppercase(); + assert!(invalid_identity.parse::().is_err()); + } + + #[test] + #[should_panic] + fn identity_default_for_plugin_rejects_invalid_chars() { + Identity::default_for_plugin(INVALID_PLUGIN_NAME); + } + + #[test] + fn recipient_plugin_v1_rejects_invalid_chars() { + assert!(matches!( + RecipientPluginV1::new(INVALID_PLUGIN_NAME, &[], &[], NoCallbacks), + Err(EncryptError::MissingPlugin { binary_name }) if binary_name == INVALID_PLUGIN_NAME, + )); + } + + #[test] + fn identity_plugin_v1_rejects_invalid_chars() { + assert!(matches!( + IdentityPluginV1::new(INVALID_PLUGIN_NAME, &[], NoCallbacks), + Err(DecryptError::MissingPlugin { binary_name }) if binary_name == INVALID_PLUGIN_NAME, + )); + } } diff --git a/rage/CHANGELOG.md b/rage/CHANGELOG.md index 95e71ac0..8ff404bd 100644 --- a/rage/CHANGELOG.md +++ b/rage/CHANGELOG.md @@ -10,6 +10,14 @@ to 1.0.0 are beta releases. ## [Unreleased] +## [0.6.1, 0.7.2, 0.8.2, 0.9.3] - 2024-11-18 +### Security +- The age plugin protocol previously allowed plugin names that could be + interpreted as file paths. Under certain conditions, this could lead to a + different binary being executed as an age plugin than intended. Plugin names + are now required to only contain alphanumeric characters or the four special + characters `+-._`. + ## [0.10.0] - 2024-02-04 ### Added - Russian translation! diff --git a/rage/src/bin/rage/main.rs b/rage/src/bin/rage/main.rs index 38bbfc56..82e5c69d 100644 --- a/rage/src/bin/rage/main.rs +++ b/rage/src/bin/rage/main.rs @@ -227,6 +227,13 @@ fn write_output( Ok(()) } +#[inline] +fn valid_plugin_name(plugin_name: &str) -> bool { + plugin_name + .bytes() + .all(|b| b.is_ascii_alphanumeric() | matches!(b, b'+' | b'-' | b'.' | b'_')) +} + fn decrypt(opts: AgeOptions) -> Result<(), error::DecryptError> { if opts.armor { return Err(error::DecryptError::ArmorFlag); @@ -263,6 +270,12 @@ fn decrypt(opts: AgeOptions) -> Result<(), error::DecryptError> { let identities = if plugin_name.is_empty() { read_identities(opts.identity, opts.max_work_factor, &mut stdin_guard)? } else { + if !valid_plugin_name(plugin_name) { + return Err(age::DecryptError::MissingPlugin { + binary_name: plugin_name.into(), + } + .into()); + } // Construct the default plugin. vec![Box::new(plugin::IdentityPluginV1::new( plugin_name, diff --git a/rage/tests/cmd/rage/decrypt-invalid-identity-chars.in/file.age.txt b/rage/tests/cmd/rage/decrypt-invalid-identity-chars.in/file.age.txt new file mode 100644 index 00000000..6f9dc67f --- /dev/null +++ b/rage/tests/cmd/rage/decrypt-invalid-identity-chars.in/file.age.txt @@ -0,0 +1,8 @@ +-----BEGIN AGE ENCRYPTED FILE----- +YWdlLWVuY3J5cHRpb24ub3JnL3YxCi0+IFgyNTUxOSBHUGc3Zlhpekp0K012aXdu +T1VZN0lmWlRmNjdLYVB4RldkTFVLTkNDUXlBCmJjRUcrM3E0a0U0N3IyK1JsTitG +dHVTd0N6TVFRTWgzdG5uSzJmNm9YMTgKLT4gQXQ1WWAtZ3JlYXNlIDxodGFSVHJg +IFg0cWYsO0ogZ2Fzc1EKZGtPSTB3Ci0tLSBKazRIaHJxdnNJcHpyclRkQjg3QW5r +SVE2MHdtWkErYTNrNWJibWd1bmNBCkK9FoOkiLB93gD79vNed8L3LM9rhKm5qma2 +lSiwRx/aM1DKaZO0CMmYQkoM2tPReA== +-----END AGE ENCRYPTED FILE----- diff --git a/rage/tests/cmd/rage/decrypt-invalid-identity-chars.toml b/rage/tests/cmd/rage/decrypt-invalid-identity-chars.toml new file mode 100644 index 00000000..d3350d2a --- /dev/null +++ b/rage/tests/cmd/rage/decrypt-invalid-identity-chars.toml @@ -0,0 +1,13 @@ +bin.name = "rage" +args = "--decrypt --identity - file.age.txt" +status = "failed" +stdin = """ +AGE-PLUGIN-FOOBAR/../../../../../../../USR/BIN/ECHO-1HKGPY3 +""" +stdout = "" +stderr = """ +Error: identity file contains non-identity data on line 1 + +[ Did rage not do what you expected? Could an error be more useful? ] +[ Tell us: https://str4d.xyz/rage/report ] +""" diff --git a/rage/tests/cmd/rage/decrypt-invalid-plugin-name-chars.toml b/rage/tests/cmd/rage/decrypt-invalid-plugin-name-chars.toml new file mode 100644 index 00000000..968a8b25 --- /dev/null +++ b/rage/tests/cmd/rage/decrypt-invalid-plugin-name-chars.toml @@ -0,0 +1,12 @@ +bin.name = "rage" +args = "--decrypt -j foobar/../../../../../../../usr/bin/echo" +status = "failed" +stdin = "" +stdout = "" +stderr = """ +Error: Could not find 'foobar/../../../../../../../usr/bin/echo' on the PATH. +Have you installed the plugin? + +[ Did rage not do what you expected? Could an error be more useful? ] +[ Tell us: https://str4d.xyz/rage/report ] +""" diff --git a/rage/tests/cmd/rage/encrypt-invalid-recipient-chars.toml b/rage/tests/cmd/rage/encrypt-invalid-recipient-chars.toml new file mode 100644 index 00000000..ce03d5e5 --- /dev/null +++ b/rage/tests/cmd/rage/encrypt-invalid-recipient-chars.toml @@ -0,0 +1,11 @@ +bin.name = "rage" +args = "--encrypt --recipient age1foobar/../../../../../../../usr/bin/echo1849l6e" +status = "failed" +stdin = "" +stdout = "" +stderr = """ +Error: Invalid recipient 'age1foobar/../../../../../../../usr/bin/echo1849l6e'. + +[ Did rage not do what you expected? Could an error be more useful? ] +[ Tell us: https://str4d.xyz/rage/report ] +"""