[PATCH] find_appropriate_src() fix

[Kovacs Krisztian <piglet@sch.bme.hu>]

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


  Hi,

  I've created a patch for the (suspected) problem described below. I've
test-booted the patch, and it seems to work in a simple MASQ setup, but
since this is my small home-network, I can't tell if it really does things
right.

  Could someone give it a try, and see if SNAT still works after applying
these changes? (The patch is for 2.6, won't do any good on 2.4 since the
in_range() function in 2.4 is seriously broken.)

On Tue, Mar 02, 2004 at 12:20:07PM +0100, KOVACS Krisztian wrote:
>   Oops, you're right. However, I don't think this is the intended
> operation of find_appropriate_src(), I suspect this is a bug. The
> problem seems to be the following:
> 
> 1. ip_nat_setup_info() passes the inverse of the reply tuple to
> get_unique_tuple() as orig_tuple, in our case this will be the same as
> the tuple in the original direction (A.1 -> C.2). mr will be set to
> MANIP_SRC to B. 
> 
> 2. get_unique_tuple() calls find_appropriate_src() with orig_tuple and
> mr.
> 
> 3. find_appropriate_src() traverses the appropriate hash chain,
> returning the first list entry for which src_cmp() returns true.
> 
> 4. src_cmp() gets the following arguments: the current nat hash entry,
> orig_tuple, and mr. It compares the list entry's ORIG_DIR source to
> orig_tuple's source. In our case this matches. Then it calls in_range()
> 
> 5. in_range() gets the following arguments: orig_tuple, ORIG_DIR source
> of the current list entry as the manip, and mr. It constructs newtuple:
> the source will be current conntrack's ORIG_DIR's source, the
> destination that of orig_tuple. Note that orig_tuple is the same as the
> to-be-NAT-ed connection's ORIG_DIR tuple, and the conntrack's source
> must be equal, if in_range() is called by src_cmp(). However, in this
> case, in_range returns false, because newtuple's source IP (A) won't be
> the same as that given by mr (B).
> 
>   So, in our case in_range() does not apply the source manip to
> orig_tuple, because src_cmp() passes the wrong manip to it. Maybe
> src_cmp() should pass
> 
> &i->conntrack->tuplehash[IP_CT_DIR_REPLY].tuple.dst
> instead of
> &i->conntrack->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src
> 
> as manip to in_range()? I think it would make more sense, since we are
> interested in the probably matching connection's NAT-ed source.

-- 
 Regards,
   Krisztian KOVACS


[-- Attachment #2: find_appropriate_src_fix.patch --]
[-- Type: text/plain, Size: 2550 bytes --]

Index: linux/net/ipv4/netfilter/ip_nat_core.c
===================================================================
--- linux.orig/net/ipv4/netfilter/ip_nat_core.c	2004-03-06 18:52:05.000000000 +0100
+++ linux/net/ipv4/netfilter/ip_nat_core.c	2004-03-07 13:05:42.000000000 +0100
@@ -176,20 +176,25 @@
 	const struct ip_conntrack_tuple *tuple,
 	const struct ip_nat_multi_range *mr)
 {
-	return (i->conntrack->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.protonum
-		== tuple->dst.protonum
-		&& i->conntrack->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.ip
-		== tuple->src.ip
-		&& i->conntrack->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.u.all
-		== tuple->src.u.all
-		&& in_range(tuple,
-			    &i->conntrack->tuplehash[IP_CT_DIR_ORIGINAL]
-			    .tuple.src,
-			    mr));
+	struct ip_conntrack_tuple reverse;
+
+	if (i->conntrack->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.protonum
+	    == tuple->dst.protonum
+	    && i->conntrack->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.ip
+	    == tuple->src.ip
+	    && i->conntrack->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.u.all
+	    == tuple->src.u.all) {
+		/* the original source is the same, check if the same manip
+		 * would be appropriate for the given multi-range */
+		invert_tuplepr(&reverse, &i->conntrack->tuplehash[IP_CT_DIR_REPLY].tuple);
+		return in_range(tuple, &reverse.src, mr);
+	}
+
+	return 0;
 }
 
 /* Only called for SRC manip */
-static struct ip_conntrack_manip *
+static struct ip_conntrack_tuple *
 find_appropriate_src(const struct ip_conntrack_tuple *tuple,
 		     const struct ip_nat_multi_range *mr)
 {
@@ -199,7 +204,7 @@
 	MUST_BE_READ_LOCKED(&ip_nat_lock);
 	i = LIST_FIND(&bysource[h], src_cmp, struct ip_nat_hash *, tuple, mr);
 	if (i)
-		return &i->conntrack->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src;
+		return &i->conntrack->tuplehash[IP_CT_DIR_REPLY].tuple;
 	else
 		return NULL;
 }
@@ -419,13 +424,14 @@
 	   So far, we don't do local source mappings, so multiple
 	   manips not an issue.  */
 	if (hooknum == NF_IP_POST_ROUTING) {
-		struct ip_conntrack_manip *manip;
+		struct ip_conntrack_tuple *srctuple, mtuple;
 
-		manip = find_appropriate_src(orig_tuple, mr);
-		if (manip) {
+		srctuple = find_appropriate_src(orig_tuple, mr);
+		if (srctuple) {
 			/* Apply same source manipulation. */
+			invert_tuplepr(&mtuple, srctuple);
 			*tuple = ((struct ip_conntrack_tuple)
-				  { *manip, orig_tuple->dst });
+				  { mtuple.src, orig_tuple->dst });
 			DEBUGP("get_unique_tuple: Found current src map\n");
 			if (!ip_nat_used_tuple(tuple, conntrack))
 				return 1;