REJECT using invalid data

REJECT using invalid data

[Simon Kirby]

Hello,

After some recent interesting network issues involving an onboard Tigon3
card with faulty buffer memory, we discovered that netfilter has some
issues with handling corrupted packets.  This took quite some time to
diagnose, I might add. :)

It appears that a packet with a bad TCP checksum will be disregarded by
the state tracking code (and apparently shows up as "INVALID").  Fine.

However, the same packet will then likely continue traversing rules until
it hits some kind of REJECT rule.  REJECT can be set to reject with a
tcp-reset or some ICMP response at this point.  If so, it will actually
use the possibly-incorrect information from the bad TCP packet and send a
rejection packet.  As far as I can tell, this is a bug.

What happens as a result of this is that any corrupted packet will result
in TCP sessions being immediately terminated.  This is bad because
normally TCP would retransmit and recover from the error.

I'm quite busy but I can look at creating a patch for this if nobody has
any immediate objections (or already knows how to easily make the patch). 

Thanks,

Simon-

Re: REJECT using invalid data

[Pablo Neira]

Simon Kirby wrote:

>After some recent interesting network issues involving an onboard Tigon3
>card with faulty buffer memory, we discovered that netfilter has some
>issues with handling corrupted packets.  This took quite some time to
>diagnose, I might add. :)
>
>It appears that a packet with a bad TCP checksum will be disregarded by
>the state tracking code (and apparently shows up as "INVALID").  Fine.
>
>However, the same packet will then likely continue traversing rules until
>it hits some kind of REJECT rule.
>

not really, my ruleset drops it, actually another rule here is 
previously logging it.

-A INPUT -m state --state INVALID -j DROP

>  REJECT can be set to reject with a
>tcp-reset or some ICMP response at this point.  If so, it will actually
>use the possibly-incorrect information from the bad TCP packet and send a
>rejection packet.  As far as I can tell, this is a bug.
>  
>

yes, that's a bug, but in your ruleset, people should log/drop/let 
continue invalid packets, but not reject them. So I don't see the point 
of sending a patch. Maybe you could send a patch to clarify this in the 
man page.

--
Pablo

Re: REJECT using invalid data

[Simon Kirby]

On Tue, Dec 07, 2004 at 12:07:15PM +0100, Pablo Neira wrote:

> not really, my ruleset drops it, actually another rule here is 
> previously logging it.
> 
> -A INPUT -m state --state INVALID -j DROP

That's all fine and dandy, except that we can't use state tracking in our
configuration because of asymmetric routing (which is required due to
BGP).  This is fairly common, and not an incorrect setup.

> > REJECT can be set to reject with a
> >tcp-reset or some ICMP response at this point.  If so, it will actually
> >use the possibly-incorrect information from the bad TCP packet and send a
> >rejection packet.  As far as I can tell, this is a bug.
> 
> yes, that's a bug, but in your ruleset, people should log/drop/let 

No!  It is not a bug in our ruleset, it is a bug in REJECT.

It is incorrect to reply to packet layers that have bad checksums.
REJECT in this case must DROP, because anything else would be broken.

Simon-

Re: REJECT using invalid data

[Pablo Neira]

[-- Attachment #1: Type: text/plain, Size: 1329 bytes --]

Simon Kirby wrote:

>On Tue, Dec 07, 2004 at 12:07:15PM +0100, Pablo Neira wrote:
>  
>
>>not really, my ruleset drops it, actually another rule here is 
>>previously logging it.
>>
>>-A INPUT -m state --state INVALID -j DROP
>>    
>>
>
>That's all fine and dandy, except that we can't use state tracking in our
>configuration because of asymmetric routing (which is required due to
>BGP).  This is fairly common, and not an incorrect setup.
>  
>

Hm I thought that you were using state tracking... I agree your setup is 
correct.

>>>REJECT can be set to reject with a
>>>tcp-reset or some ICMP response at this point.  If so, it will actually
>>>use the possibly-incorrect information from the bad TCP packet and send a
>>>rejection packet.  As far as I can tell, this is a bug.
>>>      
>>>
>>yes, that's a bug, but in your ruleset, people should log/drop/let 
>>    
>>
>
>No!  It is not a bug in our ruleset, it is a bug in REJECT.
>
>It is incorrect to reply to packet layers that have bad checksums.
>REJECT in this case must DROP, because anything else would be broken.
>  
>
Now I see, if state tracking is not enable there's no way to avoid this 
problem. But I guess that we should drop all malformed packets, not only 
those which have bad checksums. Would you like to give a try to the 
patch attached?

--
Pablo

[-- Attachment #2: reject.patch --]
[-- Type: text/x-patch, Size: 3030 bytes --]

===== net/ipv4/netfilter/ipt_REJECT.c 1.32 vs edited =====
--- 1.32/net/ipv4/netfilter/ipt_REJECT.c	2004-11-13 14:41:07 +01:00
+++ edited/net/ipv4/netfilter/ipt_REJECT.c	2004-12-07 23:03:29 +01:00
@@ -352,6 +352,76 @@
 		ip_finish_output);
 }	
 
+#define TH_FIN  0x01
+#define TH_SYN  0x02
+#define TH_RST  0x04
+#define TH_PUSH 0x08
+#define TH_ACK  0x10
+#define TH_URG  0x20
+#define TH_ECE  0x40
+#define TH_CWR  0x80
+
+/* table of valid flag combinations - ECE and CWR are always valid */
+static u8 tcp_valid_flags[(TH_FIN|TH_SYN|TH_RST|TH_PUSH|TH_ACK|TH_URG) + 1] =
+{
+	[TH_SYN]                        = 1,
+	[TH_SYN|TH_ACK]                 = 1,
+	[TH_RST]                        = 1,
+	[TH_RST|TH_ACK]                 = 1,
+	[TH_RST|TH_ACK|TH_PUSH]         = 1,
+	[TH_FIN|TH_ACK]                 = 1,
+	[TH_ACK]                        = 1,
+	[TH_ACK|TH_PUSH]                = 1,
+	[TH_ACK|TH_URG]                 = 1,
+	[TH_ACK|TH_URG|TH_PUSH]         = 1,
+	[TH_FIN|TH_ACK|TH_PUSH]         = 1,
+	[TH_FIN|TH_ACK|TH_URG]          = 1,
+	[TH_FIN|TH_ACK|TH_URG|TH_PUSH]  = 1,
+};
+
+/* Protect against broken packets. Reduced version of tcp_error, taken 
+ * from tcp connection tracking. 
+ * 
+ * If someone else needs this piece of code, we should move this elsewhere 
+ * instead of copying it over and over again --pablo 
+ */
+static int tcp_error(struct sk_buff *skb, unsigned int hooknum)
+{
+	struct iphdr *iph = skb->nh.iph;
+	struct tcphdr _tcph, *th;
+	unsigned int tcplen = skb->len - iph->ihl * 4;
+	u_int8_t tcpflags;
+
+	/* Smaller that minimal TCP header? */
+	th = skb_header_pointer(skb, iph->ihl * 4,
+				sizeof(_tcph), &_tcph);
+	if (th == NULL) 
+		return 1;
+  
+	/* Not whole TCP header or malformed packet */
+	if (th->doff*4 < sizeof(struct tcphdr) || tcplen < th->doff*4) 
+		return 1;
+  
+	/* Checksum invalid? Ignore.
+	 * We skip checking packets on the outgoing path
+	 * because the semantic of CHECKSUM_HW is different there 
+	 * and moreover root might send raw packets.
+	 */
+	/* FIXME: Source route IP option packets --RR */
+	if (hooknum == NF_IP_PRE_ROUTING
+	    && csum_tcpudp_magic(iph->saddr, iph->daddr, tcplen, IPPROTO_TCP,
+			         skb->ip_summed == CHECKSUM_HW ? skb->csum
+			      	 : skb_checksum(skb, iph->ihl*4, tcplen, 0)))
+		return 1;
+
+	/* Check TCP flags. */
+	tcpflags = (((u_int8_t *)th)[13] & ~(TH_ECE|TH_CWR));
+	if (!tcp_valid_flags[tcpflags]) 
+		return 1;
+
+	return 0;
+}
+
 static unsigned int reject(struct sk_buff **pskb,
 			   const struct net_device *in,
 			   const struct net_device *out,
@@ -364,6 +434,12 @@
 	/* Our naive response construction doesn't deal with IP
            options, and probably shouldn't try. */
 	if ((*pskb)->nh.iph->ihl<<2 != sizeof(struct iphdr))
+		return NF_DROP;
+
+	/* It is incorrect to reply to malformed packet.
+	 * REJECT in this case must DROP, because anything
+	 * else would be broken. */
+	if (tcp_error(*pskb, hooknum))
 		return NF_DROP;
 
 	/* WARNING: This code causes reentry within iptables.

Re: REJECT using invalid data

[Kiran Kumar Immidi]

On Wednesday 08 December 2004 03:44, Pablo Neira wrote:

>Now I see, if state tracking is not enable there's no way to avoid this 
>problem. But I guess that we should drop all malformed packets, not only 
>those which have bad checksums. Would you like to give a try to the 
>patch attached?

  Just a comment on this;

+static u8 tcp_valid_flags[(TH_FIN|TH_SYN|TH_RST|TH_PUSH|TH_ACK|TH_URG) + 1] =

  This makes the array about 64 bytes long, would be better to store as an 
array of valid flags rather than as a bit mask;

-- 
Regards,
Kiran Kumar Immidi

Re: REJECT using invalid data

[Pablo Neira]

Krzysztof Oledzki wrote:

>I'm not sure if changing "-j REJECT" to drop invalid tcp packets is good
>idea since we have -m unclean and we can drop such packets explicitly
>with this match.
>

unclean was removed from 2.6 series

> And what about other protocols (udp, etc) when REJECT
>generates ICMP port-unreachable?
>  
>

you are right, udp stuff is missing, checkings here are trivial to add 
anyway. I'll think about this issue more carefully in next days.

--
Pablo

Re: REJECT using invalid data

[Krzysztof Oledzki]

On Wed, 8 Dec 2004, Pablo Neira wrote:

> Krzysztof Oledzki wrote:
>
> >I'm not sure if changing "-j REJECT" to drop invalid tcp packets is good
> >idea since we have -m unclean and we can drop such packets explicitly
> >with this match.
> >
>
> unclean was removed from 2.6 series

Oh? I did not know about that... After some googling I have just found
http://lists.netfilter.org/pipermail/netfilter-devel/2003-August/012199.html
So, how it is now possible to DROP invalid packets? I think that it should
be at least a match which can be used to drop broken or malformed packets
such as ones used to expoloit buggy IP stack on old win95 hosts.

> > And what about other protocols (udp, etc) when REJECT
> >generates ICMP port-unreachable?
> >
> you are right, udp stuff is missing, checkings here are trivial to add
> anyway. I'll think about this issue more carefully in next days.
OK.

Best regards,

				Krzysztof Olędzki

Re: REJECT using invalid data

[Simon Kirby]

On Wed, Dec 08, 2004 at 12:54:09AM +0100, Pablo Neira wrote:

> >And what about other protocols (udp, etc) when REJECT
> >generates ICMP port-unreachable?
> 
> you are right, udp stuff is missing, checkings here are trivial to add 
> anyway. I'll think about this issue more carefully in next days.

I don't think it's a good idea to try to filter all TCP flags in REJECT
unless we're trying to avoid rejecting rejects and we don't already do
that for some reason (eep), for the same reason that unclean was removed
(ECN and other new functionality could break).  I would think that all
that is needed is a check for the RST bit.

At the same time, it would be nice to have a match (or at least some
functionality) resulting in the ability to drop corrupted packets
("corrupted" as in with the checksum) that would otherwise be accepted. 

Hmm.  Maybe this should be done at a different level?  It should
basically not match "-p tcp" in the rule "iptables -p tcp -j REJECT". 
Doing it at "-p tcp" time would also correct "iptables -p tcp --dport 80
-j ACCEPT", which would otherwise also be affected by the same problem
(the TCP port could be corrupted).

Simon-

Re: REJECT using invalid data

[Simon Kirby]

On Fri, Dec 10, 2004 at 01:29:30PM +0100, Krzysztof Oledzki wrote:

> > I don't think it's a good idea to try to filter all TCP flags in REJECT
> > unless we're trying to avoid rejecting rejects and we don't already do
> > that for some reason (eep), for the same reason that unclean was removed
> > (ECN and other new functionality could break).  I would think that all
> > that is needed is a check for the RST bit.
> >
> > At the same time, it would be nice to have a match (or at least some
> > functionality) resulting in the ability to drop corrupted packets
> > ("corrupted" as in with the checksum) that would otherwise be accepted.
> >
> > Hmm.  Maybe this should be done at a different level?  It should
> > basically not match "-p tcp" in the rule "iptables -p tcp -j REJECT".
> > Doing it at "-p tcp" time would also correct "iptables -p tcp --dport 80
> > -j ACCEPT", which would otherwise also be affected by the same problem
> > (the TCP port could be corrupted).
> 
> No. We simply can't do that. Please consider such config:
> iptables -A INPUT -p tcp -j ACCEPT
> iptables -A INPUT -j REJECT

Yes, what's the problem?

If the TCP checksum is fine, "-p tcp" will match and it will accept the
packet.  If it is corrupted, "-p tcp" will not match and the ACCEPT rule
will not match.  The REJECT rule will then DROP the packet.  There is
nothing wrong with this.

The only difference here is the use of "-p tcp" alone to literally match
the protocol in the IP header (under the IP checksum) versus the use of
"-p tcp" to enable inspection of the TCP header.

For example, if this rule were to match a packet with a bad TCP checksum,
it would be broken (because the port could be corrupted):

	iptables -A INPUT -p tcp --dport 80 -j ACCEPT

On the other hand, it would not be broken to match in this case:

	iptables -A INPUT -p tcp -j ACCEPT

...which may be what you are getting at.  Fixing this requires a syntax
or design change, most likely.

> I thing we really need somethig like "-m malformed" so we can do tricks
> like: iptables -A FORWARD -m malformed -j DROP (to drop malformed packets)
> or: iptables -A FORWARD -m malformed -j ACCEPT (to stop further processing such packets)

Sure, adding the a corrupted test is useful for things such as logging,
etc.  However, requiring the firewall user to check for corruption
explicitly before any "-p tcp --tcp-blah" rules is just going to make
the iptables learning curve even higher, and 99% of us are going to miss
it.  Simply, I think it's broken.

So, how does the code structure of iptables work in this case?  Can the
checksum be added in a single place such that "-p tcp" alone will match
but any of --dport, --sport, --syn, --tcp-flags, --tcp-option, --mss,
multiport, etc?

Simon-

Re: REJECT using invalid data

[Simon Kirby]

On Fri, Dec 10, 2004 at 02:52:29PM +0100, Krzysztof Oledzki wrote:

> No, REJECT does not DROP. It sends back an error packet in response. So it
> sends response to broken packets.

Yes, but if REJECT can't REJECT it, it very well better DROP instead. 
Would you rather it ACCEPT it?

> Oh, but what if protocol filed is broken? With one bit error we can
> accidently match for example igmp packets with -p tcp.

"tcp", "udp", "igmp", etc., are in the IP header, which better be checked
by that point as well.  That is what I'm saying in my previous message. 
"-p tcp" by itself is matching in only the IP header, which has a
separate checksum.

> Not only for logging. You can use this to protect some hosts with
> broken IP stack like for example unpatched Win95/98, etc.

I'm not against a feature to actually match broken packets in any way. 
I'm just saying that not checking the checksum and skipping such packets
with the rule "-p tcp --dport 80" is wrong.

> So maybe broken packets should go a to different table (broken) and not
> traversal other tables and conntrack? Or any other smart solution.

Possibly.  The kernel would need to know all protocols in order to be
able to filter them before any other rules, though.

Let me make this very clear.  I am talking only about the bad checksum
case, and not about strange or "unclean" packets in any other way.

Would you ever really ever want to accept a packet with a corrupted
checksum?  Would it ever be useful?

Ideally, "-p tcp" could check just the checksums up to the IP level and
NOT the tcp checksum.  "-p tcp" with any other TCP-specific option would
then check the TCP checksum.

So, we could have an example ruleset like this that should work as
expected:

iptables -A FORWARD -d windows_box -m broken --broken-win -J DROP
iptables -A FORWARD -m unclean --some-unclean -m limit ... -J LOG
iptables -A FORWARD -p tcp --dport 80 -J ACCEPT
iptables -A FORWARD -p tcp -j REJECT --reject-with tcp-reset

The above rules would protect the Windows box, log corrupted packets,
accept TCP port 80 packets that don't have a corrupted TCP checksum,
REJECT any other valid TCP packet with a TCP RST, DROP invalid TCP
packets, and follow the policy for the rest.  Simple.

Simon-