Percent-decode request paths and query parameters before access-control privilege matching

.github/actions/spelling/expect.txt

@@ -1,4 +1,4 @@
 aab
 AAFFBB
 aarch
 abe
@@ -106,12 +106,15 @@
 fffi
 ffi
 FIXEDFILEINFO
+Fmanagement
 FOF
+Fresource
 FSETID
 FSO
 fsprogs
 fstorage
 fstype
+ftoken
 fwlink
 Fzpeng
 gaplugin
@@ -312,6 +315,7 @@
 topdir
 totalentries
 transitioning
+trustyuser
 UBR
 UBRSTRING
 udev
@@ -323,6 +327,7 @@
 unregisters
 unspec
 uzers
+valu
 VCpus
 vcruntime
 vendored
@@ -365,6 +370,7 @@
 workarounds
 WORKINGSET
 WORKDIR
+wrongvalue
 WScript
 wsf
 Wsh
@@ -380,4 +386,4 @@
 xxxx
 xxxxxxxx
 xxxxxxxxxxx
 zipsas

proxy_agent/Cargo.toml

@@ -30,6 +30,7 @@ thiserror = "1.0.64"
 libc = "0.2.147"
 socket2 = "0.5"               # Set socket options without tokio/std conversion
 base64 = "0.22"
+percent-encoding = "2.3"
 
 [dependencies.uuid]
 version = "1.3.0"

proxy_agent/src/key_keeper/key.rs

@@ -216,7 +216,7 @@ impl Clone for Privilege {
 impl Privilege {
     /// Note: `self.path` and `self.queryParameters` keys/values are expected to be
     /// pre-lowercased (done in `ComputedAuthorizationItem::from_authorization_item`).
-    /// `lowered_request_path` should be `request_url.path().to_lowercase()`, hoisted by the caller.
+    /// `lowered_request_path` should be the percent-decoded, lowercased request path.
     pub fn is_match(
         &self,
         logger: &mut ConnectionLogger,
@@ -227,7 +227,18 @@ impl Privilege {
             LoggerLevel::Trace,
             format!("Start to match privilege '{}'", self.name),
         );
-        if lowered_request_path.starts_with(&self.path) {
+
+        // The decoded path may contain '?' if the attacker encoded it as %3F.
+        // Split so we match only the path portion, and extract any embedded query parameters.
+        let (actual_path, embedded_query) = match lowered_request_path.find('?') {
+            Some(pos) => (
+                &lowered_request_path[..pos],
+                Some(&lowered_request_path[pos + 1..]),
+            ),
+            None => (lowered_request_path, None),
+        };
+
+        if actual_path.starts_with(&self.path) {
             logger.write(
                 LoggerLevel::Trace,
                 format!("Matched privilege path '{}'", self.path),
@@ -242,15 +253,35 @@ impl Privilege {
                     ),
                 );
 
+                // Collect query pairs from the URI query string.
+                let mut all_query_pairs = hyper_client::query_pairs(request_url);
+
+                // Also collect query pairs embedded in the decoded path (from encoded %3F).
+                // These are already percent-decoded and lowercased from lowered_request_path.
+                if let Some(eq) = embedded_query {
+                    for pair in eq.split('&') {
+                        let mut split = pair.splitn(2, '=');
+                        let key = split.next().unwrap_or("");
+                        if key.is_empty() {
+                            continue;
+                        }
+                        let value = split.next().unwrap_or("");
+                        all_query_pairs.push((key.to_string(), value.to_string()));
+                    }
+                }
+
                 for (key, value) in query_parameters {
-                    // We may need to optimize this like `lowered_request_path` if there are too many query parameters in the future,
-                    // but currently we expect only a few query parameters at most, so the performance impact should be minimal.
-                    match hyper_client::query_pairs(request_url)
-                        .into_iter()
-                        .find(|(k, _)| k.to_lowercase() == *key)
-                    {
+                    // Percent-decode query keys/values before matching to prevent encoded bypass attacks.
+                    match all_query_pairs.iter().find(|(k, _)| {
+                        percent_encoding::percent_decode_str(k)
+                            .decode_utf8_lossy()
+                            .to_lowercase()
+                            == *key
+                    }) {
                         Some((_, v)) => {
-                            if v.to_lowercase() == *value {
+                            let decoded_v =
+                                percent_encoding::percent_decode_str(v).decode_utf8_lossy();
+                            if decoded_v.to_lowercase() == *value {
                                 logger.write(
                                     LoggerLevel::Trace,
                                     format!(
@@ -873,6 +904,7 @@ pub async fn attest_key(host: &str, port: u16, key: &Key) -> Result<()> {
 
 #[cfg(test)]
 mod tests {
+    use std::collections::HashMap;
     use std::ffi::OsString;
     #[cfg(not(windows))]
     use std::os::unix::ffi::OsStringExt;
@@ -1533,6 +1565,74 @@ mod tests {
             !privilege2.is_match(&mut logger, &url, &url.path().to_lowercase()),
             "privilege should not be matched"
         );
+
+        // Test percent-encoded query key: key1 encoded as k%65y1 should still match
+        let url: Uri = "http://localhost/test?k%65y1=value1&key2=value2"
+            .parse()
+            .unwrap();
+        assert!(
+            privilege.is_match(&mut logger, &url, &url.path().to_lowercase()),
+            "percent-encoded query key should match"
+        );
+
+        // Test percent-encoded query value: value1 encoded as valu%651 should still match
+        let url: Uri = "http://localhost/test?key1=valu%651&key2=value2"
+            .parse()
+            .unwrap();
+        assert!(
+            privilege.is_match(&mut logger, &url, &url.path().to_lowercase()),
+            "percent-encoded query value should match"
+        );
+
+        // Test percent-encoded slash in query value: resource=https%3A%2F%2Fmanagement.azure.com%2F
+        let privilege_with_resource = Privilege {
+            name: "token".to_string(),
+            path: "/metadata/identity/oauth2/token".to_string(),
+            queryParameters: Some(HashMap::from([(
+                "resource".to_string(),
+                "https://management.azure.com/".to_string(),
+            )])),
+        };
+        let url: Uri = "http://169.254.169.254/metadata/identity/oauth2/token?resource=https%3A%2F%2Fmanagement.azure.com%2F"
+            .parse()
+            .unwrap();
+        assert!(
+            privilege_with_resource.is_match(&mut logger, &url, &url.path().to_lowercase()),
+            "percent-encoded slashes/colons in query value should match decoded privilege"
+        );
+
+        // Test both key and value percent-encoded simultaneously
+        let url: Uri = "http://localhost/test?k%65y1=valu%651&k%65y2=valu%652"
+            .parse()
+            .unwrap();
+        assert!(
+            privilege.is_match(&mut logger, &url, &url.path().to_lowercase()),
+            "both percent-encoded key and value should match"
+        );
+
+        // Test encoded key that does NOT match should still fail
+        let url: Uri = "http://localhost/test?k%65y1=value1&k%65y2=wrongvalue"
+            .parse()
+            .unwrap();
+        assert!(
+            !privilege.is_match(&mut logger, &url, &url.path().to_lowercase()),
+            "percent-encoded key with wrong value should not match"
+        );
+
+        // Test encoded '?' (%3F) in path: IMDS decodes the full URL so the query params are real.
+        // The caller (authorization_rules.rs) percent-decodes the path before passing it here,
+        // so lowered_request_path will contain '?' from the decoded %3F.
+        let url: Uri = "http://169.254.169.254/metadata/identity/oauth2/token%3Fresource=https%3A%2F%2Fmanagement.azure.com%2F"
+            .parse()
+            .unwrap();
+        // Simulate what authorization_rules.rs does: percent-decode then lowercase
+        let decoded_path = percent_encoding::percent_decode_str(url.path())
+            .decode_utf8_lossy()
+            .to_lowercase();
+        assert!(
+            privilege_with_resource.is_match(&mut logger, &url, &decoded_path),
+            "encoded %3F query separator must be decoded and query params matched"
+        );
     }
 
     #[tokio::test]

proxy_agent/src/proxy/authorization_rules.rs

@@ -191,7 +191,9 @@ impl ComputedAuthorizationItem {
             return true;
         }
 
-        let lowered_request_path = request_url.path().to_lowercase();
+        let decoded_path =
+            percent_encoding::percent_decode_str(request_url.path()).decode_utf8_lossy();
+        let lowered_request_path = decoded_path.to_lowercase();
         let mut any_privilege_matched = false;
         for privilege in self.privileges.values() {
             let privilege_name = &privilege.name;
@@ -635,4 +637,107 @@ mod tests {
         // clean up and ignore the clean up errors
         _ = std::fs::remove_dir_all(&temp_test_path);
     }
+
+    #[tokio::test]
+    async fn test_percent_encoded_path_must_not_bypass_privilege() {
+        let mut test_logger = ConnectionLogger::new(0, 0);
+
+        // Simulate a privilege restricting /metadata/identity/oauth2/token
+        let access_control_rules = AccessControlRules {
+            roles: Some(vec![Role {
+                name: "tokenRole".to_string(),
+                privileges: vec!["tokenPrivilege".to_string()],
+            }]),
+            privileges: Some(vec![Privilege {
+                name: "tokenPrivilege".to_string(),
+                path: "/metadata/identity/oauth2/token".to_string(),
+                queryParameters: None,
+            }]),
+            identities: Some(vec![Identity {
+                name: "trustedUser".to_string(),
+                userName: Some("trustyuser".to_string()),
+                groupName: None,
+                exePath: None,
+                processName: None,
+            }]),
+            roleAssignments: Some(vec![RoleAssignment {
+                role: "tokenRole".to_string(),
+                identities: vec!["trustedUser".to_string()],
+            }]),
+        };
+        let authorization_item = AuthorizationItem {
+            defaultAccess: "allow".to_string(),
+            mode: "enforce".to_string(),
+            rules: Some(access_control_rules),
+            id: "0".to_string(),
+        };
+        let rules = ComputedAuthorizationItem::from_authorization_item(authorization_item);
+
+        let attacker_claims = Claims {
+            userId: 9999,
+            userName: "attacker".to_string(),
+            userGroups: vec!["users".to_string()],
+            processId: 1234,
+            processFullPath: PathBuf::from("/usr/bin/curl"),
+            clientIp: "127.0.0.1".to_string(),
+            clientPort: 12345,
+            processName: OsString::from("curl"),
+            processCmdLine: "curl".to_string(),
+            runAsElevated: false,
+        };
+
+        // Normal path is correctly denied for attacker
+        let url = hyper::Uri::from_str("http://169.254.169.254/metadata/identity/oauth2/token?api-version=2018-02-01&resource=https://management.azure.com/").unwrap();
+        assert!(
+            !rules.is_allowed(&mut test_logger, url, attacker_claims.clone()),
+            "Normal path must be denied for attacker"
+        );
+
+        // Percent-encoded %2F bypass: must also be denied
+        let url_encoded = hyper::Uri::from_str("http://169.254.169.254/metadata/identity/oauth2%2Ftoken?api-version=2018-02-01&resource=https://management.azure.com/").unwrap();
+        assert!(
+            !rules.is_allowed(&mut test_logger, url_encoded, attacker_claims.clone()),
+            "Percent-encoded path (%2F) must NOT bypass privilege matching"
+        );
+
+        // Mixed encoding: %2f (lowercase hex) must also be caught
+        let url_lower_hex = hyper::Uri::from_str("http://169.254.169.254/metadata/identity/oauth2%2ftoken?api-version=2018-02-01&resource=https://management.azure.com/").unwrap();
+        assert!(
+            !rules.is_allowed(&mut test_logger, url_lower_hex, attacker_claims.clone()),
+            "Percent-encoded path (%2f lowercase) must NOT bypass privilege matching"
+        );
+
+        // Trusted user should still be allowed through normal path
+        let trusted_claims = Claims {
+            userId: 1000,
+            userName: "trustyuser".to_string(),
+            userGroups: vec!["users".to_string()],
+            processId: 5678,
+            processFullPath: PathBuf::from("/usr/bin/curl"),
+            clientIp: "127.0.0.1".to_string(),
+            clientPort: 12345,
+            processName: OsString::from("curl"),
+            processCmdLine: "curl".to_string(),
+            runAsElevated: false,
+        };
+        let url = hyper::Uri::from_str("http://169.254.169.254/metadata/identity/oauth2/token?api-version=2018-02-01&resource=https://management.azure.com/").unwrap();
+        assert!(
+            rules.is_allowed(&mut test_logger, url, trusted_claims.clone()),
+            "Trusted user must be allowed through normal path"
+        );
+
+        // Trusted user should still be allowed through percent-encoded path (%2F)
+        let url = hyper::Uri::from_str("http://169.254.169.254/metadata/identity/oauth2%2Ftoken?api-version=2018-02-01&resource=https://management.azure.com/").unwrap();
+        assert!(
+            rules.is_allowed(&mut test_logger, url, trusted_claims.clone()),
+            "Trusted user must be allowed through percent-encoded path (%2F)"
+        );
+
+        // Trusted user should still be allowed through percent-encoded path (%2f) with lowercase hex
+        let url = hyper::Uri::from_str("http://169.254.169.254/metadata/identity/oauth2%2ftoken?api-version=2018-02-01&resource=https://management.azure.com/").unwrap();
+        assert!(
+            rules.is_allowed(&mut test_logger, url, trusted_claims.clone()),
+            "Trusted user must be allowed through percent-encoded path (%2f)"
+        );
+    }
 }

proxy_agent_shared/src/hyper_client.rs

@@ -714,4 +714,16 @@ mod tests {
             "sync time should be close to the custom header time"
         );
     }
+
+    #[test]
+    fn query_pairs_test() {
+        let url = "/test?key1=value1&key%202=value%202"
+            .parse::<hyper::Uri>()
+            .unwrap();
+        let pairs = super::query_pairs(&url);
+        assert_eq!(pairs.len(), 2);
+        assert_eq!(pairs[0], ("key1".to_string(), "value1".to_string()));
+        // query_pairs returns raw (non-decoded) values for signature compatibility
+        assert_eq!(pairs[1], ("key%202".to_string(), "value%202".to_string()));
+    }
 }