[PATCH v2 0/3] conntrack: fixes for handling unknown protocols

[PATCH v2 0/3] conntrack: fixes for handling unknown protocols

[Mikhail Sennikovsky]

Hi Pablo & all,

Thanks a lot for the fast feedback!
Here is the updated patch set addressing your comments.

Any further comments/suggestions are very welcome.

Regards,
Mikhail

Mikhail Sennikovsky (3):
  conntrack: set reply l4 proto for unknown protocol
  conntrack: fix protocol number parsing
  conntrack: fix -o save dump for unknown protocols

 extensions/libct_proto_unknown.c    | 11 +++++++++
 src/conntrack.c                     | 28 ++++++++++++++++++++--
 tests/conntrack/testsuite/00create  | 37 +++++++++++++++++++++++++++++
 tests/conntrack/testsuite/09dumpopt | 26 ++++++++++++++++++++
 4 files changed, 100 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] conntrack: set reply l4 proto for unknown protocol

[Mikhail Sennikovsky]

Withouth reply l4 protocol being set consistently the mnl_cb_run
(in fact the kernel) would return EINVAL.

Make sure the reply l4 protocol is set properly for unknown
protocols.
Include testcases covering the issue.

Signed-off-by: Mikhail Sennikovsky <mikhail.sennikovskii@ionos.com>
---
 extensions/libct_proto_unknown.c   | 11 +++++++++++
 tests/conntrack/testsuite/00create | 27 +++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/extensions/libct_proto_unknown.c b/extensions/libct_proto_unknown.c
index 2a47704..b877c56 100644
--- a/extensions/libct_proto_unknown.c
+++ b/extensions/libct_proto_unknown.c
@@ -21,10 +21,21 @@ static void help(void)
 	fprintf(stdout, "  no options (unsupported)\n");
 }
 
+static void final_check(unsigned int flags,
+		        unsigned int cmd,
+		        struct nf_conntrack *ct)
+{
+	if (nfct_attr_is_set(ct, ATTR_REPL_L3PROTO) &&
+	    nfct_attr_is_set(ct, ATTR_L4PROTO) &&
+	    !nfct_attr_is_set(ct, ATTR_REPL_L4PROTO))
+		nfct_set_attr_u8(ct, ATTR_REPL_L4PROTO, nfct_get_attr_u8(ct, ATTR_L4PROTO));
+}
+
 struct ctproto_handler ct_proto_unknown = {
 	.name 		= "unknown",
 	.help		= help,
 	.opts		= opts,
+	.final_check	= final_check,
 	.version	= VERSION,
 };
 
diff --git a/tests/conntrack/testsuite/00create b/tests/conntrack/testsuite/00create
index 911e711..9962e23 100644
--- a/tests/conntrack/testsuite/00create
+++ b/tests/conntrack/testsuite/00create
@@ -34,3 +34,30 @@
 -I -t 29 -u SEEN_REPLY -s 1.1.1.1 -d 2.2.2.2 -r 2.2.2.2 -q 1.1.1.1 -p icmp --icmp-type 8 --icmp-code 0 --icmp-id 1226 ; OK
 # delete icmp ping request entry
 -D -u SEEN_REPLY -s 1.1.1.1 -d 2.2.2.2 -r 2.2.2.2 -q 1.1.1.1 -p icmp --icmp-type 8 --icmp-code 0 --icmp-id 1226 ; OK
+# Test protocols unknown by the conntrack tool
+# IGMP
+-I -t 10 -s 0.0.0.0 -d 224.0.0.22 -r 224.0.0.22 -q 0.0.0.0 -p 2 ; OK
+# Create again - should fail
+-I -t 10 -s 0.0.0.0 -d 224.0.0.22 -r 224.0.0.22 -q 0.0.0.0 -p 2 ; BAD
+# repeat using protocol name instead of the value, should fail as well
+-I -t 10 -s 0.0.0.0 -d 224.0.0.22 -r 224.0.0.22 -q 0.0.0.0 -p igmp ; BAD
+# delete
+-D -s 0.0.0.0 -d 224.0.0.22 -r 224.0.0.22 -q 0.0.0.0 -p 2 ; OK
+# delete again should fail
+-D -s 0.0.0.0 -d 224.0.0.22 -r 224.0.0.22 -q 0.0.0.0 -p 2 ; BAD
+# create using protocol name instead of the value
+-I -t 10 -s 0.0.0.0 -d 224.0.0.22 -r 224.0.0.22 -q 0.0.0.0 -p igmp ; OK
+# update
+-U -t 11 -s 0.0.0.0 -d 224.0.0.22 -r 224.0.0.22 -q 0.0.0.0 -p 2 ; OK
+# delete
+-D -s 0.0.0.0 -d 224.0.0.22 -r 224.0.0.22 -q 0.0.0.0 -p 2 ; OK
+# delete again should fail
+-D -s 0.0.0.0 -d 224.0.0.22 -r 224.0.0.22 -q 0.0.0.0 -p igmp ; BAD
+# take some protocol that is not normally not in /etc/protocols
+-I -t 10 -s 0.0.0.0 -d 224.0.0.22 -r 224.0.0.22 -q 0.0.0.0 -p 200 ; OK
+# update
+-U -t 11 -s 0.0.0.0 -d 224.0.0.22 -r 224.0.0.22 -q 0.0.0.0 -p 200 ; OK
+# delete
+-D -s 0.0.0.0 -d 224.0.0.22 -r 224.0.0.22 -q 0.0.0.0 -p 200 ; OK
+# delete again
+-D -s 0.0.0.0 -d 224.0.0.22 -r 224.0.0.22 -q 0.0.0.0 -p 200 ; BAD
-- 
2.25.1

[PATCH v2 2/3] conntrack: fix protocol number parsing

[Mikhail Sennikovsky]

Before this commit it was possible to successfully create a ct entry
passing -p 256 and -p some_nonsense.
In both cases an entry with the protocol=0 would be created.

Do not allow invalid protocol values to -p option.
Include testcases covering the issue.

Signed-off-by: Mikhail Sennikovsky <mikhail.sennikovskii@ionos.com>
---
 src/conntrack.c                    | 19 +++++++++++++++++--
 tests/conntrack/testsuite/00create | 10 ++++++++++
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/src/conntrack.c b/src/conntrack.c
index 500e736..e381543 100644
--- a/src/conntrack.c
+++ b/src/conntrack.c
@@ -882,6 +882,21 @@ static int ct_save_snprintf(char *buf, size_t len,
 
 extern struct ctproto_handler ct_proto_unknown;
 
+static int parse_proto_num(const char *str)
+{
+	char *endptr;
+	long val;
+
+	val = strtol(str, &endptr, 0);
+	if (val >= IPPROTO_MAX ||
+	    val < 0 ||
+	    endptr == str ||
+	    *endptr != '\0')
+		return -1;
+
+	return val;
+}
+
 static struct ctproto_handler *findproto(char *name, int *pnum)
 {
 	struct ctproto_handler *cur;
@@ -901,8 +916,8 @@ static struct ctproto_handler *findproto(char *name, int *pnum)
 		return &ct_proto_unknown;
 	}
 	/* using a protocol number? */
-	protonum = atoi(name);
-	if (protonum >= 0 && protonum <= IPPROTO_MAX) {
+	protonum = parse_proto_num(name);
+	if (protonum >= 0) {
 		/* try lookup by number, perhaps this protocol is supported */
 		list_for_each_entry(cur, &proto_list, head) {
 			if (cur->protonum == protonum) {
diff --git a/tests/conntrack/testsuite/00create b/tests/conntrack/testsuite/00create
index 9962e23..af22f18 100644
--- a/tests/conntrack/testsuite/00create
+++ b/tests/conntrack/testsuite/00create
@@ -61,3 +61,13 @@
 -D -s 0.0.0.0 -d 224.0.0.22 -r 224.0.0.22 -q 0.0.0.0 -p 200 ; OK
 # delete again
 -D -s 0.0.0.0 -d 224.0.0.22 -r 224.0.0.22 -q 0.0.0.0 -p 200 ; BAD
+# Invalid protocol values
+# 256 should fail
+-I -t 10 -s 1.1.1.1 -d 2.2.2.2 -r 2.2.2.2 -q 1.1.1.1 -p 256 ; BAD
+# take some invalid protocol name
+-I -t 10 -s 0.0.0.0 -d 224.0.0.22 -r 224.0.0.22 -q 0.0.0.0 -p foo ; BAD
+# take some other invalid protocol values
+-I -t 10 -s 0.0.0.0 -d 224.0.0.22 -r 224.0.0.22 -q 0.0.0.0 -p -10 ; BAD
+-I -t 10 -s 0.0.0.0 -d 224.0.0.22 -r 224.0.0.22 -q 0.0.0.0 -p 2000 ; BAD
+-I -t 10 -s 0.0.0.0 -d 224.0.0.22 -r 224.0.0.22 -q 0.0.0.0 -p 20foo ; BAD
+-I -t 10 -s 0.0.0.0 -d 224.0.0.22 -r 224.0.0.22 -q 0.0.0.0 -p foo20 ; BAD
-- 
2.25.1

[PATCH v2 3/3] conntrack: fix -o save dump for unknown protocols

[Mikhail Sennikovsky]

Make sure the protocol (-p) option is included in the -o save
ct entry dumps for L4 protocols unknown to the conntrack tool.

Do not use getprotobynumber for unknown protocols to ensure
"-o save" data incompatibility between hosts having different
/etc/protocols contents.

Include testcases covering the issue.

Signed-off-by: Mikhail Sennikovsky <mikhail.sennikovskii@ionos.com>
---
 src/conntrack.c                     |  9 +++++++++
 tests/conntrack/testsuite/09dumpopt | 26 ++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/src/conntrack.c b/src/conntrack.c
index e381543..d49ac1a 100644
--- a/src/conntrack.c
+++ b/src/conntrack.c
@@ -800,6 +800,7 @@ static int ct_save_snprintf(char *buf, size_t len,
 	struct ctproto_handler *cur;
 	uint8_t l3proto, l4proto;
 	int tuple_attrs[4] = {};
+	bool l4proto_set;
 	unsigned i;
 	int ret;
 
@@ -860,6 +861,7 @@ static int ct_save_snprintf(char *buf, size_t len,
 
 	l4proto = nfct_get_attr_u8(ct, ATTR_L4PROTO);
 
+	l4proto_set = false;
 	/* is it in the list of supported protocol? */
 	list_for_each_entry(cur, &proto_list, head) {
 		if (cur->protonum != l4proto)
@@ -870,9 +872,16 @@ static int ct_save_snprintf(char *buf, size_t len,
 
 		ret = ct_snprintf_opts(buf + offset, len, ct, cur->print_opts);
 		BUFFER_SIZE(ret, size, len, offset);
+
+		l4proto_set = true;
 		break;
 	}
 
+	if (!l4proto_set) {
+		ret = snprintf(buf + offset, len, "-p %d ", l4proto);
+		BUFFER_SIZE(ret, size, len, offset);
+	}
+
 	/* skip trailing space, if any */
 	for (; size && buf[size-1] == ' '; --size)
 		buf[size-1] = '\0';
diff --git a/tests/conntrack/testsuite/09dumpopt b/tests/conntrack/testsuite/09dumpopt
index 447590b..c1e0e6e 100644
--- a/tests/conntrack/testsuite/09dumpopt
+++ b/tests/conntrack/testsuite/09dumpopt
@@ -145,3 +145,29 @@
 -D -w 11 -u SEEN_REPLY -s 1.1.1.1 -d 2.2.2.2 -r 2.2.2.2 -q 1.1.1.1 -p icmp --icmp-type 8 --icmp-code 0 --icmp-id 1226 ; OK
 # clean up after yourself
 -D -w 10 ; OK
+# Cover protocols unknown to the conntrack tool
+# Create a conntrack entries
+# IGMP
+-I -w 10 -t 59 -s 0.0.0.0 -d 224.0.0.22 -r 224.0.0.22 -q 0.0.0.0 -p 2 ;
+# Some fency protocol
+-I -w 10 -t 59 -s 0.0.0.0 -d 224.0.0.22 -r 224.0.0.22 -q 0.0.0.0 -p 200 ;
+# Some fency protocol with IPv6
+-I -w 10 -t 59 -s 2001:DB8::1.1.1.1 -d 2001:DB8::2.2.2.2 -p 200 ;
+-R - ; OK
+# copy to zone 11
+-L -w 10 -o save ; |s/-w 10/-w 11/g
+-R - ; OK
+# Delete stuff in zone 10, should succeed
+# IGMP
+-D -w 10 -s 0.0.0.0 -d 224.0.0.22 -r 224.0.0.22 -q 0.0.0.0 -p 2 ; OK
+# Some fency protocol
+-D -w 10  -s 0.0.0.0 -d 224.0.0.22 -r 224.0.0.22 -q 0.0.0.0 -p 200 ; OK
+# Some fency protocol with IPv6
+-D -w 10 -s 2001:DB8::1.1.1.1 -d 2001:DB8::2.2.2.2 -p 200 ; OK
+# Delete stuff in zone 11, should succeed
+# IGMP
+-D -w 11 -s 0.0.0.0 -d 224.0.0.22 -r 224.0.0.22 -q 0.0.0.0 -p 2 ; OK
+# Some fency protocol
+-D -w 11  -s 0.0.0.0 -d 224.0.0.22 -r 224.0.0.22 -q 0.0.0.0 -p 200 ; OK
+# Some fency protocol with IPv6
+-D -w 11 -s 2001:DB8::1.1.1.1 -d 2001:DB8::2.2.2.2 -p 200 ; OK
-- 
2.25.1

Re: [PATCH v2 0/3] conntrack: fixes for handling unknown protocols

[Pablo Neira Ayuso]

On Fri, Jun 24, 2022 at 05:01:23PM +0200, Mikhail Sennikovsky wrote:
[...]
> Here is the updated patch set addressing your comments.

Series applied, thanks