Bug 238796 - ipfilter: failure to detect the same rules when arguments ordered differently
Summary: ipfilter: failure to detect the same rules when arguments ordered differently
Status: Open
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Cy Schubert
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2019-06-25 05:37 UTC by WHR
Modified: 2019-07-19 04:14 UTC (History)
2 users (show)

See Also:


Attachments
freebsd-ipfilter-rule-compare-fix.diff (3.42 KB, patch)
2019-06-25 05:37 UTC, WHR
no flags Details | Diff
Updated patch (3.31 KB, patch)
2019-06-26 05:15 UTC, Cy Schubert
no flags Details | Diff
Revert part of ip_fil.h cvs rev r1.31 (951 bytes, patch)
2019-07-13 05:45 UTC, Cy Schubert
no flags Details | Diff
Removal of all listed patches. (2 bytes, text/plain)
2019-07-15 20:12 UTC, Cy Schubert
no flags Details
This should fix this PR. (2.67 KB, patch)
2019-07-17 19:58 UTC, Cy Schubert
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description WHR 2019-06-25 05:37:57 UTC
Created attachment 205322 [details]
freebsd-ipfilter-rule-compare-fix.diff

This patch fix 2 bugs.

1. Unremovable rules:

A filter rule could becomes non-removable if it contains 'route-to' (displayed as 'to' in ipfstat(8) output), 'reply-to' or 'due-to' keyword to specify an interface name for routing.

For example:

[root@x ~]# ipfstat -Rion
# empty list for ipfilter(out)
@1 ...
@2 ...
@3 ...
@4 pass in quick on vboxnet0 to tun0:10.1.202.11 inet proto tcp from 10.12.4.0/24 port = 22 to any
@5 pass in quick on vboxnet0 to tun0:10.1.202.11 inet from 10.0.5.52/32 to any
[root@x ~]# echo "pass in quick on vboxnet0 to tun0:10.1.202.11 inet proto tcp from 10.12.4.0/24 port = 22 to any" | ipf -r -f -
29:1:ioctl(delete rule): rule not found for removing
[root@x ~]# echo "pass in quick on vboxnet0 to tun0:10.1.202.11 inet proto tcp from 10.12.4.0/24 port = 22 to any" | ipf -f -
[root@x ~]# ipfstat -Rion
# empty list for ipfilter(out)
@1 ...
@2 ...
@3 ...
@4 pass in quick on vboxnet0 to tun0:10.1.202.11 inet proto tcp from 10.12.4.0/24 port = 22 to any
@5 pass in quick on vboxnet0 to tun0:10.1.202.11 inet from 10.0.5.52/32 to any
@6 pass in quick on vboxnet0 to tun0:10.1.202.11 inet proto tcp from 10.12.4.0/24 port = 22 to any

As showing by the output, the rule @4 cannot be removed by using 'ipf -r'; trying to add the exactly same rule succeed, as rule @6; but duplicated rules are not allowed by the ipfilter design.
Rule @5 has the same issue.

The cause of this bug is when comparing 2 rules, the code failed to exclude some volatile variables such as pointers and index numbers to a volatile array.
The pointers included in rules comparison are 'fd_ptr' in 'frdest_t', which are turn be included as 'fr_tifs' and 'fr_dif' in 'struct frentry', the rule entry structure. The index numbers are 'fr_ifnames' in 'struct frentry', and 'fd_name', 'fr_tifs', 'fr_dif'; all those numbers are indexing strings in array 'fr_names' in 'struct frentry'; the actual strings should be compared instead of the indexes, since the string sequence inside 'fr_ifnames' may differ even between 2 same rules.
Another variable should be excluded from comparison is 'fd_local' in 'frdest_t'. This variable is a hit for the code to determine whether an address is at local; it shouldn't be compared, because this could be changed during runtime (an address was added to an interface after a rule was added).


2. Inefficient rule checksum

There is a member 'fr_cksum' in 'struct frentry'; it was designed to speedup rules comparison; see https://svnweb.freebsd.org/base/stable/12/sys/contrib/ipfilter/netinet/fil.c?revision=349223&view=markup#l4922

This above code calculates first part of the checksum starting from member 'fr_func', ending at 'fr_chsum'. However in ipfilter revision '2580062 from/to targets should be able to use any interface name; 2605045 destination lists aren't loaded; 2605049 destination lists need testing; 2637667 pool stats structures should not have pointers; 2644504 cannot list configured destination lists; 2644536 destination lists need more selection policies' branch 'v5-1-RELEASE' on 2009-03-08 09:08:32, the member 'fr_chsum' was moved, sitting before 'fr_func', causing this calculation be skipped.
Comment 1 Cy Schubert freebsd_committer 2019-06-26 00:50:38 UTC
Thanks for the patch. It is being reviewed along with the bug itself.
Comment 2 commit-hook freebsd_committer 2019-06-26 00:54:32 UTC
A commit references this bug:

Author: cy
Date: Wed Jun 26 00:53:49 UTC 2019
New revision: 349401
URL: https://svnweb.freebsd.org/changeset/base/349401

Log:
  While working on PR/238796 I discovered an unused variable in frdest,
  the next hop structure. It is likely this contributes to PR/238796
  though other factors remain to be investigated.

  PR:		238796
  MFC after:	1 week

Changes:
  head/sys/contrib/ipfilter/netinet/fil.c
  head/sys/contrib/ipfilter/netinet/ip_fil.h
Comment 3 WHR 2019-06-26 05:01:19 UTC
I'm actually didn't noticed that 'fd_local' is just been set but not used in the code; but it doesn't cause the issue in my test, 'fd_local' is 0 in all rules.
By inserting printf(8)s to 'ipf_rule_compare', and manually comparing each possible members, I can only seen the string index numbers in 'fr_ifnames' and 'fd_name' different between 2 instances representing a same rule.
'fd_ptr' value didn't change in the last test, because ifunit(9) returns the same pointer to 'struct ifnet' for same interface; but what if that interface recreated with same name? The 'fd_ptr' may have a different value than the new pointer returned by ifunit(9).

BTW, this bug is already exists in IP Filter 4.*; but the only problematic variable was 'fd_ptr', may be plus the unused space in 'fr_ifnames' (type char [4][LIFNAMSIZ]), in that version.
I has first discovered this bug on a Solaris system, and found the 'fd_ifp' (in 'frdest_t', renamed to 'fd_ptr' in v5 branch) is changing between old and new instances of 'struct frentry'. I later fixed this bug in IP Filter 4.1.34 for Solaris (https://git.nsscn.top/Low-power/IPFilter/commit/9bb6c656ac6fef52e53890833703bf7ddea1e18b).
Comment 4 Cy Schubert freebsd_committer 2019-06-26 05:15:44 UTC
Created attachment 205341 [details]
Updated patch

I suppose I'll post my version of your patch. I'm far from finished, I'm still digging. Far from a compete review but you get the idea where I'm going with this. Plus style(9) improvements.

I've found a lot of this kind of stuff and panics in ipfilter.

I've given our friends at NetBSD a heads up too.
Comment 5 Cy Schubert freebsd_committer 2019-06-28 19:35:25 UTC
Having tested the sample in this PR on FreeBSD 13-CURRENT, there is no need for this patch. It already works. I should have tested it first to verify the problem actually existed.

Closing this bug as not a bug.
Comment 6 commit-hook freebsd_committer 2019-07-03 17:09:48 UTC
A commit references this bug:

Author: cy
Date: Wed Jul  3 17:09:43 UTC 2019
New revision: 349655
URL: https://svnweb.freebsd.org/changeset/base/349655

Log:
  MFC r349401:

  While working on PR/238796 I discovered an unused variable in frdest,
  the next hop structure. It is likely this contributes to PR/238796
  though other factors remain to be investigated.

  PR:		238796

Changes:
_U  stable/11/
  stable/11/sys/contrib/ipfilter/netinet/fil.c
  stable/11/sys/contrib/ipfilter/netinet/ip_fil.h
_U  stable/12/
  stable/12/sys/contrib/ipfilter/netinet/fil.c
  stable/12/sys/contrib/ipfilter/netinet/ip_fil.h
Comment 7 WHR 2019-07-09 04:14:24 UTC
 (In reply to Cy Schubert from comment #5)

I has successfully reproduced this bug in kernel version 13.0-CURRENT r349753.
I don't known how does you believe this bug could disappear without actual code change.
Comment 8 WHR 2019-07-09 04:15:21 UTC
(In reply to Cy Schubert from comment #5)

> there is no need for this patch. It already works.

Why?
Comment 9 Cy Schubert freebsd_committer 2019-07-09 05:38:53 UTC
cwfw# echo "pass in quick on fxp0 to sk0:10.1.1.1 inet proto tcp from 192.168.0.0/24 port = 22 to any" | ipf -f -
cwfw# ipfstat -ion | grep 'pass in quick on fxp0 to sk0:10.1.1.1 inet'
@212 pass in quick on fxp0 to sk0:10.1.1.1 inet proto tcp from 192.168.0.0/24 port = ssh to any
cwfw# echo "pass in quick on fxp0 to sk0:10.1.1.1 inet proto tcp from 192.168.0.0/24 port = 22 to any" | ipf -r -f -
cwfw# ipfstat -ion | grep 'pass in quick on fxp0 to sk0:10.1.1.1 inet'          cwfw# 
cwfw# uname -a
FreeBSD cwfw 13.0-CURRENT FreeBSD 13.0-CURRENT #407 r349853M: Mon Jul  8 18:28:18 PDT 2019     root@cwfw:/export/obj/opt/src/svn-current/amd64.amd64/sys/PROD2  amd64
cwfw# 

I am unable to reproduce this on my production firewall. It is likely your problem is due to one of your custom patches.
Comment 10 WHR 2019-07-09 07:22:18 UTC
(In reply to Cy Schubert from comment #9)

I never said I has used any custom patch when discovering this bug, the only patch 'attachment 205322 [details]' is used to FIX the bug. In fact the 2 kernel images and associated 'ipl.ko' KLD module are downloaded directly from FreeBSD download site, with version 12.0-STABLE r349024 and 13.0-CURRENT r349753.
Comment 11 Cy Schubert freebsd_committer 2019-07-09 11:44:35 UTC
Unfortunately I cannot accept a patch for something I cannot reproduce. AFAIAC your patch does not fix any bug. Help me reproduce it here then.

A patch for something I cannot verify or reproduce is not enough. If this is a bug, what do I do to expose it? I need your help then. Help me verify that this is indeed a bug. Help me out here.
Comment 12 WHR 2019-07-09 14:27:05 UTC
(In reply to Cy Schubert from comment #11)

Although this bug is always reproduce on that particular machine, with both 12.0-STABLE and 13.0-CURRENT kernels, I failed to reproduce it on a testing VM. I plan to install another test VM with FreeBSD 13.0-CURRENT, tomorrow.
Comment 13 Cy Schubert freebsd_committer 2019-07-09 19:18:00 UTC
Unfortunately I cannot accept patches until I can reproduce the problem here. I have tested the rule in a VM with INVARIANTS and on my production firewall without INVARIANTS. I am unable to verify that this is a bug. Or, what makes your one machine different from all the others needs to be discovered. Then I can reproduce it here. If you can provide me with something I can reproduce the scenario here I will be open to committing an updated version of your patch. Otherwise I am not able to justify an unverified patch.
Comment 14 WHR 2019-07-10 03:40:22 UTC
Good news. I has reproduced this bug in a FreeBSD 13.0-CURRENT r349753 testing VM.

The steps are:

kldload ipl.ko
ifconfig tun0 plumb
ifconfig tun1 plumb
echo "pass in quick reply-to tun0:10.1.1.1 on tun0 proto tcp from any to 10.1.1.11 port = 22 flags S/FSRPAU keep state" | ipf -f -
echo "pass in quick reply-to tun1:10.1.2.1 on tun1 proto tcp from any to 10.1.2.11 port = 22 flags S/FSRPAU keep state" | ipf -f -
echo "pass in quick reply-to tun0:10.1.1.1 on tun0 proto tcp from any to 10.1.1.11 port = 22 flags S/FSRPAU keep state" | ipf -f -
ipfstat -Rion
echo "pass in quick route-to tun1:10.1.2.1 on em0 proto tcp from 10.0.3.63 to any" | ipf -f -
echo "pass in quick route-to tun0:10.1.1.1 on em0 proto tcp from 10.0.3.64 to any port = 23" | ipf -f -
echo "pass in quick reply-to tun0:10.1.1.1 on tun0 proto tcp from any to 10.1.1.11 port = 22 flags S/FSRPAU keep state" | ipf -f -
ipfstat -Ri
ipfstat -Ri | ipf -f -

Please tell if you want the VM configuration or the disk image.
Comment 15 Cy Schubert freebsd_committer 2019-07-10 06:04:53 UTC
That's perfect, thank you. I'll do some testing here. I suspect the cause is similar to a panic I am working on. Use your patch or the improved patch I posted here while I dig into the root cause.
Comment 16 Cy Schubert freebsd_committer 2019-07-13 04:49:05 UTC
This was broken by ipfilter commit c8beabe in 2009. fr_cksum was moved from the end of frentry_t to before fr_func. It's a wonder any rule compares work.
Comment 17 Cy Schubert freebsd_committer 2019-07-13 05:11:48 UTC
To be more precise (since I converted Darren's CVS tree to GIT), the CVS rev number reported by cvs blame for ip_fil.h.

1.31         (darren_r 01-Mar-09)
Comment 18 Cy Schubert freebsd_committer 2019-07-13 05:45:44 UTC
Created attachment 205744 [details]
Revert part of ip_fil.h cvs rev r1.31

This patch reverts part of ip_fil.h r1.31 (March 1, 2009) about 4 years before 5.1.2 was imported into FreeBSD. This resolves the issue on my main firewall.
Comment 19 Cy Schubert freebsd_committer 2019-07-13 06:26:49 UTC
I will produce an improved patch.
Comment 20 Cy Schubert freebsd_committer 2019-07-15 20:12:41 UTC
Created attachment 205808 [details]
Removal of all listed patches.

I've cleaned up some of the pointer arithmetic in fil.c and ip_state.c (my last three commits), which have resolved another issue but also resolve this one. Please svnup to the latest -current and try reproducing the bug again (without any other patches applied).
Comment 21 WHR 2019-07-16 09:39:03 UTC
(In reply to Cy Schubert from comment #20)

Unfortunately this bug is still happening in r350024.

I has verified that the source code I compiling contain your recent 3 commits (349978, 349979, 349980); and I don't think those changes could possibly fix this bug.
Comment 22 Cy Schubert freebsd_committer 2019-07-16 11:37:13 UTC
Hmmm. I am not able to reproduce the bug on real hardware or in a VM any more.
Comment 23 Cy Schubert freebsd_committer 2019-07-17 19:58:03 UTC
Created attachment 205851 [details]
This should fix this PR.

Can you try this pstch? Make sure your tree is up to date before applying it.
Comment 24 Cy Schubert freebsd_committer 2019-07-17 20:03:41 UTC
The problem was that the following fix in 2009, ip_fil.h r1.31 and fil.c r1.53, is incomplete. A number of issues not relating to this PR have already been fixed. The posted patch directly fixes this PR.

The upstream fix was incomplete:
2580062 from/to targets should be able to use any interface name
Comment 25 Cy Schubert freebsd_committer 2019-07-18 20:13:26 UTC
Rearranging input arguments breaks checksum.
Comment 26 WHR 2019-07-19 03:16:16 UTC
(In reply to Cy Schubert from comment #23)

This patch seems break adding rules:

[root@ipfilter-test /usr/obj]# kldload usr/src/amd64.amd64/sys/modules/ipfilter/ipl.ko 
[root@ipfilter-test /usr/obj]# kldstat
Id Refs Address                Size Name
 1    7 0xffffffff80200000  24ffe50 kernel
 2    1 0xffffffff82819000     2538 intpm.ko
 3    1 0xffffffff8281c000      a50 smbus.ko
 4    1 0xffffffff8281d000    3b468 ipl.ko
[root@ipfilter-test /usr/obj]# echo "pass in quick reply-to tun0:10.1.1.1 on tun0 proto tcp from any to 10.1.1.11 port = 22 flags S/FSRPAU keep state" | ipf -f -
8:1:ioctl(add/insert rule): no data provided with filter rule
[root@ipfilter-test /usr/obj]# echo "pass in quick reply-to tun1:10.1.2.1 on tun1 proto tcp from any to 10.1.2.11 port = 22 flags S/FSRPAU keep state" | ipf -f -
8:1:ioctl(add/insert rule): no data provided with filter rule
[root@ipfilter-test /usr/obj]# echo "pass in quick reply-to tun0:10.1.1.1 on tun0 proto tcp from any to 10.1.1.11 port = 22 flags S/FSRPAU keep state" | ipf -f -
8:1:ioctl(add/insert rule): no data provided with filter rule
[root@ipfilter-test /usr/obj]# ipfstat -Rion
# empty list for ipfilter(out)
# empty list for ipfilter(in)

Kernel version is 13.0-CURRENT r350103.
Comment 27 Cy Schubert freebsd_committer 2019-07-19 03:37:02 UTC
I'm having no such problems as you are.

Do you have INVARIANTS and WITNESS enabled in your kernel?

Send me a copy of your ipf.conf and ipnat.conf, please. If you use ippool, that too, please.

Except for rules with arguments out of order I cannot reproduce your problem on any of my four firewalls.
Comment 28 WHR 2019-07-19 04:14:05 UTC
This testing OS is installed from the latest 13.0-CURRENT snapshot built image that I downloaded today, specifically, http://ftp.freebsd.org/pub/FreeBSD/snapshots/amd64/amd64/ISO-IMAGES/13.0/FreeBSD-13.0-CURRENT-amd64-20190718-r350103-disc1.iso.xz

The source code is installed along with other parts from the installer, in /usr/src/

> Do you have INVARIANTS and WITNESS enabled in your kernel?
Yes, the prebuilt snapshot images have those enabled.

> Send me a copy of your ipf.conf and ipnat.conf, please.
Since this is a fresh installation for testing, no rules exist in ipfilter config files.