* [PATCH nf] netfilter: nft_log: restrict the log prefix length to 127
@ 2017-01-22 14:10 Liping Zhang
2017-01-24 19:26 ` Pablo Neira Ayuso
0 siblings, 1 reply; 3+ messages in thread
From: Liping Zhang @ 2017-01-22 14:10 UTC (permalink / raw)
To: pablo; +Cc: netfilter-devel, Liping Zhang
From: Liping Zhang <zlpnobody@gmail.com>
First, log prefix will be truncated to NF_LOG_PREFIXLEN-1, i.e. 127,
at nf_log_packet(), so the extra part is useless.
Second, after adding a log rule with a very very long prefix, we will
fail to dump the nft rules after this _special_ one, but acctually,
they do exist. For example:
# name_65000=$(printf "%0.sQ" {1..65000})
# nft add rule filter output log prefix "$name_65000"
# nft add rule filter output counter
# nft add rule filter output counter
# nft list chain filter output
table ip filter {
chain output {
type filter hook output priority 0; policy accept;
}
}
So now, restrict the log prefix length to NF_LOG_PREFIXLEN-1.
Fixes: 96518518cc41 ("netfilter: add nftables")
Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
---
I think this is different with http://patchwork.ozlabs.org/patch/717635/.
So another separate patch will be better.
include/uapi/linux/netfilter/nf_log.h | 2 ++
net/netfilter/nf_log.c | 1 -
net/netfilter/nft_log.c | 3 ++-
3 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/netfilter/nf_log.h b/include/uapi/linux/netfilter/nf_log.h
index 8be21e0..d0b5fa9 100644
--- a/include/uapi/linux/netfilter/nf_log.h
+++ b/include/uapi/linux/netfilter/nf_log.h
@@ -9,4 +9,6 @@
#define NF_LOG_MACDECODE 0x20 /* Decode MAC header */
#define NF_LOG_MASK 0x2f
+#define NF_LOG_PREFIXLEN 128
+
#endif /* _NETFILTER_NF_LOG_H */
diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
index 3dca90d..ffb9e8a 100644
--- a/net/netfilter/nf_log.c
+++ b/net/netfilter/nf_log.c
@@ -13,7 +13,6 @@
/* Internal logging interface, which relies on the real
LOG target modules */
-#define NF_LOG_PREFIXLEN 128
#define NFLOGGER_NAME_LEN 64
static struct nf_logger __rcu *loggers[NFPROTO_NUMPROTO][NF_LOG_TYPE_MAX] __read_mostly;
diff --git a/net/netfilter/nft_log.c b/net/netfilter/nft_log.c
index 6271e40..6f6e644 100644
--- a/net/netfilter/nft_log.c
+++ b/net/netfilter/nft_log.c
@@ -39,7 +39,8 @@ static void nft_log_eval(const struct nft_expr *expr,
static const struct nla_policy nft_log_policy[NFTA_LOG_MAX + 1] = {
[NFTA_LOG_GROUP] = { .type = NLA_U16 },
- [NFTA_LOG_PREFIX] = { .type = NLA_STRING },
+ [NFTA_LOG_PREFIX] = { .type = NLA_STRING,
+ .len = NF_LOG_PREFIXLEN - 1 },
[NFTA_LOG_SNAPLEN] = { .type = NLA_U32 },
[NFTA_LOG_QTHRESHOLD] = { .type = NLA_U16 },
[NFTA_LOG_LEVEL] = { .type = NLA_U32 },
--
2.5.5
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH nf] netfilter: nft_log: restrict the log prefix length to 127
2017-01-22 14:10 [PATCH nf] netfilter: nft_log: restrict the log prefix length to 127 Liping Zhang
@ 2017-01-24 19:26 ` Pablo Neira Ayuso
2017-01-24 20:45 ` Pablo Neira Ayuso
0 siblings, 1 reply; 3+ messages in thread
From: Pablo Neira Ayuso @ 2017-01-24 19:26 UTC (permalink / raw)
To: Liping Zhang; +Cc: netfilter-devel, Liping Zhang
On Sun, Jan 22, 2017 at 10:10:32PM +0800, Liping Zhang wrote:
> From: Liping Zhang <zlpnobody@gmail.com>
>
> First, log prefix will be truncated to NF_LOG_PREFIXLEN-1, i.e. 127,
> at nf_log_packet(), so the extra part is useless.
>
> Second, after adding a log rule with a very very long prefix, we will
> fail to dump the nft rules after this _special_ one, but acctually,
> they do exist. For example:
> # name_65000=$(printf "%0.sQ" {1..65000})
> # nft add rule filter output log prefix "$name_65000"
> # nft add rule filter output counter
> # nft add rule filter output counter
> # nft list chain filter output
> table ip filter {
> chain output {
> type filter hook output priority 0; policy accept;
> }
> }
>
> So now, restrict the log prefix length to NF_LOG_PREFIXLEN-1.
>
> Fixes: 96518518cc41 ("netfilter: add nftables")
> Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
> ---
> I think this is different with http://patchwork.ozlabs.org/patch/717635/.
> So another separate patch will be better.
>
> include/uapi/linux/netfilter/nf_log.h | 2 ++
> net/netfilter/nf_log.c | 1 -
> net/netfilter/nft_log.c | 3 ++-
> 3 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/netfilter/nf_log.h b/include/uapi/linux/netfilter/nf_log.h
> index 8be21e0..d0b5fa9 100644
> --- a/include/uapi/linux/netfilter/nf_log.h
> +++ b/include/uapi/linux/netfilter/nf_log.h
> @@ -9,4 +9,6 @@
> #define NF_LOG_MACDECODE 0x20 /* Decode MAC header */
> #define NF_LOG_MASK 0x2f
>
> +#define NF_LOG_PREFIXLEN 128
Nitpick: xt_LOG allows 30 bytes, so we can probably stick to 32 bytes
here. Then, wait for usecases for larger strings, enlarging things is
easier than shrinking stuff later on (we can't actually, given that we
would break backward).
Probably we can keep __NF_LOG_PREFIXLEN 128 for what it already exists
in nf_log.c internally, and expose NF_LOG_PREFIXLEN of 32 bytes?
> #endif /* _NETFILTER_NF_LOG_H */
> diff --git a/net/netfilter/nf_log.c b/net/netfilter/nf_log.c
> index 3dca90d..ffb9e8a 100644
> --- a/net/netfilter/nf_log.c
> +++ b/net/netfilter/nf_log.c
> @@ -13,7 +13,6 @@
> /* Internal logging interface, which relies on the real
> LOG target modules */
>
> -#define NF_LOG_PREFIXLEN 128
> #define NFLOGGER_NAME_LEN 64
>
> static struct nf_logger __rcu *loggers[NFPROTO_NUMPROTO][NF_LOG_TYPE_MAX] __read_mostly;
> diff --git a/net/netfilter/nft_log.c b/net/netfilter/nft_log.c
> index 6271e40..6f6e644 100644
> --- a/net/netfilter/nft_log.c
> +++ b/net/netfilter/nft_log.c
> @@ -39,7 +39,8 @@ static void nft_log_eval(const struct nft_expr *expr,
>
> static const struct nla_policy nft_log_policy[NFTA_LOG_MAX + 1] = {
> [NFTA_LOG_GROUP] = { .type = NLA_U16 },
> - [NFTA_LOG_PREFIX] = { .type = NLA_STRING },
> + [NFTA_LOG_PREFIX] = { .type = NLA_STRING,
> + .len = NF_LOG_PREFIXLEN - 1 },
> [NFTA_LOG_SNAPLEN] = { .type = NLA_U32 },
> [NFTA_LOG_QTHRESHOLD] = { .type = NLA_U16 },
> [NFTA_LOG_LEVEL] = { .type = NLA_U32 },
> --
> 2.5.5
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH nf] netfilter: nft_log: restrict the log prefix length to 127
2017-01-24 19:26 ` Pablo Neira Ayuso
@ 2017-01-24 20:45 ` Pablo Neira Ayuso
0 siblings, 0 replies; 3+ messages in thread
From: Pablo Neira Ayuso @ 2017-01-24 20:45 UTC (permalink / raw)
To: Liping Zhang; +Cc: netfilter-devel, Liping Zhang
On Tue, Jan 24, 2017 at 08:26:29PM +0100, Pablo Neira Ayuso wrote:
> On Sun, Jan 22, 2017 at 10:10:32PM +0800, Liping Zhang wrote:
> > From: Liping Zhang <zlpnobody@gmail.com>
> >
> > First, log prefix will be truncated to NF_LOG_PREFIXLEN-1, i.e. 127,
> > at nf_log_packet(), so the extra part is useless.
> >
> > Second, after adding a log rule with a very very long prefix, we will
> > fail to dump the nft rules after this _special_ one, but acctually,
> > they do exist. For example:
> > # name_65000=$(printf "%0.sQ" {1..65000})
> > # nft add rule filter output log prefix "$name_65000"
> > # nft add rule filter output counter
> > # nft add rule filter output counter
> > # nft list chain filter output
> > table ip filter {
> > chain output {
> > type filter hook output priority 0; policy accept;
> > }
> > }
> >
> > So now, restrict the log prefix length to NF_LOG_PREFIXLEN-1.
> >
> > Fixes: 96518518cc41 ("netfilter: add nftables")
> > Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
> > ---
> > I think this is different with http://patchwork.ozlabs.org/patch/717635/.
> > So another separate patch will be better.
> >
> > include/uapi/linux/netfilter/nf_log.h | 2 ++
> > net/netfilter/nf_log.c | 1 -
> > net/netfilter/nft_log.c | 3 ++-
> > 3 files changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/uapi/linux/netfilter/nf_log.h b/include/uapi/linux/netfilter/nf_log.h
> > index 8be21e0..d0b5fa9 100644
> > --- a/include/uapi/linux/netfilter/nf_log.h
> > +++ b/include/uapi/linux/netfilter/nf_log.h
> > @@ -9,4 +9,6 @@
> > #define NF_LOG_MACDECODE 0x20 /* Decode MAC header */
> > #define NF_LOG_MASK 0x2f
> >
> > +#define NF_LOG_PREFIXLEN 128
>
> Nitpick: xt_LOG allows 30 bytes, so we can probably stick to 32 bytes
> here. Then, wait for usecases for larger strings, enlarging things is
> easier than shrinking stuff later on (we can't actually, given that we
> would break backward).
>
> Probably we can keep __NF_LOG_PREFIXLEN 128 for what it already exists
> in nf_log.c internally, and expose NF_LOG_PREFIXLEN of 32 bytes?
Hm. We've been exposing 128 bytes already since the beginning given we
lacked any validation.
Then, this patch is fine. Applied, thanks.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-01-24 20:46 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-01-22 14:10 [PATCH nf] netfilter: nft_log: restrict the log prefix length to 127 Liping Zhang
2017-01-24 19:26 ` Pablo Neira Ayuso
2017-01-24 20:45 ` Pablo Neira Ayuso
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).