RE: iptables patch

RE: iptables patch

[Kevin Peeters]

Hello,

I am using the 'iptables' source code in one of my software projects. More in detail, I am calling libiptc and libxtables from my own software API to add/delete/... iptables firewall rules.

While developing, I bumped into one issue while using libxtables and made a patch for it which we now use on our checkout of the 'iptables' repository. We do however use multiple checkouts of this repository in different places and don't want to add the patch to each of those checkouts.
Would it be possible for you to add this patch to the mainline of your repository so we can stop patching it locally?

The details about the patch:
In libxtables/xtables.c:

The libxtables code uses a xtables_pending_matches, xtables_pending_targets, xtables_matches and xtables_targets pointer list to track all (pending) matches and targets registered to the current iptables command. In my code, I add/delete firewall rules multiple times from one main process (without killing the main process in between) by calling xtables_init_all, xtables_register_target and xtables_register_match every time. When a rule is added, I call xtables_fini to clean up.

I do notice when adding a rule in my code twice that on the second time, the (pending) targets/matches lists are not empty and when I try to register the same target (the one I registered in the previous rule) again, it links to itself and creates an infinite loop.

I managed to fix it by setting the pointers to NULL in xtables_fini.

The patch:
diff --git a/libxtables/xtables.c b/libxtables/xtables.c
index 96fd783a..ac9300c7 100644
--- a/libxtables/xtables.c
+++ b/libxtables/xtables.c
@@ -327,6 +327,48 @@ void xtables_announce_chain(const char *name)
 		notargets_hlist_insert(name);
 }
 
+static void xtables_release_matches(void)
+{
+	struct xtables_match **dptr, **ptr;
+
+	for (dptr = &xtables_pending_matches; *dptr; ) {
+		ptr = &((*dptr)->next);
+		*dptr = NULL;
+		dptr = ptr;
+
+	}
+	xtables_pending_matches = NULL;
+
+	for (dptr = &xtables_matches; *dptr; ) {
+		ptr = &((*dptr)->next);
+		*dptr = NULL;
+		dptr = ptr;
+
+	}
+	xtables_matches = NULL;
+}
+
+static void xtables_release_targets(void)
+{
+	struct xtables_target **dptr, **ptr;
+
+	for (dptr = &xtables_pending_targets; *dptr; ) {
+		ptr = &((*dptr)->next);
+		*dptr = NULL;
+		dptr = ptr;
+
+	}
+	xtables_pending_targets = NULL;
+
+	for (dptr = &xtables_targets; *dptr; ) {
+		ptr = &((*dptr)->next);
+		*dptr = NULL;
+		dptr = ptr;
+
+	}
+	xtables_targets = NULL;
+}
+
 void xtables_init(void)
 {
 	/* xtables cannot be used with setuid in a safe way. */
@@ -366,6 +408,8 @@ void xtables_fini(void)
 	dlreg_free();
 #endif
 	notargets_hlist_free();
+	xtables_release_matches();
+	xtables_release_targets();
 }
 
 void xtables_set_nfproto(uint8_t nfproto)

Thanks in advance!

Kind regards,
Kevin Peeters

Re: iptables patch

[Phil Sutter]

Hi,

On Tue, Mar 28, 2023 at 07:28:50AM +0000, Kevin Peeters wrote:
> I am using the 'iptables' source code in one of my software projects. More in detail, I am calling libiptc and libxtables from my own software API to add/delete/... iptables firewall rules.
> 
> While developing, I bumped into one issue while using libxtables and made a patch for it which we now use on our checkout of the 'iptables' repository. We do however use multiple checkouts of this repository in different places and don't want to add the patch to each of those checkouts.
> Would it be possible for you to add this patch to the mainline of your repository so we can stop patching it locally?
> 
> The details about the patch:
> In libxtables/xtables.c:
> 
> The libxtables code uses a xtables_pending_matches, xtables_pending_targets, xtables_matches and xtables_targets pointer list to track all (pending) matches and targets registered to the current iptables command. In my code, I add/delete firewall rules multiple times from one main process (without killing the main process in between) by calling xtables_init_all, xtables_register_target and xtables_register_match every time. When a rule is added, I call xtables_fini to clean up.

I don't think you should call xtables_register_{target,match} over and
over again. Why don't you follow what iptables does and call
xtables_find_{target,match} to lookup an extension? It tries loading the
DSO which calls xtables_register_*. After adding the rule, you should
free the rule, not deinit the library.

> I do notice when adding a rule in my code twice that on the second time, the (pending) targets/matches lists are not empty and when I try to register the same target (the one I registered in the previous rule) again, it links to itself and creates an infinite loop.
> 
> I managed to fix it by setting the pointers to NULL in xtables_fini.

Your patch seems to leak memory because the list elements are not freed.

Cheers, Phil

RE: iptables patch

[Kevin Peeters]

Hello Phil,

>> I am using the 'iptables' source code in one of my software projects. More in detail, I am calling libiptc and libxtables from my own software API to add/delete/... iptables firewall rules.
>> 
>> While developing, I bumped into one issue while using libxtables and made a patch for it which we now use on our checkout of the 'iptables' repository. We do however use multiple checkouts of this repository in different places and don't want to add the patch to each of those checkouts.
>> Would it be possible for you to add this patch to the mainline of your repository so we can stop patching it locally?
>> 
>> The details about the patch:
>> In libxtables/xtables.c:
>> 
>> The libxtables code uses a xtables_pending_matches, xtables_pending_targets, xtables_matches and xtables_targets pointer list to track all (pending) matches and targets registered to the current iptables command. In my code, I add/delete firewall rules multiple times from one main process (without killing the main process in between) by calling xtables_init_all, xtables_register_target and xtables_register_match every time. When a rule is added, I call xtables_fini to clean up.

> I don't think you should call xtables_register_{target,match} over and over again. Why don't you follow what iptables does and call xtables_find_{target,match} to lookup an extension? It tries loading the DSO which calls xtables_register_*. After adding the rule, you should free the rule, not deinit the library.

If I understand correctly, xtables_find_* will only look for the desired match/target in the list of pending matches/targets. If the match/target is never registered up front, the list of pending matches/targets will be empty and xtables_find_* will fail. This is also done in the iptables flow, e.g. in extensions/libxt_tcp.c.

I do free the rule after adding it, and it felt reasonable to deinit the library as well, as this is also done for iptables.

>> I do notice when adding a rule in my code twice that on the second time, the (pending) targets/matches lists are not empty and when I try to register the same target (the one I registered in the previous rule) again, it links to itself and creates an infinite loop.
>> 
>> I managed to fix it by setting the pointers to NULL in xtables_fini.

> Your patch seems to leak memory because the list elements are not freed.

Indeed, but every time I try to free the list elements, I get crashes, e.g.:

*** Error in `/usr/local/sbin/iptables': free(): invalid pointer: 0x000055cb6cea7168 ***
======= Backtrace: =========
/lib/x86_64-linux-gnu/libc.so.6(+0x70bfb)[0x7f665dcf1bfb]
/lib/x86_64-linux-gnu/libc.so.6(+0x76fc6)[0x7f665dcf7fc6]
/lib/x86_64-linux-gnu/libc.so.6(+0x7780e)[0x7f665dcf880e]
/usr/local/sbin/iptables(+0x4762d)[0x55cb6cbfd62d]
/usr/local/sbin/iptables(+0x148db)[0x55cb6cbca8db]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf1)[0x7f665dca12e1]
/usr/local/sbin/iptables(+0x130ca)[0x55cb6cbc90ca]
======= Memory map: ========
55cb6cbb6000-55cb6cc29000 r-xp 00000000 08:01 14288797                   /usr/local/sbin/xtables-legacy-multi
55cb6ce28000-55cb6ce33000 r--p 00072000 08:01 14288797                   /usr/local/sbin/xtables-legacy-multi
55cb6ce33000-55cb6ce3e000 rw-p 0007d000 08:01 14288797                   /usr/local/sbin/xtables-legacy-multi
55cb6ce3e000-55cb6cea8000 rw-p 00000000 00:00 0
55cb6ed21000-55cb6ed42000 rw-p 00000000 00:00 0                          [heap]
7f6658000000-7f6658021000 rw-p 00000000 00:00 0
7f6658021000-7f665c000000 ---p 00000000 00:00 0
7f665da6a000-7f665da80000 r-xp 00000000 08:01 14417934                   /lib/x86_64-linux-gnu/libgcc_s.so.1
7f665da80000-7f665dc7f000 ---p 00016000 08:01 14417934                   /lib/x86_64-linux-gnu/libgcc_s.so.1
7f665dc7f000-7f665dc80000 r--p 00015000 08:01 14417934                   /lib/x86_64-linux-gnu/libgcc_s.so.1
7f665dc80000-7f665dc81000 rw-p 00016000 08:01 14417934                   /lib/x86_64-linux-gnu/libgcc_s.so.1
7f665dc81000-7f665de16000 r-xp 00000000 08:01 14417941                   /lib/x86_64-linux-gnu/libc-2.24.so
7f665de16000-7f665e016000 ---p 00195000 08:01 14417941                   /lib/x86_64-linux-gnu/libc-2.24.so
7f665e016000-7f665e01a000 r--p 00195000 08:01 14417941                   /lib/x86_64-linux-gnu/libc-2.24.so
7f665e01a000-7f665e01c000 rw-p 00199000 08:01 14417941                   /lib/x86_64-linux-gnu/libc-2.24.so
7f665e01c000-7f665e020000 rw-p 00000000 00:00 0
7f665e020000-7f665e123000 r-xp 00000000 08:01 14417947                   /lib/x86_64-linux-gnu/libm-2.24.so
7f665e123000-7f665e322000 ---p 00103000 08:01 14417947                   /lib/x86_64-linux-gnu/libm-2.24.so
7f665e322000-7f665e323000 r--p 00102000 08:01 14417947                   /lib/x86_64-linux-gnu/libm-2.24.so
7f665e323000-7f665e324000 rw-p 00103000 08:01 14417947                   /lib/x86_64-linux-gnu/libm-2.24.so
7f665e324000-7f665e347000 r-xp 00000000 08:01 14417935                   /lib/x86_64-linux-gnu/ld-2.24.so
7f665e52c000-7f665e530000 rw-p 00000000 00:00 0
7f665e546000-7f665e547000 rw-p 00000000 00:00 0
7f665e547000-7f665e548000 r--p 00023000 08:01 14417935                   /lib/x86_64-linux-gnu/ld-2.24.so
7f665e548000-7f665e549000 rw-p 00024000 08:01 14417935                   /lib/x86_64-linux-gnu/ld-2.24.so
7f665e549000-7f665e54a000 rw-p 00000000 00:00 0
7ffecad4c000-7ffecad6d000 rw-p 00000000 00:00 0                          [stack]
7ffecadb0000-7ffecadb3000 r--p 00000000 00:00 0                          [vvar]
7ffecadb3000-7ffecadb5000 r-xp 00000000 00:00 0                          [vdso]
ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0                  [vsyscall]
Aborted


This crash happens when calling free(dptr) in:
static void xtables_release_matches(void)
{
	struct xtables_match **dptr, **ptr;

	for (dptr = &xtables_pending_matches; *dptr; ) {
		ptr = &((*dptr)->next);
		free(dptr);
		*dptr = NULL;
		dptr = ptr;

	}
	free(xtables_pending_matches);
	xtables_pending_matches = NULL;

	for (dptr = &xtables_matches; *dptr; ) {
		ptr = &((*dptr)->next);
		free(dptr);
		*dptr = NULL;
		dptr = ptr;

	}
	free(xtables_matches);
	xtables_matches = NULL;
}

Kind regards,
Kevin Peeters 

Re: iptables patch

[Phil Sutter]

Hi Kevin,

On Wed, Mar 29, 2023 at 07:29:06AM +0000, Kevin Peeters wrote:
> >> I am using the 'iptables' source code in one of my software projects. More in detail, I am calling libiptc and libxtables from my own software API to add/delete/... iptables firewall rules.
> >> 
> >> While developing, I bumped into one issue while using libxtables and made a patch for it which we now use on our checkout of the 'iptables' repository. We do however use multiple checkouts of this repository in different places and don't want to add the patch to each of those checkouts.
> >> Would it be possible for you to add this patch to the mainline of your repository so we can stop patching it locally?
> >> 
> >> The details about the patch:
> >> In libxtables/xtables.c:
> >> 
> >> The libxtables code uses a xtables_pending_matches, xtables_pending_targets, xtables_matches and xtables_targets pointer list to track all (pending) matches and targets registered to the current iptables command. In my code, I add/delete firewall rules multiple times from one main process (without killing the main process in between) by calling xtables_init_all, xtables_register_target and xtables_register_match every time. When a rule is added, I call xtables_fini to clean up.
> 
> > I don't think you should call xtables_register_{target,match} over and over again. Why don't you follow what iptables does and call xtables_find_{target,match} to lookup an extension? It tries loading the DSO which calls xtables_register_*. After adding the rule, you should free the rule, not deinit the library.
> 
> If I understand correctly, xtables_find_* will only look for the desired match/target in the list of pending matches/targets. If the match/target is never registered up front, the list of pending matches/targets will be empty and xtables_find_* will fail. This is also done in the iptables flow, e.g. in extensions/libxt_tcp.c.

The functions search for the extension in xtables_pending_* list, but if
not found they will call load_extension() unless NO_SHARED_LIBS is
defined. In the latter case, extension code is built-in and extensions'
_init() functions are called from init_extensions*() functions which in
turn are called by iptables at program start.

> I do free the rule after adding it, and it felt reasonable to deinit the library as well, as this is also done for iptables.

The various iptables binaries deinit the library at program exit and
don't reuse it. 

I don't know what you used as blueprint for your implementation, but you
might want to have a look at iptables_restore_main() in
iptables/iptables-restore.c. It basically does:

| xtables_init()
| xtables_set_nfproto()
| 
| init_extensions()
| 
| /* do all the work, repeatedly, unlimited if reading from stdin */
| 
| xtables_fini()
| exit()

Cheers, Phil

RE: iptables patch

[Kevin Peeters]

Hello 

>> >> I am using the 'iptables' source code in one of my software projects. More in detail, I am calling libiptc and libxtables from my own software API to add/delete/... iptables firewall rules.
>> >> 
>> >> While developing, I bumped into one issue while using libxtables and made a patch for it which we now use on our checkout of the 'iptables' repository. We do however use multiple checkouts of this repository in different places and don't want to add the patch to each of those checkouts.
>> >> Would it be possible for you to add this patch to the mainline of your repository so we can stop patching it locally?
>> >> 
>> >> The details about the patch:
>> >> In libxtables/xtables.c:
>> >> 
>> >> The libxtables code uses a xtables_pending_matches, xtables_pending_targets, xtables_matches and xtables_targets pointer list to track all (pending) matches and targets registered to the current iptables command. In my code, I add/delete firewall rules multiple times from one main process (without killing the main process in between) by calling xtables_init_all, xtables_register_target and xtables_register_match every time. When a rule is added, I call xtables_fini to clean up.
>> 
>> > I don't think you should call xtables_register_{target,match} over and over again. Why don't you follow what iptables does and call xtables_find_{target,match} to lookup an extension? It tries loading the DSO which calls xtables_register_*. After adding the rule, you should free the rule, not deinit the library.
>> 
>> If I understand correctly, xtables_find_* will only look for the desired match/target in the list of pending matches/targets. If the match/target is never registered up front, the list of pending matches/targets will be empty and xtables_find_* will fail. This is also done in the iptables flow, e.g. in extensions/libxt_tcp.c.

>The functions search for the extension in xtables_pending_* list, but if not found they will call load_extension() unless NO_SHARED_LIBS is defined. In the latter case, extension code is built-in and extensions'
_init() functions are called from init_extensions*() functions which in turn are called by iptables at program start.

Ah indeed! It seems like I have NO_SHARED_LIBS defined, which is why I have to manually register the targets/matches.

>> I do free the rule after adding it, and it felt reasonable to deinit the library as well, as this is also done for iptables.

> The various iptables binaries deinit the library at program exit and don't reuse it. 

> I don't know what you used as blueprint for your implementation, but you might want to have a look at iptables_restore_main() in iptables/iptables-restore.c. It basically does:

> | xtables_init()
> | xtables_set_nfproto()
> | 
> | init_extensions()
> | 
> | /* do all the work, repeatedly, unlimited if reading from stdin */
> | 
> | xtables_fini()
> | exit()

This is what I based my code on indeed. The only difference in my code is that I do the init()/fini() calls on every rule I add. This is no hard requirement so I can change it if you think that is the best option.

Cheers, Phil