[PATCH] netfilter: xt_NFLOG: allow 128 character log prefixes

[PATCH] netfilter: xt_NFLOG: allow 128 character log prefixes

[Kyle Bowman]

From: Alex Forster <aforster@cloudflare.com>

nftables defines NF_LOG_PREFIXLEN as 128 characters, while iptables
limits the NFLOG prefix to 64 characters. In order to eventually make
the two consistent, introduce a v1 target revision of xt_NFLOG that
allows userspace to provide a 128 character NFLOG prefix.

Signed-off-by: Alex Forster <aforster@cloudflare.com>
Signed-off-by: Kyle Bowman <kbowman@cloudflare.com>
---
 include/uapi/linux/netfilter/xt_NFLOG.h | 11 ++++
 net/netfilter/xt_NFLOG.c                | 73 +++++++++++++++++++++----
 2 files changed, 73 insertions(+), 11 deletions(-)

diff --git a/include/uapi/linux/netfilter/xt_NFLOG.h b/include/uapi/linux/netfilter/xt_NFLOG.h
index 517809771909..3f1119a2e522 100644
--- a/include/uapi/linux/netfilter/xt_NFLOG.h
+++ b/include/uapi/linux/netfilter/xt_NFLOG.h
@@ -3,6 +3,7 @@
 #define _XT_NFLOG_TARGET

 #include <linux/types.h>
+#include <linux/netfilter/nf_log.h>

 #define XT_NFLOG_DEFAULT_GROUP		0x1
 #define XT_NFLOG_DEFAULT_THRESHOLD	0
@@ -22,4 +23,14 @@ struct xt_nflog_info {
 	char		prefix[64];
 };

+struct xt_nflog_info_v1 {
+	/* 'len' will be used iff you set XT_NFLOG_F_COPY_LEN in flags */
+	__u32	len;
+	__u16	group;
+	__u16	threshold;
+	__u16	flags;
+	__u16	pad;
+	char	prefix[NF_LOG_PREFIXLEN];
+};
+
 #endif /* _XT_NFLOG_TARGET */
diff --git a/net/netfilter/xt_NFLOG.c b/net/netfilter/xt_NFLOG.c
index fb5793208059..82279a6be0ff 100644
--- a/net/netfilter/xt_NFLOG.c
+++ b/net/netfilter/xt_NFLOG.c
@@ -39,6 +39,28 @@ nflog_tg(struct sk_buff *skb, const struct xt_action_param *par)
 	return XT_CONTINUE;
 }

+static unsigned int
+nflog_tg_v1(struct sk_buff *skb, const struct xt_action_param *par)
+{
+	const struct xt_nflog_info_v1 *info = par->targinfo;
+	struct net *net = xt_net(par);
+	struct nf_loginfo li;
+
+	li.type		     = NF_LOG_TYPE_ULOG;
+	li.u.ulog.copy_len   = info->len;
+	li.u.ulog.group	     = info->group;
+	li.u.ulog.qthreshold = info->threshold;
+	li.u.ulog.flags	     = 0;
+
+	if (info->flags & XT_NFLOG_F_COPY_LEN)
+		li.u.ulog.flags |= NF_LOG_F_COPY_LEN;
+
+	nf_log_packet(net, xt_family(par), xt_hooknum(par), skb, xt_in(par),
+		      xt_out(par), &li, "%s", info->prefix);
+
+	return XT_CONTINUE;
+}
+
 static int nflog_tg_check(const struct xt_tgchk_param *par)
 {
 	const struct xt_nflog_info *info = par->targinfo;
@@ -51,30 +73,59 @@ static int nflog_tg_check(const struct xt_tgchk_param *par)
 	return nf_logger_find_get(par->family, NF_LOG_TYPE_ULOG);
 }

+static int nflog_tg_check_v1(const struct xt_tgchk_param *par)
+{
+	const struct xt_nflog_info_v1 *info = par->targinfo;
+
+	if (info->flags & ~XT_NFLOG_MASK)
+		return -EINVAL;
+	if (info->prefix[sizeof(info->prefix) - 1] != '\0')
+		return -EINVAL;
+
+	return nf_logger_find_get(par->family, NF_LOG_TYPE_ULOG);
+}
+
 static void nflog_tg_destroy(const struct xt_tgdtor_param *par)
 {
 	nf_logger_put(par->family, NF_LOG_TYPE_ULOG);
 }

-static struct xt_target nflog_tg_reg __read_mostly = {
-	.name       = "NFLOG",
-	.revision   = 0,
-	.family     = NFPROTO_UNSPEC,
-	.checkentry = nflog_tg_check,
-	.destroy    = nflog_tg_destroy,
-	.target     = nflog_tg,
-	.targetsize = sizeof(struct xt_nflog_info),
-	.me         = THIS_MODULE,
+static void nflog_tg_destroy_v1(const struct xt_tgdtor_param *par)
+{
+	nf_logger_put(par->family, NF_LOG_TYPE_ULOG);
+}
+
+static struct xt_target nflog_tg_reg[] __read_mostly = {
+	{
+		.name       = "NFLOG",
+		.revision   = 0,
+		.family     = NFPROTO_UNSPEC,
+		.checkentry = nflog_tg_check,
+		.destroy    = nflog_tg_destroy,
+		.target     = nflog_tg,
+		.targetsize = sizeof(struct xt_nflog_info),
+		.me         = THIS_MODULE,
+	},
+	{
+		.name       = "NFLOG",
+		.revision   = 1,
+		.family     = NFPROTO_UNSPEC,
+		.checkentry = nflog_tg_check_v1,
+		.destroy    = nflog_tg_destroy_v1,
+		.target     = nflog_tg_v1,
+		.targetsize = sizeof(struct xt_nflog_info_v1),
+		.me         = THIS_MODULE,
+	}
 };

 static int __init nflog_tg_init(void)
 {
-	return xt_register_target(&nflog_tg_reg);
+	return xt_register_targets(nflog_tg_reg, ARRAY_SIZE(nflog_tg_reg));
 }

 static void __exit nflog_tg_exit(void)
 {
-	xt_unregister_target(&nflog_tg_reg);
+	xt_unregister_targets(nflog_tg_reg, ARRAY_SIZE(nflog_tg_reg));
 }

 module_init(nflog_tg_init);
--
2.32.0

Re: [PATCH] netfilter: xt_NFLOG: allow 128 character log prefixes

[Pablo Neira Ayuso]

Hi,

On Tue, Jul 27, 2021 at 02:00:00PM -0500, Kyle Bowman wrote:
> From: Alex Forster <aforster@cloudflare.com>
> 
> nftables defines NF_LOG_PREFIXLEN as 128 characters, while iptables
> limits the NFLOG prefix to 64 characters. In order to eventually make
> the two consistent [...]

Why do you need to make the two consistent? iptables NFLOG prefix
length is a subset of nftables log action, this is sufficient for the
iptables-nft layer. I might be missing the use-case on your side,
could you please elaborate?

Re: [PATCH] netfilter: xt_NFLOG: allow 128 character log prefixes

[Alex Forster]

(And again, this time as plain-text...)

> Why do you need to make the two consistent? iptables NFLOG prefix
> length is a subset of nftables log action, this is sufficient for the
> iptables-nft layer. I might be missing the use-case on your side,
> could you please elaborate?

We use the nflog prefix space to attach various bits of metadata to
iptables and nftables rules that are dynamically generated and
installed on our edge. 63 printable chars is a bit too tight to fit
everything that we need, so we're running this patch internally and
are looking to upstream it.

Alex Forster

Re: [PATCH] netfilter: xt_NFLOG: allow 128 character log prefixes

[Pablo Neira Ayuso]

On Tue, Jul 27, 2021 at 03:06:05PM -0500, Alex Forster wrote:
> (And again, this time as plain-text...)
> 
> > Why do you need to make the two consistent? iptables NFLOG prefix
> > length is a subset of nftables log action, this is sufficient for the
> > iptables-nft layer. I might be missing the use-case on your side,
> > could you please elaborate?
> 
> We use the nflog prefix space to attach various bits of metadata to
> iptables and nftables rules that are dynamically generated and
> installed on our edge. 63 printable chars is a bit too tight to fit
> everything that we need, so we're running this patch internally and
> are looking to upstream it.

It should be possible to update iptables-nft to use nft_log from
userspace (instead of xt_LOG) which removes this limitation, there is
no need for a kernel upgrade.

Re: [PATCH] netfilter: xt_NFLOG: allow 128 character log prefixes

[Alex Forster]

> It should be possible to update iptables-nft to use nft_log from
> userspace (instead of xt_LOG) which removes this limitation, there is
> no need for a kernel upgrade.

We have been able to migrate some parts of this workload to the
nftables subsystem by treating network namespaces sort of like VRFs.
Unfortunately, we have not been able to use nftables to handle all
traffic, since it does not have an equivalent for xt_bpf.

Alex Forster

Re: [PATCH] netfilter: xt_NFLOG: allow 128 character log prefixes

[Pablo Neira Ayuso]

On Tue, Jul 27, 2021 at 04:22:10PM -0500, Alex Forster wrote:
> > It should be possible to update iptables-nft to use nft_log from
> > userspace (instead of xt_LOG) which removes this limitation, there is
> > no need for a kernel upgrade.
> 
> We have been able to migrate some parts of this workload to the
> nftables subsystem by treating network namespaces sort of like VRFs.
> Unfortunately, we have not been able to use nftables to handle all
> traffic, since it does not have an equivalent for xt_bpf.

I'm not refering to nftables, I'm refering to iptables-nft.

Re: [PATCH] netfilter: xt_NFLOG: allow 128 character log prefixes

[Alex Forster]

> I'm not refering to nftables, I'm refering to iptables-nft.

Possibly I'm misunderstanding. Here's a realistic-ish example of a
rule we might install:

    iptables -A INPUT -d 11.22.33.44/32 -m bpf --bytecode "43,0 0 0
0,48 0 0 0,...sic..." -m statistic --mode random --probability 0.0001
-j NFLOG --nflog-prefix "drop 10000 c37904a83b344404
e4ec6050966d4d2f9952745de09d1308"

Is there a way to install such a rule with an nflog prefix that is >63 chars?

Re: [PATCH] netfilter: xt_NFLOG: allow 128 character log prefixes

[Pablo Neira Ayuso]

On Tue, Jul 27, 2021 at 04:44:42PM -0500, Alex Forster wrote:
> > I'm not refering to nftables, I'm refering to iptables-nft.
> 
> Possibly I'm misunderstanding. Here's a realistic-ish example of a
> rule we might install:
> 
>     iptables -A INPUT -d 11.22.33.44/32 -m bpf --bytecode "43,0 0 0
> 0,48 0 0 0,...sic..." -m statistic --mode random --probability 0.0001
> -j NFLOG --nflog-prefix "drop 10000 c37904a83b344404
> e4ec6050966d4d2f9952745de09d1308"
> 
> Is there a way to install such a rule with an nflog prefix that is >63 chars?

Yes, you can update iptables-nft to use nft_log instead of xt_LOG,
that requires no kernel upgrades and it will work with older kernels.

Re: [PATCH] netfilter: xt_NFLOG: allow 128 character log prefixes

[Alex Forster]

> Yes, you can update iptables-nft to use nft_log instead of xt_LOG,
> that requires no kernel upgrades and it will work with older kernels.

I've always been under the impression that mixing xtables and nftables
was impossible. Forgive me, but I just want to clarify one more time:
you're saying we should be able to modify iptables-nft such that the
following rule will use xt_bpf to match a packet and then nft_log to
log it, rather than xt_log as it does today?

    iptables-nft -A test-chain -d 11.22.33.44/32 -m bpf --bytecode
"1,6 0 0 65536" -j NFLOG --nflog-prefix
"0123456789012345678901234567890123456789012345678901234567890123456789"

We had some unexplained performance loss when we were evaluating
switching to iptables-nft, but if this sort of mixing is possible then
it is certainly worth reevaluating.

Re: [PATCH] netfilter: xt_NFLOG: allow 128 character log prefixes

[Pablo Neira Ayuso]

On Tue, Jul 27, 2021 at 05:45:09PM -0500, Alex Forster wrote:
> > Yes, you can update iptables-nft to use nft_log instead of xt_LOG,
> > that requires no kernel upgrades and it will work with older kernels.
> 
> I've always been under the impression that mixing xtables and nftables
> was impossible. Forgive me, but I just want to clarify one more time:
> you're saying we should be able to modify iptables-nft such that the
> following rule will use xt_bpf to match a packet and then nft_log to
> log it, rather than xt_log as it does today?

You could actually use *any* of the existing extensions to match a
packet, the matching side is completely irrelevant to this picture.

As I said, userspace iptables-nft can be updated to use nft_log
instead of xt_LOG.

>     iptables-nft -A test-chain -d 11.22.33.44/32 -m bpf --bytecode
> "1,6 0 0 65536" -j NFLOG --nflog-prefix
> "0123456789012345678901234567890123456789012345678901234567890123456789"
> 
> We had some unexplained performance loss when we were evaluating
> switching to iptables-nft, but if this sort of mixing is possible then
> it is certainly worth reevaluating.

Re: [netfilter-core] [PATCH] netfilter: xt_NFLOG: allow 128 character log prefixes

[Phil Sutter]

Hi,

On Tue, Jul 27, 2021 at 05:45:09PM -0500, Alex Forster via netfilter-core wrote:
> > Yes, you can update iptables-nft to use nft_log instead of xt_LOG,
> > that requires no kernel upgrades and it will work with older kernels.
> 
> I've always been under the impression that mixing xtables and nftables
> was impossible. Forgive me, but I just want to clarify one more time:
> you're saying we should be able to modify iptables-nft such that the
> following rule will use xt_bpf to match a packet and then nft_log to
> log it, rather than xt_log as it does today?

iptables-nft is free to use either xtables extensions or native nftables
expressions and it may mix them within the same rule. Internally, this
is all nftables but calling xtables extensions via a compat expression.

You might want to check iptables commit ccf154d7420c0 ("xtables: Don't
use native nftables comments") for reference, it does the opposite of
what you want to do.

>     iptables-nft -A test-chain -d 11.22.33.44/32 -m bpf --bytecode
> "1,6 0 0 65536" -j NFLOG --nflog-prefix
> "0123456789012345678901234567890123456789012345678901234567890123456789"

Keep in mind though, you may end with rulesets an older iptables(-nft)
will reject. I've seen people running into such compat issues when using
containers for things they shouldn't, but that's a different story.

> We had some unexplained performance loss when we were evaluating
> switching to iptables-nft, but if this sort of mixing is possible then
> it is certainly worth reevaluating.

There were some significant performance improvements in the near past.
Repeating the check might yield better results in this aspect, too.

Cheers, Phil

Re: [netfilter-core] [PATCH] netfilter: xt_NFLOG: allow 128 character log prefixes

[Kyle Bowman]

Hi Phil,

On Wed, Jul 28, 2021 at 03:43:47AM +0200, Phil Sutter wrote:
> You might want to check iptables commit ccf154d7420c0 ("xtables: Don't
> use native nftables comments") for reference, it does the opposite of
> what you want to do.

I went ahead and looked through this commit and also found found the
code that initially added this functionality; commit d64ef34a9961
("iptables-compat: use nft built-in comments support ").

Additionally I found some other commits that moved code to nft native
implementations of the xtables counterpart so that proved helpful.

After a couple days of research I did end up figuring out what to do
and have added a (mostly complete) native nft log support in
iptables-nft. It all seems to work without any kernel changes
required. The only problem I'm now faced with is that since we want to
take the string passed into the iptables-nft command and add it to the
nftnl expression (`NFTNL_EXPR_LOG_PREFIX`) I'm not entirely sure where
to get the original sized string from aside from `argv` in the `struct
iptables_command_state`. I would get it from the `struct
xt_nflog_info`, but that's going to store the truncated version and we
would like to be able to store 128 characters of the string as opposed
to 64.

Any recommendations about how I might do this safely?

An example of the program running with my patch:

kyle@debian:~/netfilter/iptables$ sudo /usr/local/sbin/iptables-nft -S

-P INPUT ACCEPT
-P FORWARD ACCEPT
-P OUTPUT ACCEPT
-N test-chain

kyle@debian:~/netfilter/iptables$ sudo /usr/local/sbin/iptables-nft -A
test-chain -j NFLOG --nflog-prefix "this string is hard coded for
testing so what I put here doesn't end up in the prefix"

kyle@debian:~/netfilter/iptables$ sudo /usr/local/sbin/iptables-nft -S
-P INPUT ACCEPT
-P FORWARD ACCEPT
-P OUTPUT ACCEPT
-N test-chain
-A test-chain -j NFLOG --nflog-prefix  "iff the value at the end is 12
then this string is truncated 12"

kyle@debian:~/netfilter/iptables$ sudo nft list ruleset
table ip filter {
	chain test-chain {
    counter packets 0 bytes 0 log prefix "iff the value at the end is
    12 then this string is truncated 123"
}

	[...]
}

See below for the patch:

From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Kbowman <kbowman@cloudflare.com>
Date: Thu, 29 Jul 2021 15:12:28 -0500
Subject: [PATCH] iptables-nft: use nft built-in logging instead of xt_NFLOG

Replaces the use of xt_NFLOG with the nft built-in log statement.

This additionally adds support for using longer log prefixes of 128
characters in size. A caveat to this is that the string will be
truncated when the rule is printed via iptables-nft but will remain
untruncated in nftables.

Some changes have also been made to nft_is_expr_compatible() since
xt_NFLOG does not support log level or log flags. With the new changes
this means that when a log is used and sets either NFTNL_EXPR_LOG_LEVEL
or NFTNL_LOG_FLAGS to a value aside from their default (log level
defaults to 4, log flags will not be set) this will produce a
compatibility error.
---
 iptables/nft-shared.c | 45 +++++++++++++++++++++++++++++++++++++++++++
 iptables/nft.c        | 38 ++++++++++++++++++++++++++++++++++++
 iptables/nft.h        |  1 +
 3 files changed, 84 insertions(+)

diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
index 4253b081..b5259db0 100644
--- a/iptables/nft-shared.c
+++ b/iptables/nft-shared.c
@@ -22,6 +22,7 @@
 
 #include <linux/netfilter/xt_comment.h>
 #include <linux/netfilter/xt_limit.h>
+#include <linux/netfilter/xt_NFLOG.h>
 
 #include <libmnl/libmnl.h>
 #include <libnftnl/rule.h>
@@ -595,6 +596,48 @@ static void nft_parse_limit(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
 		ctx->h->ops->parse_match(match, ctx->cs);
 }
 
+static void nft_parse_log(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
+{
+    __u16 group =  nftnl_expr_get_u16(e, NFTNL_EXPR_LOG_GROUP);
+    __u16 qthreshold = nftnl_expr_get_u16(e, NFTNL_EXPR_LOG_QTHRESHOLD);
+    __u32 snaplen = nftnl_expr_get_u32(e, NFTNL_EXPR_LOG_SNAPLEN);
+    const char *prefix = nftnl_expr_get_str(e, NFTNL_EXPR_LOG_PREFIX);
+    struct xtables_target *target;
+    struct xt_entry_target *t;
+    size_t target_size;
+
+    void *data = ctx->cs;
+
+    target = xtables_find_target("NFLOG", XTF_TRY_LOAD);
+    if (target == NULL)
+        return;
+
+    target_size = XT_ALIGN(sizeof(struct xt_entry_target)) + target->size;
+
+    t = xtables_calloc(1, target_size);
+    t->u.target_size = target_size;
+    strcpy(t->u.user.name, target->name);
+    t->u.user.revision = target->revision;
+
+    target->t = t;
+
+    struct xt_nflog_info *info = xtables_malloc(sizeof(struct xt_nflog_info));
+    info->group = group;
+    info->len = snaplen;
+    info->threshold = qthreshold;
+
+    /* Here, because we allow 128 characters in nftables but only 64
+     * characters in xtables (in xt_nflog_info specifically), we may
+     * end up truncating the string when parsing it.
+     */
+    strncpy(info->prefix, prefix, sizeof(info->prefix));
+    info->prefix[sizeof(info->prefix) - 1] = '\0';
+
+    memcpy(&target->t->data, info, target->size);
+
+    ctx->h->ops->parse_target(target, data);
+}
+
 static void nft_parse_lookup(struct nft_xt_ctx *ctx, struct nft_handle *h,
 			     struct nftnl_expr *e)
 {
@@ -644,6 +687,8 @@ void nft_rule_to_iptables_command_state(struct nft_handle *h,
 			nft_parse_limit(&ctx, expr);
 		else if (strcmp(name, "lookup") == 0)
 			nft_parse_lookup(&ctx, h, expr);
+		else if (strcmp(name, "log") == 0)
+		    nft_parse_log(&ctx, expr);
 
 		expr = nftnl_expr_iter_next(iter);
 	}
diff --git a/iptables/nft.c b/iptables/nft.c
index f1deb82f..dce8fe0b 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -39,6 +39,7 @@
 #include <linux/netfilter/nf_tables_compat.h>
 
 #include <linux/netfilter/xt_limit.h>
+#include <linux/netfilter/xt_NFLOG.h>
 
 #include <libmnl/libmnl.h>
 #include <libnftnl/gen.h>
@@ -1340,6 +1341,8 @@ int add_action(struct nftnl_rule *r, struct iptables_command_state *cs,
 		       ret = add_verdict(r, NF_DROP);
 	       else if (strcmp(cs->jumpto, XTC_LABEL_RETURN) == 0)
 		       ret = add_verdict(r, NFT_RETURN);
+	       else if (strcmp(cs->jumpto, "NFLOG") == 0)
+	           ret = add_log(r, cs);
 	       else
 		       ret = add_target(r, cs->target->t);
        } else if (strlen(cs->jumpto) > 0) {
@@ -1352,6 +1355,36 @@ int add_action(struct nftnl_rule *r, struct iptables_command_state *cs,
        return ret;
 }
 
+int add_log(struct nftnl_rule *r, struct iptables_command_state *cs)
+{
+    struct nftnl_expr *expr;
+    struct xt_nflog_info *info = (struct xt_nflog_info *)cs->target->t->data;
+
+    expr = nftnl_expr_alloc("log");
+    if (!expr)
+        return -ENOMEM;
+
+    if (info->prefix != NULL) {
+        //char prefix[NF_LOG_PREFIXLEN] = {};
+
+        // get prefix here from somewhere...
+        // maybe in cs->argv?
+        nftnl_expr_set_str(expr, NFTNL_EXPR_LOG_PREFIX, "iff the value at the end is 12 then this string is truncated 123");
+    }
+    if (info->group) {
+        nftnl_expr_set_u16(expr, NFTNL_EXPR_LOG_GROUP, info->group);
+        if (info->flags & XT_NFLOG_F_COPY_LEN)
+            nftnl_expr_set_u32(expr, NFTNL_EXPR_LOG_SNAPLEN,
+                               info->len);
+        if (info->threshold)
+            nftnl_expr_set_u16(expr, NFTNL_EXPR_LOG_QTHRESHOLD,
+                               info->threshold);
+    }
+
+    nftnl_rule_add_expr(r, expr);
+    return 0;
+}
+
 static void nft_rule_print_debug(struct nftnl_rule *r, struct nlmsghdr *nlh)
 {
 #ifdef NLDEBUG
@@ -3487,6 +3520,11 @@ static int nft_is_expr_compatible(struct nftnl_expr *expr, void *data)
 	    nftnl_expr_get_u32(expr, NFTNL_EXPR_LIMIT_FLAGS) == 0)
 		return 0;
 
+	if (!strcmp(name, "log") &&
+	    nftnl_expr_get_u32(expr, NFTNL_EXPR_LOG_LEVEL) == 4 &&
+	    !nftnl_expr_is_set(expr, NFTNL_EXPR_LOG_FLAGS))
+	    return 0;
+
 	return -1;
 }
 
diff --git a/iptables/nft.h b/iptables/nft.h
index 4ac7e009..28dc81b7 100644
--- a/iptables/nft.h
+++ b/iptables/nft.h
@@ -193,6 +193,7 @@ int add_match(struct nft_handle *h, struct nftnl_rule *r, struct xt_entry_match
 int add_target(struct nftnl_rule *r, struct xt_entry_target *t);
 int add_jumpto(struct nftnl_rule *r, const char *name, int verdict);
 int add_action(struct nftnl_rule *r, struct iptables_command_state *cs, bool goto_set);
+int add_log(struct nftnl_rule *r, struct iptables_command_state *cs);
 char *get_comment(const void *data, uint32_t data_len);
 
 enum nft_rule_print {
-- 
2.32.0

Re: [netfilter-core] [PATCH] netfilter: xt_NFLOG: allow 128 character log prefixes

[Jeremy Sowden]

[-- Attachment #1.1: Type: text/plain, Size: 9308 bytes --]

On 2021-07-30, at 13:27:49 -0500, Kyle Bowman wrote:
> On Wed, Jul 28, 2021 at 03:43:47AM +0200, Phil Sutter wrote:
> > You might want to check iptables commit ccf154d7420c0 ("xtables:
> > Don't use native nftables comments") for reference, it does the
> > opposite of what you want to do.
>
> I went ahead and looked through this commit and also found found the
> code that initially added this functionality; commit d64ef34a9961
> ("iptables-compat: use nft built-in comments support ").
>
> Additionally I found some other commits that moved code to nft native
> implementations of the xtables counterpart so that proved helpful.
>
> After a couple days of research I did end up figuring out what to do
> and have added a (mostly complete) native nft log support in
> iptables-nft. It all seems to work without any kernel changes
> required. The only problem I'm now faced with is that since we want to
> take the string passed into the iptables-nft command and add it to the
> nftnl expression (`NFTNL_EXPR_LOG_PREFIX`) I'm not entirely sure where
> to get the original sized string from aside from `argv` in the `struct
> iptables_command_state`. I would get it from the `struct
> xt_nflog_info`, but that's going to store the truncated version and we
> would like to be able to store 128 characters of the string as opposed
> to 64.
>
> Any recommendations about how I might do this safely?

The xtables_target struct has a `udata` member which I think would be
suitable.  libxt_RATEEST does something similar.  I've attached a patch
which should apply cleanly on top of yours.

Here's an example:

  $ sudo /usr/local/sbin/iptables-nft -A INPUT -j NFLOG --nflog-prefix '0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef|0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF|'
  $ sudo /usr/local/sbin/iptables-nft -L INPUT
  # Warning: iptables-legacy tables present, use iptables-legacy to see them
  Chain INPUT (policy ACCEPT)
  target     prot opt source               destination
  NFLOG      all  --  anywhere             anywhere             nflog-prefix  0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcde
  $ sudo nft list ruleset
  table ip filter {
          chain INPUT {
                  type filter hook input priority filter; policy accept;
                  counter packets 113 bytes 8894 log prefix "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef|0123456789ABCDEF0123456789ABCDEF0123456789ABCDEF0123456789ABCD"
          }
  }

J.

> From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
> From: Kbowman <kbowman@cloudflare.com>
> Date: Thu, 29 Jul 2021 15:12:28 -0500
> Subject: [PATCH] iptables-nft: use nft built-in logging instead of xt_NFLOG
>
> Replaces the use of xt_NFLOG with the nft built-in log statement.
>
> This additionally adds support for using longer log prefixes of 128
> characters in size. A caveat to this is that the string will be
> truncated when the rule is printed via iptables-nft but will remain
> untruncated in nftables.
>
> Some changes have also been made to nft_is_expr_compatible() since
> xt_NFLOG does not support log level or log flags. With the new changes
> this means that when a log is used and sets either
> NFTNL_EXPR_LOG_LEVEL or NFTNL_LOG_FLAGS to a value aside from their
> default (log level defaults to 4, log flags will not be set) this will
> produce a compatibility error.
> ---
>  iptables/nft-shared.c | 45 +++++++++++++++++++++++++++++++++++++++++++
>  iptables/nft.c        | 38 ++++++++++++++++++++++++++++++++++++
>  iptables/nft.h        |  1 +
>  3 files changed, 84 insertions(+)

One note about formatting: you've used four spaces for indentation, but
Netfilter uses tabs.

> diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
> index 4253b081..b5259db0 100644
> --- a/iptables/nft-shared.c
> +++ b/iptables/nft-shared.c
> @@ -22,6 +22,7 @@
>
>  #include <linux/netfilter/xt_comment.h>
>  #include <linux/netfilter/xt_limit.h>
> +#include <linux/netfilter/xt_NFLOG.h>
>
>  #include <libmnl/libmnl.h>
>  #include <libnftnl/rule.h>
> @@ -595,6 +596,48 @@ static void nft_parse_limit(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
>  		ctx->h->ops->parse_match(match, ctx->cs);
>  }
>
> +static void nft_parse_log(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
> +{
> +    __u16 group =  nftnl_expr_get_u16(e, NFTNL_EXPR_LOG_GROUP);
> +    __u16 qthreshold = nftnl_expr_get_u16(e, NFTNL_EXPR_LOG_QTHRESHOLD);
> +    __u32 snaplen = nftnl_expr_get_u32(e, NFTNL_EXPR_LOG_SNAPLEN);
> +    const char *prefix = nftnl_expr_get_str(e, NFTNL_EXPR_LOG_PREFIX);
> +    struct xtables_target *target;
> +    struct xt_entry_target *t;
> +    size_t target_size;
> +
> +    void *data = ctx->cs;
> +
> +    target = xtables_find_target("NFLOG", XTF_TRY_LOAD);
> +    if (target == NULL)
> +        return;
> +
> +    target_size = XT_ALIGN(sizeof(struct xt_entry_target)) + target->size;
> +
> +    t = xtables_calloc(1, target_size);
> +    t->u.target_size = target_size;
> +    strcpy(t->u.user.name, target->name);
> +    t->u.user.revision = target->revision;
> +
> +    target->t = t;
> +
> +    struct xt_nflog_info *info = xtables_malloc(sizeof(struct xt_nflog_info));
> +    info->group = group;
> +    info->len = snaplen;
> +    info->threshold = qthreshold;
> +
> +    /* Here, because we allow 128 characters in nftables but only 64
> +     * characters in xtables (in xt_nflog_info specifically), we may
> +     * end up truncating the string when parsing it.
> +     */
> +    strncpy(info->prefix, prefix, sizeof(info->prefix));
> +    info->prefix[sizeof(info->prefix) - 1] = '\0';
> +
> +    memcpy(&target->t->data, info, target->size);
> +
> +    ctx->h->ops->parse_target(target, data);
> +}
> +
>  static void nft_parse_lookup(struct nft_xt_ctx *ctx, struct nft_handle *h,
>  			     struct nftnl_expr *e)
>  {
> @@ -644,6 +687,8 @@ void nft_rule_to_iptables_command_state(struct nft_handle *h,
>  			nft_parse_limit(&ctx, expr);
>  		else if (strcmp(name, "lookup") == 0)
>  			nft_parse_lookup(&ctx, h, expr);
> +		else if (strcmp(name, "log") == 0)
> +		    nft_parse_log(&ctx, expr);
>
>  		expr = nftnl_expr_iter_next(iter);
>  	}
> diff --git a/iptables/nft.c b/iptables/nft.c
> index f1deb82f..dce8fe0b 100644
> --- a/iptables/nft.c
> +++ b/iptables/nft.c
> @@ -39,6 +39,7 @@
>  #include <linux/netfilter/nf_tables_compat.h>
>
>  #include <linux/netfilter/xt_limit.h>
> +#include <linux/netfilter/xt_NFLOG.h>
>
>  #include <libmnl/libmnl.h>
>  #include <libnftnl/gen.h>
> @@ -1340,6 +1341,8 @@ int add_action(struct nftnl_rule *r, struct iptables_command_state *cs,
>  		       ret = add_verdict(r, NF_DROP);
>  	       else if (strcmp(cs->jumpto, XTC_LABEL_RETURN) == 0)
>  		       ret = add_verdict(r, NFT_RETURN);
> +	       else if (strcmp(cs->jumpto, "NFLOG") == 0)
> +	           ret = add_log(r, cs);
>  	       else
>  		       ret = add_target(r, cs->target->t);
>         } else if (strlen(cs->jumpto) > 0) {
> @@ -1352,6 +1355,36 @@ int add_action(struct nftnl_rule *r, struct iptables_command_state *cs,
>         return ret;
>  }
>
> +int add_log(struct nftnl_rule *r, struct iptables_command_state *cs)
> +{
> +    struct nftnl_expr *expr;
> +    struct xt_nflog_info *info = (struct xt_nflog_info *)cs->target->t->data;
> +
> +    expr = nftnl_expr_alloc("log");
> +    if (!expr)
> +        return -ENOMEM;
> +
> +    if (info->prefix != NULL) {
> +        //char prefix[NF_LOG_PREFIXLEN] = {};
> +
> +        // get prefix here from somewhere...
> +        // maybe in cs->argv?
> +        nftnl_expr_set_str(expr, NFTNL_EXPR_LOG_PREFIX, "iff the value at the end is 12 then this string is truncated 123");
> +    }
> +    if (info->group) {
> +        nftnl_expr_set_u16(expr, NFTNL_EXPR_LOG_GROUP, info->group);
> +        if (info->flags & XT_NFLOG_F_COPY_LEN)
> +            nftnl_expr_set_u32(expr, NFTNL_EXPR_LOG_SNAPLEN,
> +                               info->len);
> +        if (info->threshold)
> +            nftnl_expr_set_u16(expr, NFTNL_EXPR_LOG_QTHRESHOLD,
> +                               info->threshold);
> +    }
> +
> +    nftnl_rule_add_expr(r, expr);
> +    return 0;
> +}
> +
>  static void nft_rule_print_debug(struct nftnl_rule *r, struct nlmsghdr *nlh)
>  {
>  #ifdef NLDEBUG
> @@ -3487,6 +3520,11 @@ static int nft_is_expr_compatible(struct nftnl_expr *expr, void *data)
>  	    nftnl_expr_get_u32(expr, NFTNL_EXPR_LIMIT_FLAGS) == 0)
>  		return 0;
>
> +	if (!strcmp(name, "log") &&
> +	    nftnl_expr_get_u32(expr, NFTNL_EXPR_LOG_LEVEL) == 4 &&
> +	    !nftnl_expr_is_set(expr, NFTNL_EXPR_LOG_FLAGS))
> +	    return 0;
> +
>  	return -1;
>  }
>
> diff --git a/iptables/nft.h b/iptables/nft.h
> index 4ac7e009..28dc81b7 100644
> --- a/iptables/nft.h
> +++ b/iptables/nft.h
> @@ -193,6 +193,7 @@ int add_match(struct nft_handle *h, struct nftnl_rule *r, struct xt_entry_match
>  int add_target(struct nftnl_rule *r, struct xt_entry_target *t);
>  int add_jumpto(struct nftnl_rule *r, const char *name, int verdict);
>  int add_action(struct nftnl_rule *r, struct iptables_command_state *cs, bool goto_set);
> +int add_log(struct nftnl_rule *r, struct iptables_command_state *cs);
>  char *get_comment(const void *data, uint32_t data_len);
>
>  enum nft_rule_print {
> --
> 2.32.0

[-- Attachment #1.2: 0001-extensions-libxt_NFLOG-use-udata-to-store-longer-pre.patch --]
[-- Type: text/x-diff, Size: 2432 bytes --]

From 7bc91dbe4f3cc9f88fbb73137e9be9d1dba89deb Mon Sep 17 00:00:00 2001
From: Jeremy Sowden <jeremy@azazel.net>
Date: Sun, 1 Aug 2021 14:47:52 +0100
Subject: [PATCH] extensions: libxt_NFLOG: use udata to store longer prefixes
 suitable for the nft log statement.

NFLOG truncates the log-prefix to 64 characters which is the limit
supported by iptables-legacy.  We now store the longer 128-character
prefix in struct xtables_target's udata member for use by iptables-nft.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 extensions/libxt_NFLOG.c | 6 ++++++
 iptables/nft.c           | 6 +-----
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/extensions/libxt_NFLOG.c b/extensions/libxt_NFLOG.c
index 02a1b4aa35a3..9057230d7ee7 100644
--- a/extensions/libxt_NFLOG.c
+++ b/extensions/libxt_NFLOG.c
@@ -5,6 +5,7 @@
 #include <getopt.h>
 #include <xtables.h>
 
+#include <linux/netfilter/nf_log.h>
 #include <linux/netfilter/x_tables.h>
 #include <linux/netfilter/xt_NFLOG.h>
 
@@ -53,12 +54,16 @@ static void NFLOG_init(struct xt_entry_target *t)
 
 static void NFLOG_parse(struct xt_option_call *cb)
 {
+	char *nf_log_prefix = cb->udata;
+
 	xtables_option_parse(cb);
 	switch (cb->entry->id) {
 	case O_PREFIX:
 		if (strchr(cb->arg, '\n') != NULL)
 			xtables_error(PARAMETER_PROBLEM,
 				   "Newlines not allowed in --log-prefix");
+
+		snprintf(nf_log_prefix, NF_LOG_PREFIXLEN, "%s", cb->arg);
 		break;
 	}
 }
@@ -149,6 +154,7 @@ static struct xtables_target nflog_target = {
 	.save		= NFLOG_save,
 	.x6_options	= NFLOG_opts,
 	.xlate		= NFLOG_xlate,
+	.udata_size     = NF_LOG_PREFIXLEN
 };
 
 void _init(void)
diff --git a/iptables/nft.c b/iptables/nft.c
index dce8fe0b4a18..13cbf0a8b87b 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1365,11 +1365,7 @@ int add_log(struct nftnl_rule *r, struct iptables_command_state *cs)
         return -ENOMEM;
 
     if (info->prefix != NULL) {
-        //char prefix[NF_LOG_PREFIXLEN] = {};
-
-        // get prefix here from somewhere...
-        // maybe in cs->argv?
-        nftnl_expr_set_str(expr, NFTNL_EXPR_LOG_PREFIX, "iff the value at the end is 12 then this string is truncated 123");
+        nftnl_expr_set_str(expr, NFTNL_EXPR_LOG_PREFIX, cs->target->udata);
     }
     if (info->group) {
         nftnl_expr_set_u16(expr, NFTNL_EXPR_LOG_GROUP, info->group);
-- 
2.30.2


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [netfilter-core] [PATCH] netfilter: xt_NFLOG: allow 128 character log prefixes

[Jeremy Sowden]

[-- Attachment #1.1: Type: text/plain, Size: 9018 bytes --]

[CC trimmed since this is all about Netfilter userspace.]

On 2021-08-01, at 15:14:42 +0100, Jeremy Sowden wrote:
> On 2021-07-30, at 13:27:49 -0500, Kyle Bowman wrote:
> > On Wed, Jul 28, 2021 at 03:43:47AM +0200, Phil Sutter wrote:
> > > You might want to check iptables commit ccf154d7420c0 ("xtables:
> > > Don't use native nftables comments") for reference, it does the
> > > opposite of what you want to do.
> >
> > I went ahead and looked through this commit and also found found the
> > code that initially added this functionality; commit d64ef34a9961
> > ("iptables-compat: use nft built-in comments support ").
> >
> > Additionally I found some other commits that moved code to nft
> > native implementations of the xtables counterpart so that proved
> > helpful.
> >
> > After a couple days of research I did end up figuring out what to do
> > and have added a (mostly complete) native nft log support in
> > iptables-nft. It all seems to work without any kernel changes
> > required. The only problem I'm now faced with is that since we want
> > to take the string passed into the iptables-nft command and add it
> > to the nftnl expression (`NFTNL_EXPR_LOG_PREFIX`) I'm not entirely
> > sure where to get the original sized string from aside from `argv`
> > in the `struct iptables_command_state`. I would get it from the
> > `struct xt_nflog_info`, but that's going to store the truncated
> > version and we would like to be able to store 128 characters of the
> > string as opposed to 64.
> >
> > Any recommendations about how I might do this safely?
>
> The xtables_target struct has a `udata` member which I think would be
> suitable.  libxt_RATEEST does something similar.

Actually, if we embed struct xf_nflog_info in another structure along
with the longer prefix, we can get iptables-nft to print it untruncated.
I've attached a patch.

J.

> > From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
> > From: Kbowman <kbowman@cloudflare.com>
> > Date: Thu, 29 Jul 2021 15:12:28 -0500
> > Subject: [PATCH] iptables-nft: use nft built-in logging instead of xt_NFLOG
> >
> > Replaces the use of xt_NFLOG with the nft built-in log statement.
> >
> > This additionally adds support for using longer log prefixes of 128
> > characters in size. A caveat to this is that the string will be
> > truncated when the rule is printed via iptables-nft but will remain
> > untruncated in nftables.
> >
> > Some changes have also been made to nft_is_expr_compatible() since
> > xt_NFLOG does not support log level or log flags. With the new
> > changes this means that when a log is used and sets either
> > NFTNL_EXPR_LOG_LEVEL or NFTNL_LOG_FLAGS to a value aside from their
> > default (log level defaults to 4, log flags will not be set) this
> > will produce a compatibility error.
> > ---
> >  iptables/nft-shared.c | 45 +++++++++++++++++++++++++++++++++++++++++++
> >  iptables/nft.c        | 38 ++++++++++++++++++++++++++++++++++++
> >  iptables/nft.h        |  1 +
> >  3 files changed, 84 insertions(+)
>
> One note about formatting: you've used four spaces for indentation,
> but Netfilter uses tabs.
>
> > diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
> > index 4253b081..b5259db0 100644
> > --- a/iptables/nft-shared.c
> > +++ b/iptables/nft-shared.c
> > @@ -22,6 +22,7 @@
> >
> >  #include <linux/netfilter/xt_comment.h>
> >  #include <linux/netfilter/xt_limit.h>
> > +#include <linux/netfilter/xt_NFLOG.h>
> >
> >  #include <libmnl/libmnl.h>
> >  #include <libnftnl/rule.h>
> > @@ -595,6 +596,48 @@ static void nft_parse_limit(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
> >  		ctx->h->ops->parse_match(match, ctx->cs);
> >  }
> >
> > +static void nft_parse_log(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
> > +{
> > +    __u16 group =  nftnl_expr_get_u16(e, NFTNL_EXPR_LOG_GROUP);
> > +    __u16 qthreshold = nftnl_expr_get_u16(e, NFTNL_EXPR_LOG_QTHRESHOLD);
> > +    __u32 snaplen = nftnl_expr_get_u32(e, NFTNL_EXPR_LOG_SNAPLEN);
> > +    const char *prefix = nftnl_expr_get_str(e, NFTNL_EXPR_LOG_PREFIX);
> > +    struct xtables_target *target;
> > +    struct xt_entry_target *t;
> > +    size_t target_size;
> > +
> > +    void *data = ctx->cs;
> > +
> > +    target = xtables_find_target("NFLOG", XTF_TRY_LOAD);
> > +    if (target == NULL)
> > +        return;
> > +
> > +    target_size = XT_ALIGN(sizeof(struct xt_entry_target)) + target->size;
> > +
> > +    t = xtables_calloc(1, target_size);
> > +    t->u.target_size = target_size;
> > +    strcpy(t->u.user.name, target->name);
> > +    t->u.user.revision = target->revision;
> > +
> > +    target->t = t;
> > +
> > +    struct xt_nflog_info *info = xtables_malloc(sizeof(struct xt_nflog_info));
> > +    info->group = group;
> > +    info->len = snaplen;
> > +    info->threshold = qthreshold;
> > +
> > +    /* Here, because we allow 128 characters in nftables but only 64
> > +     * characters in xtables (in xt_nflog_info specifically), we may
> > +     * end up truncating the string when parsing it.
> > +     */
> > +    strncpy(info->prefix, prefix, sizeof(info->prefix));
> > +    info->prefix[sizeof(info->prefix) - 1] = '\0';
> > +
> > +    memcpy(&target->t->data, info, target->size);
> > +
> > +    ctx->h->ops->parse_target(target, data);
> > +}
> > +
> >  static void nft_parse_lookup(struct nft_xt_ctx *ctx, struct nft_handle *h,
> >  			     struct nftnl_expr *e)
> >  {
> > @@ -644,6 +687,8 @@ void nft_rule_to_iptables_command_state(struct nft_handle *h,
> >  			nft_parse_limit(&ctx, expr);
> >  		else if (strcmp(name, "lookup") == 0)
> >  			nft_parse_lookup(&ctx, h, expr);
> > +		else if (strcmp(name, "log") == 0)
> > +		    nft_parse_log(&ctx, expr);
> >
> >  		expr = nftnl_expr_iter_next(iter);
> >  	}
> > diff --git a/iptables/nft.c b/iptables/nft.c
> > index f1deb82f..dce8fe0b 100644
> > --- a/iptables/nft.c
> > +++ b/iptables/nft.c
> > @@ -39,6 +39,7 @@
> >  #include <linux/netfilter/nf_tables_compat.h>
> >
> >  #include <linux/netfilter/xt_limit.h>
> > +#include <linux/netfilter/xt_NFLOG.h>
> >
> >  #include <libmnl/libmnl.h>
> >  #include <libnftnl/gen.h>
> > @@ -1340,6 +1341,8 @@ int add_action(struct nftnl_rule *r, struct iptables_command_state *cs,
> >  		       ret = add_verdict(r, NF_DROP);
> >  	       else if (strcmp(cs->jumpto, XTC_LABEL_RETURN) == 0)
> >  		       ret = add_verdict(r, NFT_RETURN);
> > +	       else if (strcmp(cs->jumpto, "NFLOG") == 0)
> > +	           ret = add_log(r, cs);
> >  	       else
> >  		       ret = add_target(r, cs->target->t);
> >         } else if (strlen(cs->jumpto) > 0) {
> > @@ -1352,6 +1355,36 @@ int add_action(struct nftnl_rule *r, struct iptables_command_state *cs,
> >         return ret;
> >  }
> >
> > +int add_log(struct nftnl_rule *r, struct iptables_command_state *cs)
> > +{
> > +    struct nftnl_expr *expr;
> > +    struct xt_nflog_info *info = (struct xt_nflog_info *)cs->target->t->data;
> > +
> > +    expr = nftnl_expr_alloc("log");
> > +    if (!expr)
> > +        return -ENOMEM;
> > +
> > +    if (info->prefix != NULL) {
> > +        //char prefix[NF_LOG_PREFIXLEN] = {};
> > +
> > +        // get prefix here from somewhere...
> > +        // maybe in cs->argv?
> > +        nftnl_expr_set_str(expr, NFTNL_EXPR_LOG_PREFIX, "iff the value at the end is 12 then this string is truncated 123");
> > +    }
> > +    if (info->group) {
> > +        nftnl_expr_set_u16(expr, NFTNL_EXPR_LOG_GROUP, info->group);
> > +        if (info->flags & XT_NFLOG_F_COPY_LEN)
> > +            nftnl_expr_set_u32(expr, NFTNL_EXPR_LOG_SNAPLEN,
> > +                               info->len);
> > +        if (info->threshold)
> > +            nftnl_expr_set_u16(expr, NFTNL_EXPR_LOG_QTHRESHOLD,
> > +                               info->threshold);
> > +    }
> > +
> > +    nftnl_rule_add_expr(r, expr);
> > +    return 0;
> > +}
> > +
> >  static void nft_rule_print_debug(struct nftnl_rule *r, struct nlmsghdr *nlh)
> >  {
> >  #ifdef NLDEBUG
> > @@ -3487,6 +3520,11 @@ static int nft_is_expr_compatible(struct nftnl_expr *expr, void *data)
> >  	    nftnl_expr_get_u32(expr, NFTNL_EXPR_LIMIT_FLAGS) == 0)
> >  		return 0;
> >
> > +	if (!strcmp(name, "log") &&
> > +	    nftnl_expr_get_u32(expr, NFTNL_EXPR_LOG_LEVEL) == 4 &&
> > +	    !nftnl_expr_is_set(expr, NFTNL_EXPR_LOG_FLAGS))
> > +	    return 0;
> > +
> >  	return -1;
> >  }
> >
> > diff --git a/iptables/nft.h b/iptables/nft.h
> > index 4ac7e009..28dc81b7 100644
> > --- a/iptables/nft.h
> > +++ b/iptables/nft.h
> > @@ -193,6 +193,7 @@ int add_match(struct nft_handle *h, struct nftnl_rule *r, struct xt_entry_match
> >  int add_target(struct nftnl_rule *r, struct xt_entry_target *t);
> >  int add_jumpto(struct nftnl_rule *r, const char *name, int verdict);
> >  int add_action(struct nftnl_rule *r, struct iptables_command_state *cs, bool goto_set);
> > +int add_log(struct nftnl_rule *r, struct iptables_command_state *cs);
> >  char *get_comment(const void *data, uint32_t data_len);
> >
> >  enum nft_rule_print {
> > --
> > 2.32.0

[-- Attachment #1.2: 0001-extensions-libxt_NFLOG-embed-struct-xt_nflog_info-in.patch --]
[-- Type: text/x-diff, Size: 7247 bytes --]

From 3c18555c6356e03731812688d7e6956a04ce820e Mon Sep 17 00:00:00 2001
From: Jeremy Sowden <jeremy@azazel.net>
Date: Sun, 1 Aug 2021 14:47:52 +0100
Subject: [PATCH] extensions: libxt_NFLOG: embed struct xt_nflog_info in
 another structure to store longer prefixes suitable for the nft log
 statement.

NFLOG truncates the log-prefix to 64 characters which is the limit
supported by iptables-legacy.  We now store the longer 128-character
prefix in a new structure, struct xt_nflog_state, alongside the struct
xt_nflog_info for use by iptables-nft.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 extensions/libxt_NFLOG.c | 38 ++++++++++++++++++++++++++++----------
 extensions/libxt_NFLOG.h | 12 ++++++++++++
 iptables/nft-shared.c    | 17 +++++++++++------
 iptables/nft.c           | 10 ++++------
 4 files changed, 55 insertions(+), 22 deletions(-)
 create mode 100644 extensions/libxt_NFLOG.h

diff --git a/extensions/libxt_NFLOG.c b/extensions/libxt_NFLOG.c
index 02a1b4aa35a3..6e1482122f11 100644
--- a/extensions/libxt_NFLOG.c
+++ b/extensions/libxt_NFLOG.c
@@ -8,6 +8,8 @@
 #include <linux/netfilter/x_tables.h>
 #include <linux/netfilter/xt_NFLOG.h>
 
+#include "libxt_NFLOG.h"
+
 enum {
 	O_GROUP = 0,
 	O_PREFIX,
@@ -53,12 +55,17 @@ static void NFLOG_init(struct xt_entry_target *t)
 
 static void NFLOG_parse(struct xt_option_call *cb)
 {
+	struct xt_nflog_state *state = (struct xt_nflog_state *)cb->data;
+
 	xtables_option_parse(cb);
 	switch (cb->entry->id) {
 	case O_PREFIX:
 		if (strchr(cb->arg, '\n') != NULL)
 			xtables_error(PARAMETER_PROBLEM,
 				   "Newlines not allowed in --log-prefix");
+
+		snprintf(state->nf_log_prefix, sizeof(state->nf_log_prefix),
+			 "%s", cb->arg);
 		break;
 	}
 }
@@ -75,11 +82,26 @@ static void NFLOG_check(struct xt_fcheck_call *cb)
 		info->flags |= XT_NFLOG_F_COPY_LEN;
 }
 
-static void nflog_print(const struct xt_nflog_info *info, char *prefix)
+static void nflog_print(const void *data, size_t target_size,
+			const char *prefix)
 {
+	size_t data_size = target_size - offsetof(struct xt_entry_target, data);
+	const struct xt_nflog_info *info;
+	const char *nf_log_prefix;
+
+	if (data_size == XT_ALIGN(sizeof(struct xt_nflog_state))) {
+		const struct xt_nflog_state *state = data;
+
+		info = &state->info;
+		nf_log_prefix = state->nf_log_prefix;
+	} else {
+		info = data;
+		nf_log_prefix = NULL;
+	}
+
 	if (info->prefix[0] != '\0') {
 		printf(" %snflog-prefix ", prefix);
-		xtables_save_string(info->prefix);
+		xtables_save_string(nf_log_prefix ? : info->prefix);
 	}
 	if (info->group)
 		printf(" %snflog-group %u", prefix, info->group);
@@ -94,16 +116,12 @@ static void nflog_print(const struct xt_nflog_info *info, char *prefix)
 static void NFLOG_print(const void *ip, const struct xt_entry_target *target,
 			int numeric)
 {
-	const struct xt_nflog_info *info = (struct xt_nflog_info *)target->data;
-
-	nflog_print(info, "");
+	nflog_print(target->data, target->u.target_size, "");
 }
 
 static void NFLOG_save(const void *ip, const struct xt_entry_target *target)
 {
-	const struct xt_nflog_info *info = (struct xt_nflog_info *)target->data;
-
-	nflog_print(info, "--");
+	nflog_print(target->data, target->u.target_size, "--");
 }
 
 static void nflog_print_xlate(const struct xt_nflog_info *info,
@@ -139,8 +157,8 @@ static struct xtables_target nflog_target = {
 	.family		= NFPROTO_UNSPEC,
 	.name		= "NFLOG",
 	.version	= XTABLES_VERSION,
-	.size		= XT_ALIGN(sizeof(struct xt_nflog_info)),
-	.userspacesize	= XT_ALIGN(sizeof(struct xt_nflog_info)),
+	.size		= XT_ALIGN(sizeof(struct xt_nflog_state)),
+	.userspacesize	= XT_ALIGN(sizeof(struct xt_nflog_state)),
 	.help		= NFLOG_help,
 	.init		= NFLOG_init,
 	.x6_fcheck	= NFLOG_check,
diff --git a/extensions/libxt_NFLOG.h b/extensions/libxt_NFLOG.h
new file mode 100644
index 000000000000..f3599a77ef2e
--- /dev/null
+++ b/extensions/libxt_NFLOG.h
@@ -0,0 +1,12 @@
+#ifndef LIBXT_NFLOG_H_INCLUDED
+#define LIBXT_NFLOG_H_INCLUDED
+
+#include <linux/netfilter/nf_log.h>
+#include <linux/netfilter/xt_NFLOG.h>
+
+struct xt_nflog_state {
+	struct xt_nflog_info info;
+	char nf_log_prefix[NF_LOG_PREFIXLEN];
+};
+
+#endif /* !defined(LIBXT_NFLOG_H_INCLUDED) */
diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
index b5259db07723..0a9c4de034be 100644
--- a/iptables/nft-shared.c
+++ b/iptables/nft-shared.c
@@ -32,6 +32,7 @@
 #include "nft-bridge.h"
 #include "xshared.h"
 #include "nft.h"
+#include "extensions/libxt_NFLOG.h"
 
 extern struct nft_family_ops nft_family_ops_ipv4;
 extern struct nft_family_ops nft_family_ops_ipv6;
@@ -621,19 +622,23 @@ static void nft_parse_log(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
 
     target->t = t;
 
-    struct xt_nflog_info *info = xtables_malloc(sizeof(struct xt_nflog_info));
+    struct xt_nflog_state state = { 0 };
+
+    struct xt_nflog_info *info = &state.info;
     info->group = group;
     info->len = snaplen;
     info->threshold = qthreshold;
 
     /* Here, because we allow 128 characters in nftables but only 64
-     * characters in xtables (in xt_nflog_info specifically), we may
-     * end up truncating the string when parsing it.
+     * characters in xtables (in xt_nflog_info specifically), we may end up
+     * truncating the string when parsing it.  The longer prefix will be
+     * available in state.nf_log_prefix.
      */
-    strncpy(info->prefix, prefix, sizeof(info->prefix));
-    info->prefix[sizeof(info->prefix) - 1] = '\0';
+    snprintf(info->prefix, sizeof(info->prefix), "%s", prefix);
+
+    snprintf(state.nf_log_prefix, sizeof(state.nf_log_prefix), "%s", prefix);
 
-    memcpy(&target->t->data, info, target->size);
+    memcpy(&target->t->data, &state, sizeof(state));
 
     ctx->h->ops->parse_target(target, data);
 }
diff --git a/iptables/nft.c b/iptables/nft.c
index dce8fe0b4a18..addcfffdd0cc 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -59,6 +59,7 @@
 #include "nft-cache.h"
 #include "nft-shared.h"
 #include "nft-bridge.h" /* EBT_NOPROTO */
+#include "extensions/libxt_NFLOG.h"
 
 static void *nft_fn;
 
@@ -1358,18 +1359,15 @@ int add_action(struct nftnl_rule *r, struct iptables_command_state *cs,
 int add_log(struct nftnl_rule *r, struct iptables_command_state *cs)
 {
     struct nftnl_expr *expr;
-    struct xt_nflog_info *info = (struct xt_nflog_info *)cs->target->t->data;
+    struct xt_nflog_state *state = (struct xt_nflog_state *)cs->target->t->data;
+    struct xt_nflog_info *info = &state->info;
 
     expr = nftnl_expr_alloc("log");
     if (!expr)
         return -ENOMEM;
 
     if (info->prefix != NULL) {
-        //char prefix[NF_LOG_PREFIXLEN] = {};
-
-        // get prefix here from somewhere...
-        // maybe in cs->argv?
-        nftnl_expr_set_str(expr, NFTNL_EXPR_LOG_PREFIX, "iff the value at the end is 12 then this string is truncated 123");
+        nftnl_expr_set_str(expr, NFTNL_EXPR_LOG_PREFIX, state->nf_log_prefix);
     }
     if (info->group) {
         nftnl_expr_set_u16(expr, NFTNL_EXPR_LOG_GROUP, info->group);
-- 
2.30.2


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [netfilter-core] [PATCH] netfilter: xt_NFLOG: allow 128 character log prefixes

[Jeremy Sowden]

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

On 2021-08-02, at 12:20:18 +0100, Jeremy Sowden wrote:
> On 2021-08-01, at 15:14:42 +0100, Jeremy Sowden wrote:
> > On 2021-07-30, at 13:27:49 -0500, Kyle Bowman wrote:
> > > On Wed, Jul 28, 2021 at 03:43:47AM +0200, Phil Sutter wrote:
> > > > You might want to check iptables commit ccf154d7420c0 ("xtables:
> > > > Don't use native nftables comments") for reference, it does the
> > > > opposite of what you want to do.
> > >
> > > I went ahead and looked through this commit and also found found the
> > > code that initially added this functionality; commit d64ef34a9961
> > > ("iptables-compat: use nft built-in comments support ").
> > >
> > > Additionally I found some other commits that moved code to nft
> > > native implementations of the xtables counterpart so that proved
> > > helpful.
> > >
> > > After a couple days of research I did end up figuring out what to do
> > > and have added a (mostly complete) native nft log support in
> > > iptables-nft. It all seems to work without any kernel changes
> > > required. The only problem I'm now faced with is that since we want
> > > to take the string passed into the iptables-nft command and add it
> > > to the nftnl expression (`NFTNL_EXPR_LOG_PREFIX`) I'm not entirely
> > > sure where to get the original sized string from aside from `argv`
> > > in the `struct iptables_command_state`. I would get it from the
> > > `struct xt_nflog_info`, but that's going to store the truncated
> > > version and we would like to be able to store 128 characters of the
> > > string as opposed to 64.
> > >
> > > Any recommendations about how I might do this safely?
> >
> > The xtables_target struct has a `udata` member which I think would be
> > suitable.  libxt_RATEEST does something similar.
>
> Actually, if we embed struct xf_nflog_info in another structure along
> with the longer prefix, we can get iptables-nft to print it untruncated.

> From 3c18555c6356e03731812688d7e6956a04ce820e Mon Sep 17 00:00:00 2001
> From: Jeremy Sowden <jeremy@azazel.net>
> Date: Sun, 1 Aug 2021 14:47:52 +0100
> Subject: [PATCH] extensions: libxt_NFLOG: embed struct xt_nflog_info in
>  another structure to store longer prefixes suitable for the nft log
>  statement.
>
> NFLOG truncates the log-prefix to 64 characters which is the limit
> supported by iptables-legacy.  We now store the longer 128-character
> prefix in a new structure, struct xt_nflog_state, alongside the struct
> xt_nflog_info for use by iptables-nft.
>
> Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
> ---
>  extensions/libxt_NFLOG.c | 38 ++++++++++++++++++++++++++++----------
>  extensions/libxt_NFLOG.h | 12 ++++++++++++
>  iptables/nft-shared.c    | 17 +++++++++++------
>  iptables/nft.c           | 10 ++++------
>  4 files changed, 55 insertions(+), 22 deletions(-)
>  create mode 100644 extensions/libxt_NFLOG.h
>
> diff --git a/extensions/libxt_NFLOG.c b/extensions/libxt_NFLOG.c
> index 02a1b4aa35a3..6e1482122f11 100644
> --- a/extensions/libxt_NFLOG.c
> +++ b/extensions/libxt_NFLOG.c
> @@ -8,6 +8,8 @@
>  #include <linux/netfilter/x_tables.h>
>  #include <linux/netfilter/xt_NFLOG.h>
>
> +#include "libxt_NFLOG.h"
> +
>  enum {
>  	O_GROUP = 0,
>  	O_PREFIX,
> @@ -53,12 +55,17 @@ static void NFLOG_init(struct xt_entry_target *t)
>
>  static void NFLOG_parse(struct xt_option_call *cb)
>  {
> +	struct xt_nflog_state *state = (struct xt_nflog_state *)cb->data;
> +
>  	xtables_option_parse(cb);
>  	switch (cb->entry->id) {
>  	case O_PREFIX:
>  		if (strchr(cb->arg, '\n') != NULL)
>  			xtables_error(PARAMETER_PROBLEM,
>  				   "Newlines not allowed in --log-prefix");
> +
> +		snprintf(state->nf_log_prefix, sizeof(state->nf_log_prefix),
> +			 "%s", cb->arg);
>  		break;
>  	}
>  }
> @@ -75,11 +82,26 @@ static void NFLOG_check(struct xt_fcheck_call *cb)
>  		info->flags |= XT_NFLOG_F_COPY_LEN;
>  }
>
> -static void nflog_print(const struct xt_nflog_info *info, char *prefix)
> +static void nflog_print(const void *data, size_t target_size,
> +			const char *prefix)
>  {
> +	size_t data_size = target_size - offsetof(struct xt_entry_target, data);
> +	const struct xt_nflog_info *info;
> +	const char *nf_log_prefix;
> +
> +	if (data_size == XT_ALIGN(sizeof(struct xt_nflog_state))) {
> +		const struct xt_nflog_state *state = data;
> +
> +		info = &state->info;
> +		nf_log_prefix = state->nf_log_prefix;
> +	} else {
> +		info = data;
> +		nf_log_prefix = NULL;
> +	}
> +
>  	if (info->prefix[0] != '\0') {
>  		printf(" %snflog-prefix ", prefix);
> -		xtables_save_string(info->prefix);
> +		xtables_save_string(nf_log_prefix ? : info->prefix);
>  	}
>  	if (info->group)
>  		printf(" %snflog-group %u", prefix, info->group);
> @@ -94,16 +116,12 @@ static void nflog_print(const struct xt_nflog_info *info, char *prefix)
>  static void NFLOG_print(const void *ip, const struct xt_entry_target *target,
>  			int numeric)
>  {
> -	const struct xt_nflog_info *info = (struct xt_nflog_info *)target->data;
> -
> -	nflog_print(info, "");
> +	nflog_print(target->data, target->u.target_size, "");
>  }
>
>  static void NFLOG_save(const void *ip, const struct xt_entry_target *target)
>  {
> -	const struct xt_nflog_info *info = (struct xt_nflog_info *)target->data;
> -
> -	nflog_print(info, "--");
> +	nflog_print(target->data, target->u.target_size, "--");
>  }
>
>  static void nflog_print_xlate(const struct xt_nflog_info *info,
> @@ -139,8 +157,8 @@ static struct xtables_target nflog_target = {
>  	.family		= NFPROTO_UNSPEC,
>  	.name		= "NFLOG",
>  	.version	= XTABLES_VERSION,
> -	.size		= XT_ALIGN(sizeof(struct xt_nflog_info)),
> -	.userspacesize	= XT_ALIGN(sizeof(struct xt_nflog_info)),
> +	.size		= XT_ALIGN(sizeof(struct xt_nflog_state)),
> +	.userspacesize	= XT_ALIGN(sizeof(struct xt_nflog_state)),

Unfortuantely the change in size breaks iptables-legacy.  Whoops.  More
thought required.

>  	.help		= NFLOG_help,
>  	.init		= NFLOG_init,
>  	.x6_fcheck	= NFLOG_check,
> diff --git a/extensions/libxt_NFLOG.h b/extensions/libxt_NFLOG.h
> new file mode 100644
> index 000000000000..f3599a77ef2e
> --- /dev/null
> +++ b/extensions/libxt_NFLOG.h
> @@ -0,0 +1,12 @@
> +#ifndef LIBXT_NFLOG_H_INCLUDED
> +#define LIBXT_NFLOG_H_INCLUDED
> +
> +#include <linux/netfilter/nf_log.h>
> +#include <linux/netfilter/xt_NFLOG.h>
> +
> +struct xt_nflog_state {
> +	struct xt_nflog_info info;
> +	char nf_log_prefix[NF_LOG_PREFIXLEN];
> +};
> +
> +#endif /* !defined(LIBXT_NFLOG_H_INCLUDED) */
> diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
> index b5259db07723..0a9c4de034be 100644
> --- a/iptables/nft-shared.c
> +++ b/iptables/nft-shared.c
> @@ -32,6 +32,7 @@
>  #include "nft-bridge.h"
>  #include "xshared.h"
>  #include "nft.h"
> +#include "extensions/libxt_NFLOG.h"
>
>  extern struct nft_family_ops nft_family_ops_ipv4;
>  extern struct nft_family_ops nft_family_ops_ipv6;
> @@ -621,19 +622,23 @@ static void nft_parse_log(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
>
>      target->t = t;
>
> -    struct xt_nflog_info *info = xtables_malloc(sizeof(struct xt_nflog_info));
> +    struct xt_nflog_state state = { 0 };
> +
> +    struct xt_nflog_info *info = &state.info;
>      info->group = group;
>      info->len = snaplen;
>      info->threshold = qthreshold;
>
>      /* Here, because we allow 128 characters in nftables but only 64
> -     * characters in xtables (in xt_nflog_info specifically), we may
> -     * end up truncating the string when parsing it.
> +     * characters in xtables (in xt_nflog_info specifically), we may end up
> +     * truncating the string when parsing it.  The longer prefix will be
> +     * available in state.nf_log_prefix.
>       */
> -    strncpy(info->prefix, prefix, sizeof(info->prefix));
> -    info->prefix[sizeof(info->prefix) - 1] = '\0';
> +    snprintf(info->prefix, sizeof(info->prefix), "%s", prefix);
> +
> +    snprintf(state.nf_log_prefix, sizeof(state.nf_log_prefix), "%s", prefix);
>
> -    memcpy(&target->t->data, info, target->size);
> +    memcpy(&target->t->data, &state, sizeof(state));
>
>      ctx->h->ops->parse_target(target, data);
>  }
> diff --git a/iptables/nft.c b/iptables/nft.c
> index dce8fe0b4a18..addcfffdd0cc 100644
> --- a/iptables/nft.c
> +++ b/iptables/nft.c
> @@ -59,6 +59,7 @@
>  #include "nft-cache.h"
>  #include "nft-shared.h"
>  #include "nft-bridge.h" /* EBT_NOPROTO */
> +#include "extensions/libxt_NFLOG.h"
>
>  static void *nft_fn;
>
> @@ -1358,18 +1359,15 @@ int add_action(struct nftnl_rule *r, struct iptables_command_state *cs,
>  int add_log(struct nftnl_rule *r, struct iptables_command_state *cs)
>  {
>      struct nftnl_expr *expr;
> -    struct xt_nflog_info *info = (struct xt_nflog_info *)cs->target->t->data;
> +    struct xt_nflog_state *state = (struct xt_nflog_state *)cs->target->t->data;
> +    struct xt_nflog_info *info = &state->info;
>
>      expr = nftnl_expr_alloc("log");
>      if (!expr)
>          return -ENOMEM;
>
>      if (info->prefix != NULL) {
> -        //char prefix[NF_LOG_PREFIXLEN] = {};
> -
> -        // get prefix here from somewhere...
> -        // maybe in cs->argv?
> -        nftnl_expr_set_str(expr, NFTNL_EXPR_LOG_PREFIX, "iff the value at the end is 12 then this string is truncated 123");
> +        nftnl_expr_set_str(expr, NFTNL_EXPR_LOG_PREFIX, state->nf_log_prefix);
>      }
>      if (info->group) {
>          nftnl_expr_set_u16(expr, NFTNL_EXPR_LOG_GROUP, info->group);
> --
> 2.30.2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [netfilter-core] [PATCH] netfilter: xt_NFLOG: allow 128 character log prefixes

[Jeremy Sowden]

[-- Attachment #1.1: Type: text/plain, Size: 3407 bytes --]

On 2021-08-02, at 17:40:36 +0100, Jeremy Sowden wrote:
> On 2021-08-02, at 12:20:18 +0100, Jeremy Sowden wrote:
> > On 2021-08-01, at 15:14:42 +0100, Jeremy Sowden wrote:
> > > On 2021-07-30, at 13:27:49 -0500, Kyle Bowman wrote:
> > > > On Wed, Jul 28, 2021 at 03:43:47AM +0200, Phil Sutter wrote:
> > > > > You might want to check iptables commit ccf154d7420c0
> > > > > ("xtables: Don't use native nftables comments") for reference,
> > > > > it does the opposite of what you want to do.
> > > >
> > > > I went ahead and looked through this commit and also found found
> > > > the code that initially added this functionality; commit
> > > > d64ef34a9961 ("iptables-compat: use nft built-in comments
> > > > support ").
> > > >
> > > > Additionally I found some other commits that moved code to nft
> > > > native implementations of the xtables counterpart so that proved
> > > > helpful.
> > > >
> > > > After a couple days of research I did end up figuring out what
> > > > to do and have added a (mostly complete) native nft log support
> > > > in iptables-nft. It all seems to work without any kernel changes
> > > > required. The only problem I'm now faced with is that since we
> > > > want to take the string passed into the iptables-nft command and
> > > > add it to the nftnl expression (`NFTNL_EXPR_LOG_PREFIX`) I'm not
> > > > entirely sure where to get the original sized string from aside
> > > > from `argv` in the `struct iptables_command_state`. I would get
> > > > it from the `struct xt_nflog_info`, but that's going to store
> > > > the truncated version and we would like to be able to store 128
> > > > characters of the string as opposed to 64.
> > > >
> > > > Any recommendations about how I might do this safely?
> > >
> > > The xtables_target struct has a `udata` member which I think would
> > > be suitable.  libxt_RATEEST does something similar.
> >
> > Actually, if we embed struct xf_nflog_info in another structure
> > along with the longer prefix, we can get iptables-nft to print it
> > untruncated.
>
> > From 3c18555c6356e03731812688d7e6956a04ce820e Mon Sep 17 00:00:00 2001
> > From: Jeremy Sowden <jeremy@azazel.net>
> > Date: Sun, 1 Aug 2021 14:47:52 +0100
> > Subject: [PATCH] extensions: libxt_NFLOG: embed struct xt_nflog_info
> >  in another structure to store longer prefixes suitable for the nft
> >  log statement.
> >
> > NFLOG truncates the log-prefix to 64 characters which is the limit
> > supported by iptables-legacy.  We now store the longer 128-character
> > prefix in a new structure, struct xt_nflog_state, alongside the
> > struct xt_nflog_info for use by iptables-nft.
> >
> > [...]
> >
> > @@ -139,8 +157,8 @@ static struct xtables_target nflog_target = {
> >  	.family		= NFPROTO_UNSPEC,
> >  	.name		= "NFLOG",
> >  	.version	= XTABLES_VERSION,
> > -	.size		= XT_ALIGN(sizeof(struct xt_nflog_info)),
> > -	.userspacesize	= XT_ALIGN(sizeof(struct xt_nflog_info)),
> > +	.size		= XT_ALIGN(sizeof(struct xt_nflog_state)),
> > +	.userspacesize	= XT_ALIGN(sizeof(struct xt_nflog_state)),
>
> Unfortuantely the change in size breaks iptables-legacy.  Whoops.
> More thought required.

Right, take three.  Firstly, use udata as I previously suggested, and
then use a new struct with a layout compatible with struct xt_nflog_info
just for printing and saving iptables-nft targets.

Seems to work.  Doesn't break iptables-legacy.

Patches attached.

J.

[-- Attachment #1.2: v3-0001-extensions-libxt_NFLOG-use-udata-to-store-longer-.patch --]
[-- Type: text/x-diff, Size: 2453 bytes --]

From 5fcf518401d71b8821cd1c0a00a958138ad9dca1 Mon Sep 17 00:00:00 2001
From: Jeremy Sowden <jeremy@azazel.net>
Date: Sun, 1 Aug 2021 14:47:52 +0100
Subject: [PATCH v3 1/2] extensions: libxt_NFLOG: use udata to store longer
 prefixes suitable for the nft log statement.

Hitherto NFLOG has truncated the log-prefix to the 64-character limit
supported by iptables-legacy.  We now use the struct xtables_target's
udata member to store the longer 128-character prefix supported by
iptables-nft.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 extensions/libxt_NFLOG.c | 6 ++++++
 iptables/nft.c           | 6 +-----
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/extensions/libxt_NFLOG.c b/extensions/libxt_NFLOG.c
index 02a1b4aa35a3..9057230d7ee7 100644
--- a/extensions/libxt_NFLOG.c
+++ b/extensions/libxt_NFLOG.c
@@ -5,6 +5,7 @@
 #include <getopt.h>
 #include <xtables.h>
 
+#include <linux/netfilter/nf_log.h>
 #include <linux/netfilter/x_tables.h>
 #include <linux/netfilter/xt_NFLOG.h>
 
@@ -53,12 +54,16 @@ static void NFLOG_init(struct xt_entry_target *t)
 
 static void NFLOG_parse(struct xt_option_call *cb)
 {
+	char *nf_log_prefix = cb->udata;
+
 	xtables_option_parse(cb);
 	switch (cb->entry->id) {
 	case O_PREFIX:
 		if (strchr(cb->arg, '\n') != NULL)
 			xtables_error(PARAMETER_PROBLEM,
 				   "Newlines not allowed in --log-prefix");
+
+		snprintf(nf_log_prefix, NF_LOG_PREFIXLEN, "%s", cb->arg);
 		break;
 	}
 }
@@ -149,6 +154,7 @@ static struct xtables_target nflog_target = {
 	.save		= NFLOG_save,
 	.x6_options	= NFLOG_opts,
 	.xlate		= NFLOG_xlate,
+	.udata_size     = NF_LOG_PREFIXLEN
 };
 
 void _init(void)
diff --git a/iptables/nft.c b/iptables/nft.c
index dce8fe0b4a18..13cbf0a8b87b 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1365,11 +1365,7 @@ int add_log(struct nftnl_rule *r, struct iptables_command_state *cs)
         return -ENOMEM;
 
     if (info->prefix != NULL) {
-        //char prefix[NF_LOG_PREFIXLEN] = {};
-
-        // get prefix here from somewhere...
-        // maybe in cs->argv?
-        nftnl_expr_set_str(expr, NFTNL_EXPR_LOG_PREFIX, "iff the value at the end is 12 then this string is truncated 123");
+        nftnl_expr_set_str(expr, NFTNL_EXPR_LOG_PREFIX, cs->target->udata);
     }
     if (info->group) {
         nftnl_expr_set_u16(expr, NFTNL_EXPR_LOG_GROUP, info->group);
-- 
2.30.2


[-- Attachment #1.3: v3-0002-extensions-libxt_NFLOG-don-t-truncate-log-prefix-.patch --]
[-- Type: text/x-diff, Size: 3091 bytes --]

From 06a9108c7a4b9a8b603d2020214b5cf5c5d86880 Mon Sep 17 00:00:00 2001
From: Jeremy Sowden <jeremy@azazel.net>
Date: Mon, 2 Aug 2021 22:54:27 +0100
Subject: [PATCH v3 2/2] extensions: libxt_NFLOG: don't truncate log-prefix
 when printing and saving iptables-nft nflog targets.

When parsing the rule, use a struct with a layout compatible to that of
struct xt_nflog_info, but with a buffer large enough to contain the
whole 128-character nft prefix.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 iptables/nft-shared.c | 36 ++++++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
index b5259db07723..842318f95db4 100644
--- a/iptables/nft-shared.c
+++ b/iptables/nft-shared.c
@@ -20,6 +20,7 @@
 
 #include <xtables.h>
 
+#include <linux/netfilter/nf_log.h>
 #include <linux/netfilter/xt_comment.h>
 #include <linux/netfilter/xt_limit.h>
 #include <linux/netfilter/xt_NFLOG.h>
@@ -606,13 +607,26 @@ static void nft_parse_log(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
     struct xt_entry_target *t;
     size_t target_size;
 
-    void *data = ctx->cs;
+    /*
+     * In order to handle the longer log-prefix supported by nft, instead of
+     * using struct xt_nflog_info, we use a struct with a compatible layout, but
+     * a larger buffer for the prefix.
+     */
+    struct xt_nflog_info_nft {
+        __u32 len;
+        __u16 group;
+        __u16 threshold;
+        __u16 flags;
+        __u16 pad;
+        char  prefix[NF_LOG_PREFIXLEN];
+    } info = { 0 };
 
     target = xtables_find_target("NFLOG", XTF_TRY_LOAD);
     if (target == NULL)
         return;
 
-    target_size = XT_ALIGN(sizeof(struct xt_entry_target)) + target->size;
+    target_size = XT_ALIGN(sizeof(struct xt_entry_target)) +
+                  XT_ALIGN(sizeof(struct xt_nflog_info_nft));
 
     t = xtables_calloc(1, target_size);
     t->u.target_size = target_size;
@@ -621,21 +635,15 @@ static void nft_parse_log(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
 
     target->t = t;
 
-    struct xt_nflog_info *info = xtables_malloc(sizeof(struct xt_nflog_info));
-    info->group = group;
-    info->len = snaplen;
-    info->threshold = qthreshold;
+    info.group = group;
+    info.len = snaplen;
+    info.threshold = qthreshold;
 
-    /* Here, because we allow 128 characters in nftables but only 64
-     * characters in xtables (in xt_nflog_info specifically), we may
-     * end up truncating the string when parsing it.
-     */
-    strncpy(info->prefix, prefix, sizeof(info->prefix));
-    info->prefix[sizeof(info->prefix) - 1] = '\0';
+    snprintf(info.prefix, sizeof(info.prefix), "%s", prefix);
 
-    memcpy(&target->t->data, info, target->size);
+    memcpy(&target->t->data, &info, sizeof(info));
 
-    ctx->h->ops->parse_target(target, data);
+    ctx->h->ops->parse_target(target, ctx->cs);
 }
 
 static void nft_parse_lookup(struct nft_xt_ctx *ctx, struct nft_handle *h,
-- 
2.30.2


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [netfilter-core] [PATCH] netfilter: xt_NFLOG: allow 128 character log prefixes

[Kyle Bowman]

On Tue, Aug 03, 2021 at 10:06:41AM +0100, Jeremy Sowden wrote:
> 
> Right, take three.  Firstly, use udata as I previously suggested, and
> then use a new struct with a layout compatible with struct xt_nflog_info
> just for printing and saving iptables-nft targets.
> 
> Seems to work.  Doesn't break iptables-legacy.
> 
> Patches attached.
> 
> J.

Hey Jeremy,
Thanks for writing in and helping with this, I appreciate it. I
actually was trying to make this work last night in a similar way to
how you've solved it but I gave up after a few hours. I'll go ahead and
organize this together and send the patches in a separate thread.

Kyle

Re: [netfilter-core] [PATCH] netfilter: xt_NFLOG: allow 128 character log prefixes

[Jeremy Sowden]

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

On 2021-08-03, at 13:36:04 -0500, Kyle Bowman wrote:
> On Tue, Aug 03, 2021 at 10:06:41AM +0100, Jeremy Sowden wrote:
> >
> > Right, take three.  Firstly, use udata as I previously suggested, and
> > then use a new struct with a layout compatible with struct xt_nflog_info
> > just for printing and saving iptables-nft targets.
> >
> > Seems to work.  Doesn't break iptables-legacy.
> >
> > Patches attached.
>
> Thanks for writing in and helping with this, I appreciate it. I
> actually was trying to make this work last night in a similar way to
> how you've solved it but I gave up after a few hours. I'll go ahead
> and organize this together and send the patches in a separate thread.

One thing before you do.  Some of iptables' unit-tests related to NFLOG
are now failing.  For example:

  $ sudo python3 ./iptables-test.py -n extensions/libxt_NFLOG.t
  Cannot run in own namespace, connectivity might break
  extensions/libxt_NFLOG.t: ERROR: line 2 (cannot find: iptables -I INPUT -j NFLOG --nflog-group 1)
  extensions/libxt_NFLOG.t: ERROR: line 3 (cannot find: iptables -I INPUT -j NFLOG --nflog-group 65535)
  extensions/libxt_NFLOG.t: ERROR: line 6 (cannot find: iptables -I INPUT -j NFLOG --nflog-range 1)
  extensions/libxt_NFLOG.t: ERROR: line 7 (cannot find: iptables -I INPUT -j NFLOG --nflog-range 4294967295)
  extensions/libxt_NFLOG.t: ERROR: line 10 (cannot find: iptables -I INPUT -j NFLOG --nflog-size 0)
  extensions/libxt_NFLOG.t: ERROR: line 11 (cannot find: iptables -I INPUT -j NFLOG --nflog-size 1)
  extensions/libxt_NFLOG.t: ERROR: line 12 (cannot find: iptables -I INPUT -j NFLOG --nflog-size 4294967295)
  extensions/libxt_NFLOG.t: ERROR: line 19 (cannot find: iptables -I INPUT -j NFLOG --nflog-threshold 1)
  extensions/libxt_NFLOG.t: ERROR: line 22 (cannot find: iptables -I INPUT -j NFLOG --nflog-threshold 65535)
  1 test files, 17 unit tests, 8 passed

I'm working my way through them.  I've got fixes for most.  I'll
send patches when I've sorted out the remaining ones.

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [netfilter-core] [PATCH] netfilter: xt_NFLOG: allow 128 character log prefixes

[Jeremy Sowden]

[-- Attachment #1.1: Type: text/plain, Size: 2508 bytes --]

On 2021-08-05, at 11:42:58 +0100, Jeremy Sowden wrote:
> On 2021-08-03, at 13:36:04 -0500, Kyle Bowman wrote:
> > On Tue, Aug 03, 2021 at 10:06:41AM +0100, Jeremy Sowden wrote:
> > > Right, take three.  Firstly, use udata as I previously suggested,
> > > and then use a new struct with a layout compatible with struct
> > > xt_nflog_info just for printing and saving iptables-nft targets.
> > >
> > > Seems to work.  Doesn't break iptables-legacy.
> > >
> > > Patches attached.
> >
> > Thanks for writing in and helping with this, I appreciate it. I
> > actually was trying to make this work last night in a similar way to
> > how you've solved it but I gave up after a few hours. I'll go ahead
> > and organize this together and send the patches in a separate
> > thread.
>
> One thing before you do.  Some of iptables' unit-tests related to
> NFLOG are now failing.  For example:
>
>   $ sudo python3 ./iptables-test.py -n extensions/libxt_NFLOG.t
>   Cannot run in own namespace, connectivity might break
>   extensions/libxt_NFLOG.t: ERROR: line 2 (cannot find: iptables -I INPUT -j NFLOG --nflog-group 1)
>   extensions/libxt_NFLOG.t: ERROR: line 3 (cannot find: iptables -I INPUT -j NFLOG --nflog-group 65535)
>   extensions/libxt_NFLOG.t: ERROR: line 6 (cannot find: iptables -I INPUT -j NFLOG --nflog-range 1)
>   extensions/libxt_NFLOG.t: ERROR: line 7 (cannot find: iptables -I INPUT -j NFLOG --nflog-range 4294967295)
>   extensions/libxt_NFLOG.t: ERROR: line 10 (cannot find: iptables -I INPUT -j NFLOG --nflog-size 0)
>   extensions/libxt_NFLOG.t: ERROR: line 11 (cannot find: iptables -I INPUT -j NFLOG --nflog-size 1)
>   extensions/libxt_NFLOG.t: ERROR: line 12 (cannot find: iptables -I INPUT -j NFLOG --nflog-size 4294967295)
>   extensions/libxt_NFLOG.t: ERROR: line 19 (cannot find: iptables -I INPUT -j NFLOG --nflog-threshold 1)
>   extensions/libxt_NFLOG.t: ERROR: line 22 (cannot find: iptables -I INPUT -j NFLOG --nflog-threshold 65535)
>   1 test files, 17 unit tests, 8 passed
>
> I'm working my way through them.  I've got fixes for most.  I'll send
> patches when I've sorted out the remaining ones.

Patches attached.  The first is the same as before.  The second is
slightly different: I've moved around the initialization of the info
struct, but there are no functional differences.  Of the remaining
patches, 3-8 fix bugs that were flagged up by the failing unit-tests, 9
adds a comment explaining that we don't support `--nflog-range`, and 10
drops unit-tests for this.

J.

[-- Attachment #1.2: v4-0001-extensions-libxt_NFLOG-use-udata-to-store-longer-.patch --]
[-- Type: text/x-diff, Size: 2455 bytes --]

From a50a604d2dfdb8951433a7c5adce02c589a07dae Mon Sep 17 00:00:00 2001
From: Jeremy Sowden <jeremy@azazel.net>
Date: Sun, 1 Aug 2021 14:47:52 +0100
Subject: [PATCH v4 01/10] extensions: libxt_NFLOG: use udata to store longer
 prefixes suitable for the nft log statement.

Hitherto NFLOG has truncated the log-prefix to the 64-character limit
supported by iptables-legacy.  We now use the struct xtables_target's
udata member to store the longer 128-character prefix supported by
iptables-nft.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 extensions/libxt_NFLOG.c | 6 ++++++
 iptables/nft.c           | 6 +-----
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/extensions/libxt_NFLOG.c b/extensions/libxt_NFLOG.c
index 02a1b4aa35a3..9057230d7ee7 100644
--- a/extensions/libxt_NFLOG.c
+++ b/extensions/libxt_NFLOG.c
@@ -5,6 +5,7 @@
 #include <getopt.h>
 #include <xtables.h>
 
+#include <linux/netfilter/nf_log.h>
 #include <linux/netfilter/x_tables.h>
 #include <linux/netfilter/xt_NFLOG.h>
 
@@ -53,12 +54,16 @@ static void NFLOG_init(struct xt_entry_target *t)
 
 static void NFLOG_parse(struct xt_option_call *cb)
 {
+	char *nf_log_prefix = cb->udata;
+
 	xtables_option_parse(cb);
 	switch (cb->entry->id) {
 	case O_PREFIX:
 		if (strchr(cb->arg, '\n') != NULL)
 			xtables_error(PARAMETER_PROBLEM,
 				   "Newlines not allowed in --log-prefix");
+
+		snprintf(nf_log_prefix, NF_LOG_PREFIXLEN, "%s", cb->arg);
 		break;
 	}
 }
@@ -149,6 +154,7 @@ static struct xtables_target nflog_target = {
 	.save		= NFLOG_save,
 	.x6_options	= NFLOG_opts,
 	.xlate		= NFLOG_xlate,
+	.udata_size     = NF_LOG_PREFIXLEN
 };
 
 void _init(void)
diff --git a/iptables/nft.c b/iptables/nft.c
index dce8fe0b4a18..13cbf0a8b87b 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1365,11 +1365,7 @@ int add_log(struct nftnl_rule *r, struct iptables_command_state *cs)
         return -ENOMEM;
 
     if (info->prefix != NULL) {
-        //char prefix[NF_LOG_PREFIXLEN] = {};
-
-        // get prefix here from somewhere...
-        // maybe in cs->argv?
-        nftnl_expr_set_str(expr, NFTNL_EXPR_LOG_PREFIX, "iff the value at the end is 12 then this string is truncated 123");
+        nftnl_expr_set_str(expr, NFTNL_EXPR_LOG_PREFIX, cs->target->udata);
     }
     if (info->group) {
         nftnl_expr_set_u16(expr, NFTNL_EXPR_LOG_GROUP, info->group);
-- 
2.30.2


[-- Attachment #1.3: v4-0002-extensions-libxt_NFLOG-don-t-truncate-log-prefix-.patch --]
[-- Type: text/x-diff, Size: 3683 bytes --]

From 05f656f9ac89cb348f82d522a5fc1c19e367504a Mon Sep 17 00:00:00 2001
From: Jeremy Sowden <jeremy@azazel.net>
Date: Mon, 2 Aug 2021 22:54:27 +0100
Subject: [PATCH v4 02/10] extensions: libxt_NFLOG: don't truncate log-prefix
 when printing and saving iptables-nft nflog targets.

When parsing the rule, use a struct with a layout compatible to that of
struct xt_nflog_info, but with a buffer large enough to contain the
whole 128-character nft prefix.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 iptables/nft-shared.c | 45 +++++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 21 deletions(-)

diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
index b5259db07723..22a1a08ed862 100644
--- a/iptables/nft-shared.c
+++ b/iptables/nft-shared.c
@@ -20,6 +20,7 @@
 
 #include <xtables.h>
 
+#include <linux/netfilter/nf_log.h>
 #include <linux/netfilter/xt_comment.h>
 #include <linux/netfilter/xt_limit.h>
 #include <linux/netfilter/xt_NFLOG.h>
@@ -598,21 +599,35 @@ static void nft_parse_limit(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
 
 static void nft_parse_log(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
 {
-    __u16 group =  nftnl_expr_get_u16(e, NFTNL_EXPR_LOG_GROUP);
-    __u16 qthreshold = nftnl_expr_get_u16(e, NFTNL_EXPR_LOG_QTHRESHOLD);
-    __u32 snaplen = nftnl_expr_get_u32(e, NFTNL_EXPR_LOG_SNAPLEN);
-    const char *prefix = nftnl_expr_get_str(e, NFTNL_EXPR_LOG_PREFIX);
     struct xtables_target *target;
     struct xt_entry_target *t;
     size_t target_size;
-
-    void *data = ctx->cs;
+    /*
+     * In order to handle the longer log-prefix supported by nft, instead of
+     * using struct xt_nflog_info, we use a struct with a compatible layout, but
+     * a larger buffer for the prefix.
+     */
+    struct xt_nflog_info_nft {
+        __u32 len;
+        __u16 group;
+        __u16 threshold;
+        __u16 flags;
+        __u16 pad;
+        char  prefix[NF_LOG_PREFIXLEN];
+    } info = {
+        .group     = nftnl_expr_get_u16(e, NFTNL_EXPR_LOG_GROUP),
+        .threshold = nftnl_expr_get_u16(e, NFTNL_EXPR_LOG_QTHRESHOLD),
+        .len       = nftnl_expr_get_u32(e, NFTNL_EXPR_LOG_SNAPLEN),
+    };
+    snprintf(info.prefix, sizeof(info.prefix), "%s",
+             nftnl_expr_get_str(e, NFTNL_EXPR_LOG_PREFIX));
 
     target = xtables_find_target("NFLOG", XTF_TRY_LOAD);
     if (target == NULL)
         return;
 
-    target_size = XT_ALIGN(sizeof(struct xt_entry_target)) + target->size;
+    target_size = XT_ALIGN(sizeof(struct xt_entry_target)) +
+                  XT_ALIGN(sizeof(struct xt_nflog_info_nft));
 
     t = xtables_calloc(1, target_size);
     t->u.target_size = target_size;
@@ -621,21 +636,9 @@ static void nft_parse_log(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
 
     target->t = t;
 
-    struct xt_nflog_info *info = xtables_malloc(sizeof(struct xt_nflog_info));
-    info->group = group;
-    info->len = snaplen;
-    info->threshold = qthreshold;
-
-    /* Here, because we allow 128 characters in nftables but only 64
-     * characters in xtables (in xt_nflog_info specifically), we may
-     * end up truncating the string when parsing it.
-     */
-    strncpy(info->prefix, prefix, sizeof(info->prefix));
-    info->prefix[sizeof(info->prefix) - 1] = '\0';
-
-    memcpy(&target->t->data, info, target->size);
+    memcpy(&target->t->data, &info, sizeof(info));
 
-    ctx->h->ops->parse_target(target, data);
+    ctx->h->ops->parse_target(target, ctx->cs);
 }
 
 static void nft_parse_lookup(struct nft_xt_ctx *ctx, struct nft_handle *h,
-- 
2.30.2


[-- Attachment #1.4: v4-0003-extensions-libxt_NFLOG-only-send-the-prefix-if-it.patch --]
[-- Type: text/x-diff, Size: 913 bytes --]

From c75e6f19307ffdecf1f99205f31c3aa61cde7350 Mon Sep 17 00:00:00 2001
From: Jeremy Sowden <jeremy@azazel.net>
Date: Thu, 5 Aug 2021 15:38:40 +0100
Subject: [PATCH v4 03/10] extensions: libxt_NFLOG: only send the prefix if it
 is not empty.

`info->prefix` is an array, not a pointer, and so is never `NULL`.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 iptables/nft.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 13cbf0a8b87b..e5e5b8e1046b 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1364,7 +1364,7 @@ int add_log(struct nftnl_rule *r, struct iptables_command_state *cs)
     if (!expr)
         return -ENOMEM;
 
-    if (info->prefix != NULL) {
+    if (info->prefix[0] != '\0') {
         nftnl_expr_set_str(expr, NFTNL_EXPR_LOG_PREFIX, cs->target->udata);
     }
     if (info->group) {
-- 
2.30.2


[-- Attachment #1.5: v4-0004-extensions-libxt_NFLOG-check-that-the-prefix-is-s.patch --]
[-- Type: text/x-diff, Size: 1283 bytes --]

From cb11ef308b8624fff2f8611648daee2545902184 Mon Sep 17 00:00:00 2001
From: Jeremy Sowden <jeremy@azazel.net>
Date: Thu, 5 Aug 2021 16:01:07 +0100
Subject: [PATCH v4 04/10] extensions: libxt_NFLOG: check that the prefix is
 set.

Since we don't send the prefix if it empty, we may not get one back from
the kernel.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 iptables/nft-shared.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
index 22a1a08ed862..ecce64cd166f 100644
--- a/iptables/nft-shared.c
+++ b/iptables/nft-shared.c
@@ -619,8 +619,10 @@ static void nft_parse_log(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
         .threshold = nftnl_expr_get_u16(e, NFTNL_EXPR_LOG_QTHRESHOLD),
         .len       = nftnl_expr_get_u32(e, NFTNL_EXPR_LOG_SNAPLEN),
     };
-    snprintf(info.prefix, sizeof(info.prefix), "%s",
-             nftnl_expr_get_str(e, NFTNL_EXPR_LOG_PREFIX));
+    if (nftnl_expr_is_set(e, NFTNL_EXPR_LOG_PREFIX)) {
+        snprintf(info.prefix, sizeof(info.prefix), "%s",
+                 nftnl_expr_get_str(e, NFTNL_EXPR_LOG_PREFIX));
+    }
 
     target = xtables_find_target("NFLOG", XTF_TRY_LOAD);
     if (target == NULL)
-- 
2.30.2


[-- Attachment #1.6: v4-0005-extensions-libxt_NFLOG-always-send-the-nflog-grou.patch --]
[-- Type: text/x-diff, Size: 1598 bytes --]

From 0db1b2666240effef0b75b31cd07233146b36c45 Mon Sep 17 00:00:00 2001
From: Jeremy Sowden <jeremy@azazel.net>
Date: Thu, 5 Aug 2021 15:52:58 +0100
Subject: [PATCH v4 05/10] extensions: libxt_NFLOG: always send the
 nflog-group.

For nft, log and nflog targets are handled by the same kernel module,
and are distinguished by whether they define a nflog-group.  Therefore,
we must send the group even if it is zero, or the kernel will configure
the target as a log, not an nflog.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 iptables/nft.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index e5e5b8e1046b..4bfc85a7d0bc 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1367,15 +1367,14 @@ int add_log(struct nftnl_rule *r, struct iptables_command_state *cs)
     if (info->prefix[0] != '\0') {
         nftnl_expr_set_str(expr, NFTNL_EXPR_LOG_PREFIX, cs->target->udata);
     }
-    if (info->group) {
-        nftnl_expr_set_u16(expr, NFTNL_EXPR_LOG_GROUP, info->group);
-        if (info->flags & XT_NFLOG_F_COPY_LEN)
+
+    nftnl_expr_set_u16(expr, NFTNL_EXPR_LOG_GROUP, info->group);
+    if (info->flags & XT_NFLOG_F_COPY_LEN)
             nftnl_expr_set_u32(expr, NFTNL_EXPR_LOG_SNAPLEN,
                                info->len);
-        if (info->threshold)
+    if (info->threshold)
             nftnl_expr_set_u16(expr, NFTNL_EXPR_LOG_QTHRESHOLD,
                                info->threshold);
-    }
 
     nftnl_rule_add_expr(r, expr);
     return 0;
-- 
2.30.2


[-- Attachment #1.7: v4-0006-extensions-libxt_NFLOG-only-targets-which-have-a-.patch --]
[-- Type: text/x-diff, Size: 1089 bytes --]

From d32fda241b2a60eda0709e62602f6b703d4c3347 Mon Sep 17 00:00:00 2001
From: Jeremy Sowden <jeremy@azazel.net>
Date: Thu, 5 Aug 2021 15:55:28 +0100
Subject: [PATCH v4 06/10] extensions: libxt_NFLOG: only targets which have a
 nflog-group are compatible.

Since nflog targets are distinguished by having a nflog-group, we ignore
targets without one.  Since nflog targets don't support levels or flags,
we can ignore these attributes.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 iptables/nft.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/iptables/nft.c b/iptables/nft.c
index 4bfc85a7d0bc..5778496e9ef2 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -3516,9 +3516,8 @@ static int nft_is_expr_compatible(struct nftnl_expr *expr, void *data)
 		return 0;
 
 	if (!strcmp(name, "log") &&
-	    nftnl_expr_get_u32(expr, NFTNL_EXPR_LOG_LEVEL) == 4 &&
-	    !nftnl_expr_is_set(expr, NFTNL_EXPR_LOG_FLAGS))
-	    return 0;
+	    nftnl_expr_is_set(expr, NFTNL_EXPR_LOG_GROUP))
+		return 0;
 
 	return -1;
 }
-- 
2.30.2


[-- Attachment #1.8: v4-0007-extensions-libxt_NFLOG-check-whether-the-snap-len.patch --]
[-- Type: text/x-diff, Size: 1305 bytes --]

From a9c88a338218a4eabc89b357d8d9f21939e20898 Mon Sep 17 00:00:00 2001
From: Jeremy Sowden <jeremy@azazel.net>
Date: Thu, 5 Aug 2021 16:04:25 +0100
Subject: [PATCH v4 07/10] extensions: libxt_NFLOG: check whether the
 snap-length is set.

We don't always send the length to the kernel, so we need to check
whether we have received it.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 iptables/nft-shared.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
index ecce64cd166f..8227c4308f84 100644
--- a/iptables/nft-shared.c
+++ b/iptables/nft-shared.c
@@ -617,8 +617,10 @@ static void nft_parse_log(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
     } info = {
         .group     = nftnl_expr_get_u16(e, NFTNL_EXPR_LOG_GROUP),
         .threshold = nftnl_expr_get_u16(e, NFTNL_EXPR_LOG_QTHRESHOLD),
-        .len       = nftnl_expr_get_u32(e, NFTNL_EXPR_LOG_SNAPLEN),
     };
+    if (nftnl_expr_is_set(e, NFTNL_EXPR_LOG_SNAPLEN)) {
+        info.len = nftnl_expr_get_u32(e, NFTNL_EXPR_LOG_SNAPLEN);
+    }
     if (nftnl_expr_is_set(e, NFTNL_EXPR_LOG_PREFIX)) {
         snprintf(info.prefix, sizeof(info.prefix), "%s",
                  nftnl_expr_get_str(e, NFTNL_EXPR_LOG_PREFIX));
-- 
2.30.2


[-- Attachment #1.9: v4-0008-extensions-libxt_NFLOG-set-copy-len-flag-if-the-s.patch --]
[-- Type: text/x-diff, Size: 1009 bytes --]

From e88fb277d5d406034c9099af1c25cceac02db1a7 Mon Sep 17 00:00:00 2001
From: Jeremy Sowden <jeremy@azazel.net>
Date: Thu, 5 Aug 2021 16:06:25 +0100
Subject: [PATCH v4 08/10] extensions: libxt_NFLOG: set copy-len flag if the
 snap-length is set.

Without this, iptables mistakes `nflog-size` for `nflog-range`.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 iptables/nft-shared.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/iptables/nft-shared.c b/iptables/nft-shared.c
index 8227c4308f84..70d1e20f80f1 100644
--- a/iptables/nft-shared.c
+++ b/iptables/nft-shared.c
@@ -620,6 +620,7 @@ static void nft_parse_log(struct nft_xt_ctx *ctx, struct nftnl_expr *e)
     };
     if (nftnl_expr_is_set(e, NFTNL_EXPR_LOG_SNAPLEN)) {
         info.len = nftnl_expr_get_u32(e, NFTNL_EXPR_LOG_SNAPLEN);
+        info.flags = XT_NFLOG_F_COPY_LEN;
     }
     if (nftnl_expr_is_set(e, NFTNL_EXPR_LOG_PREFIX)) {
         snprintf(info.prefix, sizeof(info.prefix), "%s",
-- 
2.30.2


[-- Attachment #1.10: v4-0009-extensions-libxt_NFLOG-add-a-comment-to-the-code-.patch --]
[-- Type: text/x-diff, Size: 1722 bytes --]

From 0da888807aa82a6ab327bc252f447df1ded81ddc Mon Sep 17 00:00:00 2001
From: Jeremy Sowden <jeremy@azazel.net>
Date: Thu, 5 Aug 2021 21:15:58 +0100
Subject: [PATCH v4 09/10] extensions: libxt_NFLOG: add a comment to the code
 explaining that we ignore `--nflog-range`.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 iptables/nft.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/iptables/nft.c b/iptables/nft.c
index 5778496e9ef2..3a3e70d5824f 100644
--- a/iptables/nft.c
+++ b/iptables/nft.c
@@ -1369,6 +1369,21 @@ int add_log(struct nftnl_rule *r, struct iptables_command_state *cs)
     }
 
     nftnl_expr_set_u16(expr, NFTNL_EXPR_LOG_GROUP, info->group);
+    /*
+     * In iptables-legacy, `--nflog-range` sets the length, and `--nflog-size`
+     * set the length _and_ the `XT_NFLOG_COPY_LEN` flag.  For iptables-nft, we
+     * cannot set a flag: setting the length always implies (the equivalent
+     * of) `--nflog-size` (`snaplen` in nft parlance).  This means we cannot
+     * emulate `--nflog-range`.  However, `--nflog-range` is broken and doesn't
+     * do anything.  If given `--nflog-range`, we have two choices: we can send
+     * the given length anyway, effectively converting it to `--nflog-size`, or
+     * we can ignore it.  `--nflog-size` was added explicitly to avoid changing
+     * the broken behaviour of `--nflog-range`:
+     *
+     *   https://lore.kernel.org/netfilter-devel/20160624204231.GA3062@akamai.com/
+     *
+     * so we ignore it.
+     */
     if (info->flags & XT_NFLOG_F_COPY_LEN)
             nftnl_expr_set_u32(expr, NFTNL_EXPR_LOG_SNAPLEN,
                                info->len);
-- 
2.30.2


[-- Attachment #1.11: v4-0010-extensions-libxf_NFLOG-remove-nflog-range-Python-.patch --]
[-- Type: text/x-diff, Size: 1163 bytes --]

From 311eaed59c399966bf352712be6a753296ebe568 Mon Sep 17 00:00:00 2001
From: Jeremy Sowden <jeremy@azazel.net>
Date: Thu, 5 Aug 2021 20:37:15 +0100
Subject: [PATCH v4 10/10] extensions: libxf_NFLOG: remove `--nflog-range`
 Python unit-tests.

nft has no equivalent to `--nflog-range`, so we cannot emulate it and
the Python unit-tests for it fail.  However, since `--nflog-range` is
broken and doesn't do anything, the tests are not testing anything
useful.

Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
---
 extensions/libxt_NFLOG.t | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/extensions/libxt_NFLOG.t b/extensions/libxt_NFLOG.t
index 933fa22160e5..33a15c061ed7 100644
--- a/extensions/libxt_NFLOG.t
+++ b/extensions/libxt_NFLOG.t
@@ -3,10 +3,6 @@
 -j NFLOG --nflog-group 65535;=;OK
 -j NFLOG --nflog-group 65536;;FAIL
 -j NFLOG --nflog-group 0;-j NFLOG;OK
--j NFLOG --nflog-range 1;=;OK
--j NFLOG --nflog-range 4294967295;=;OK
--j NFLOG --nflog-range 4294967296;;FAIL
--j NFLOG --nflog-range -1;;FAIL
 -j NFLOG --nflog-size 0;=;OK
 -j NFLOG --nflog-size 1;=;OK
 -j NFLOG --nflog-size 4294967295;=;OK
-- 
2.30.2


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]