Bug 217262

Summary:

ipfw lookup tables match on index instead of value

Product:

Base System

Reporter:

Allan Jude <allanjude>

Component:

kern

Assignee:

Andrey V. Elsukov <ae>

Status:

Closed FIXED

Severity:

Affects Many People

CC:

cem, crest, ipfw, melifaro, rgrimes

Priority:

---

Flags:

rgrimes: mfc-stable11+

Version:

CURRENT

Hardware:

Any

OS:

Any

Attachments:

Description	Flags
Proposed patch	none

Description Allan Jude freebsd_committer

2017-02-21 02:19:33 UTC

When you create tables in ipfw, with a valtype of legacy, they match on the index of the value in the table, not the actual value as expected.

This bug was previously detected for the 'ipfw table XXX lookup KEY' case, and fixed in r307628

https://svnweb.freebsd.org/base?view=revision&revision=307628

However, it seems the same bug exists when packets are matched against the rules.


Create a table with enough unique values so that the tables do not work by accident (if the first value you create happens to be 1, its index will be 1, and it will seem like it is working):


sh -c 'for t in $(jot 50 10); do ipfw table 53 add 1.2.3.${t}/32 ${t};done'
...
added: 1.2.3.42/32 42
...
#ipfw table 53 lookup 1.2.3.42/32
1.2.3.42/32 42 [from debugging without r307628, we know index = 33]

#ipfw table 53 add 8.8.8.8/32 1
added: 8.8.8.8/32 1
#ipfw table 53 add 8.8.4.4/32 0
added: 8.8.4.4/32 0
#ipfw table 53 lookup 8.8.8.8
8.8.8.8/32 1 [Index = 51]
#ipfw table 53 lookup 8.8.4.4
8.8.4.4/32 0 [Index = 52]

 # We create a set of rules, traffic to 8.8.8.8 should match rule 5000, since its value is 1, but instead will match rule 5001, because the index of the value in the table is 51.

#ipfw add 5000 allow tcp from any to 'table(53,1)' dst-port 53
#ipfw add 5001 allow tcp from any to 'table(53,51)' dst-port 53
#ipfw add 6000 deny tcp from any to 8.8.8.8,8.8.4.4 dst-port 53

#ipfw table 53 list
--- table(53), set(0) ---
1.2.3.10/32 10
1.2.3.11/32 11
...
1.2.3.58/32 58
1.2.3.59/32 59
8.8.4.4/32 0
8.8.8.8/32 1

#ipfw show
00100       522        31320 allow ip from any to any via lo0
00200         0            0 deny ip from any to 127.0.0.0/8
00300         0            0 deny ip from 127.0.0.0/8 to any
00400         0            0 deny ip from any to ::1
00500         0            0 deny ip from ::1 to any
00600         0            0 allow ipv6-icmp from :: to ff02::/16
00700         0            0 allow ipv6-icmp from fe80::/10 to fe80::/10
00800        14         1084 allow ipv6-icmp from fe80::/10 to ff02::/16
00900         0            0 allow ipv6-icmp from any to any ip6 icmp6types 1
01000        48         3304 allow ipv6-icmp from any to any ip6 icmp6types 2,135,136
05000         0            0 allow tcp from any to table(53,1) dst-port 53
05001         0            0 allow tcp from any to table(53,51) dst-port 53
06000         0            0 deny tcp from any to 8.8.8.8,8.8.4.4 dst-port 53
65000 154480098 238424204503 allow ip from any to any
65535         0            0 deny ip from any to any


#telnet 8.8.8.8 53
Trying 8.8.8.8...
Connected to google-public-dns-a.google.com.
Escape character is '^]'.
Connection closed by foreign host.

#ipfw show 5000 5001 6000
05000         0            0 allow tcp from any to table(53,1) dst-port 53
05001         4          216 allow tcp from any to table(53,51) dst-port 53
06000         0            0 deny tcp from any to 8.8.8.8,8.8.4.4 dst-port 53

 # MATCHED THE WRONG RULE!

#telnet 8.8.4.4 53
Trying 8.8.4.4...
telnet: connect to address 8.8.4.4: Permission denied
telnet: Unable to connect to remote host

#ipfw show 5000 5001 6000
05000         0            0 allow tcp from any to table(53,1) dst-port 53
05001         4          216 allow tcp from any to table(53,51) dst-port 53
06000         9          540 deny tcp from any to 8.8.8.8,8.8.4.4 dst-port 53

Comment 1 Andrey V. Elsukov freebsd_committer

2017-03-02 18:46:58 UTC

Created attachment 180444 [details]
Proposed patch

Hi, can you test this patch? I think it should fix the problem.

Comment 2 Rodney W. Grimes freebsd_committer

2017-03-02 20:14:48 UTC

I ran the tests Allan posted here against your patch on an 11.0p1 system and it indeed has fixed the problem.

Please note that we need this and r307628 merged back to 11.0-stable and it would probably be a good idea to file an EN as this has a silent failure mode that allows unwanted packets through certain firewall types, though I doubt many are using the table,value mechanism it caused me a fair bit of trouble.

Comment 3 commit-hook freebsd_committer

2017-03-03 20:23:07 UTC

A commit references this bug:

Author: ae
Date: Fri Mar  3 20:22:42 UTC 2017
New revision: 314614
URL: https://svnweb.freebsd.org/changeset/base/314614

Log:
  Fix matching table entry value. Use real table value instead of its index
  in valuestate array.

  When opcode has size equal to ipfw_insn_u32, this means that it should
  additionally match value specified in d[0] with table entry value.
  ipfw_table_lookup() returns table value index, use TARG_VAL() macro to
  convert it to its value. The actual 32-bit value stored in the tag field
  of table_value structure, where all unspecified u32 values are kept.

  PR:		217262
  Reviewed by:	melifaro
  MFC after:	1 week
  Sponsored by:	Yandex LLC

Changes:
  head/sys/netpfil/ipfw/ip_fw2.c

Comment 4 commit-hook freebsd_committer

2017-03-10 05:44:39 UTC

A commit references this bug:

Author: ae
Date: Fri Mar 10 05:44:14 UTC 2017
New revision: 314990
URL: https://svnweb.freebsd.org/changeset/base/314990

Log:
  MFC r314614:
    Fix matching table entry value. Use real table value instead of its index
    in valuestate array.

    When opcode has size equal to ipfw_insn_u32, this means that it should
    additionally match value specified in d[0] with table entry value.
    ipfw_table_lookup() returns table value index, use TARG_VAL() macro to
    convert it to its value. The actual 32-bit value stored in the tag field
    of table_value structure, where all unspecified u32 values are kept.

    PR:		217262

Changes:
_U  stable/11/
  stable/11/sys/netpfil/ipfw/ip_fw2.c