fix(query): update ebs not optimized queries (Checkmarx#5020)

* fix(query): update ebs not optimized queries - Add a list of optimized queries by default on `common.json`; - Add a function on `common.rego` to check if instance type is listed on optimized by default instance's list; - Add this check to ebs optimizes rules on terraform, ansible and cloud formation; - Add new test files to validate cases where instance type is optimized by default; - Fix some minor errors; For this fix is important to note there is an explanation which documentation was used to decide what is default value for instance type. Signed-off-by: Felipe Avelar <[email protected]> * fix test samples Signed-off-by: Felipe Avelar <[email protected]> * removed default value for terraform Signed-off-by: Felipe Avelar <[email protected]> * add key check to default ebs optimized instances when ebs_optimized is false Signed-off-by: Felipe Avelar <[email protected]>
Dipesh-Reen · Mar 25, 2022 · d6dee9e · d6dee9e
1 parent 10f8906
commit d6dee9e
Show file tree

Hide file tree

Showing 22 changed files with 782 additions and 54 deletions.
diff --git a/assets/libraries/common.json b/assets/libraries/common.json
diff --git a/assets/libraries/common.rego b/assets/libraries/common.rego
@@ -442,8 +442,8 @@ is_unrestricted(sourceRange) {
 	sourceRange == cidrs[_]
 }
 
-# Matches a value against a list of patterns 
-# and returns set of unique verdicts in form of boolean values  
+# Matches a value against a list of patterns
+# and returns set of unique verdicts in form of boolean values
 get_match_verdicts(value, patterns) = match_verdicts{
    match_verdicts := {regex.match(pattern, value) | pattern := normalize_to_list(patterns)[_]}
 }
@@ -463,7 +463,7 @@ aws_wc_to_re(string, not_flag) = re_pat{
 }
 
 
-# Normalizes a value to list 
+# Normalizes a value to list
 # Useful as Principal, Action etc can contain both string and array of string type
 normalize_to_list(values) = values{
     is_array(values)
@@ -486,7 +486,7 @@ convert_value_to_regex(statement, field) = value{
 	value = []
 }
 
-# Principal is a little complex object which can have sub-fields such as AWS, Service unlike 
+# Principal is a little complex object which can have sub-fields such as AWS, Service unlike
 # resource and action which can have only string or array of strings. This method normalizes
 # the Principal and NotPrincipal fields in regex compatible policy
 handle_principle_regex(statement, principal_field) = regex_compatible_principals{
@@ -499,7 +499,7 @@ handle_principle_regex(statement, principal_field) = regex_compatible_principals
     principal != []
     is_object(principal)
     available_keys = [key | _ = principal[key]]
-    # The below statement creates an array of individual objects looking something like below 
+    # The below statement creates an array of individual objects looking something like below
     # [{"aws": ["asd.*"]}, {"service": ["lambda.amazonaws.com"]}]
     # Couldn't find a way to dynamically update an object hence relying on list comprehension.
     # There needs to be a better way to return it as single object than this off beat array format.
@@ -520,8 +520,8 @@ normalize_statement(statement) = ps {
          "not_action": convert_value_to_regex(statement, "NotAction"),
          "resource": convert_value_to_regex(statement, "Resource"),
          "not_resource": convert_value_to_regex(statement, "NotResource"),
-         # Condition is not normalized as other fields since it is technically not feasible to 
-         # evaluate in a declarative fashion. The consumer may have to manually evaluate the 
+         # Condition is not normalized as other fields since it is technically not feasible to
+         # evaluate in a declarative fashion. The consumer may have to manually evaluate the
          # condition based on the context of a control
          "condition": object.get(statement, "Condition", []),
          "sid": object.get(statement, "Sid", "")
@@ -534,7 +534,7 @@ make_regex_compatible_policy_statement(policy_statements) = regex_compatible_sta
 }
 
 # This method helps in evaluating if an action matches in a statement.
-# In case true is returned , it just means that this action is affected by this statement. 
+# In case true is returned , it just means that this action is affected by this statement.
 # One has to explicitly check whether it is allowed / denied
 statement_matches_action(statement, action) {
 	object.get(statement, "not_action", []) != []
@@ -545,7 +545,7 @@ statement_matches_action(statement, action) {
 }
 
 # This method helps in evaluating if a resource matches in a statement.
-# In case true is returned , it just means that this resource's access is affected by this statement. 
+# In case true is returned , it just means that this resource's access is affected by this statement.
 # One has to explicitly check whether it is allowed / denied.
 statement_matches_resource(statement, resource) {
 	object.get(statement, "not_resource", []) != []
@@ -556,7 +556,7 @@ statement_matches_resource(statement, resource) {
 }
 
 # This method helps in evaluating if a principal matches in a statement.
-# In case true is returned , it just means that this principal's access is affected by this statement. 
+# In case true is returned , it just means that this principal's access is affected by this statement.
 # One has to explicitly check whether it is allowed / denied.
 statement_matches_principal(statement, principal, principal_type) {
 	not_principal_object := object.get(statement, "not_principal", "empty")
@@ -599,11 +599,11 @@ statement_matches_sid(statement, pattern) {
 }
 
 # Checks whether statement explicitly denies action
-# This does not consider context of denial such as if action is denied only 
+# This does not consider context of denial such as if action is denied only
 # on any specific resource.
 # This only considers action and effect.
 statement_explicitly_denies_action(statement, action) {
-	# If a policy has a condition field, then it signifies that it is a conditional denial and not 
+	# If a policy has a condition field, then it signifies that it is a conditional denial and not
     # absolute denial which also means there exists a scope for denial to not apply for a provided context.
 	# Hence it is reasonable to not consider it for all general purposes.
     not statement_has_condition(statement)
@@ -612,7 +612,7 @@ statement_explicitly_denies_action(statement, action) {
 }
 
 # Checks whether statement explicitly allows action
-# This does not consider context of allowance such as if action is allowed only 
+# This does not consider context of allowance such as if action is allowed only
 # on any specific resource.
 # This only considers action and effect.
 statement_explicitly_allows_action(statement, action) {
@@ -621,11 +621,11 @@ statement_explicitly_allows_action(statement, action) {
 }
 
 # Checks whether statement explicitly is denied on a resource
-# This does not consider context of denial such as if resource is denied only 
+# This does not consider context of denial such as if resource is denied only
 # on any specifc case.
 # This only considers resource and effect.
 statement_explicitly_denies_resource(statement, resource) {
-	# If a policy has a condition field, then it signifies that it is a conditional denial and not 
+	# If a policy has a condition field, then it signifies that it is a conditional denial and not
     # absolute denial which also means there exists a scope for denial to not apply for a provided context.
 	# Hence it is reasonable to not consider it for all general purposes.
     not statement_has_condition(statement)
@@ -634,7 +634,7 @@ statement_explicitly_denies_resource(statement, resource) {
 }
 
 # Checks whether statement explicitly is allowed on a resource
-# This does not consider context of allowance such as if resource is allowed only 
+# This does not consider context of allowance such as if resource is allowed only
 # on any specifc case.
 # This only considers resource and effect.
 statement_explicitly_allows_resource(statement, resource) {
@@ -643,7 +643,7 @@ statement_explicitly_allows_resource(statement, resource) {
 }
 
 statement_explicitly_denies_principal(statement, principal, principal_type) {
-	# If a policy has a condition field, then it signifies that it is a conditional denial and not 
+	# If a policy has a condition field, then it signifies that it is a conditional denial and not
     # absolute denial which also means there exists a scope for denial to not apply for a provided context.
 	# Hence it is reasonable to not consider it for all general purposes.
     not statement_has_condition(statement)
@@ -671,7 +671,7 @@ has_highly_permissive_principal(policy_statement){
 	policy_statement.principal[_] == ".*"
     statement_allows(policy_statement)
 } else {
-	# A statement with NotPrincipal and with a non wild card value is 
+	# A statement with NotPrincipal and with a non wild card value is
     # practically equivalent of allowing world leaving a reasonably most probable small subset. Hence this stance.
 	policy_statement.not_principal[_] != ".*"
     statement_allows(policy_statement)
@@ -744,3 +744,9 @@ remove_last_point(searchKey) = sk {
 } else = sk {
    sk := searchKey
 }
+
+# This function is based on this docs(https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ebs-optimized.html#describe-ebs-optimization)
+
+is_aws_ebs_optimized_by_default(instanceType) {
+	inArray(data.common_lib.aws_ebs_optimized_by_default, instanceType)
+}
diff --git a/assets/queries/ansible/aws/ec2_not_ebs_optimized/query.rego b/assets/queries/ansible/aws/ec2_not_ebs_optimized/query.rego
@@ -9,14 +9,16 @@ CxPolicy[result] {
 	ec2 := task[modules[m]]
 	ans_lib.checkState(ec2)
 
+	instanceType := get_instance_type(ec2)
+	not common_lib.is_aws_ebs_optimized_by_default(instanceType)
 	not common_lib.valid_key(ec2, "ebs_optimized")
 
 	result := {
 		"documentId": id,
 		"searchKey": sprintf("name={{%s}}.{{%s}}", [task.name, modules[m]]),
 		"issueType": "MissingAttribute",
 		"keyExpectedValue": "ec2 to have ebs_optimized set to true.",
-		"keyActualValue":  "ec2 doesn't have ebs_optimized set to true.",
+		"keyActualValue": "ec2 doesn't have ebs_optimized set to true.",
 	}
 }
 
@@ -26,14 +28,23 @@ CxPolicy[result] {
 	ec2 := task[modules[m]]
 	ans_lib.checkState(ec2)
 
-	ec2["ebs_optimized"] == false
+	instanceType := get_instance_type(ec2)
+	not common_lib.is_aws_ebs_optimized_by_default(instanceType)
+	ec2.ebs_optimized == false
 
 	result := {
 		"documentId": id,
-		"searchKey": sprintf("name={{%s}}.{{%s}}", [task.name, modules[m]]),
+		"searchKey": sprintf("name={{%s}}.{{%s}}.ebs_optimized", [task.name, modules[m]]),
 		"issueType": "IncorrectValue",
 		"keyExpectedValue": "ec2 to have ebs_optimized set to true.",
-		"keyActualValue":  "ec2 ebs_optimized is set to false.",
+		"keyActualValue": "ec2 ebs_optimized is set to false.",
 	}
 }
 
+# The default InstanceType is t2.micro as defined by thesse docs(https://docs.ansible.com/ansible/latest/collections/amazon/aws/ec2_instance_module.html#ansible-collections-amazon-aws-ec2-instance-module)
+get_instance_type(instanceProperties) = result {
+	common_lib.valid_key(instanceProperties, "instance_type")
+	result = instanceProperties.instance_type
+} else = result {
+	result = "t2.micro"
+}
diff --git a/.../ec2_not_ebs_optimized/test/negative.yaml → ...ec2_not_ebs_optimized/test/negative1.yaml b/.../ec2_not_ebs_optimized/test/negative.yaml → ...ec2_not_ebs_optimized/test/negative1.yaml
@@ -1,7 +1,6 @@
-- name: example2
+- name: example4
   amazon.aws.ec2:
     key_name: mykey
-    instance_type: t2.micro
     image: ami-123456
     wait: yes
     group: my_sg

diff --git a/assets/queries/ansible/aws/ec2_not_ebs_optimized/test/negative2.yaml b/assets/queries/ansible/aws/ec2_not_ebs_optimized/test/negative2.yaml
@@ -0,0 +1,9 @@
+- name: example5
+  amazon.aws.ec2:
+    key_name: mykey
+    instance_type: t3.nano
+    image: ami-123456
+    wait: yes
+    group: my_sg
+    count: 3
+    vpc_subnet_id: subnet-29e63245
diff --git a/assets/queries/ansible/aws/ec2_not_ebs_optimized/test/negative3.yaml b/assets/queries/ansible/aws/ec2_not_ebs_optimized/test/negative3.yaml
@@ -0,0 +1,10 @@
+- name: example5
+  amazon.aws.ec2:
+    key_name: mykey
+    instance_type: t3.nano
+    image: ami-123456
+    wait: yes
+    group: my_sg
+    count: 3
+    vpc_subnet_id: subnet-29e63245
+    ebs_optimized: false
diff --git a/assets/queries/ansible/aws/ec2_not_ebs_optimized/test/positive3.yaml b/assets/queries/ansible/aws/ec2_not_ebs_optimized/test/positive3.yaml
@@ -0,0 +1,8 @@
+- name: example3
+  amazon.aws.ec2:
+    key_name: mykey
+    image: ami-123456
+    wait: yes
+    group: default
+    count: 3
+    vpc_subnet_id: subnet-29e63245
diff --git a/assets/queries/ansible/aws/ec2_not_ebs_optimized/test/positive_expected_result.json b/assets/queries/ansible/aws/ec2_not_ebs_optimized/test/positive_expected_result.json
@@ -8,7 +8,13 @@
   {
     "queryName": "EC2 Not EBS Optimized",
     "severity": "INFO",
-    "line": 2,
+    "line": 10,
     "fileName": "positive2.yaml"
+  },
+  {
+    "queryName": "EC2 Not EBS Optimized",
+    "severity": "INFO",
+    "line": 2,
+    "fileName": "positive3.yaml"
   }
 ]
diff --git a/assets/queries/cloudFormation/aws/ec2_not_ebs_optimized/query.rego b/assets/queries/cloudFormation/aws/ec2_not_ebs_optimized/query.rego
@@ -4,8 +4,10 @@ import data.generic.common as common_lib
 
 CxPolicy[result] {
 	resource := input.document[i].Resources[name]
-  resource.Type == "AWS::EC2::Instance"
+	resource.Type == "AWS::EC2::Instance"
 
+	instanceType := get_instance_type(resource.Properties)
+	not common_lib.is_aws_ebs_optimized_by_default(instanceType)
 	not common_lib.valid_key(resource.Properties, "EbsOptimized")
 
 	result := {
@@ -19,9 +21,11 @@ CxPolicy[result] {
 
 CxPolicy[result] {
 	resource := input.document[i].Resources[name]
-  resource.Type == "AWS::EC2::Instance"
+	resource.Type == "AWS::EC2::Instance"
 
-	resource.Properties["EbsOptimized"] == false
+	resource.Properties.EbsOptimized == false
+	instanceType := get_instance_type(resource.Properties)
+	not common_lib.is_aws_ebs_optimized_by_default(instanceType)
 
 	result := {
 		"documentId": input.document[i].id,
@@ -31,3 +35,11 @@ CxPolicy[result] {
 		"keyActualValue": sprintf("Resources.%s.Properties.EbsOptimized is set to false.", [name]),
 	}
 }
+
+# The default InstanceType is m1.small as defined by thesse docs(https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ec2-instance.html)
+get_instance_type(instanceProperties) = result {
+	common_lib.valid_key(instanceProperties, "InstanceType")
+	result = instanceProperties.InstanceType
+} else = result {
+	result = "m1.small"
+}
diff --git a/assets/queries/cloudFormation/aws/ec2_not_ebs_optimized/test/negative3.yaml b/assets/queries/cloudFormation/aws/ec2_not_ebs_optimized/test/negative3.yaml
@@ -0,0 +1,16 @@
+Resources:
+  MyEC2Instance:
+    Type: AWS::EC2::Instance
+    Properties:
+      InstanceType: t3.nano
+      ImageId: "ami-79fd7eee"
+      KeyName: "testkey"
+      BlockDeviceMappings:
+        - DeviceName: "/dev/sdm"
+          Ebs:
+            VolumeType: "io1"
+            Iops: "200"
+            DeleteOnTermination: "false"
+            VolumeSize: "20"
+        - DeviceName: "/dev/sdk"
+          NoDevice: {}
diff --git a/assets/queries/cloudFormation/aws/ec2_not_ebs_optimized/test/negative4.json b/assets/queries/cloudFormation/aws/ec2_not_ebs_optimized/test/negative4.json
@@ -0,0 +1,27 @@
+{
+  "Resources": {
+    "MyEC2Instance": {
+      "Type": "AWS::EC2::Instance",
+      "Properties": {
+        "InstanceType": "t3.nano",
+        "ImageId": "ami-79fd7eee",
+        "KeyName": "testkey",
+        "BlockDeviceMappings": [
+          {
+            "DeviceName": "/dev/sdm",
+            "Ebs": {
+              "VolumeType": "io1",
+              "Iops": "200",
+              "DeleteOnTermination": "false",
+              "VolumeSize": "20"
+            }
+          },
+          {
+            "DeviceName": "/dev/sdk",
+            "NoDevice": {}
+          }
+        ]
+      }
+    }
+  }
+}
diff --git a/assets/queries/cloudFormation/aws/ec2_not_ebs_optimized/test/positive5.yaml b/assets/queries/cloudFormation/aws/ec2_not_ebs_optimized/test/positive5.yaml
@@ -0,0 +1,16 @@
+Resources:
+  MyEC2Instance:
+    Type: AWS::EC2::Instance
+    Properties:
+      InstanceType: t2.small
+      ImageId: "ami-79fd7eee"
+      KeyName: "testkey"
+      BlockDeviceMappings:
+        - DeviceName: "/dev/sdm"
+          Ebs:
+            VolumeType: "io1"
+            Iops: "200"
+            DeleteOnTermination: "false"
+            VolumeSize: "20"
+        - DeviceName: "/dev/sdk"
+          NoDevice: {}
diff --git a/assets/queries/cloudFormation/aws/ec2_not_ebs_optimized/test/positive6.json b/assets/queries/cloudFormation/aws/ec2_not_ebs_optimized/test/positive6.json
@@ -0,0 +1,27 @@
+{
+  "Resources": {
+    "MyEC2Instance": {
+      "Type": "AWS::EC2::Instance",
+      "Properties": {
+        "InstanceType": "t2.small",
+        "ImageId": "ami-79fd7eee",
+        "KeyName": "testkey",
+        "BlockDeviceMappings": [
+          {
+            "DeviceName": "/dev/sdm",
+            "Ebs": {
+              "VolumeType": "io1",
+              "Iops": "200",
+              "DeleteOnTermination": "false",
+              "VolumeSize": "20"
+            }
+          },
+          {
+            "DeviceName": "/dev/sdk",
+            "NoDevice": {}
+          }
+        ]
+      }
+    }
+  }
+}