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

[Jeremy Sowden <jeremy@azazel.net>]

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