Bug when updating ICMP flows using conntrack tools

Bug when updating ICMP flows using conntrack tools

[Luuk Paulussen]

Hi,

We recently upgraded the version of conntrack-tools and libnetfilter-conntrack that we are using and have found a bug when using conntrack to update ICMP flows. I've tracked down where the bug is occurring, but am looking for some guidance on the preferred way to fix it.

E.g. Doing the following when there are icmp flows present has no effect on ICMP flows:
conntrack -f ipv4 -U --mark=1

In conntrack-tools in conntrack.c in update_cb, the ORIG data and META data are copied, but not the REPL data. This has been this way for quite some time.
nfct_copy(tmp, ct, NFCT_CP_ORIG);
nfct_copy(tmp, obj, NFCT_CP_META);

However, in libnetfilter-conntrack the way that the message is built has been changed, and in nfct_nlmsg_build, the check about whether to build the REPL tuple has been extended to include
test_bit(ATTR_ICMP_TYPE, ct->head.set) ||
test_bit(ATTR_ICMP_CODE, ct->head.set) ||
test_bit(ATTR_ICMP_ID, ct->head.set)

In the old building functions, these bits weren't present, so it wouldn't try to build the REPL tuple, so everything worked. However, now it tries to build the REPL tuple, and because none of the REPL data is present, building the tuple fails and it errors out without sending the request to the kernel.

My question is whether it is better to remove the checks in libnetfilter-conntrack or add the copy of the REPL data in conntrack tools.

Upgraded:
libnetfilter-conntrack from 1.0.6 to 1.0.8
conntrack-tools from 1.4.4 to 1.4.6

Kind regards,
Luuk

Re: Bug when updating ICMP flows using conntrack tools

[Florian Westphal]

Luuk Paulussen <Luuk.Paulussen@alliedtelesis.co.nz> wrote:
> However, in libnetfilter-conntrack the way that the message is built has been changed, and in nfct_nlmsg_build, the check about whether to build the REPL tuple has been extended to include
> test_bit(ATTR_ICMP_TYPE, ct->head.set) ||
> test_bit(ATTR_ICMP_CODE, ct->head.set) ||
> test_bit(ATTR_ICMP_ID, ct->head.set)

That looks like a bug, those checks should only be done for ORIG.

[PATCH libnetfilter_conntrack] conntrack: Don't use ICMP attrs in decision to build repl tuple

[Luuk Paulussen]

conntrack-tools doesn't set the REPL attributes by default for updates,
so for ICMP flows, the update won't be sent as building the repl tuple
will fail.

Signed-off-by: Luuk Paulussen <luuk.paulussen@alliedtelesis.co.nz>
---
 src/conntrack/build_mnl.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/src/conntrack/build_mnl.c b/src/conntrack/build_mnl.c
index d9ad268..0067a1c 100644
--- a/src/conntrack/build_mnl.c
+++ b/src/conntrack/build_mnl.c
@@ -496,10 +496,7 @@ nfct_nlmsg_build(struct nlmsghdr *nlh, const struct nf_conntrack *ct)
 	    test_bit(ATTR_REPL_PORT_DST, ct->head.set) ||
 	    test_bit(ATTR_REPL_L3PROTO, ct->head.set) ||
 	    test_bit(ATTR_REPL_L4PROTO, ct->head.set) ||
-	    test_bit(ATTR_REPL_ZONE, ct->head.set) ||
-	    test_bit(ATTR_ICMP_TYPE, ct->head.set) ||
-	    test_bit(ATTR_ICMP_CODE, ct->head.set) ||
-	    test_bit(ATTR_ICMP_ID, ct->head.set)) {
+	    test_bit(ATTR_REPL_ZONE, ct->head.set)) {
 		const struct __nfct_tuple *t = &ct->repl;
 		struct nlattr *nest;
 
-- 
2.31.0

Re: [PATCH libnetfilter_conntrack] conntrack: Don't use ICMP attrs in decision to build repl tuple

[Pablo Neira Ayuso]

On Fri, Mar 19, 2021 at 08:59:19AM +1300, Luuk Paulussen wrote:
> conntrack-tools doesn't set the REPL attributes by default for updates,
> so for ICMP flows, the update won't be sent as building the repl tuple
> will fail.

Applied, thanks.