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

Wildcard in Account ID results in false positive for Internet accessibility #150

Open
bseb opened this issue Dec 16, 2021 · 1 comment · May be fixed by #162
Open

Wildcard in Account ID results in false positive for Internet accessibility #150

bseb opened this issue Dec 16, 2021 · 1 comment · May be fixed by #162

Comments

@bseb
Copy link

bseb commented Dec 16, 2021

cc @k-bailey

We have seen wildcards in account ID ARN field result in false positives for a resource being internet accessible when the resource is restricted to an AWS Org. In this case the roles contain wildcards but the resource itself is restricted to only roles belonging to the ORG

Example policy that shows as Internet Accessible when processed by Policy Universe :

{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Condition": {
        "StringEquals": {
          "aws:PrincipalOrgID": [
            "o-12345",
            "o-67890"
          ]
        },
        "StringLike": {
          "aws:PrincipalArn": [
            "arn:aws:iam::*:role/ARole-*",
            "arn:aws:iam::*:role/BRole-*",
            "arn:aws:iam::*:role/CRole-*",
            "arn:aws:iam::*:role/DRole",
            "arn:aws:iam::*:role/ERole"
          ]
        }
      },
      "Action": "secretsmanager:GetSecretValue",
      "Resource": "*",
      "Effect": "Allow",
      "Principal": "*"
    }
  ]
}
@tweedge
Copy link
Contributor

tweedge commented Oct 6, 2022

Tinkered with this a bit and found that changing statement.py from:

    def is_condition_internet_accessible(self):
        condition_entries = self.condition_entries
        if len(condition_entries) == 0:
            return True

        for entry in condition_entries:
            if self._is_condition_entry_internet_accessible(entry):
                return True

        return False

to:

    def is_condition_internet_accessible(self):
        condition_entries = self.condition_entries
        if len(condition_entries) == 0:
            return True

        for entry in condition_entries:
            if not self._is_condition_entry_internet_accessible(entry):
                return False

        return True

Treats the internet accessibility checks for components of a condition block like and operators instead of or, fixing this case without requiring even a single test be modified. This is generally more appropriate to how I see people using resource policies. However, it would miss cases like this one where the different keys within a condition should be or'd:

{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Condition": {
        "ForAnyValue:StringLike": {
          "aws:userId": [
            "arn:aws:iam::111122223333:role/ARole-*",
            "arn:aws:iam::*:role/BRole-*"
          ]
        }
      },
      "Action": "secretsmanager:GetSecretValue",
      "Resource": "*",
      "Effect": "Allow",
      "Principal": "*"
    }
  ]
}

To be most accurate to the documentation, I think it would be appropriate to track each key where a value was found in the Condition block (essentially, extending the current NamedTuple to a triplet, or keeping a separate tuple mapping each key->value) so internet accessibility would more closely match AWS' mechanism. Would welcome any thoughts or critique on the approach.

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

Successfully merging a pull request may close this issue.

2 participants