diff --git a/age/CHANGELOG.md b/age/CHANGELOG.md index dc6cd61..76ba8f5 100644 --- a/age/CHANGELOG.md +++ b/age/CHANGELOG.md @@ -10,6 +10,14 @@ to 1.0.0 are beta releases. ## [Unreleased] +## [0.6.1] - 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.7.1] - 2021-12-27 ### Fixed - Bumped `age-core` to 0.7.1 to fix a bug where non-canonical recipient stanza diff --git a/age/src/plugin.rs b/age/src/plugin.rs index 426d5e6..aa002a8 100644 --- a/age/src/plugin.rs +++ b/age/src/plugin.rs @@ -30,6 +30,13 @@ const CMD_REQUEST_PUBLIC: &str = "request-public"; const CMD_REQUEST_SECRET: &str = "request-secret"; const CMD_FILE_KEY: &str = "file-key"; +#[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) } @@ -54,10 +61,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") } @@ -98,14 +110,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") } @@ -192,22 +210,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(), }) + } } } @@ -358,17 +382,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>( @@ -478,7 +508,25 @@ 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"; + + struct NoCallbacks; + impl Callbacks for NoCallbacks { + fn display_message(&self, _: &str) {} + fn request_public_string(&self, _: &str) -> Option { + None + } + fn request_passphrase(&self, _: &str) -> Option { + None + } + } #[test] fn default_for_plugin() { @@ -487,4 +535,43 @@ 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] + 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 840f2e1..60e2a12 100644 --- a/rage/CHANGELOG.md +++ b/rage/CHANGELOG.md @@ -10,6 +10,14 @@ to 1.0.0 are beta releases. ## [Unreleased] +## [0.6.1] - 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.7.1] - 2021-12-27 ### Fixed - Fixed a bug in 0.7.0 where non-canonical recipient stanza bodies in an age 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 0000000..6f9dc67 --- /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 0000000..d3350d2 --- /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 0000000..968a8b2 --- /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 0000000..ce03d5e --- /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 ] +"""