toolkit: limit default security warnings to "risky" changes

[rix0rrr]

Right now, the toolkit will warn on ANY IAM/SecurityGroup widening. We should probably restrict this to "risky" changes in order to avoid alarm fatigue.

With the current implementation, it will most likely put up warnings on every deployment that adds new resources, as most of them involve creating a new IAM role.

---

Attacks

Here are some attack scenarios (by 3rd party construct library authors) that we should ideally be able to detect.

• Opens up permissions to other account - the malicious construct opens up permissions on a resource to somewhere outside the deployed AWS account.
  • Example: adds Principal: * to a bucket policy.
  • Fairly easy to detect, as the dangerous permissions can immediately be seen from the diff.
• Opens up permissions via Lambda - the malicious construct creates a Lambda that opens up the permissions later.
  • Example: adds Resource: [Bucket1, Bucket2], Action: s3:SetBucketPolicy to the Lambdas execution role.
  • Harder but possible to detect, as a Lambda gets permission to do permission-related calls (Control Plane calls).
• Exfiltrates data via Lambda directly - the malicious construct creates a Lambda that runs periodically, reads all S3 buckets and sends the data out over the internet.
  • Example: adds Resource: *, Action: s3:GetObject to the Lambda Execution Role.
  • More advanced example: iterates over all Buckets in the construct tree and adds Resource: [Bucket1, Bucket2, ...], Action: s3:GetObject to the Lambda Execution Role to bypass * checks.
  • Very hard to detect, especially if the 2nd variant is used. To detect this, we need to have the knowledge that the Lambda could have no possible reason to need to access the indicated buckets.

[rix0rrr]

Proposing the following guiding principle:

Things "external" to the template are risky. This includes things external to the account, which are even more risky. Relationships between objects internal to the template should be considered non-risky.

[rix0rrr]

I know I should write these things in a MarkDown document, but I'm putting my thoughts here right now because I don't want to switch branches.

---

I'm proposing the following rules:

IAM

• IAM roles with an AssumeRole policy that is NOT an AWS Service.

• IAM identity policies that have a resource not in the same template. This encompasses policies that have resource *.

  • This will unfortunately tag all roles that need to do CloudWatch things, because they'll have a resource-* policy. We can get rid of this common false positive by either inspection Actions (dangerous logic) or switching to a managed policy for log-writing permissions (see below).

• IAM identity policies that have an Action with a * in it.

• Managed policy attachments that are NOT AWS-managed (we trust managed policies to be safe).

• IAM resource policies that have a principal that is both not a service and not in the same template. This includes root users from any account (including the current).

Security Groups

• Ingress rules with a CIDRIP peer (most likely used to include everyone).
• Ingress rules with a SecurityGroup peer that is not in the same template.

Prefix Lists are all AWS managed so we trust them.

We ignore egress. Enterprises worried about exfiltration have a variety of other options to work with (stricted CDK checking level, inspecting the CDK machine-readable diff, putting boxes in Isolated subnets; general AWS offerings such as VPC flow logs, a variety of security-posture checking tools announced at reinvent, ...).

---

Some implementation notes flying through my head:

• We need to take into account NotResource, NotAction, NotPrincipal as well. Any Not variant used is automatically marked as risky. Let's not try to reason over these. CDK will never generate them, and human changes should be subject to scrutiny.
• Conditions can only serve to restrict further, so do not need to be evaluated for riskiness. They can never make a statement riskier than the riskiness implied by the (Resource, Action, Principal) tuple.

[eladb]

I disagree that "template-locality" implies less risk. One of the main drivers for improving this diff UI is in order to ensure that when people use 3rd party constructs in their code, those constructs will not expose their applications to unwanted risk and/or attacks.

I agree that we'll probably eventually want to reduce noise around these warnings, but I would rather give this some "bake time" and feel it out before we implement any filters.

[rix0rrr]

I would rather give this some "bake time" and feel it out before we implement any filters.

Agreed, but it doesn't hurt to start thinking about it. Also, I fear the requests for filtering are going to come in sooner rather than later.

I disagree that "template-locality" implies less risk.

We should come up with some potential attacks. I'll add them to the post.

[lroling8350]

If/when this idea gets implemented any thoughts on opening it up as a plugin and letting downstream users choose their rules so they could vary the level of risk? The scenario I have is allowing developers to create CDK apps but sending out request for approval when deploying a stack with permission changes. A plugin would allow us to customize the security level as well as a possible post out to a third party app for approval before continuing.

PS. This is an awesome project and I really like the direction it's heading in.

[github-actions[bot]]

This issue has not received any attention in 1 year. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.