[PATCH nft] parser: support reject unreach|reset

[PATCH nft] parser: support reject unreach|reset

[Florian Westphal]

reject did not allow to use tcp reset instead of icmp unreach.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 After this patch its possibe to do something like

 rule filter output reject reset

 Which makes kernel generate bogus tcp resets in repsonse
 to non-tcp packets.

 In iptables this is avoided by making checkentry fail if -p tcp is not
 specified when tcp-reset is requested.

 How should this be handled in nft?

 src/netlink_delinearize.c |  1 +
 src/parser.y              | 22 +++++++++++++++++++---
 src/scanner.l             |  2 ++
 src/statement.c           |  8 +++++++-
 4 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 5eec6cf..9503da7 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -428,6 +428,7 @@ static void netlink_parse_reject(struct netlink_parse_ctx *ctx,
 	struct stmt *stmt;
 
 	stmt = reject_stmt_alloc(loc);
+	stmt->reject.type = nft_rule_expr_get_u32(expr, NFT_EXPR_REJECT_TYPE);
 	list_add_tail(&stmt->list, &ctx->rule->stmts);
 }
 
diff --git a/src/parser.y b/src/parser.y
index b3acc74..e748c58 100644
--- a/src/parser.y
+++ b/src/parser.y
@@ -339,6 +339,8 @@ static void location_update(struct location *loc, struct location *rhs, int n)
 %token WEEK			"week"
 
 %token _REJECT			"reject"
+%token REJECT_RESET		"reset"
+%token REJECT_UNREACH		"unreach"
 
 %token SNAT			"snat"
 %token DNAT			"dnat"
@@ -398,8 +400,8 @@ static void location_update(struct location *loc, struct location *rhs, int n)
 %type <stmt>			limit_stmt
 %destructor { stmt_free($$); }	limit_stmt
 %type <val>			time_unit
-%type <stmt>			reject_stmt
-%destructor { stmt_free($$); }	reject_stmt
+%type <stmt>			reject_stmt reject_stmt_alloc
+%destructor { stmt_free($$); }	reject_stmt reject_stmt_alloc
 %type <stmt>			nat_stmt nat_stmt_alloc
 %destructor { stmt_free($$); }	nat_stmt nat_stmt_alloc
 %type <stmt>			queue_stmt queue_stmt_alloc
@@ -1142,12 +1144,26 @@ time_unit		:	SECOND		{ $$ = 1ULL; }
 			|	WEEK		{ $$ = 1ULL * 60 * 60 * 24 * 7; }
 			;
 
-reject_stmt		:	_REJECT
+reject_stmt		:	reject_stmt_alloc
+			| 	reject_stmt_alloc reject_args
+			;
+
+reject_stmt_alloc	:	_REJECT
 			{
 				$$ = reject_stmt_alloc(&@$);
 			}
 			;
 
+reject_args		:	REJECT_RESET
+			{
+				$<stmt>0->reject.type = NFT_REJECT_TCP_RST;
+			}
+			|	REJECT_UNREACH
+			{
+				$<stmt>0->reject.type = NFT_REJECT_ICMP_UNREACH;
+			}
+			;
+
 nat_stmt		:	nat_stmt_alloc	nat_stmt_args
 			;
 
diff --git a/src/scanner.l b/src/scanner.l
index 45c6476..dcaee13 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -293,6 +293,8 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 "week"			{ return WEEK; }
 
 "reject"		{ return _REJECT; }
+"unreach"		{ return REJECT_UNREACH; }
+"reset"			{ return REJECT_RESET; }
 
 "snat"			{ return SNAT; }
 "dnat"			{ return DNAT; }
diff --git a/src/statement.c b/src/statement.c
index 3fdd9e2..611ce5e 100644
--- a/src/statement.c
+++ b/src/statement.c
@@ -205,7 +205,13 @@ struct stmt *queue_stmt_alloc(const struct location *loc)
 
 static void reject_stmt_print(const struct stmt *stmt)
 {
-	printf("reject");
+	printf("reject ");
+	switch (stmt->reject.type) {
+	case NFT_REJECT_ICMP_UNREACH: printf("unreach"); break;
+	case NFT_REJECT_TCP_RST: printf("reset"); break;
+	default:
+		printf("[ unknown type %d ]", stmt->reject.type);
+	}
 }
 
 static const struct stmt_ops reject_stmt_ops = {
-- 
1.8.1.5

Re: [PATCH nft] parser: support reject unreach|reset

[Eric Leblond]

Hello,

On Mon, 2014-03-03 at 22:12 +0100, Florian Westphal wrote:
> reject did not allow to use tcp reset instead of icmp unreach.

I'm currently working on a patchset to support this and also setting the
ICMP code. But I'm fighting on the ICMP code filtering.

> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  After this patch its possibe to do something like
> 
>  rule filter output reject reset

I found syntax a bit short ;) If we add ICMP code support
and follow the logic:

rule filter output reject administratively-prohibited

My plan was to do something like:

rule filter output reject with tcp reset
rule filter output reject with icmp|code administratively-prohibited

> 
>  Which makes kernel generate bogus tcp resets in repsonse
>  to non-tcp packets.
> 
>  In iptables this is avoided by making checkentry fail if -p tcp is not
>  specified when tcp-reset is requested.
>
>  How should this be handled in nft?

Good point. It looks a bit like what Patrick did mention in "Re:
[nftables RFC PATCH 0/1] implementing icmp code filterin"

"We do something similar in ct_expr_update_type() for ct expressions."

Idea is to update the entry and in this case to output an error if we
don't have tcp. But I'm not sure we can access to the other expressions
(and henve to the TCP or not info) in that point.

BR,
-- 
Eric Leblond <eric@regit.org>

Re: [PATCH nft] parser: support reject unreach|reset

[Florian Westphal]

Eric Leblond <eric@regit.org> wrote:
> On Mon, 2014-03-03 at 22:12 +0100, Florian Westphal wrote:
> > reject did not allow to use tcp reset instead of icmp unreach.
> 
> I'm currently working on a patchset to support this and also setting the
> ICMP code. But I'm fighting on the ICMP code filtering.

Good to hear this, very nice!

In that case its probably best if this patch is tossed to not
interfere with your work.

> > Signed-off-by: Florian Westphal <fw@strlen.de>
> > ---
> >  After this patch its possibe to do something like
> > 
> >  rule filter output reject reset
> 
> I found syntax a bit short ;) If we add ICMP code support
> and follow the logic:
> 
> rule filter output reject administratively-prohibited
> 
> My plan was to do something like:
> 
> rule filter output reject with tcp reset
> rule filter output reject with icmp|code administratively-prohibited
> 
> > 
> >  Which makes kernel generate bogus tcp resets in repsonse
> >  to non-tcp packets.
> > 
> >  In iptables this is avoided by making checkentry fail if -p tcp is not
> >  specified when tcp-reset is requested.
> >
> >  How should this be handled in nft?
> 
> Good point. It looks a bit like what Patrick did mention in "Re:
> [nftables RFC PATCH 0/1] implementing icmp code filterin"
> 
> "We do something similar in ct_expr_update_type() for ct expressions."
> 
> Idea is to update the entry and in this case to output an error if we
> don't have tcp. But I'm not sure we can access to the other expressions
> (and henve to the TCP or not info) in that point.

Thanks for the pointer, this seems to work:

static int stmt_evaluate_reject(struct eval_ctx *ctx, struct stmt *stmt)
{
        if (stmt->reject.type == NFT_REJECT_TCP_RST) {
                const struct proto_desc *desc;
                desc = ctx->pctx.protocol[PROTO_BASE_TRANSPORT_HDR].desc;
                if (desc != &proto_tcp)
                        return stmt_error(ctx, stmt, "reset option can only be used with tcp");
        }

        stmt->flags |= STMT_F_TERMINAL;
        return 0;
}

Re: [PATCH nft] parser: support reject unreach|reset

[Patrick McHardy]

On Mon, Mar 03, 2014 at 10:12:26PM +0100, Florian Westphal wrote:
> reject did not allow to use tcp reset instead of icmp unreach.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  After this patch its possibe to do something like
> 
>  rule filter output reject reset

Actually this looks a lot like "iptables thinking" :) Why not simply use
"reset" as keyword?

>  Which makes kernel generate bogus tcp resets in repsonse
>  to non-tcp packets.
> 
>  In iptables this is avoided by making checkentry fail if -p tcp is not
>  specified when tcp-reset is requested.
> 
>  How should this be handled in nft?

We could either add some infrastructure to the kernel to mimic the iptables
check, add runtime fallback to ICMP for non-TCP packets or simply ignore them.

It shouldn't be difficult to check within the rule for the required matches,
however I'm not so sure this is really the way to go since it precludes
optimizations that spawn over multiple rules, f.i. checking for TCP before
a chain jump and have only TCP-specific rules in that chain.

I'm tending towards simply ignoring non-TCP packets at runtime.

>  %token _REJECT			"reject"
> +%token REJECT_RESET		"reset"
> +%token REJECT_UNREACH		"unreach"

Please don't use expression/statement-specific token names. We're simply
dealing with tokens, how they're used depends on the remaining grammar.

Re: [PATCH nft] parser: support reject unreach|reset

[Patrick McHardy]

On Mon, Mar 03, 2014 at 10:41:32PM +0100, Eric Leblond wrote:
> Hello,
> 
> On Mon, 2014-03-03 at 22:12 +0100, Florian Westphal wrote:
> > reject did not allow to use tcp reset instead of icmp unreach.
> 
> I'm currently working on a patchset to support this and also setting the
> ICMP code. But I'm fighting on the ICMP code filtering.
> 
> > 
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> > ---
> >  After this patch its possibe to do something like
> > 
> >  rule filter output reject reset
> 
> I found syntax a bit short ;) If we add ICMP code support
> and follow the logic:
> 
> rule filter output reject administratively-prohibited
> 
> My plan was to do something like:
> 
> rule filter output reject with tcp reset
> rule filter output reject with icmp|code administratively-prohibited

I really prefer a less bloated syntax. For TCP reset simply stating:

filter output tcp dport 22 reset

is both understandable and contains all necessary information. For ICMP
types I guess we could live with the "with" keyword, although I prefer

filter output reject [ network-unreachable | ... ]

or something like that.

> >  Which makes kernel generate bogus tcp resets in repsonse
> >  to non-tcp packets.
> > 
> >  In iptables this is avoided by making checkentry fail if -p tcp is not
> >  specified when tcp-reset is requested.
> >
> >  How should this be handled in nft?
> 
> Good point. It looks a bit like what Patrick did mention in "Re:
> [nftables RFC PATCH 0/1] implementing icmp code filterin"
> 
> "We do something similar in ct_expr_update_type() for ct expressions."
> 
> Idea is to update the entry and in this case to output an error if we
> don't have tcp. But I'm not sure we can access to the other expressions
> (and henve to the TCP or not info) in that point.

This is actually easier than the ICMP case, the protocol context contains
all necessary information. Unlike iptables we execute statements at the
point where they occur in the rule, so the user is expected to use some
logical ordering in his statements:

"tcp dport ssh reset" instead of "reset tcp dport ssh".

Meaning we have the full protocol context available.

Re: [PATCH nft] parser: support reject unreach|reset

[Florian Westphal]

Patrick McHardy <kaber@trash.net> wrote:
> On Mon, Mar 03, 2014 at 10:12:26PM +0100, Florian Westphal wrote:
> > reject did not allow to use tcp reset instead of icmp unreach.
> > 
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> > ---
> >  After this patch its possibe to do something like
> > 
> >  rule filter output reject reset
> 
> Actually this looks a lot like "iptables thinking" :)

*ahem* :)

> Why not simply use "reset" as keyword?

Yes, that would work as well.

> >  Which makes kernel generate bogus tcp resets in repsonse
> >  to non-tcp packets.
> > 
> >  In iptables this is avoided by making checkentry fail if -p tcp is not
> >  specified when tcp-reset is requested.
> > 
> >  How should this be handled in nft?
> 
> We could either add some infrastructure to the kernel to mimic the iptables
> check, add runtime fallback to ICMP for non-TCP packets or simply ignore them.
> 
> It shouldn't be difficult to check within the rule for the required matches,
> however I'm not so sure this is really the way to go since it precludes
> optimizations that spawn over multiple rules, f.i. checking for TCP before
> a chain jump and have only TCP-specific rules in that chain.

Good point.

> I'm tending towards simply ignoring non-TCP packets at runtime.

Eric, please let me know if you want to address all of this in your icmp
code patch series, or if you would like me to handle this.

If you want me to handle it I will resubmit this patch with the issues
pointed out by Patrick resolved, plus a small kernel patch to not xmit
reset for non-tcp packets.

Otherwise I'll just mark this patch as "rejected" in patchwork.

> >  %token _REJECT			"reject"
> > +%token REJECT_RESET		"reset"
> > +%token REJECT_UNREACH		"unreach"
> 
> Please don't use expression/statement-specific token names. We're simply
> dealing with tokens, how they're used depends on the remaining grammar.

Makes sense.

Thanks for your help Patrick.

Re: [PATCH nft] parser: support reject unreach|reset

[Patrick McHardy]

On Mon, Mar 03, 2014 at 11:03:51PM +0100, Florian Westphal wrote:
> Eric Leblond <eric@regit.org> wrote:
> > > 
> > >  In iptables this is avoided by making checkentry fail if -p tcp is not
> > >  specified when tcp-reset is requested.
> > >
> > >  How should this be handled in nft?
> > 
> > Good point. It looks a bit like what Patrick did mention in "Re:
> > [nftables RFC PATCH 0/1] implementing icmp code filterin"
> > 
> > "We do something similar in ct_expr_update_type() for ct expressions."
> > 
> > Idea is to update the entry and in this case to output an error if we
> > don't have tcp. But I'm not sure we can access to the other expressions
> > (and henve to the TCP or not info) in that point.
> 
> Thanks for the pointer, this seems to work:
> 
> static int stmt_evaluate_reject(struct eval_ctx *ctx, struct stmt *stmt)
> {
>         if (stmt->reject.type == NFT_REJECT_TCP_RST) {
>                 const struct proto_desc *desc;
>                 desc = ctx->pctx.protocol[PROTO_BASE_TRANSPORT_HDR].desc;
>                 if (desc != &proto_tcp)
>                         return stmt_error(ctx, stmt, "reset option can only be used with tcp");
>         }
> 
>         stmt->flags |= STMT_F_TERMINAL;
>         return 0;
> }

Yes, this is what we should do in userspace. As an add-on, we could (as we
do for payload matches) add implicit matches if no TCP match is present,
IOW, just as

"filter output tcp dport ssh"

really generates

"filter output ip protocol tcp tcp dport ssh",

"filter output reset"

would generate

"filter output ip protocol 6 reset"

and only return an error if a conflicting protocol expression is specified.
I'm not entirely sure whether we want to do this for statements or just for
match expressions though, for now I guess just the check is fine.

In the kernel we still should silently ignore packets that are not TCP,
rules might be installed by other means than nft.