Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Resource Policy Condition Keys - Handling it as non-case sensitive #353

Open
fabiodouek opened this issue Mar 24, 2023 · 1 comment
Labels

Comments

@fabiodouek
Copy link

Describe the issue

There are a few Guard Rules examples in this repo that are not handling Resource Policies Condition keys correctly. As a result, the rule enforcement can easily be bypassed by defining condition keys mixing it with upper/lower case.

Any examples
Please supply:
https://github.com/aws-cloudformation/cloudformation-guard/blob/main/guard-examples/cross-account/sns-cross-account.guard

let source_accounts = %expected_conditions[ keys == /(aws|AWS):[sS]ource(Account|Owner|Arn|ARN)/ ]

The above regex will handle a few key combinations. I will list some of them for SourceAccount as an example:

  • aws:SourceAccount
  • aws:sourceAccount
  • AWS:SourceAccount
  • AWS:sourceAccount

The problem here is that Condition key name is not case sensitive as documented here: https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_condition.html

With the above rule, if the user specifies in their resource policy the condition key as aws:SOURCEACCOUNT, allowing an account not specified in the allowed_accounts variable, the rule result will be SKIP instead of FAIL.

The Regex could be rewritten from:

let source_accounts = %expected_conditions[ keys == /(aws|AWS):[sS]ource(Account|Owner|Arn|ARN)/ ]

to the following which addresses the case sensitive problem:

let source_accounts = %expected_conditions[ keys == /(?i)aws:Source(Account|Owner|Arn)/ ]

It could be further enhanced to prevent invalid prefix/suffix in the condition key as follows:

let source_accounts = %expected_conditions[ keys == /^(?i)aws:Source(Account|Owner|Arn)$/ ]

Operating System:
Amazon Linux 2

OS Version
Amazon Linux 2

Guard Version 2.1.3

@fabiodouek fabiodouek changed the title [GENERAL ISSUE] Resource Policy Condition Keys - Handling it as non-case sensitive [BUG] Resource Policy Condition Keys - Handling it as non-case sensitive Mar 24, 2023
@akshayrane
Copy link
Collaborator

Hi Fabio,

Thank you for taking a closer look at the Guard rule example we have provided with the repository. We think that the Guard rule has clauses that will make the keys sensitive. Here's how:

From the same AWS documentation you linked, it also states:

Case-sensitivity of condition key values depends on the condition operator that you use. For example, the following condition includes the StringEquals operator to ensure that only requests made by johndoe match. Users named JohnDoe are denied access.

The guard rule in question applies to the keys StringEquals, StringLike, ArnEquals and ArnLike, the following snippet will ensure that:

rule check_via_aws_service(statement) {
    when %statement.Principal.Service exists {
        %statement.Condition[ keys == /String(Equals|Like)|Arn(Equals|Like)/ ] not empty {
            let source_accounts = this[ keys == /(aws|AWS):[sS]ource(Account|Owner|Arn|ARN)/ ]
            %source_accounts in %allowed
        }
    }
}

Now, for the templates where this rule is skipped meaning the four keys that we are checking are actually not used, where as IgnoreCase equivalent is used I agree there is a possibility of bypassing the rule.

We may need to either tweak the rule or add another clause applicable to ignore case condition operators as well. We will make the necessary changes and raise a PR for this change.

HTH,
Akshay

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants