Infrastructure Modification check ignores conditions set on policy

It appears that the checks for Infrastructure Modification report actions even when conditions apply - while, on the other hand, the checks for e.g. Data Exfiltration and Privilege Escalation do not flag actions when conditions apply.

This causes false negatives when we have a policy that, for example, allows ec2.RunInstances but includes a condition that ec2:Owner must be a certain account; such a policy has been crafted deliberately, but Cloudsplaining still flags it as allowing Infrastructure Modification.

To reproduce, consider this policy:

{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Action": [
        "ec2:RunInstances"
      ],
      "Condition": {
        "StringEquals": {
            "ec2:Owner": "ValidOwnerId"
          }
      },
      "Resource": "*",
      "Effect": "Allow"
    },
    {
        "Effect": "Allow",
        "Action": "s3:GetObject",
        "Condition": {
            "StringEquals": {
                "ec2:Owner": "ValidOwnerId"
              }
          },
        "Resource": "*"
    },
    {
        "Effect": "Allow",
        "Action": "glue:updatedevendpoint",
        "Condition": {
            "StringEquals": {
                "ec2:Owner": "ValidOwnerId"
              }
          },
        "Resource": "*"
    }
  ]
}

If we remove those Condition blocks, Cloudsplaining identifies 3 issues:

After adding the Conditions, Data Exfiltration and Privilege Escalation disappear, but Infrastructure Modification remains:

Expected Behaviour: The presence of Conditions causes Cloudsplaining to suppress Infrastructure Modification warnings.

Actual Behaviour: Cloudsplaining reports Infrastructure Modification even though Conditions exist on the policy.

[kmcquade]

This was by design to reduce noise. It was introduced in #257

You can skip this behavior by using the --flag-all-risky-actions flag in the scan command

@kmcquade Thanks for looking into this, Kinnaird. I just want to avoid misunderstanding:

It sounds like you're saying that, when the --flag-all-risky-actions flag is set, then Cloudsplaining will return all warnings regardless of whether or not Resource/Condition constraints exist.

I infer that, if that flag is not set, Cloudsplaining should not return warnings for policies where Resource/Condition constraints exist. Is that correct?

The issue I reported is that Cloudsplaining is returning Intrastructure Modification warnings for policies with Resource/Condition constraints, despite --flag-all-risky-actions=False.¹

Are you saying the above bolded statement is the intended behaviour? Because that seems wrong to me. Why would Infrastructure Modification (the "noisiest" issue type) be exempt from the change meant to reduce noise? :)

Footnotes

1. Note: I am using scan_multi_account, and there is no way to set the --flag-all-risky-actions flag in that code path. As seen here, the AuthorizationDetails constructor is called without specifying values for the flag_conditional_statements and flag_resource_arn_statements arguments. ↩

[kmcquade]

Ohhh, I get what you are saying now. Thank you for clarifying.

I tried it on a test file with those contents, and what you are saying is correct.

I think this can be broken down into two issues:

1. The scan-policy-file command should have a --flag-all-risky-actions flag, just like the scan command.
2. As you said, the Infrastructure Modification warning should behave just like Privilege Escalation, Resource Exposure, and Data Exfil.

To be honest, I am not sure why this is happening. I agree that it shouldn't return warnings for policies where Condition constraints exist. However, I don't really have the time to fix this bug since I am no longer working in that space and I am focusing on my own startup. I responded because it looked important and I felt like it was important to clarify. I wish I could fix that but I just don't have the time.

Despite that limited availability, if you or anyone else wanted to fix this bug and made a PR, I'd be happy to approve a PR that fixes it, provided it has unit tests, and is reasonable. If you'd like to take that on, I would encourage you to do so. I'm sorry I can't help more than that. I'll leave this issue open so others can discover it.

Edit: A key attribute of such a PR is that I would have to figure out what is going on by reading the code - I definitely do not encourage making wholesale changes to it since I can't approve it if I have no idea what is going on. Unit tests help achieve this goal; it also helps if the PR is not "building a whole new module" or a "whole new UI"

[anshubansal2000]

@danjacobson-orbis Do you want to collaborate on this issue and other issues? I think this requires a code change and introduce a condition in cloudsplaining to honor "Condition Flag". Pls let me know if you want to collaborate. I am thinking to build a whole new module with nice UI.

@anshubansal2000 Thank you the offer to collaborate! I would like to eventually resolve this issue. However, as for building a new module with UI, my organization is running Cloudsplaining "headless", from code - so I won't be contributing to UI work on this project.

Update: I started tracing this through the code, and it appears the only place condition restrictions are checked in the codebase is in statement_detail.py: StatementDetails.has_condition.

That property is only checked by the code paths used for

• Privilege Escalation
• Data Exfiltration
• Credentials Exposure

So I'm guessing (haven't tested yet to confirm) that the presence of conditions will not suppress findings for any of

• Infrastructure Modification
• Resource Exposure
• Service Wildcard

Those three issue types use somewhat different methods of identifying problem actions (including calling out to Policy Sentry libraries), so the respective methods in either statement_detail (or policy_document, for Service Wildcard) are probably our targets for fixing this (assuming it is correct for Cloudsplaining to skip all finding types where conditions exist!)