ipt_recent 0.3.0 --rttl still doesn't work (I made a patch instead, can't wait)

ipt_recent 0.3.0 --rttl still doesn't work (I made a patch instead, can't wait)

[per j]

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

I've played around with -j TTL and no it doesn't have to do with the TTL 
mangled in the FORWARD chain because all my rules with -m recent are in 
INPUT chain or the beginning of the FORWARD chain before ACCEPT.

I tried out the ipt_recent-0.3.0 release and the problem still exists.  I 
turned on debug and found out r_list[location].ttl is always the same and 
doesn't even come close to the ttl value stored in the recent list.

I got pretty fussed up patching and re-patching my kernel and your latest 
release still didn't fix the --rttl problem.  Finally I spend quite some 
time reviewing your source code.  Your code on the IPT_RECENT_TTL section 
doesn't do a single thing because the location variable in r_list[location] 
doesn't change when hash_result is incremented.  I tried adding location = 
hash_table[hash_result] in the while loop, but that caused kernel panic.  I 
rewrote the whole IPT_RECENT_TTL code and now my tests for TTL matching 
works.  I fixed it!

Also I fixed the (unreported) problem with ttl not being set zero when 
manually adding ip addresses into the recent list with a for loop in a bash 
script - eg. for blah_ip in bleh; do echo blah_ip > recentlist; done.  
Somehow the ttl value is non-zero but all the entires added into the recent 
list have that same non-zero ttl value.  However, doing echo xx.xx.xx.xx > 
recentlist in the command-line does properly set ttl zero.  I wonder why it 
behaves well when not in a script.

After some tests, I am confident that the following patch to the 
ipt_recent-0.3.0 source code will fix the --rttl and non-zero ttl problem in 
userctl.  It works for me.

Apologies for posting code here as I've not yet signed up at the 
developer-list, but let me know if you see any errors.

------------------------------------------------
--- ipt_recent.c.orig	2003-03-03 00:27:47.000000000 -0500
+++ ipt_recent.c	2003-03-03 07:19:16.000000000 -0500
@@ -266,6 +266,7 @@

	skb.nh.iph->saddr = addr;
	skb.nh.iph->daddr = addr;
+	skb.nh.iph->ttl = 0;	// ttl happens to be randomly assigned, make sure it 
is 0.
	match(&skb,NULL,NULL,&info,0,NULL,sizeof(info),NULL);
	kfree(skb.nh.iph);

@@ -354,21 +355,9 @@

	orig_hash_result = hash_result = hash_func(addr,ip_list_hash_size);
	/* Hash entry at this result used */
-	/* Check for TTL match if requested.  If TTL is zero then a match would 
never
-	 * happen, so match regardless of existing TTL in that case.  Zero means 
the
-	 * entry was added via the /proc interface anyway, so we will just use the
-	 * first TTL we get for that IP address. */
-	if(info->check_set & IPT_RECENT_TTL) {
-		while(hash_table[hash_result] != -1 && 
!(r_list[hash_table[hash_result]].addr == addr &&
-			(!r_list[location].ttl || r_list[location].ttl == ttl))) {
-			/* Collision in hash table */
-			hash_result = (hash_result + 1) % ip_list_hash_size;
-		}
-	} else {
-		while(hash_table[hash_result] != -1 && 
r_list[hash_table[hash_result]].addr != addr) {
-			/* Collision in hash table */
-			hash_result = (hash_result + 1) % ip_list_hash_size;
-		}
+	while(hash_table[hash_result] != -1 && 
r_list[hash_table[hash_result]].addr != addr) {
+		/* Collision in hash table */
+		hash_result = (hash_result + 1) % ip_list_hash_size;
	}

	if(hash_table[hash_result] == -1 && !(info->check_set & IPT_RECENT_SET)) {
@@ -459,6 +448,20 @@
				if(hits_found >= info->hit_count) ans = !info->invert; else ans = 
info->invert;
			}
		}
+	/* Check for TTL match if requested.  If TTL is zero then a match would 
never
+	 * happen, so match regardless of existing TTL in that case.  Zero means 
the
+	 * entry was added via the /proc interface anyway, so we will just use the
+	 * first TTL we get for that IP address. */
+		/* IPT_RECENT_TTL -> !ttl match -> ans = info->invert
+		 * TTL match section fix by bluecloud <cloudatcu teydotseeohem>
+		 * [aka perj8AThotmailDOTcom on the mailing list] */
+		if(info->check_set & IPT_RECENT_TTL && r_list[location].ttl && 
r_list[location].ttl != ttl) {
+			// TTL did not match
+			ans = info->invert;
+		}
+		if(debug && IPT_RECENT_TTL) printk(KERN_INFO "RECENT_TTL: match(): 
rttl:%u,ttl:%u \n",
+				r_list[location].ttl,
+				ttl);
		if(debug) {
			if(ans)
				printk(KERN_INFO "RECENT_NAME: match(): match addr: %u\n",addr);
---------------------------------------------------

>* per j (perj8@hotmail.com) wrote:
> > Have you got any luck fixing the --rttl bug?   It's already been a 
>couple
> > weeks since the last post.  I tried to go around this problem with no
> > success.  I don't want to put all my rules into one chain, the INPUT 
>chain,
> > to get it to work and prefer separate chains to make my firewall rules
> > easier to maintain.  Apparently --rttl doesn't work when --set is on a
> > different chain on the filter table.  That's the problem.
>
>Ok, coming back to this issue I'm pretty sure I have an idea as to what
>the problem is.  It's pretty simple, really, the TTL is going to change
>somewhere while in the kernel.  Probably in the routing logic.  This
>means that in PREROUTING the TTL is one thing but in FORWARD (after
>being routed) it's been decremented by one.
>
>The 'solution' to this problem would really be for the recent module to
>always go with the initial TTL and detect if the routing logic has been
>called or not to decide if it needs to increment the TTL to get back to
>the original TTL.  Unfortunately at the moment I'm not sure if that will
>be very easy or not but I'll look around and see if I can't make this
>work.
>
>	Stephen
><< attach3 >>


_________________________________________________________________
The new MSN 8: smart spam protection and 2 months FREE*  
http://join.msn.com/?page=features/junkmail

[-- Attachment #2: patch-ctl-rtt-ipt_recent.c-0.3.0.bz2 --]
[-- Type: application/octet-stream, Size: 1106 bytes --]

[-- Attachment #3: patch-ctl-rtt-ipt_recent.c-0.3.0.bz2.md5sum --]
[-- Type: text/plain, Size: 72 bytes --]

707719e9ad9c64d30c8273e9c9a405ba  patch-ctl-rtt-ipt_recent.c-0.3.0.bz2

Re: ipt_recent 0.3.0 --rttl still doesn't work (I made a patch instead, can't wait)

[Stephen Frost]

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

* per j (perj8@hotmail.com) wrote:
> I've played around with -j TTL and no it doesn't have to do with the TTL 
> mangled in the FORWARD chain because all my rules with -m recent are in 
> INPUT chain or the beginning of the FORWARD chain before ACCEPT.

If your patch is against 0.3.0 then it may very well matter since your
patch doesn't remove the 'if(out) ttl++;' line from 0.3.0.  Could you
check for that?

> I tried out the ipt_recent-0.3.0 release and the problem still exists.  I 
> turned on debug and found out r_list[location].ttl is always the same and 
> doesn't even come close to the ttl value stored in the recent list.

Yeah, that's an old bug from when I switched to using hash tables
apparently.  I believe that *should* read:
while(hash_table[hash_result] != -1 && !(r_list[hash_table[hash_result]].addr == addr &&
	(!r_list[hash_table[hash_result]].ttl || r_list[hash_table[hash_result]].ttl != ttl))) {

Instead.  Note that I also had to invert the ttl check, it should be
considered a collision when the ttl doesn't match and matching ttl has
been requested.

> I got pretty fussed up patching and re-patching my kernel and your latest 
> release still didn't fix the --rttl problem.  Finally I spend quite some 
> time reviewing your source code.  Your code on the IPT_RECENT_TTL section 
> doesn't do a single thing because the location variable in r_list[location] 
> doesn't change when hash_result is incremented.  I tried adding location = 
> hash_table[hash_result] in the while loop, but that caused kernel panic.  I 
> rewrote the whole IPT_RECENT_TTL code and now my tests for TTL matching 
> works.  I fixed it!

Unfortunately what you did doesn't really work.  The goal of doing the
TTL match is to differentiate between IP addresses based on their TTL so
that they are really considered two seperate entities.  What you've done
is to only allow one TTL value per IP address, whichever TTL came first.
So, for example:

10.10.0.1 sends to router, TTL == 32
  10.10.0.1 gets added to recent list with ttl == 32
10.10.0.1 sends to router, TTL == 25
  10.10.0.1 gets 'found' in the hash table but then fails the TTL match

10.10.0.1 with TTL of 25 will therefore never make it into the recent
list, which is wrong.  Instead of just picking the first TTL and going
with that these two need to be treated seperately as if they were two
different IP addresses.

If you could try the above changes I'd like to hear if they work for you
or not.  I'll try and find some time to really test the TTL code soon.

> Also I fixed the (unreported) problem with ttl not being set zero when 
> manually adding ip addresses into the recent list with a for loop in a bash 

Thanks, added that into my local version.

	Stephen

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: ipt_recent 0.3.0 --rttl still doesn't work (I made a patch instead, can't wait)

[per j]

Nope, my patch doesn't remove 'if(out) ttl++;'.  However I'm not too certain 
whether one would really use it.  It doesn't really matter unless --rttl is 
in the POSTROUTING chains.  So far -m recent and LOG are both seeing the 
same TTL for me and I have no --rttl in POSTROUTING.  Why would one use 
--rttl in POSTROUTING?  That's pretty late to filter incoming packets (or 
rather outgoing packets).  Perhaps one would use it to mangle forwarded 
parkets, but it would mean the packet already traversed the FORWARD chain!  
Maybe you could better explain 'if(out) ttl++;'?  I guess it's there for 
those who must use --rttl in POSTROUTING?

On the other hand, I use -j TTL --ttl-inc 1 in PREROUTING to restore the TTL 
of packets going through the gateway as if packets forwarded through the 
gateway are coming from the gateway.  So this _conflicts_ with your 'if(out) 
ttl++;' if I were to use --rttl in POSTROUTING after -j TTL in the mangle 
table.  I think it best not to use 'if(out) ttl++;' in ipt_recent in this 
case.

I see your point about making a new table entry for any different TTL from 
the same ip address.  Valid point indeed!  --rttl seems to work fine with 
your suggested code change to r_list[location].ttl, so I'm using the 
following patch instead.

I wonder why you left the ?old? bug hanging around without giving your users 
any notice?  You should have publicly announced --rttl code is not yet done 
somewhere in the documentation or on your announcement pages.  It's kinda a 
lie to publicly release ipt_recent without announcing the TODO list of _old_ 
bugs.  Before this patch, --rttl worked only in extremely rare condition 
when r_list[location].addr==addr and location==0.  and u knew this was an 
old bug for a year or so? (sigh)

Patched against ipt_recent-0.3.0
--- linux/net/ipv4/netfilter/ipt_recent.c.orig	2003-03-03 00:27:47.000000000 
-0500
+++ linux/net/ipv4/netfilter/ipt_recent.c	2003-03-03 15:51:37.000000000 
-0500
@@ -266,6 +266,7 @@

	skb.nh.iph->saddr = addr;
	skb.nh.iph->daddr = addr;
+	skb.nh.iph->ttl = 0;
	match(&skb,NULL,NULL,&info,0,NULL,sizeof(info),NULL);
	kfree(skb.nh.iph);

@@ -360,7 +361,7 @@
	 * first TTL we get for that IP address. */
	if(info->check_set & IPT_RECENT_TTL) {
		while(hash_table[hash_result] != -1 && 
!(r_list[hash_table[hash_result]].addr == addr &&
-			(!r_list[location].ttl || r_list[location].ttl == ttl))) {
+			(!r_list[hash_table[hash_result]].ttl || 
r_list[hash_table[hash_result]].ttl == ttl))) {
			/* Collision in hash table */
			hash_result = (hash_result + 1) % ip_list_hash_size;
		}



>* per j (perj8@hotmail.com) wrote:
> > I've played around with -j TTL and no it doesn't have to do with the TTL
> > mangled in the FORWARD chain because all my rules with -m recent are in
> > INPUT chain or the beginning of the FORWARD chain before ACCEPT.
>
>If your patch is against 0.3.0 then it may very well matter since your
>patch doesn't remove the 'if(out) ttl++;' line from 0.3.0.  Could you
>check for that?
>
> > I tried out the ipt_recent-0.3.0 release and the problem still exists.  
>I
> > turned on debug and found out r_list[location].ttl is always the same 
>and
> > doesn't even come close to the ttl value stored in the recent list.
>
>Yeah, that's an old bug from when I switched to using hash tables
>apparently.  I believe that *should* read:
>while(hash_table[hash_result] != -1 && 
>!(r_list[hash_table[hash_result]].addr == addr &&
>	(!r_list[hash_table[hash_result]].ttl || 
>r_list[hash_table[hash_result]].ttl != ttl))) {
>
>Instead.  Note that I also had to invert the ttl check, it should be
>considered a collision when the ttl doesn't match and matching ttl has
>been requested.
>
> > I got pretty fussed up patching and re-patching my kernel and your 
>latest
> > release still didn't fix the --rttl problem.  Finally I spend quite some
> > time reviewing your source code.  Your code on the IPT_RECENT_TTL 
>section
> > doesn't do a single thing because the location variable in 
>r_list[location]
> > doesn't change when hash_result is incremented.  I tried adding location 
>=
> > hash_table[hash_result] in the while loop, but that caused kernel panic. 
>  I
> > rewrote the whole IPT_RECENT_TTL code and now my tests for TTL matching
> > works.  I fixed it!
>
>Unfortunately what you did doesn't really work.  The goal of doing the
>TTL match is to differentiate between IP addresses based on their TTL so
>that they are really considered two seperate entities.  What you've done
>is to only allow one TTL value per IP address, whichever TTL came first.
>So, for example:
>
>10.10.0.1 sends to router, TTL == 32
>   10.10.0.1 gets added to recent list with ttl == 32
>10.10.0.1 sends to router, TTL == 25
>   10.10.0.1 gets 'found' in the hash table but then fails the TTL match
>
>10.10.0.1 with TTL of 25 will therefore never make it into the recent
>list, which is wrong.  Instead of just picking the first TTL and going
>with that these two need to be treated seperately as if they were two
>different IP addresses.
>
>If you could try the above changes I'd like to hear if they work for you
>or not.  I'll try and find some time to really test the TTL code soon.
>
> > Also I fixed the (unreported) problem with ttl not being set zero when
> > manually adding ip addresses into the recent list with a for loop in a 
>bash
>
>Thanks, added that into my local version.
>
>	Stephen
><< attach3 >>


_________________________________________________________________
The new MSN 8: advanced junk mail protection and 2 months FREE*  
http://join.msn.com/?page=features/junkmail

Re: ipt_recent 0.3.0 --rttl still doesn't work (I made a patch instead, can't wait)

[per j]

>apparently.  I believe that *should* read:
>while(hash_table[hash_result] != -1 && 
>!(r_list[hash_table[hash_result]].addr == addr &&
>	(!r_list[hash_table[hash_result]].ttl || 
>r_list[hash_table[hash_result]].ttl != ttl))) {
>
>Instead.  Note that I also had to invert the ttl check, it should be
>considered a collision when the ttl doesn't match and matching ttl has
>been requested.
>

It's already inverted at '-1 && !(...)', so I'm using the original ttl 
check.

_________________________________________________________________
MSN 8 with e-mail virus protection service: 2 months FREE*  
http://join.msn.com/?page=features/virus

Re: ipt_recent 0.3.0 --rttl still doesn't work (I made a patch instead, can't wait)

[Stephen Frost]

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

* per j (perj8@hotmail.com) wrote:
> Nope, my patch doesn't remove 'if(out) ttl++;'.  However I'm not too 
> certain whether one would really use it.  It doesn't really matter unless 
> --rttl is in the POSTROUTING chains.  So far -m recent and LOG are both 
> seeing the same TTL for me and I have no --rttl in POSTROUTING.  Why would 
> one use --rttl in POSTROUTING?  That's pretty late to filter incoming 
> packets (or rather outgoing packets).  Perhaps one would use it to mangle 
> forwarded parkets, but it would mean the packet already traversed the 
> FORWARD chain!  Maybe you could better explain 'if(out) ttl++;'?  I guess 
> it's there for those who must use --rttl in POSTROUTING?

The routing is done before the FORWARD chain is done (how else would you
know the outgoing interface?) and I believe the ttl incrementation
happens in the routing code.  I don't know that for sure and havn't had
time to test it.  The 'if(out) ttl++;' is meant to make TTL matching
work for people who use recent matching between PREROUTING and
FORWARD/POSTROUTING.  If you're using -j TTL and whatnot then really all
bets are off and there's no way for ipt_recent to figure that out so if
you're doing that you're kind of on your own with the ipt_recent TTL
matching.  The point of the 'if(out) ttl++;' is to allow you to 'set' in
PREROUTING and then 'rcheck' or 'update' in FORWARD, after the routing
code is done and have the TTL match work correctly, provided you are not
otherwise mucking the TTL.

> On the other hand, I use -j TTL --ttl-inc 1 in PREROUTING to restore the 
> TTL of packets going through the gateway as if packets forwarded through 
> the gateway are coming from the gateway.  So this _conflicts_ with your 
> 'if(out) ttl++;' if I were to use --rttl in POSTROUTING after -j TTL in the 
> mangle table.  I think it best not to use 'if(out) ttl++;' in ipt_recent in 
> this case.

I guess I could make that bit of code optional if there's really a need
for it to be?

> I see your point about making a new table entry for any different TTL from 
> the same ip address.  Valid point indeed!  --rttl seems to work fine with 
> your suggested code change to r_list[location].ttl, so I'm using the 
> following patch instead.

Glad that's working.

> I wonder why you left the ?old? bug hanging around without giving your 
> users any notice?  You should have publicly announced --rttl code is not 
> yet done somewhere in the documentation or on your announcement pages.  
> It's kinda a lie to publicly release ipt_recent without announcing the TODO 
> list of _old_ bugs.  Before this patch, --rttl worked only in extremely 
> rare condition when r_list[location].addr==addr and location==0.  and u 
> knew this was an old bug for a year or so? (sigh)

I didn't know about the bug before you pointed it out to me.  After you
pointed it out I realized how old a bug it was because of what it was
doing.

	Stephen

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: ipt_recent 0.3.0 --rttl still doesn't work (I made a patch instead, can't wait)

[Stephen Frost]

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

* per j (perj8@hotmail.com) wrote:
> >apparently.  I believe that *should* read:
> >while(hash_table[hash_result] != -1 && 
> >!(r_list[hash_table[hash_result]].addr == addr &&
> >	(!r_list[hash_table[hash_result]].ttl || 
> >r_list[hash_table[hash_result]].ttl != ttl))) {
> >
> >Instead.  Note that I also had to invert the ttl check, it should be
> >considered a collision when the ttl doesn't match and matching ttl has
> >been requested.
> >
> 
> It's already inverted at '-1 && !(...)', so I'm using the original ttl 
> check.

Oh, right, good point, thanks.

	Stephen

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]