From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 57F2620EC7F for ; Tue, 23 Apr 2024 18:03:25 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id ECD3A35912; Tue, 23 Apr 2024 18:03:28 +0200 (CEST) From: Stefan Hanreich To: pve-devel@lists.proxmox.com Date: Tue, 23 Apr 2024 18:02:53 +0200 Message-Id: <20240423160253.477830-1-s.hanreich@proxmox.com> X-Mailer: git-send-email 2.39.2 MIME-Version: 1.0 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.269 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment KAM_LAZY_DOMAIN_SECURITY 1 Sending domain does not have any anti-forgery methods RDNS_NONE 0.793 Delivered to internal network by a host with no rDNS SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_NONE 0.001 SPF: sender does not publish an SPF Record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [firewall.rs, statement.rs, rule.rs] Subject: [pve-devel] [PATCH proxmox-firewall] firewall: properly handle REJECT rules X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" 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. Signed-off-by: Stefan Hanreich --- Seems like the proper handling for this got lost somewhere during my big refactoring :/ .../resources/proxmox-firewall.nft | 7 +- proxmox-firewall/src/firewall.rs | 9 +- proxmox-firewall/src/rule.rs | 22 ++- proxmox-firewall/tests/input/100.fw | 2 + proxmox-firewall/tests/input/host.fw | 2 + .../integration_tests__firewall.snap | 158 +++++++++++++++++- proxmox-nftables/src/statement.rs | 6 +- 7 files changed, 197 insertions(+), 9 deletions(-) diff --git a/proxmox-firewall/resources/proxmox-firewall.nft b/proxmox-firewall/resources/proxmox-firewall.nft index 67dd8c8..f36bf3b 100644 --- a/proxmox-firewall/resources/proxmox-firewall.nft +++ b/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 { diff --git a/proxmox-firewall/src/firewall.rs b/proxmox-firewall/src/firewall.rs index b137f58..509e295 100644 --- a/proxmox-firewall/src/firewall.rs +++ b/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(()) diff --git a/proxmox-firewall/src/rule.rs b/proxmox-firewall/src/rule.rs index c8099d0..02f964e 100644 --- a/proxmox-firewall/src/rule.rs +++ b/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, 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)?; diff --git a/proxmox-firewall/tests/input/100.fw b/proxmox-firewall/tests/input/100.fw index 6cf9fff..1aa9b00 100644 --- a/proxmox-firewall/tests/input/100.fw +++ b/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 diff --git a/proxmox-firewall/tests/input/host.fw b/proxmox-firewall/tests/input/host.fw index 8fa57e6..a61b0bd 100644 --- a/proxmox-firewall/tests/input/host.fw +++ b/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 diff --git a/proxmox-firewall/tests/snapshots/integration_tests__firewall.snap b/proxmox-firewall/tests/snapshots/integration_tests__firewall.snap index 7611a64..092ccef 100644 --- a/proxmox-firewall/tests/snapshots/integration_tests__firewall.snap +++ b/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" + } } ] } diff --git a/proxmox-nftables/src/statement.rs b/proxmox-nftables/src/statement.rs index e89f678..5483368 100644 --- a/proxmox-nftables/src/statement.rs +++ b/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 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(), } } -- 2.39.2 _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel