From 2a5af195f1e2e4fe4f59aa2bd6e2b60ab1568032 Mon Sep 17 00:00:00 2001 From: 3hhh Date: Fri, 14 May 2021 12:46:20 +0200 Subject: [PATCH 01/11] Export DNS information obtained during firewall setup to QubesDB This can e.g. allow DNS applications to pin a FQDN to the IP used during Qubes OS firewall setup. Can help with QubesOS/qubes-issues#5225 and other related issues. --- qubesagent/firewall.py | 36 +++++++++++++++++++++++++++++++----- 1 file changed, 31 insertions(+), 5 deletions(-) diff --git a/qubesagent/firewall.py b/qubesagent/firewall.py index 1f72bf9..c3c4fff 100755 --- a/qubesagent/firewall.py +++ b/qubesagent/firewall.py @@ -127,6 +127,23 @@ class FirewallWorker(object): rules.append({'action': policy}) return rules + def write_dns_info(self, source, host, hostaddrs): + """ + Write resolved DNS addresses back to QubesDB. This can be useful + for the user or DNS applications to pin these DNS addresses to the + IPs resolved during firewall setup. + + :param source: VM IP + :param host: hostname + :param hostaddrs: set of IP addresses :host: was resolved to + :return: None + """ + self.qdb.write('/dns/{}/{}'.format(source, host), str(hostaddrs)) + + def clear_dns_info(self, source): + """ Clear all DNS info for the given VM IP.""" + self.qdb.rm('/dns/{}/'.format(source)) + def list_targets(self): return set(t.split('/')[2] for t in self.qdb.list('/qubes-firewall/')) @@ -264,13 +281,14 @@ class IptablesWorker(FirewallWorker): ['-I', 'QBS-FORWARD', '-s', addr, '-j', chain]) self.chains[family].add(chain) - def prepare_rules(self, chain, rules, family): + def prepare_rules(self, chain, rules, family, source): """ Helper function to translate rules list into input for iptables-restore :param chain: name of the chain to put rules into :param rules: list of rules :param family: address family (4 or 6) + :param source: source for which to apply the chain :return: input for iptables-restore :rtype: str """ @@ -281,6 +299,8 @@ class IptablesWorker(FirewallWorker): dns = list(addr + fullmask for addr in self.dns_addresses(family)) + self.clear_dns_info(source) + for rule in rules: unsupported_opts = set(rule.keys()).difference( set(self.supported_rule_opts)) @@ -312,6 +332,7 @@ class IptablesWorker(FirewallWorker): raise RuleParseError('Failed to resolve {}: {}'.format( rule['dsthost'], str(e))) dsthosts = set(item[4][0] + fullmask for item in addrinfo) + self.write_dns_info(source, rule['dsthost'], dsthosts) else: dsthosts = None @@ -391,7 +412,7 @@ class IptablesWorker(FirewallWorker): if chain not in self.chains[family]: self.create_chain(source, chain, family) - iptables = self.prepare_rules(chain, rules, family) + iptables = self.prepare_rules(chain, rules, family, source) try: self.run_ipt(family, ['-F', chain]) p = self.run_ipt_restore(family, ['-n']) @@ -559,13 +580,14 @@ class NftablesWorker(FirewallWorker): self.run_nft(nft_input) - def prepare_rules(self, chain, rules, family): + def prepare_rules(self, chain, rules, family, source): """ Helper function to translate rules list into input for iptables-restore :param chain: name of the chain to put rules into :param rules: list of rules :param family: address family (4 or 6) + :param source: source for which to apply the chain :return: input for iptables-restore :rtype: str """ @@ -578,6 +600,8 @@ class NftablesWorker(FirewallWorker): dns = list(addr + fullmask for addr in self.dns_addresses(family)) + self.clear_dns_info(source) + for rule in rules: unsupported_opts = set(rule.keys()).difference( set(self.supported_rule_opts)) @@ -622,8 +646,10 @@ class NftablesWorker(FirewallWorker): except UnicodeError as e: raise RuleParseError('Invalid destination {}: {}'.format( rule['dsthost'], str(e))) + dsthosts = set(item[4][0] + fullmask for item in addrinfo) nft_rule += ' {} daddr {{ {} }}'.format(ip_match, - ', '.join(set(item[4][0] + fullmask for item in addrinfo))) + ', '.join(dsthosts)) + self.write_dns_info(source, rule['dsthost'], dsthosts) if 'dstports' in rule: dstports = rule['dstports'] @@ -694,7 +720,7 @@ class NftablesWorker(FirewallWorker): if chain not in self.chains[family]: self.create_chain(source, chain, family) - self.run_nft(self.prepare_rules(chain, rules, family)) + self.run_nft(self.prepare_rules(chain, rules, family, source)) def apply_rules(self, source, rules): if self.is_ip6(source): From 196014831b73a2cfa5f7de77f14975b884c5ebf6 Mon Sep 17 00:00:00 2001 From: 3hhh Date: Sat, 15 May 2021 10:19:18 +0200 Subject: [PATCH 02/11] firewall: refactor to remove side effects from prepare_rules() --- qubesagent/firewall.py | 50 ++++++++++++++++++++++-------------------- 1 file changed, 26 insertions(+), 24 deletions(-) diff --git a/qubesagent/firewall.py b/qubesagent/firewall.py index c3c4fff..de21647 100755 --- a/qubesagent/firewall.py +++ b/qubesagent/firewall.py @@ -127,23 +127,22 @@ class FirewallWorker(object): rules.append({'action': policy}) return rules - def write_dns_info(self, source, host, hostaddrs): + def update_dns_info(self, source, dns): """ Write resolved DNS addresses back to QubesDB. This can be useful for the user or DNS applications to pin these DNS addresses to the IPs resolved during firewall setup. :param source: VM IP - :param host: hostname - :param hostaddrs: set of IP addresses :host: was resolved to + :param dns: dict: hostname -> set of IP addresses :return: None """ - self.qdb.write('/dns/{}/{}'.format(source, host), str(hostaddrs)) - - def clear_dns_info(self, source): - """ Clear all DNS info for the given VM IP.""" + #clear old info self.qdb.rm('/dns/{}/'.format(source)) + for host, hostaddrs in dns.items(): + self.qdb.write('/dns/{}/{}'.format(source, host), str(hostaddrs)) + def list_targets(self): return set(t.split('/')[2] for t in self.qdb.list('/qubes-firewall/')) @@ -281,16 +280,16 @@ class IptablesWorker(FirewallWorker): ['-I', 'QBS-FORWARD', '-s', addr, '-j', chain]) self.chains[family].add(chain) - def prepare_rules(self, chain, rules, family, source): + def prepare_rules(self, chain, rules, family): """ Helper function to translate rules list into input for iptables-restore :param chain: name of the chain to put rules into :param rules: list of rules :param family: address family (4 or 6) - :param source: source for which to apply the chain - :return: input for iptables-restore - :rtype: str + :return: tuple: (input for iptables-restore, dict of DNS records resolved + during execution) + :rtype: (str, dict) """ iptables = "*filter\n" @@ -299,7 +298,7 @@ class IptablesWorker(FirewallWorker): dns = list(addr + fullmask for addr in self.dns_addresses(family)) - self.clear_dns_info(source) + ret_dns = {} for rule in rules: unsupported_opts = set(rule.keys()).difference( @@ -332,7 +331,7 @@ class IptablesWorker(FirewallWorker): raise RuleParseError('Failed to resolve {}: {}'.format( rule['dsthost'], str(e))) dsthosts = set(item[4][0] + fullmask for item in addrinfo) - self.write_dns_info(source, rule['dsthost'], dsthosts) + ret_dns[rule['dsthost']] = dsthosts else: dsthosts = None @@ -395,7 +394,7 @@ class IptablesWorker(FirewallWorker): iptables += ipt_rule iptables += 'COMMIT\n' - return iptables + return (iptables, ret_dns) def apply_rules_family(self, source, rules, family): """ @@ -412,7 +411,7 @@ class IptablesWorker(FirewallWorker): if chain not in self.chains[family]: self.create_chain(source, chain, family) - iptables = self.prepare_rules(chain, rules, family, source) + (iptables, dns) = self.prepare_rules(chain, rules, family) try: self.run_ipt(family, ['-F', chain]) p = self.run_ipt_restore(family, ['-n']) @@ -420,6 +419,7 @@ class IptablesWorker(FirewallWorker): if p.returncode != 0: raise RuleApplyError( 'iptables-restore failed: {}'.format(output)) + self.update_dns_info(source, dns) except subprocess.CalledProcessError as e: raise RuleApplyError('\'iptables -F {}\' failed: {}'.format( chain, e.output)) @@ -580,16 +580,16 @@ class NftablesWorker(FirewallWorker): self.run_nft(nft_input) - def prepare_rules(self, chain, rules, family, source): + def prepare_rules(self, chain, rules, family): """ - Helper function to translate rules list into input for iptables-restore + Helper function to translate rules list into input for nft :param chain: name of the chain to put rules into :param rules: list of rules :param family: address family (4 or 6) - :param source: source for which to apply the chain - :return: input for iptables-restore - :rtype: str + :return: tuple: (input for nft, dict of DNS records resolved + during execution) + :rtype: (str, dict) """ assert family in (4, 6) @@ -600,7 +600,7 @@ class NftablesWorker(FirewallWorker): dns = list(addr + fullmask for addr in self.dns_addresses(family)) - self.clear_dns_info(source) + ret_dns = {} for rule in rules: unsupported_opts = set(rule.keys()).difference( @@ -649,7 +649,7 @@ class NftablesWorker(FirewallWorker): dsthosts = set(item[4][0] + fullmask for item in addrinfo) nft_rule += ' {} daddr {{ {} }}'.format(ip_match, ', '.join(dsthosts)) - self.write_dns_info(source, rule['dsthost'], dsthosts) + ret_dns[rule['dsthost']] = dsthosts if 'dstports' in rule: dstports = rule['dstports'] @@ -703,7 +703,7 @@ class NftablesWorker(FirewallWorker): table='qubes-firewall', chain=chain, rules='\n '.join(nft_rules) - )) + ), ret_dns) def apply_rules_family(self, source, rules, family): """ @@ -720,7 +720,9 @@ class NftablesWorker(FirewallWorker): if chain not in self.chains[family]: self.create_chain(source, chain, family) - self.run_nft(self.prepare_rules(chain, rules, family, source)) + (nft, dns) = self.prepare_rules(chain, rules, family) + self.run_nft(nft) + self.update_dns_info(source, dns) def apply_rules(self, source, rules): if self.is_ip6(source): From dda500b83752232afe62f5f830777e37a0ebb8f7 Mon Sep 17 00:00:00 2001 From: 3hhh Date: Sat, 15 May 2021 12:33:24 +0200 Subject: [PATCH 03/11] mock qubesdb.rm() --- qubesagent/test_firewall.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/qubesagent/test_firewall.py b/qubesagent/test_firewall.py index 909b3e4..5c107fc 100644 --- a/qubesagent/test_firewall.py +++ b/qubesagent/test_firewall.py @@ -29,6 +29,14 @@ class DummyQubesDB(object): except KeyError: return None + def rm(self, path): + if path.endswith('/'): + for key in self.entries: + if key.startswith(path): + self.entries.pop(key) + else: + self.entries.pop(path) + def multiread(self, prefix): result = {} for key, value in self.entries.items(): From 78de37da927c1f303ca792ba7826a25936c042ce Mon Sep 17 00:00:00 2001 From: 3hhh Date: Sat, 15 May 2021 12:35:50 +0200 Subject: [PATCH 04/11] firewall: mark an IP as handled in /qubes-firewall_handled/[ip] after each handling iteration Actually a counter is increased after each handling iteration. This is useful for user applications to remain up to date with the changes implemented by the Qubes firewall. --- qubesagent/firewall.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/qubesagent/firewall.py b/qubesagent/firewall.py index de21647..a9aa998 100755 --- a/qubesagent/firewall.py +++ b/qubesagent/firewall.py @@ -143,6 +143,19 @@ class FirewallWorker(object): for host, hostaddrs in dns.items(): self.qdb.write('/dns/{}/{}'.format(source, host), str(hostaddrs)) + def update_handled(self, addr): + """ + Update the QubesDB count of how often the given address was handled. + User applications may watch these paths for count increases to remain + up to date with QubesDB changes. + """ + cnt = self.qdb.read('/qubes-firewall_handled/{}'.format(addr)) + try: + cnt = int(cnt) + except (TypeError, ValueError): + cnt = 0 + self.qdb.write('/qubes-firewall_handled/{}'.format(addr), str(cnt+1)) + def list_targets(self): return set(t.split('/')[2] for t in self.qdb.list('/qubes-firewall/')) @@ -179,6 +192,8 @@ class FirewallWorker(object): self.log_error( 'Failed to block traffic for {}'.format(addr)) + self.update_handled(addr) + @staticmethod def dns_addresses(family=None): with open('/etc/resolv.conf') as resolv: From 3067e469d3b16c5f290dbe3ff2a136fef41e4dd4 Mon Sep 17 00:00:00 2001 From: 3hhh Date: Sat, 15 May 2021 22:13:01 +0200 Subject: [PATCH 05/11] firewall: adjust tests to the new tuple returned by prepare_rules() --- qubesagent/test_firewall.py | 44 +++++++++++++++++++++++++++---------- 1 file changed, 32 insertions(+), 12 deletions(-) diff --git a/qubesagent/test_firewall.py b/qubesagent/test_firewall.py index 5c107fc..a721847 100644 --- a/qubesagent/test_firewall.py +++ b/qubesagent/test_firewall.py @@ -1,5 +1,6 @@ import logging import operator +import re from unittest import TestCase from unittest.mock import patch @@ -37,6 +38,9 @@ class DummyQubesDB(object): else: self.entries.pop(path) + def write(self, path, val): + self.entries[path] = val + def multiread(self, prefix): result = {} for key, value in self.entries.items(): @@ -220,8 +224,12 @@ class TestIptablesWorker(TestCase): "--reject-with icmp-admin-prohibited\n" "COMMIT\n" ) - self.assertEqual(self.obj.prepare_rules('chain', rules, 4), - expected_iptables) + ret = self.obj.prepare_rules('chain', rules, 4) + self.assertEqual(ret[0], expected_iptables) + self.assertEqual(ret[1].keys(), {'yum.qubes-os.org'}) + self.assertIsInstance(ret[1]['yum.qubes-os.org'], set) + self.assertIsNotNone(re.match('^\d+\.\d+\.\d+\.\d+/32$', + ret[1]['yum.qubes-os.org'].pop())) with self.assertRaises(qubesagent.firewall.RuleParseError): self.obj.prepare_rules('chain', [{'unknown': 'xxx'}], 4) with self.assertRaises(qubesagent.firewall.RuleParseError): @@ -258,8 +266,12 @@ class TestIptablesWorker(TestCase): "--reject-with icmp6-adm-prohibited\n" "COMMIT\n" ) - self.assertEqual(self.obj.prepare_rules('chain', rules, 6), - expected_iptables) + ret = self.obj.prepare_rules('chain', rules, 6) + self.assertEqual(ret[0], expected_iptables) + self.assertEqual(ret[1].keys(), {'ripe.net'}) + self.assertIsInstance(ret[1]['ripe.net'], set) + self.assertIsNotNone(re.match('^[0-9a-f:]+/\d+$', + ret[1]['ripe.net'].pop())) def test_004_apply_rules4(self): rules = [{'action': 'accept'}] @@ -271,7 +283,7 @@ class TestIptablesWorker(TestCase): ['-I', 'QBS-FORWARD', '-s', '10.137.0.1', '-j', chain], ['-F', chain]]) self.assertEqual(self.obj.loaded_iptables[4], - self.obj.prepare_rules(chain, rules, 4)) + self.obj.prepare_rules(chain, rules, 4)[0]) self.assertEqual(self.obj.called_commands[6], []) self.assertIsNone(self.obj.loaded_iptables[6]) @@ -285,7 +297,7 @@ class TestIptablesWorker(TestCase): ['-I', 'QBS-FORWARD', '-s', '2000::a', '-j', chain], ['-F', chain]]) self.assertEqual(self.obj.loaded_iptables[6], - self.obj.prepare_rules(chain, rules, 6)) + self.obj.prepare_rules(chain, rules, 6)[0]) self.assertEqual(self.obj.called_commands[4], []) self.assertIsNone(self.obj.loaded_iptables[4]) @@ -448,8 +460,12 @@ class TestNftablesWorker(TestCase): ' }\n' '}\n' ) - self.assertEqual(self.obj.prepare_rules('chain', rules, 4), - expected_nft) + ret = self.obj.prepare_rules('chain', rules, 4) + self.assertEqual(ret[0], expected_nft) + self.assertEqual(ret[1].keys(), {'yum.qubes-os.org'}) + self.assertIsInstance(ret[1]['yum.qubes-os.org'], set) + self.assertIsNotNone(re.match('^\d+\.\d+\.\d+\.\d+/32$', + ret[1]['yum.qubes-os.org'].pop())) with self.assertRaises(qubesagent.firewall.RuleParseError): self.obj.prepare_rules('chain', [{'unknown': 'xxx'}], 4) with self.assertRaises(qubesagent.firewall.RuleParseError): @@ -485,8 +501,12 @@ class TestNftablesWorker(TestCase): ' }\n' '}\n' ) - self.assertEqual(self.obj.prepare_rules('chain', rules, 6), - expected_nft) + ret = self.obj.prepare_rules('chain', rules, 6) + self.assertEqual(ret[0], expected_nft) + self.assertEqual(ret[1].keys(), {'ripe.net'}) + self.assertIsInstance(ret[1]['ripe.net'], set) + self.assertIsNotNone(re.match('^[0-9a-f:]+/\d+$', + ret[1]['ripe.net'].pop())) def test_004_apply_rules4(self): rules = [{'action': 'accept'}] @@ -494,7 +514,7 @@ class TestNftablesWorker(TestCase): self.obj.apply_rules('10.137.0.1', rules) self.assertEqual(self.obj.loaded_rules, [self.expected_create_chain('ip', '10.137.0.1', chain), - self.obj.prepare_rules(chain, rules, 4), + self.obj.prepare_rules(chain, rules, 4)[0], ]) def test_005_apply_rules6(self): @@ -503,7 +523,7 @@ class TestNftablesWorker(TestCase): self.obj.apply_rules('2000::a', rules) self.assertEqual(self.obj.loaded_rules, [self.expected_create_chain('ip6', '2000::a', chain), - self.obj.prepare_rules(chain, rules, 6), + self.obj.prepare_rules(chain, rules, 6)[0], ]) def test_006_init(self): From 0993115bdc02bd294fa94f850d2ac178ee7f7c90 Mon Sep 17 00:00:00 2001 From: 3hhh Date: Sun, 16 May 2021 07:32:10 +0200 Subject: [PATCH 06/11] add some checks for QubesDB /qubes-firewall_handled/[ip] --- qubesagent/test_firewall.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/qubesagent/test_firewall.py b/qubesagent/test_firewall.py index a721847..9665045 100644 --- a/qubesagent/test_firewall.py +++ b/qubesagent/test_firewall.py @@ -675,11 +675,17 @@ class TestFirewallWorker(TestCase): def test_handle_addr(self): self.obj.handle_addr('10.137.0.2') self.assertEqual(self.obj.rules['10.137.0.2'], [{'action': 'accept'}]) + self.assertEqual(self.obj.qdb.entries['/qubes-firewall_handled/10.137.0.2'], '1') + self.obj.handle_addr('10.137.0.2') + self.assertEqual(self.obj.rules['10.137.0.2'], [{'action': 'accept'}]) + self.assertEqual(self.obj.qdb.entries['/qubes-firewall_handled/10.137.0.2'], '2') # fallback to block all self.obj.handle_addr('10.137.0.3') self.assertEqual(self.obj.rules['10.137.0.3'], [{'action': 'drop'}]) + self.assertEqual(self.obj.qdb.entries['/qubes-firewall_handled/10.137.0.3'], '1') self.obj.handle_addr('10.137.0.4') self.assertEqual(self.obj.rules['10.137.0.4'], [{'action': 'drop'}]) + self.assertEqual(self.obj.qdb.entries['/qubes-firewall_handled/10.137.0.4'], '1') @patch('os.path.isfile') @patch('os.access') From 3230f471b0c986ffbbea06398a39807f2986913f Mon Sep 17 00:00:00 2001 From: 3hhh Date: Sun, 16 May 2021 07:32:57 +0200 Subject: [PATCH 07/11] tests/firewall: some code refactoring --- qubesagent/test_firewall.py | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/qubesagent/test_firewall.py b/qubesagent/test_firewall.py index 9665045..504ca7c 100644 --- a/qubesagent/test_firewall.py +++ b/qubesagent/test_firewall.py @@ -166,8 +166,20 @@ class NftablesWorker(qubesagent.firewall.NftablesWorker): else: return ['2001::1', '2001::2'] +class WorkerTestCase(TestCase): + def assertPrepareRulesDnsRet(self, dns_ret, expected_domain, family): + self.assertEqual(dns_ret.keys(), {expected_domain}) + self.assertIsInstance(dns_ret[expected_domain], set) + if family == 4: + self.assertIsNotNone(re.match('^\d+\.\d+\.\d+\.\d+/32$', + dns_ret[expected_domain].pop())) + elif family == 6: + self.assertIsNotNone(re.match('^[0-9a-f:]+/\d+$', + dns_ret[expected_domain].pop())) + else: + raise ValueError() -class TestIptablesWorker(TestCase): +class TestIptablesWorker(WorkerTestCase): def setUp(self): super(TestIptablesWorker, self).setUp() self.obj = IptablesWorker() @@ -226,10 +238,7 @@ class TestIptablesWorker(TestCase): ) ret = self.obj.prepare_rules('chain', rules, 4) self.assertEqual(ret[0], expected_iptables) - self.assertEqual(ret[1].keys(), {'yum.qubes-os.org'}) - self.assertIsInstance(ret[1]['yum.qubes-os.org'], set) - self.assertIsNotNone(re.match('^\d+\.\d+\.\d+\.\d+/32$', - ret[1]['yum.qubes-os.org'].pop())) + self.assertPrepareRulesDnsRet(ret[1], 'yum.qubes-os.org', 4) with self.assertRaises(qubesagent.firewall.RuleParseError): self.obj.prepare_rules('chain', [{'unknown': 'xxx'}], 4) with self.assertRaises(qubesagent.firewall.RuleParseError): @@ -268,10 +277,7 @@ class TestIptablesWorker(TestCase): ) ret = self.obj.prepare_rules('chain', rules, 6) self.assertEqual(ret[0], expected_iptables) - self.assertEqual(ret[1].keys(), {'ripe.net'}) - self.assertIsInstance(ret[1]['ripe.net'], set) - self.assertIsNotNone(re.match('^[0-9a-f:]+/\d+$', - ret[1]['ripe.net'].pop())) + self.assertPrepareRulesDnsRet(ret[1], 'ripe.net', 6) def test_004_apply_rules4(self): rules = [{'action': 'accept'}] @@ -393,7 +399,7 @@ class TestIptablesWorker(TestCase): ]) -class TestNftablesWorker(TestCase): +class TestNftablesWorker(WorkerTestCase): def setUp(self): super(TestNftablesWorker, self).setUp() self.obj = NftablesWorker() @@ -462,10 +468,7 @@ class TestNftablesWorker(TestCase): ) ret = self.obj.prepare_rules('chain', rules, 4) self.assertEqual(ret[0], expected_nft) - self.assertEqual(ret[1].keys(), {'yum.qubes-os.org'}) - self.assertIsInstance(ret[1]['yum.qubes-os.org'], set) - self.assertIsNotNone(re.match('^\d+\.\d+\.\d+\.\d+/32$', - ret[1]['yum.qubes-os.org'].pop())) + self.assertPrepareRulesDnsRet(ret[1], 'yum.qubes-os.org', 4) with self.assertRaises(qubesagent.firewall.RuleParseError): self.obj.prepare_rules('chain', [{'unknown': 'xxx'}], 4) with self.assertRaises(qubesagent.firewall.RuleParseError): @@ -503,10 +506,7 @@ class TestNftablesWorker(TestCase): ) ret = self.obj.prepare_rules('chain', rules, 6) self.assertEqual(ret[0], expected_nft) - self.assertEqual(ret[1].keys(), {'ripe.net'}) - self.assertIsInstance(ret[1]['ripe.net'], set) - self.assertIsNotNone(re.match('^[0-9a-f:]+/\d+$', - ret[1]['ripe.net'].pop())) + self.assertPrepareRulesDnsRet(ret[1], 'ripe.net', 6) def test_004_apply_rules4(self): rules = [{'action': 'accept'}] From adfe982bfd3f77ffb008b188c367fdf53fab7117 Mon Sep 17 00:00:00 2001 From: 3hhh Date: Sun, 16 May 2021 08:09:19 +0200 Subject: [PATCH 08/11] tests/firewall: added test for /dns/[ip]/[domain] info --- qubesagent/test_firewall.py | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/qubesagent/test_firewall.py b/qubesagent/test_firewall.py index 504ca7c..b87f605 100644 --- a/qubesagent/test_firewall.py +++ b/qubesagent/test_firewall.py @@ -32,7 +32,7 @@ class DummyQubesDB(object): def rm(self, path): if path.endswith('/'): - for key in self.entries: + for key in list(self.entries): if key.startswith(path): self.entries.pop(key) else: @@ -166,7 +166,7 @@ class NftablesWorker(qubesagent.firewall.NftablesWorker): else: return ['2001::1', '2001::2'] -class WorkerTestCase(TestCase): +class WorkerCommon(object): def assertPrepareRulesDnsRet(self, dns_ret, expected_domain, family): self.assertEqual(dns_ret.keys(), {expected_domain}) self.assertIsInstance(dns_ret[expected_domain], set) @@ -179,7 +179,18 @@ class WorkerTestCase(TestCase): else: raise ValueError() -class TestIptablesWorker(WorkerTestCase): + def test_701_dns_info(self): + rules = [ + {'action': 'accept', 'proto': 'tcp', + 'dstports': '80-80', 'dsthost': 'ripe.net'}, + {'action': 'drop'}, + ] + self.obj.apply_rules('10.137.0.1', rules) + self.assertIsNotNone(self.obj.qdb.read('/dns/10.137.0.1/ripe.net')) + self.obj.apply_rules('10.137.0.1', [{'action': 'drop'}]) + self.assertIsNone(self.obj.qdb.read('/dns/10.137.0.1/ripe.net')) + +class TestIptablesWorker(TestCase, WorkerCommon): def setUp(self): super(TestIptablesWorker, self).setUp() self.obj = IptablesWorker() @@ -398,8 +409,7 @@ class TestIptablesWorker(WorkerTestCase): ['-t', 'mangle', '-F', 'QBS-POSTROUTING'], ]) - -class TestNftablesWorker(WorkerTestCase): +class TestNftablesWorker(TestCase, WorkerCommon): def setUp(self): super(TestNftablesWorker, self).setUp() self.obj = NftablesWorker() From 795bec8038c948fd0179452ecb70f7eccea472a7 Mon Sep 17 00:00:00 2001 From: 3hhh Date: Sun, 16 May 2021 08:27:45 +0200 Subject: [PATCH 09/11] firewall: start watches before initial load This should avoid a race condition where we miss an update to QubesDB that happens right after the initial load, but before the watch start. Instead, we might now install the same stuff twice - but that's no problem. --- qubesagent/firewall.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/qubesagent/firewall.py b/qubesagent/firewall.py index a9aa998..7a5ffc0 100755 --- a/qubesagent/firewall.py +++ b/qubesagent/firewall.py @@ -211,14 +211,14 @@ class FirewallWorker(object): self.run_firewall_dir() self.run_user_script() self.sd_notify('READY=1') + self.qdb.watch('/qubes-firewall/') + self.qdb.watch('/connected-ips') + self.qdb.watch('/connected-ips6') # 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': From 50e448e23a813e69ef9eae9878e492a7b1053c64 Mon Sep 17 00:00:00 2001 From: 3hhh Date: Tue, 18 May 2021 17:17:28 +0200 Subject: [PATCH 10/11] firewall: put DNS resolving into its own function --- qubesagent/firewall.py | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/qubesagent/firewall.py b/qubesagent/firewall.py index 7a5ffc0..1b9c331 100755 --- a/qubesagent/firewall.py +++ b/qubesagent/firewall.py @@ -127,6 +127,26 @@ class FirewallWorker(object): rules.append({'action': policy}) return rules + def resolve_dns(self, fqdn, family): + """ + Resolve the given FQDN via DNS. + :param fqdn: FQDN + :param family: 4 or 6 for IPv4 or IPv6 + :return: see socket.getaddrinfo() + :raises: RuleParseError + """ + try: + addrinfo = socket.getaddrinfo(fqdn, None, + (socket.AF_INET6 if family == 6 else socket.AF_INET)) + except socket.gaierror as e: + raise RuleParseError('Failed to resolve {}: {}'.format( + fqdn, str(e))) + except UnicodeError as e: + raise RuleParseError('Invalid destination {}: {}'.format( + fqdn, str(e))) + return addrinfo + + def update_dns_info(self, source, dns): """ Write resolved DNS addresses back to QubesDB. This can be useful @@ -339,12 +359,7 @@ class IptablesWorker(FirewallWorker): elif 'dst6' in rule: dsthosts = [rule['dst6']] elif 'dsthost' in rule: - try: - addrinfo = socket.getaddrinfo(rule['dsthost'], None, - (socket.AF_INET6 if family == 6 else socket.AF_INET)) - except socket.gaierror as e: - raise RuleParseError('Failed to resolve {}: {}'.format( - rule['dsthost'], str(e))) + addrinfo = self.resolve_dns(rule['dsthost'], family) dsthosts = set(item[4][0] + fullmask for item in addrinfo) ret_dns[rule['dsthost']] = dsthosts else: @@ -652,15 +667,7 @@ class NftablesWorker(FirewallWorker): elif 'dst6' in rule: nft_rule += ' ip6 daddr {}'.format(rule['dst6']) elif 'dsthost' in rule: - try: - addrinfo = socket.getaddrinfo(rule['dsthost'], None, - (socket.AF_INET6 if family == 6 else socket.AF_INET)) - except socket.gaierror as e: - raise RuleParseError('Failed to resolve {}: {}'.format( - rule['dsthost'], str(e))) - except UnicodeError as e: - raise RuleParseError('Invalid destination {}: {}'.format( - rule['dsthost'], str(e))) + addrinfo = self.resolve_dns(rule['dsthost'], family) dsthosts = set(item[4][0] + fullmask for item in addrinfo) nft_rule += ' {} daddr {{ {} }}'.format(ip_match, ', '.join(dsthosts)) From 1cbbcd7b80c4bb4265e5666377f0c337f7d8103f Mon Sep 17 00:00:00 2001 From: 3hhh Date: Mon, 24 May 2021 09:59:39 +0200 Subject: [PATCH 11/11] firewall: prefer - over _ for QubesDB path --- qubesagent/firewall.py | 4 ++-- qubesagent/test_firewall.py | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/qubesagent/firewall.py b/qubesagent/firewall.py index 1b9c331..8b3335f 100755 --- a/qubesagent/firewall.py +++ b/qubesagent/firewall.py @@ -169,12 +169,12 @@ class FirewallWorker(object): User applications may watch these paths for count increases to remain up to date with QubesDB changes. """ - cnt = self.qdb.read('/qubes-firewall_handled/{}'.format(addr)) + cnt = self.qdb.read('/qubes-firewall-handled/{}'.format(addr)) try: cnt = int(cnt) except (TypeError, ValueError): cnt = 0 - self.qdb.write('/qubes-firewall_handled/{}'.format(addr), str(cnt+1)) + self.qdb.write('/qubes-firewall-handled/{}'.format(addr), str(cnt+1)) def list_targets(self): return set(t.split('/')[2] for t in self.qdb.list('/qubes-firewall/')) diff --git a/qubesagent/test_firewall.py b/qubesagent/test_firewall.py index b87f605..f3fe8ff 100644 --- a/qubesagent/test_firewall.py +++ b/qubesagent/test_firewall.py @@ -685,17 +685,17 @@ class TestFirewallWorker(TestCase): def test_handle_addr(self): self.obj.handle_addr('10.137.0.2') self.assertEqual(self.obj.rules['10.137.0.2'], [{'action': 'accept'}]) - self.assertEqual(self.obj.qdb.entries['/qubes-firewall_handled/10.137.0.2'], '1') + self.assertEqual(self.obj.qdb.entries['/qubes-firewall-handled/10.137.0.2'], '1') self.obj.handle_addr('10.137.0.2') self.assertEqual(self.obj.rules['10.137.0.2'], [{'action': 'accept'}]) - self.assertEqual(self.obj.qdb.entries['/qubes-firewall_handled/10.137.0.2'], '2') + self.assertEqual(self.obj.qdb.entries['/qubes-firewall-handled/10.137.0.2'], '2') # fallback to block all self.obj.handle_addr('10.137.0.3') self.assertEqual(self.obj.rules['10.137.0.3'], [{'action': 'drop'}]) - self.assertEqual(self.obj.qdb.entries['/qubes-firewall_handled/10.137.0.3'], '1') + self.assertEqual(self.obj.qdb.entries['/qubes-firewall-handled/10.137.0.3'], '1') self.obj.handle_addr('10.137.0.4') self.assertEqual(self.obj.rules['10.137.0.4'], [{'action': 'drop'}]) - self.assertEqual(self.obj.qdb.entries['/qubes-firewall_handled/10.137.0.4'], '1') + self.assertEqual(self.obj.qdb.entries['/qubes-firewall-handled/10.137.0.4'], '1') @patch('os.path.isfile') @patch('os.access')