[pve-devel] [PATCH pve-firewall 1/3] global -m conntrack --ctstate INVALID : PVEFW-reject instead DROP

[Alexandre DERUMIER]

>>when we stop yes, but not when we restart, which is registered to use HUP (reload) signal.
>>And in our postinst script we explicitly do a try-reload-or-restart in the case of a running daemon.

Yes, but this is also trigger with other race bug, if /etc/pve is unavaible for example.(pmxcfs/ pve-cluster restart).
or when I directly edited the cluster.fw 
This is fast flushing rules, then readd them, and put all traffic as invalid.



>>the second point should be already the case for package upgrades, so either 
>>you run into the issue with pve-cluster restarting too (thus no config to read 
>>from) or I oversaw something... 

yes indeed.

But I'm more thinking about user which disable firewall(miss-click, test,or urgency/panic  and want to reenable it later,
and don't want to break connections or nat.

I really think we need some kind of protection to avoid that. 


----- Mail original -----
De: "Thomas Lamprecht" <t.lamprecht at proxmox.com>
À: "pve-devel" <pve-devel at pve.proxmox.com>, "Alexandre Derumier" <aderumier at odiso.com>, "Wolfgang Bumiller" <w.bumiller at proxmox.com>
Envoyé: Mardi 19 Février 2019 11:07:42
Objet: Re: [pve-devel] [PATCH pve-firewall 1/3] global -m conntrack --ctstate INVALID : PVEFW-reject instead DROP

Hi, 

On 2/19/19 1:54 AM, Alexandre DERUMIER wrote: 
> Hi again, 
> 
> I think I have found the problem with stop/start of firewall and conntrack 
> 
> It's possible to mitigated it with 
> 
> echo 1 > /proc/sys/net/netfilter/nf_conntrack_tcp_be_liberal 
> 
> 
> from conntrack source 
> https://github.com/torvalds/linux/blob/master/net/netfilter/nf_conntrack_proto_tcp.c 
> 
> res = false; 
> if (sender->flags & IP_CT_TCP_FLAG_BE_LIBERAL || 
> tn->tcp_be_liberal) 
> res = true; 
> if (!res) { 
> nf_ct_l4proto_log_invalid(skb, ct, 
> "%s", 
> before(seq, sender->td_maxend + 1) ? 
> in_recv_win ? 
> before(sack, receiver->td_end + 1) ? 
> after(sack, receiver->td_end - MAXACKWINDOW(sender) - 1) ? "BUG" 
> : "ACK is under the lower bound (possible overly delayed ACK)" 
> : "ACK is over the upper bound (ACKed data not seen yet)" 
> : "SEQ is under the lower bound (already ACKed data retransmitted)" 
> : "SEQ is over the upper bound (over the window of the receiver)"); 
> } 
> 
> 
> I think that when we stop pve-firewall, we remove the conntrack rules,0 
> then conntrack don't track anymore the packets sequences, 
> and when we reenable firewall, it's dropping packets. (The connection entry it's still present in the conntrack table). 

when we stop yes, but not when we restart, which is registered to use HUP (reload) signal. 
And in our postinst script we explicitly do a try-reload-or-restart in the case of a running daemon. 

I can reproduce the disappearing of rules with: 
# systemct restart pve-firewall.service 
but not with 
# systemct reload pve-firewall.service 
(or an package upgrade, which results in the same actions happening) 


> 
> 
> I think we have 3 possiblity: 
> 
> - flushing conntrack on pve-firewall stop (but maybe it'll break nat if users have defined them manually ?) 
> 
> - keep conntrack rules somewhere
 
> - use nf_conntrack_tcp_be_liberal as default (I'm using it some years ago with ciscofw randomizing sequences numbers) 
> 
> 
> What do you think ? 
> 

the second point should be already the case for package upgrades, so either 
you run into the issue with pve-cluster restarting too (thus no config to read 
from) or I oversaw something... 

> 
> 
> 
> 
> ----- Mail original ----- 
> De: "aderumier" <aderumier at odiso.com> 
> À: "Wolfgang Bumiller" <w.bumiller at proxmox.com> 
> Cc: "pve-devel" <pve-devel at pve.proxmox.com> 
> Envoyé: Lundi 18 Février 2019 11:53:26 
> Objet: Re: [pve-devel] [PATCH pve-firewall 1/3] global -m conntrack --ctstate INVALID : PVEFW-reject instead DROP 
> 
>>> Rejecting in forward chains is dangerous, we should never do that by 
>>> default unless we already know for sure the packet was meant to go down 
>>> this path, otherwise we end up rejecting connections for other guests if 
>>> the bridge hasn't learned or forgot the neighbor entry for the packet's 
>>> mac address. 
> 
> Is it more dangerous than DROP ? 
> the difference is to send an RST. (this could be bad for spoofed attack) 
> 
> In my case, it seem that flushing conntrack after pve-firewall stop, 
> fix the problem when pve-firewall start again. 
> 
> 
> Maybe could we make it optionnal ? 
> I'm seeing that we have a new option "nf_conntrack_allow_invalid" to make ACCEPT. 
> 
> maybe could we rename is "nf_conntrack_invalid_action" : DROP(default)/REJECT/ACCEPT ? 
> 
> 
> 
> ----- Mail original ----- 
> De: "Wolfgang Bumiller" <w.bumiller at proxmox.com> 
> À: "aderumier" <aderumier at odiso.com> 
> Cc: "pve-devel" <pve-devel at pve.proxmox.com> 
> Envoyé: Lundi 18 Février 2019 10:56:42 
> Objet: Re: [pve-devel] [PATCH pve-firewall 1/3] global -m conntrack --ctstate INVALID : PVEFW-reject instead DROP 
> 
> On Fri, Feb 15, 2019 at 10:48:01AM +0100, Alexandre Derumier wrote: 
>> --- 
>> src/PVE/Firewall.pm | 2 +- 
>> 1 file changed, 1 insertion(+), 1 deletion(-) 
>> 
>> diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm 
>> index 2125d3b..61d5599 100644 
>> --- a/src/PVE/Firewall.pm 
>> +++ b/src/PVE/Firewall.pm 
>> @@ -2137,7 +2137,7 @@ sub ruleset_chain_add_conn_filters { 
>> my ($ruleset, $chain, $allow_invalid, $accept) = @_; 
>> 
>> if (!$allow_invalid) { 
>> - ruleset_addrule($ruleset, $chain, "-m conntrack --ctstate INVALID", "-j DROP"); 
>> + ruleset_addrule($ruleset, $chain, "-m conntrack --ctstate INVALID", "-j PVEFW-reject"); 
> 
> Rejecting in forward chains is dangerous, we should never do that by 
> default unless we already know for sure the packet was meant to go down 
> this path, otherwise we end up rejecting connections for other guests if 
> the bridge hasn't learned or forgot the neighbor entry for the packet's 
> mac address. 
> 
> _______________________________________________ 
> pve-devel mailing list 
> pve-devel at pve.proxmox.com 
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel 
> 
> _______________________________________________ 
> pve-devel mailing list 
> pve-devel at pve.proxmox.com 
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel 
>