* [nft PATCH v2] payload: generate dependency with wrong byteorder value format
@ 2014-07-11 11:09 Alvaro Neira Ayuso
2014-07-12 13:10 ` Pablo Neira Ayuso
0 siblings, 1 reply; 3+ messages in thread
From: Alvaro Neira Ayuso @ 2014-07-11 11:09 UTC (permalink / raw)
To: netfilter-devel; +Cc: kaber
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];
+ 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,
.templates = {
[ESPHDR_SPI] = ESPHDR_FIELD("spi", spi),
[ESPHDR_SEQUENCE] = ESPHDR_FIELD("sequence", seq_no),
@@ -326,6 +327,7 @@ static const struct datatype icmp_type_type = {
const struct proto_desc proto_icmp = {
.name = "icmp",
.base = PROTO_BASE_TRANSPORT_HDR,
+ .protocol_key = IPPROTO_ICMP,
.templates = {
[ICMPHDR_TYPE] = ICMPHDR_TYPE("type", &icmp_type_type, type),
[ICMPHDR_CODE] = ICMPHDR_FIELD("code", code),
@@ -348,6 +350,7 @@ const struct proto_desc proto_icmp = {
const struct proto_desc proto_udp = {
.name = "udp",
.base = PROTO_BASE_TRANSPORT_HDR,
+ .protocol_key = IPPROTO_UDP,
.templates = {
[UDPHDR_SPORT] = INET_SERVICE("sport", struct udphdr, source),
[UDPHDR_DPORT] = INET_SERVICE("dport", struct udphdr, dest),
@@ -359,6 +362,7 @@ const struct proto_desc proto_udp = {
const struct proto_desc proto_udplite = {
.name = "udplite",
.base = PROTO_BASE_TRANSPORT_HDR,
+ .protocol_key = IPPROTO_UDPLITE,
.templates = {
[UDPHDR_SPORT] = INET_SERVICE("sport", struct udphdr, source),
[UDPHDR_DPORT] = INET_SERVICE("dport", struct udphdr, dest),
@@ -403,6 +407,7 @@ static const struct datatype tcp_flag_type = {
const struct proto_desc proto_tcp = {
.name = "tcp",
.base = PROTO_BASE_TRANSPORT_HDR,
+ .protocol_key = IPPROTO_TCP,
.templates = {
[TCPHDR_SPORT] = INET_SERVICE("sport", struct tcphdr, source),
[TCPHDR_DPORT] = INET_SERVICE("dport", struct tcphdr, dest),
@@ -456,6 +461,7 @@ static const struct datatype dccp_pkttype_type = {
const struct proto_desc proto_dccp = {
.name = "dccp",
.base = PROTO_BASE_TRANSPORT_HDR,
+ .protocol_key = IPPROTO_DCCP,
.templates = {
[DCCPHDR_SPORT] = INET_SERVICE("sport", struct dccp_hdr, dccph_sport),
[DCCPHDR_DPORT] = INET_SERVICE("dport", struct dccp_hdr, dccph_dport),
@@ -473,6 +479,7 @@ const struct proto_desc proto_dccp = {
const struct proto_desc proto_sctp = {
.name = "sctp",
.base = PROTO_BASE_TRANSPORT_HDR,
+ .protocol_key = IPPROTO_SCTP,
.templates = {
[SCTPHDR_SPORT] = INET_SERVICE("sport", struct sctphdr, source),
[SCTPHDR_DPORT] = INET_SERVICE("dport", struct sctphdr, dest),
@@ -566,6 +573,7 @@ static const struct datatype icmp6_type_type = {
const struct proto_desc proto_icmp6 = {
.name = "icmpv6",
.base = PROTO_BASE_TRANSPORT_HDR,
+ .protocol_key = IPPROTO_ICMPV6,
.templates = {
[ICMP6HDR_TYPE] = ICMP6HDR_TYPE("type", &icmp6_type_type, icmp6_type),
[ICMP6HDR_CODE] = ICMP6HDR_FIELD("code", icmp6_code),
--
1.7.10.4
--
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [nft PATCH v2] payload: generate dependency with wrong byteorder value format
2014-07-11 11:09 [nft PATCH v2] payload: generate dependency with wrong byteorder value format Alvaro Neira Ayuso
@ 2014-07-12 13:10 ` Pablo Neira Ayuso
2014-07-12 19:16 ` Patrick McHardy
0 siblings, 1 reply; 3+ messages in thread
From: Pablo Neira Ayuso @ 2014-07-12 13:10 UTC (permalink / raw)
To: Alvaro Neira Ayuso; +Cc: netfilter-devel, kaber
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [nft PATCH v2] payload: generate dependency with wrong byteorder value format
2014-07-12 13:10 ` Pablo Neira Ayuso
@ 2014-07-12 19:16 ` Patrick McHardy
0 siblings, 0 replies; 3+ messages in thread
From: Patrick McHardy @ 2014-07-12 19:16 UTC (permalink / raw)
To: Pablo Neira Ayuso, Alvaro Neira Ayuso; +Cc: netfilter-devel
On 12. Juli 2014 14:10:12 GMT+01:00, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>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:
The protocol *key* specifies the header field contaibing the next layer protocol value, not the protocol number, so using it this way is not correct.
The easy fix would be to always use big endian byteorder, as all other cases have 1 byte keys. I'm actually wondering though, the ethertype field is defined using big endian, so the constant should be converted to the correct byte order during evaluation. Unfortunately my notebook broke so I can't check the code right now, but my feeling is that this is where the bug is actually located.
>
>enum transport_protocol {
> L4PROTO_INVALID,
> L4PROTO_TCP,
> L4PROTO_UDP,
> ...
>};
>
>that you should use instead.
--
Diese Nachricht wurde von meinem Android-Mobiltelefon mit K-9 Mail gesendet.
--
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-07-12 19:22 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-07-11 11:09 [nft PATCH v2] payload: generate dependency with wrong byteorder value format Alvaro Neira Ayuso
2014-07-12 13:10 ` Pablo Neira Ayuso
2014-07-12 19:16 ` Patrick McHardy
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.