Restrict set of valid characters for plugin names

Previously, plugin names were allowed to be `1*VCHAR`, which permits path separators and parent directory syntax. Under certain conditions, this could cause `rage` to execute a different binary than intended when launching a plugin. Plugin names are now restricted to alphanumeric characters or +-._ which covers all binary names generally observed in practice.
str4d · Nov 18, 2024 · 703152e · 703152e
1 parent b6c8f06
commit 703152e
Show file tree

Hide file tree

Showing 3 changed files with 79 additions and 37 deletions.
diff --git a/age/CHANGELOG.md b/age/CHANGELOG.md
@@ -9,6 +9,12 @@ and this project adheres to Rust's notion of
 to 1.0.0 are beta releases.
 
 ## [Unreleased]
+### 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.6.0] - 2021-05-02
 ### Security

diff --git a/age/src/plugin.rs b/age/src/plugin.rs
@@ -29,6 +29,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)
 }
@@ -53,10 +60,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")
                 }
@@ -97,14 +109,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")
                 }
@@ -178,22 +196,28 @@ impl<C: Callbacks> RecipientPluginV1<C> {
         identities: &[Identity],
         callbacks: C,
     ) -> Result<Self, EncryptError> {
-        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(),
             })
+        }
     }
 }
 
@@ -344,17 +368,23 @@ impl<C: Callbacks> IdentityPluginV1<C> {
         identities: &[Identity],
         callbacks: C,
     ) -> Result<Self, DecryptError> {
-        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>(

diff --git a/rage/CHANGELOG.md b/rage/CHANGELOG.md
@@ -9,6 +9,12 @@ and this project adheres to Rust's notion of
 to 1.0.0 are beta releases.
 
 ## [Unreleased]
+### 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.6.0] - 2021-05-02
 ### Added