[pve-devel] [PATCH pve-firewall] ebtables: add arp filtering

[Wolfgang Bumiller]

On Fri, Mar 08, 2019 at 09:14:08AM +0100, Alexandre Derumier wrote:
> This implemented arp filtering if ipfilter is enable
> https://bugzilla.proxmox.com/show_bug.cgi?id=2125
> 
> They are another filters possible (ipv4,rarp),
> i don't known if we need them.
> 
> Signed-off-by: Alexandre Derumier <aderumier at odiso.com>
> ---
>  src/PVE/Firewall.pm | 42 +++++++++++++++++++++++++++++++++---------
>  1 file changed, 33 insertions(+), 9 deletions(-)
> 
> diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
> index 2125d3b..f8ed345 100644
> --- a/src/PVE/Firewall.pm
> +++ b/src/PVE/Firewall.pm
> @@ -3694,9 +3694,16 @@ sub compile_ebtables_filter {
>  		next if !$net->{firewall};
>  		my $iface = "tap${vmid}i$1";
>  		my $macaddr = $net->{macaddr};
> -
> -		generate_tap_layer2filter($ruleset, $iface, $macaddr, $vmfw_conf, $vmid);
> -
> +		my $arpfilter = [];
> +		my $ipsets = $vmfw_conf->{ipset};
> +		if (defined($ipsets->{"ipfilter-$netid"})){
> +		    foreach my $ipaddr (@{$ipsets->{"ipfilter-$netid"} }) {

Please try to avoid duplicating hash accesses. Eg. use:

    if (defined(my $ipset = $ipsets->{"ipfilter-$netid"})){
        foreach my $ipaddr (@$ipset) {

> +			my($ip,$version) = parse_ip_or_cidr($ipaddr->{cidr});

Missing spaces (`my ($ip, $version) = `)

> +			next if !$ip || ($version && $version != 4);
> +			push(@$arpfilter, $ip);
> +		    }
> +		}
> +		generate_tap_layer2filter($ruleset, $iface, $macaddr, $vmfw_conf, $vmid, $arpfilter);
>  	    }
>  	};
>  	warn $@ if $@; # just to be sure - should not happen
> @@ -3716,7 +3723,9 @@ sub compile_ebtables_filter {
>  		next if !$net->{firewall};
>  		my $iface = "veth${vmid}i$1";
>  		my $macaddr = $net->{hwaddr};
> -		generate_tap_layer2filter($ruleset, $iface, $macaddr, $vmfw_conf, $vmid);
> +		my $arpfilter = [];
> +		push(@$arpfilter, $net->{ip}) if $net->{ip};

We do have ipfilter-net* ipsets for containers as well, so this would
need to be filled, too.

> +		generate_tap_layer2filter($ruleset, $iface, $macaddr, $vmfw_conf, $vmid, $arpfilter);
>  	    }
>  	};
>  	warn $@ if $@; # just to be sure - should not happen
> @@ -3726,7 +3735,7 @@ sub compile_ebtables_filter {
>  }
>  
>  sub generate_tap_layer2filter {
> -    my ($ruleset, $iface, $macaddr, $vmfw_conf, $vmid) = @_;
> +    my ($ruleset, $iface, $macaddr, $vmfw_conf, $vmid, $arpfilter) = @_;
>      my $options = $vmfw_conf->{options};
>  
>      my $tapchain = $iface."-OUT";
> @@ -3741,15 +3750,30 @@ sub generate_tap_layer2filter {
>  	    ruleset_addrule($ruleset, $tapchain, "-s ! $macaddr", '-j DROP');
>      }
>  
> +    if (defined($arpfilter)){

This is always defined because it starts out as an array ref (empty but
defined). You need to use `if (@$arpfilter) {` here.
(As with this currently it'll always jump to an empty -ARP chain
containing only the final `-j DROP` if no ipfilter-netX exists.)

> +	my $arpchain = $tapchain."-ARP";
> +	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');

I wish this could just use the actual ipset :-\

> +	}
> +	ruleset_addrule($ruleset, $arpchain, '', '-j DROP');
> +    }
> +
>      if (defined($options->{layer2_protocols})){

This hunk below seems unrelated? Should probably be a separate patch, or
is this related somehow?

> +	my $protochain = $tapchain."-PROTO";
> +	ruleset_addrule($ruleset, $tapchain, '', "-j $protochain");
> +	ruleset_create_chain($ruleset, $protochain);
> +
>  	foreach my $proto (split(/,/, $options->{layer2_protocols})) {
> -	    ruleset_addrule($ruleset, $tapchain, "-p $proto", '-j ACCEPT');
> +	    ruleset_addrule($ruleset, $protochain, "-p $proto", '-j RETURN');
>  	}
> -	ruleset_addrule($ruleset, $tapchain, '', "-j DROP");
> -    } else {
> -	ruleset_addrule($ruleset, $tapchain, '', '-j ACCEPT');
> +	ruleset_addrule($ruleset, $protochain, '', '-j DROP');
>      }
>  
> +    ruleset_addrule($ruleset, $tapchain, '', '-j ACCEPT');
> +
>      ruleset_addrule($ruleset, 'PVEFW-FWBR-OUT', "-i $iface", "-j $tapchain");
>  }
>  
> -- 
> 2.11.0