All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH nf-next] netfilter: exthdr: add support for tcp option removal
@ 2021-12-20 14:32 Florian Westphal
  2021-12-22 23:50 ` Pablo Neira Ayuso
  0 siblings, 1 reply; 5+ messages in thread
From: Florian Westphal @ 2021-12-20 14:32 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Florian Westphal

This allows to replace a tcp option with nop padding to selectively disable
a particular tcp option.

Optstrip mode is chosen when userspace passes the exthdr expression with
neither a source nor a destination register attribute.

This is identical to xtables TCPOPTSTRIP extension.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 proposed userspace syntax is:

 nft add rule f in delete tcp option sack-perm

 net/netfilter/nft_exthdr.c | 88 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 87 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nft_exthdr.c b/net/netfilter/nft_exthdr.c
index dbe1f2e7dd9e..37e427ca2b34 100644
--- a/net/netfilter/nft_exthdr.c
+++ b/net/netfilter/nft_exthdr.c
@@ -308,6 +308,55 @@ static void nft_exthdr_tcp_set_eval(const struct nft_expr *expr,
 	regs->verdict.code = NFT_BREAK;
 }
 
+static void nft_exthdr_tcp_strip_eval(const struct nft_expr *expr,
+				      struct nft_regs *regs,
+				      const struct nft_pktinfo *pkt)
+{
+	u8 buff[sizeof(struct tcphdr) + MAX_TCP_OPTION_SPACE];
+	struct nft_exthdr *priv = nft_expr_priv(expr);
+	unsigned int i, tcphdr_len, optl;
+	struct tcphdr *tcph;
+	u8 *opt;
+
+	tcph = nft_tcp_header_pointer(pkt, sizeof(buff), buff, &tcphdr_len);
+	if (!tcph)
+		goto err;
+
+	if (skb_ensure_writable(pkt->skb, nft_thoff(pkt) + tcphdr_len))
+		goto err;
+
+	opt = (u8 *)nft_tcp_header_pointer(pkt, sizeof(buff), buff, &tcphdr_len);
+	if (!opt)
+		goto err;
+	for (i = sizeof(*tcph); i < tcphdr_len - 1; i += optl) {
+		unsigned int j;
+
+		optl = optlen(opt, i);
+		if (priv->type != opt[i])
+			continue;
+
+		if (i + optl > tcphdr_len)
+			goto err;
+
+		for (j = 0; j < optl; ++j) {
+			u16 n = TCPOPT_NOP;
+			u16 o = opt[i+j];
+
+			if ((i + j) % 2 == 0) {
+				o <<= 8;
+				n <<= 8;
+			}
+			inet_proto_csum_replace2(&tcph->check, pkt->skb, htons(o),
+						 htons(n), false);
+		}
+		memset(opt + i, TCPOPT_NOP, optl);
+		return;
+	}
+	return;
+err:
+	regs->verdict.code = NFT_BREAK;
+}
+
 static void nft_exthdr_sctp_eval(const struct nft_expr *expr,
 				 struct nft_regs *regs,
 				 const struct nft_pktinfo *pkt)
@@ -457,6 +506,28 @@ static int nft_exthdr_tcp_set_init(const struct nft_ctx *ctx,
 				       priv->len);
 }
 
+static int nft_exthdr_tcp_strip_init(const struct nft_ctx *ctx,
+				     const struct nft_expr *expr,
+				     const struct nlattr * const tb[])
+{
+	struct nft_exthdr *priv = nft_expr_priv(expr);
+
+	if (tb[NFTA_EXTHDR_SREG] ||
+	    tb[NFTA_EXTHDR_DREG] ||
+	    tb[NFTA_EXTHDR_FLAGS] ||
+	    tb[NFTA_EXTHDR_OFFSET] ||
+	    tb[NFTA_EXTHDR_LEN])
+		return -EINVAL;
+
+	if (!tb[NFTA_EXTHDR_TYPE])
+		return -EINVAL;
+
+	priv->type = nla_get_u8(tb[NFTA_EXTHDR_TYPE]);
+	priv->op = NFT_EXTHDR_OP_TCPOPT;
+
+	return 0;
+}
+
 static int nft_exthdr_ipv4_init(const struct nft_ctx *ctx,
 				const struct nft_expr *expr,
 				const struct nlattr * const tb[])
@@ -517,6 +588,13 @@ static int nft_exthdr_dump_set(struct sk_buff *skb, const struct nft_expr *expr)
 	return nft_exthdr_dump_common(skb, priv);
 }
 
+static int nft_exthdr_dump_strip(struct sk_buff *skb, const struct nft_expr *expr)
+{
+	const struct nft_exthdr *priv = nft_expr_priv(expr);
+
+	return nft_exthdr_dump_common(skb, priv);
+}
+
 static const struct nft_expr_ops nft_exthdr_ipv6_ops = {
 	.type		= &nft_exthdr_type,
 	.size		= NFT_EXPR_SIZE(sizeof(struct nft_exthdr)),
@@ -549,6 +627,14 @@ static const struct nft_expr_ops nft_exthdr_tcp_set_ops = {
 	.dump		= nft_exthdr_dump_set,
 };
 
+static const struct nft_expr_ops nft_exthdr_tcp_strip_ops = {
+	.type		= &nft_exthdr_type,
+	.size		= NFT_EXPR_SIZE(sizeof(struct nft_exthdr)),
+	.eval		= nft_exthdr_tcp_strip_eval,
+	.init		= nft_exthdr_tcp_strip_init,
+	.dump		= nft_exthdr_dump_strip,
+};
+
 static const struct nft_expr_ops nft_exthdr_sctp_ops = {
 	.type		= &nft_exthdr_type,
 	.size		= NFT_EXPR_SIZE(sizeof(struct nft_exthdr)),
@@ -576,7 +662,7 @@ nft_exthdr_select_ops(const struct nft_ctx *ctx,
 			return &nft_exthdr_tcp_set_ops;
 		if (tb[NFTA_EXTHDR_DREG])
 			return &nft_exthdr_tcp_ops;
-		break;
+		return &nft_exthdr_tcp_strip_ops;
 	case NFT_EXTHDR_OP_IPV6:
 		if (tb[NFTA_EXTHDR_DREG])
 			return &nft_exthdr_ipv6_ops;
-- 
2.33.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH nf-next] netfilter: exthdr: add support for tcp option removal
  2021-12-20 14:32 [PATCH nf-next] netfilter: exthdr: add support for tcp option removal Florian Westphal
@ 2021-12-22 23:50 ` Pablo Neira Ayuso
  2021-12-24 16:07   ` Pablo Neira Ayuso
  2021-12-27 14:11   ` Florian Westphal
  0 siblings, 2 replies; 5+ messages in thread
From: Pablo Neira Ayuso @ 2021-12-22 23:50 UTC (permalink / raw)
  To: Florian Westphal; +Cc: netfilter-devel

On Mon, Dec 20, 2021 at 03:32:47PM +0100, Florian Westphal wrote:
> This allows to replace a tcp option with nop padding to selectively disable
> a particular tcp option.
> 
> Optstrip mode is chosen when userspace passes the exthdr expression with
> neither a source nor a destination register attribute.
> 
> This is identical to xtables TCPOPTSTRIP extension.

Is it worth to retain the bitmap approach?

> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  proposed userspace syntax is:
> 
>  nft add rule f in delete tcp option sack-perm

   nft add rule f in tcp option reset sack-perm,...

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH nf-next] netfilter: exthdr: add support for tcp option removal
  2021-12-22 23:50 ` Pablo Neira Ayuso
@ 2021-12-24 16:07   ` Pablo Neira Ayuso
  2021-12-27 14:11   ` Florian Westphal
  1 sibling, 0 replies; 5+ messages in thread
From: Pablo Neira Ayuso @ 2021-12-24 16:07 UTC (permalink / raw)
  To: Florian Westphal; +Cc: netfilter-devel

On Thu, Dec 23, 2021 at 12:50:17AM +0100, Pablo Neira Ayuso wrote:
> On Mon, Dec 20, 2021 at 03:32:47PM +0100, Florian Westphal wrote:
> > This allows to replace a tcp option with nop padding to selectively disable
> > a particular tcp option.
> > 
> > Optstrip mode is chosen when userspace passes the exthdr expression with
> > neither a source nor a destination register attribute.
> > 
> > This is identical to xtables TCPOPTSTRIP extension.
> 
> Is it worth to retain the bitmap approach?

Probably a new nested attribute to store the list of types that you
would like to strip:

        NFTA_EXTHDR_TYPES
         NFTA_EXTHDR_TYPE
         NFTA_EXTHDR_TYPE

From the kernel, you could build a bitmap (just like TCPOPTSTRIP)
based on this list.

From the dump path, you can iterate over the bitmap to check for
bitset to build this nest.

> > Signed-off-by: Florian Westphal <fw@strlen.de>
> > ---
> >  proposed userspace syntax is:
> > 
> >  nft add rule f in delete tcp option sack-perm
> 
>    nft add rule f in tcp option reset sack-perm,...

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH nf-next] netfilter: exthdr: add support for tcp option removal
  2021-12-22 23:50 ` Pablo Neira Ayuso
  2021-12-24 16:07   ` Pablo Neira Ayuso
@ 2021-12-27 14:11   ` Florian Westphal
  2021-12-27 14:28     ` Pablo Neira Ayuso
  1 sibling, 1 reply; 5+ messages in thread
From: Florian Westphal @ 2021-12-27 14:11 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: Florian Westphal, netfilter-devel

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Mon, Dec 20, 2021 at 03:32:47PM +0100, Florian Westphal wrote:
> > This allows to replace a tcp option with nop padding to selectively disable
> > a particular tcp option.
> > 
> > Optstrip mode is chosen when userspace passes the exthdr expression with
> > neither a source nor a destination register attribute.
> > 
> > This is identical to xtables TCPOPTSTRIP extension.
> 
> Is it worth to retain the bitmap approach?

I don't think so.  For TCPOPTSTRIP it makes sense because
you can't use multiple targets in one rule.

I'd rework this to not set BREAK if the option wasn't present
in the first place, so you could do

delete tcp option sack-perm delete tcp option timestamp ...

and so on.

Let me know if you disagree.

I could also rework it so that option comes from sreg instead
of imm, but i could not find a use-case where having the option number
coming from a map lookup would make sense.

> > Signed-off-by: Florian Westphal <fw@strlen.de>
> > ---
> >  proposed userspace syntax is:
> > 
> >  nft add rule f in delete tcp option sack-perm
> 
>    nft add rule f in tcp option reset sack-perm,...

Why 'reset'?  My initial version had 'remove' but 'delete'
already exists as a token so it was simpler.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH nf-next] netfilter: exthdr: add support for tcp option removal
  2021-12-27 14:11   ` Florian Westphal
@ 2021-12-27 14:28     ` Pablo Neira Ayuso
  0 siblings, 0 replies; 5+ messages in thread
From: Pablo Neira Ayuso @ 2021-12-27 14:28 UTC (permalink / raw)
  To: Florian Westphal; +Cc: netfilter-devel

On Mon, Dec 27, 2021 at 03:11:21PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Mon, Dec 20, 2021 at 03:32:47PM +0100, Florian Westphal wrote:
> > > This allows to replace a tcp option with nop padding to selectively disable
> > > a particular tcp option.
> > > 
> > > Optstrip mode is chosen when userspace passes the exthdr expression with
> > > neither a source nor a destination register attribute.
> > > 
> > > This is identical to xtables TCPOPTSTRIP extension.
> > 
> > Is it worth to retain the bitmap approach?
> 
> I don't think so.  For TCPOPTSTRIP it makes sense because
> you can't use multiple targets in one rule.
> 
> I'd rework this to not set BREAK if the option wasn't present
> in the first place, so you could do
> 
> delete tcp option sack-perm delete tcp option timestamp ...
> 
> and so on.
> 
> Let me know if you disagree.

It's OK if you prefer this way. I can see references on the web to
reseting multiple options, not sure if it is actually useful in
practise, in such you can to parse the packet several times.

> I could also rework it so that option comes from sreg instead
> of imm, but i could not find a use-case where having the option number
> coming from a map lookup would make sense.
> 
> > > Signed-off-by: Florian Westphal <fw@strlen.de>
> > > ---
> > >  proposed userspace syntax is:
> > > 
> > >  nft add rule f in delete tcp option sack-perm
> > 
> >    nft add rule f in tcp option reset sack-perm,...
> 
> Why 'reset'?  My initial version had 'remove' but 'delete'
> already exists as a token so it was simpler.

'reset' also exists as a token. This is setting to nop, I just though
reset might make more sense, there might be a nice to really remove
TCP options in the future (costful but paranoid scenario, an observer
can spot options that have been nop)

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-12-27 14:28 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-20 14:32 [PATCH nf-next] netfilter: exthdr: add support for tcp option removal Florian Westphal
2021-12-22 23:50 ` Pablo Neira Ayuso
2021-12-24 16:07   ` Pablo Neira Ayuso
2021-12-27 14:11   ` Florian Westphal
2021-12-27 14:28     ` Pablo Neira Ayuso

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.