All of lore.kernel.org
 help / color / mirror / Atom feed
* bugs in ftp conntrack
@ 2007-05-22  6:24 YU, Haitao
  2007-05-24 18:16 ` Patrick McHardy
  0 siblings, 1 reply; 8+ messages in thread
From: YU, Haitao @ 2007-05-22  6:24 UTC (permalink / raw)
  To: netfilter-devel

Hi,


If the order of ftp packets are wrong, function find_nl_seq() in
net/ipv4/netfilter/ip_conntrack_ftp.c will make mistake.

i.e., consider three ftp packets: "port", "list" and "noop", 
if the "list" and "noop" packets reach firewall before "port" packet,
then info->seq_aft_nl will record  the sequence of "noop". Kenerl will
not parse "port" packet because the seq does not match the recored one .

If kernel can't trace expect connection, then the attack described in
[phrack-63, 0x13] will happen.


Another problem is if the packet length is changed bye NAT, then the
next packet will not be parsed. So kernel can not parse the 2nd "port"
packet of two continual "port" packets. Though it's impossible in legal
ftp connection, and I also don't know how to use this to hack firewall.


Third, the value of "oldest" in function udpate_bl_seq() seem unchanged
after four packets.


Regards,

YU, haitao

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: bugs in ftp conntrack
  2007-05-22  6:24 bugs in ftp conntrack YU, Haitao
@ 2007-05-24 18:16 ` Patrick McHardy
  0 siblings, 0 replies; 8+ messages in thread
From: Patrick McHardy @ 2007-05-24 18:16 UTC (permalink / raw)
  To: YU, Haitao; +Cc: netfilter-devel

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

YU, Haitao wrote:
> If the order of ftp packets are wrong, function find_nl_seq() in
> net/ipv4/netfilter/ip_conntrack_ftp.c will make mistake.
> 
> i.e., consider three ftp packets: "port", "list" and "noop", 
> if the "list" and "noop" packets reach firewall before "port" packet,
> then info->seq_aft_nl will record  the sequence of "noop". Kenerl will
> not parse "port" packet because the seq does not match the recored one .


There's not much we can do about this case, we already keep a history
of sequence numbers. As you note below that code is pretty broken
currently, but I have patches queued to fix it (attached to this mail).

> If kernel can't trace expect connection, then the attack described in
> [phrack-63, 0x13] will happen.


Shouldn't be, we don't assign helpers to expected connections except
when the first helper explicitly want us to.

> Another problem is if the packet length is changed bye NAT, then the
> next packet will not be parsed. So kernel can not parse the 2nd "port"
> packet of two continual "port" packets. Though it's impossible in legal
> ftp connection, and I also don't know how to use this to hack firewall.


Good catch, that is broken since about 2.6.11.

> Third, the value of "oldest" in function udpate_bl_seq() seem unchanged
> after four packets.


Its changes randomly because the saved sequence number is compared to
the index of the oldest entry.


[-- Attachment #2: 01.diff --]
[-- Type: text/plain, Size: 1933 bytes --]

[NETFILTER]: nf_conntrack_ftp: fix newline sequence number update

When trying to locate the oldest entry in the history of newline character
sequence numbers, the sequence number of the current entry is incorrectly
compared with the index of the oldest sequence number instead of the number
itself.

Additionally it is not made sure that the current sequence number really
is after the oldest known one.

Based on report by YU, Haitao <yuhaitao@tsinghua.org.cn>

Signed-off-by: Patrick McHardy <kaber@trash.net>

---
commit ce7fed0c554b66ea7ea36abb91d69fa74fa3a3e2
tree 156e9de9fc8105096623d98506b0c563f1eefbca
parent ff33b5e57721b1f2a7c8b9fcef32acdb55816131
author Patrick McHardy <kaber@trash.net> Thu, 24 May 2007 20:05:19 +0200
committer Patrick McHardy <kaber@trash.net> Thu, 24 May 2007 20:05:19 +0200

 net/netfilter/nf_conntrack_ftp.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/net/netfilter/nf_conntrack_ftp.c b/net/netfilter/nf_conntrack_ftp.c
index a186799..3357642 100644
--- a/net/netfilter/nf_conntrack_ftp.c
+++ b/net/netfilter/nf_conntrack_ftp.c
@@ -335,15 +335,17 @@ static void update_nl_seq(u32 nl_seq, struct nf_ct_ftp_master *info, int dir,
 		if (info->seq_aft_nl[dir][i] == nl_seq)
 			return;
 
-		if (oldest == info->seq_aft_nl_num[dir]
-		    || before(info->seq_aft_nl[dir][i], oldest))
+		if (oldest == info->seq_aft_nl_num[dir] ||
+		    before(info->seq_aft_nl[dir][i],
+		    	   info->seq_aft_nl[dir][oldest]))
 			oldest = i;
 	}
 
 	if (info->seq_aft_nl_num[dir] < NUM_SEQ_TO_REMEMBER) {
 		info->seq_aft_nl[dir][info->seq_aft_nl_num[dir]++] = nl_seq;
 		nf_conntrack_event_cache(IPCT_HELPINFO_VOLATILE, skb);
-	} else if (oldest != NUM_SEQ_TO_REMEMBER) {
+	} else if (oldest != NUM_SEQ_TO_REMEMBER &&
+		   after(nl_seq, info->seq_aft_nl[dir][oldest])) {
 		info->seq_aft_nl[dir][oldest] = nl_seq;
 		nf_conntrack_event_cache(IPCT_HELPINFO_VOLATILE, skb);
 	}

[-- Attachment #3: 02.diff --]
[-- Type: text/plain, Size: 5271 bytes --]

[NETFILTER]: nf_conntrack_ftp: fix newline sequence number calculation

When the packet size is changed by the FTP NAT helper, the connection
tracking helper adjusts the sequence number of the newline character
by the size difference. This is wrong because NAT sequence number
adjustment happens after helpers are called, so the unadjusted number
is compared to the already adjusted one.

Based on report by YU, Haitao <yuhaitao@tsinghua.org.cn>

Signed-off-by: Patrick McHardy <kaber@trash.net>

---
commit 4aa8feb60ca94d73f6ebb186b088e78970e2dcc7
tree 3245b16e02ec807d8535ad3e84c9fa974ba68e85
parent ce7fed0c554b66ea7ea36abb91d69fa74fa3a3e2
author Patrick McHardy <kaber@trash.net> Thu, 24 May 2007 20:05:57 +0200
committer Patrick McHardy <kaber@trash.net> Thu, 24 May 2007 20:05:57 +0200

 include/linux/netfilter/nf_conntrack_ftp.h |    3 +--
 net/ipv4/netfilter/nf_nat_ftp.c            |   20 ++++++--------------
 net/netfilter/nf_conntrack_ftp.c           |    5 ++---
 3 files changed, 9 insertions(+), 19 deletions(-)

diff --git a/include/linux/netfilter/nf_conntrack_ftp.h b/include/linux/netfilter/nf_conntrack_ftp.h
index 81453ea..b7c360f 100644
--- a/include/linux/netfilter/nf_conntrack_ftp.h
+++ b/include/linux/netfilter/nf_conntrack_ftp.h
@@ -37,8 +37,7 @@ extern unsigned int (*nf_nat_ftp_hook)(struct sk_buff **pskb,
 				       enum nf_ct_ftp_type type,
 				       unsigned int matchoff,
 				       unsigned int matchlen,
-				       struct nf_conntrack_expect *exp,
-				       u32 *seq);
+				       struct nf_conntrack_expect *exp);
 #endif /* __KERNEL__ */
 
 #endif /* _NF_CONNTRACK_FTP_H */
diff --git a/net/ipv4/netfilter/nf_nat_ftp.c b/net/ipv4/netfilter/nf_nat_ftp.c
index 751b598..e6bc8e5 100644
--- a/net/ipv4/netfilter/nf_nat_ftp.c
+++ b/net/ipv4/netfilter/nf_nat_ftp.c
@@ -40,8 +40,7 @@ mangle_rfc959_packet(struct sk_buff **pskb,
 		     unsigned int matchoff,
 		     unsigned int matchlen,
 		     struct nf_conn *ct,
-		     enum ip_conntrack_info ctinfo,
-		     u32 *seq)
+		     enum ip_conntrack_info ctinfo)
 {
 	char buffer[sizeof("nnn,nnn,nnn,nnn,nnn,nnn")];
 
@@ -50,7 +49,6 @@ mangle_rfc959_packet(struct sk_buff **pskb,
 
 	DEBUGP("calling nf_nat_mangle_tcp_packet\n");
 
-	*seq += strlen(buffer) - matchlen;
 	return nf_nat_mangle_tcp_packet(pskb, ct, ctinfo, matchoff,
 					matchlen, buffer, strlen(buffer));
 }
@@ -63,8 +61,7 @@ mangle_eprt_packet(struct sk_buff **pskb,
 		   unsigned int matchoff,
 		   unsigned int matchlen,
 		   struct nf_conn *ct,
-		   enum ip_conntrack_info ctinfo,
-		   u32 *seq)
+		   enum ip_conntrack_info ctinfo)
 {
 	char buffer[sizeof("|1|255.255.255.255|65535|")];
 
@@ -72,7 +69,6 @@ mangle_eprt_packet(struct sk_buff **pskb,
 
 	DEBUGP("calling nf_nat_mangle_tcp_packet\n");
 
-	*seq += strlen(buffer) - matchlen;
 	return nf_nat_mangle_tcp_packet(pskb, ct, ctinfo, matchoff,
 					matchlen, buffer, strlen(buffer));
 }
@@ -85,8 +81,7 @@ mangle_epsv_packet(struct sk_buff **pskb,
 		   unsigned int matchoff,
 		   unsigned int matchlen,
 		   struct nf_conn *ct,
-		   enum ip_conntrack_info ctinfo,
-		   u32 *seq)
+		   enum ip_conntrack_info ctinfo)
 {
 	char buffer[sizeof("|||65535|")];
 
@@ -94,14 +89,13 @@ mangle_epsv_packet(struct sk_buff **pskb,
 
 	DEBUGP("calling nf_nat_mangle_tcp_packet\n");
 
-	*seq += strlen(buffer) - matchlen;
 	return nf_nat_mangle_tcp_packet(pskb, ct, ctinfo, matchoff,
 					matchlen, buffer, strlen(buffer));
 }
 
 static int (*mangle[])(struct sk_buff **, __be32, u_int16_t,
 		       unsigned int, unsigned int, struct nf_conn *,
-		       enum ip_conntrack_info, u32 *seq)
+		       enum ip_conntrack_info)
 = {
 	[NF_CT_FTP_PORT] = mangle_rfc959_packet,
 	[NF_CT_FTP_PASV] = mangle_rfc959_packet,
@@ -116,8 +110,7 @@ static unsigned int nf_nat_ftp(struct sk_buff **pskb,
 			       enum nf_ct_ftp_type type,
 			       unsigned int matchoff,
 			       unsigned int matchlen,
-			       struct nf_conntrack_expect *exp,
-			       u32 *seq)
+			       struct nf_conntrack_expect *exp)
 {
 	__be32 newip;
 	u_int16_t port;
@@ -145,8 +138,7 @@ static unsigned int nf_nat_ftp(struct sk_buff **pskb,
 	if (port == 0)
 		return NF_DROP;
 
-	if (!mangle[type](pskb, newip, port, matchoff, matchlen, ct, ctinfo,
-			  seq)) {
+	if (!mangle[type](pskb, newip, port, matchoff, matchlen, ct, ctinfo)) {
 		nf_conntrack_unexpect_related(exp);
 		return NF_DROP;
 	}
diff --git a/net/netfilter/nf_conntrack_ftp.c b/net/netfilter/nf_conntrack_ftp.c
index 3357642..09add2f 100644
--- a/net/netfilter/nf_conntrack_ftp.c
+++ b/net/netfilter/nf_conntrack_ftp.c
@@ -48,8 +48,7 @@ unsigned int (*nf_nat_ftp_hook)(struct sk_buff **pskb,
 				enum nf_ct_ftp_type type,
 				unsigned int matchoff,
 				unsigned int matchlen,
-				struct nf_conntrack_expect *exp,
-				u32 *seq);
+				struct nf_conntrack_expect *exp);
 EXPORT_SYMBOL_GPL(nf_nat_ftp_hook);
 
 #if 0
@@ -521,7 +520,7 @@ static int help(struct sk_buff **pskb,
 	nf_nat_ftp = rcu_dereference(nf_nat_ftp_hook);
 	if (nf_nat_ftp && ct->status & IPS_NAT_MASK)
 		ret = nf_nat_ftp(pskb, ctinfo, search[dir][i].ftptype,
-				 matchoff, matchlen, exp, &seq);
+				 matchoff, matchlen, exp);
 	else {
 		/* Can't expect this?  Best to drop packet now. */
 		if (nf_conntrack_expect_related(exp) != 0)

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: bugs in ftp conntrack
@ 2007-05-25  1:10 YU, Haitao
  2007-05-26  3:28 ` Henrik Nordstrom
  2007-05-26  9:03 ` Patrick McHardy
  0 siblings, 2 replies; 8+ messages in thread
From: YU, Haitao @ 2007-05-25  1:10 UTC (permalink / raw)
  To: Patrick McHardy; +Cc: netfilter-devel



In your mail:
>From: Patrick McHardy <kaber@trash.net>
>Reply-To: 
>To: "YU, Haitao" <yuhaitao@tsinghua.org.cn>
>Subject: Re: bugs in ftp conntrack
>Date:Thu, 24 May 2007 20:16:05 +0200
>
>
>There's not much we can do about this case, we already keep a history
>of sequence numbers. As you note below that code is pretty broken
>currently, but I have patches queued to fix it (attached to this mail).
>


I think if it's possible that we don't record the new seqence of too new packets.
"Too new packets" means the sequence of the packet is greater than all
seq_aft_nl[] values.  We just wait until another "port", "227", etc. packet is
parsed correctly.


So the return value of function find_nl_seq() shoule be three: 
1: too new, then parse pattern, only if found > 0,  then update seq, else keep
current seq_aft_nl;
0: match one of the remembered seq, then parse pattern, update seq when found >=
0;
-1: too old(less than all remembered seq),  just goto out, (not goto
out_update_nl)


Thanks,

YU, Haitao

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: bugs in ftp conntrack
  2007-05-25  1:10 YU, Haitao
@ 2007-05-26  3:28 ` Henrik Nordstrom
  2007-05-26  9:07   ` Patrick McHardy
  2007-05-26  9:03 ` Patrick McHardy
  1 sibling, 1 reply; 8+ messages in thread
From: Henrik Nordstrom @ 2007-05-26  3:28 UTC (permalink / raw)
  To: YU, Haitao; +Cc: netfilter-devel, Patrick McHardy

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

fre 2007-05-25 klockan 09:10 +0800 skrev YU, Haitao:

> I think if it's possible that we don't record the new seqence of too new packets.
> "Too new packets" means the sequence of the packet is greater than all
> seq_aft_nl[] values.  We just wait until another "port", "227", etc. packet is
> parsed correctly.

The best (save for doing a full TCP stream reassembly or proxying) would
be to drop such out-of-order packets, relying on the sender to
retransmit them later..  Might result in quite inefficient
communication, but will make the protocol parsing behave correctly.

Regards
Henrik

[-- Attachment #2: Detta är en digitalt signerad meddelandedel --]
[-- Type: application/pgp-signature, Size: 307 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: bugs in ftp conntrack
  2007-05-25  1:10 YU, Haitao
  2007-05-26  3:28 ` Henrik Nordstrom
@ 2007-05-26  9:03 ` Patrick McHardy
  1 sibling, 0 replies; 8+ messages in thread
From: Patrick McHardy @ 2007-05-26  9:03 UTC (permalink / raw)
  To: YU, Haitao; +Cc: netfilter-devel

YU, Haitao wrote:
>>From: Patrick McHardy <kaber@trash.net>
>>
>>There's not much we can do about this case, we already keep a history
>>of sequence numbers. As you note below that code is pretty broken
>>currently, but I have patches queued to fix it (attached to this mail).
>>
> 
> 
> 
> I think if it's possible that we don't record the new seqence of too new packets.
> "Too new packets" means the sequence of the packet is greater than all
> seq_aft_nl[] values.  We just wait until another "port", "227", etc. packet is
> parsed correctly.
> 
> 
> So the return value of function find_nl_seq() shoule be three: 
> 1: too new, then parse pattern, only if found > 0,  then update seq, else keep
> current seq_aft_nl;
> 0: match one of the remembered seq, then parse pattern, update seq when found >=
> 0;
> -1: too old(less than all remembered seq),  just goto out, (not goto
> out_update_nl)


Would you mind sending a patch to demonstrate your idea?

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: bugs in ftp conntrack
  2007-05-26  3:28 ` Henrik Nordstrom
@ 2007-05-26  9:07   ` Patrick McHardy
  0 siblings, 0 replies; 8+ messages in thread
From: Patrick McHardy @ 2007-05-26  9:07 UTC (permalink / raw)
  To: Henrik Nordstrom; +Cc: YU, Haitao, netfilter-devel

Henrik Nordstrom wrote:
> fre 2007-05-25 klockan 09:10 +0800 skrev YU, Haitao:
> 
> 
>>I think if it's possible that we don't record the new seqence of too new packets.
>>"Too new packets" means the sequence of the packet is greater than all
>>seq_aft_nl[] values.  We just wait until another "port", "227", etc. packet is
>>parsed correctly.
> 
> 
> The best (save for doing a full TCP stream reassembly or proxying) would
> be to drop such out-of-order packets, relying on the sender to
> retransmit them later..  Might result in quite inefficient
> communication, but will make the protocol parsing behave correctly.


That also sounds like a good idea. We shouldn't see any reordering
in FTP command streams since clients usually wait for an reply before
sending the next command (at least I believe so, or is it always?),
so I'm not too worried about inefficient communication.

That would also allow to find silly bugs in newline sequence number
tracking faster :)

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: bugs in ftp conntrack
@ 2007-05-27  1:35 YU, Haitao
  0 siblings, 0 replies; 8+ messages in thread
From: YU, Haitao @ 2007-05-27  1:35 UTC (permalink / raw)
  To: Patrick McHardy; +Cc: netfilter-devel




In your mail:
>From: Patrick McHardy <kaber@trash.net>
>Reply-To: 
>To: "YU, Haitao" <yuhaitao@tsinghua.org.cn>
>Subject: Re: bugs in ftp conntrack
>Date:Sat, 26 May 2007 11:03:35 +0200
>
>YU, Haitao wrote:
>>>From: Patrick McHardy <kaber@trash.net>
>>>
>>>There's not much we can do about this case, we already keep a history
>>>of sequence numbers. As you note below that code is pretty broken
>>>currently, but I have patches queued to fix it (attached to this mail).
>>>
>> 
>> 
>> 
>> I think if it's possible that we don't record the new seqence of too new
packets.
>> "Too new packets" means the sequence of the packet is greater than all
>> seq_aft_nl[] values.  We just wait until another "port", "227", etc. packet is
>> parsed correctly.
>> 
>> 
>> So the return value of function find_nl_seq() shoule be three: 
>> 1: too new, then parse pattern, only if found > 0,  then update seq, else keep
>> current seq_aft_nl;
>> 0: match one of the remembered seq, then parse pattern, update seq when found
>=
>> 0;
>> -1: too old(less than all remembered seq),  just goto out, (not goto
>> out_update_nl)
>
>
>Would you mind sending a patch to demonstrate your idea?
>
>
> 

patch for 2.4.21.1

--- linux-2.4.21.1/net/ipv4/netfilter/ip_conntrack_ftp.c	2007-04-28
05:49:26.000000000 +0800
+++ linux-2.4.21.1-new/net/ipv4/netfilter/ip_conntrack_ftp.c	2007-05-27
09:31:35.000000000 +0800
@@ -263,10 +263,19 @@
 static int find_nl_seq(u32 seq, const struct ip_ct_ftp_master *info, int dir)
 {
 	unsigned int i;
+	int old=0, new=0;
 
-	for (i = 0; i < info->seq_aft_nl_num[dir]; i++)
+	for (i = 0; i < info->seq_aft_nl_num[dir]; i++){
 		if (info->seq_aft_nl[dir][i] == seq)
-			return 1;
+			return 0;
+		else if(before(seq, info->seq_aft_nl[dir][i]))
+			old++;
+		else
+			new++;
+	}
+	if( i == 0 ) return 0;
+	if( old == info->seq_aft_nl_num[dir] ) return -1;
+	if( new == info->seq_aft_nl_num[dir] ) return 1;
 	return 0;
 }
 
@@ -281,15 +290,17 @@
 		if (info->seq_aft_nl[dir][i] == nl_seq)
 			return;
 
-		if (oldest == info->seq_aft_nl_num[dir]
-		    || before(info->seq_aft_nl[dir][i], oldest))
+		if (oldest == info->seq_aft_nl_num[dir] ||
+		    before(info->seq_aft_nl[dir][i],
+		    	   info->seq_aft_nl[dir][oldest]))
 			oldest = i;
 	}
 
 	if (info->seq_aft_nl_num[dir] < NUM_SEQ_TO_REMEMBER) {
 		info->seq_aft_nl[dir][info->seq_aft_nl_num[dir]++] = nl_seq;
 		ip_conntrack_event_cache(IPCT_HELPINFO_VOLATILE, skb);
-	} else if (oldest != NUM_SEQ_TO_REMEMBER) {
+	} else if (oldest != NUM_SEQ_TO_REMEMBER &&
+		   after(nl_seq, info->seq_aft_nl[dir][oldest])) {
 		info->seq_aft_nl[dir][oldest] = nl_seq;
 		ip_conntrack_event_cache(IPCT_HELPINFO_VOLATILE, skb);
 	}
@@ -309,7 +320,8 @@
 	struct ip_ct_ftp_master *ct_ftp_info = &ct->help.ct_ftp_info;
 	struct ip_conntrack_expect *exp;
 	unsigned int i;
-	int found = 0, ends_in_nl;
+	int found = 0, ends_in_nl, seq_type;
+	__u32 parse_ip;
 	typeof(ip_nat_ftp_hook) ip_nat_ftp;
 
 	/* Until there's been traffic both ways, don't look in packets. */
@@ -341,13 +353,14 @@
 	seq = ntohl(th->seq) + datalen;
 
 	/* Look up to see if we're just after a \n. */
-	if (!find_nl_seq(ntohl(th->seq), ct_ftp_info, dir)) {
+	seq_type = find_nl_seq(ntohl(th->seq), ct_ftp_info, dir);
+	if(seq_type < 0) {
 		/* Now if this ends in \n, update ftp info. */
 		DEBUGP("ip_conntrack_ftp_help: wrong seq pos %s(%u) or %s(%u)\n",
 		       ct_ftp_info->seq_aft_nl[0][dir]
 		       old_seq_aft_nl_set ? "":"(UNSET) ", old_seq_aft_nl);
 		ret = NF_ACCEPT;
-		goto out_update_nl;
+		goto out;
 	}
 
 	/* Initialize IP array to expected address (it's not mentioned
@@ -399,8 +412,9 @@
 	 * Doesn't matter unless NAT is happening.  */
 	exp->tuple.dst.ip = ct->tuplehash[!dir].tuple.dst.ip;
 
-	if (htonl((array[0] << 24) | (array[1] << 16) | (array[2] << 8) | array[3])
-	    != ct->tuplehash[dir].tuple.src.ip) {
+	parse_ip=htonl((array[0] << 24) | (array[1] << 16) | (array[2] << 8) |
array[3]);
+	if(parse_ip != ct->tuplehash[dir].tuple.src.ip
+		&& parse_ip != ct->tuplehash[!dir].tuple.dst.ip) {
 		/* Enrico Scholz's passive FTP to partially RNAT'd ftp
 		   server: it really wants us to connect to a
 		   different IP address.  Simply don't record it for
@@ -415,6 +429,7 @@
 		   networks, or the packet filter itself). */
 		if (!loose) {
 			ret = NF_ACCEPT;
+			found=0;
 			goto out_put_expect;
 		}
 		exp->tuple.dst.ip = htonl((array[0] << 24) | (array[1] << 16)
@@ -437,7 +452,7 @@
 	ip_nat_ftp = rcu_dereference(ip_nat_ftp_hook);
 	if (ip_nat_ftp)
 		ret = ip_nat_ftp(pskb, ctinfo, search[dir][i].ftptype,
-				 matchoff, matchlen, exp, &seq);
+				 matchoff, matchlen, exp);
 	else {
 		/* Can't expect this?  Best to drop packet now. */
 		if (ip_conntrack_expect_related(exp) != 0)
@@ -452,7 +467,7 @@
 out_update_nl:
 	/* Now if this ends in \n, update ftp info.  Seq may have been
 	 * adjusted by NAT code. */
-	if (ends_in_nl)
+	if (ends_in_nl && (found > 0 || seq_type == 0))
 		update_nl_seq(seq, ct_ftp_info,dir, *pskb);
  out:
 	spin_unlock_bh(&ip_ftp_lock);

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: bugs in ftp conntrack
@ 2007-05-28  3:04 YU, Haitao
  0 siblings, 0 replies; 8+ messages in thread
From: YU, Haitao @ 2007-05-28  3:04 UTC (permalink / raw)
  To: Patrick McHardy; +Cc: netfilter-devel




In your mail:
>From: Patrick McHardy <kaber@trash.net>
>Reply-To: 
>To: "YU, Haitao" <yuhaitao@tsinghua.org.cn>
>Subject: Re: bugs in ftp conntrack
>Date:Sat, 26 May 2007 11:03:35 +0200
>
>> I think if it's possible that we don't record the new seqence of too new
packets.
>> "Too new packets" means the sequence of the packet is greater than all
>> seq_aft_nl[] values.  We just wait until another "port", "227", etc. packet is
>> parsed correctly.
>> 
>> 
>> So the return value of function find_nl_seq() shoule be three: 
>> 1: too new, then parse pattern, only if found > 0,  then update seq, else keep
>> current seq_aft_nl;
>> 0: match one of the remembered seq, then parse pattern, update seq when found
>=
>> 0;
>> -1: too old(less than all remembered seq),  just goto out, (not goto
>> out_update_nl)
>
>
>Would you mind sending a patch to demonstrate your idea?
>
>
> 
Sorry , make a mistake. It is for 2.6, not 2.4.

patch for 2.6.21.1

--- linux-2.6.21.1/net/ipv4/netfilter/ip_conntrack_ftp.c	2007-04-28
05:49:26.000000000 +0800
+++ linux-2.6.21.1-new/net/ipv4/netfilter/ip_conntrack_ftp.c	2007-05-27
09:31:35.000000000 +0800
@@ -263,10 +263,19 @@
 static int find_nl_seq(u32 seq, const struct ip_ct_ftp_master *info, int dir)
 {
 	unsigned int i;
+	int old=0, new=0;
 
-	for (i = 0; i < info->seq_aft_nl_num[dir]; i++)
+	for (i = 0; i < info->seq_aft_nl_num[dir]; i++){
 		if (info->seq_aft_nl[dir][i] == seq)
-			return 1;
+			return 0;
+		else if(before(seq, info->seq_aft_nl[dir][i]))
+			old++;
+		else
+			new++;
+	}
+	if( i == 0 ) return 0;
+	if( old == info->seq_aft_nl_num[dir] ) return -1;
+	if( new == info->seq_aft_nl_num[dir] ) return 1;
 	return 0;
 }
 
@@ -281,15 +290,17 @@
 		if (info->seq_aft_nl[dir][i] == nl_seq)
 			return;
 
-		if (oldest == info->seq_aft_nl_num[dir]
-		    || before(info->seq_aft_nl[dir][i], oldest))
+		if (oldest == info->seq_aft_nl_num[dir] ||
+		    before(info->seq_aft_nl[dir][i],
+		    	   info->seq_aft_nl[dir][oldest]))
 			oldest = i;
 	}
 
 	if (info->seq_aft_nl_num[dir] < NUM_SEQ_TO_REMEMBER) {
 		info->seq_aft_nl[dir][info->seq_aft_nl_num[dir]++] = nl_seq;
 		ip_conntrack_event_cache(IPCT_HELPINFO_VOLATILE, skb);
-	} else if (oldest != NUM_SEQ_TO_REMEMBER) {
+	} else if (oldest != NUM_SEQ_TO_REMEMBER &&
+		   after(nl_seq, info->seq_aft_nl[dir][oldest])) {
 		info->seq_aft_nl[dir][oldest] = nl_seq;
 		ip_conntrack_event_cache(IPCT_HELPINFO_VOLATILE, skb);
 	}
@@ -309,7 +320,8 @@
 	struct ip_ct_ftp_master *ct_ftp_info = &ct->help.ct_ftp_info;
 	struct ip_conntrack_expect *exp;
 	unsigned int i;
-	int found = 0, ends_in_nl;
+	int found = 0, ends_in_nl, seq_type;
+	__u32 parse_ip;
 	typeof(ip_nat_ftp_hook) ip_nat_ftp;
 
 	/* Until there's been traffic both ways, don't look in packets. */
@@ -341,13 +353,14 @@
 	seq = ntohl(th->seq) + datalen;
 
 	/* Look up to see if we're just after a \n. */
-	if (!find_nl_seq(ntohl(th->seq), ct_ftp_info, dir)) {
+	seq_type = find_nl_seq(ntohl(th->seq), ct_ftp_info, dir);
+	if(seq_type < 0) {
 		/* Now if this ends in \n, update ftp info. */
 		DEBUGP("ip_conntrack_ftp_help: wrong seq pos %s(%u) or %s(%u)\n",
 		       ct_ftp_info->seq_aft_nl[0][dir]
 		       old_seq_aft_nl_set ? "":"(UNSET) ", old_seq_aft_nl);
 		ret = NF_ACCEPT;
-		goto out_update_nl;
+		goto out;
 	}
 
 	/* Initialize IP array to expected address (it's not mentioned
@@ -399,8 +412,9 @@
 	 * Doesn't matter unless NAT is happening.  */
 	exp->tuple.dst.ip = ct->tuplehash[!dir].tuple.dst.ip;
 
-	if (htonl((array[0] << 24) | (array[1] << 16) | (array[2] << 8) | array[3])
-	    != ct->tuplehash[dir].tuple.src.ip) {
+	parse_ip=htonl((array[0] << 24) | (array[1] << 16) | (array[2] << 8) |
array[3]);
+	if(parse_ip != ct->tuplehash[dir].tuple.src.ip
+		&& parse_ip != ct->tuplehash[!dir].tuple.dst.ip) {
 		/* Enrico Scholz's passive FTP to partially RNAT'd ftp
 		   server: it really wants us to connect to a
 		   different IP address.  Simply don't record it for
@@ -415,6 +429,7 @@
 		   networks, or the packet filter itself). */
 		if (!loose) {
 			ret = NF_ACCEPT;
+			found=0;
 			goto out_put_expect;
 		}
 		exp->tuple.dst.ip = htonl((array[0] << 24) | (array[1] << 16)
@@ -437,7 +452,7 @@
 	ip_nat_ftp = rcu_dereference(ip_nat_ftp_hook);
 	if (ip_nat_ftp)
 		ret = ip_nat_ftp(pskb, ctinfo, search[dir][i].ftptype,
-				 matchoff, matchlen, exp, &seq);
+				 matchoff, matchlen, exp);
 	else {
 		/* Can't expect this?  Best to drop packet now. */
 		if (ip_conntrack_expect_related(exp) != 0)
@@ -452,7 +467,7 @@
 out_update_nl:
 	/* Now if this ends in \n, update ftp info.  Seq may have been
 	 * adjusted by NAT code. */
-	if (ends_in_nl)
+	if (ends_in_nl && (found > 0 || seq_type == 0))
 		update_nl_seq(seq, ct_ftp_info,dir, *pskb);
  out:
 	spin_unlock_bh(&ip_ftp_lock);

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2007-05-28  3:04 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-05-22  6:24 bugs in ftp conntrack YU, Haitao
2007-05-24 18:16 ` Patrick McHardy
  -- strict thread matches above, loose matches on Subject: below --
2007-05-25  1:10 YU, Haitao
2007-05-26  3:28 ` Henrik Nordstrom
2007-05-26  9:07   ` Patrick McHardy
2007-05-26  9:03 ` Patrick McHardy
2007-05-27  1:35 YU, Haitao
2007-05-28  3:04 YU, Haitao

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.