[nft PATCH] Reject invalid chain priority values in user space

[nft PATCH] Reject invalid chain priority values in user space

[Phil Sutter]

The kernel doesn't accept nat type chains with a priority of -200 or
below. Catch this and provide a better error message than the kernel's
EOPNOTSUPP.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/evaluate.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/src/evaluate.c b/src/evaluate.c
index d24f8b66b0de8..af4844c1ef6cc 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -4842,6 +4842,8 @@ static int chain_evaluate(struct eval_ctx *ctx, struct chain *chain)
 	}
 
 	if (chain->flags & CHAIN_F_BASECHAIN) {
+		int priority;
+
 		chain->hook.num = str2hooknum(chain->handle.family,
 					      chain->hook.name);
 		if (chain->hook.num == NF_INET_NUMHOOKS)
@@ -4854,6 +4856,14 @@ static int chain_evaluate(struct eval_ctx *ctx, struct chain *chain)
 			return __stmt_binary_error(ctx, &chain->priority.loc, NULL,
 						   "invalid priority expression %s in this context.",
 						   expr_name(chain->priority.expr));
+
+		if (!strcmp(chain->type.str, "nat") &&
+		    (mpz_export_data(&priority, chain->priority.expr->value,
+				    BYTEORDER_HOST_ENDIAN, sizeof(int))) &&
+		    priority <= -200)
+			return __stmt_binary_error(ctx, &chain->priority.loc, NULL,
+						   "Nat type chains must have a priority value above -200.");
+
 		if (chain->policy) {
 			expr_set_context(&ctx->ectx, &policy_type,
 					 NFT_NAME_MAXLEN * BITS_PER_BYTE);
-- 
2.38.0

Re: [nft PATCH] Reject invalid chain priority values in user space

[Pablo Neira Ayuso]

On Fri, Mar 10, 2023 at 01:13:14AM +0100, Phil Sutter wrote:
> The kernel doesn't accept nat type chains with a priority of -200 or
> below. Catch this and provide a better error message than the kernel's
> EOPNOTSUPP.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  src/evaluate.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/src/evaluate.c b/src/evaluate.c
> index d24f8b66b0de8..af4844c1ef6cc 100644
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -4842,6 +4842,8 @@ static int chain_evaluate(struct eval_ctx *ctx, struct chain *chain)
>  	}
>  
>  	if (chain->flags & CHAIN_F_BASECHAIN) {
> +		int priority;
> +
>  		chain->hook.num = str2hooknum(chain->handle.family,
>  					      chain->hook.name);
>  		if (chain->hook.num == NF_INET_NUMHOOKS)
> @@ -4854,6 +4856,14 @@ static int chain_evaluate(struct eval_ctx *ctx, struct chain *chain)
>  			return __stmt_binary_error(ctx, &chain->priority.loc, NULL,
>  						   "invalid priority expression %s in this context.",
>  						   expr_name(chain->priority.expr));
> +

maybe get this here to declutter the branch?

                mpz_export_data(&priority, chain->priority.expr->value,
                                BYTEORDER_HOST_ENDIAN, sizeof(int)));

this is in basechain context, so it should be fine.

> +		if (!strcmp(chain->type.str, "nat") &&
> +		    (mpz_export_data(&priority, chain->priority.expr->value,
> +				    BYTEORDER_HOST_ENDIAN, sizeof(int))) &&
> +		    priority <= -200)
> +			return __stmt_binary_error(ctx, &chain->priority.loc, NULL,
> +						   "Nat type chains must have a priority value above -200.");
                                                    ^^^

I'd suggest lower case 'nat' which is what the user specifies in the
chain declaration.

Thanks for addressing my feedback.

> +
>  		if (chain->policy) {
>  			expr_set_context(&ctx->ectx, &policy_type,
>  					 NFT_NAME_MAXLEN * BITS_PER_BYTE);
> -- 
> 2.38.0
> 

Re: [nft PATCH] Reject invalid chain priority values in user space

[Phil Sutter]

On Fri, Mar 10, 2023 at 10:12:36AM +0100, Pablo Neira Ayuso wrote:
> On Fri, Mar 10, 2023 at 01:13:14AM +0100, Phil Sutter wrote:
> > The kernel doesn't accept nat type chains with a priority of -200 or
> > below. Catch this and provide a better error message than the kernel's
> > EOPNOTSUPP.
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >  src/evaluate.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/src/evaluate.c b/src/evaluate.c
> > index d24f8b66b0de8..af4844c1ef6cc 100644
> > --- a/src/evaluate.c
> > +++ b/src/evaluate.c
> > @@ -4842,6 +4842,8 @@ static int chain_evaluate(struct eval_ctx *ctx, struct chain *chain)
> >  	}
> >  
> >  	if (chain->flags & CHAIN_F_BASECHAIN) {
> > +		int priority;
> > +
> >  		chain->hook.num = str2hooknum(chain->handle.family,
> >  					      chain->hook.name);
> >  		if (chain->hook.num == NF_INET_NUMHOOKS)
> > @@ -4854,6 +4856,14 @@ static int chain_evaluate(struct eval_ctx *ctx, struct chain *chain)
> >  			return __stmt_binary_error(ctx, &chain->priority.loc, NULL,
> >  						   "invalid priority expression %s in this context.",
> >  						   expr_name(chain->priority.expr));
> > +
> 
> maybe get this here to declutter the branch?
> 
>                 mpz_export_data(&priority, chain->priority.expr->value,
>                                 BYTEORDER_HOST_ENDIAN, sizeof(int)));

Will do. Initially I wanted to use mpz_get_si() but it expects a larger
data size. It even works if one casts the return value to int, but I
guess it won't anymore on Big Endian (and is a hack anyway).

> this is in basechain context, so it should be fine.

Yes, indeed. Also the call to evaluate_priority() ensures
chain->priority.expr is as expected.

> > +		if (!strcmp(chain->type.str, "nat") &&
> > +		    (mpz_export_data(&priority, chain->priority.expr->value,
> > +				    BYTEORDER_HOST_ENDIAN, sizeof(int))) &&
> > +		    priority <= -200)
> > +			return __stmt_binary_error(ctx, &chain->priority.loc, NULL,
> > +						   "Nat type chains must have a priority value above -200.");
>                                                     ^^^
> 
> I'd suggest lower case 'nat' which is what the user specifies in the
> chain declaration.

I'll rewrite the sentence.

> Thanks for addressing my feedback.

Thanks for the quick review!