All of lore.kernel.org
 help / color / mirror / Atom feed
* 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-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
* 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
* 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

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-25  1:10 bugs in ftp conntrack YU, Haitao
2007-05-26  3:28 ` Henrik Nordstrom
2007-05-26  9:07   ` Patrick McHardy
2007-05-26  9:03 ` Patrick McHardy
  -- strict thread matches above, loose matches on Subject: below --
2007-05-28  3:04 YU, Haitao
2007-05-27  1:35 YU, Haitao
2007-05-22  6:24 YU, Haitao
2007-05-24 18:16 ` Patrick McHardy

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.