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

[Arthur Bied-Charreton <a.bied-charreton@proxmox.com>]

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