From nobody Tue Dec 03 19:28:25 2024 X-Original-To: dev-commits-src-main@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4Y2rKj5Y7Fz5gdCJ; Tue, 03 Dec 2024 19:28:25 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R10" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4Y2rKj53g7z4fbQ; Tue, 3 Dec 2024 19:28:25 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1733254105; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=aJKiBVs0EOU0SoI4l10SvkWlOoMevmU7vqCi65I6vSI=; b=if+Qcn9wWwQFm9bgxcfqA4A90hwOmZy7DvLjTTEfCh0dNzpIYYajuYknp54kay3O9PdN1q bpb0GneJud1jNhqCx3TTRAzvhsldGykKbx3ZcJg7hUlV0DwYENN4ko2eiUIu5ccXxVhvbJ YiQimXgjXHk+Q5raEqaw1TQs0S/OK+U0dkeLvueiVeWcqUmcDjTQwQpTBije6pSqeVLeb3 r5k1Z7ESytrDrnSqjeNnPyTiiRSNmWa+LVFPU6VWcOoLvX0oggW7Z0KeDvOXv4WtSnQNMQ VjRMtfk3aOq+T808MF/nFeQipslzO0JzGAURXMPyAMU2p/LgoHSgjzDipf7fFg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1733254105; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=aJKiBVs0EOU0SoI4l10SvkWlOoMevmU7vqCi65I6vSI=; b=bzpqNwHT+pxt/zt4crocbxM7MBW71GMsV9uVzx91ByT/bOy1b/hhlM+vTv/v9ONTqrSoOL I+LrhBgqL7R8VFNovAjoolA97n5LDUboEKw6XNAhWwca/i5FCDQoMF5lKRo7uu216S+gNZ 6bIu5ZVZ+JuhiAMRuIzd/fbNvARMk2xmO75HdjHT9e3U1+EqPaSy519af4iR3LhBxmVnXs n4xs406hq26BGBzrssMmKiUUTGAyMn0LDtALrzET/VRO97uDz5bcyWCrxuwpaPXBnx+62W EErHqRlWYzBoYjZMHKj4qc6mI9D9HfITMk9JUenmrwX73ihiUYb9nDG24QwP4g== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1733254105; a=rsa-sha256; cv=none; b=V8YE+9CHibPF4t2o4kYdtQ0dqEGcLLfH+Mx1Nr58TLVD2/NtMGlkHxFNMOBT3AqxBIeglY X7T0mb6hB2BvTvtBak1y3esJQo6b3LNb+I7HQ2YaUhBICHhxbIHG+QfCCu/0MMui0CQ/dT YA81p7NDEsO6KeIldH+0YUmakUWcY8WKPyUW8kqDNWE3/PyaqiJOvi0ZrgcAO0LmZVWq+i 8pSZjVQl2g5yKTOVll7gJFT1nhZlJuChNArDUUw4DBlYKw7Lhh3cIpPyrD6u026hhhYFWj aHcyU5F5z4oz9iAUmY4OlDXky7OYjazG7dqol6q7lXmLkvMAwJBCEEHkfM80vg== Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 4Y2rKj4Swlzc2Q; Tue, 3 Dec 2024 19:28:25 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.18.1/8.18.1) with ESMTP id 4B3JSPlS087375; Tue, 3 Dec 2024 19:28:25 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.18.1/8.18.1/Submit) id 4B3JSPpp087372; Tue, 3 Dec 2024 19:28:25 GMT (envelope-from git) Date: Tue, 3 Dec 2024 19:28:25 GMT Message-Id: <202412031928.4B3JSPpp087372@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: Kristof Provost Subject: git: c22c98798456 - main - pf: fix potential NULL dereference in SCTP multihome handling List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-main@freebsd.org Sender: owner-dev-commits-src-main@FreeBSD.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: kp X-Git-Repository: src X-Git-Refname: refs/heads/main X-Git-Reftype: branch X-Git-Commit: c22c9879845653abb365e468daaa621e3f8f767a Auto-Submitted: auto-generated The branch main has been updated by kp: URL: https://cgit.FreeBSD.org/src/commit/?id=c22c9879845653abb365e468daaa621e3f8f767a commit c22c9879845653abb365e468daaa621e3f8f767a Author: Kristof Provost AuthorDate: 2024-12-03 18:27:49 +0000 Commit: Kristof Provost CommitDate: 2024-12-03 18:27:49 +0000 pf: fix potential NULL dereference in SCTP multihome handling 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 MFC after: 3 weeks Sponsored by: Orange Business Services --- sys/netpfil/pf/pf.c | 6 ---- sys/netpfil/pf/pf_if.c | 3 ++ tests/sys/netpfil/pf/sctp.py | 77 +++++++++++++++++++++++++++++++++++++++++--- 3 files changed, 76 insertions(+), 10 deletions(-) diff --git a/sys/netpfil/pf/pf.c b/sys/netpfil/pf/pf.c index fa656f92f79b..f587fa3fb196 100644 --- a/sys/netpfil/pf/pf.c +++ b/sys/netpfil/pf/pf.c @@ -6592,12 +6592,6 @@ 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. - */ - j->pd.kif = V_pfi_all; ret = pf_test_rule(&r, &sm, &j->pd, &ra, &rs, NULL); PF_RULES_RUNLOCK(); diff --git a/sys/netpfil/pf/pf_if.c b/sys/netpfil/pf/pf_if.c index 95f63ed663ca..650a7e4db799 100644 --- a/sys/netpfil/pf/pf_if.c +++ b/sys/netpfil/pf/pf_if.c @@ -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); diff --git a/tests/sys/netpfil/pf/sctp.py b/tests/sys/netpfil/pf/sctp.py index 6042badffb64..38bb9f2dff74 100644 --- a/tests/sys/netpfil/pf/sctp.py +++ b/tests/sys/netpfil/pf/sctp.py @@ -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,38 @@ 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. + 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"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. @@ -387,9 +421,9 @@ 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"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. @@ -518,9 +554,42 @@ class TestSCTPv6(VnetTestTemplate): 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([ + "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.