Netflix-Skunkworks/policyuniverse

Wildcard in Account ID results in false positive for Internet accessibility

Opened this issue · 1 comments

bseb commented

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": "*"
    }
  ]
}

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.