From 880d95fb6fb7165ed4027ea2051eba6d91a62897 Mon Sep 17 00:00:00 2001
From: Gadi Naor <gadi_naor@rapid7.com>
Date: Thu, 27 Jan 2022 19:37:17 +0200
Subject: [PATCH] - verify allowedTo field exist before accessing it

---
 pkg/analysis/default-rules.yaml    | 18 +++++++--------
 pkg/analysis/default_rules_test.go | 37 ++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 9 deletions(-)

diff --git a/pkg/analysis/default-rules.yaml b/pkg/analysis/default-rules.yaml
index af17963..898dd88 100644
--- a/pkg/analysis/default-rules.yaml
+++ b/pkg/analysis/default-rules.yaml
@@ -40,7 +40,7 @@ Rules:
     # In the expression when evaluating resources - use plural form (secrets not secret)
     AnalysisExpr: |
         subjects.filter(
-            subject, subject.allowedTo.exists(
+            subject, has(subject.allowedTo) && subject.allowedTo.exists(
               rule,
               (has(rule.verb)     && rule.verb in ['get', '*'])         && (has(rule.resource)
               && rule.resource in ['secrets', '*']) &&                         (has(rule.apiGroup)
@@ -71,7 +71,7 @@ Rules:
     # In the expression when evaluating rule.resource - use plural form (secrets not secret)
     AnalysisExpr: |
       subjects.filter(
-        subject, subject.allowedTo.exists(
+        subject, has(subject.allowedTo) && subject.allowedTo.exists(
           rule,
           (has(rule.verb)
            && rule.verb in ['create', 'update', 'patch', '*']) &&(has(rule.resource)
@@ -97,7 +97,7 @@ Rules:
     # In the expression when evaluating rule.resource - use plural form (secrets not secret)
     AnalysisExpr: |
       subjects.filter(
-        subject, subject.allowedTo.exists(
+        subject, has(subject.allowedTo) && subject.allowedTo.exists(
           rule,
           (has(rule.verb)
            && rule.verb in ['impersonate', '*'])         &&(has(rule.resource)
@@ -125,7 +125,7 @@ Rules:
     # In the expression when evaluating rule.resource - use plural form (secrets not secret)
     AnalysisExpr: |
       subjects.filter(
-          subject, subject.allowedTo.exists(
+          subject, has(subject.allowedTo) && subject.allowedTo.exists(
               rule,
               (has(rule.verb)     && rule.verb in ['bind', 'create', 'update', 'patch', 'escalate', '*']) &&
               (has(rule.resource) && rule.resource in ['clusterroles', 'roles', '*']) &&
@@ -150,7 +150,7 @@ Rules:
     # In the expression when evaluating rule.resource - use plural form (secrets not secret)
     AnalysisExpr: |
       subjects.filter(
-        subject, subject.allowedTo.exists(
+        subject, has(subject.allowedTo) && subject.allowedTo.exists(
           rule,
           (has(rule.verb) && rule.verb in ['create', 'delete', 'update', 'patch', '*']) &&
           (
@@ -186,7 +186,7 @@ Rules:
     # In the expression when evaluating rule.resource - use plural form (secrets not secret)
     AnalysisExpr: |
       subjects.filter(
-        subject, subject.allowedTo.exists(
+        subject, has(subject.allowedTo) && subject.allowedTo.exists(
           rule,
           (has(rule.verb) && rule.verb in ['create', 'delete', 'update', 'patch', '*']) &&
           (
@@ -224,7 +224,7 @@ Rules:
     # In the expression when evaluating rule.resource - use plural form (secrets not secret)
     AnalysisExpr: |
       subjects.filter(
-        subject, subject.allowedTo.exists(
+        subject, has(subject.allowedTo) && subject.allowedTo.exists(
           rule,
           (has(rule.verb) && rule.verb in ['create', 'update', 'patch', '*']) &&
           (
@@ -250,7 +250,7 @@ Rules:
     # In the expression when evaluating rule.resource - use plural form (secrets not secret)
     AnalysisExpr: |
       subjects.filter(
-        subject, subject.allowedTo.exists(
+        subject, has(subject.allowedTo) && subject.allowedTo.exists(
           rule,
           (has(rule.verb) && rule.verb in ['create', 'update', 'patch', 'delete', '*']) &&
           (
@@ -277,7 +277,7 @@ Rules:
     # In the expression when evaluating rule.resource - use plural form (secrets not secret)
     AnalysisExpr: |
       subjects.filter(
-        subject, subject.allowedTo.exists(
+        subject, has(subject.allowedTo) && subject.allowedTo.exists(
           rule,
           (has(rule.verb) && rule.verb in ['create', 'update', 'patch', 'delete', '*']) &&
           (
diff --git a/pkg/analysis/default_rules_test.go b/pkg/analysis/default_rules_test.go
index 3825033..2b5afdf 100644
--- a/pkg/analysis/default_rules_test.go
+++ b/pkg/analysis/default_rules_test.go
@@ -158,3 +158,40 @@ func Test__RuleExclusion(t *testing.T) {
 
 	//t.Logf("%v", pretty.Sprint(report))
 }
+
+func Test__EmptySubjectPolicyList(t *testing.T) {
+	defer klog.Flush()
+
+	config := DefaultAnalysisConfig()
+
+	analyzer := CreateAnalyzer(
+		config,
+		[]rbac.SubjectPolicyList{
+			{Subject: v1.Subject{
+				Kind:      "ServiceAccount",
+				APIGroup:  "",
+				Name:      "test-sa",
+				Namespace: "test",
+			},
+				AllowedTo: []rbac.NamespacedPolicyRule{},
+			},
+		},
+	)
+
+	if analyzer == nil {
+		t.Fail()
+	}
+
+	report, err := analyzer.Analyze()
+	if err != nil {
+		t.Fatalf("Analysis failed - %v", err)
+		t.Fail()
+	}
+
+	if len(report.Findings) != 0 {
+		t.Fatalf("Expecting no findings")
+		t.Fail()
+	}
+
+	//t.Logf("%v", pretty.Sprint(report))
+}