Repository: tfsec/tfsec
Pull request #768 (2021-06-07 11:05:04)

Add a check for IAM policies for KMS
AWS SecurityHub flags policies that grant permissions
to all possible KMS keys. This falls under the umbrella of "least
privilege" as having such broad permissions would allow an entity
to decrypt data they are not privy to.

felipesere: This is the first time I'm contributing a check like this so I'd be keen to see if I missed anything.
Also, I'm worried the pattern might be too broad/not broad enough?
owenrumney: I appreciate my use of the work "simplified" might be open for argument - I mean more that it is in keeping with self-contained checks
felipesere: I took out the changes to `aws046.go` to keep this PR focused.

On a tangential note: We work a lot with SecurityHub and I wonder if there is value in somehow tagging rules with "AWS SecurityHub" and bringing over more of their controls? 

Patch:
diff --git a/internal/app/tfsec/rules/aws097.go b/internal/app/tfsec/rules/aws097.go
new file mode 100644
index 000000000..c16794ed8
--- /dev/null
+++ b/internal/app/tfsec/rules/aws097.go
@@ -0,0 +1,96 @@
+package rules
+
+import (
+	"fmt"
+	"github.com/tfsec/tfsec/pkg/severity"
+
+	"github.com/tfsec/tfsec/pkg/result"
+
+	"github.com/tfsec/tfsec/pkg/provider"
+
+	"github.com/tfsec/tfsec/internal/app/tfsec/hclcontext"
+
+	"github.com/tfsec/tfsec/internal/app/tfsec/block"
+
+	"github.com/tfsec/tfsec/pkg/rule"
+
+	"github.com/tfsec/tfsec/internal/app/tfsec/scanner"
+)
+
+const AWSKMSManagedPoliciesShouldNotAllowDecryptionActionsOnAllKMSKeys = "AWS097"
+const AWSKMSManagedPoliciesShouldNotAllowDecryptionActionsOnAllKMSKeysDescription = "IAM customer managed policies should not allow decryption actions on all KMS keys"
+const AWSKMSManagedPoliciesShouldNotAllowDecryptionActionsOnAllKMSKeysImpact = "Identities may be able to decrypt data which they should not have access to"
+const AWSKMSManagedPoliciesShouldNotAllowDecryptionActionsOnAllKMSKeysResolution = "Scope down the resources of the IAM policy to specific keys"
+const AWSKMSManagedPoliciesShouldNotAllowDecryptionActionsOnAllKMSKeysExplanation = `
+IAM policies define which actions an identity (user, group, or role) can perform on which resources. Following security best practices, AWS recommends that you allow least privilege. In other words, you should grant to identities only the kms:Decrypt or kms:ReEncryptFrom permissions and only for the keys that are required to perform a task.
+`
+const AWSKMSManagedPoliciesShouldNotAllowDecryptionActionsOnAllKMSKeysBadExample = `
+data "aws_iam_policy_document" "kms_policy" {
+  statement {
+    principals {
+      type        = "AWS"
+      identifiers = ["arn:aws:iam::${data.aws_caller_identity.current.account_id}:root"]
+    }
+    actions   = ["kms:*"]
+    resources = ["*"]
+  }
+}
+`
+const AWSKMSManagedPoliciesShouldNotAllowDecryptionActionsOnAllKMSKeysGoodExample = `
+data "aws_iam_policy_document" "kms_policy" {
+  statement {
+    principals {
+      type        = "AWS"
+      identifiers = ["arn:aws:iam::${data.aws_caller_identity.current.account_id}:root"]
+    }
+    actions   = ["kms:*"]
+    resources = [aws_kms_key.main.arn]
+  }
+}
+`
+
+func init() {
+	scanner.RegisterCheckRule(rule.Rule{
+		ID: AWSKMSManagedPoliciesShouldNotAllowDecryptionActionsOnAllKMSKeys,
+		Documentation: rule.RuleDocumentation{
+			Summary:     AWSKMSManagedPoliciesShouldNotAllowDecryptionActionsOnAllKMSKeysDescription,
+			Explanation: AWSKMSManagedPoliciesShouldNotAllowDecryptionActionsOnAllKMSKeysExplanation,
+			Impact:      AWSKMSManagedPoliciesShouldNotAllowDecryptionActionsOnAllKMSKeysImpact,
+			Resolution:  AWSKMSManagedPoliciesShouldNotAllowDecryptionActionsOnAllKMSKeysResolution,
+			BadExample:  AWSKMSManagedPoliciesShouldNotAllowDecryptionActionsOnAllKMSKeysBadExample,
+			GoodExample: AWSKMSManagedPoliciesShouldNotAllowDecryptionActionsOnAllKMSKeysGoodExample,
+			Links: []string{
+				"https://docs.aws.amazon.com/securityhub/latest/userguide/securityhub-standards-fsbp-controls.html#fsbp-kms-1",
+			},
+		},
+		Provider:       provider.AWSProvider,
+		RequiredTypes:  []string{"data"},
+		RequiredLabels: []string{"aws_iam_policy_document"},
+		CheckFunc: func(set result.Set, b *block.Block, _ *hclcontext.Context) {
+
+			if statementBlocks := b.GetBlocks("statement"); statementBlocks != nil {
+				for _, statementBlock := range statementBlocks {
+
+					// Denying a broad set of KMS access is fine
+					if statementBlock.HasChild("effect") && statementBlock.GetAttribute("effect").Equals("deny", block.IgnoreCase) {
+						continue
+					}
+
+					if statementBlock.HasChild("actions") && statementBlock.GetAttribute("actions").Contains("kms") {
+						if resources := statementBlock.GetAttribute("resources"); resources != nil {
+							if resources.Contains("*") {
+								set.Add(
+									result.New().
+										WithDescription(fmt.Sprintf("Resource '%s' a policy with KMS actions for all KMS keys.", b.FullName())).
+										WithRange(b.Range()).
+										WithAttributeAnnotation(resources).
+										WithSeverity(severity.Error),
+								)
+							}
+						}
+					}
+				}
+			}
+		},
+	})
+}


Test patch:
diff --git a/internal/app/tfsec/test/aws097_test.go b/internal/app/tfsec/test/aws097_test.go
new file mode 100644
index 000000000..153e844d3
--- /dev/null
+++ b/internal/app/tfsec/test/aws097_test.go
@@ -0,0 +1,94 @@
+package test
+
+import (
+	"testing"
+
+	"github.com/tfsec/tfsec/internal/app/tfsec/rules"
+)
+
+func Test_AWSKMSManagedPoliciesShouldNotAllowDecryptionActionsOnAllKMSKeys(t *testing.T) {
+
+	var tests = []struct {
+		name                  string
+		source                string
+		mustIncludeResultCode string
+		mustExcludeResultCode string
+	}{
+		{
+			name: "outright '*' in policy that contains KMS actions",
+			source: `
+data "aws_iam_policy_document" "kms_policy" {
+  statement {
+    principals {
+      type        = "AWS"
+      identifiers = ["arn:aws:iam::${data.aws_caller_identity.current.account_id}:root"]
+    }
+    actions   = ["kms:*"]
+    resources = ["*"]
+  }
+}
+`,
+			mustIncludeResultCode: rules.AWSKMSManagedPoliciesShouldNotAllowDecryptionActionsOnAllKMSKeys,
+		},
+        {
+            name: "partial key and alias arn with asterisk in them",
+            source: `
+data "aws_iam_policy_document" "kms_policy" {
+  statement {
+    principals {
+      type        = "AWS"
+      identifiers = ["arn:aws:iam::${data.aws_caller_identity.current.account_id}:root"]
+    }
+    actions   = ["kms:*"]
+    resources = [
+      "arn:aws:kms:*:*:alias/*",
+      "arn:aws:kms:*:*:key/*",
+    ]
+  }
+}
+`,
+            mustIncludeResultCode: rules.AWSKMSManagedPoliciesShouldNotAllowDecryptionActionsOnAllKMSKeys,
+        },
+        {
+            name: "partial key and alias arn with asterisk in them",
+            source: `
+data "aws_iam_policy_document" "kms_policy" {
+  statement {
+    principals {
+      type        = "AWS"
+      identifiers = ["arn:aws:iam::${data.aws_caller_identity.current.account_id}:root"]
+    }
+    actions   = ["kms:*"]
+    resources = ["arn:aws:kms:123456789:eu-west-1:key/hflksadhfldsk"]
+  }
+}
+`,
+            mustExcludeResultCode: rules.AWSKMSManagedPoliciesShouldNotAllowDecryptionActionsOnAllKMSKeys,
+        },
+        {
+            name: "denies access to any KMS resource",
+            source: `
+data "aws_iam_policy_document" "kms_policy" {
+  statement {
+    principals {
+      type        = "AWS"
+      identifiers = ["arn:aws:iam::${data.aws_caller_identity.current.account_id}:root"]
+    }
+    effect = "deny"
+    actions   = ["kms:*"]
+    resources = ["*"]
+  }
+}
+`,
+            mustExcludeResultCode: rules.AWSKMSManagedPoliciesShouldNotAllowDecryptionActionsOnAllKMSKeys,
+        },
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			results := scanSource(test.source)
+			assertCheckCode(t, test.mustIncludeResultCode, test.mustExcludeResultCode, results)
+		})
+	}
+
+}