qubes-firewall: reject packets instead of dropping

qubes-firewall service is meant as mistakes mitigation, not a
high-volume external network filter. Providing feedback (ICMP
admin-prohibited error packet) to the VM reduces timeouts and give
immediate connection failure. This is especially useful when some
website tries to load unwanted (not whitelisted) 3rd party elements -
providing error response avoids long site loading time.

Fixes QubesOS/qubes-issues#3607

qubesagent/firewall.py

@@ -330,6 +330,16 @@ class IptablesWorker(FirewallWorker):
             if dsthosts is None:
                 dsthosts = [None]
 
+            if rule['action'] == 'accept':
+                action = 'ACCEPT'
+            elif rule['action'] == 'drop':
+                action = 'REJECT --reject-with {}'.format(
+                    'icmp6-adm-prohibited' if family == 6 else
+                    'icmp-admin-prohibited')
+            else:
+                raise RuleParseError(
+                    'Invalid rule action {}'.format(rule['action']))
+
             # sorting here is only to ease writing tests
             for proto in sorted(protos):
                 for dsthost in sorted(dsthosts):
@@ -342,8 +352,7 @@ class IptablesWorker(FirewallWorker):
                         ipt_rule += ' --dport {}'.format(dstports)
                     if icmptype is not None:
                         ipt_rule += ' --icmp-type {}'.format(icmptype)
-                    ipt_rule += ' -j {}\n'.format(
-                        str(rule['action']).upper())
+                    ipt_rule += ' -j {}\n'.format(action)
                     iptables += ipt_rule
 
         iptables += 'COMMIT\n'
@@ -491,6 +500,15 @@ class NftablesWorker(FirewallWorker):
 
             nft_rule = ""
 
+            if rule['action'] == 'accept':
+                action = 'accept'
+            elif rule['action'] == 'drop':
+                action = 'reject with icmp{} type admin-prohibited'.format(
+                    '6' if family == 6 else '')
+            else:
+                raise RuleParseError(
+                    'Invalid rule action {}'.format(rule['action']))
+
             if 'proto' in rule:
                 if family == 4:
                     nft_rule += ' ip protocol {}'.format(rule['proto'])
@@ -544,16 +562,16 @@ class NftablesWorker(FirewallWorker):
                 if 'proto' not in rule:
                     nft_rules.append(
                         nft_rule + ' tcp dport {} {}'.format(
-                            dstports, rule['action']))
+                            dstports, action))
                     nft_rules.append(
                         nft_rule + ' udp dport {} {}'.format(
-                            dstports, rule['action']))
+                            dstports, action))
                 else:
                     nft_rules.append(
                         nft_rule + ' {} dport {} {}'.format(
-                            rule['proto'], dstports, rule['action']))
+                            rule['proto'], dstports, action))
             else:
-                nft_rules.append(nft_rule + ' ' + rule['action'])
+                nft_rules.append(nft_rule + ' ' + action)
 
         return (
             'flush chain {family} {table} {chain}\n'

qubesagent/test_firewall.py

@@ -198,10 +198,14 @@ class TestIptablesWorker(TestCase):
             "-A chain -d 2.2.2.2/32 -p tcp --dport 53:53 -j ACCEPT\n"
             "-A chain -d 1.1.1.1/32 -p udp --dport 53:53 -j ACCEPT\n"
             "-A chain -d 2.2.2.2/32 -p udp --dport 53:53 -j ACCEPT\n"
-            "-A chain -d 1.1.1.1/32 -p udp --dport 53:53 -j DROP\n"
-            "-A chain -d 2.2.2.2/32 -p udp --dport 53:53 -j DROP\n"
-            "-A chain -p icmp -j DROP\n"
-            "-A chain -j DROP\n"
+            "-A chain -d 1.1.1.1/32 -p udp --dport 53:53 -j REJECT "
+            "--reject-with icmp-admin-prohibited\n"
+            "-A chain -d 2.2.2.2/32 -p udp --dport 53:53 -j REJECT "
+            "--reject-with icmp-admin-prohibited\n"
+            "-A chain -p icmp -j REJECT "
+            "--reject-with icmp-admin-prohibited\n"
+            "-A chain -j REJECT "
+            "--reject-with icmp-admin-prohibited\n"
             "COMMIT\n"
         )
         self.assertEqual(self.obj.prepare_rules('chain', rules, 4),
@@ -232,10 +236,14 @@ class TestIptablesWorker(TestCase):
             "-A chain -d 2001::2/128 -p tcp --dport 53:53 -j ACCEPT\n"
             "-A chain -d 2001::1/128 -p udp --dport 53:53 -j ACCEPT\n"
             "-A chain -d 2001::2/128 -p udp --dport 53:53 -j ACCEPT\n"
-            "-A chain -d 2001::1/128 -p udp --dport 53:53 -j DROP\n"
-            "-A chain -d 2001::2/128 -p udp --dport 53:53 -j DROP\n"
-            "-A chain -p icmpv6 -j DROP\n"
-            "-A chain -j DROP\n"
+            "-A chain -d 2001::1/128 -p udp --dport 53:53 -j REJECT "
+            "--reject-with icmp6-adm-prohibited\n"
+            "-A chain -d 2001::2/128 -p udp --dport 53:53 -j REJECT "
+            "--reject-with icmp6-adm-prohibited\n"
+            "-A chain -p icmpv6 -j REJECT "
+            "--reject-with icmp6-adm-prohibited\n"
+            "-A chain -j REJECT "
+            "--reject-with icmp6-adm-prohibited\n"
             "COMMIT\n"
         )
         self.assertEqual(self.obj.prepare_rules('chain', rules, 6),
@@ -367,9 +375,9 @@ class TestNftablesWorker(TestCase):
             '    ip daddr { 1.1.1.1/32, 2.2.2.2/32 } tcp dport 53 accept\n'
             '    ip daddr { 1.1.1.1/32, 2.2.2.2/32 } udp dport 53 accept\n'
             '    ip protocol udp ip daddr { 1.1.1.1/32, 2.2.2.2/32 } udp dport '
-            '53 drop\n'
-            '    ip protocol icmp drop\n'
-            '    drop\n'
+            '53 reject with icmp type admin-prohibited\n'
+            '    ip protocol icmp reject with icmp type admin-prohibited\n'
+            '    reject with icmp type admin-prohibited\n'
             '  }\n'
             '}\n'
         )
@@ -403,9 +411,10 @@ class TestNftablesWorker(TestCase):
             '    ip6 daddr { 2001::1/128, 2001::2/128 } tcp dport 53 accept\n'
             '    ip6 daddr { 2001::1/128, 2001::2/128 } udp dport 53 accept\n'
             '    ip6 nexthdr udp ip6 daddr { 2001::1/128, 2001::2/128 } '
-            'udp dport 53 drop\n'
-            '    ip6 nexthdr icmpv6 icmpv6 type 128 drop\n'
-            '    drop\n'
+            'udp dport 53 reject with icmp6 type admin-prohibited\n'
+            '    ip6 nexthdr icmpv6 icmpv6 type 128 reject with icmp6 type '
+            'admin-prohibited\n'
+            '    reject with icmp6 type admin-prohibited\n'
             '  }\n'
             '}\n'
         )