Repository: aquasecurity/tfsec
Pull request #950 (2021-07-26 15:36:51)

Rule for GCP IAM assigning privileged role to service account


Patch:
diff --git a/internal/app/tfsec/block/attribute.go b/internal/app/tfsec/block/attribute.go
index d4cbec228..24e69b35f 100644
--- a/internal/app/tfsec/block/attribute.go
+++ b/internal/app/tfsec/block/attribute.go
@@ -27,6 +27,7 @@ type Attribute interface {
 	GreaterThanOrEqualTo(checkValue interface{}) bool
 	IsDataBlockReference() bool
 	Reference() (*Reference, error)
+	AllReferences() []*Reference
 	IsResourceBlockReference(resourceType string) bool
 	ReferencesBlock(b Block) bool
 	IsResolvable() bool
diff --git a/internal/app/tfsec/block/hclattribute.go b/internal/app/tfsec/block/hclattribute.go
index 32078a540..83b18be0b 100644
--- a/internal/app/tfsec/block/hclattribute.go
+++ b/internal/app/tfsec/block/hclattribute.go
@@ -453,7 +453,7 @@ func (attr *HCLAttribute) IsDataBlockReference() bool {
 	return false
 }
 
-func createDotReferenceFromTraversal(traversals ...hcl.Traversal) *Reference {
+func createDotReferenceFromTraversal(traversals ...hcl.Traversal) (*Reference, error) {
 	var refParts []string
 
 	for _, x := range traversals {
@@ -474,20 +474,56 @@ func (attr *HCLAttribute) Reference() (*Reference, error) {
 	case *hclsyntax.RelativeTraversalExpr:
 		switch s := t.Source.(type) {
 		case *hclsyntax.IndexExpr:
-			collectionRef := createDotReferenceFromTraversal(s.Collection.Variables()...)
+			collectionRef, err := createDotReferenceFromTraversal(s.Collection.Variables()...)
+			if err != nil {
+				return nil, err
+			}
 			key, _ := s.Key.Value(attr.ctx)
 			collectionRef.SetKey(key)
 			return collectionRef, nil
 		default:
-			return createDotReferenceFromTraversal(t.Source.Variables()...), nil
+			return createDotReferenceFromTraversal(t.Source.Variables()...)
 		}
 	case *hclsyntax.ScopeTraversalExpr:
-		return createDotReferenceFromTraversal(t.Traversal), nil
+		return createDotReferenceFromTraversal(t.Traversal)
+	case *hclsyntax.TemplateExpr:
+		refs := attr.referencesInTemplate()
+		if len(refs) == 0 {
+			return nil, fmt.Errorf("no references in template")
+		}
+		return refs[0], nil
 	default:
 		return nil, fmt.Errorf("not a reference: no scope traversal")
 	}
 }
 
+func (attr *HCLAttribute) AllReferences() []*Reference {
+	refs := attr.referencesInTemplate()
+	if len(refs) > 0 {
+		return refs
+	}
+	ref, err := attr.Reference()
+	if err != nil {
+		return nil
+	}
+	return append(refs, ref)
+}
+
+func (attr *HCLAttribute) referencesInTemplate() []*Reference {
+	var refs []*Reference
+	switch t := attr.hclAttribute.Expr.(type) {
+	case *hclsyntax.TemplateExpr:
+		for _, part := range t.Parts {
+			ref, err := createDotReferenceFromTraversal(part.Variables()...)
+			if err != nil {
+				continue
+			}
+			refs = append(refs, ref)
+		}
+	}
+	return refs
+}
+
 func (attr *HCLAttribute) IsResourceBlockReference(resourceType string) bool {
 	switch t := attr.hclAttribute.Expr.(type) {
 	case *hclsyntax.ScopeTraversalExpr:
@@ -498,11 +534,12 @@ func (attr *HCLAttribute) IsResourceBlockReference(resourceType string) bool {
 }
 
 func (attr *HCLAttribute) ReferencesBlock(b Block) bool {
-	ref, err := attr.Reference()
-	if err != nil {
-		return false
+	for _, ref := range attr.AllReferences() {
+		if ref.RefersTo(b) {
+			return true
+		}
 	}
-	return ref.RefersTo(b)
+	return false
 }
 
 func getRawValue(value cty.Value) interface{} {
diff --git a/internal/app/tfsec/block/hclblock.go b/internal/app/tfsec/block/hclblock.go
index 2f9fa8b25..8fcfc9010 100644
--- a/internal/app/tfsec/block/hclblock.go
+++ b/internal/app/tfsec/block/hclblock.go
@@ -311,7 +311,8 @@ func (block *HCLBlock) Reference() *Reference {
 		parts = append(parts, block.Type())
 	}
 	parts = append(parts, block.Labels()...)
-	return newReference(parts)
+	ref, _ := newReference(parts)
+	return ref
 }
 
 // LocalName is the name relative to the current module
diff --git a/internal/app/tfsec/block/reference.go b/internal/app/tfsec/block/reference.go
index f9b776c4c..86e54222e 100644
--- a/internal/app/tfsec/block/reference.go
+++ b/internal/app/tfsec/block/reference.go
@@ -15,30 +15,31 @@ type Reference struct {
 	key       string
 }
 
-func newReference(parts []string) *Reference {
+func newReference(parts []string) (*Reference, error) {
 
 	var ref Reference
 
-	if len(parts) > 0 {
+	if len(parts) == 0 {
+		return nil, fmt.Errorf("cannot create empty reference")
+	}
 
-		blockType, err := TypeFromRefName(parts[0])
-		if err != nil {
-			blockType = &TypeResource
-		}
+	blockType, err := TypeFromRefName(parts[0])
+	if err != nil {
+		blockType = &TypeResource
+	}
 
-		ref.blockType = *blockType
+	ref.blockType = *blockType
 
-		if ref.blockType.removeTypeInReference {
-			ref.typeLabel = parts[0]
-			if len(parts) > 1 {
-				ref.nameLabel = parts[1]
-			}
-		} else {
-			if len(parts) > 1 {
-				ref.typeLabel = parts[1]
-				if len(parts) > 2 {
-					ref.nameLabel = parts[2]
-				}
+	if ref.blockType.removeTypeInReference {
+		ref.typeLabel = parts[0]
+		if len(parts) > 1 {
+			ref.nameLabel = parts[1]
+		}
+	} else {
+		if len(parts) > 1 {
+			ref.typeLabel = parts[1]
+			if len(parts) > 2 {
+				ref.nameLabel = parts[2]
 			}
 		}
 	}
@@ -53,7 +54,7 @@ func newReference(parts []string) *Reference {
 		ref.remainder = parts[3:]
 	}
 
-	return &ref
+	return &ref, nil
 }
 
 func (r *Reference) BlockType() Type {
diff --git a/internal/app/tfsec/hclcontext/context.go b/internal/app/tfsec/hclcontext/context.go
index a52577904..abdab8eb4 100644
--- a/internal/app/tfsec/hclcontext/context.go
+++ b/internal/app/tfsec/hclcontext/context.go
@@ -53,16 +53,14 @@ func (c *Context) GetProviderBlocksByProvider(providerName string, alias string)
 }
 
 func (c *Context) GetReferencedBlock(referringAttr block.Attribute) (block.Block, error) {
-	ref, err := referringAttr.Reference()
-	if err != nil {
-		return nil, err
-	}
-	for _, block := range c.blocks {
-		if ref.RefersTo(block) {
-			return block, nil
+	for _, ref := range referringAttr.AllReferences() {
+		for _, block := range c.blocks {
+			if ref.RefersTo(block) {
+				return block, nil
+			}
 		}
 	}
-	return nil, fmt.Errorf("no block found for reference %s", ref)
+	return nil, fmt.Errorf("no referenced block found in '%s'", referringAttr.Name())
 }
 
 func (c *Context) GetReferencingResources(originalBlock block.Block, referencingLabel string, referencingAttributeName string) (block.Blocks, error) {
diff --git a/internal/app/tfsec/rules/google/iam/no_privileged_service_accounts_rule.go b/internal/app/tfsec/rules/google/iam/no_privileged_service_accounts_rule.go
new file mode 100644
index 000000000..a19b5543b
--- /dev/null
+++ b/internal/app/tfsec/rules/google/iam/no_privileged_service_accounts_rule.go
@@ -0,0 +1,113 @@
+package iam
+
+import (
+	"fmt"
+	"strings"
+
+	"github.com/aquasecurity/tfsec/pkg/result"
+	"github.com/aquasecurity/tfsec/pkg/severity"
+
+	"github.com/aquasecurity/tfsec/pkg/provider"
+
+	"github.com/aquasecurity/tfsec/internal/app/tfsec/hclcontext"
+
+	"github.com/aquasecurity/tfsec/internal/app/tfsec/block"
+
+	"github.com/aquasecurity/tfsec/pkg/rule"
+
+	"github.com/aquasecurity/tfsec/internal/app/tfsec/scanner"
+)
+
+func init() {
+	scanner.RegisterCheckRule(rule.Rule{
+		Service:   "iam",
+		ShortCode: "no-privileged-service-accounts",
+		Documentation: rule.RuleDocumentation{
+			Summary:     "Service accounts should not have roles assigned with excessive privileges",
+			Explanation: `Service accounts should have a minimal set of permissions assigned in order to do their job. They should never have excessive access as if compromised, an attacker can escalate privileges and take over the entire account.`,
+			Impact:      "Cloud account takeover if a resource using a service account is compromised",
+			Resolution:  "Limit service account access to minimal required set",
+			BadExample: `
+resource "google_service_account" "test" {
+  account_id   = "account123"
+  display_name = "account123"
+}
+
+resource "google_project_iam_member" "project" {
+	project = "your-project-id"
+	role    = "roles/owner"
+	member  = "serviceAccount:${google_service_account.test.email}"
+}
+			`,
+			GoodExample: `
+resource "google_service_account" "test" {
+	account_id   = "account123"
+	display_name = "account123"
+}
+
+resource "google_project_iam_member" "project" {
+	project = "your-project-id"
+	role    = "roles/logging.logWriter"
+	member  = "serviceAccount:${google_service_account.test.email}"
+}
+			`,
+			Links: []string{
+				"https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/google_project_iam",
+				"https://cloud.google.com/iam/docs/understanding-roles",
+			},
+		},
+		Provider:        provider.GoogleProvider,
+		RequiredTypes:   []string{"resource"},
+		RequiredLabels:  []string{"google_project_iam_member"},
+		DefaultSeverity: severity.High,
+		CheckFunc: func(set result.Set, resourceBlock block.Block, ctx *hclcontext.Context) {
+
+			// is this a sensitive role?
+			roleAttr := resourceBlock.GetAttribute("role")
+			if roleAttr == nil || !roleAttr.IsString() || !roleAttr.Value().IsKnown() {
+				return
+			}
+			if !isRolePrivileged(roleAttr.Value().AsString()) {
+				return
+			}
+
+			// is it linked to a service account?
+			memberAttr := resourceBlock.GetAttribute("member")
+			if memberAttr == nil {
+				return
+			}
+			if memberAttr.IsString() && memberAttr.Value().IsKnown() {
+				if memberAttr.StartsWith("serviceAccount:") {
+					set.Add(
+						result.New(resourceBlock).
+							WithDescription(fmt.Sprintf("Resource '%s' provides privileged access to a service account", resourceBlock)).
+							WithAttributeAnnotation(roleAttr),
+					)
+				}
+			}
+
+			// the service account may be populated via a templated reference that we don't have, so we need to check references
+			if serviceAccountBlock, err := ctx.GetReferencedBlock(memberAttr); err != nil {
+				return
+			} else if serviceAccountBlock != nil && serviceAccountBlock.TypeLabel() == "google_service_account" {
+				set.Add(
+					result.New(resourceBlock).
+						WithDescription(fmt.Sprintf("Resource '%s' provides privileged access to service account at %s", resourceBlock, serviceAccountBlock.FullName())).
+						WithAttributeAnnotation(roleAttr),
+				)
+			}
+		},
+	})
+}
+
+func isRolePrivileged(role string) bool {
+	switch {
+	case role == "roles/owner":
+		return true
+	case role == "roles/editor":
+		return true
+	case strings.HasSuffix(strings.ToLower(role), "admin"):
+		return true
+	}
+	return false
+}


Test patch:
diff --git a/internal/app/tfsec/rules/google/iam/no_privileged_service_accounts_rule_test.go b/internal/app/tfsec/rules/google/iam/no_privileged_service_accounts_rule_test.go
new file mode 100644
index 000000000..5c8deee56
--- /dev/null
+++ b/internal/app/tfsec/rules/google/iam/no_privileged_service_accounts_rule_test.go
@@ -0,0 +1,92 @@
+package iam
+
+import (
+	"testing"
+
+	"github.com/aquasecurity/tfsec/internal/app/tfsec/testutil"
+)
+
+func Test_GoogleNoPrivilegedServiceAccounts(t *testing.T) {
+	expectedCode := "google-iam-no-privileged-service-accounts"
+
+	var tests = []struct {
+		name                  string
+		source                string
+		mustIncludeResultCode string
+		mustExcludeResultCode string
+	}{
+		{
+			name: "Rule matches when service account is assigned a privileged role (owner)",
+			source: `
+		resource "google_project_iam_member" "project" {
+			project = "your-project-id"
+			role    = "roles/owner"
+			member  = "serviceAccount:test@test.com"
+		}
+		`,
+			mustIncludeResultCode: expectedCode,
+		},
+		{
+			name: "Rule matches when service account is assigned a privileged role (admin)",
+			source: `
+		resource "google_project_iam_member" "project" {
+			project = "your-project-id"
+			role    = "roles/admin"
+			member  = "serviceAccount:test@test.com"
+		}
+		`,
+			mustIncludeResultCode: expectedCode,
+		},
+		{
+			name: "Rule matches when service account is assigned a privileged role (editor)",
+			source: `
+		resource "google_project_iam_member" "project" {
+			project = "your-project-id"
+			role    = "roles/editor"
+			member  = "serviceAccount:test@test.com"
+		}
+		`,
+			mustIncludeResultCode: expectedCode,
+		},
+		{
+			name: "Rule matches when service account is assigned a privileged role via templated reference",
+			source: `
+resource "google_service_account" "test" {
+	account_id   = "account123"
+	display_name = "account123"
+}
+
+resource "google_project_iam_member" "project" {
+	project = "your-project-id"
+	role    = "roles/owner"
+	member  = "serviceAccount:${google_service_account.test.email}"
+}
+`,
+			mustIncludeResultCode: expectedCode,
+		},
+		{
+			name: "Rule not not match when service account is assigned an unprivileged role",
+			source: `
+resource "google_service_account" "test" {
+	account_id   = "account123"
+	display_name = "account123"
+}
+
+resource "google_project_iam_member" "project" {
+	project = "your-project-id"
+	role    = "roles/logging.logWriter"
+	member  = "serviceAccount:${google_service_account.test.email}"
+}
+`,
+			mustExcludeResultCode: expectedCode,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+
+			results := testutil.ScanHCL(test.source, t)
+			testutil.AssertCheckCode(t, test.mustIncludeResultCode, test.mustExcludeResultCode, results)
+		})
+	}
+}