[PATCH 0/4] netfilter patches for consideration

[PATCH 0/4] netfilter patches for consideration

[philipc]

Here's some miscellaneous patches I am using.  I am mostly looking
for feedback about how good/bad these patches are, but feel free
to apply them too if you think they are okay.

In particular, patch 3 should be made more general before applying,
and I disagree with patch 4 fixing the problem in the firewall,
but I have no choice in that.
--

[PATCH 1/4] TCPMSS: dont drop packets

[philipc]

[-- Attachment #1: tcpmssdrop.patch --]
[-- Type: text/plain, Size: 2093 bytes --]

MSS clamping should be best effort, since we are doing the clamping
to avoid dropped packets.  If we can't clamp, then forward it anyway
and hope PMTU discovery works.

The test case that caused problems was syn packet containing data.

Related questions:
This patch changes the behaviour when setting the MSS too, is this
acceptable?

Should skb_make_writable() and skb_checksum_help() cause dropped packets
if they fail?

Which mangle targets should drop packets if they fail?
MARK should, since the mark may be used in filter rules.
What about DSCP, ECN, TOS, TTL?

Signed-off-by: Philip Craig <philipc@snapgear.com>

Index: linux-2.6.17-rc4.orig/net/ipv4/netfilter/ipt_TCPMSS.c
===================================================================
--- linux-2.6.17-rc4.orig.orig/net/ipv4/netfilter/ipt_TCPMSS.c	2006-05-24 11:56:47.000000000 +1000
+++ linux-2.6.17-rc4.orig/net/ipv4/netfilter/ipt_TCPMSS.c	2006-05-24 12:59:32.000000000 +1000
@@ -81,7 +81,7 @@ ipt_tcpmss_target(struct sk_buff **pskb,
 			printk(KERN_ERR
 			       "ipt_tcpmss_target: bad length (%d bytes)\n",
 			       (*pskb)->len);
-		return NF_DROP;
+		return IPT_CONTINUE;
 	}
 
 	if(tcpmssinfo->mss == IPT_TCPMSS_CLAMP_PMTU) {
@@ -89,14 +89,14 @@ ipt_tcpmss_target(struct sk_buff **pskb,
 			if (net_ratelimit())
 				printk(KERN_ERR
 			       		"ipt_tcpmss_target: no dst?! can't determine path-MTU\n");
-			return NF_DROP; /* or IPT_CONTINUE ?? */
+			return IPT_CONTINUE;
 		}
 
 		if(dst_mtu((*pskb)->dst) <= (sizeof(struct iphdr) + sizeof(struct tcphdr))) {
 			if (net_ratelimit())
 				printk(KERN_ERR
 		       			"ipt_tcpmss_target: unknown or invalid path-MTU (%d)\n", dst_mtu((*pskb)->dst));
-			return NF_DROP; /* or IPT_CONTINUE ?? */
+			return IPT_CONTINUE;
 		}
 
 		newmss = dst_mtu((*pskb)->dst) - sizeof(struct iphdr) - sizeof(struct tcphdr);
@@ -147,7 +147,7 @@ ipt_tcpmss_target(struct sk_buff **pskb,
 			if (net_ratelimit())
 				printk(KERN_ERR "ipt_tcpmss_target:"
 				       " unable to allocate larger skb\n");
-			return NF_DROP;
+			return IPT_CONTINUE;
 		}
 
 		kfree_skb(*pskb);

--

Re: [PATCH 1/4] TCPMSS: dont drop packets

[Patrick McHardy]

philipc@snapgear.com wrote:
> MSS clamping should be best effort, since we are doing the clamping
> to avoid dropped packets.  If we can't clamp, then forward it anyway
> and hope PMTU discovery works.
> 
> The test case that caused problems was syn packet containing data.

That seems to be an invalid testcase.

> Related questions:
> This patch changes the behaviour when setting the MSS too, is this
> acceptable?
> 
> Should skb_make_writable() and skb_checksum_help() cause dropped packets
> if they fail?
> 
> Which mangle targets should drop packets if they fail?
> MARK should, since the mark may be used in filter rules.
> What about DSCP, ECN, TOS, TTL?

I don't think its a good idea to change this long standing behaviour.
The next incarnation of iptables will support user-supplied verdicts
for non-terminal targets, but until then it seems reasonable to say
"user said do x, can't do it, so drop".

It should be noted that most of these conditions can't be true anyway,
I have a half-finished patch to remove most of them.

Re: [PATCH 1/4] TCPMSS: dont drop packets

[Philip Craig]

On 05/25/2006 02:16 AM, Patrick McHardy wrote:
> philipc@snapgear.com wrote:
>> The test case that caused problems was syn packet containing data.
> 
> That seems to be an invalid testcase.

I'm not sure if any stacks send or accept this in practice, but it is
allowed by RFC 793, see second paragraph in section 3.4.  Please correct
me if this is no longer valid.


> I don't think its a good idea to change this long standing behaviour.
> The next incarnation of iptables will support user-supplied verdicts
> for non-terminal targets, but until then it seems reasonable to say
> "user said do x, can't do it, so drop".

Sounds reasonable to me.

Re: [PATCH 1/4] TCPMSS: dont drop packets

[Patrick McHardy]

Philip Craig wrote:
> On 05/25/2006 02:16 AM, Patrick McHardy wrote:
> 
>>philipc@snapgear.com wrote:
>>
>>>The test case that caused problems was syn packet containing data.
>>
>>That seems to be an invalid testcase.
> 
> 
> I'm not sure if any stacks send or accept this in practice, but it is
> allowed by RFC 793, see second paragraph in section 3.4.  Please correct
> me if this is no longer valid.


No, you seem to be right. So I guess the entire check should be removed.
I'll look into this when finishing my other TCPMSS pathces.

[PATCH 2/4] TCPMSS: clamp to input interface MTU too

[philipc]

[-- Attachment #1: tcpmssin.patch --]
[-- Type: text/plain, Size: 2017 bytes --]

Ideally we would clamp the MSS based on the PMTU from src to dst.
Currently we use the PMTU from the packet filter to the dst.
This patch better approximates the full PTMU by using the input
interface MTU too.

Signed-off-by: Philip Craig <philipc@snapgear.com>

Index: linux-2.6.17-rc4.orig/net/ipv4/netfilter/ipt_TCPMSS.c
===================================================================
--- linux-2.6.17-rc4.orig.orig/net/ipv4/netfilter/ipt_TCPMSS.c	2006-05-24 11:57:22.000000000 +1000
+++ linux-2.6.17-rc4.orig/net/ipv4/netfilter/ipt_TCPMSS.c	2006-05-24 11:57:23.000000000 +1000
@@ -27,6 +27,8 @@ MODULE_DESCRIPTION("iptables TCP MSS mod
 #define DEBUGP(format, args...)
 #endif
 
+#define HDRSIZE (sizeof(struct iphdr) + sizeof(struct tcphdr))
+
 static u_int16_t
 cheat_check(u_int32_t oldvalinv, u_int32_t newval, u_int16_t oldcheck)
 {
@@ -55,7 +57,7 @@ ipt_tcpmss_target(struct sk_buff **pskb,
 	const struct ipt_tcpmss_info *tcpmssinfo = targinfo;
 	struct tcphdr *tcph;
 	struct iphdr *iph;
-	u_int16_t tcplen, newtotlen, oldval, newmss;
+	u_int16_t tcplen, newtotlen, oldval, newmss, mtu;
 	unsigned int i;
 	u_int8_t *opt;
 
@@ -92,14 +94,27 @@ ipt_tcpmss_target(struct sk_buff **pskb,
 			return IPT_CONTINUE;
 		}
 
-		if(dst_mtu((*pskb)->dst) <= (sizeof(struct iphdr) + sizeof(struct tcphdr))) {
+		mtu = dst_mtu((*pskb)->dst);
+		if (mtu <= HDRSIZE) {
 			if (net_ratelimit())
 				printk(KERN_ERR
-		       			"ipt_tcpmss_target: unknown or invalid path-MTU (%d)\n", dst_mtu((*pskb)->dst));
+		       			"ipt_tcpmss_target: unknown or "
+					"invalid path-MTU (%d)\n", mtu);
 			return IPT_CONTINUE;
 		}
 
-		newmss = dst_mtu((*pskb)->dst) - sizeof(struct iphdr) - sizeof(struct tcphdr);
+		if (in && in->mtu < mtu) {
+			mtu = in->mtu;
+			if (mtu <= HDRSIZE) {
+				if (net_ratelimit())
+					printk(KERN_ERR
+						"ipt_tcpmss_target: invalid "
+						"interface MTU (%d)\n", mtu);
+				return IPT_CONTINUE;
+			}
+		}
+
+		newmss = mtu - HDRSIZE;
 	} else
 		newmss = tcpmssinfo->mss;
 

--

Re: [PATCH 2/4] TCPMSS: clamp to input interface MTU too

[Patrick McHardy]

philipc@snapgear.com wrote:
> Ideally we would clamp the MSS based on the PMTU from src to dst.
> Currently we use the PMTU from the packet filter to the dst.
> This patch better approximates the full PTMU by using the input
> interface MTU too.


Also one of the things I have a half-finished patch for :)
But it (optionally) does a routing lookup for the source
to get a better estimate than the interface MTU.

[PATCH 3/4] log dropped ICMP redirects

[philipc]

[-- Attachment #1: invalidicmpredirect.patch --]
[-- Type: text/plain, Size: 1393 bytes --]

Log dropped ICMP redirects for untracked connections.

This was required to meet a certification test.  But there are
many more locations that can drop packets without logging them,
which weren't tested for certification.  Should all instances of
NF_DROP in netfilter/iptables have a call to nf_log_packet?

Signed-off-by: Philip Craig <philipc@snapgear.com>

Index: linux-2.6.17-rc4.orig/net/ipv4/netfilter/ip_nat_standalone.c
===================================================================
--- linux-2.6.17-rc4.orig.orig/net/ipv4/netfilter/ip_nat_standalone.c	2006-05-24 11:57:28.000000000 +1000
+++ linux-2.6.17-rc4.orig/net/ipv4/netfilter/ip_nat_standalone.c	2006-05-24 13:22:38.000000000 +1000
@@ -41,6 +41,7 @@
 #include <linux/netfilter_ipv4/ip_nat_helper.h>
 #include <linux/netfilter_ipv4/ip_tables.h>
 #include <linux/netfilter_ipv4/ip_conntrack_core.h>
+#include <linux/netfilter_ipv4/ip_conntrack_protocol.h>
 #include <linux/netfilter_ipv4/listhelp.h>
 
 #if 0
@@ -132,8 +133,13 @@ ip_nat_fn(unsigned int hooknum,
 						(*pskb)->nh.iph->ihl*4,
 						sizeof(_hdr), &_hdr);
 			if (hp != NULL &&
-			    hp->type == ICMP_REDIRECT)
+			    hp->type == ICMP_REDIRECT) {
+				if (LOG_INVALID(IPPROTO_ICMP))
+					nf_log_packet(PF_INET, 0, *pskb,
+						NULL, NULL, NULL, "ip_nat: "
+						"untracked ICMP redirect ");
 				return NF_DROP;
+			}
 		}
 		return NF_ACCEPT;
 	}

--

Re: [PATCH 3/4] log dropped ICMP redirects

[Patrick McHardy]

philipc@snapgear.com wrote:
> Log dropped ICMP redirects for untracked connections.
> 
> This was required to meet a certification test.  But there are
> many more locations that can drop packets without logging them,
> which weren't tested for certification.  Should all instances of
> NF_DROP in netfilter/iptables have a call to nf_log_packet?


There was a patch called "dropped table" once that hooked in kfree_skb.
I'm also not completely satisfied with the current situation .. but
I don't think adding excessive packet logging is the best solution,
often drops just occur because of failed validity checks which can
just as well happen outside of the netfilter code.

In this case I'm actually having trouble understanding what the code
is supposed to do, why would we receive an ICMP redirect for a
unhashed connection (unhashed == first packet hasn't even left the
box yet)?

> @@ -132,8 +133,13 @@ ip_nat_fn(unsigned int hooknum,
>  						(*pskb)->nh.iph->ihl*4,
>  						sizeof(_hdr), &_hdr);
>  			if (hp != NULL &&
> -			    hp->type == ICMP_REDIRECT)
> +			    hp->type == ICMP_REDIRECT) {
> +				if (LOG_INVALID(IPPROTO_ICMP))
> +					nf_log_packet(PF_INET, 0, *pskb,
> +						NULL, NULL, NULL, "ip_nat: "
> +						"untracked ICMP redirect ");
>  				return NF_DROP;
> +			}
>  		}
>  		return NF_ACCEPT;
>  	}
> 
> --
> 
> 

Re: [PATCH 3/4] log dropped ICMP redirects

[Philip Craig]

On 05/25/2006 02:26 AM, Patrick McHardy wrote:
> There was a patch called "dropped table" once that hooked in kfree_skb.
> I'm also not completely satisfied with the current situation .. but
> I don't think adding excessive packet logging is the best solution,
> often drops just occur because of failed validity checks which can
> just as well happen outside of the netfilter code.

I'm not sure what checks occur outside of netfilter code, but I assume
they are mostly things that would be dropped by any router, not just
a firewall?

For connection tracking, the goal should be to never drop anything,
and instead mark it as invalid.  My first version of this patch
just removed this test completely, so then I could log it with an
iptables rule before dropping it.  But I didn't think that was suitable
for general use.

For other validity checks within netfilter, this is a possible use for
the unclean match.   This would allow you to use rules to do the
checks, log the packet, and drop it.  And if this was done in the raw
table, then it could cover some of the connection tracking checks too.
What was the problem with the unclean match, did it simply check too much?

> In this case I'm actually having trouble understanding what the code
> is supposed to do, why would we receive an ICMP redirect for a
> unhashed connection (unhashed == first packet hasn't even left the
> box yet)?

I don't understand the reason for the code either.

Re: [PATCH 3/4] log dropped ICMP redirects

[Patrick McHardy]

Philip Craig wrote:
> On 05/25/2006 02:26 AM, Patrick McHardy wrote:
> 
>>There was a patch called "dropped table" once that hooked in kfree_skb.
>>I'm also not completely satisfied with the current situation .. but
>>I don't think adding excessive packet logging is the best solution,
>>often drops just occur because of failed validity checks which can
>>just as well happen outside of the netfilter code.
> 
> 
> I'm not sure what checks occur outside of netfilter code, but I assume
> they are mostly things that would be dropped by any router, not just
> a firewall?

In most cases yes.

> For connection tracking, the goal should be to never drop anything,
> and instead mark it as invalid.  My first version of this patch
> just removed this test completely, so then I could log it with an
> iptables rule before dropping it.  But I didn't think that was suitable
> for general use.

For invalid checks, it is :) But if you can already log it before it
gets dropped, why remove the check?


> For other validity checks within netfilter, this is a possible use for
> the unclean match.   This would allow you to use rules to do the
> checks, log the packet, and drop it.  And if this was done in the raw
> table, then it could cover some of the connection tracking checks too.
> What was the problem with the unclean match, did it simply check too much?

The main reason is that is was very strict, so any future enhancements
of protocols would probably be detected as invalid. This is a big
problem, think of ECN for example. But it would only cover a subset
of the possible reasons anyway.

Bottom line: I can't see a good solution for this problem right now.

>>In this case I'm actually having trouble understanding what the code
>>is supposed to do, why would we receive an ICMP redirect for a
>>unhashed connection (unhashed == first packet hasn't even left the
>>box yet)?
> 
> 
> I don't understand the reason for the code either.


I think I do now. It is meant to drop locally generated ICMP redirects
in cases where the source and NATed destination are on the same network.
The condition can't be true anymore since we started attaching conntrack
references to locally generated ICMP packets, which suggests that we
need to do this somewhere else.

[PATCH 4/4] drop ftp bounce attacks

[philipc]

[-- Attachment #1: dropftpbounce.patch --]
[-- Type: text/plain, Size: 1671 bytes --]

FTP bounce attacks work by specifying a different IP address in
the PORT command for active mode.  This causes the FTP server to
open a connection to another machine.

The best solution for this problem is to fix the FTP server.
This is a well known problem, and all major FTP servers should
have been fixed.

An alternative solution is to drop the packet in connection tracking.  
Dropping packets isn't the intended use of connection tracking,
but creating a new match to do this seems inefficient.

Signed-off-by: Philip Craig <philipc@snapgear.com>

Index: linux-2.6.17-rc4.orig/net/ipv4/netfilter/ip_conntrack_ftp.c
===================================================================
--- linux-2.6.17-rc4.orig.orig/net/ipv4/netfilter/ip_conntrack_ftp.c	2006-05-24 11:57:28.000000000 +1000
+++ linux-2.6.17-rc4.orig/net/ipv4/netfilter/ip_conntrack_ftp.c	2006-05-24 13:09:44.000000000 +1000
@@ -405,8 +405,14 @@ static int help(struct sk_buff **pskb,
 		   problem (DMZ machines opening holes to internal
 		   networks, or the packet filter itself). */
 		if (!loose) {
-			ret = NF_ACCEPT;
-			goto out_put_expect;
+			if (net_ratelimit())
+				printk("conntrack_ftp: ip mismatch: "
+				       "%u,%u,%u,%u != %u.%u.%u.%u\n",
+				       array[0], array[1], array[2], array[3],
+				       NIPQUAD(ct->tuplehash[dir].tuple.src.ip));
+			ret = NF_DROP;
+			ip_conntrack_expect_put(exp);
+			goto out;
 		}
 		exp->tuple.dst.ip = htonl((array[0] << 24) | (array[1] << 16)
 					 | (array[2] << 8) | array[3]);
@@ -436,7 +442,6 @@ static int help(struct sk_buff **pskb,
 			ret = NF_ACCEPT;
 	}
 
-out_put_expect:
 	ip_conntrack_expect_put(exp);
 
 out_update_nl:

--

Re: [PATCH 4/4] drop ftp bounce attacks

[Patrick McHardy]

philipc@snapgear.com wrote:
> FTP bounce attacks work by specifying a different IP address in
> the PORT command for active mode.  This causes the FTP server to
> open a connection to another machine.
> 
> The best solution for this problem is to fix the FTP server.
> This is a well known problem, and all major FTP servers should
> have been fixed.
> 
> An alternative solution is to drop the packet in connection tracking.  
> Dropping packets isn't the intended use of connection tracking,
> but creating a new match to do this seems inefficient.

The best solution would be to mark the packet INVALID and let the
user decide using the state match. But that isn't possible in the
current infrastructure anymore because helpers get called last.
Just dropping is also not really nice, rejecting would be better
IMO. But again that is a policy decision that shouldn't be hard-coded,
I actually don't want any helper to drop packets on my firewall.
I think we really want a more generic solution.


> Signed-off-by: Philip Craig <philipc@snapgear.com>
> 
> Index: linux-2.6.17-rc4.orig/net/ipv4/netfilter/ip_conntrack_ftp.c
> ===================================================================
> --- linux-2.6.17-rc4.orig.orig/net/ipv4/netfilter/ip_conntrack_ftp.c	2006-05-24 11:57:28.000000000 +1000
> +++ linux-2.6.17-rc4.orig/net/ipv4/netfilter/ip_conntrack_ftp.c	2006-05-24 13:09:44.000000000 +1000
> @@ -405,8 +405,14 @@ static int help(struct sk_buff **pskb,
>  		   problem (DMZ machines opening holes to internal
>  		   networks, or the packet filter itself). */
>  		if (!loose) {
> -			ret = NF_ACCEPT;
> -			goto out_put_expect;
> +			if (net_ratelimit())
> +				printk("conntrack_ftp: ip mismatch: "
> +				       "%u,%u,%u,%u != %u.%u.%u.%u\n",
> +				       array[0], array[1], array[2], array[3],
> +				       NIPQUAD(ct->tuplehash[dir].tuple.src.ip));
> +			ret = NF_DROP;
> +			ip_conntrack_expect_put(exp);
> +			goto out;
>  		}
>  		exp->tuple.dst.ip = htonl((array[0] << 24) | (array[1] << 16)
>  					 | (array[2] << 8) | array[3]);
> @@ -436,7 +442,6 @@ static int help(struct sk_buff **pskb,
>  			ret = NF_ACCEPT;
>  	}
>  
> -out_put_expect:
>  	ip_conntrack_expect_put(exp);
>  
>  out_update_nl:
> 
> --
> 
> 

Re: [PATCH 4/4] drop ftp bounce attacks

[Philip Craig]

On 05/25/2006 02:31 AM, Patrick McHardy wrote:
> The best solution would be to mark the packet INVALID and let the
> user decide using the state match.

Marking the packet INVALID is definitely better than always dropping
it.

The reason I said fixing the ftp server is the best solution is
that I'm not sure whether the ftp conntrack module can match on
all possible port commands, taking into account fragmentation,
reordering, and retransmits.

This is different from attacks against the firewall, in which the
attack works only if the firewall recognizes the port command and
creates an expectation.

Re: [PATCH 4/4] drop ftp bounce attacks

[Patrick McHardy]

Philip Craig wrote:
> On 05/25/2006 02:31 AM, Patrick McHardy wrote:
> 
>>The best solution would be to mark the packet INVALID and let the
>>user decide using the state match.
> 
> 
> Marking the packet INVALID is definitely better than always dropping
> it.
> 
> The reason I said fixing the ftp server is the best solution is
> that I'm not sure whether the ftp conntrack module can match on
> all possible port commands, taking into account fragmentation,
> reordering, and retransmits.
> 
> This is different from attacks against the firewall, in which the
> attack works only if the firewall recognizes the port command and
> creates an expectation.


Absolutely. I actually made a quite similar patch because of the
same reason (certification requirements), but in the end I don't
think its very useful for the reasons you mention above and because
this is absolutely not what connection tracking helpers are meant
for, people should use application layer proxies for this.