The branch main has been updated by markj:
URL:
https://cgit.FreeBSD.org/src/commit/?id=886396f1b1a727c642071965612e2c2c9dd11d6c
commit 886396f1b1a727c642071965612e2c2c9dd11d6c
Author: Mark Johnston <ma...@freebsd.org>
AuthorDate: 2025-01-16 15:44:40 +0000
Commit: Mark Johnston <ma...@freebsd.org>
CommitDate: 2025-01-16 16:45:16 +0000
pf: Force logging if pf_create_state() fails
Currently packets are logged before pf_create_state() is called, so we
might log a packet as passed that is subsequently dropped due to state
creation failure. In particular, the drop is not logged, which is
wrong.
Improve the situation a bit: force logging if state creation fails.
This isn't totally right as we'll end up logging the packet twice in
this case, but it's better than not logging the drop at all.
Add a regression test.
Discussed with: kp, ks
Co-authored-by: Franco Fichtner <fra...@opnsense.org>
MFC after: 2 weeks
Sponsored by: Klara, Inc.
Sponsored by: OPNsense
Differential Revision: https://reviews.freebsd.org/D47953
---
sys/netpfil/pf/pf.c | 1 +
tests/sys/netpfil/pf/pflog.sh | 64 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 65 insertions(+)
diff --git a/sys/netpfil/pf/pf.c b/sys/netpfil/pf/pf.c
index a49216a9dc20..cfab6a828d5f 100644
--- a/sys/netpfil/pf/pf.c
+++ b/sys/netpfil/pf/pf.c
@@ -5911,6 +5911,7 @@ nextrule:
&match_rules, udp_mapping);
if (action != PF_PASS) {
pf_udp_mapping_release(udp_mapping);
+ pd->act.log |= PF_LOG_FORCE;
if (action == PF_DROP &&
(r->rule_flag & PFRULE_RETURN))
pf_return(r, nr, pd, sk, th,
diff --git a/tests/sys/netpfil/pf/pflog.sh b/tests/sys/netpfil/pf/pflog.sh
index f5a1241cb5a8..968d165f3dcb 100644
--- a/tests/sys/netpfil/pf/pflog.sh
+++ b/tests/sys/netpfil/pf/pflog.sh
@@ -2,6 +2,7 @@
# SPDX-License-Identifier: BSD-2-Clause
#
# Copyright (c) 2023 Rubicon Communications, LLC (Netgate)
+# Copyright (c) 2024 Deciso B.V.
#
# Redistribution and use in source and binary forms, with or without
# modification, are permitted provided that the following conditions
@@ -132,8 +133,71 @@ matches_cleanup()
pft_cleanup
}
+atf_test_case "state_max" "cleanup"
+state_max_head()
+{
+ atf_set descr 'Ensure that drops due to state limits are logged'
+ atf_set require.user root
+}
+
+state_max_body()
+{
+ pflog_init
+
+ epair=$(vnet_mkepair)
+
+ vnet_mkjail alcatraz ${epair}a
+ jexec alcatraz ifconfig ${epair}a 192.0.2.1/24 up
+
+ ifconfig ${epair}b 192.0.2.2/24 up
+
+ # Sanity check
+ atf_check -s exit:0 -o ignore \
+ ping -c 1 192.0.2.1
+
+ jexec alcatraz pfctl -e
+ jexec alcatraz ifconfig pflog0 up
+ pft_set_rules alcatraz "pass log inet keep state (max 1)"
+
+ jexec alcatraz tcpdump -n -e -ttt --immediate-mode -l -U -i pflog0 >>
${PWD}/pflog.txt &
+ sleep 1 # Wait for tcpdump to start
+
+ atf_check -s exit:0 -o ignore \
+ ping -c 1 192.0.2.1
+
+ atf_check -s exit:2 -o ignore \
+ ping -c 1 192.0.2.1
+
+ echo "Rules"
+ jexec alcatraz pfctl -sr -vv
+ echo "States"
+ jexec alcatraz pfctl -ss -vv
+ echo "Log"
+ cat ${PWD}/pflog.txt
+
+ # First ping passes.
+ atf_check -o match:".*rule 0/0\(match\): pass in on ${epair}a:
192.0.2.2 > 192.0.2.1: ICMP echo request.*" \
+ cat pflog.txt
+
+ # Second ping is blocked due to the state limit.
+ atf_check -o match:".*rule 0/0\(match\): block in on ${epair}a:
192.0.2.2 > 192.0.2.1: ICMP echo request.*" \
+ cat pflog.txt
+
+ # At most three lines should be written: one for the first ping, and
+ # two for the second: one for the initial pass through the ruleset, and
+ # then a drop because of the state limit. Ideally only the drop would
+ # be logged; if this is fixed, the count will be 2 instead of 3.
+ atf_check -o match:3 grep -c . pflog.txt
+}
+
+state_max_cleanup()
+{
+ pft_cleanup
+}
+
atf_init_test_cases()
{
atf_add_test_case "malformed"
atf_add_test_case "matches"
+ atf_add_test_case "state_max"
}
