xt_recent.c bug - and cleanup

xt_recent.c bug - and cleanup

[Valentijn Sessink]

Dear list,

There is a bug in the "recent" module's "!" option, as follows.

Suppose I want a list with IPv4 addresses that are "friends". My 
iptables rules are simple:
-A INPUT -m state --state RELATED,ESTABLISHED -j ACCEPT
-A INPUT -m state --state INVALID -j DROP
-A INPUT -m recent ! --update --name friends --rsource -j LOG 
--log-prefix "go away: "

This will log "go away" for everyone not on my list of friends (how safe 
;-) and it should update the "last seen" of everyone who is a friend.

However, the update never happens. First, if you're not a friend (I used 
"cut" for brevity of the dmesg output):
~$ ssh -A root@stout 'cat /proc/net/xt_recent/friends; dmesg'|cut -c1-50
[ 4987.361751] go away: IN=eth0 OUT= MAC=00:13:8f:
~$ ssh -A root@stout 'cat /proc/net/xt_recent/friends; dmesg'|cut -c1-50
[ 4987.361751] go away: IN=eth0 OUT= MAC=00:13:8f:
[ 4988.320653] go away: IN=eth0 OUT= MAC=00:13:8f:

(You're told to "go away" time and again). Now let's add you to the 
friends list, clear the kernel log and try again:
root@stout:~# echo +192.168.112.12 > /proc/net/xt_recent/friends; dmesg 
-c > /dev/null

... and try again:
~$ ssh -A root@stout 'cat /proc/net/xt_recent/friends; dmesg'
src=192.168.112.12 ttl: 0 last_seen: 1187856 oldest_pkt: 1 1187856
~$ ssh -A root@stout 'cat /proc/net/xt_recent/friends; dmesg'
src=192.168.112.12 ttl: 0 last_seen: 1187856 oldest_pkt: 1 1187856

As you can see, the entry is never updated. It gets even stranger when 
you add a "--seconds" check, because now your entry is only updated when 
the check didn't match; if you did match, there's no update. (I will not 
give an example for this, as the bug is complicated enough without it).

Now IMHO, this bug largely comes from the intermingled use of a variable 
named "ret" in recent_mt(), which is supposed to only be the return 
value, but is in fact also used as a means to check if the "!" option is 
used - and after "ret = !ret", the logic fails.

I reported this in 2011, - see my bug report at
https://bugzilla.kernel.org/show_bug.cgi?id=29332

Since then, nothing happened. In my bug report is "quick hack" for a 
fix, that leaves the double use of "ret" and two spurious "goto" 
statements intact, but I'd rather have my cleanup patch accepted, 
because it makes recent_mt() much more readable. See here: 
https://bugzilla.kernel.org/attachment.cgi?id=48292&action=diff

So I kindly ask: is there a way to get my patch accepted, with the 
cleanup? Could someone please look into it? Is there anything else I 
should do?

Best regards,

Valentijn

Re: xt_recent.c bug - and cleanup

[Phil Oester]

On Thu, Aug 29, 2013 at 12:16:23PM +0200, Valentijn Sessink wrote:
> There is a bug in the "recent" module's "!" option, as follows.
> 
> Suppose I want a list with IPv4 addresses that are "friends". My
> iptables rules are simple:
> -A INPUT -m state --state RELATED,ESTABLISHED -j ACCEPT
> -A INPUT -m state --state INVALID -j DROP
> -A INPUT -m recent ! --update --name friends --rsource -j LOG
> --log-prefix "go away: "
> 
> This will log "go away" for everyone not on my list of friends (how
> safe ;-) and it should update the "last seen" of everyone who is a
> friend.

I disagree with your assessment.  Take a closer look at the man page
for recent match (emphasis added to relevant portion):

  [!] --update
     Like --rcheck, except it will update the "last seen" timestamp **if it matches**

Your rule _does not match_ for "friends".  Because you used inversion, it only
matches for non-friends (and then, of course, there is no timestamp to
update).

So put more simply:

	       update: -m recent --update
	do not update: -m recent ! --update

> As you can see, the entry is never updated. 

By design.

> It gets even stranger
> when you add a "--seconds" check, because now your entry is only
> updated when the check didn't match; if you did match, there's no
> update. (I will not give an example for this, as the bug is
> complicated enough without it).

This is confusing, to be sure, but from my review of the code, it is also
by design.  Unfortunately when you use --seconds, it DOES NOT MATCH,
and therefore it does not update.  But then on the next packet, since
there was no update on the first packet, it still does not match on the
second.  And on and on...  It will never match, and it will never update
the entry.

It seems the sane way to use --seconds would be in two rules: a --set first
and then a followup --rcheck --seconds.  

> I reported this in 2011, - see my bug report at
> https://bugzilla.kernel.org/show_bug.cgi?id=29332

...should be closed

> Since then, nothing happened. In my bug report is "quick hack" for a
> fix, that leaves the double use of "ret" and two spurious "goto"
> statements intact, but I'd rather have my cleanup patch accepted,
> because it makes recent_mt() much more readable. See here:
> https://bugzilla.kernel.org/attachment.cgi?id=48292&action=diff

One note on your cleanup: there is nothing wrong with using "goto"
in the kernel.  The code as it exists today is easier to read with
the use of it.

> So I kindly ask: is there a way to get my patch accepted, with the
> cleanup? Could someone please look into it? Is there anything else I
> should do?

I see no bug here.

Phil

Re: xt_recent.c bug - and cleanup

[Valentijn Sessink]

Hi Phil,

First, thanks for looking into this.

On 30-08-13 00:09, Phil Oester wrote:
> I disagree with your assessment.  Take a closer look at the man page
> for recent match (emphasis added to relevant portion):
>   [!] --update
>      Like --rcheck, except it will update the "last seen" timestamp **if it matches**
> Your rule _does not match_ for "friends".  Because you used inversion, it only
> matches for non-friends (and then, of course, there is no timestamp to
> update).

The manpage says "This will always return success (or failure if ! is
passed in).", which IMHO means the "!" is only meant to reverse the
return value. But I agree that the man page is not very clear.

But, as you have reviewed the code, I'd like to ask you why the code
calls recent_entry_update(t, e) when there's nothing to update (i.e. a
call to recent_entry_init(), if any, would be more appropriate) - and
the code knows it?

> By design.

Then calling recent_entry_update is a bug. Besides, see below how the
--seconds check fits in.

You're right though, that --update --seconds 10 only updates within the
10 seconds scope as well.

> This is confusing, to be sure, but from my review of the code, it is also
> by design.  Unfortunately when you use --seconds, it DOES NOT MATCH,
> and therefore it does not update.  But then on the next packet, since
> there was no update on the first packet, it still does not match on the
> second.  And on and on...  It will never match, and it will never update
> the entry.

Let's say you use
-A INPUT -m recent ! --update --name friends --seconds 600 --rsource -j
LOG --log-prefix "go away: "

Then the first 600 seconds, you're welcome. Then after 600 seconds, you
don't match - ** and suddenly your entry is updated **. So for the next
600 seconds, you're welcome again! After that, you're not welcome, so
your entry is updated again.

That's silly.

The reason I found this, is that I'm trying to actually * use * the
recent module with the inversion, and if this is not a bug, well, then
its really odd at least - I can't even think how it could be useful this
way.

Best regards,

Valentijn

Re: xt_recent.c bug - and cleanup

[Phil Oester]

On Fri, Aug 30, 2013 at 07:27:01AM +0200, Valentijn Sessink wrote:
> The manpage says "This will always return success (or failure if ! is
> passed in).", which IMHO means the "!" is only meant to reverse the
> return value. But I agree that the man page is not very clear.

Yes, it says that, but ONLY for the --set option, NOT for any of the others,
including the --update option you are using.  As noted previously, the
--update option includes "if it matches" as a qualifier.

> But, as you have reviewed the code, I'd like to ask you why the code
> calls recent_entry_update(t, e) when there's nothing to update (i.e. a
> call to recent_entry_init(), if any, would be more appropriate) - and
> the code knows it?

Eh?  You might be misreading the code.  First of all, this segment of
code is only reached if an entry has been found to exist in the relevant
table:

        if (info->check_set & XT_RECENT_SET ||
            (info->check_set & XT_RECENT_UPDATE && ret)) {
                recent_entry_update(t, e);

And it clearly states that it will update the entry unconditionally if you
used --set, and only if all other options matched in the case of --update.
What makes you think there's nothing to update here??

> The reason I found this, is that I'm trying to actually * use * the
> recent module with the inversion, and if this is not a bug, well, then
> its really odd at least - I can't even think how it could be useful this
> way.

Well, clearly the author had in mind the example in the man page:

	you can create a "badguy" list out of people attempting to connect to
	port 139 on your firewall and then DROP all future packets from them
	without considering them.

Which takes two rules to achieve: a --set first, then a --rcheck or --update.

You can certainly use inversion with this match, but in your original example,
you are expecting that anything which does not match will get it's timestamp
updated.  That is simply not how the match works.  You probably will need to
use two rules to achieve this: an --update with an -j ACCEPT for "friends",
and a second rule for the rest.

Phil

Re: xt_recent.c bug - and cleanup

[Valentijn Sessink]

Hi Phil,

On 30-08-13 17:24, Phil Oester wrote:
> Eh?  You might be misreading the code.

I was. I stand corrected.
[...]

> You can certainly use inversion with this match, but in your original example,
> you are expecting that anything which does not match will get it's timestamp
> updated.  That is simply not how the match works.

I contacted Stephen Frost to ask his opinion about the "!" qualifier and 
he agrees with the current source, i.e. only update if the match returns 
true.

Thank you for your thorough reading and sorry for the misunderstanding. 
I have closed the bug.

Best regards,

Valentijn Sessink