IP tos field matching in pf uses bad comparison logic

classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

IP tos field matching in pf uses bad comparison logic

Steve Welham-2
>Submitter-Id: net
>Originator: Steve Welham
>Organization: net
>Synopsis: IP tos field matching in pf uses bad comparison logic
>Severity: non-critical
>Priority: high
>Category: kernel
>Class: sw-bug
>Release:     OpenBSD 3.9-stable (GENERIC) #1: Wed Aug 30 21:49:15
BST 2006
>Environment:
        System      : OpenBSD 3.9
        Architecture: OpenBSD.i386
        Machine     : i386
>Description:
The filter option "tos": is defined in man pf.conf as

 tos            = "tos" ( "lowdelay" | "throughput" | "reliability" |
                      [ "0x" ] number )

In pf.c the tos field match is done based on bitwise AND. This is
incorrect. For instance a packet with tos of 0xF0 will match the rule
"pass all tos 0x10".

>How-To-Repeat:

Install hping from ports (/usr/ports/net/hping/) to reproduce packets
easily.

# cat /etc/pf.conf
#

set skip on lo

pass all
pass log all tos 0x10
pass log all tos 0x8
pass log all tos 0x80


# pfctl -f pf.conf
# pfctl -sr
pass all
pass log all tos 0x10
pass log all tos 0x08
pass log all tos 0x80
# pfctl -sn
# pfctl -sq
No queue in use

Open 3 windows for testing - the first two for monitoring, each with a
tcpdump
running as follows:

1#tcpdump -enttti pflog0 not port 22
2#tcpdump -enttti fxp0 host 192.168.1.1

In the third window run the test, my output is included:

# pfctl -z
pf: rule counters cleared
# pfctl -vvsr
@0 pass all
  [ Evaluations: 21        Packets: 7         Bytes: 280         States:
0     ]
  [ Inserted: uid 0 pid 27359 ]
@1 pass log all tos 0x10
  [ Evaluations: 25        Packets: 17        Bytes: 1692        States:
0     ]
  [ Inserted: uid 0 pid 27359 ]
@2 pass log all tos 0x08
  [ Evaluations: 30        Packets: 0         Bytes: 0           States:
0     ]
  [ Inserted: uid 0 pid 27359 ]
@3 pass log all tos 0x80
  [ Evaluations: 34        Packets: 0         Bytes: 0           States:
0     ]
  [ Inserted: uid 0 pid 27359 ]
# hping -I fxp0 -S 192.168.1.1 -p 80 -c 1 -o F0
HPING 192.168.1.1 (bridge0 192.168.1.1): S set, 40 headers + 0 data bytes

--- 192.168.1.1 hping statistic ---
1 packets tramitted, 0 packets received, 100% packet loss
round-trip min/avg/max = 0.0/0.0/0.0 ms
# pfctl -vvsr
@0 pass all
  [ Evaluations: 247       Packets: 154       Bytes: 41682       States:
0     ]
  [ Inserted: uid 0 pid 27359 ]
@1 pass log all tos 0x10
  [ Evaluations: 251       Packets: 95        Bytes: 9764        States:
0     ]
  [ Inserted: uid 0 pid 27359 ]
@2 pass log all tos 0x08
  [ Evaluations: 255       Packets: 0         Bytes: 0           States:
0     ]
  [ Inserted: uid 0 pid 27359 ]
@3 pass log all tos 0x80
  [ Evaluations: 260       Packets: 1         Bytes: 40          States:
0     ]
  [ Inserted: uid 0 pid 27359 ]

Output from monitoring windows was:

1#
Aug 30 21:04:40.976118 rule 3/(match) pass out on xl0: 127.0.0.1.2577 >
192.168.1.1.80: [|tcp] [tos 0xf0]

2#
Aug 30 21:04:40.976129 0:1:2:d8:94:ab 0:3:2f:29:cc:3c 0800 54:
127.0.0.1.2577 > 192.168.1.1.80: S 879112598:879112598(0) win 512 [tos 0xf0]

So we can see from stats and pflog0 that rule 3 has matched - i.e. our
tos 0xf0
packet has matched a rule for tos 0x80 (it would also have matched rule 1).

>Fix:

The issue requires changing the pf.c comparison for evaluating a tos
rule from
bitwise AND to equality. For instance:

else if (r->tos && !(r->tos & pd->tos))

becomes

else if (r->tos && !(r->tos == pd->tos))

Below is a patch to fix the issue.

--- pf.c.orig   Wed Aug 30 21:08:33 2006
+++ pf.c.issue1 Thu Aug 31 11:47:55 2006
@@ -2878,7 +2878,7 @@
                else if (r->dst.port_op && !pf_match_port(r->dst.port_op,
                    r->dst.port[0], r->dst.port[1], th->th_dport))
                        r = r->skip[PF_SKIP_DST_PORT].ptr;
-               else if (r->tos && !(r->tos & pd->tos))
+               else if (r->tos && !(r->tos == pd->tos))
                        r = TAILQ_NEXT(r, entries);
                else if (r->rule_flag & PFRULE_FRAGMENT)
                        r = TAILQ_NEXT(r, entries);
@@ -3252,7 +3252,7 @@
                else if (r->dst.port_op && !pf_match_port(r->dst.port_op,
                    r->dst.port[0], r->dst.port[1], uh->uh_dport))
                        r = r->skip[PF_SKIP_DST_PORT].ptr;
-               else if (r->tos && !(r->tos & pd->tos))
+               else if (r->tos && !(r->tos == pd->tos))
                        r = TAILQ_NEXT(r, entries);
                else if (r->rule_flag & PFRULE_FRAGMENT)
                        r = TAILQ_NEXT(r, entries);
@@ -3587,7 +3587,7 @@
                        r = TAILQ_NEXT(r, entries);
                else if (r->code && r->code != icmpcode + 1)
                        r = TAILQ_NEXT(r, entries);
-               else if (r->tos && !(r->tos & pd->tos))
+               else if (r->tos && !(r->tos == pd->tos))
                        r = TAILQ_NEXT(r, entries);
                else if (r->rule_flag & PFRULE_FRAGMENT)
                        r = TAILQ_NEXT(r, entries);
@@ -3840,7 +3840,7 @@
                        r = r->skip[PF_SKIP_SRC_ADDR].ptr;
                else if (PF_MISMATCHAW(&r->dst.addr, pd->dst, af,
r->dst.neg))
                        r = r->skip[PF_SKIP_DST_ADDR].ptr;
-               else if (r->tos && !(r->tos & pd->tos))
+               else if (r->tos && !(r->tos == pd->tos))
                        r = TAILQ_NEXT(r, entries);
                else if (r->rule_flag & PFRULE_FRAGMENT)
                        r = TAILQ_NEXT(r, entries);
@@ -4051,7 +4051,7 @@
                        r = r->skip[PF_SKIP_SRC_ADDR].ptr;
                else if (PF_MISMATCHAW(&r->dst.addr, pd->dst, af,
r->dst.neg))
                        r = r->skip[PF_SKIP_DST_ADDR].ptr;
-               else if (r->tos && !(r->tos & pd->tos))
+               else if (r->tos && !(r->tos == pd->tos))
                        r = TAILQ_NEXT(r, entries);
                else if (r->src.port_op || r->dst.port_op ||
                    r->flagset || r->type || r->code ||


Finally my dmesg:
OpenBSD 3.9-stable (GENERIC) #2: Thu Aug 31 00:03:15 BST 2006
    [hidden email]:/usr/src/sys/arch/i386/compile/GENERIC
cpu0: Intel Pentium III ("GenuineIntel" 686-class) 1 GHz
cpu0:
FPU,V86,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,SER,MMX,FXSR,SSE
real mem  = 534810624 (522276K)
avail mem = 480972800 (469700K)
using 4278 buffers containing 26845184 bytes (26216K) of memory
mainbus0 (root)
bios0 at mainbus0: AT/286+(57) BIOS, date 12/01/01, BIOS32 rev. 0 @ 0xfd870
apm0 at bios0: Power Management spec V1.2
apm0: AC on, battery charge unknown
apm0: flags 30102 dobusy 0 doidle 1
pcibios0 at bios0: rev 2.1 @ 0xfd800/0x800
pcibios0: PCI IRQ Routing Table rev 1.0 @ 0xfdf30/176 (9 entries)
pcibios0: PCI Interrupt Router at 000:31:0 ("Intel 82371FB ISA" rev 0x00)
pcibios0: PCI bus #1 is the last bus
bios0: ROM list: 0xc0000/0xc000 0xcc000/0x600! 0xdf000/0x1000!
0xe0000/0x4000!
cpu0 at mainbus0
pci0 at mainbus0 bus 0: configuration mode 1 (no bios)
pchb0 at pci0 dev 0 function 0 "Intel 82815 Hub" rev 0x04: rng active,
9Kb/sec
vga1 at pci0 dev 2 function 0 "Intel 82815 Graphics" rev 0x04: aperture
at 0xf0000000, size 0x4000000
wsdisplay0 at vga1 mux 1: console (80x25, vt100 emulation)
wsdisplay0: screen 1-5 added (80x25, vt100 emulation)
ppb0 at pci0 dev 30 function 0 "Intel 82801BA AGP" rev 0x05
pci1 at ppb0 bus 1
xl0 at pci1 dev 1 function 0 "3Com 3c905C 100Base-TX" rev 0x78: irq 3,
address 00:01:02:d8:94:ab
bmtphy0 at xl0 phy 24: Broadcom 3C905C internal PHY, rev. 7
fxp0 at pci1 dev 8 function 0 "Intel 82562" rev 0x03, i82562: irq 9,
address 00:04:23:15:a7:c0
inphy0 at fxp0 phy 1: i82562ET 10/100 PHY, rev. 0
ichpcib0 at pci0 dev 31 function 0 "Intel 82801BA LPC" rev 0x05
pciide0 at pci0 dev 31 function 1 "Intel 82801BA IDE" rev 0x05: DMA,
channel 0 wired to compatibility, channel 1 wired to compatibility
wd0 at pciide0 channel 0 drive 0: <ST320410A>
wd0: 16-sector PIO, LBA, 19458MB, 39851760 sectors
wd0(pciide0:0:0): using PIO mode 4, Ultra-DMA mode 5
atapiscsi0 at pciide0 channel 1 drive 0
scsibus0 at atapiscsi0: 2 targets
cd0 at scsibus0 targ 0 lun 0: <OEM, CD-ROM F522B, 1.10> SCSI0 5/cdrom
removable
cd0(pciide0:1:0): using PIO mode 4, Ultra-DMA mode 2
uhci0 at pci0 dev 31 function 2 "Intel 82801BA USB" rev 0x05: irq 9
usb0 at uhci0: USB revision 1.0
uhub0 at usb0
uhub0: Intel UHCI root hub, rev 1.00/1.00, addr 1
uhub0: 2 ports with 2 removable, self powered
ichiic0 at pci0 dev 31 function 3 "Intel 82801BA SMBus" rev 0x05: irq 5
iic0 at ichiic0
uhci1 at pci0 dev 31 function 4 "Intel 82801BA USB" rev 0x05: irq 11
usb1 at uhci1: USB revision 1.0
uhub1 at usb1
uhub1: Intel UHCI root hub, rev 1.00/1.00, addr 1
uhub1: 2 ports with 2 removable, self powered
auich0 at pci0 dev 31 function 5 "Intel 82801BA AC97" rev 0x05: irq 5,
ICH2 AC97
ac97: codec id 0x41445360 (Analog Devices AD1885)
ac97: codec features headphone, Analog Devices Phat Stereo
audio0 at auich0
isa0 at ichpcib0
isadma0 at isa0
pckbc0 at isa0 port 0x60/5
pckbd0 at pckbc0 (kbd slot)
pckbc0: using irq 1 for kbd slot
wskbd0 at pckbd0: console keyboard, using wsdisplay0
pcppi0 at isa0 port 0x61
midi0 at pcppi0: <PC speaker>
spkr0 at pcppi0
lpt0 at isa0 port 0x378/4 irq 7
npx0 at isa0 port 0xf0/16: using exception 16
pccom0 at isa0 port 0x3f8/8 irq 4: ns16550a, 16 byte fifo
pccom0: console
fdc0 at isa0 port 0x3f0/6 irq 6 drq 2
fd0 at fdc0 drive 0: 1.44MB 80 cyl, 2 head, 18 sec
biomask ff65 netmask ff6d ttymask ffef
pctr: 686-class user-level performance counters enabled
mtrr: Pentium Pro MTRR support
uhidev0 at uhub1 port 1 configuration 1 interface 0
uhidev0: Logitech USB Receiver, rev 1.10/17.00, addr 2, iclass 3/1
ukbd0 at uhidev0: 8 modifier keys, 6 key codes
wskbd1 at ukbd0 mux 1
wskbd1: connecting to wsdisplay0
uhidev1 at uhub1 port 1 configuration 1 interface 1
uhidev1: Logitech USB Receiver, rev 1.10/17.00, addr 2, iclass 3/1
uhidev1: 4 report ids
ums0 at uhidev1 reportid 1: 16 buttons and Z dir.
wsmouse0 at ums0 mux 0
uhid0 at uhidev1 reportid 2: input=2, output=0, feature=0
uhid1 at uhidev1 reportid 3: input=1, output=0, feature=0
uhid2 at uhidev1 reportid 4: input=3, output=0, feature=0
dkcsum: wd0 matches BIOS drive 0x80
root on wd0a
rootdev=0x0 rrootdev=0x300 rawdev=0x30


--k7VFALiJ023677.1157037536/toad.pond.net--