[prev in list] [next in list] [prev in thread] [next in thread] 

List:       netfilter-devel
Subject:    Bug in MAC address matching
From:       "Erick C. Jones" <erick () baronservices ! com>
Date:       2001-08-30 2:20:00
[Download RAW message or body]

I don't really know much about this code, I'm just passing along a very small
bug that I think I fixed.

The MAC address match apparently fails when a packet is very small.

The following code from ipt_mac.c, around line 21, appears to be the culprit:

/* Is mac pointer valid? */
return (skb->mac.raw >= skb->head
            && skb->mac.raw < skb->head + skb->len - ETH_HLEN
            /* If so, compare... */

(I'm testing this against the 2.4.2 kernel, but I checked and saw that
ipt_mac.c has not been changed in the latest kernel.  My apologies if this
problem has already been fixed by a change elsewhere in the code.)

The pointer validity check apparently assumes that skb->len is the total
length of the area pointed to by skb->head.  I'm not sure exactly what
skb->len is, but it's not that.

If I try to apply this rule to a 2-byte UDP packet (that is, the application's
sendto call only sends 2 bytes), skb->len is 30.  (In general, it seems to be
28 plus the payload size.)

Does that mean that skb->head points to a 30-byte region?  But the payload
itself actually starts at skb->head + 60!

What about the skb->end field?  In the case of the 2-byte UDP packet, skb->end
- skb->head == 80, which seems a more reasonable measurement of the size (but
I'm just guessing here).

Regardless of what skb->len actually means, let's consider what it would take
for the conditions above to be met.  "skb->mac.raw >= skb->head" should always
be true (unless we have corrupt data or something, I guess).  But when will
"skb->mac.raw < skb->head + skb->len - ETH_HLEN" be true?  I've found that
skb->mac.raw - skb->head == 18, so this simplifies to "18 < skb->len -
ETH_HLEN".  And since skb->len is the payload size plus 28 and ETH_HLEN is 14,
we have "18 < 28 + p - 14", or "4 < p" (where p is the payload size).  So this
condition will fail if the size of the payload is less than or equal to 4.

Anyway, I fixed this by simply implementing a more lenient validity check:

/* Is mac pointer valid? */
return (skb->mac.raw >= skb->head
            && skb->mac.raw + ETH_HLEN <= skb->end
            /* If so, compare... */

Correct solution?  Am I missing something?

Why do I even care about this?  Well, I have two firewalls and two routers all
connected to a single hub, and each firewall must be able to determine whether
a given packet came from the other firewall or from a router.  Since all the
hosts directly connected to the hub are trusted, the mac address seems like a
reliable indicator.  (In other words, the routers themselves can be trusted to
correctly report their own mac addresses, even if the packets they're relaying
are from malevolent hackers; similarly, the other firewall will correctly
report its mac address, and a little more trust can be assigned to packets
from it because they must have come from inside the building.)  Unfortunately,
PCAnywhere likes to initiate connections by sending a 2-byte UDP packet, and
it was failing the mac address check.

[prev in list] [next in list] [prev in thread] [next in thread]