firewall: properly handle REJECT rules

Currently we generated DROP statements for all rules involving REJECT.
We only need to generate DROP when in the postrouting chain of tables
with type bridge, since REJECT is disallowed there. Otherwise we jump
into the do-reject chain which properly handles rejects for different
protocol types.

Reported-By: Stefan Sterz <s.sterz@proxmox.com>
Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>

proxmox-firewall/resources/proxmox-firewall.nft

@@ -285,7 +285,12 @@ table bridge proxmox-firewall-guests {
     }
 
     chain do-reject {
-        drop
+        meta pkttype broadcast drop
+        ip saddr 224.0.0.0/4 drop
+
+        meta l4proto tcp reject with tcp reset
+        meta l4proto icmp reject with icmp type port-unreachable
+        reject with icmp type host-prohibited
     }
 
     chain after-vm-in {

proxmox-firewall/src/firewall.rs

@@ -28,7 +28,7 @@ use proxmox_ve_config::guest::types::Vmid;
 
 use crate::config::FirewallConfig;
 use crate::object::{NftObjectEnv, ToNftObjects};
-use crate::rule::{NftRule, NftRuleEnv};
+use crate::rule::{generate_verdict, NftRule, NftRuleEnv};
 
 static CLUSTER_TABLE_NAME: &str = "proxmox-firewall";
 static HOST_TABLE_NAME: &str = "proxmox-firewall";
@@ -715,7 +715,10 @@ impl Firewall {
             None,
         )?;
 
-        commands.push(Add::rule(AddRule::from_statement(chain, default_policy)));
+        commands.push(Add::rule(AddRule::from_statement(
+            chain,
+            generate_verdict(default_policy, &env),
+        )));
 
         Ok(())
     }
@@ -827,7 +830,7 @@ impl Firewall {
 
         commands.push(Add::rule(AddRule::from_statement(
             chain,
-            config.default_policy(direction),
+            generate_verdict(config.default_policy(direction), &env),
         )));
 
         Ok(())

proxmox-firewall/src/rule.rs

@@ -4,7 +4,7 @@ use anyhow::{format_err, Error};
 use proxmox_nftables::{
     expression::{Ct, IpFamily, Meta, Payload, Prefix},
     statement::{Log, LogLevel, Match, Operator},
-    types::{AddRule, ChainPart, SetName},
+    types::{AddRule, ChainPart, SetName, TableFamily, TablePart},
     Expression, Statement,
 };
 use proxmox_ve_config::{
@@ -16,7 +16,7 @@ use proxmox_ve_config::{
             alias::AliasName,
             ipset::{Ipfilter, IpsetName},
             log::LogRateLimit,
-            rule::{Direction, Kind, RuleGroup},
+            rule::{Direction, Kind, RuleGroup, Verdict as ConfigVerdict},
             rule_match::{
                 Icmp, Icmpv6, IpAddrMatch, IpMatch, Ports, Protocol, RuleMatch, Sctp, Tcp, Udp,
             },
@@ -146,6 +146,14 @@ impl NftRuleEnv<'_> {
     fn contains_family(&self, family: Family) -> bool {
         self.chain.table().family().families().contains(&family)
     }
+
+    fn table(&self) -> &TablePart {
+        self.chain.table()
+    }
+
+    fn direction(&self) -> Direction {
+        self.direction
+    }
 }
 
 pub(crate) trait ToNftRules {
@@ -204,6 +212,14 @@ impl ToNftRules for RuleGroup {
     }
 }
 
+pub(crate) fn generate_verdict(verdict: ConfigVerdict, env: &NftRuleEnv) -> Statement {
+    match (env.table().family(), env.direction(), verdict) {
+        (TableFamily::Bridge, Direction::In, ConfigVerdict::Reject) => Statement::make_drop(),
+        (_, _, ConfigVerdict::Reject) => Statement::jump("do-reject"),
+        _ => Statement::from(verdict),
+    }
+}
+
 impl ToNftRules for RuleMatch {
     fn to_nft_rules(&self, rules: &mut Vec<NftRule>, env: &NftRuleEnv) -> Result<(), Error> {
         if env.direction != self.direction() {
@@ -230,7 +246,7 @@ impl ToNftRules for RuleMatch {
             }
         }
 
-        rules.push(NftRule::new(Statement::from(self.verdict())));
+        rules.push(NftRule::new(generate_verdict(self.verdict(), env)));
 
         if let Some(name) = &self.iface() {
             handle_iface(rules, env, name)?;

proxmox-firewall/tests/input/100.fw

@@ -19,4 +19,6 @@ dc/network1
 GROUP network1 -i net1
 IN ACCEPT -source 192.168.0.1/24,127.0.0.1-127.255.255.0,172.16.0.1 -dport 123,222:333 -sport http -p tcp
 IN DROP --icmp-type echo-request --proto icmp --log info
+IN REJECT -p udp --dport 443
+OUT REJECT -p udp --dport 443
 

proxmox-firewall/tests/input/host.fw

@@ -20,4 +20,6 @@ nf_conntrack_helpers: amanda,ftp,irc,netbios-ns,pptp,sane,sip,snmp,tftp
 IN DNS(ACCEPT) -source dc/network1 -log nolog
 IN DHCPv6(ACCEPT) -log nolog
 IN DHCPfwd(ACCEPT) -log nolog
+IN REJECT -p udp --dport 443
+OUT REJECT -p udp --dport 443
 

proxmox-firewall/tests/snapshots/integration_tests__firewall.snap

@@ -2153,6 +2153,84 @@ expression: "firewall.full_host_fw().expect(\"firewall can be generated\")"
         }
       }
     },
+    {
+      "add": {
+        "rule": {
+          "family": "inet",
+          "table": "proxmox-firewall",
+          "chain": "host-in",
+          "expr": [
+            {
+              "match": {
+                "op": "==",
+                "left": {
+                  "meta": {
+                    "key": "l4proto"
+                  }
+                },
+                "right": "udp"
+              }
+            },
+            {
+              "match": {
+                "op": "==",
+                "left": {
+                  "payload": {
+                    "protocol": "th",
+                    "field": "dport"
+                  }
+                },
+                "right": 443
+              }
+            },
+            {
+              "jump": {
+                "target": "do-reject"
+              }
+            }
+          ]
+        }
+      }
+    },
+    {
+      "add": {
+        "rule": {
+          "family": "inet",
+          "table": "proxmox-firewall",
+          "chain": "host-out",
+          "expr": [
+            {
+              "match": {
+                "op": "==",
+                "left": {
+                  "meta": {
+                    "key": "l4proto"
+                  }
+                },
+                "right": "udp"
+              }
+            },
+            {
+              "match": {
+                "op": "==",
+                "left": {
+                  "payload": {
+                    "protocol": "th",
+                    "field": "dport"
+                  }
+                },
+                "right": 443
+              }
+            },
+            {
+              "jump": {
+                "target": "do-reject"
+              }
+            }
+          ]
+        }
+      }
+    },
     {
       "add": {
         "set": {
@@ -3047,6 +3125,43 @@ expression: "firewall.full_host_fw().expect(\"firewall can be generated\")"
         }
       }
     },
+    {
+      "add": {
+        "rule": {
+          "family": "bridge",
+          "table": "proxmox-firewall-guests",
+          "chain": "guest-100-in",
+          "expr": [
+            {
+              "match": {
+                "op": "==",
+                "left": {
+                  "meta": {
+                    "key": "l4proto"
+                  }
+                },
+                "right": "udp"
+              }
+            },
+            {
+              "match": {
+                "op": "==",
+                "left": {
+                  "payload": {
+                    "protocol": "th",
+                    "field": "dport"
+                  }
+                },
+                "right": 443
+              }
+            },
+            {
+              "drop": null
+            }
+          ]
+        }
+      }
+    },
     {
       "add": {
         "element": {
@@ -3147,6 +3262,45 @@ expression: "firewall.full_host_fw().expect(\"firewall can be generated\")"
         }
       }
     },
+    {
+      "add": {
+        "rule": {
+          "family": "bridge",
+          "table": "proxmox-firewall-guests",
+          "chain": "guest-100-out",
+          "expr": [
+            {
+              "match": {
+                "op": "==",
+                "left": {
+                  "meta": {
+                    "key": "l4proto"
+                  }
+                },
+                "right": "udp"
+              }
+            },
+            {
+              "match": {
+                "op": "==",
+                "left": {
+                  "payload": {
+                    "protocol": "th",
+                    "field": "dport"
+                  }
+                },
+                "right": 443
+              }
+            },
+            {
+              "jump": {
+                "target": "do-reject"
+              }
+            }
+          ]
+        }
+      }
+    },
     {
       "add": {
         "element": {
@@ -3198,7 +3352,9 @@ expression: "firewall.full_host_fw().expect(\"firewall can be generated\")"
           "chain": "guest-100-out",
           "expr": [
             {
-              "drop": null
+              "jump": {
+                "target": "do-reject"
+              }
             }
           ]
         }

proxmox-nftables/src/statement.rs

@@ -39,6 +39,10 @@ impl Statement {
         Statement::Verdict(Verdict::Accept(Null))
     }
 
+    pub fn make_reject() -> Self {
+        Statement::Reject(Reject::default())
+    }
+
     pub const fn make_drop() -> Self {
         Statement::Verdict(Verdict::Drop(Null))
     }
@@ -118,7 +122,7 @@ impl From<ConfigVerdict> for Statement {
     fn from(value: ConfigVerdict) -> Self {
         match value {
             ConfigVerdict::Accept => Statement::make_accept(),
-            ConfigVerdict::Reject => Statement::make_drop(),
+            ConfigVerdict::Reject => Statement::make_reject(),
             ConfigVerdict::Drop => Statement::make_drop(),
         }
     }