[pve-devel] [PATCH proxmox-firewall 1/2] fix: firewall: apply `nt_conntrack_allow_invalid` to all chains

[pve-devel] [PATCH proxmox-firewall 1/2] fix: firewall: apply `nt_conntrack_allow_invalid` to all chains

[Hannes Laimer]

... on the guest table. There is no reason to not repect that option
on those two chains. These two were missed in the referenced commit.

Fixes: 64dc344b ("firewall: apply `nt_conntrack_allow_invalid` option to guest table")
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 proxmox-firewall/resources/proxmox-firewall.nft | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/proxmox-firewall/resources/proxmox-firewall.nft b/proxmox-firewall/resources/proxmox-firewall.nft
index 2dd7c48..30f7b4f 100644
--- a/proxmox-firewall/resources/proxmox-firewall.nft
+++ b/proxmox-firewall/resources/proxmox-firewall.nft
@@ -356,7 +356,7 @@ table bridge proxmox-firewall-guests {
     }
 
     chain pre-vm-out {
-        meta protocol != arp ct state vmap { established : accept, related : accept, invalid : drop }
+        meta protocol != arp ct state vmap { established : accept, related : accept, invalid : jump invalid-conntrack }
     }
 
     chain vm-out {
@@ -384,7 +384,7 @@ table bridge proxmox-firewall-guests {
 
     chain before-bridge {
         meta protocol arp accept
-        meta protocol != arp ct state vmap { established : accept, related : accept, invalid : drop }
+        meta protocol != arp ct state vmap { established : accept, related : accept, invalid : jump invalid-conntrack }
     }
 
     chain forward {
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH proxmox-firewall 2/2] firewall: apply `nt_conntrack_allow_invalid` option to host table

[Hannes Laimer]

... on all chains that check for ct state. Since we support this option,
we should also use it in our firewall rule generation.

This is a follow-up to
  64dc344b ("firewall: apply `nt_conntrack_allow_invalid` option to guest table")

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 .../resources/proxmox-firewall.nft            | 15 +++++-------
 proxmox-firewall/src/firewall.rs              | 11 ++++++---
 .../integration_tests__firewall.snap          | 23 ++++++++++++-------
 3 files changed, 29 insertions(+), 20 deletions(-)

diff --git a/proxmox-firewall/resources/proxmox-firewall.nft b/proxmox-firewall/resources/proxmox-firewall.nft
index 30f7b4f..455d1c3 100644
--- a/proxmox-firewall/resources/proxmox-firewall.nft
+++ b/proxmox-firewall/resources/proxmox-firewall.nft
@@ -14,7 +14,6 @@ add chain inet proxmox-firewall allow-ndp-in
 add chain inet proxmox-firewall block-ndp-in
 add chain inet proxmox-firewall allow-ndp-out
 add chain inet proxmox-firewall block-ndp-out
-add chain inet proxmox-firewall block-conntrack-invalid
 add chain inet proxmox-firewall block-smurfs
 add chain inet proxmox-firewall allow-icmp
 add chain inet proxmox-firewall log-drop-smurfs
@@ -55,7 +54,6 @@ flush chain inet proxmox-firewall allow-ndp-in
 flush chain inet proxmox-firewall block-ndp-in
 flush chain inet proxmox-firewall allow-ndp-out
 flush chain inet proxmox-firewall block-ndp-out
-flush chain inet proxmox-firewall block-conntrack-invalid
 flush chain inet proxmox-firewall block-smurfs
 flush chain inet proxmox-firewall allow-icmp
 flush chain inet proxmox-firewall log-drop-smurfs
@@ -176,10 +174,6 @@ table inet proxmox-firewall {
         icmpv6 type { nd-router-solicit, nd-neighbor-solicit, nd-neighbor-advert } drop
     }
 
-    chain block-conntrack-invalid {
-        ct state invalid drop
-    }
-
     chain block-smurfs {
         ip saddr 0.0.0.0/32 return
         meta pkttype broadcast goto log-drop-smurfs
@@ -229,7 +223,7 @@ table inet proxmox-firewall {
         oifname "lo" accept
 
         jump allow-icmp
-        ct state vmap { invalid : drop, established : accept, related : accept }
+        ct state vmap { invalid : jump invalid-conntrack, established : accept, related : accept }
     }
 
     chain option-in {}
@@ -241,7 +235,7 @@ table inet proxmox-firewall {
 
     chain before-bridge {
         meta protocol arp accept
-        meta protocol != arp ct state vmap { established : accept, related : accept, invalid : drop }
+        meta protocol != arp ct state vmap { established : accept, related : accept, invalid : jump invalid-conntrack }
     }
 
     chain host-bridge-input {
@@ -284,9 +278,12 @@ table inet proxmox-firewall {
     chain host-out {}
 
     chain cluster-forward {}
-    chain host-forward {}
+    chain host-forward {
+        meta protocol != arp ct state vmap { established : accept, related : accept, invalid : jump invalid-conntrack }
+    }
 
     chain ct-in {}
+    chain invalid-conntrack { }
 }
 
 table bridge proxmox-firewall-guests {
diff --git a/proxmox-firewall/src/firewall.rs b/proxmox-firewall/src/firewall.rs
index 88fb460..607fc75 100644
--- a/proxmox-firewall/src/firewall.rs
+++ b/proxmox-firewall/src/firewall.rs
@@ -99,6 +99,10 @@ impl Firewall {
         ChainPart::new(Self::guest_table(), "invalid-conntrack".to_string())
     }
 
+    fn host_invalid_conntrack_chain() -> ChainPart {
+        ChainPart::new(Self::host_table(), "invalid-conntrack".to_string())
+    }
+
     fn host_conntrack_chain() -> ChainPart {
         ChainPart::new(Self::host_table(), "ct-in".to_string())
     }
@@ -144,6 +148,7 @@ impl Firewall {
             Flush::chain(Self::host_option_chain(Direction::Out)),
             Flush::chain(Self::host_chain(Direction::Forward)),
             Flush::chain(Self::guest_invalid_conntrack_chain()),
+            Flush::chain(Self::host_invalid_conntrack_chain()),
             Flush::map(Self::guest_vmap(Direction::In)),
             Flush::map(Self::guest_vmap(Direction::Out)),
             Flush::map(Self::bridge_vmap(Self::guest_table())),
@@ -533,12 +538,12 @@ impl Firewall {
             log::debug!("set block_invalid_conntrack");
 
             commands.push(Add::rule(AddRule::from_statement(
-                chain_in,
-                Statement::jump("block-conntrack-invalid"),
+                Self::guest_invalid_conntrack_chain(),
+                Statement::make_drop(),
             )));
 
             commands.push(Add::rule(AddRule::from_statement(
-                Self::guest_invalid_conntrack_chain(),
+                Self::host_invalid_conntrack_chain(),
                 Statement::make_drop(),
             )));
         }
diff --git a/proxmox-firewall/tests/snapshots/integration_tests__firewall.snap b/proxmox-firewall/tests/snapshots/integration_tests__firewall.snap
index 9194fc6..24f66a5 100644
--- a/proxmox-firewall/tests/snapshots/integration_tests__firewall.snap
+++ b/proxmox-firewall/tests/snapshots/integration_tests__firewall.snap
@@ -104,6 +104,15 @@ snapshot_kind: text
         }
       }
     },
+    {
+      "flush": {
+        "chain": {
+          "family": "inet",
+          "table": "proxmox-firewall",
+          "name": "invalid-conntrack"
+        }
+      }
+    },
     {
       "flush": {
         "map": {
@@ -3280,14 +3289,12 @@ snapshot_kind: text
     {
       "add": {
         "rule": {
-          "family": "inet",
-          "table": "proxmox-firewall",
-          "chain": "option-in",
+          "family": "bridge",
+          "table": "proxmox-firewall-guests",
+          "chain": "invalid-conntrack",
           "expr": [
             {
-              "jump": {
-                "target": "block-conntrack-invalid"
-              }
+              "drop": null
             }
           ]
         }
@@ -3296,8 +3303,8 @@ snapshot_kind: text
     {
       "add": {
         "rule": {
-          "family": "bridge",
-          "table": "proxmox-firewall-guests",
+          "family": "inet",
+          "table": "proxmox-firewall",
           "chain": "invalid-conntrack",
           "expr": [
             {
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH proxmox-firewall 1/2] fix: firewall: apply `nt_conntrack_allow_invalid` to all chains

[Stefan Hanreich]

default-in is also checking for conntrack status, so we should put this
there as well. Other than that consider this:

Tested-by: Stefan Hanreich <s.hanreich@proxmox.com>
Reviewed-by: Stefan Hanreich <s.hanreich@proxmox.com>


On 2/20/25 16:14, Hannes Laimer wrote:
> ... on the guest table. There is no reason to not repect that option
> on those two chains. These two were missed in the referenced commit.
> 
> Fixes: 64dc344b ("firewall: apply `nt_conntrack_allow_invalid` option to guest table")
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>  proxmox-firewall/resources/proxmox-firewall.nft | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/proxmox-firewall/resources/proxmox-firewall.nft b/proxmox-firewall/resources/proxmox-firewall.nft
> index 2dd7c48..30f7b4f 100644
> --- a/proxmox-firewall/resources/proxmox-firewall.nft
> +++ b/proxmox-firewall/resources/proxmox-firewall.nft
> @@ -356,7 +356,7 @@ table bridge proxmox-firewall-guests {
>      }
>  
>      chain pre-vm-out {
> -        meta protocol != arp ct state vmap { established : accept, related : accept, invalid : drop }
> +        meta protocol != arp ct state vmap { established : accept, related : accept, invalid : jump invalid-conntrack }
>      }
>  
>      chain vm-out {
> @@ -384,7 +384,7 @@ table bridge proxmox-firewall-guests {
>  
>      chain before-bridge {
>          meta protocol arp accept
> -        meta protocol != arp ct state vmap { established : accept, related : accept, invalid : drop }
> +        meta protocol != arp ct state vmap { established : accept, related : accept, invalid : jump invalid-conntrack }
>      }
>  
>      chain forward {



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH proxmox-firewall 1/2] fix: firewall: apply `nt_conntrack_allow_invalid` to all chains

[Hannes Laimer]

On 3/4/25 13:24, Stefan Hanreich wrote:
> default-in is also checking for conntrack status, so we should put this

I think `default-in` is currently noop'ing[1] ct state invalid, am I
missing something? I though maybe there's a reason for that, so I
left it as is, as with the change we'd drop there with invalid ct
state.

[1] 
https://git.proxmox.com/?p=proxmox-firewall.git;a=blob;f=proxmox-firewall/resources/proxmox-firewall.nft;h=2dd7c48bc68b3ddf404e53a1c7be9e624227be13;hb=refs/heads/master#l208
> there as well. Other than that consider this:



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH proxmox-firewall 1/2] fix: firewall: apply `nt_conntrack_allow_invalid` to all chains

[Stefan Hanreich]

On 3/12/25 11:18, Hannes Laimer wrote:
> On 3/4/25 13:24, Stefan Hanreich wrote:
>> default-in is also checking for conntrack status, so we should put this
> 
> I think `default-in` is currently noop'ing[1] ct state invalid, am I
> missing something? I though maybe there's a reason for that, so I
> left it as is, as with the change we'd drop there with invalid ct
> state.

Yes, it is - I think I also remember the reason now for not including
invalid initially. CT has issues with multicast / broadcast traffic,
since it is impossible to know the return IP. We had some issues with
DHCP on bridges with firewall-enabled guests for instance.

There are workarounds / solutions for this (ICMPv6 has explicit
exceptions in the conntrack module for instance, some protocols have
conntrack helpers), but they're not a silver bullet that work for every
conceivable protocol. There were some additional fixes in the kernel for
this in the meanwhile [1].

The old firewall does drop invalid CT traffic going in / out the host
unless the nt_conntrack_allow_invalid setting is set:

  -A PVEFW-HOST-IN -m conntrack --ctstate INVALID -j DROP
  -A PVEFW-HOST-IN -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT

  [...]

  -A PVEFW-HOST-OUT -m conntrack --ctstate INVALID -j DROP
  -A PVEFW-HOST-OUT -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT

Same for guest traffic:

  -A PVEFW-FORWARD -m conntrack --ctstate INVALID -j DROP
  -A PVEFW-FORWARD -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT


IMO we should explicitly check for CT state invalid everywhere we are
checking CT state, since dropping it at some place but not the other
seems wrong/inconsistent. I'd also definitely default to dropping
invalid traffic everywhere, unless explicitly disabled by the user. I
think it's best for now to stick to what the old firewall is doing? It
should have the same effect in the new firewall, and we don't really
have lots of reports of this behavior causing issues.

For users that *are* encountering issues with this behavior, we have an
escape hatch with nt_conntrack_allow_invalid, which is okay IMO since
that doesn't bypass the firewall (all invalid packets still go through
the ruleset and get either accepted / dropped according to the ruleset).
It only hurts performance for that connection and consumes some CPU cycles.

We need to rework how we are utilizing CT in the near future though. It
would be nice to implement support for notrack [2] in the firewall,
which would help users to handle this more surgically by explicitly
exempting certain connections from conntrack. For EVPN we also need to
revisit CT handling of the firewall. So this is something that's on the
roadmap anyway.


[1]
https://lore.kernel.org/lkml/ZfyeC8mjLnGkqnVT@calendula/t/#m32cb12199c69a77f14786ec17b2df8566f34fe95
[2] https://bugzilla.proxmox.com/show_bug.cgi?id=2441


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel