Repository: tfsec/tfsec
Pull request #583 (2021-02-09 10:21:15)

When checking submatches, look for all possible candidate blocks
`subMatch` and `subMatches` originally only looked for the first named block to evaluate.
With resources like IAM policy documents, one can have multiple repetitions of `statement`, making it impossible
to check the second.

I believe this changes the semantics slightly: Every block that could match by name must fulfil the `subMatch`, `subMatches`.  This had no negative impact on our suite. It actually caught a few more policies with potential issues.

Let me know if you feel like this needs some toggle/flag.


Patch:
diff --git a/internal/app/tfsec/custom/processing.go b/internal/app/tfsec/custom/processing.go
index 0f28d3fda..bc25f95d9 100644
--- a/internal/app/tfsec/custom/processing.go
+++ b/internal/app/tfsec/custom/processing.go
@@ -154,19 +154,23 @@ func evalMatchSpec(block *parser.Block, spec *MatchSpec, ctx *scanner.Context) b
 	evalResult = matchFunctions[spec.Action](block, spec)
 
 	if spec.SubMatch != nil {
-		if block.HasBlock(spec.Name) {
-			block = block.GetBlock(spec.Name)
+		for _, block := range block.GetBlocks(spec.Name) {
+			evalResult = evalMatchSpec(block, spec.SubMatch, nil)
+			if !evalResult {
+				break
+			}
 		}
-		evalResult = evalMatchSpec(block, spec.SubMatch, nil)
 	}
+
 	if len(spec.SubMatches) > 0 {
-		if block.HasBlock(spec.Name) {
-			block = block.GetBlock(spec.Name)
-		}
-		for i := range spec.SubMatches {
-			evalResult = evalMatchSpec(block, &spec.SubMatches[i], nil)
-			if !evalResult {
-				break
+
+	AllBlocks:
+		for _, block := range block.GetBlocks(spec.Name) {
+			for i := range spec.SubMatches {
+				evalResult = evalMatchSpec(block, &spec.SubMatches[i], nil)
+				if !evalResult {
+					break AllBlocks
+				}
 			}
 		}
 	}


Test patch:
diff --git a/internal/app/tfsec/custom/check_all_statements_test.go b/internal/app/tfsec/custom/check_all_statements_test.go
new file mode 100644
index 000000000..c66d01158
--- /dev/null
+++ b/internal/app/tfsec/custom/check_all_statements_test.go
@@ -0,0 +1,166 @@
+package custom
+
+import (
+	"github.com/stretchr/testify/assert"
+	"testing"
+)
+
+func init() {
+	givenCheck(`{
+  "checks": [
+    {
+      "code": "DP011",
+      "description": "Amazon S3 permissions granted to other AWS accounts in bucket policies should be restricted",
+      "requiredTypes": [
+        "data"
+      ],
+      "requiredLabels": [
+        "aws_iam_policy_document"
+      ],
+      "severity": "ERROR",
+      "matchSpec": {
+        "name": "statement",
+        "action": "isPresent",
+        "subMatch": {
+          "name": "actions",
+          "action": "notContains",
+          "value": "s3:DeleteBucketPolicy",
+          "ignoreUndefined": true
+        }
+      }
+    },
+    {
+      "code": "DP012",
+      "description": "Amazon S3 permissions granted to other AWS accounts in bucket policies should be restricted",
+      "requiredTypes": [
+        "data"
+      ],
+      "requiredLabels": [
+        "imaginary_resource"
+      ],
+      "severity": "ERROR",
+      "matchSpec": {
+        "name": "statement",
+        "action": "isPresent",
+        "subMatches": [
+          {
+            "name": "actions",
+            "action": "notContains",
+            "value": "s3:Foo",
+            "ignoreUndefined": true
+          },
+          {
+            "name": "actions",
+            "action": "notContains",
+            "value": "s3:Bar",
+            "ignoreUndefined": true
+          }
+        ]
+      }
+    }
+  ]
+}
+`)
+}
+
+func TestSingleStatementMatches(t *testing.T) {
+	scanResults := scanTerraform(t, `
+data "aws_iam_policy_document" "bucket_policy" {
+  statement {
+    principals {
+      type = "AWS"
+      identifiers = ["*"]
+    }
+    actions = [
+      "s3:DeleteBucketPolicy",
+      "s3:GetObjectTagging",
+    ]
+    resources = ["*"]
+  }
+}
+
+`)
+	assert.Len(t, scanResults, 1)
+}
+
+func TestMultipleStatementsSecondMatch(t *testing.T) {
+	scanResults := scanTerraform(t, `
+data "aws_iam_policy_document" "bucket_policy" {
+  statement {}
+
+  statement {
+    principals {
+      type = "AWS"
+      identifiers = ["*"]
+    }
+    actions = [
+      "s3:DeleteBucketPolicy",
+      "s3:GetObjectTagging",
+    ]
+    resources = ["*"]
+  }
+}
+`)
+	assert.Len(t, scanResults, 1)
+}
+
+func TestMultipleStatementsFirstMatch(t *testing.T) {
+	scanResults := scanTerraform(t, `
+data "aws_iam_policy_document" "bucket_policy" {
+
+  statement {
+    principals {
+      type = "AWS"
+      identifiers = ["*"]
+    }
+    actions = [
+      "s3:DeleteBucketPolicy",
+      "s3:GetObjectTagging",
+    ]
+    resources = ["*"]
+  }
+
+  statement {}
+}
+`)
+	assert.Len(t, scanResults, 1)
+}
+
+func TestNoMatches(t *testing.T) {
+	scanResults := scanTerraform(t, `
+data "aws_iam_policy_document" "bucket_policy" {
+  statement {}
+}
+`)
+	assert.Len(t, scanResults, 0)
+}
+func TestMultipleSubmatchesOntoMultipleStatemetns(t *testing.T) {
+	scanResults := scanTerraform(t, `
+data "imaginary_resource" "bucket_policy" {
+  statement {
+    actions = ["s3:Foo"]
+  }
+}
+`)
+	assert.Len(t, scanResults, 1)
+
+	scanResults = scanTerraform(t, `
+data "imaginary_resource" "bucket_policy" {
+  statement {
+    actions = ["s3:Bar"]
+  }
+}
+`)
+	assert.Len(t, scanResults, 1)
+
+	scanResults = scanTerraform(t, `
+data "imaginary_resource" "bucket_policy" {
+  statement {}
+
+  statement {
+    actions = ["s3:Bar"]
+  }
+}
+`)
+	assert.Len(t, scanResults, 1)
+}