upstream/opnsense-src
1
0
Fork 0
mirror of https://github.com/opnsense/src.git synced 2026-05-28 04:12:45 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions

pf: fix potential NULL dereference in SCTP multihome handling

Browse source 
When processing an SCTP ASCONF we re-run the rules processing to check
if the new state should be allowed as well. We used to do so against the
'all' interface, to allow new connections to use any interface.

This is problematic for two reasons, the first being it may unexpectedly
bypass interface restrictions. The more important one is that it
can trigger panics. If the ruleset contains a rule which filters on
interface group we'd attempt to process the group list for the 'all'
interface. As this isn't a real interface it doesn't have an associated
struct ifnet, and we end up dereferencing a NULL pointer.

Solve this by not overriding the interface, instead leaving the physical
interface the SCTP ASCONF arrived on. This implies that we may end up
binding to that interface (if if-bound), and thus denying traffic on
other interfaces. Users can allow this anyway by setting 'state-policy
floating' on the relevant SCTP rules. This arguably better reflects user
intent as well. That is, we'll consider SCTP multihomed states to be
floating if we're in floating mode, and if-bound if we're if-bound.

Update the test cases to account for this, while adding a "pass on
lo" (i.e. pass on an interface group") rule to provoke this issue. Add
separate test cases for the floating and if-bound scenarios.

Reported by:	Franco Fichtner <franco@opnsense.org>
MFC after:	3 weeks
Sponsored by:	Orange Business Services
This commit is contained in:
Kristof Provost 2024-12-03 19:27:49 +01:00 committed by Franco Fichtner
parent e2912ba47a
commit 8a76301ae7
3 changed files with 77 additions and 10 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

7
sys/netpfil/pf/pf.c
View file

@ -6409,12 +6409,7 @@ again:
j->pd.sctp_flags |= PFDESC_SCTP_ADD_IP;
PF_RULES_RLOCK();
sm = NULL;
/*
* New connections need to be floating, because
* we cannot know what interfaces it will use.
* That's why we pass V_pfi_all rather than kif.
*/
ret = pf_test_rule(&r, &sm, V_pfi_all,
ret = pf_test_rule(&r, &sm, kif,
j->m, off, &j->pd, &ra, &rs, NULL);
PF_RULES_RUNLOCK();
SDT_PROBE4(pf, sctp, multihome, test, kif, r, j->m, ret);

3
sys/netpfil/pf/pf_if.c
View file

@ -426,6 +426,9 @@ pfi_kkif_match(struct pfi_kkif *rule_kif, struct pfi_kkif *packet_kif)
NET_EPOCH_ASSERT();
MPASS(packet_kif != NULL);
MPASS(packet_kif->pfik_ifp != NULL);
if (rule_kif == NULL || rule_kif == packet_kif)
return (1);

77
tests/sys/netpfil/pf/sctp.py
View file

@ -268,7 +268,8 @@ class TestSCTP(VnetTestTemplate):
ToolsHelper.print_output("/sbin/pfctl -e")
ToolsHelper.pf_rules([
"block proto sctp",
"pass inet proto sctp to 192.0.2.0/24"])
"pass inet proto sctp to 192.0.2.0/24",
"pass on lo"])
# Sanity check, we can communicate with the primary address.
client = SCTPClient("192.0.2.3", 1234)
@ -305,6 +306,7 @@ class TestSCTP(VnetTestTemplate):
ToolsHelper.print_output("/sbin/pfctl -e")
ToolsHelper.pf_rules([
"block proto sctp",
"pass on lo",
"pass inet proto sctp from 192.0.2.0/24"])
# Sanity check, we can communicate with the primary address.
@ -362,7 +364,7 @@ class TestSCTP(VnetTestTemplate):
@pytest.mark.require_user("root")
def test_permutation(self):
def test_permutation_if_bound(self):
# Test that we generate all permutations of src/dst addresses.
# Assign two addresses to each end, and check for the expected states
srv_vnet = self.vnet_map["vnet2"]
@ -374,6 +376,7 @@ class TestSCTP(VnetTestTemplate):
ToolsHelper.pf_rules([
"set state-policy if-bound",
"block proto sctp",
"pass on lo",
"pass inet proto sctp to 192.0.2.0/24"])
# Sanity check, we can communicate with the primary address.
@ -387,9 +390,40 @@ class TestSCTP(VnetTestTemplate):
# Check that we have a state for 192.0.2.3 and 192.0.2.2 to 192.0.2.1, but also to 192.0.2.4
states = ToolsHelper.get_output("/sbin/pfctl -ss")
print(states)
assert re.search(r".*sctp 192.0.2.1:.*192.0.2.3:1234", states)
assert re.search(r"epair.*sctp 192.0.2.1:.*192.0.2.3:1234", states)
assert re.search(r"epair.*sctp 192.0.2.1:.*192.0.2.2:1234", states)
assert re.search(r"epair.*sctp 192.0.2.4:.*192.0.2.3:1234", states)
assert re.search(r"epair.*sctp 192.0.2.4:.*192.0.2.2:1234", states)
@pytest.mark.require_user("root")
def test_permutation_floating(self):
# Test that we generate all permutations of src/dst addresses.
# Assign two addresses to each end, and check for the expected states
srv_vnet = self.vnet_map["vnet2"]
ifname = self.vnet_map["vnet1"].iface_alias_map["if1"].name
ToolsHelper.print_output("/sbin/ifconfig %s inet alias 192.0.2.4/24" % ifname)
ToolsHelper.print_output("/sbin/pfctl -e")
ToolsHelper.pf_rules([
"block proto sctp",
"pass on lo",
"pass inet proto sctp to 192.0.2.0/24"])
# Sanity check, we can communicate with the primary address.
client = SCTPClient("192.0.2.3", 1234)
client.send(b"hello", 0)
rcvd = self.wait_object(srv_vnet.pipe)
print(rcvd)
assert rcvd['ppid'] == 0
assert rcvd['data'] == "hello"
# Check that we have a state for 192.0.2.3 and 192.0.2.2 to 192.0.2.1, but also to 192.0.2.4
states = ToolsHelper.get_output("/sbin/pfctl -ss")
print(states)
assert re.search(r"all sctp 192.0.2.1:.*192.0.2.3:1234", states)
assert re.search(r"all sctp 192.0.2.1:.*192.0.2.2:1234", states)
assert re.search(r".*sctp 192.0.2.4:.*192.0.2.3:1234", states)
assert re.search(r"all sctp 192.0.2.4:.*192.0.2.3:1234", states)
assert re.search(r"all sctp 192.0.2.4:.*192.0.2.2:1234", states)
class TestSCTPv6(VnetTestTemplate):
@ -417,6 +451,7 @@ class TestSCTPv6(VnetTestTemplate):
ToolsHelper.print_output("/sbin/pfctl -e")
ToolsHelper.pf_rules([
"block proto sctp",
"pass on lo",
"pass inet6 proto sctp to 2001:db8::0/64"])
# Sanity check, we can communicate with the primary address.
@ -454,6 +489,7 @@ class TestSCTPv6(VnetTestTemplate):
ToolsHelper.print_output("/sbin/pfctl -e")
ToolsHelper.pf_rules([
"block proto sctp",
"pass on lo",
"pass inet6 proto sctp from 2001:db8::/64"])
# Sanity check, we can communicate with the primary address.
@ -520,7 +556,40 @@ class TestSCTPv6(VnetTestTemplate):
ToolsHelper.print_output("/sbin/pfctl -e")
ToolsHelper.pf_rules([
"set state-policy if-bound",
"block proto sctp",
"pass on lo",
"pass inet6 proto sctp to 2001:db8::0/64"])
# Sanity check, we can communicate with the primary address.
client = SCTPClient("2001:db8::3", 1234)
client.send(b"hello", 0)
rcvd = self.wait_object(srv_vnet.pipe)
print(rcvd)
assert rcvd['ppid'] == 0
assert rcvd['data'] == "hello"
# Check that we have a state for 2001:db8::3 and 2001:db8::2 to 2001:db8::1, but also to 2001:db8::4
states = ToolsHelper.get_output("/sbin/pfctl -ss")
print(states)
assert re.search(r"epair.*sctp 2001:db8::1\[.*2001:db8::2\[1234\]", states)
assert re.search(r"epair.*sctp 2001:db8::1\[.*2001:db8::3\[1234\]", states)
assert re.search(r"epair.*sctp 2001:db8::4\[.*2001:db8::2\[1234\]", states)
assert re.search(r"epair.*sctp 2001:db8::4\[.*2001:db8::3\[1234\]", states)
@pytest.mark.require_user("root")
def test_permutation_floating(self):
# Test that we generate all permutations of src/dst addresses.
# Assign two addresses to each end, and check for the expected states
srv_vnet = self.vnet_map["vnet2"]
ifname = self.vnet_map["vnet1"].iface_alias_map["if1"].name
ToolsHelper.print_output("/sbin/ifconfig %s inet6 alias 2001:db8::4/64" % ifname)
ToolsHelper.print_output("/sbin/pfctl -e")
ToolsHelper.pf_rules([
"block proto sctp",
"pass on lo",
"pass inet6 proto sctp to 2001:db8::0/64"])
# Sanity check, we can communicate with the primary address.