nccgroup / ScoutSuite

Multi-Cloud Security Auditing Tool
GNU General Public License v2.0
6.66k stars 1.06k forks source link

AWS CloudFormation "Role Passed to Stack" has incorrect condition #968

Open sbutler opened 3 years ago

sbutler commented 3 years ago

Describe the bug

The finding "Role Passed to Stack" has an incorrect condition. The intent to is find CloudFormation stacks where a role has not been specified. The linked AWS documentation for the finding says:

In most cases, users require full access to manage all of the resources in a template. AWS CloudFormation makes calls to create, modify, and delete those resources on their behalf. To separate permissions between a user and the AWS CloudFormation service, use a service role. AWS CloudFormation uses the service role's policy to make calls instead of the user's policy.

However, the finding check does this:

{
    "description": "Role Passed to Stack",
    "rationale": "Passing a role to CloudFormation stacks may result in privilege escalation because IAM users with privileges within the CloudFormation scope implicitly inherit the stack's role's permissions. Consequently, it should be ensured that the IAM privileges assigned to the stack's role follow the principle of least privilege.",
    "references": [
        "https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/best-practices.html"
    ],
    "dashboard_name": "Stacks",
    "path": "cloudformation.regions.id.stacks.id",
    "conditions": [
        "and",
        [
            "this",
            "withKey",
            "iam_role"
        ],
        [
            "this.iam_role.id",
            "notEmpty",
            ""
        ]
    ]
}

The check "notEmpty" should instead be "empty", to find CloudFormation stacks without roles.

To Reproduce

Please provide:

x4v13r64 commented 3 years ago

The intent to is find CloudFormation stacks where a role has not been specified.

No, our objective here is to flag stacks that do have a role to review them for least privilege configuration, as it's a privilege escalation vector (as per the finding rationale).

juanitosvq commented 3 years ago

Hey, we are struggling a bit with this one. Is the intention to flag all the stacks that have a role passed to them? We are not sure how to resolve this particular error, is it supposed to flag the roles so that they can be manually investigated?

sbutler commented 3 years ago

In our scenario, we are using CloudFormation as part of CodePipelines and the CodePipeline docs say the RoleArn is required for CloudFormation. We've decided to ignore this finding in our scans as it appears unresolvable in that scenario.

ScoutSuites recommendation is to remove the RoleArn from your CloudFormation stacks and to let CloudFormation use the role of the user invoking the create/update/delete option (it's default behavior if it has no role configured). But like I said, that's not possible in all situations.

juanitosvq commented 3 years ago

@sbutler thanks for replying so quickly! Yeah we are in exactly the same situation, so we'll also have to ignore this finding.

Shouldn't this be changed so that if the role used is a service role that only allows cloudformation.amazonaws.com access, then it shouldn't be flagged as an error? https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/using-iam-servicerole.html

our objective here is to flag stacks that do have a role to review them for least privilege configuration @j4v I still don't understand what you mean by to review them in your response. I was expecting that if we use a service role with only the permissions needed to create the resources, this shouldn't be flagged as an error.

Thanks!

x4v13r64 commented 3 years ago

is it supposed to flag the roles so that they can be manually investigated?

Yes

We are not sure how to resolve this particular error

As with all findings, they are potential issues that should be manually reviewed. If you don't want a specific error to pop up every time you run Scout, use an exceptions file (https://github.com/nccgroup/ScoutSuite/wiki/Creating-and-Using-an-Exceptions-File).

Shouldn't this be changed so that if the role used is a service role that only allows cloudformation.amazonaws.com access, then it shouldn't be flagged as an error?

Ideally, yes. The difficulty in implementing this is that rules are typically per-service. This one checks the properties of a stack's role, but validating the configuration of said role is part of another service (IAM). Adding logic to evaluate the role's properties would require adding stuff to https://github.com/nccgroup/ScoutSuite/blob/master/ScoutSuite/providers/aws/provider.py#L57.

I was expecting that if we use a service role with only the permissions needed to create the resources, this shouldn't be flagged as an error.

That's a lot of "stuff" to evaluate within a rule. As mentioned above checking if it's a service role is possible although it requires adding some code. Evaluating "the permissions needed" would be opening pandora's box, there's only so much an automated tool can do for you.

x4v13r64 commented 3 years ago

Ideally, yes. The difficulty in implementing this is that rules are typically per-service. This one checks the properties of a stack's role, but validating the configuration of said role is part of another service (IAM). Adding logic to evaluate the role's properties would require adding stuff to https://github.com/nccgroup/ScoutSuite/blob/master/ScoutSuite/providers/aws/provider.py#L57.

Reopening to implement this eventually.

juanitosvq commented 3 years ago

Thanks for the detailed answer, @j4v, that all makes sense.