diff --git a/crates/policy/src/policy/policy.rs b/crates/policy/src/policy/policy.rs index 6cde4e9d..7eb9e2a3 100644 --- a/crates/policy/src/policy/policy.rs +++ b/crates/policy/src/policy/policy.rs @@ -63,16 +63,23 @@ pub struct Policy { impl Policy { pub async fn is_allowed(&self, args: &Args<'_>) -> bool { + // First, check all Deny statements - if any Deny matches, deny the request for statement in self.statements.iter().filter(|s| matches!(s.effect, Effect::Deny)) { if !statement.is_allowed(args).await { return false; } } - if args.deny_only || args.is_owner { + // Owner has all permissions + if args.is_owner { return true; } + if args.deny_only { + return false; + } + + // Check Allow statements for statement in self.statements.iter().filter(|s| matches!(s.effect, Effect::Allow)) { if statement.is_allowed(args).await { return true; @@ -594,6 +601,102 @@ mod test { Ok(()) } + #[tokio::test] + async fn test_deny_only_security_fix() -> Result<()> { + let data = r#" +{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Action": ["s3:GetObject"], + "Resource": ["arn:aws:s3:::bucket1/*"] + } + ] +} +"#; + + let policy = Policy::parse_config(data.as_bytes())?; + let conditions = HashMap::new(); + let claims = HashMap::new(); + + // Test with deny_only=true but no matching Allow statement + let args_deny_only = Args { + account: "testuser", + groups: &None, + action: Action::S3Action(crate::policy::action::S3Action::PutObjectAction), + bucket: "bucket2", + conditions: &conditions, + is_owner: false, + object: "test.txt", + claims: &claims, + deny_only: true, // Should NOT automatically allow + }; + + // Should return false because deny_only=true, regardless of whether there's a matching Allow statement + assert!( + !policy.is_allowed(&args_deny_only).await, + "deny_only should return false when deny_only=true, regardless of Allow statements" + ); + + // Test with deny_only=true and matching Allow statement + let args_deny_only_allowed = Args { + account: "testuser", + groups: &None, + action: Action::S3Action(crate::policy::action::S3Action::GetObjectAction), + bucket: "bucket1", + conditions: &conditions, + is_owner: false, + object: "test.txt", + claims: &claims, + deny_only: true, + }; + + // Should return false because deny_only=true prevents checking Allow statements (unless is_owner=true) + assert!( + !policy.is_allowed(&args_deny_only_allowed).await, + "deny_only should return false even with matching Allow statement" + ); + + // Test with deny_only=false (normal case) + let args_normal = Args { + account: "testuser", + groups: &None, + action: Action::S3Action(crate::policy::action::S3Action::GetObjectAction), + bucket: "bucket1", + conditions: &conditions, + is_owner: false, + object: "test.txt", + claims: &claims, + deny_only: false, + }; + + // Should return true because there's an Allow statement + assert!( + policy.is_allowed(&args_normal).await, + "normal policy evaluation should allow with matching Allow statement" + ); + + let args_owner_deny_only = Args { + account: "testuser", + groups: &None, + action: Action::S3Action(crate::policy::action::S3Action::PutObjectAction), + bucket: "bucket2", + conditions: &conditions, + is_owner: true, // Owner has all permissions + object: "test.txt", + claims: &claims, + deny_only: true, // Even with deny_only=true, owner should be allowed + }; + + assert!( + policy.is_allowed(&args_owner_deny_only).await, + "owner should retain all permissions even when deny_only=true" + ); + + Ok(()) + } + #[tokio::test] async fn test_aws_username_policy_variable() -> Result<()> { let data = r#" diff --git a/rustfs/src/admin/handlers/service_account.rs b/rustfs/src/admin/handlers/service_account.rs index 739cdbbb..503bc005 100644 --- a/rustfs/src/admin/handlers/service_account.rs +++ b/rustfs/src/admin/handlers/service_account.rs @@ -112,8 +112,6 @@ impl Operation for AddServiceAccount { return Err(s3_error!(InvalidRequest, "iam not init")); }; - let deny_only = constant_time_eq(&cred.access_key, &target_user) || constant_time_eq(&cred.parent_user, &target_user); - if !iam_store .is_allowed(&Args { account: &cred.access_key, @@ -130,7 +128,7 @@ impl Operation for AddServiceAccount { is_owner: owner, object: "", claims: cred.claims.as_ref().unwrap_or(&HashMap::new()), - deny_only, + deny_only: false, // Always require explicit Allow permission }) .await { diff --git a/rustfs/src/admin/handlers/user.rs b/rustfs/src/admin/handlers/user.rs index 80233b4d..e7ddf1be 100644 --- a/rustfs/src/admin/handlers/user.rs +++ b/rustfs/src/admin/handlers/user.rs @@ -118,12 +118,14 @@ impl Operation for AddUser { return Err(s3_error!(InvalidArgument, "access key is not utf8")); } - let deny_only = ak == cred.access_key; + // Security fix: Always require explicit Allow permission for CreateUser + // Do not use deny_only to bypass permission checks, even when creating for self + // This ensures consistent security semantics and prevents privilege escalation validate_admin_request( &req.headers, &cred, owner, - deny_only, + false, // Always require explicit Allow permission vec![Action::AdminAction(AdminAction::CreateUserAdminAction)], req.extensions.get::>().and_then(|opt| opt.map(|a| a.0)), ) @@ -375,12 +377,14 @@ impl Operation for GetUserInfo { let (cred, owner) = check_key_valid(get_session_token(&req.uri, &req.headers).unwrap_or_default(), &input_cred.access_key).await?; - let deny_only = ak == cred.access_key; + // Security fix: Always require explicit Allow permission for GetUser + // Users should have explicit GetUser permission to view account information + // This ensures consistent security semantics across all admin operations validate_admin_request( &req.headers, &cred, owner, - deny_only, + false, // Always require explicit Allow permission vec![Action::AdminAction(AdminAction::GetUserAdminAction)], req.extensions.get::>().and_then(|opt| opt.map(|a| a.0)), )