Merge remote-tracking branch 'origin/pr/201'

* origin/pr/201:
  update_connected_ips: set iptables policy to drop while updating
  update_connected_ips: reload nftables using one command
  get_connected_ips: handle empty and missing keys, add tests
  update_connected_ips: correctly handle byte-string
  firewall: fix family / family_name
  qubes-firewall: correctly handle empty connected-ips list
  Update tests for anti-spoofing, add test for the method itself
  Update rule priorities for anti-spoofing
  Update firewall tests
  qubes-firewall: add anti-spoofing rules for connected machines

network/ip6tables

@@ -1,4 +1,12 @@
 # Generated by ip6tables-save v1.4.14 on Tue Sep 25 16:00:20 2012
+*raw
+:QBS-PREROUTING - [0:0]
+-A PREROUTING -j QBS-PREROUTING
+COMMIT
+*mangle
+:QBS-POSTROUTING - [0:0
+-A POSTROUTING -j QBS-POSTROUTING
+COMMIT
 *filter
 :INPUT DROP [0:0]
 :FORWARD DROP [0:0]

network/ip6tables-enabled

@@ -10,6 +10,14 @@
 -A POSTROUTING -o lo -j ACCEPT
 -A POSTROUTING -j MASQUERADE
 COMMIT
+*raw
+:QBS-PREROUTING - [0:0]
+-A PREROUTING -j QBS-PREROUTING
+COMMIT
+*mangle
+:QBS-POSTROUTING - [0:0]
+-A POSTROUTING -j QBS-POSTROUTING
+COMMIT
 *filter
 :INPUT DROP [0:0]
 :FORWARD DROP [0:0]

network/iptables

@@ -11,6 +11,14 @@
 -A POSTROUTING -o lo -j ACCEPT
 -A POSTROUTING -j MASQUERADE
 COMMIT
+*raw
+:QBS-PREROUTING - [0:0]
+-A PREROUTING -j QBS-PREROUTING
+COMMIT
+*mangle
+:QBS-POSTROUTING - [0:0]
+-A POSTROUTING -j QBS-POSTROUTING
+COMMIT
 # Completed on Mon Sep  6 08:57:46 2010
 # Generated by iptables-save v1.4.5 on Mon Sep  6 08:57:46 2010
 *filter

qubesagent/firewall.py

@@ -77,6 +77,15 @@ class FirewallWorker(object):
         """Apply rules in given source address"""
         raise NotImplementedError
 
+    def update_connected_ips(self, family):
+        raise NotImplementedError
+
+    def get_connected_ips(self, family):
+        ips = self.qdb.read('/connected-ips6' if family == 6 else '/connected-ips')
+        if ips is None:
+            return []
+        return ips.decode().split()
+
     def run_firewall_dir(self):
         """Run scripts dir contents, before user script"""
         script_dir_paths = ['/etc/qubes/qubes-firewall.d',
@@ -175,15 +184,25 @@ class FirewallWorker(object):
         # initial load
         for source_addr in self.list_targets():
             self.handle_addr(source_addr)
+        self.update_connected_ips(4)
+        self.update_connected_ips(6)
         self.qdb.watch('/qubes-firewall/')
+        self.qdb.watch('/connected-ips')
+        self.qdb.watch('/connected-ips6')
         try:
             for watch_path in iter(self.qdb.read_watch, None):
+                if watch_path == '/connected-ips':
+                    self.update_connected_ips(4)
+
+                if watch_path == '/connected-ips6':
+                    self.update_connected_ips(6)
+
                 # ignore writing rules itself - wait for final write at
                 # source_addr level empty write (/qubes-firewall/SOURCE_ADDR)
-                if watch_path.count('/') > 2:
-                    continue
-                source_addr = watch_path.split('/')[2]
-                self.handle_addr(source_addr)
+                if watch_path.startswith('/qubes-firewall/') and watch_path.count('/') == 2:
+                    source_addr = watch_path.split('/')[2]
+                    self.handle_addr(source_addr)
+
         except OSError:  # EINTR
             # signal received, don't continue the loop
             pass
@@ -391,25 +410,63 @@ class IptablesWorker(FirewallWorker):
         else:
             self.apply_rules_family(source, rules, 4)
 
+    def update_connected_ips(self, family):
+        ips = self.get_connected_ips(family)
+
+        if not ips:
+            # Just flush.
+            self.run_ipt(family, ['-t', 'raw', '-F', 'QBS-PREROUTING'])
+            self.run_ipt(family, ['-t', 'mangle', '-F', 'QBS-POSTROUTING'])
+            return
+
+        # Temporarily set policy to DROP while updating the rules.
+        self.run_ipt(family, ['-t', 'raw', '-P', 'PREROUTING', 'DROP'])
+        self.run_ipt(family, ['-t', 'mangle', '-P', 'POSTROUTING', 'DROP'])
+
+        self.run_ipt(family, ['-t', 'raw', '-F', 'QBS-PREROUTING'])
+        self.run_ipt(family, ['-t', 'mangle', '-F', 'QBS-POSTROUTING'])
+
+        for ip in ips:
+            self.run_ipt(family, [
+                '-t', 'raw', '-A', 'QBS-PREROUTING',
+                '!', '-i', 'vif+', '-s', ip, '-j', 'DROP'])
+            self.run_ipt(family, [
+                '-t', 'mangle', '-A', 'QBS-POSTROUTING',
+                '!', '-o', 'vif+', '-d', ip, '-j', 'DROP'])
+
+        self.run_ipt(family, ['-t', 'raw', '-P', 'PREROUTING', 'ACCEPT'])
+        self.run_ipt(family, ['-t', 'mangle', '-P', 'POSTROUTING', 'ACCEPT'])
+
     def init(self):
-        # make sure 'QBS_FORWARD' chain exists - should be created before
-        # starting qubes-firewall
+        # Chains QBS-FORWARD, QBS-PREROUTING, QBS-POSTROUTING
+        # need to be created before running this.
         try:
             self.run_ipt(4, ['-F', 'QBS-FORWARD'])
             self.run_ipt(4,
                 ['-A', 'QBS-FORWARD', '!', '-i', 'vif+', '-j', 'RETURN'])
             self.run_ipt(4, ['-A', 'QBS-FORWARD', '-j', 'DROP'])
+            self.run_ipt(4, ['-t', 'raw', '-F', 'QBS-PREROUTING'])
+            self.run_ipt(4, ['-t', 'mangle', '-F', 'QBS-POSTROUTING'])
+
             self.run_ipt(6, ['-F', 'QBS-FORWARD'])
             self.run_ipt(6,
                 ['-A', 'QBS-FORWARD', '!', '-i', 'vif+', '-j', 'RETURN'])
             self.run_ipt(6, ['-A', 'QBS-FORWARD', '-j', 'DROP'])
+            self.run_ipt(6, ['-t', 'raw', '-F', 'QBS-PREROUTING'])
+            self.run_ipt(6, ['-t', 'mangle', '-F', 'QBS-POSTROUTING'])
         except subprocess.CalledProcessError:
-            self.log_error('\'QBS-FORWARD\' chain not found, create it first')
+            self.log_error(
+                'Error initializing iptables. '
+                'You probably need to create QBS-FORWARD, QBS-PREROUTING and '
+                'QBS-POSTROUTING chains first.'
+            )
             sys.exit(1)
 
     def cleanup(self):
         for family in (4, 6):
             self.run_ipt(family, ['-F', 'QBS-FORWARD'])
+            self.run_ipt(family, ['-t', 'raw', '-F', 'QBS-PREROUTING'])
+            self.run_ipt(family, ['-t', 'mangle', '-F', 'QBS-POSTROUTING'])
             for chain in self.chains[family]:
                 self.run_ipt(family, ['-F', chain])
                 self.run_ipt(family, ['-X', chain])
@@ -468,6 +525,41 @@ class NftablesWorker(FirewallWorker):
         self.run_nft(nft_input)
         self.chains[family].add(chain)
 
+    def update_connected_ips(self, family):
+        family_name = ('ip6' if family == 6 else 'ip')
+        table = 'qubes-firewall'
+
+        nft_input = (
+            'flush chain {family_name} {table} prerouting\n'
+            'flush chain {family_name} {table} postrouting\n'
+        ).format(family_name=family_name, table=table)
+
+        ips = self.get_connected_ips(family)
+        if ips:
+            addr = '{' + ', '.join(ips) + '}'
+            irule = 'iifname != "vif*" {family_name} saddr {addr} drop\n'.format(
+                family_name=family_name, addr=addr)
+            orule = 'oifname != "vif*" {family_name} daddr {addr} drop\n'.format(
+                family_name=family_name, addr=addr)
+
+            nft_input += (
+                'table {family_name} {table} {{\n'
+                '  chain prerouting {{\n'
+                '    {irule}'
+                '  }}\n'
+                '  chain postrouting {{\n'
+                '    {orule}'
+                '  }}\n'
+                '}}\n'
+            ).format(
+                family_name=family_name,
+                table=table,
+                irule=irule,
+                orule=orule,
+            )
+
+        self.run_nft(nft_input)
+
     def prepare_rules(self, chain, rules, family):
         """
         Helper function to translate rules list into input for iptables-restore
@@ -609,8 +701,6 @@ class NftablesWorker(FirewallWorker):
             self.apply_rules_family(source, rules, 4)
 
     def init(self):
-        # make sure 'QBS_FORWARD' chain exists - should be created before
-        # starting qubes-firewall
         nft_init = (
             'table {family} qubes-firewall {{\n'
             '  chain forward {{\n'
@@ -619,6 +709,14 @@ class NftablesWorker(FirewallWorker):
             '    ct state established,related accept\n'
             '    meta iifname != "vif*" accept\n'
             '  }}\n'
+            '  chain prerouting {{\n'
+            '    type filter hook prerouting priority -300;\n'
+            '    policy accept;\n'
+            '  }}\n'
+            '  chain postrouting {{\n'
+            '    type filter hook postrouting priority -300;\n'
+            '    policy accept;\n'
+            '  }}\n'
             '}}\n'
         )
         nft_init = ''.join(

qubesagent/test_firewall.py

@@ -64,6 +64,7 @@ class FirewallWorker(qubesagent.firewall.FirewallWorker):
         self.init_called = False
         self.cleanup_called = False
         self.user_script_called = False
+        self.update_connected_ips_called_with = []
         self.rules = {}
 
     def apply_rules(self, source_addr, rules):
@@ -78,6 +79,9 @@ class FirewallWorker(qubesagent.firewall.FirewallWorker):
     def run_user_script(self):
         self.user_script_called = True
 
+    def update_connected_ips(self, family):
+        self.update_connected_ips_called_with.append(family)
+
 
 class IptablesWorker(qubesagent.firewall.IptablesWorker):
     '''Override methods actually modifying system state to only log what
@@ -282,11 +286,17 @@ class TestIptablesWorker(TestCase):
         self.assertEqual(self.obj.called_commands[4], [
             ['-F', 'QBS-FORWARD'],
             ['-A', 'QBS-FORWARD', '!', '-i', 'vif+', '-j', 'RETURN'],
-            ['-A', 'QBS-FORWARD', '-j', 'DROP']])
+            ['-A', 'QBS-FORWARD', '-j', 'DROP'],
+            ['-t', 'raw', '-F', 'QBS-PREROUTING'],
+            ['-t', 'mangle', '-F', 'QBS-POSTROUTING'],
+        ])
         self.assertEqual(self.obj.called_commands[6], [
             ['-F', 'QBS-FORWARD'],
             ['-A', 'QBS-FORWARD', '!', '-i', 'vif+', '-j', 'RETURN'],
-            ['-A', 'QBS-FORWARD', '-j', 'DROP']])
+            ['-A', 'QBS-FORWARD', '-j', 'DROP'],
+            ['-t', 'raw', '-F', 'QBS-PREROUTING'],
+            ['-t', 'mangle', '-F', 'QBS-POSTROUTING'],
+        ])
 
     def test_007_cleanup(self):
         self.obj.init()
@@ -300,18 +310,67 @@ class TestIptablesWorker(TestCase):
         self.obj.cleanup()
         self.assertEqual([self.obj.called_commands[4][0]] +
                 sorted(self.obj.called_commands[4][1:], key=operator.itemgetter(1)),
-            [['-F', 'QBS-FORWARD'],
+            [
+                ['-F', 'QBS-FORWARD'],
                 ['-F', 'chain-ip4-1'],
                 ['-X', 'chain-ip4-1'],
                 ['-F', 'chain-ip4-2'],
-                ['-X', 'chain-ip4-2']])
+                ['-X', 'chain-ip4-2'],
+                ['-t', 'mangle', '-F', 'QBS-POSTROUTING'],
+                ['-t', 'raw', '-F', 'QBS-PREROUTING'],
+            ])
         self.assertEqual([self.obj.called_commands[6][0]] +
                 sorted(self.obj.called_commands[6][1:], key=operator.itemgetter(1)),
-            [['-F', 'QBS-FORWARD'],
+            [
+                ['-F', 'QBS-FORWARD'],
                 ['-F', 'chain-ip6-1'],
                 ['-X', 'chain-ip6-1'],
                 ['-F', 'chain-ip6-2'],
-                ['-X', 'chain-ip6-2']])
+                ['-X', 'chain-ip6-2'],
+                ['-t', 'mangle', '-F', 'QBS-POSTROUTING'],
+                ['-t', 'raw', '-F', 'QBS-PREROUTING'],
+            ])
+
+    def test_008_update_connected_ips(self):
+        self.obj.qdb.entries['/connected-ips'] = b'10.137.0.1 10.137.0.2'
+        self.obj.called_commands[4] = []
+        self.obj.update_connected_ips(4)
+
+        self.assertEqual(self.obj.called_commands[4], [
+            ['-t', 'raw', '-P', 'PREROUTING', 'DROP'],
+            ['-t', 'mangle', '-P', 'POSTROUTING', 'DROP'],
+            ['-t', 'raw', '-F', 'QBS-PREROUTING'],
+            ['-t', 'mangle', '-F', 'QBS-POSTROUTING'],
+            ['-t', 'raw', '-A', 'QBS-PREROUTING',
+             '!', '-i', 'vif+', '-s', '10.137.0.1', '-j', 'DROP'],
+            ['-t', 'mangle', '-A', 'QBS-POSTROUTING',
+             '!', '-o', 'vif+', '-d', '10.137.0.1', '-j', 'DROP'],
+            ['-t', 'raw', '-A', 'QBS-PREROUTING',
+             '!', '-i', 'vif+', '-s', '10.137.0.2', '-j', 'DROP'],
+            ['-t', 'mangle', '-A', 'QBS-POSTROUTING',
+             '!', '-o', 'vif+', '-d', '10.137.0.2', '-j', 'DROP'],
+            ['-t', 'raw', '-P', 'PREROUTING', 'ACCEPT'],
+            ['-t', 'mangle', '-P', 'POSTROUTING', 'ACCEPT'],
+        ])
+
+    def test_009_update_connected_ips_empty(self):
+        self.obj.qdb.entries['/connected-ips'] = b''
+        self.obj.called_commands[4] = []
+        self.obj.update_connected_ips(4)
+
+        self.assertEqual(self.obj.called_commands[4], [
+            ['-t', 'raw', '-F', 'QBS-PREROUTING'],
+            ['-t', 'mangle', '-F', 'QBS-POSTROUTING'],
+        ])
+
+    def test_010_update_connected_ips_missing(self):
+        self.obj.called_commands[4] = []
+        self.obj.update_connected_ips(4)
+
+        self.assertEqual(self.obj.called_commands[4], [
+            ['-t', 'raw', '-F', 'QBS-PREROUTING'],
+            ['-t', 'mangle', '-F', 'QBS-POSTROUTING'],
+        ])
 
 
 class TestNftablesWorker(TestCase):
@@ -450,6 +509,14 @@ class TestNftablesWorker(TestCase):
             '    ct state established,related accept\n'
             '    meta iifname != "vif*" accept\n'
             '  }\n'
+            '  chain prerouting {\n'
+            '    type filter hook prerouting priority -300;\n'
+            '    policy accept;\n'
+            '  }\n'
+            '  chain postrouting {\n'
+            '    type filter hook postrouting priority -300;\n'
+            '    policy accept;\n'
+            '  }\n'
             '}\n'
             'table ip6 qubes-firewall {\n'
             '  chain forward {\n'
@@ -458,6 +525,14 @@ class TestNftablesWorker(TestCase):
             '    ct state established,related accept\n'
             '    meta iifname != "vif*" accept\n'
             '  }\n'
+            '  chain prerouting {\n'
+            '    type filter hook prerouting priority -300;\n'
+            '    policy accept;\n'
+            '  }\n'
+            '  chain postrouting {\n'
+            '    type filter hook postrouting priority -300;\n'
+            '    policy accept;\n'
+            '  }\n'
             '}\n'
         ])
 
@@ -473,7 +548,44 @@ class TestNftablesWorker(TestCase):
         self.assertEqual(self.obj.loaded_rules,
             ['delete table ip qubes-firewall\n'
              'delete table ip6 qubes-firewall\n',
-             ])
+            ])
+
+    def test_008_update_connected_ips(self):
+        self.obj.qdb.entries['/connected-ips'] = b'10.137.0.1 10.137.0.2'
+        self.obj.loaded_rules = []
+        self.obj.update_connected_ips(4)
+
+        self.assertEqual(self.obj.loaded_rules, [
+            'flush chain ip qubes-firewall prerouting\n'
+            'flush chain ip qubes-firewall postrouting\n'
+            'table ip qubes-firewall {\n'
+            '  chain prerouting {\n'
+            '    iifname != "vif*" ip saddr {10.137.0.1, 10.137.0.2} drop\n'
+            '  }\n'
+            '  chain postrouting {\n'
+            '    oifname != "vif*" ip daddr {10.137.0.1, 10.137.0.2} drop\n'
+            '  }\n'
+            '}\n'
+        ])
+
+    def test_009_update_connected_ips_empty(self):
+        self.obj.qdb.entries['/connected-ips'] = b''
+        self.obj.loaded_rules = []
+        self.obj.update_connected_ips(4)
+
+        self.assertEqual(self.obj.loaded_rules, [
+            'flush chain ip qubes-firewall prerouting\n'
+            'flush chain ip qubes-firewall postrouting\n'
+        ])
+
+    def test_010_update_connected_ips_missing(self):
+        self.obj.loaded_rules = []
+        self.obj.update_connected_ips(4)
+
+        self.assertEqual(self.obj.loaded_rules, [
+            'flush chain ip qubes-firewall prerouting\n'
+            'flush chain ip qubes-firewall postrouting\n'
+        ])
 
 class TestFirewallWorker(TestCase):
     def setUp(self):
@@ -567,5 +679,6 @@ class TestFirewallWorker(TestCase):
         self.assertTrue(self.obj.init_called)
         self.assertTrue(self.obj.cleanup_called)
         self.assertTrue(self.obj.user_script_called)
+        self.assertEqual(self.obj.update_connected_ips_called_with, [4, 6])
         self.assertEqual(set(self.obj.rules.keys()), self.obj.list_targets())
         # rules content were already tested