[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 --]