Re: [PATCH nft v2] evaluate: fix expr_set_context call for shift binops.

[Jeremy Sowden <jeremy@azazel.net>]

[-- Attachment #1: Type: text/plain, Size: 2047 bytes --]

On 2020-01-06, at 10:28:42 +0100, Pablo Neira Ayuso wrote:
> On Tue, Dec 24, 2019 at 11:14:28PM +0000, Jeremy Sowden wrote:
> > expr_evaluate_binop calls expr_set_context for shift expressions to
> > set the context data-type to `integer`.  This clobbers the
> > byte-order of the context, resulting in unexpected conversions to
> > NBO.  For example:
> >
> >   $ sudo nft flush ruleset
> >   $ sudo nft add table t
> >   $ sudo nft add chain t c '{ type filter hook output priority mangle; }'
> >   $ sudo nft add rule t c oif lo tcp dport ssh ct mark set '0x10 | 0xe'
> >   $ sudo nft add rule t c oif lo tcp dport ssh ct mark set '0xf << 1'
> >   $ sudo nft list table t
> >   table ip t {
> >           chain c {
> >                   type filter hook output priority mangle; policy accept;
> >                   oif "lo" tcp dport 22 ct mark set 0x0000001e
> >                   oif "lo" tcp dport 22 ct mark set 0x1e000000
> >           }
> >   }
> >
> > Replace it with a call to __expr_set_context in order to preserve host
> > endianness.
> >
> > Signed-off-by: Jeremy Sowden <jeremy@azazel.net>
> > ---
> >  src/evaluate.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > Change-log:
> >
> >   In v1, I just dropped the expr_set_context call; however, it is needed
> >   to ensure that the right operand has integer type.  Instead, I now
> >   change it to __expr_set_context in order to ensure that the byte-order
> >   is correct.
> >
> > diff --git a/src/evaluate.c b/src/evaluate.c
> > index a865902c0fc7..43637e1cf6c8 100644
> > --- a/src/evaluate.c
> > +++ b/src/evaluate.c
> > @@ -1145,7 +1145,8 @@ static int expr_evaluate_binop(struct eval_ctx *ctx, struct expr **expr)
> >  	left = op->left;
> >
> >  	if (op->op == OP_LSHIFT || op->op == OP_RSHIFT)
> > -		expr_set_context(&ctx->ectx, &integer_type, ctx->ectx.len);
> > +		__expr_set_context(&ctx->ectx, &integer_type,
> > +				   BYTEORDER_HOST_ENDIAN, ctx->ectx.len, 0);
>
> tests/py spews a few warnings after this patch.

I'll take a look.

Cheers.

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]