Update Network ACL overlapping port rule (Fixes #487, #483) (#491)

* Update tests for network acl entry issues (Fixes #487, Fixes #483)
- Test for port values from references
- Test for entries without logical nacl resource
- Test for correctly evaluating metadata to ignore warning
Fix up various linting errors in yaml templates
Refactor nacl grouping logic

* Protect against missing port range

lib/cfn-nag/custom_rules/EC2NetworkAclEntryOverlappingPortsRule.rb

@@ -18,79 +18,80 @@ def rule_id
   end
 
   def audit_impl(cfn_model)
+    nacl_entries = cfn_model.resources_by_type('AWS::EC2::NetworkAclEntry')
+
+    # Select nacl entries that can be evaluated
+    nacl_entries.select! do |nacl_entry|
+      tcp_or_udp_protocol?(nacl_entry) && valid_ports?(nacl_entry)
+    end
+
     violating_nacl_entries = []
-    cfn_model.resources_by_type('AWS::EC2::NetworkAcl').each do |nacl|
-      violating_nacl_entries += violating_nacl_entries(nacl)
+
+    # Group entries by nacl id, ip type, and egress/ingress
+    grouped_nacl_entries = group_nacl_entries(nacl_entries)
+
+    grouped_nacl_entries.each do |grouping|
+      violating_nacl_entries += overlapping_port_entries(grouping)
     end
     violating_nacl_entries.map(&:logical_resource_id)
   end
 
   private
 
-  def overlapping_port_entries(nacl_entries)
-    unique_pairs(nacl_entries).select do |nacl_entry_pair|
-      tcp_or_udp_protocol?(nacl_entry_pair[0], nacl_entry_pair[1]) && overlap?(nacl_entry_pair[0], nacl_entry_pair[1])
-    end
+  def tcp_or_udp_protocol?(entry)
+    %w[6 17].include?(entry.protocol.to_s)
   end
 
-  def tcp_or_udp_protocol?(entry1, entry2)
-    %w[6 17].include?(entry1.protocol.to_s) && %w[6 17].include?(entry2.protocol.to_s)
+  def valid_ports?(entry)
+    !entry.portRange.nil? && valid_port_number?(entry.portRange['From']) && valid_port_number?(entry.portRange['To'])
   end
 
-  def unique_pairs(arr)
-    pairs_without_dupes = arr.product(arr).select { |pair| pair[0] != pair[1] }
-    pairs_without_dupes.reduce(Set.new) { |set_of_sets, pair| set_of_sets << Set.new(pair) }.to_a.map(&:to_a)
+  def valid_port_number?(port)
+    port.is_a?(Numeric) || (port.is_a?(String) && port.to_i(10) != 0)
   end
 
-  def overlap?(entry1, entry2)
-    roverlap?(entry1, entry2) || loverlap?(entry1, entry2)
-  end
+  def group_nacl_entries(nacl_entries)
+    grouped_nacl_entries = []
 
-  def roverlap?(entry1, entry2)
-    entry1.portRange['From'].between?(entry2.portRange['From'], entry2.portRange['To']) ||
-      entry1.portRange['To'].between?(entry2.portRange['From'], entry2.portRange['To'])
-  end
+    # Group by NaclID
+    nacl_entries.group_by(&:networkAclId).each_value do |entries|
+      # Split entries by ip type
+      ipv4_entries, ipv6_entries = entries.partition { |nacl_entry| nacl_entry.ipv6CidrBlock.nil? }
 
-  def loverlap?(entry1, entry2)
-    entry2.portRange['From'].between?(entry1.portRange['From'], entry1.portRange['To']) ||
-      entry2.portRange['To'].between?(entry1.portRange['From'], entry1.portRange['To'])
-  end
+      # Split entries by egress/ingress
+      egress4, ingress4 = ipv4_entries.partition { |nacl_entry| truthy?(nacl_entry.egress) }
+      egress6, ingress6 = ipv6_entries.partition { |nacl_entry| truthy?(nacl_entry.egress) }
 
-  def egress_entries(nacl_entries)
-    nacl_entries.select do |nacl_entry|
-      truthy?(nacl_entry.egress)
+      grouped_nacl_entries << egress4
+      grouped_nacl_entries << ingress4
+      grouped_nacl_entries << egress6
+      grouped_nacl_entries << ingress6
     end
-  end
 
-  def ingress_entries(nacl_entries)
-    nacl_entries.select do |nacl_entry|
-      not_truthy?(nacl_entry.egress)
-    end
+    grouped_nacl_entries
   end
 
-  def ip6_entries(nacl_entries)
-    nacl_entries.select do |nacl_entry|
-      !nacl_entry.ipv6CidrBlock.nil?
-    end
+  def overlapping_port_entries(nacl_entries)
+    unique_pairs(nacl_entries).select do |nacl_entry_pair|
+      overlap?(nacl_entry_pair[0], nacl_entry_pair[1])
+    end.flatten.uniq
   end
 
-  def ip4_entries(nacl_entries)
-    nacl_entries.select do |nacl_entry|
-      nacl_entry.ipv6CidrBlock.nil?
-    end
+  def unique_pairs(arr)
+    pairs_without_dupes = arr.product(arr).select { |pair| pair[0] != pair[1] }
+    pairs_without_dupes.reduce(Set.new) { |set_of_sets, pair| set_of_sets << Set.new(pair) }.to_a.map(&:to_a)
   end
 
-  def violating_nacl_entries(nacl)
-    violating_ip4_nacl_entries(nacl) || violating_ip6_nacl_entries(nacl)
+  def overlap?(entry1, entry2)
+    port_overlap?(entry1.portRange, entry2.portRange) || port_overlap?(entry2.portRange, entry1.portRange)
   end
 
-  def violating_ip4_nacl_entries(nacl)
-    overlapping_port_entries(egress_entries(ip4_entries(nacl.network_acl_entries))).flatten.uniq &&
-      overlapping_port_entries(ingress_entries(ip4_entries(nacl.network_acl_entries))).flatten.uniq
+  def port_overlap?(port_range1, port_range2)
+    port_number(port_range1['From']).between?(port_number(port_range2['From']), port_number(port_range2['To'])) ||
+      port_number(port_range1['To']).between?(port_number(port_range2['From']), port_number(port_range2['To']))
   end
 
-  def violating_ip6_nacl_entries(nacl)
-    overlapping_port_entries(egress_entries(ip6_entries(nacl.network_acl_entries))).flatten.uniq &&
-      overlapping_port_entries(ingress_entries(ip6_entries(nacl.network_acl_entries))).flatten.uniq
+  def port_number(port)
+    port.to_i
   end
 end

spec/cfn_nag_integration/cfn_nag_ec2_network_acl_entry_spec.rb

@@ -0,0 +1,30 @@
+require 'spec_helper'
+require 'cfn-nag/cfn_nag_config'
+require 'cfn-nag/cfn_nag'
+require 'cfn-nag/cfn_nag_logging'
+
+describe CfnNag do
+  before(:all) do
+    CfnNagLogging.configure_logging(debug: false)
+    @cfn_nag = CfnNag.new(config: CfnNagConfig.new)
+  end
+
+  context 'when nacl entry rule has metadata ignore' do
+    it 'does not warn' do
+      template_name = 'yaml/ec2_networkaclentry/metadata_overlapping_ports.yml'
+
+      expected_aggregate_results = [
+        {
+          filename: test_template_path(template_name),
+          file_results: {
+            failure_count: 0,
+            violations: []
+          }
+        }
+      ]
+
+      actual_aggregate_results = @cfn_nag.audit_aggregate_across_files input_path: test_template_path(template_name)
+      expect(actual_aggregate_results).to eq expected_aggregate_results
+    end
+  end
+end

spec/custom_rules/EC2NetworkAclEntryOverlappingPortsRule_spec.rb

@@ -89,4 +89,42 @@
       expect(actual_logical_resource_ids).to eq expected_logical_resource_ids
     end
   end
+
+  context 'EC2 Network ACLs entries contain references' do
+    it 'returns an empty list' do
+      cfn_model = CfnParser.new.parse read_test_template(
+                                        'yaml/ec2_networkaclentry/ref_ports.yml'
+                                      )
+
+      actual_logical_resource_ids = EC2NetworkAclEntryOverlappingPortsRule.new.audit_impl cfn_model
+      expected_logical_resource_ids = %w[]
+
+      expect(actual_logical_resource_ids).to eq expected_logical_resource_ids
+    end
+
+    it 'returns warnings when provided conflicting parameter values' do
+      cfn_model = CfnParser.new.parse read_test_template(
+                                        'yaml/ec2_networkaclentry/ref_ports.yml'
+                                      ),
+                                      JSON.generate({Parameters: { NaclPort: 80 }})
+
+      actual_logical_resource_ids = EC2NetworkAclEntryOverlappingPortsRule.new.audit_impl cfn_model
+      expected_logical_resource_ids = %w[InboundHTTPPublicNetworkAclEntry InboundHTTPPublicNetworkAclEntry2]
+
+      expect(actual_logical_resource_ids).to eq expected_logical_resource_ids
+    end
+  end
+
+  context 'EC2 Network ACLs entries without direct nacl resource' do
+    it 'returns expected duplicates' do
+      cfn_model = CfnParser.new.parse read_test_template(
+                                        'yaml/ec2_networkaclentry/overlapping_ports_no_nacl.yml'
+                                      )
+
+      actual_logical_resource_ids = EC2NetworkAclEntryOverlappingPortsRule.new.audit_impl cfn_model
+      expected_logical_resource_ids = %w[BadNaclEntry1 BadNaclEntry2]
+
+      expect(actual_logical_resource_ids).to eq expected_logical_resource_ids
+    end
+  end
 end

spec/test_templates/yaml/ec2_networkaclentry/ec2_networkaclentry_port_overlap_non_tcp_udp.yml

@@ -17,7 +17,7 @@ Resources:
       CidrBlock: "10.0.0.0/16"
       Egress: false
       NetworkAclId: !Ref 'myNetworkAcl'
-      Protocol: '-1'
+      Protocol: -1
       RuleAction: allow
       RuleNumber: 1000
   myNetworkAclEntry2:
@@ -26,6 +26,6 @@ Resources:
       CidrBlock: "10.0.0.0/16"
       Egress: false
       NetworkAclId: !Ref 'myNetworkAcl'
-      Protocol: '-1'
+      Protocol: -1
       RuleAction: allow
       RuleNumber: 2000

spec/test_templates/yaml/ec2_networkaclentry/ec2_networkaclentry_port_overlaps.yml

@@ -15,48 +15,47 @@ Resources:
     Type: AWS::EC2::NetworkAclEntry
     Properties:
       NetworkAclId: !Ref myNetworkAcl
-      Protocol: "6"
+      Protocol: 6
       RuleAction: "allow"
-      RuleNumber: "100"
+      RuleNumber: 100
       CidrBlock: "10.0.0.0/16"
       Egress: false
       PortRange:
-        From: '400'
-        To: '500'
+        From: 400
+        To: 500
   myNetworkAclEntry2:
     Type: AWS::EC2::NetworkAclEntry
     Properties:
       NetworkAclId: !Ref myNetworkAcl
-      Protocol: "6"
+      Protocol: 6
       RuleAction: "deny"
-      RuleNumber: "200"
+      RuleNumber: 200
       CidrBlock: "0.0.0.0/0"
       Egress: false
       PortRange:
-        From: '450'
-        To: '475'
+        From: 450
+        To: 475
   myNetworkAclEntry3:
     Type: AWS::EC2::NetworkAclEntry
     Properties:
       NetworkAclId: !Ref myNetworkAcl
-      Protocol: "6"
+      Protocol: 6
       RuleAction: "deny"
-      RuleNumber: "300"
+      RuleNumber: 300
       CidrBlock: "0.0.0.0/0"
       Egress: false
       PortRange:
-        From: '200'
-        To: '200'
+        From: 200
+        To: 200
   myNetworkAclEntry4:
     Type: AWS::EC2::NetworkAclEntry
     Properties:
       NetworkAclId: !Ref myNetworkAcl
-      Protocol: "6"
+      Protocol: 6
       RuleAction: "deny"
-      RuleNumber: "300"
+      RuleNumber: 300
       CidrBlock: "0.0.0.0/0"
       Egress: false
       PortRange:
-        From: '300'
-        To: '400'
-
+        From: 300
+        To: 400

spec/test_templates/yaml/ec2_networkaclentry/metadata_overlapping_ports.yml

@@ -0,0 +1,37 @@
+---
+AWSTemplateFormatVersion: '2010-09-09'
+Description: Test
+Parameters:
+    PublicNetworkAcl:
+      Type: String
+      Description: 'Nacl Port'
+Resources:
+  InboundHTTPPublicNetworkAclEntry:
+    Type: AWS::EC2::NetworkAclEntry
+    Metadata:
+      cfn_nag:
+        rules_to_suppress:
+          - id: W72
+            reason: I know what I am doing
+    Properties:
+      NetworkAclId: !Ref 'PublicNetworkAcl'
+      RuleNumber: 100
+      Protocol: 6
+      RuleAction: 'allow'
+      Egress: false
+      CidrBlock: '0.0.0.0/0'
+      PortRange:
+        From: 80
+        To:  80
+  InboundHTTPPublicNetworkAclEntry2:
+    Type: AWS::EC2::NetworkAclEntry
+    Properties:
+      NetworkAclId: !Ref 'PublicNetworkAcl'
+      RuleNumber: 101
+      Protocol: 6
+      RuleAction: 'allow'
+      Egress: false
+      CidrBlock: '0.0.0.0/0'
+      PortRange:
+        From: 80
+        To:  80

spec/test_templates/yaml/ec2_networkaclentry/overlapping_ports_no_nacl.yml

@@ -0,0 +1,47 @@
+---
+AWSTemplateFormatVersion: '2010-09-09'
+Description: Test
+Parameters:
+    NaclIdOne:
+      Type: String
+      Description: 'Id for first Nacl'
+    NaclIdTwo:
+      Type: String
+      Description: 'Id for first Nacl'
+Resources:
+  BadNaclEntry1:
+    Type: AWS::EC2::NetworkAclEntry
+    Properties:
+      NetworkAclId: !Ref 'NaclIdOne'
+      RuleNumber: 100
+      Protocol: 6
+      RuleAction: 'allow'
+      Egress: false
+      CidrBlock: '0.0.0.0/0'
+      PortRange:
+        From: 80
+        To:  80
+  BadNaclEntry2:
+    Type: AWS::EC2::NetworkAclEntry
+    Properties:
+      NetworkAclId: !Ref 'NaclIdOne'
+      RuleNumber: 101
+      Protocol: 6
+      RuleAction: 'allow'
+      Egress: false
+      CidrBlock: '0.0.0.0/0'
+      PortRange:
+        From: 80
+        To:  80
+  GoodNaclEntry1:
+    Type: AWS::EC2::NetworkAclEntry
+    Properties:
+      NetworkAclId: !Ref 'NaclIdTwo'
+      RuleNumber: 101
+      Protocol: 6
+      RuleAction: 'allow'
+      Egress: false
+      CidrBlock: '0.0.0.0/0'
+      PortRange:
+        From: 80
+        To:  80