On systems with the FreeBSD-SA-24:05 fix [1] applied and pf firewall enabled, replies to ping initiated from machines on networks behind pf firewall/NAT to anything outside the local networks get blocked (state violation), resulting in 'Request timed out'. - Reverting the security "fix" fixes the issue. - Disabling pf completely (pfctl -d) mitigates the issue. - For IPv4, permitting ICMP time exceeded in addition to echo reply does not help either (NAT state is missing). [1] https://www.freebsd.org/security/advisories/FreeBSD-SA-24:05.pf.asc
We're going to need more details here. What's the ruleset, what's the network configuration and what's the traffic?
Created attachment 252624 [details] pf ruleset
(In reply to Kristof Provost from comment #1) This bug is trivially reproducible. - Dead simple WAN (DHCP) and LAN (static /24). - The traffic is a simple traceroute from a LAN machine. - Ruleset attached above. Broken with the SA applied: > tracert 8.8.8.8 Tracing route to dns.google [8.8.8.8] over a maximum of 30 hops: 1 <1 ms <1 ms <1 ms gw.localdomain [192.168.1.1] 2 * * * Request timed out. 3 * * * Request timed out. 4 * * * Request timed out. 5 * * * Request timed out. 6 * * * Request timed out. 7 * * * Request timed out. 8 8 ms 7 ms 8 ms dns.google [8.8.8.8] Working without the SA applied: > tracert 8.8.8.8 Tracing route to dns.google [8.8.8.8] over a maximum of 30 hops: 1 <1 ms <1 ms <1 ms gw.localdomain [192.168.1.1] 2 7 ms 6 ms 6 ms <redacted>.tmcz.cz [redacted] 3 * * * Request timed out. 4 8 ms 8 ms 8 ms 213.29.94.201 5 8 ms 8 ms 8 ms 192.178.68.76 6 8 ms 8 ms 8 ms 192.178.98.175 7 8 ms 8 ms 8 ms 209.85.245.247 8 7 ms 7 ms 7 ms dns.google [8.8.8.8]
Please include a network capture on the WAN side. `tcpdump -n -i $wan -s0 -w wan.pcap`
Created attachment 252625 [details] PCAP - broken with SA-24:05 applied
Created attachment 252626 [details] PCAP - working with SA-24:05 reverted (In reply to Kristof Provost from comment #4) Relevant captures in broken (SA-24:05 applied) and working (SA-24:05 reverted) states attached.
This affects at least OpnSense 24.7.1 installations. And IPv6 is also affected in the same way as IPv4 trafic - ICMP traceroutes cannot pass any more since the "fix".
Have same issues after upgrading to 13.3-RELEASE-p5. from router: $ mtr -wrn -c 10 -4 8.8.8.8 Start: 2024-08-12T11:04:48+0300 HOST: xxxx.viklenko.net Loss% Snt Last Avg Best Wrst StDev 1.|-- 10.99.0.2 0.0% 10 3.1 2.1 1.5 4.2 0.9 2.|-- 142.250.162.106 0.0% 10 10.7 10.7 10.5 11.0 0.2 3.|-- 142.250.238.59 0.0% 10 9.6 10.0 8.7 13.5 1.3 4.|-- 74.125.245.84 0.0% 10 11.7 11.0 9.6 16.5 2.2 5.|-- 142.251.224.82 0.0% 10 24.9 24.9 24.3 25.2 0.3 6.|-- 142.251.77.181 0.0% 10 24.9 26.7 24.2 34.9 3.8 7.|-- 192.178.72.181 0.0% 10 25.2 25.2 25.1 25.5 0.1 8.|-- 172.253.65.37 0.0% 10 24.3 24.4 23.9 24.7 0.2 9.|-- 8.8.8.8 0.0% 10 24.3 24.3 24.1 24.5 0.1 from host behind the router: $ mtr -wrn -c 10 -4 8.8.8.8 Start: 2024-08-12T11:05:26+0300 HOST: xxx.viklenko.net Loss% Snt Last Avg Best Wrst StDev 1.|-- 192.168.32.62 0.0% 10 1.4 0.6 0.2 1.6 0.5 2.|-- ??? 100.0 10 0.0 0.0 0.0 0.0 0.0 3.|-- ??? 100.0 10 0.0 0.0 0.0 0.0 0.0 4.|-- ??? 100.0 10 0.0 0.0 0.0 0.0 0.0 5.|-- ??? 100.0 10 0.0 0.0 0.0 0.0 0.0 6.|-- ??? 100.0 10 0.0 0.0 0.0 0.0 0.0 7.|-- ??? 100.0 10 0.0 0.0 0.0 0.0 0.0 8.|-- ??? 100.0 10 0.0 0.0 0.0 0.0 0.0 9.|-- ??? 100.0 10 0.0 0.0 0.0 0.0 0.0 10.|-- 8.8.8.8 0.0% 10 24.4 24.4 23.8 24.8 0.3
I can confirm an issue on 14.1-RELEASE-p3 too.
We did a quick bisect and it's likely caused by a change within https://cgit.freebsd.org/src/commit/?id=534ee17e61ee094ec
Looks like some code has been missed when forward-porting a 2009 (!!!) OpenBSD patch. Something between the patch date and OpenBSD 4.8 release. https://marc.info/?l=openbsd-misc&m=128218328308200&w=2 Happy hunting.
Going fishing... How about this one? :) https://github.com/openbsd/src/commit/ef4bccd7509e
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=34063cb714602972b6d985ad747fc8f66a8daae1 commit 34063cb714602972b6d985ad747fc8f66a8daae1 Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2024-08-12 10:14:43 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2024-08-13 11:24:17 +0000 pf tests: ensure that traceroutes using ICMP work PR: 280701 MFC after: 1 week Sponsored by: Rubicon Communications, LLC ("Netgate") tests/sys/netpfil/pf/icmp.sh | 65 +++++++++++++++++++++++++++++++++++++++++++ tests/sys/netpfil/pf/icmp6.sh | 65 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+)
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=2da98eef1f352c496ffd458b4c68ddee972bb903 commit 2da98eef1f352c496ffd458b4c68ddee972bb903 Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2024-08-12 14:07:35 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2024-08-13 11:23:17 +0000 pf: fix icmp-in-icmp state lookup In 534ee17e6 pf state checking for ICMP(v6) was made stricter. This change failed to correctly set the pf_pdesc for ICMP-in-ICMP lookups, resulting in ICMP error packets potentially being dropped incorrectly. Specially, it copied the ICMP header into a separate variable, not into the pf_pdesc. Populate the required pf_pdesc fields for the embedded ICMP packet's state lookup. PR: 280701 MFC after: 1 week Sponsored by: Rubicon Communications, LLC ("Netgate") sys/netpfil/pf/pf.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-)
(In reply to commit-hook from comment #14) Unfortunately, that fixes IPv4 but is even more broken with ICMPv6, now even the first hop (the FreeBSD router) is not shown from machines behind the router. Windows 11 machine: > tracert -6 www.google.com Tracing route to www.google.com [2a00:1450:4014:80a::2004] over a maximum of 30 hops: 1 * * * Request timed out. 2 * * * Request timed out. 3 * * * Request timed out. 4 * * * Request timed out. 5 * * * Request timed out. 6 * * * Request timed out. 7 * * * Request timed out. 8 7 ms 7 ms 7 ms prg03s10-in-x04.1e100.net [2a00:1450:4014:80a::2004] Trace complete. Ubuntu 22 LTS machine: $ traceroute6 -I www.google.com traceroute to www.google.com (2a00:1450:4014:80a::2004), 30 hops max, 80 byte packets 1 * * * 2 * * * 3 * * * 4 * * * 5 * * * 6 * * * 7 * * * 8 prg03s10-in-x04.1e100.net (2a00:1450:4014:80a::2004) 6.992 ms 7.055 ms 7.051 ms Directly from the router, it works. # traceroute6 -I www.google.com traceroute6 to www.google.com (2a00:1450:4014:80a::2004) from 2001:1ae9::xxxx, 64 hops max, 20 byte packets 1 * * * 2 * * * 3 2001:af0:f::1da 6.427 ms 6.587 ms * 4 2001:4860:1:1::1d50 6.787 ms 6.929 ms 6.860 ms 5 2001:4860:0:1::7ee5 6.873 ms 6.702 ms 6.545 ms 6 2001:4860:0:1::389b 7.082 ms 6.724 ms 6.658 ms 7 prg03s10-in-x04.1e100.net 6.766 ms 6.754 ms 6.170 ms # mtr -wrn -c 10 -6 www.google.com Start: 2024-08-14T09:47:37+0200 HOST: gw.localocaldomain Loss% Snt Last Avg Best Wrst StDev 1.|-- ??? 100.0 10 0.0 0.0 0.0 0.0 0.0 2.|-- ??? 100.0 10 0.0 0.0 0.0 0.0 0.0 3.|-- 2001:af0:f::1da 60.0% 10 6.7 7.0 6.7 7.3 0.3 4.|-- 2001:4860:1:1::1d50 0.0% 10 7.0 7.0 6.6 7.6 0.3 5.|-- 2001:4860:0:1::7ee5 0.0% 10 7.0 6.9 6.6 7.4 0.3 6.|-- 2001:4860:0:1::389b 0.0% 10 7.0 7.1 6.6 7.7 0.3 7.|-- 2a00:1450:4014:80a::2004 0.0% 10 6.7 7.0 6.7 7.3 0.2
We will want to issue a revised SA-24:05.pf advisory with a corrected patch. The revised advisory should also include a patch to fix systems broken by the previous patch. See https://www.freebsd.org/security/advisories/FreeBSD-SA-18:01.ipsec.asc for how we handled this the previous time a security advisory broke working installations. What's a good timeline for this?
(In reply to Philip Paeps from comment #16) I think it'd be goog to wait for feedback from other users who confirmed the regression here, since - for me at least - things are still broken with ICMPv6 even with 2da98eef1f352c496ffd458b4c68ddee972bb903 applied on top of the SA patch, see comment #15.
I agree that we shouldn't rush this. Whenever we're happy that both the security issue and the regression have been addressed (and thoroughly tested), we can send out a revised advisory.
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=89f6723288b0d27d3f14f93e6e83f672fa2b8aca commit 89f6723288b0d27d3f14f93e6e83f672fa2b8aca Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2024-08-14 09:29:30 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2024-08-14 12:15:07 +0000 pf: invert direction for inner icmp state lookups (e.g. traceroute with icmp) ok henning, jsing Also extend the test case to cover this scenario. PR: 280701 Obtained from: OpenBSD MFC after: 1 week Sponsored by: Rubicon Communications, LLC ("Netgate") sys/netpfil/pf/pf.c | 21 +++++++++++---------- tests/sys/netpfil/pf/icmp.sh | 4 +++- tests/sys/netpfil/pf/icmp6.sh | 4 +++- 3 files changed, 17 insertions(+), 12 deletions(-)
(In reply to commit-hook from comment #19) Nice, looks good now with both IPv4 and IPv6. Also tried logging of the packets with pf, seems to work as well.
Both extra patches combined look promising. There are some conflicting reports on whether they fix all edge cases: 1. mtr may still have issues. 2. IPv6 ICMP ping packets appear to be dropped sometimes. 3. Client devices (Android and Chromecast) disconnecting sporadically. Both things are confirmed working on kernels prior to this SA or when reverted. In the spirit of moving this along we will ship the fixes instead of reverts and will be happy to assist with remaining issues if they persist. Cheers, Franco
A commit in branch stable/14 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=46c4fc50d3012ca3c8756df243589add36b70830 commit 46c4fc50d3012ca3c8756df243589add36b70830 Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2024-08-14 09:29:30 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2024-08-20 15:15:10 +0000 pf: invert direction for inner icmp state lookups (e.g. traceroute with icmp) ok henning, jsing Also extend the test case to cover this scenario. PR: 280701 Obtained from: OpenBSD MFC after: 1 week Sponsored by: Rubicon Communications, LLC ("Netgate") (cherry picked from commit 89f6723288b0d27d3f14f93e6e83f672fa2b8aca) sys/netpfil/pf/pf.c | 21 +++++++++++---------- tests/sys/netpfil/pf/icmp.sh | 4 +++- tests/sys/netpfil/pf/icmp6.sh | 4 +++- 3 files changed, 17 insertions(+), 12 deletions(-)
A commit in branch stable/14 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=3455a02b5aed6f24f425b6a4fad4256fe74b13ed commit 3455a02b5aed6f24f425b6a4fad4256fe74b13ed Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2024-08-12 10:14:43 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2024-08-20 15:15:10 +0000 pf tests: ensure that traceroutes using ICMP work PR: 280701 MFC after: 1 week Sponsored by: Rubicon Communications, LLC ("Netgate") (cherry picked from commit 34063cb714602972b6d985ad747fc8f66a8daae1) tests/sys/netpfil/pf/icmp.sh | 65 +++++++++++++++++++++++++++++++++++++++++++ tests/sys/netpfil/pf/icmp6.sh | 65 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+)
A commit in branch stable/14 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=27a1a56b0d2e6ffa6ab1de69ef84fe66b7fd41e0 commit 27a1a56b0d2e6ffa6ab1de69ef84fe66b7fd41e0 Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2024-08-12 14:07:35 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2024-08-20 15:15:09 +0000 pf: fix icmp-in-icmp state lookup In 534ee17e6 pf state checking for ICMP(v6) was made stricter. This change failed to correctly set the pf_pdesc for ICMP-in-ICMP lookups, resulting in ICMP error packets potentially being dropped incorrectly. Specially, it copied the ICMP header into a separate variable, not into the pf_pdesc. Populate the required pf_pdesc fields for the embedded ICMP packet's state lookup. PR: 280701 MFC after: 1 week Sponsored by: Rubicon Communications, LLC ("Netgate") (cherry picked from commit 2da98eef1f352c496ffd458b4c68ddee972bb903) sys/netpfil/pf/pf.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-)
A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=5f3f07397a7909e8f9449d1aa0b465159cbf0d60 commit 5f3f07397a7909e8f9449d1aa0b465159cbf0d60 Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2024-08-14 09:29:30 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2024-08-20 07:25:21 +0000 pf: invert direction for inner icmp state lookups (e.g. traceroute with icmp) ok henning, jsing Also extend the test case to cover this scenario. PR: 280701 Obtained from: OpenBSD MFC after: 1 week Sponsored by: Rubicon Communications, LLC ("Netgate") (cherry picked from commit 89f6723288b0d27d3f14f93e6e83f672fa2b8aca) sys/netpfil/pf/pf.c | 21 +++++++++++---------- tests/sys/netpfil/pf/icmp.sh | 4 +++- tests/sys/netpfil/pf/icmp6.sh | 4 +++- 3 files changed, 17 insertions(+), 12 deletions(-)
A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=0d8d4cc3ea47f1ee61d749b22b135eb73c7d33cd commit 0d8d4cc3ea47f1ee61d749b22b135eb73c7d33cd Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2024-08-12 14:07:35 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2024-08-20 07:21:51 +0000 pf: fix icmp-in-icmp state lookup In 534ee17e6 pf state checking for ICMP(v6) was made stricter. This change failed to correctly set the pf_pdesc for ICMP-in-ICMP lookups, resulting in ICMP error packets potentially being dropped incorrectly. Specially, it copied the ICMP header into a separate variable, not into the pf_pdesc. Populate the required pf_pdesc fields for the embedded ICMP packet's state lookup. PR: 280701 MFC after: 1 week Sponsored by: Rubicon Communications, LLC ("Netgate") (cherry picked from commit 2da98eef1f352c496ffd458b4c68ddee972bb903) sys/netpfil/pf/pf.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-)
A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=7024e1066d5aba76dbbc85eb191357da7d32c619 commit 7024e1066d5aba76dbbc85eb191357da7d32c619 Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2024-08-12 10:14:43 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2024-08-20 07:21:57 +0000 pf tests: ensure that traceroutes using ICMP work PR: 280701 MFC after: 1 week Sponsored by: Rubicon Communications, LLC ("Netgate") (cherry picked from commit 34063cb714602972b6d985ad747fc8f66a8daae1) tests/sys/netpfil/pf/icmp.sh | 65 +++++++++++++++++++++++++++++++++++++++++++ tests/sys/netpfil/pf/icmp6.sh | 65 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+)
A commit in branch releng/13.4 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=6a7bac2ae79667c2b31169a8d0e91410986336fa commit 6a7bac2ae79667c2b31169a8d0e91410986336fa Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2024-08-12 10:14:43 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2024-08-21 07:44:16 +0000 pf tests: ensure that traceroutes using ICMP work PR: 280701 Approved by: re (cperciva) MFC after: 1 week Sponsored by: Rubicon Communications, LLC ("Netgate") (cherry picked from commit 34063cb714602972b6d985ad747fc8f66a8daae1) (cherry picked from commit 7024e1066d5aba76dbbc85eb191357da7d32c619) tests/sys/netpfil/pf/icmp.sh | 65 +++++++++++++++++++++++++++++++++++++++++++ tests/sys/netpfil/pf/icmp6.sh | 65 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+)
A commit in branch releng/13.4 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=9c67287ccfb7257d140b46c8d8aed7276c94d5f1 commit 9c67287ccfb7257d140b46c8d8aed7276c94d5f1 Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2024-08-14 09:29:30 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2024-08-21 07:44:25 +0000 pf: invert direction for inner icmp state lookups (e.g. traceroute with icmp) ok henning, jsing Also extend the test case to cover this scenario. PR: 280701 Approved by: re (cperciva) Obtained from: OpenBSD MFC after: 1 week Sponsored by: Rubicon Communications, LLC ("Netgate") (cherry picked from commit 89f6723288b0d27d3f14f93e6e83f672fa2b8aca) (cherry picked from commit 5f3f07397a7909e8f9449d1aa0b465159cbf0d60) sys/netpfil/pf/pf.c | 21 +++++++++++---------- tests/sys/netpfil/pf/icmp.sh | 4 +++- tests/sys/netpfil/pf/icmp6.sh | 4 +++- 3 files changed, 17 insertions(+), 12 deletions(-)
A commit in branch releng/13.4 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=7d3a0370c8a3dadad0739ed88fc26536649119c5 commit 7d3a0370c8a3dadad0739ed88fc26536649119c5 Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2024-08-12 14:07:35 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2024-08-21 07:44:01 +0000 pf: fix icmp-in-icmp state lookup In 534ee17e6 pf state checking for ICMP(v6) was made stricter. This change failed to correctly set the pf_pdesc for ICMP-in-ICMP lookups, resulting in ICMP error packets potentially being dropped incorrectly. Specially, it copied the ICMP header into a separate variable, not into the pf_pdesc. Populate the required pf_pdesc fields for the embedded ICMP packet's state lookup. PR: 280701 Approved by: re (cperciva) MFC after: 1 week Sponsored by: Rubicon Communications, LLC ("Netgate") (cherry picked from commit 2da98eef1f352c496ffd458b4c68ddee972bb903) (cherry picked from commit 0d8d4cc3ea47f1ee61d749b22b135eb73c7d33cd) sys/netpfil/pf/pf.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-)
According to multiple users the ICMP patch series causes stalls in neighbor discovery and only a full revert brings back the desired behaviour. A TCP dump showed that the Cisco is sending ICMP6 neighbour solicitations, which are answered by the opnsense with a large delay. The cisco switch looses it's IPv6 neighbour. tcpdump -n -i ix0 icmp6 and host fe80::86b8:2ff:fe1a:c67f 07:34:42.764553 IP6 fe80::86b8:2ff:fe1a:c67f > 2001:xxxx:x:x::x:2: ICMP6, neighbor solicitation, who has 2001:xxxx:x:x::x:2, length 32 07:34:43.852542 IP6 fe80::86b8:2ff:fe1a:c67f > 2001:xxxx:x:x::x:2: ICMP6, neighbor solicitation, who has 2001:xxxx:x:x::x:2, length 32 07:34:44.940525 IP6 fe80::86b8:2ff:fe1a:c67f > 2001:xxxx:x:x::x:2: ICMP6, neighbor solicitation, who has 2001:xxxx:x:x::x:2, length 32 07:34:46.094207 IP6 fe80::86b8:2ff:fe1a:c67f > ff02::1:ff56:2: ICMP6, neighbor solicitation, who has 2001:xxxx:x:x::x:2, length 32 07:34:47.120778 IP6 fe80::86b8:2ff:fe1a:c67f > ff02::1:ff56:2: ICMP6, neighbor solicitation, who has 2001:xxxx:x:x::x:2, length 32 07:34:48.201460 IP6 fe80::86b8:2ff:fe1a:c67f > ff02::1:ff56:2: ICMP6, neighbor solicitation, who has 2001:xxxx:x:x::x:2, length 32 07:34:49.336747 IP6 fe80::86b8:2ff:fe1a:c67f > ff02::1:ff56:2: ICMP6, neighbor solicitation, who has 2001:xxxx:x:x::x:2, length 32 07:34:50.360952 IP6 fe80::86b8:2ff:fe1a:c67f > ff02::1:ff56:2: ICMP6, neighbor solicitation, who has 2001:xxxx:x:x::x:2, length 32 07:34:51.385618 IP6 fe80::86b8:2ff:fe1a:c67f > ff02::1:ff56:2: ICMP6, neighbor solicitation, who has 2001:xxxx:x:x::x:2, length 32 07:34:52.436467 IP6 fe80::86b8:2ff:fe1a:c67f > ff02::1:ff56:2: ICMP6, neighbor solicitation, who has 2001:xxxx:x:x::x:2, length 32 07:34:53.529962 IP6 fe80::86b8:2ff:fe1a:c67f > ff02::1:ff56:2: ICMP6, neighbor solicitation, who has 2001:xxxx:x:x::x:2, length 32 07:34:54.617082 IP6 fe80::86b8:2ff:fe1a:c67f > ff02::1:ff56:2: ICMP6, neighbor solicitation, who has 2001:xxxx:x:x::x:2, length 32 07:34:55.717592 IP6 fe80::86b8:2ff:fe1a:c67f > ff02::1:ff56:2: ICMP6, neighbor solicitation, who has 2001:xxxx:x:x::x:2, length 32 07:34:56.765964 IP6 fe80::86b8:2ff:fe1a:c67f > ff02::1:ff56:2: ICMP6, neighbor solicitation, who has 2001:xxxx:x:x::x:2, length 32 07:34:57.796680 IP6 fe80::86b8:2ff:fe1a:c67f > ff02::1:ff56:2: ICMP6, neighbor solicitation, who has 2001:xxxx:x:x::x:2, length 32 07:34:58.888994 IP6 fe80::86b8:2ff:fe1a:c67f > ff02::1:ff56:2: ICMP6, neighbor solicitation, who has 2001:xxxx:x:x::x:2, length 32 07:34:58.889051 IP6 fe80::3eec:efff:fe70:7326 > fe80::86b8:2ff:fe1a:c67f: ICMP6, neighbor advertisement, tgt is 2001:xxxx:x:x::x:2, length 32 via: https://github.com/opnsense/src/issues/217
Another user notes via https://github.com/opnsense/core/issues/7804 that the following counters seem to be rising while NDs are ignored: # pfctl -vvsInterfaces | grep -e '^[a-z]' -e Out6/Block LAN/WAN on my end with the current patching applied: igb0 Out6/Block: [ Packets: 28 Bytes: 2032 ] igb1 Out6/Block: [ Packets: 674 Bytes: 54907 ] I'm going to retest with a full revert to see how these counters differ in this particular environment here.
There is no significant change with the reverted changes: igb0 Out6/Block: [ Packets: 11 Bytes: 896 ] igb1 Out6/Block: [ Packets: 286 Bytes: 19938 ] The only clue that it's pf is that "pfctl -d" brings it back to normal on the patched versions as mentioned earlier.
This seems fixed in FreeBSD. I'll pick up the patches that landed in the stable/X branches and work on a revised advisory. Not sure what to make of the opnsense counters, but I suspect that's a downstream issue. Kristof: okay to mark this one as closed/fixed?
(In reply to Philip Paeps from comment #34) > Kristof: okay to mark this one as closed/fixed? Yes, I'm not aware of any remaining issues.
Just wanted to note that I see the delayed ND answers and rising counters as well on OpnSense.
I suspect it's a behavioural change in ICMPv6 state handling introduced WRT the ND discard observed which is not overly practical in production, downstream-related or not. I don't think closing this now is the greatest idea given that the test coverage didn't account for any this to begin with. Cheers, Franco
What concrete evidence do you have that the neighbour discovery behaviour you are observing on opnsense is related to this regression on FreeBSD? Counters are not helpful here. Please submit a test case that reproduces the problem you are seeing on FreeBSD and run it on FreeBSD pre-SA-24:05 and on FreeBSD post-SA-24:05+corrections. Unless the test case reproduces the problem on FreeBSD, it should not hold up FreeBSD publishing a corrected advisory.
The evidence is the original SA patch series which spans hundreds of lines of code changes and a lack of actual test coverage. The lack of benefit of doubt is strange in my opinion. I can revert only these patches and the problem disappears. Do you want to know which exact commit is responsible? I can offer you this information. The further evidence is that pfctl -d fixes missing ND responses immediately on affected systems. You wouldn't see these issues unless you used pf heavily coupled with IPv6 connectivity. These things are not prevalent in FreeBSD users, but they will certainly manifest in pfSense quite soon as well. I can see that with other patch submissions I have done over the past few weeks for FreeBSD 14.1 none of which have been wilfully been looked at by the relevant authors of bugs in FreeBSD 14.0 and 14.1. I guess we can continue this hide and seek, but I would rather have it that we work together to fix issues within FreeBSD production releases together? Cheers, Franco
Ok, so... let's recap this: What original SA deals with - let me quote: "When pf is configured to allow ND and block incoming Echo Requests, a crafted Echo Request packet after a Neighbor Solicitation (NS) can trigger an Echo Reply." This is "fixed" by a series of patches which cause the regression described here, among others. Even after fixing the regression by another series of patches, people still report that this caused yet more regressions, which directly match the area touched by the original "issue" described in the SA. That is, breaks ND/NS (see Comment #31 and following). Additionally, people report that ONLY reverting all the patches to the state before this "pressing" "security" issue of responding to ping - unnoticed by anyone from 2009 at least, let alone exploited (for what exactly?) - gets things back to working state without regressions. The response here - downstream issue, lets close it. Between the breakage caused here and responding to pings, it's everyone's guess what users prefer. The original "security" "issue" has caused zero problems for 15+ years. Something's not responding to pings - yeah, there's a box with a firewall in place blocking ping. If there was no computer with given address connected, the evil attacker crafting the packets as per the SA would get ICMP Destination Unreachable (ICMP Type 3) with one of the codes (such as 0 - net unreachable, 1 - host unreachable ... etc). Blocking pings actually confirms the computer is there, up and running. The only thing blocking responses to ping does - make basic networking diagnostics / troubleshooting a PITA. How's this whole thing a security issue deserving an SA and urgent patching causing the above regressions which are impacting real network operation and many users, goes beyond me, sorry. Once upon a time, common sense was in used, as documented by http://www.faqs.org/rfcs/rfc1122.html - 3.2.2.6. Back to the drawing board. Sigh. Have a nice day.
Problems with how FreeBSD code behaves when merged into a downstream product are beyond the scope of this bug tracker. As far as FreeBSD is concerned, an issue is resolved when it no longer manifests on FreeBSD. It is up to our downstreams to ensure that they do not introduce bugs in their products when merging code from FreeBSD. Unless a test case materialises that shows there is still a regression on FreeBSD, we can proceed with a corrected security advisory for FreeBSD SA-24:05.pf. For what it's worth: FreeBSD.org firewalls run pf. Can you quantify the "heavily coupled with IPv6 connectivity" required for the issue to manifest? Again: is there any evidence that this problem still manifests on FreeBSD?
Sigh, Franco, would a plain vanilla FreeBSD kernel like FreeBSD post-SA-24:05+corrections underneath OpnSense be feasible? If the ND problems persisted with that kernel (and I am sure they do, because the problems go away when the "security" patch has been fully reverted (this has been established before)), maybe the maintainers realize that the band-aid fixes only part of the problem.
> Again: is there any evidence that this problem still manifests on FreeBSD? Is there any evidence it wouldn't given a single FreeBSD commit? I think what you are implying is that someone else should do that work, paid or unpaid, to get to the bottom of this either with a test case, regression test improvement or an actual code fix. From the last year alone I can tell you that none of these things which can cost a lot of time and effort and eventually money are well received by FreeBSD with patches and bug reports being ignored for exactly the reasoning that you are going with here. I've actually talked to release engineering about this and the response that I got I will quote here for effect: > FreeBSD is a volunteer project. If you don't like what you get, contribute. This was actually while contributing. You know what happened since then? Nobody cared to even look at the patches and my concerns for things broken in FreeBSD 14.0 and 14.1 so much so that I went from Phabricator to GitHub: https://github.com/freebsd/freebsd-src/pull/1390 https://github.com/freebsd/freebsd-src/pull/1391 For me the top of the irony is that the people directly involved in these problems either say it's not a FreeBSD issue or outright ignore reports and effort and their own patches making production FreeBSD releases less good than they could be. This is a tangent that ties back right to the handling of this report and what you actually expect of FreeBSD users when eventually nobody takes user reports seriously or bothers to merge their own patches to stable/14. releng/14.1 breakage is really just the icing on this particular cake. You want involvement? Please, do. I have absolutely no reason to distrust OPNsense users on their reports and positive feedback on the fixes that I have submitted so far. The same goes for bug reports I'm trying to help with without doing work that is going to be shrugged off anyway. Please fix your policies. Cheers, Franco
(In reply to Franco Fichtner from comment #43) Franco, from our testing and practical experience, we are not seeing this issue manifest itself in the stock FreeBSD kernel once the fixes are applied. All of the continued reports appear to only affect OpnSense kernels. If there is a reliable report this is continuing to be a problem with stock FreeBSD kernels, we would be happy to look at it. Unfortunately, the evidence is "this is a problem with OpnSense." As Dr. Uwe Meyer-Gruhl states in comment #42, if this can be reproduced with a plain vanilla FreeBSD kernel, we would love the opportunity to see the details to ferret out the issues. If such testing does occur, please let us know and we'll be happy to reopen this issue.
> we are not seeing this issue manifest itself in the stock FreeBSD kernel once the fixes are applied I appreciate the whole of FreeBSD insiders sticking together on this. Though I'd like to verify what you said: Is this a statement based on observation main, stable/14, releng/14.1, releng/13.3? One, all? And are you talking about traceroute not working as initially suggested or neighbor discoveries being ignored intermittently specifically as found out later? Or both? I agree that traceroute seems fixed. This isn't in dispute. The evidence for the neighbor discovery suggests otherwise as we tested each commit in the original SA in an controlled environment that has no other changes at all. This is specifically with code from releng/14.1 although I don't see how a commit within the scope of any applicable FreeBSD branch (or downstream prjects) coupled with a relevant user side ruleset for pf would not be affected in this case. I'm reading hereby FreeBSD doesn't see a neighbor discovery problem. Whether or not this is because it all works as expected is covered by test cases or purely by evidence with existing machines by developers is left to be guessed. I'm seeing intermittent IPv6 connectivity drops as well now. We have daily user reports regarding this now. It's hard to pin it down which is likely where the boldness in believing this doesn't apply to FreeBSD comes from. Fine, I understand why this message is being put out. I'm refraining from posting more links to our crowdsourced test methods for lack of enthusiasm from this end in the meantime and report back when we have proper evidence. I just don't want anyone to be surprised after the fact. Cheers, Franco
Ok here we go: https://cgit.freebsd.org/src/commit/?id=534ee17e61 This first SA commit adds state tracking to ND_NEIGHBOR_SOLICIT/ND_NEIGHBOR_ADVERT that wasn't there before. From packet captures you can see solicit being unanswered for a while with that commit applied (or all other SA related commits). As a stopgap I disabled state tracking via: https://github.com/opnsense/src/commit/ee7b012c54 This brings the solicit/advertise back to the state before the SA was introduced. All solicits are immediately answered. No solicits are repeated by the external router. These are to relevant commits from OpenBSD regarding the matter https://github.com/openbsd/src/commit/2633ae8c4c8a https://github.com/openbsd/src/commit/49f39043a02d You can see that the second commit also disables state tracking for solicit messages like the stopgap patch. Since solicit is the one that is not being answered by a system running the SA I am fairly certain that this is the same problem scope. Anyone got a thought why this could not be relevant to FreeBSD? Cheers, Franco
Also why was this excluded during the port from OpenBSD? Same for MLD_LISTENER_* BTW. https://github.com/openbsd/src/blob/master/sys/net/pf.c#L2699-L2704
Franco, thanks for the additional detail! I'm reopening to get further eyes on the issue based on the pointers provided. kp, does the analysis in comment 46 indicate an issue that needs further review?
(In reply to Gordon Tetlow from comment #48) > kp, does the analysis in comment 46 indicate an issue that needs further review? What issue? There's been a lot of conspiracy theorising, but no actual bug report beyond "opnsense is broken". I genuinely don't know what's supposed to be broken.
I don't know why this keeps happening, Kristof. If you don't think it's worth investigating please don't shrug it off in the name of external entities / bug reporters. If you don't want bug reports you can say that, too. Cheers, Franco
(In reply to Kristof Provost from comment #49) Sir, as I already hinted in Comment #11 - your port of code from 2009 is *incomplete* and buggy. ND states behaviour is broken. Many people took their time to pinpoint the missing bits. You are wasting everyone's time here and causing harm to FreeBSD users and the project. Sigh.
If you do not understand and / or believe what is left broken, read the reports of how ND fails even after applying the patches contained here. If you want to construct a test setup to cover this, try directing the following command from another machine to a potentially affected FreeBSD machine and look at the results: while : do ndisc6 -m -n -r 1 fe80::1111:2222:3333:4444 eth0 done Of course, fill in the target's EUI-64 instead of 1111:2222:3333:4444 and use the correct interface instead of eth0. You will find that even after the current commits, a machine with the SA applied does not always respond in due time to these requests and the requests time out, whereas a machine without the SA always answers correctly.
And since it already got to this point, @Kristof - perhaps reviewing Section 13 of the Committer's Guide [1] would benefit you, others contributors and - first of all - FreeBSD. (Seems last updated in 2022, so I assume that is still applicable to the FreeBSD project.) The one bit that comes to mind: "Argue your position from its merits." What you are doing here (and what's going on repeatedly elsewhere) is not based on merits but on your personal dislike and perhaps conflict of interests with another downstream project based on FreeBSD. [1] https://docs.freebsd.org/en/articles/committers-guide/
(In reply to Dr. Uwe Meyer-Gruhl from comment #52) Oh hey, an actionable bit of information! That's nice. I'll try that on Monday.
(In reply to Kristof Provost from comment #54) Yes, that's the same information what's been posted in Comment #31. That's also something that should be covered by the testcases but clearly is not. I am very sure the code and other contributions by Netgate are very welcome by FreeBSD and its users. Also, I am very sure that your attitude towards developers of OPNsense and the user base of that project is appreciated by noone but Netgate. This needs to end. Noone here cares about your historical disagreements. People want broken code fixed and working for everyone, vanilla FreeBSD or downstream projects that use that code. (Which includes your upcoming pfSense Plus release, BTW.) Yet, apparently this hostility is still going on.
Created attachment 253088 [details] Packet loss / RTT times (Source: https://github.com/opnsense/src/issues/218) Perhaps pics work better than words. I am adding one documenting how badly broken the IPv6 experience is with this faulty series of patches applied for some users and what's the scope of the regressions. I believe this is not the experience you want to force on FreeBSD users and blame downstream for that. The difference between broken and working states in those pictures? Well, it's already been mentioned in Comment #46. Hopefully these reports will be taken more seriously in the future. https://github.com/opnsense/src/issues/218
In closing I'd like to add a few things. It was made known that a proper bug report and steps to reproduce should be raised. I think that's only fair. This, however, requires the undesired behaviour to follow established rules which do not readily apply here. The core of the problem as we as OPNsense see it is that pf state tracking was added to several ICMPv6 types that were not there before -- first and foremost ND_NEIGHBOR_SOLICIT/ND_NEIGHBOR_ADVERT. It can be said that the state tracking is insufficient now in at least these two types of ICMPv6 communication which results in intermittent package drops. This also results in easily visible ping drops as neighbour discovery fails intermittently. The full scope of this change is highly speculative and it has been hinted at in OPNsense and OpenBSD that further issues exist with other ICMPv6 types contained within this change. The way forward in FreeBSD releases now should be treated with the appropriate amount of foresight. If you want to ask for easily reproducible steps please also ask how easily reliable tests could have been added for this. Testing all of this is very difficult as I'm sure we all know now. I think we are all here to help avoid and remedy problems together. It's not my place to question why adding state tracking to pf/ICMPv6 was a good idea to everyone involved in bringing this to all FreeBSD releases immediately so far. Someone should ask that question internally probably. A better target for this would be FreeBSD 15.0 in my very humble opinion. It would be beneficial now to have a real IPv6 expert inspect these state tracking attempts because I think so far that hasn't happened. OPNsense does a lot of IPv6 and does it quite well, but we are in now way experts. My first reaction to seeing the ICMP patches on stable/14 was to ignore them, but that was made impossible by pushing them to SA state in the way they were. Also to remind everyone what downstream does: we are trying to run projects based on FreeBSD and we mostly build integration for other software. Ideally we do this on unmodified FreeBSD. Yet upstreaming patches is increasingly difficult and hostile. Our kernels only diverge because of: (1) Too strict errata policy on FreeBSD releases, and (2) upstreaming patches and stable turnaround times are too long. This causes friction with committers because they don't trust us or our capabilities or reports and think things like kernel patches are our own problems. All of it only leads to more divergence. I think that should be said here once for emphasis. And as a personal matter we should stop with the idea of "conspiracy theorising" and "downstream is broken". This will not advance FreeBSD in the way that it should. As far as this discussion goes I think FreeBSD has all the information that it needs to progress this. As downstream we certainly will make a move based on what we found out so far, too. Good luck. Cheers, Franco
I found these inconsistencies in the ported patches from OpenBSD: diff --git a/sys/netpfil/pf/pf.c b/sys/netpfil/pf/pf.c index ef488bad26d..c9180e877d5 100644 --- a/sys/netpfil/pf/pf.c +++ b/sys/netpfil/pf/pf.c @@ -1878,7 +1878,7 @@ pf_icmp_mapping(struct pf_pdesc *pd, u_int8_t type, */ *icmp_dir = PF_IN; *virtual_type = MLD_LISTENER_QUERY; - *virtual_id = 0; + *virtual_id = 0; /* XXX missing fake id */ break; } case MLD_MTRACE: @@ -1892,7 +1892,8 @@ pf_icmp_mapping(struct pf_pdesc *pd, u_int8_t type, *icmp_dir = PF_IN; case ND_NEIGHBOR_ADVERT: { *virtual_type = ND_NEIGHBOR_SOLICIT; - *virtual_id = 0; + *multi = PF_ICMP_MULTI_SOLICITED; + *virtual_id = 0; /* XXX missing fake id */ break; } From early testing, however, it's not working any better with plugging back PF_ICMP_MULTI_SOLICITED only. I'm unsure about the mock id effect. But what I can say is that it's better to skip dealing with PF_ICMP_MULTI_SOLICITED as OpenBSD did in 2012 too: https://github.com/openbsd/src/commit/2633ae8c4c8a64 Another patch from 2023 is relevant as well as it disables half the state tracking for unsolicited advertise cases: https://github.com/openbsd/src/commit/49f39043a02d6
Should this fix also be applied to releng/13.3 branch?
(In reply to Natalino Picone from comment #59) Do you mean the patch posted in comment #58? You can apply that patch to whichever branch you wish, however it will not fix the regressions, as noted in the same comment. For regression-free experience, the solution is reverting to the state before the FreeBSD-SA-24:05 patch.
(In reply to doktornotor from comment #60) Sorry for the missing details. This is a very long thread, and it's unclear whether an official FreeBSD patch is now in the base or not to fix the issues caused by FreeBSD-SA-24:05. I'm focused on the releng/13.3 branch. FreeBSD-SA-24:05 was released at the beginning of August, so it's almost one month since a security patch (which usually should not be ignored) broke the standard FreeBSD base. Thanks
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=5ab1e5f7e5585558a73b723f07528977a82cee82 commit 5ab1e5f7e5585558a73b723f07528977a82cee82 Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2024-08-26 12:59:38 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2024-09-01 15:05:28 +0000 pf: improve the ICMPv6 direction check Following bluhm's advice this changes the way we setup state keys and perform state lookups for ICMPv6 Neighbor Discovery packets: - replace the NS-dst with ND target address; - replace the NA-src with ND target address; - replace the NA-dst with unspecified address if it is a multicast. This allows pf to match Address Resolution, Neighbor Unreachability Detection and Duplicate Address Detection packets to the corresponding states without the need to create new ones or match unrelated ones. As a side effect we're doing now one state table lookup for ND packets instead of two. Fixes a bug uncovered by one of the previous commits that virtually breaks IPv6 connectivity after few minutes of use. ok stsp henning, with and ok bluhm PR: 280701 MFC after: 1 week Obtained from: OpenBSD, mikeb <mikeb@openbsd.org>, 2633ae8c4c8a Sponsored by: Rubicon Communications, LLC ("Netgate") sys/net/pfvar.h | 4 +- sys/netpfil/pf/pf.c | 116 ++++++++++++++++++++++++++++++++++--------------- sys/netpfil/pf/pf_lb.c | 2 +- 3 files changed, 85 insertions(+), 37 deletions(-)
Thanks. As a possible workaround for affected systems can be an additional rule like this: pass in quick inet6 proto icmp6 no state If you are not afraid of icmp echo.
Not sure about other people here suffering from the regressions, but I'd seriously appreciate some form of communication beyond automated commit-hook@ messages. On another note, perhaps start with comparing the current OpenBSD vs. FreeBSD pf.c & co. code and go on from there, instead of playing catch me if you can games and hunting for missed commits for the past 15+ years would be beneficial to everyone involved and FreeBSD as a whole. Also, setting some realistic target reflecting the "severity" of the (alleged} security issue - as opposed to dumping these experiments on people all the way back to FreeBSD 13.x. And finally, there appears to be a bunch of related commits on -main not linked here. I'd say people here would be willing to test whether those make things better, worse or whatever, but if you expect them hunting for commits, well, good luck with this communication strategy. Sigh.
In reply to doktornotor from comment #64: > Not sure about other people here suffering from the regressions, but I'd > seriously appreciate some form of communication beyond automated > commit-hook@ messages. While I can't address the rest of this comment, in regards to more communication, secteam will publish an erratum for this issue shortly. We are letting it sit for a hot minute to ensure we don't have (additional) breakages. As you can imagine, the last thing we want to do is issue an erratum for an erratum. There was also a question earlier from comment #40: > How's this whole thing a security issue deserving an SA and urgent patching > causing the above regressions which are impacting real network operation and > many users, goes beyond me, sorry. As to the question of why the original "fix" was treated as a security advisory – The issue was originally brought to us by an external researcher (as credited in the SA-24:05.pf write up) as a security issue, so there was an anchoring bias. secteam did have a debate internally as to whether it should be a security advisory or an erratum. Ultimately, we decided to call it an SA due to the fact that security software on the system was behaving in an unexpected way and allowing things through that it shouldn't have. I, with my security-officer hat on, stand by this decision and would likely make the same one given the same facts today.
(In reply to Gordon Tetlow from comment #65) I'm afraid the point was sort of missed here. What I meant by "more communication" is that whoever committed the buggy code should not maintain radio silence here. We do not exactly need an erratum to tell us it was broken, we already know that. What we do *not* know is whether those regressions are fixed - and worse yet, we do not know what are we supposed to test. "Letting it sit for a hot minute" won't get any testing done.
There are some open release engineering questions in this thread, lack of professionalism discarding a problem that was later fixed without comment aside. Doing the least bit of rectifying the previous behaviour would be a good start to a useful discussion on the subject. The first and foremost question is how this was tested and verified? Was the researcher involved in all steps? The commits don't have a "Reviewed by" or "Tested by" either. Is this normal now? Does release engineering not assess the risk of spreading an SA fix over 4 commits with about 500 LOC changed introducing new features while at it? That then grew to 6 commits, with 10 commits at the moment. It's a classic scope creep that should be avoided on releases at all cost. The test coverage wasn't there to make an educated choice either. Why is the fake id portion of the original OpenBSD patch omitted? At least https://github.com/openbsd/src/commit/49f39043a02d is still missing. Can anyone comment on why one would think that we should try to get away with the least bit of commits here when we can clearly see all the related problems were seen and handled in OpenBSD in the meantime? Why does nobody ask the reporters here to test this again? Why are the insights given by reporters brushed off? You can clearly see where the problem started given that nobody cares answering these questions. TLDR: SO should do this again, please, but RE shouldn't. Cheers, Franco
Franco, as an outside observer of this discussion, I'm a little confused. Who, exactly, do you think you are in relation to Gordon and Kristof and the other people actually doing the work here, and why do you think your tone and behavior are appropriate in this situation? Same goes for doktornotor btw. As FreeBSD users or integrators, one would think you'd want to be part of the solution, but you seen to be doing everything in your power to be part of the problem instead. What's up with that?
(In reply to Dag-Erling Smørgrav from comment #68) Thank you for participating. You may want to read comment 57 first.
I am only speaking for me, but from a "downstream user" perspective and I do not want to sound disrespectful. I acknowledge and appreciate the hard work that has been put into FreeBSD. However, when the first problem with this specific SA was raised and test cases have been provided, a band-aid was tried, which did not fix all the problems the SA had created. Again this was reported but quickly dismissed as "downstream problem" - which, AFAIK was not the first time to happen. Another band-aid was done, which reportedly still does not contain all of the fixes than OpenBSD had done in the years before. Discussing why this SA with that far of a reach was applied anyway is spilled milk (tm), but there is always a tradeoff between security and useability. If that SA really seemed so important, it should have been handled with more care from the beginning. In both of these cases, the fixes were not discussed here (only automatic hints for other patches could be seen), test coverage seems barely sufficient and there was no comeback to us reporters to re-test anything. So, as far as communication goes, this is by far the worst I have seen so far. There would two ways to solve this: 1. Tell us here what has been done so far and communicate to enable us to re-test specific bugs or 2. Point us to the "leading" bug report where the impact of the SA fixes are reported / handled and close this bug. Randomly changing code behind the scenes and expecting us to follow along is not the right way, IMHO. While it is true that the bugs caused by the SA may affect "normal" FreeBSD users less than those who use one of the downstream router platforms, networking is a core requirement for any decent OS anyway. I do not know what proportion of all FreeBSD users uses those router platforms, but they are surely affected by the bugs. Thus, dismissing those problems as "downstream" is inadequate from any perspective. Of course, FreeBSD "owes" downstream nothing, but ironically, this SA and the bugs first and foremost affect the pf subsystem. pf is the most given reason not to migrate said router platforms to Linux, simply because Linux does not have it. From my point of view, pf is one of the crown jewels of FreeBSD - as opposed to, say, driver coverage. If that were not so, a migration like from TrueNAS Core to TrueNAS Scale might have happened already for the router platforms. So my take would be the FreeBSD project to consider if better coverage of exactly the pf / networking subsystem and taking reports of the downstream projects that actually use them, should be taken more seriously.
A commit in branch stable/14 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=0121a4baaca09049d130d830aa9179e3cb9c9e88 commit 0121a4baaca09049d130d830aa9179e3cb9c9e88 Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2024-08-26 12:59:38 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2024-09-04 08:38:15 +0000 pf: improve the ICMPv6 direction check Following bluhm's advice this changes the way we setup state keys and perform state lookups for ICMPv6 Neighbor Discovery packets: - replace the NS-dst with ND target address; - replace the NA-src with ND target address; - replace the NA-dst with unspecified address if it is a multicast. This allows pf to match Address Resolution, Neighbor Unreachability Detection and Duplicate Address Detection packets to the corresponding states without the need to create new ones or match unrelated ones. As a side effect we're doing now one state table lookup for ND packets instead of two. Fixes a bug uncovered by one of the previous commits that virtually breaks IPv6 connectivity after few minutes of use. ok stsp henning, with and ok bluhm PR: 280701 MFC after: 1 week Obtained from: OpenBSD, mikeb <mikeb@openbsd.org>, 2633ae8c4c8a Sponsored by: Rubicon Communications, LLC ("Netgate") (cherry picked from commit 5ab1e5f7e5585558a73b723f07528977a82cee82) sys/net/pfvar.h | 4 +- sys/netpfil/pf/pf.c | 116 ++++++++++++++++++++++++++++++++++--------------- sys/netpfil/pf/pf_lb.c | 2 +- 3 files changed, 85 insertions(+), 37 deletions(-)
A commit in branch stable/13 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=b84344206721ed2803d5da68585289d5880efe3f commit b84344206721ed2803d5da68585289d5880efe3f Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2024-08-26 12:59:38 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2024-09-04 08:53:06 +0000 pf: improve the ICMPv6 direction check Following bluhm's advice this changes the way we setup state keys and perform state lookups for ICMPv6 Neighbor Discovery packets: - replace the NS-dst with ND target address; - replace the NA-src with ND target address; - replace the NA-dst with unspecified address if it is a multicast. This allows pf to match Address Resolution, Neighbor Unreachability Detection and Duplicate Address Detection packets to the corresponding states without the need to create new ones or match unrelated ones. As a side effect we're doing now one state table lookup for ND packets instead of two. Fixes a bug uncovered by one of the previous commits that virtually breaks IPv6 connectivity after few minutes of use. ok stsp henning, with and ok bluhm PR: 280701 MFC after: 1 week Obtained from: OpenBSD, mikeb <mikeb@openbsd.org>, 2633ae8c4c8a Sponsored by: Rubicon Communications, LLC ("Netgate") (cherry picked from commit 5ab1e5f7e5585558a73b723f07528977a82cee82) sys/net/pfvar.h | 2 +- sys/netpfil/pf/pf.c | 116 ++++++++++++++++++++++++++++++++++--------------- sys/netpfil/pf/pf_lb.c | 2 +- 3 files changed, 84 insertions(+), 36 deletions(-)
Franco, you may think comment 57 makes you look good, but I assure you that it does not.
(In reply to Dr. Uwe Meyer-Gruhl from comment #70) MFC after: 1 week gets merged in 3 days to stable, all concerns here ignored. Just great. > So, as far as communication goes, this is by far the worst I have seen so far. And apparently not improving. :-X
(In reply to Dag-Erling Smørgrav from comment #73) Dag, you may think this SA makes FreeBSD look good, but I assure you that it does not. Dunking on reporters while involved individuals are sharing nothing but their view on "conspiracies" and then silently doing what others suggested anyway seems to be a recurring theme that needs to stop. I also don't need anyone to value my work and dedication so no worries. Need something done? How about ask? Like testing the latest commits before they hit stable. ;) The handling of this SA's regressions could have been very smooth given that every technical aspect and missing upstream commits has been provided in advance so far despite all the efforts to brush this off instead of working with the reporters to fix this in the first place. Cheers, Franco
(In reply to Dag-Erling Smørgrav from comment #68) I don't know, maybe I just need your glasses to see things the way you do, but if you're okay with FreeBSD's communication in this PR, I see it the other way around. This PR is an absolutely horrible example of discouraging normal users from reporting anything. It gives me a pretty bitter taste in my mouth, and I'm in no way associated with any downstream project, even as a user. I just have pure FreeBSD everywhere.
FreeBSD Core team is looking at this issue. The parties in the dispute have a long story of heated conversation, so it will take a few days to come up with a balanced resolution. I ask everybody to stop piling up more emotions or any kind of other non-technical responses to this bug.
(In reply to Gleb Smirnoff from comment #77) Well, until that balanced resolution is reached, here's some reading for the involved maintainer, as well as for amusement of other FreeBSD users affected by the related regressions. Don't despair - there's still a long time to beat *the* legend. :-P https://bugzilla.redhat.com/show_bug.cgi?id=119185 https://web.archive.org/web/20061223222514/http://www.kuro5hin.org/story/2006/6/5/101431/9311 Thanks, and enjoy! ;-)
A commit in branch releng/13.4 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=d3ee2188686dce00083ba382c1a773d4e293b242 commit d3ee2188686dce00083ba382c1a773d4e293b242 Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2024-08-26 12:59:38 +0000 Commit: Kristof Provost <kp@FreeBSD.org> CommitDate: 2024-09-05 07:34:26 +0000 pf: improve the ICMPv6 direction check Following bluhm's advice this changes the way we setup state keys and perform state lookups for ICMPv6 Neighbor Discovery packets: - replace the NS-dst with ND target address; - replace the NA-src with ND target address; - replace the NA-dst with unspecified address if it is a multicast. This allows pf to match Address Resolution, Neighbor Unreachability Detection and Duplicate Address Detection packets to the corresponding states without the need to create new ones or match unrelated ones. As a side effect we're doing now one state table lookup for ND packets instead of two. Fixes a bug uncovered by one of the previous commits that virtually breaks IPv6 connectivity after few minutes of use. ok stsp henning, with and ok bluhm PR: 280701 MFC after: 1 week Obtained from: OpenBSD, mikeb <mikeb@openbsd.org>, 2633ae8c4c8a Sponsored by: Rubicon Communications, LLC ("Netgate") (cherry picked from commit 5ab1e5f7e5585558a73b723f07528977a82cee82) (cherry picked from commit b84344206721ed2803d5da68585289d5880efe3f) Approved-by: re (cperciva) sys/net/pfvar.h | 2 +- sys/netpfil/pf/pf.c | 116 ++++++++++++++++++++++++++++++++++--------------- sys/netpfil/pf/pf_lb.c | 2 +- 3 files changed, 84 insertions(+), 36 deletions(-)
As for the technical input: here is another *downstream* issue [1] with pf debug log (i.e., set debug misc) getting flooded (300K+/day) with > pf: ICMP error message too short (ip6) from ND (NS/NA) packets. That *downstream* issue also surprisingly goes away [2] when reverting [3] *all* those *upstream* patches related to FreeBSD-SA-24:05. Hmmmm... [1] https://github.com/opnsense/core/issues/7840 [2] https://forum.opnsense.org/index.php?topic=42632.msg211600#msg211600 [3] https://github.com/opnsense/src/commit/164bfe67604
The first and original regression reported with this bug has been resolved and fixed. The original bug report > replies to ping initiated from machines on networks behind pf firewall/NAT > to anything outside the local networks get blocked (state violation), > resulting in 'Request timed out'. was fixed by 2da98eef1f35, 46c4fc50d301. However, there are reports of more regressions remaining in the OPNsense fork, that were not confirmed on FreeBSD, yet. With the official policy of one bug per bug report the FreeBSD core team is forcing this bug into closed state. For any other bugs related or not to SA-24:05 we request submitters to create separate bug reports. We also recommend to provide at least reproduce recipes or at most atf(7) automated test cases. That would speed up bug resolving immensely. Finally, I'd like to remind that the project code of conduct applies not only to the developers with a commit access, but to all participants in any discussion in the project space: https://www.freebsd.org/internal/code-of-conduct/
Thanks for the response! Based on this particular resolution in Comment 81, OPNsense will back out the SA in full effective with the next stable release 24.7.4 and we wish all involved parties the best on their way forward. Respectfully, Franco
Just for reference: I just created bugs https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=281395 and https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=281397.
(In reply to Gleb Smirnoff from comment #81) > Finally, I'd like to remind that the project code of conduct applies not only > to the developers with a commit access, but to all participants in any > discussion in the project space: > https://www.freebsd.org/internal/code-of-conduct/ That you for the reference and reminder. To avoid future clashes with your CoC, I will do my best to refrain from reporting bugs in any code committed / sponsored by Rubicon Communications, LLC ("Netgate"). Hopefully, that will make FreeBSD even better. Have a nice day.
A commit in branch releng/14.1 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=84b57a4c5b848d44ec0918c28d8c27bec948a151 commit 84b57a4c5b848d44ec0918c28d8c27bec948a151 Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2024-08-14 09:29:30 +0000 Commit: Mark Johnston <markj@FreeBSD.org> CommitDate: 2024-09-19 12:55:09 +0000 pf: invert direction for inner icmp state lookups (e.g. traceroute with icmp) ok henning, jsing Also extend the test case to cover this scenario. Approved by: so Security: FreeBSD-EN-24:16.pf PR: 280701 Obtained from: OpenBSD MFC after: 1 week Sponsored by: Rubicon Communications, LLC ("Netgate") (cherry picked from commit 89f6723288b0d27d3f14f93e6e83f672fa2b8aca) (cherry picked from commit 46c4fc50d3012ca3c8756df243589add36b70830) sys/netpfil/pf/pf.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)
A commit in branch releng/14.1 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=fdc0afd4391ef45b5dcba33238b37f135972d71a commit fdc0afd4391ef45b5dcba33238b37f135972d71a Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2024-08-26 12:59:38 +0000 Commit: Mark Johnston <markj@FreeBSD.org> CommitDate: 2024-09-19 12:55:21 +0000 pf: improve the ICMPv6 direction check Following bluhm's advice this changes the way we setup state keys and perform state lookups for ICMPv6 Neighbor Discovery packets: - replace the NS-dst with ND target address; - replace the NA-src with ND target address; - replace the NA-dst with unspecified address if it is a multicast. This allows pf to match Address Resolution, Neighbor Unreachability Detection and Duplicate Address Detection packets to the corresponding states without the need to create new ones or match unrelated ones. As a side effect we're doing now one state table lookup for ND packets instead of two. Fixes a bug uncovered by one of the previous commits that virtually breaks IPv6 connectivity after few minutes of use. ok stsp henning, with and ok bluhm Approved by: so Security: FreeBSD-EN-24:16.pf PR: 280701 MFC after: 1 week Obtained from: OpenBSD, mikeb <mikeb@openbsd.org>, 2633ae8c4c8a Sponsored by: Rubicon Communications, LLC ("Netgate") (cherry picked from commit 5ab1e5f7e5585558a73b723f07528977a82cee82) (cherry picked from commit 0121a4baaca09049d130d830aa9179e3cb9c9e88) sys/net/pfvar.h | 4 +- sys/netpfil/pf/pf.c | 116 ++++++++++++++++++++++++++++++++++--------------- sys/netpfil/pf/pf_lb.c | 2 +- 3 files changed, 85 insertions(+), 37 deletions(-)
A commit in branch releng/14.1 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=fb925cf0a4b38bffc4c9733bae3212f07a481931 commit fb925cf0a4b38bffc4c9733bae3212f07a481931 Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2024-08-12 14:07:35 +0000 Commit: Mark Johnston <markj@FreeBSD.org> CommitDate: 2024-09-19 12:54:47 +0000 pf: fix icmp-in-icmp state lookup In 534ee17e6 pf state checking for ICMP(v6) was made stricter. This change failed to correctly set the pf_pdesc for ICMP-in-ICMP lookups, resulting in ICMP error packets potentially being dropped incorrectly. Specially, it copied the ICMP header into a separate variable, not into the pf_pdesc. Populate the required pf_pdesc fields for the embedded ICMP packet's state lookup. Approved by: so Security: FreeBSD-EN-24:16.pf PR: 280701 MFC after: 1 week Sponsored by: Rubicon Communications, LLC ("Netgate") (cherry picked from commit 2da98eef1f352c496ffd458b4c68ddee972bb903) (cherry picked from commit 27a1a56b0d2e6ffa6ab1de69ef84fe66b7fd41e0) sys/netpfil/pf/pf.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-)
A commit in branch releng/14.0 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=2fd8437daed57e34e50beb50013910b64b456f91 commit 2fd8437daed57e34e50beb50013910b64b456f91 Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2024-08-26 12:59:38 +0000 Commit: Mark Johnston <markj@FreeBSD.org> CommitDate: 2024-09-19 12:58:25 +0000 pf: improve the ICMPv6 direction check Following bluhm's advice this changes the way we setup state keys and perform state lookups for ICMPv6 Neighbor Discovery packets: - replace the NS-dst with ND target address; - replace the NA-src with ND target address; - replace the NA-dst with unspecified address if it is a multicast. This allows pf to match Address Resolution, Neighbor Unreachability Detection and Duplicate Address Detection packets to the corresponding states without the need to create new ones or match unrelated ones. As a side effect we're doing now one state table lookup for ND packets instead of two. Fixes a bug uncovered by one of the previous commits that virtually breaks IPv6 connectivity after few minutes of use. ok stsp henning, with and ok bluhm Approved by: so Security: FreeBSD-EN-24:16.pf PR: 280701 MFC after: 1 week Obtained from: OpenBSD, mikeb <mikeb@openbsd.org>, 2633ae8c4c8a Sponsored by: Rubicon Communications, LLC ("Netgate") (cherry picked from commit 5ab1e5f7e5585558a73b723f07528977a82cee82) (cherry picked from commit 0121a4baaca09049d130d830aa9179e3cb9c9e88) sys/net/pfvar.h | 4 +- sys/netpfil/pf/pf.c | 116 ++++++++++++++++++++++++++++++++++--------------- sys/netpfil/pf/pf_lb.c | 2 +- 3 files changed, 85 insertions(+), 37 deletions(-)
A commit in branch releng/14.0 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=d1c4f6decb10c7dc826d4a3a27763dc3f531ffe5 commit d1c4f6decb10c7dc826d4a3a27763dc3f531ffe5 Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2024-08-12 14:07:35 +0000 Commit: Mark Johnston <markj@FreeBSD.org> CommitDate: 2024-09-19 12:57:46 +0000 pf: fix icmp-in-icmp state lookup In 534ee17e6 pf state checking for ICMP(v6) was made stricter. This change failed to correctly set the pf_pdesc for ICMP-in-ICMP lookups, resulting in ICMP error packets potentially being dropped incorrectly. Specially, it copied the ICMP header into a separate variable, not into the pf_pdesc. Populate the required pf_pdesc fields for the embedded ICMP packet's state lookup. Approved by: so Security: FreeBSD-EN-24:16.pf PR: 280701 MFC after: 1 week Sponsored by: Rubicon Communications, LLC ("Netgate") (cherry picked from commit 2da98eef1f352c496ffd458b4c68ddee972bb903) (cherry picked from commit 27a1a56b0d2e6ffa6ab1de69ef84fe66b7fd41e0) sys/netpfil/pf/pf.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-)
A commit in branch releng/14.0 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=9481d7a260822d20d60d582bfff20bdd754c49c5 commit 9481d7a260822d20d60d582bfff20bdd754c49c5 Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2024-08-14 09:29:30 +0000 Commit: Mark Johnston <markj@FreeBSD.org> CommitDate: 2024-09-19 12:58:15 +0000 pf: invert direction for inner icmp state lookups (e.g. traceroute with icmp) ok henning, jsing Also extend the test case to cover this scenario. Approved by: so Security: FreeBSD-EN-24:16.pf PR: 280701 Obtained from: OpenBSD MFC after: 1 week Sponsored by: Rubicon Communications, LLC ("Netgate") (cherry picked from commit 89f6723288b0d27d3f14f93e6e83f672fa2b8aca) (cherry picked from commit 46c4fc50d3012ca3c8756df243589add36b70830) sys/netpfil/pf/pf.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)
A commit in branch releng/13.3 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=a16cd5ff50ea2e5954958d90ef6de18e0f1aa4bc commit a16cd5ff50ea2e5954958d90ef6de18e0f1aa4bc Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2024-08-12 10:14:43 +0000 Commit: Mark Johnston <markj@FreeBSD.org> CommitDate: 2024-09-19 13:00:40 +0000 pf tests: ensure that traceroutes using ICMP work Approved by: so Security: FreeBSD-EN-24:16.pf PR: 280701 MFC after: 1 week Sponsored by: Rubicon Communications, LLC ("Netgate") (cherry picked from commit 34063cb714602972b6d985ad747fc8f66a8daae1) (cherry picked from commit 7024e1066d5aba76dbbc85eb191357da7d32c619) tests/sys/netpfil/pf/icmp.sh | 65 +++++++++++++++++++++++++++++++++++++++++++ tests/sys/netpfil/pf/icmp6.sh | 65 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+)
A commit in branch releng/13.3 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=f51f7cb8997f2e43047a84e937144c2ac7e84587 commit f51f7cb8997f2e43047a84e937144c2ac7e84587 Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2024-08-12 14:07:35 +0000 Commit: Mark Johnston <markj@FreeBSD.org> CommitDate: 2024-09-19 13:00:07 +0000 pf: fix icmp-in-icmp state lookup In 534ee17e6 pf state checking for ICMP(v6) was made stricter. This change failed to correctly set the pf_pdesc for ICMP-in-ICMP lookups, resulting in ICMP error packets potentially being dropped incorrectly. Specially, it copied the ICMP header into a separate variable, not into the pf_pdesc. Populate the required pf_pdesc fields for the embedded ICMP packet's state lookup. Approved by: so Security: FreeBSD-EN-24:16.pf PR: 280701 MFC after: 1 week Sponsored by: Rubicon Communications, LLC ("Netgate") (cherry picked from commit 2da98eef1f352c496ffd458b4c68ddee972bb903) (cherry picked from commit 0d8d4cc3ea47f1ee61d749b22b135eb73c7d33cd) sys/netpfil/pf/pf.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-)
A commit in branch releng/13.3 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=7dabb892096e4e3ba7526914b94f97218d9690d3 commit 7dabb892096e4e3ba7526914b94f97218d9690d3 Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2024-08-26 12:59:38 +0000 Commit: Mark Johnston <markj@FreeBSD.org> CommitDate: 2024-09-19 13:00:57 +0000 pf: improve the ICMPv6 direction check Following bluhm's advice this changes the way we setup state keys and perform state lookups for ICMPv6 Neighbor Discovery packets: - replace the NS-dst with ND target address; - replace the NA-src with ND target address; - replace the NA-dst with unspecified address if it is a multicast. This allows pf to match Address Resolution, Neighbor Unreachability Detection and Duplicate Address Detection packets to the corresponding states without the need to create new ones or match unrelated ones. As a side effect we're doing now one state table lookup for ND packets instead of two. Fixes a bug uncovered by one of the previous commits that virtually breaks IPv6 connectivity after few minutes of use. ok stsp henning, with and ok bluhm Approved by: so Security: FreeBSD-EN-24:16.pf PR: 280701 MFC after: 1 week Obtained from: OpenBSD, mikeb <mikeb@openbsd.org>, 2633ae8c4c8a Sponsored by: Rubicon Communications, LLC ("Netgate") (cherry picked from commit 5ab1e5f7e5585558a73b723f07528977a82cee82) (cherry picked from commit b84344206721ed2803d5da68585289d5880efe3f) sys/net/pfvar.h | 2 +- sys/netpfil/pf/pf.c | 116 ++++++++++++++++++++++++++++++++++--------------- sys/netpfil/pf/pf_lb.c | 2 +- 3 files changed, 84 insertions(+), 36 deletions(-)
A commit in branch releng/13.3 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=36265a707dc51189843498e059361010ea3c9718 commit 36265a707dc51189843498e059361010ea3c9718 Author: Kristof Provost <kp@FreeBSD.org> AuthorDate: 2024-08-14 09:29:30 +0000 Commit: Mark Johnston <markj@FreeBSD.org> CommitDate: 2024-09-19 13:00:48 +0000 pf: invert direction for inner icmp state lookups (e.g. traceroute with icmp) ok henning, jsing Also extend the test case to cover this scenario. Approved by: so Security: FreeBSD-EN-24:16.pf PR: 280701 Obtained from: OpenBSD MFC after: 1 week Sponsored by: Rubicon Communications, LLC ("Netgate") (cherry picked from commit 89f6723288b0d27d3f14f93e6e83f672fa2b8aca) (cherry picked from commit 5f3f07397a7909e8f9449d1aa0b465159cbf0d60) sys/netpfil/pf/pf.c | 21 +++++++++++---------- tests/sys/netpfil/pf/icmp.sh | 4 +++- tests/sys/netpfil/pf/icmp6.sh | 4 +++- 3 files changed, 17 insertions(+), 12 deletions(-)