Merge "[CVE-2016-9599] Enforce Firewall TCP / UDP rules management"

3 files changed, 54 insertions, 11 deletions

diff --git a/manifests/firewall/rule.pp b/manifests/firewall/rule.pp
index 6801dc4..816e6fe 100644
--- a/manifests/firewall/rule.pp
+++ b/manifests/firewall/rule.pp
@@ -77,8 +77,16 @@ define tripleo::firewall::rule (
   $extras      = {},
 ) {
 
+  if $port == 'all' {
+    warning("All ${proto} traffic will be open on this host.")
+    # undef so the IPtables rule won't have any port specified.
+    $port_real = undef
+  } else {
+    $port_real = $port
+  }
+
   $basic = {
-    'port'        => $port,
+    'port'        => $port_real,
     'dport'       => $dport,
     'sport'       => $sport,
     'proto'       => $proto,
@@ -100,6 +108,15 @@ define tripleo::firewall::rule (
   $rule = merge($basic, $state_rule, $extras)
   validate_hash($rule)
 
+  # This conditional will ensure that TCP and UDP firewall rules have
+  # a port specified in the configuration when using INPUT or OUTPUT chains.
+  # If not, the Puppet catalog will fail.
+  # If we don't do this sanity check, a user could create some TCP/UDP
+  # rules without port, and the result would be an iptables rule that allow any
+  # traffic on the host.
+  if ($proto in ['tcp', 'udp']) and (! ($port or $dport or $sport) and ($chain != 'FORWARD')) {
+    fail("${title} firewall rule cannot be created. TCP or UDP rules for INPUT or OUTPUT need port or sport or dport.")
+  }
   create_resources('firewall', { "${title}" => $rule })
 
 }
diff --git a/manifests/haproxy/endpoint.pp b/manifests/haproxy/endpoint.pp
index 4311049..0bba245 100644
--- a/manifests/haproxy/endpoint.pp
+++ b/manifests/haproxy/endpoint.pp
@@ -149,14 +149,27 @@ define tripleo::haproxy::endpoint (
   }
   if hiera('manage_firewall', true) {
     include ::tripleo::firewall
-    $firewall_rules = {
-      "100 ${name}_haproxy"     => {
-        'dport' => $service_port,
-      },
-      "100 ${name}_haproxy_ssl" => {
-        'dport' => $public_ssl_port,
-      },
+    # This block will construct firewall rules only when we specify
+    # a port for the regular service and also the ssl port for the service.
+    # It makes sure we're not trying to create TCP iptables rules where no port
+    # is specified.
+    if $service_port {
+      $haproxy_firewall_rules = {
+        "100 ${name}_haproxy"     => {
+          'dport' => $service_port,
+        },
+      }
+    }
+    if $public_ssl_port {
+      $haproxy_ssl_firewall_rules = {
+        "100 ${name}_haproxy_ssl" => {
+          'dport' => $public_ssl_port,
+        },
+      }
+    }
+    $firewall_rules = merge($haproxy_firewall_rules, $haproxy_ssl_firewall_rules)
+    if $service_port or $public_ssl_port {
+      create_resources('tripleo::firewall::rule', $firewall_rules)
     }
-    create_resources('tripleo::firewall::rule', $firewall_rules)
   }
 }
diff --git a/spec/classes/tripleo_firewall_spec.rb b/spec/classes/tripleo_firewall_spec.rb
index 3116a51..3a1a0a0 100644
--- a/spec/classes/tripleo_firewall_spec.rb
+++ b/spec/classes/tripleo_firewall_spec.rb
@@ -74,7 +74,7 @@ describe 'tripleo::firewall' do
           :firewall_rules => {
             '300 add custom application 1' => {'port' => '999', 'proto' => 'udp', 'action' => 'accept'},
             '301 add custom application 2' => {'port' => '8081', 'proto' => 'tcp', 'action' => 'accept'},
-            '302 fwd custom cidr 1'        => {'chain' => 'FORWARD', 'destination' => '192.0.2.0/24'},
+            '302 fwd custom cidr 1'        => {'port' => 'all', 'chain' => 'FORWARD', 'destination' => '192.0.2.0/24'},
             '303 add custom application 3' => {'dport' => '8081', 'proto' => 'tcp', 'action' => 'accept'},
             '304 add custom application 4' => {'sport' => '1000', 'proto' => 'tcp', 'action' => 'accept'},
             '305 add gre rule'             => {'proto' => 'gre'}
@@ -96,7 +96,8 @@ describe 'tripleo::firewall' do
         )
         is_expected.to contain_firewall('302 fwd custom cidr 1').with(
           :chain   => 'FORWARD',
-          :destination  => '192.0.2.0/24',
+          :proto   => 'tcp',
+          :destination => '192.0.2.0/24',
         )
         is_expected.to contain_firewall('303 add custom application 3').with(
           :dport   => '8081',
@@ -114,6 +115,18 @@ describe 'tripleo::firewall' do
       end
     end
 
+    context 'with TCP rule without port or dport or sport specified' do
+      before :each do
+        params.merge!(
+          :manage_firewall => true,
+          :firewall_rules  => {
+            '500 wrong tcp rule' => {'proto' => 'tcp', 'action' => 'accept'},
+          }
+        )
+      end
+      it_raises 'a Puppet::Error', /500 wrong tcp rule firewall rule cannot be created. TCP or UDP rules for INPUT or OUTPUT need port or sport or dport./
+    end
+
   end
 
   on_supported_os.each do |os, facts|