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

[Jeremy Sowden <jeremy@azazel.net>]

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