| Message ID | 20191224231428.1972155-1-jeremy@azazel.net |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nft,v2] evaluate: fix expr_set_context call for shift binops. | expand |
Hi Jeremy, 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.
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.
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); if (expr_evaluate(ctx, &op->right) < 0) return -1; right = op->right;
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.