BUG: libiptc chain references bug

BUG: libiptc chain references bug

[Jesper Dangaard Brouer]

I have found a chain reference bug in libiptc or iptables.c.

When deleting a rule, with iptables do_command("-D xxx -j yyy") that
calls "iptc_delete_entry" the reference count of the rules jump
target chain is not decremented.

When deleting the rule with iptables do_command("-D xxx rule_num")
that calls "iptc_delete_num_entry" the reference count is correctly
decremeted.

Notice, that the chain gets the correct reference count after a
"commit" of the handle.


  Here is the call sequence, of the bug:

+------
[Function] init() libiptc handle
[Function] create_chain(badehat)

[Function] iptables_do_command(-I FORWARD -j badehat)
[Function] get_references(badehat): refs=1

[Function] iptables_do_command(-D FORWARD -j badehat)
[Function] get_references(badehat): refs=1

[Function] commit()

System command: iptables -L badehat
Chain badehat (0 references)
target     prot opt source               destination
+------

  Here is the call sequence, where the bug does not occur:

+------
[Function] init() libiptc handle
[Function] create_chain(badehat)

[Function] iptables_do_command(-I FORWARD -j badehat)
[Function] get_references(badehat): refs=1

[Function] iptables_do_command(-D FORWARD 1)
[Function] get_references(badehat): refs=0

[Function] commit()

System command: iptables -L badehat
Chain badehat (0 references)
target     prot opt source               destination
+------


Hilsen
   Jesper Brouer

--
-------------------------------------------------------------------
MSc. Master of Computer Science
Dept. of Computer Science, University of Copenhagen
Author of http://www.adsl-optimizer.dk
-------------------------------------------------------------------

Re: BUG: libiptc chain references bug

[Jesper Dangaard Brouer]

Hi

I have found the bug, and it's in libiptc.c.

More specifically in iptc_delete_entry() / TC_DELETE_ENTRY, 
and is a result of constructing "r" the rule entry that is used for 
comparison. The bug is that the function iptcc_map_target() increase the 
target chains references count...

I'll cook up a patch, once I have tested it.

This call sequence reveals the bug. I'm trying to delete a rule that does 
not exist, but during construction of "r" the match entry, the target 
chain "badehat" gets its reference count incremented.

[Function] init() libiptc handle
[Function] create_chain(badehat)
[Function] get_references(badehat): refs=0
[Function] iptables_do_command(-D FORWARD -j badehat): No chain/target/match by that name
[Function] iptables_do_command(-D FORWARD -j badehat): No chain/target/match by that name
[Function] iptables_do_command(-D FORWARD -j badehat): No chain/target/match by that name
[Function] iptables_do_command(-D FORWARD -j badehat): No chain/target/match by that name
[Function] get_references(badehat): refs=4
[Function] commit()

--
Best regards
   Jesper Brouer
   ComX Networks A/S
   Linux Network developer
   Cand. Scient Datalog / MSc.
   Author of http://adsl-optimizer.dk



On Wed, 12 Jul 2006, Jesper Dangaard Brouer wrote:

> I have found a chain reference bug in libiptc or iptables.c.
>
> When deleting a rule, with iptables do_command("-D xxx -j yyy") that
> calls "iptc_delete_entry" the reference count of the rules jump
> target chain is not decremented.
>
> When deleting the rule with iptables do_command("-D xxx rule_num")
> that calls "iptc_delete_num_entry" the reference count is correctly
> decremeted.
>
> Notice, that the chain gets the correct reference count after a
> "commit" of the handle.
>
>
> Here is the call sequence, of the bug:
>
> +------
> [Function] init() libiptc handle
> [Function] create_chain(badehat)
>
> [Function] iptables_do_command(-I FORWARD -j badehat)
> [Function] get_references(badehat): refs=1
>
> [Function] iptables_do_command(-D FORWARD -j badehat)
> [Function] get_references(badehat): refs=1
>
> [Function] commit()
>
> System command: iptables -L badehat
> Chain badehat (0 references)
> target     prot opt source               destination
> +------
>
> Here is the call sequence, where the bug does not occur:
>
> +------
> [Function] init() libiptc handle
> [Function] create_chain(badehat)
>
> [Function] iptables_do_command(-I FORWARD -j badehat)
> [Function] get_references(badehat): refs=1
>
> [Function] iptables_do_command(-D FORWARD 1)
> [Function] get_references(badehat): refs=0
>
> [Function] commit()
>
> System command: iptables -L badehat
> Chain badehat (0 references)
> target     prot opt source               destination
> +------

[PATCH] BUG: libiptc chain references bug

[Jesper Dangaard Brouer]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 695 bytes --]


Correcting a chain references increment bug in libiptc.

The bug lies in function iptc_delete_entry() / TC_DELETE_ENTRY.  The
problem is the construction of "r" the rule entry, that is used for
comparison. The problem is that the function iptcc_map_target()
increase the target chains references count.

The fix is to use function iptcc_delete_rule() to delete the "r" rule
(as it decrement the counter again). To make it work a small NULL
pointer check is also added iptcc_delete_rule().

Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>

-- 
Best regards
   Jesper Brouer
   ComX Networks A/S
   Linux Network developer
   Cand. Scient Datalog / MSc.
   Author of http://adsl-optimizer.dk

[-- Attachment #2: Type: TEXT/PLAIN, Size: 1126 bytes --]

Index: iptables/libiptc/libiptc.c
===================================================================
--- iptables/libiptc/libiptc.c	(revision 6644)
+++ iptables/libiptc/libiptc.c	(working copy)
@@ -347,7 +347,8 @@ static void iptcc_delete_rule(struct rul
 	    && r->jump)
 		r->jump->references--;
 
-	list_del(&r->list);
+	if (r->list.next && r->list.prev)
+	  list_del(&r->list);
 	free(r);
 }
 
@@ -1515,6 +1516,8 @@ TC_DELETE_ENTRY(const IPT_CHAINLABEL cha
 
 	memcpy(r->entry, origfw, origfw->next_offset);
 	r->counter_map.maptype = COUNTER_MAP_NOMAP;
+
+	/* Remember that iptcc_map_target increment target chain references */
 	if (!iptcc_map_target(*handle, r)) {
 		DEBUGP("unable to map target of rule for chain `%s'\n", chain);
 		free(r);
@@ -1542,13 +1545,13 @@ TC_DELETE_ENTRY(const IPT_CHAINLABEL cha
 
 		c->num_rules--;
 		iptcc_delete_rule(i);
+		iptcc_delete_rule(r); /* free and decrement references */
 
 		set_changed(*handle);
-		free(r);
 		return 1;
 	}
 
-	free(r);
+	iptcc_delete_rule(r); /* free and decrement references */
 	errno = ENOENT;
 	return 0;
 }

Re: [PATCH] BUG: libiptc chain references bug

[Pablo Neira Ayuso]

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

Hi Jesper,

Jesper Dangaard Brouer wrote:
> 
> Correcting a chain references increment bug in libiptc.
> 
> The bug lies in function iptc_delete_entry() / TC_DELETE_ENTRY.  The
> problem is the construction of "r" the rule entry, that is used for
> comparison. The problem is that the function iptcc_map_target()
> increase the target chains references count.
> 
> The fix is to use function iptcc_delete_rule() to delete the "r" rule
> (as it decrement the counter again). To make it work a small NULL
> pointer check is also added iptcc_delete_rule().
> 
> Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>

I don't like too much the is-the-rule-in-list checking in delete_entry, 
please, could you tell me what you think about the patch attached? I 
think it's cleaner. Thanks.

-- 
The dawn of the fourth age of Linux firewalling is coming; a time of 
great struggle and heroic deeds -- J.Kadlecsik got inspired by J.Morris

[-- Attachment #2: x --]
[-- Type: text/plain, Size: 637 bytes --]

Based on Jesper Dangaard Brouer's patch.

Index: libiptc/libiptc.c
===================================================================
--- libiptc/libiptc.c	(revisión: 6644)
+++ libiptc/libiptc.c	(copia de trabajo)
@@ -1543,6 +1543,14 @@
 		c->num_rules--;
 		iptcc_delete_rule(i);
 
+		/* Since iptcc_map_target increments refcounting of
+		 * the target chain used by the fake rule, once we
+		 * have matched it against the real rule, do not forget
+		 * to drop the refcount that the fake rule holds */
+		if (r->type == IPTCC_R_JUMP
+		    && r->jump)
+			r->jump->references--;
+
 		set_changed(*handle);
 		free(r);
 		return 1;

Re: [PATCH] BUG: libiptc chain references bug

[Jesper Dangaard Brouer]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2077 bytes --]


On Sun, 16 Jul 2006, Pablo Neira Ayuso wrote:

> Hi Jesper,

Hi Pablo, nice to hear from you!

> Jesper Dangaard Brouer wrote:
>> 
>> Correcting a chain references increment bug in libiptc.
>> 
>> The bug lies in function iptc_delete_entry() / TC_DELETE_ENTRY.  The
>> problem is the construction of "r" the rule entry, that is used for
>> comparison. The problem is that the function iptcc_map_target()
>> increase the target chains references count.
>> 
>> The fix is to use function iptcc_delete_rule() to delete the "r" rule
>> (as it decrement the counter again). To make it work a small NULL
>> pointer check is also added iptcc_delete_rule().
>> 
>> Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>
>
> I don't like too much the is-the-rule-in-list checking in delete_entry, 
> please, could you tell me what you think about the patch attached? I think 
> it's cleaner. Thanks.

First of all, (no offence) your patch will not work, as it does not catch 
all control-flow cases. That is, if no match was found your patch does not 
decrement the refcount. Hint, look at the places free(r) is called.

I have attached a patch, that does catch all cases.  This is achived by 
adding a else statement to the if statement where iptcc_map_target is 
called.

I did consider, your strategy, but the reason I decided not to, was that I 
though it was cleaner to always call "iptcc_delete_rule" when we want to 
delete a rule, instead of free'ing it manually.

Well, I think Patrick should make the decision. As long as we fix the bug, 
I don't care which patch goes in.

Cheers,
   Jesper Brouer

p.s. Pablo, I was not going to piss in the bush... I was just discussing 
with Gert Hansen about how dry the plant was... Perhaps the plant would 
have preferred that I would have taken a leak... See you around ;-)

--
-------------------------------------------------------------------
MSc. Master of Computer Science
Dept. of Computer Science, University of Copenhagen
Author of http://www.adsl-optimizer.dk
-------------------------------------------------------------------

[-- Attachment #2: Type: TEXT/PLAIN, Size: 721 bytes --]

Index: libiptc/libiptc.c
===================================================================
--- libiptc/libiptc.c	(revision 6644)
+++ libiptc/libiptc.c	(working copy)
@@ -1518,8 +1518,16 @@ TC_DELETE_ENTRY(const IPT_CHAINLABEL cha
 	if (!iptcc_map_target(*handle, r)) {
 		DEBUGP("unable to map target of rule for chain `%s'\n", chain);
 		free(r);
 		return 0;
+	} else {
+		/* iptcc_map_target increment target chain references
+		 * since this is a fake rule only used for matching
+		 * the chain references count is decremented again. 
+		 */
+		if (r->type == IPTCC_R_JUMP
+		    && r->jump)
+			r->jump->references--;
 	}
 
 	list_for_each_entry(i, &c->rules, list) {
 		unsigned char *mask;

Re: [PATCH] BUG: libiptc chain references bug

[Pablo Neira Ayuso]

Jesper Dangaard Brouer wrote:
> On Sun, 16 Jul 2006, Pablo Neira Ayuso wrote:
>> Jesper Dangaard Brouer wrote:
>>>
>>> Correcting a chain references increment bug in libiptc.
>>>
>>> The bug lies in function iptc_delete_entry() / TC_DELETE_ENTRY.  The
>>> problem is the construction of "r" the rule entry, that is used for
>>> comparison. The problem is that the function iptcc_map_target()
>>> increase the target chains references count.
>>>
>>> The fix is to use function iptcc_delete_rule() to delete the "r" rule
>>> (as it decrement the counter again). To make it work a small NULL
>>> pointer check is also added iptcc_delete_rule().
>>>
>>> Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>
>>
>>
>> I don't like too much the is-the-rule-in-list checking in 
>> delete_entry, please, could you tell me what you think about the patch 
>> attached? I think it's cleaner. Thanks.
> 
> First of all, (no offence) your patch will not work, as it does not 
> catch all control-flow cases. That is, if no match was found your patch 
> does not decrement the refcount. Hint, look at the places free(r) is 
> called.

You are right, I missed that exit path.

> I have attached a patch, that does catch all cases.  This is achived by 
> adding a else statement to the if statement where iptcc_map_target is 
> called.

I think I prefer this one, let see what Patrick thinks.

> I did consider, your strategy, but the reason I decided not to, was that 
> I though it was cleaner to always call "iptcc_delete_rule" when we want 
> to delete a rule, instead of free'ing it manually.
> 
> Well, I think Patrick should make the decision. As long as we fix the 
> bug, I don't care which patch goes in.
> 
> Cheers,
>   Jesper Brouer
> 
> p.s. Pablo, I was not going to piss in the bush... I was just discussing 
> with Gert Hansen about how dry the plant was... Perhaps the plant would 
> have preferred that I would have taken a leak... See you around ;-)

That bush surely would prefer it these damned hot days of summer 8)

-- 
The dawn of the fourth age of Linux firewalling is coming; a time of 
great struggle and heroic deeds -- J.Kadlecsik got inspired by J.Morris

Re: [PATCH] BUG: libiptc chain references bug

[Patrick McHardy]

Jesper Dangaard Brouer wrote:
> 
> On Sun, 16 Jul 2006, Pablo Neira Ayuso wrote:
> 
>> I don't like too much the is-the-rule-in-list checking in
>> delete_entry, please, could you tell me what you think about the patch
>> attached? I think it's cleaner. Thanks.
> 
> 
> First of all, (no offence) your patch will not work, as it does not
> catch all control-flow cases. That is, if no match was found your patch
> does not decrement the refcount. Hint, look at the places free(r) is
> called.
> 
> I have attached a patch, that does catch all cases.  This is achived by
> adding a else statement to the if statement where iptcc_map_target is
> called.
> 
> I did consider, your strategy, but the reason I decided not to, was that
> I though it was cleaner to always call "iptcc_delete_rule" when we want
> to delete a rule, instead of free'ing it manually.
> 
> Well, I think Patrick should make the decision. As long as we fix the
> bug, I don't care which patch goes in.


I like the second patch better than the first one, so I've applied
this one. Thanks Jesper.