Re: [nft PATCH v2] payload: generate dependency with wrong byteorder value format

[Pablo Neira Ayuso <pablo@netfilter.org>]

Hi Alvaro,

On Fri, Jul 11, 2014 at 01:09:34PM +0200, Alvaro Neira Ayuso wrote:
> From: Álvaro Neira Ayuso <alvaroneay@gmail.com>
>
> In all case that we have added a payload dependency, we have supposed
> that the byteorder must to be BYTEORDER_HOST_ENDIAN, the problem is
> when we want to add a dependency that the value has another byteorder.
> For example, if we try to add a new payload dependency in a bridge table
> and we use ether type, the byteorder is BYTEORDER_BIG_ENDIAN. The value
> of the type ip is 0x0800 in ether but when we add the payload dependency
> for this specific protocol, we will have a payload like this:
>
> [ payload load 2b @ link header + 12 => reg 1 ]
>   [ cmp eq reg 1 0x00000008 ]
>
> This patch allows to create payload dependency with the byteorder of the
> template. For that I have updated the function for updating the context for
> using the byteorder of the template too. With this changes we have a payload
> with the correct format:
>
> [ payload load 2b @ link header + 12 => reg 1 ]
>   [ cmp eq reg 1 0x00000800 ]
>
> Signed-off-by: Alvaro Neira Ayuso <alvaroneay@gmail.com>
> ---
> [tested with the rules]
> nft add rule filter input ip protocol tcp counter
> nft add rule filter input ip protocol udp counter
> nft add rule filter input tcp dport 22 counter
>
>  src/payload.c |    9 +++++++--
>  src/proto.c   |    8 ++++++++
>  2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/src/payload.c b/src/payload.c
> index a1785a5..fb78ba5 100644
> --- a/src/payload.c
> +++ b/src/payload.c
> @@ -69,13 +69,18 @@ static void payload_expr_pctx_update(struct proto_ctx *ctx,
>  {
>  	const struct expr *left = expr->left, *right = expr->right;
>  	const struct proto_desc *base, *desc;
> +	const struct proto_hdr_template *tmpl;
> +	uint32_t value = 0;
>
>  	if (!(left->flags & EXPR_F_PROTOCOL))
>  		return;
>
>  	assert(expr->op == OP_EQ);
>  	base = ctx->protocol[left->payload.base].desc;
> -	desc = proto_find_upper(base, mpz_get_uint32(right->value));
> +	tmpl = &base->templates[base->protocol_key];

Note here, you are fetching the template based on the protocol key:

struct proto_desc {
        const char                      *name;
        enum proto_bases                base;
        unsigned int                    protocol_key;
        struct {
                unsigned int                    num;
                const struct proto_desc         *desc;
        }                               protocols[PROTO_UPPER_MAX];
        struct proto_hdr_template       templates[PROTO_HDRS_MAX]; <---
};

However, the length of the templates array is:

#define PROTO_HDRS_MAX          20

and now see below:

> +	mpz_export_data(&value, right->value, tmpl->dtype->byteorder,
> +			div_round_up(tmpl->len, BITS_PER_BYTE));
> +	desc = proto_find_upper(base, value);
>
>  	proto_ctx_update(ctx, left->payload.base + 1, &expr->location, desc);
>  }
> @@ -208,7 +213,7 @@ int payload_gen_dependency(struct eval_ctx *ctx, const struct expr *expr,
>  		left = payload_expr_alloc(&expr->location, desc, desc->protocol_key);
>
>  	right = constant_expr_alloc(&expr->location, tmpl->dtype,
> -				    BYTEORDER_HOST_ENDIAN,
> +				    tmpl->dtype->byteorder,
>  				    tmpl->len,
>  				    constant_data_ptr(protocol, tmpl->len));
>
> diff --git a/src/proto.c b/src/proto.c
> index 546ef10..4192108 100644
> --- a/src/proto.c
> +++ b/src/proto.c
> @@ -249,6 +249,7 @@ const struct proto_desc proto_ah = {
>  const struct proto_desc proto_esp = {
>  	.name		= "esp",
>  	.base		= PROTO_BASE_TRANSPORT_HDR,
> +	.protocol_key	= IPPROTO_ESP,

IPPROTO_ESP is 50, this won't fly since it's over the array boundary,
so you will be accessing out of boundary memory area.

I think the protocol_key needs to be an internal value defined in
include/proto.h, for example:

enum transport_protocol {
        L4PROTO_INVALID,
        L4PROTO_TCP,
        L4PROTO_UDP,
        ...
};

that you should use instead.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html