[PATCH pve-firewall 0/2] ebtables: fix ARP filter for alias and nomatch ipfilter entries

[PATCH pve-firewall 0/2] ebtables: fix ARP filter for alias and nomatch ipfilter entries

[Arthur Bied-Charreton]

Following this forum post [0], I looked into what might be generating
the 

    value does not look like a valid IP address or CIDR network

log lines, which show up once per affected guest on every update cycle.

When building the ARP filter from an ipfilter-netX ipset, entries were 
passed verbatim to parse_ip_or_cidr(), so an alias entry, or one the 
parser already flagged as invalid, makes it die. The error aborts the 
whole guest in compile_ebtables_filter(), so the guest ends up with no 
ebtables rules at all. 

Before bbd02dae ("firewall: log compilation errors in syslog"), the 
error went to warn(), which the daemon ignores, so the layer-2 filter
was missing silently.

Patch 1 resolves entries the same way generate_ipset_chains() does, 
skips entries the parser errored on, and logs errors per entry with 
more context to make debugging easier.

While looking into this, I also noticed that nomatch entries were added
to the ARP filter as allowed sources. Patch 2 emits DROP for them to 
honor nomatch.

[0] https://forum.proxmox.com/threads/pve-firewall-log-value-does-not-look-like-a-valid-ip-address-or-cidr-network.186990/#post-872700

Arthur Bied-Charreton (2):
  firewall: ebtables: resolve aliases in ipfilter ipsets
  firewall: ebtables: honor nomatch in ipfilter ipsets

 src/PVE/Firewall.pm                |  86 +++++++++++++-------
 src/test/Makefile                  |   2 +-
 src/test/test_ebtables_ipfilter.pl | 123 +++++++++++++++++++++++++++++
 3 files changed, 183 insertions(+), 28 deletions(-)
 create mode 100755 src/test/test_ebtables_ipfilter.pl

-- 
2.47.3

[PATCH pve-firewall 1/2] firewall: ebtables: resolve aliases in ipfilter ipsets

[Arthur Bied-Charreton]

Entries of an ipfilter-netX ipset were passed straight to
parse_ip_or_cidr() when building the ARP filter, so an alias entry or
one the parser already flagged as invalid, made it die.

The error dropped all ebtables rules for the guest, including the MAC and
ARP source filters, and was logged on every update cycle without saying
which guest was affected.

Resolve entries the same way generate_ipset_chains() does, skip
erroneous ones, and log resolution errors per entry with context.

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 src/PVE/Firewall.pm                | 70 +++++++++++++++---------
 src/test/Makefile                  |  2 +-
 src/test/test_ebtables_ipfilter.pl | 85 ++++++++++++++++++++++++++++++
 3 files changed, 132 insertions(+), 25 deletions(-)
 create mode 100755 src/test/test_ebtables_ipfilter.pl

diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index da6976e..bb3350b 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -4074,6 +4074,16 @@ sub generate_std_chains {
     }
 }
 
+my sub resolve_ipset_entry {
+    my ($cluster_conf, $fw_conf, $cidr) = @_;
+
+    if ($cidr =~ m@^(dc/|guest/)?(${ip_alias_pattern})$@) {
+        return resolve_alias($cluster_conf, $fw_conf, $2, $1 // "");
+    }
+
+    return parse_ip_or_cidr($cidr);
+}
+
 sub generate_ipset_chains {
     my ($ipset_ruleset, $clusterfw_conf, $fw_conf, $device_ips, $ipsets) = @_;
 
@@ -4092,14 +4102,8 @@ sub generate_ipset_chains {
         foreach my $entry (@$options) {
             next if $entry->{errors}; # skip entries with errors
             eval {
-                my ($cidr, $ver);
-                if ($entry->{cidr} =~ m@^(dc/|guest/)?(${ip_alias_pattern})$@) {
-                    my $scope = $1 // "";
-                    my $alias = $2;
-                    ($cidr, $ver) = resolve_alias($clusterfw_conf, $fw_conf, $alias, $scope);
-                } else {
-                    ($cidr, $ver) = parse_ip_or_cidr($entry->{cidr});
-                }
+                my ($cidr, $ver) =
+                    resolve_ipset_entry($clusterfw_conf, $fw_conf, $entry->{cidr});
                 #http://backreference.org/2013/03/01/ipv6-address-normalization/
                 if ($ver == 6) {
                     # ip_compress_address takes an address only, no CIDR
@@ -4810,6 +4814,32 @@ sub compile_ipsets {
     return $ipset_ruleset;
 }
 
+my sub resolve_arpfilter {
+    my ($ipset, $cluster_conf, $vm_conf, $vmid, $netid) = @_;
+    return [] if !defined($ipset);
+
+    my $arpfilter = [];
+    for my $entry ($ipset->@*) {
+        next if $entry->{errors};
+
+        my ($ip, $ver) = eval { resolve_ipset_entry($cluster_conf, $vm_conf, $entry->{cidr}) };
+        if ($@) {
+            my $err = "$vmid: ipfilter-$netid: skipping '$entry->{cidr}': $@";
+            if ($verbose) {
+                warn $err;
+            } else {
+                syslog('warn', $err);
+            }
+            next;
+        }
+
+        next if $ver != 4;
+        push $arpfilter->@*, $ip;
+    }
+
+    return $arpfilter;
+}
+
 sub compile_ebtables_filter {
     my ($cluster_conf, $hostfw_conf, $vmfw_configs, $vmdata) = @_;
 
@@ -4842,14 +4872,10 @@ sub compile_ebtables_filter {
                 my $iface = "tap${vmid}i$1";
                 next if !valid_interface_name($iface);
                 my $macaddr = $net->{macaddr};
-                my $arpfilter = [];
-                if (defined(my $ipset = $ipsets->{"ipfilter-$netid"})) {
-                    foreach my $ipaddr (@$ipset) {
-                        my ($ip, $version) = parse_ip_or_cidr($ipaddr->{cidr});
-                        next if !$ip || ($version && $version != 4);
-                        push(@$arpfilter, $ip);
-                    }
-                }
+                my $arpfilter = resolve_arpfilter(
+                    $ipsets->{"ipfilter-$netid"},
+                    $cluster_conf, $vmfw_conf, $vmid, $netid,
+                );
                 generate_tap_layer2filter(
                     $ruleset, $iface, $macaddr, $vmfw_conf, $vmid, $arpfilter,
                 );
@@ -4881,14 +4907,10 @@ sub compile_ebtables_filter {
                 my $iface = "veth${vmid}i$1";
                 next if !valid_interface_name($iface);
                 my $macaddr = $net->{hwaddr};
-                my $arpfilter = [];
-                if (defined(my $ipset = $ipsets->{"ipfilter-$netid"})) {
-                    foreach my $ipaddr (@$ipset) {
-                        my ($ip, $version) = parse_ip_or_cidr($ipaddr->{cidr});
-                        next if !$ip || ($version && $version != 4);
-                        push(@$arpfilter, $ip);
-                    }
-                }
+                my $arpfilter = resolve_arpfilter(
+                    $ipsets->{"ipfilter-$netid"},
+                    $cluster_conf, $vmfw_conf, $vmid, $netid,
+                );
                 if (defined(my $ip = $net->{ip}) && $vmfw_conf->{options}->{ipfilter}) {
                     # ebtables changes this to a .0/MASK network but we just
                     # want the address here, no network - see #2193
diff --git a/src/test/Makefile b/src/test/Makefile
index 272a19a..74f9e35 100644
--- a/src/test/Makefile
+++ b/src/test/Makefile
@@ -1,4 +1,4 @@
-TESTS = test_parse_address_list test_ipt_rule_to_cmds test_parameter_newlines test_compile_ipsets test_simulator_protocols test_interface_names test_restore_errors
+TESTS = test_parse_address_list test_ipt_rule_to_cmds test_parameter_newlines test_compile_ipsets test_ebtables_ipfilter test_simulator_protocols test_interface_names test_restore_errors
 
 .PHONY: all test $(TESTS) clean
 all: test
diff --git a/src/test/test_ebtables_ipfilter.pl b/src/test/test_ebtables_ipfilter.pl
new file mode 100755
index 0000000..9716ac2
--- /dev/null
+++ b/src/test/test_ebtables_ipfilter.pl
@@ -0,0 +1,85 @@
+#!/usr/bin/env perl
+
+use strict;
+use warnings;
+
+use lib qw(..);
+
+use Test::More;
+
+use PVE::Firewall;
+
+plan(skip_all => 'PVE::QemuServer::Network is not installed')
+    if !eval { require PVE::QemuServer::Network; 1 };
+
+PVE::Firewall::set_verbose(1);
+
+sub compile_arp_sources {
+    my ($entries) = @_;
+
+    my $cluster = {
+        options => {},
+        aliases => { dcalias => { name => 'dcalias', cidr => '192.0.2.2', ipversion => 4 } },
+    };
+    my $vmfw_conf = {
+        vmid => 100,
+        options => { enable => 1 },
+        aliases =>
+            { guestalias => { name => 'guestalias', cidr => '192.0.2.1', ipversion => 4 } },
+        ipset => { 'ipfilter-net0' => $entries },
+    };
+    my $vmdata = {
+        qemu => { 100 => { net0 => 'virtio=02:00:00:00:00:01,bridge=vmbr0,firewall=1' } },
+        lxc => {},
+    };
+
+    my @warnings;
+    local $SIG{__WARN__} = sub { push @warnings, @_ };
+
+    my $ruleset =
+        PVE::Firewall::compile_ebtables_filter($cluster, {}, { 100 => $vmfw_conf }, $vmdata);
+
+    ok(exists($ruleset->{'tap100i0-OUT'}), 'guest keeps its layer-2 filter');
+    my @sources = map { /--arp-ip-src (\S+)/ ? $1 : () } ($ruleset->{'tap100i0-OUT-ARP'} // [])->@*;
+
+    return (\@sources, \@warnings);
+}
+
+subtest 'aliases are resolved' => sub {
+    my ($sources, $warnings) = compile_arp_sources([
+        { cidr => '192.0.2.10' },
+        { cidr => 'guestalias' },
+        { cidr => 'guest/guestalias' },
+        { cidr => 'dc/dcalias' },
+        { cidr => 'dcalias' },
+    ]);
+    is_deeply(
+        $sources,
+        ['192.0.2.10', '192.0.2.1', '192.0.2.1', '192.0.2.2', '192.0.2.2'],
+        'ARP filter contains resolved addresses',
+    );
+    is_deeply($warnings, [], 'no warnings');
+};
+
+subtest 'entries flagged by the parser are skipped' => sub {
+    my ($sources, $warnings) = compile_arp_sources([
+        { cidr => 'c', errors => { cidr => 'invalid' } }, { cidr => '192.0.2.10' },
+    ]);
+    is_deeply($sources, ['192.0.2.10'], 'valid entry is kept');
+    is_deeply($warnings, [], 'no warnings');
+};
+
+subtest 'unresolvable entries are skipped with context' => sub {
+    my ($sources, $warnings) = compile_arp_sources([
+        { cidr => 'missing' }, { cidr => '192.0.2.10' },
+    ]);
+    is_deeply($sources, ['192.0.2.10'], 'valid entry is kept');
+    is(scalar($warnings->@*), 1, 'one warning');
+    like(
+        $warnings->[0] // '',
+        qr/^100: ipfilter-net0: skipping 'missing': /,
+        'warning names guest, ipset and entry',
+    );
+};
+
+done_testing();
-- 
2.47.3

[PATCH pve-firewall 2/2] firewall: ebtables: honor nomatch in ipfilter ipsets

[Arthur Bied-Charreton]

The ARP filter turnes every ipfilter-netX entry into a RETURN rule,
including nomatch entries. An address excluded from an allowed network
with "!" is therefore still accepted as an ARP source.

Emit DROP for nomatch entries and order the rules by prefix length. The
rules are sorted most-specific first, and equal prefix lengths keep
their original order.

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 src/PVE/Firewall.pm                | 18 ++++++++++---
 src/test/test_ebtables_ipfilter.pl | 42 ++++++++++++++++++++++++++++--
 2 files changed, 54 insertions(+), 6 deletions(-)

diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index bb3350b..9630d58 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -4834,7 +4834,7 @@ my sub resolve_arpfilter {
         }
 
         next if $ver != 4;
-        push $arpfilter->@*, $ip;
+        push $arpfilter->@*, { cidr => $ip, nomatch => $entry->{nomatch} };
     }
 
     return $arpfilter;
@@ -4916,7 +4916,7 @@ sub compile_ebtables_filter {
                     # want the address here, no network - see #2193
                     $ip =~ s|/(\d+)$||;
                     if ($ip ne 'dhcp') {
-                        push @$arpfilter, $ip;
+                        push $arpfilter->@*, { cidr => $ip };
                     }
                 }
                 generate_tap_layer2filter(
@@ -4958,8 +4958,18 @@ sub generate_tap_layer2filter {
         ruleset_addrule($ruleset, $tapchain, "-p ARP", "-j $arpchain");
         ruleset_create_chain($ruleset, $arpchain);
 
-        foreach my $ip (@{$arpfilter}) {
-            ruleset_addrule($ruleset, $arpchain, "-p ARP --arp-ip-src $ip", '-j RETURN');
+        my $prefix_len = sub { $arpfilter->[$_[0]]->{cidr} =~ m|/(\d+)$| ? $1 : 32 };
+        my @order =
+            sort { $prefix_len->($b) <=> $prefix_len->($a) || $a <=> $b } 0 .. $arpfilter->$#*;
+
+        for my $entry ($arpfilter->@[@order]) {
+            my $target = $entry->{nomatch} ? 'DROP' : 'RETURN';
+            ruleset_addrule(
+                $ruleset,
+                $arpchain,
+                "-p ARP --arp-ip-src $entry->{cidr}",
+                "-j $target",
+            );
         }
         ruleset_addrule($ruleset, $arpchain, '', '-j DROP');
     }
diff --git a/src/test/test_ebtables_ipfilter.pl b/src/test/test_ebtables_ipfilter.pl
index 9716ac2..069ee6c 100755
--- a/src/test/test_ebtables_ipfilter.pl
+++ b/src/test/test_ebtables_ipfilter.pl
@@ -40,9 +40,11 @@ sub compile_arp_sources {
         PVE::Firewall::compile_ebtables_filter($cluster, {}, { 100 => $vmfw_conf }, $vmdata);
 
     ok(exists($ruleset->{'tap100i0-OUT'}), 'guest keeps its layer-2 filter');
-    my @sources = map { /--arp-ip-src (\S+)/ ? $1 : () } ($ruleset->{'tap100i0-OUT-ARP'} // [])->@*;
+    my $arp_rules = $ruleset->{'tap100i0-OUT-ARP'} // [];
+    my @sources = map { /--arp-ip-src (\S+) -j RETURN/ ? $1 : () } $arp_rules->@*;
+    my @rules = map { /--arp-ip-src (\S+) -j (\S+)/ ? "$2 $1" : () } $arp_rules->@*;
 
-    return (\@sources, \@warnings);
+    return (\@sources, \@warnings, \@rules);
 }
 
 subtest 'aliases are resolved' => sub {
@@ -82,4 +84,40 @@ subtest 'unresolvable entries are skipped with context' => sub {
     );
 };
 
+subtest 'nomatch entries are excluded' => sub {
+    my (undef, $warnings, $rules) = compile_arp_sources([
+        { cidr => '192.0.2.0/24' }, { cidr => '192.0.2.5', nomatch => 1 },
+    ]);
+    is_deeply(
+        $rules,
+        ['DROP 192.0.2.5', 'RETURN 192.0.2.0/24'],
+        'excluded address is dropped before the allowed network',
+    );
+    is_deeply($warnings, [], 'no warnings');
+};
+
+subtest 'more specific entries take precedence over nomatch networks' => sub {
+    my (undef, $warnings, $rules) = compile_arp_sources([
+        { cidr => '192.0.2.0/24', nomatch => 1 }, { cidr => '192.0.2.5' },
+    ]);
+    is_deeply(
+        $rules,
+        ['RETURN 192.0.2.5', 'DROP 192.0.2.0/24'],
+        'allowed address is returned before the excluded network',
+    );
+    is_deeply($warnings, [], 'no warnings');
+};
+
+subtest 'entries with the same prefix length keep their order' => sub {
+    my (undef, $warnings, $rules) = compile_arp_sources([
+        { cidr => '192.0.2.20' }, { cidr => '192.0.2.0/24' }, { cidr => '192.0.2.3' },
+    ]);
+    is_deeply(
+        $rules,
+        ['RETURN 192.0.2.20', 'RETURN 192.0.2.3', 'RETURN 192.0.2.0/24'],
+        'ties are not reordered',
+    );
+    is_deeply($warnings, [], 'no warnings');
+};
+
 done_testing();
-- 
2.47.3