From: Stefan Hanreich <s.hanreich@proxmox.com>
To: pve-devel@lists.proxmox.com
Subject: [pve-devel] [PATCH proxmox-firewall] firewall: properly handle REJECT rules
Date: Tue, 23 Apr 2024 18:02:53 +0200 [thread overview]
Message-ID: <20240423160253.477830-1-s.hanreich@proxmox.com> (raw)
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 <s.hanreich@proxmox.com>
---
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<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)?;
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<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(),
}
}
--
2.39.2
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
next reply other threads:[~2024-04-23 16:03 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-23 16:02 Stefan Hanreich [this message]
2024-04-23 16:27 ` Stefan Hanreich
2024-04-23 16:37 ` [pve-devel] applied: " Thomas Lamprecht
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240423160253.477830-1-s.hanreich@proxmox.com \
--to=s.hanreich@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox