diff --git a/.github/actions/spelling/expect.txt b/.github/actions/spelling/expect.txt index 8f2e1ddf..2dc59a05 100644 --- a/.github/actions/spelling/expect.txt +++ b/.github/actions/spelling/expect.txt @@ -106,12 +106,15 @@ FFFFFFFF fffi ffi FIXEDFILEINFO +Fmanagement FOF +Fresource FSETID FSO fsprogs fstorage fstype +ftoken fwlink Fzpeng gaplugin @@ -312,6 +315,7 @@ tokio topdir totalentries transitioning +trustyuser UBR UBRSTRING udev @@ -323,6 +327,7 @@ Unregistering unregisters unspec uzers +valu VCpus vcruntime vendored @@ -365,6 +370,7 @@ WMI workarounds WORKINGSET WORKDIR +wrongvalue WScript wsf Wsh diff --git a/Cargo.lock b/Cargo.lock index cf3144ce..2a6ecf89 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -186,6 +186,7 @@ dependencies = [ "libloading", "nix", "once_cell", + "percent-encoding", "proxy_agent_shared", "regex", "serde", @@ -887,6 +888,12 @@ dependencies = [ "windows-sys", ] +[[package]] +name = "percent-encoding" +version = "2.3.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9b4f627cb1b25917193a259e49bdad08f671f8d9708acfd5fe0a8c1455d87220" + [[package]] name = "pin-project-lite" version = "0.2.14" diff --git a/proxy_agent/Cargo.toml b/proxy_agent/Cargo.toml index 9bc6b227..31c6095b 100644 --- a/proxy_agent/Cargo.toml +++ b/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" diff --git a/proxy_agent/src/key_keeper/key.rs b/proxy_agent/src/key_keeper/key.rs index 4079ae68..371ca2ae 100644 --- a/proxy_agent/src/key_keeper/key.rs +++ b/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] diff --git a/proxy_agent/src/proxy/authorization_rules.rs b/proxy_agent/src/proxy/authorization_rules.rs index 3c706083..857c3ade 100644 --- a/proxy_agent/src/proxy/authorization_rules.rs +++ b/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)" + ); + } } diff --git a/proxy_agent_shared/src/hyper_client.rs b/proxy_agent_shared/src/hyper_client.rs index 71175448..fb9e2744 100644 --- a/proxy_agent_shared/src/hyper_client.rs +++ b/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::() + .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())); + } }