[nft PATCH 1/4 v2] evaluate: fix a crash if we specify ether type or meta nfproto in reject

[nft PATCH 1/4 v2] evaluate: fix a crash if we specify ether type or meta nfproto in reject

[Alvaro Neira Ayuso]

If we use a rule:
nft add rule bridge filter input \
	ether type ip reject with icmp type host-unreachable

or this:

nft add rule inet filter input \
	meta nfproto ipv4 reject with icmp type host-unreachable

we have a segfault because we add a network dependency when we already have
network context.

Signed-off-by: Alvaro Neira Ayuso <alvaroneay@gmail.com>
---
[changes in v2]
* Fixed a incorrect refactor when we check the family in bridge

 src/evaluate.c |   57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 56 insertions(+), 1 deletion(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index 83ef749..4b7bda9 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -19,6 +19,7 @@
 #include <linux/netfilter/nf_tables.h>
 #include <netinet/ip_icmp.h>
 #include <netinet/icmp6.h>
+#include <net/ethernet.h>
 
 #include <expression.h>
 #include <statement.h>
@@ -1193,6 +1194,8 @@ static int stmt_reject_gen_dependency(struct eval_ctx *ctx, struct stmt *stmt,
 		BUG("cannot generate reject dependency for type %d",
 		    stmt->reject.type);
 	}
+	if (payload == NULL)
+		return 0;
 
 	if (payload_gen_dependency(ctx, payload, &nstmt) < 0)
 		return -1;
@@ -1204,6 +1207,9 @@ static int stmt_reject_gen_dependency(struct eval_ctx *ctx, struct stmt *stmt,
 static int stmt_evaluate_reject_family(struct eval_ctx *ctx, struct stmt *stmt,
 				       struct expr *expr)
 {
+	const struct proto_desc *desc, *base;
+	int protocol;
+
 	switch (ctx->pctx.family) {
 	case NFPROTO_ARP:
 		return stmt_error(ctx, stmt, "cannot use reject with arp");
@@ -1224,8 +1230,57 @@ static int stmt_evaluate_reject_family(struct eval_ctx *ctx, struct stmt *stmt,
 			break;
 		}
 		break;
-	case NFPROTO_INET:
 	case NFPROTO_BRIDGE:
+		base = ctx->pctx.protocol[PROTO_BASE_LL_HDR].desc;
+		desc = ctx->pctx.protocol[PROTO_BASE_NETWORK_HDR].desc;
+		if (desc != NULL &&
+		    stmt->reject.type != NFT_REJECT_TCP_RST) {
+			protocol = proto_find_num(base, desc);
+			switch (protocol) {
+			case __constant_htons(ETH_P_IP):
+				if (NFPROTO_IPV4 == stmt->reject.family)
+					break;
+				return stmt_error(ctx, stmt,
+				  "conflicting protocols specified: ip vs ip6");
+			case __constant_htons(ETH_P_IPV6):
+				if (NFPROTO_IPV6 == stmt->reject.family)
+					break;
+				return stmt_error(ctx, stmt,
+				  "conflicting protocols specified: ip vs ip6");
+			default:
+				return stmt_error(ctx, stmt,
+						"cannot reject this ether type");
+			}
+			break;
+		}
+		if (stmt->reject.type == NFT_REJECT_ICMPX_UNREACH)
+			break;
+		if (stmt_reject_gen_dependency(ctx, stmt, expr) < 0)
+			return -1;
+		break;
+	case NFPROTO_INET:
+		base = ctx->pctx.protocol[PROTO_BASE_LL_HDR].desc;
+		desc = ctx->pctx.protocol[PROTO_BASE_NETWORK_HDR].desc;
+		if (desc != NULL &&
+		    stmt->reject.type != NFT_REJECT_TCP_RST) {
+			protocol = proto_find_num(base, desc);
+			switch (protocol) {
+			case NFPROTO_IPV4:
+				if (stmt->reject.family == NFPROTO_IPV4)
+					break;
+				return stmt_error(ctx, stmt,
+				  "conflicting protocols specified: ip vs ip6");
+				break;
+			case NFPROTO_IPV6:
+				if (stmt->reject.family == NFPROTO_IPV6)
+					break;
+				return stmt_error(ctx, stmt,
+				  "conflicting protocols specified: ip vs ip6");
+			default:
+				BUG("unsupported family");
+			}
+			break;
+		}
 		if (stmt->reject.type == NFT_REJECT_ICMPX_UNREACH)
 			break;
 		if (stmt_reject_gen_dependency(ctx, stmt, expr) < 0)
-- 
1.7.10.4

[nft PATCH 2/4 v2] evaluate: fix a crash if we check the transport protocol

[Alvaro Neira Ayuso]

Example:

nft add rule inet filter input meta l4proto udp reject with tcp reset

When we check if the transport protocol is tcp, we use the network context.
If we don't have this network context, we have a crash.

Signed-off-by: Alvaro Neira Ayuso <alvaroneay@gmail.com>
---
[no changes in v2]

 src/evaluate.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/evaluate.c b/src/evaluate.c
index 4b7bda9..2f71e9b 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -1339,6 +1339,13 @@ static int stmt_evaluate_reset(struct eval_ctx *ctx, struct stmt *stmt)
 	if (desc == NULL)
 		return 0;
 
+	if (base == NULL) {
+		if (strcmp(desc->name, "tcp") == 0)
+			return 0;
+		else
+			return stmt_error(ctx, stmt,
+				 "you cannot use tcp reset with this protocol");
+	}
 	protonum = proto_find_num(base, desc);
 	switch (protonum) {
 	case IPPROTO_TCP:
-- 
1.7.10.4

[nft PATCH 3/4 v2] delinearize: list the icmpx reason with the string associated

[Alvaro Neira Ayuso]

If you add the rule:
  nft add rule inet filter input reject with icmpx type host-unreachable
  nft list table inet filter

shows:
  table inet filter {
	chain input {
		reject with icmpx type 2
	}
  }

We have to attach the icmpx datatype when we list the rules that use it. With
this patch if we list the ruleset, the output is:

  table inet filter {
	chain input {
		reject with icmpx type host-unreachable
	}
  }

Signed-off-by: Alvaro Neira Ayuso <alvaroneay@gmail.com>
---
[no changes in v2]

 src/netlink_delinearize.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 4bb4697..3e7aed4 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -928,8 +928,10 @@ static void stmt_reject_postprocess(struct rule_pp_ctx rctx, struct stmt *stmt)
 		stmt->reject.expr->dtype = &icmpv6_code_type;
 		break;
 	case NFPROTO_INET:
-		if (stmt->reject.type == NFT_REJECT_ICMPX_UNREACH)
+		if (stmt->reject.type == NFT_REJECT_ICMPX_UNREACH) {
+			stmt->reject.expr->dtype = &icmpx_code_type;
 			break;
+		}
 		base = rctx.pctx.protocol[PROTO_BASE_LL_HDR].desc;
 		desc = rctx.pctx.protocol[PROTO_BASE_NETWORK_HDR].desc;
 		protocol = proto_find_num(base, desc);
@@ -944,8 +946,10 @@ static void stmt_reject_postprocess(struct rule_pp_ctx rctx, struct stmt *stmt)
 		stmt->reject.family = protocol;
 		break;
 	case NFPROTO_BRIDGE:
-		if (stmt->reject.type == NFT_REJECT_ICMPX_UNREACH)
+		if (stmt->reject.type == NFT_REJECT_ICMPX_UNREACH) {
+			stmt->reject.expr->dtype = &icmpx_code_type;
 			break;
+		}
 		base = rctx.pctx.protocol[PROTO_BASE_LL_HDR].desc;
 		desc = rctx.pctx.protocol[PROTO_BASE_NETWORK_HDR].desc;
 		protocol = proto_find_num(base, desc);
-- 
1.7.10.4

[nft PATCH 4/4 v2] test: update and add the reject tests for ip, ip6, bridge and inet.

[Alvaro Neira Ayuso]

Signed-off-by: Alvaro Neira Ayuso <alvaroneay@gmail.com>
---
[changes in v2]
* Changed the format and added the rules with all the posible reasons

 tests/regression/bridge/reject.t |   30 ++++++++++++++++++++++++++++++
 tests/regression/inet/reject.t   |   28 ++++++++++++++++++++++++++++
 tests/regression/ip/reject.t     |   11 ++++++++++-
 tests/regression/ip6/reject.t    |    9 ++++++++-
 4 files changed, 76 insertions(+), 2 deletions(-)
 create mode 100644 tests/regression/bridge/reject.t
 create mode 100644 tests/regression/inet/reject.t

diff --git a/tests/regression/bridge/reject.t b/tests/regression/bridge/reject.t
new file mode 100644
index 0000000..68e6051
--- /dev/null
+++ b/tests/regression/bridge/reject.t
@@ -0,0 +1,30 @@
+*bridge;test-bridge
+:input;type filter hook input priority 0
+
+reject with icmp type host-unreachable;ok;ether type ip reject with icmp type host-unreachable
+reject with icmp type net-unreachable;ok;ether type ip reject with icmp type net-unreachable
+reject with icmp type prot-unreachable;ok;ether type ip reject with icmp type prot-unreachable
+reject with icmp type port-unreachable;ok;ether type ip reject
+reject with icmp type net-prohibited;ok;ether type ip reject with icmp type net-prohibited
+reject with icmp type host-prohibited;ok;ether type ip reject with icmp type host-prohibited
+reject with icmp type admin-prohibited;ok;ether type ip reject with icmp type admin-prohibited
+
+reject with icmpv6 type no-route;ok;ether type ip6 reject with icmpv6 type no-route
+reject with icmpv6 type admin-prohibited;ok;ether type ip6 reject with icmpv6 type admin-prohibited
+reject with icmpv6 type addr-unreachable;ok;ether type ip6 reject with icmpv6 type addr-unreachable
+reject with icmpv6 type port-unreachable;ok;ether type ip6 reject
+
+ip protocol tcp reject with tcp reset;ok;ip protocol 6 reject with tcp reset
+
+reject;ok
+reject with icmpx type host-unreachable;ok
+reject with icmpx type no-route;ok
+reject with icmpx type admin-prohibited;ok
+reject with icmpx type port-unreachable;ok;reject
+
+ether type ipv6 reject with icmp type host-unreachable;fail
+ether type ip6 reject with icmp type host-unreachable;fail
+ether type ip reject with icmpv6 type no-route;fail
+ether type vlan reject;fail
+ether type arp reject;fail
+ip protocol udp reject with tcp reset;fail
diff --git a/tests/regression/inet/reject.t b/tests/regression/inet/reject.t
new file mode 100644
index 0000000..7dd4598
--- /dev/null
+++ b/tests/regression/inet/reject.t
@@ -0,0 +1,28 @@
+*inet;test-inet
+:input;type filter hook input priority 0
+
+reject with icmp type host-unreachable;ok;meta nfproto ipv4 reject with icmp type host-unreachable
+reject with icmp type net-unreachable;ok;meta nfproto ipv4 reject with icmp type net-unreachable
+reject with icmp type prot-unreachable;ok;meta nfproto ipv4 reject with icmp type prot-unreachable
+reject with icmp type port-unreachable;ok;meta nfproto ipv4 reject
+reject with icmp type net-prohibited;ok;meta nfproto ipv4 reject with icmp type net-prohibited
+reject with icmp type host-prohibited;ok;meta nfproto ipv4 reject with icmp type host-prohibited
+reject with icmp type admin-prohibited;ok;meta nfproto ipv4 reject with icmp type admin-prohibited
+
+reject with icmpv6 type no-route;ok;meta nfproto ipv6 reject with icmpv6 type no-route
+reject with icmpv6 type admin-prohibited;ok;meta nfproto ipv6 reject with icmpv6 type admin-prohibited
+reject with icmpv6 type addr-unreachable;ok;meta nfproto ipv6 reject with icmpv6 type addr-unreachable
+reject with icmpv6 type port-unreachable;ok;meta nfproto ipv6 reject
+
+reject with tcp reset;ok;meta l4proto 6 reject with tcp reset
+
+reject;ok
+reject with icmpx type host-unreachable;ok
+reject with icmpx type no-route;ok
+reject with icmpx type admin-prohibited;ok
+reject with icmpx type port-unreachable;ok;reject
+
+meta nfproto ipv6 reject with icmp type host-unreachable;fail
+meta nfproto ipv4 ip protocol icmp reject with icmpv6 type no-route;fail
+meta nfproto ipv6 ip protocol icmp reject with icmp type host-unreachable;fail
+ip protocol udp reject with tcp reset;fail
diff --git a/tests/regression/ip/reject.t b/tests/regression/ip/reject.t
index e7fb15b..70a63a0 100644
--- a/tests/regression/ip/reject.t
+++ b/tests/regression/ip/reject.t
@@ -1,5 +1,14 @@
 *ip;test-ip4
-*ip;test-inet
 :output;type filter hook output priority 0
 
 reject;ok
+reject with icmp type host-unreachable;ok
+reject with icmp type net-unreachable;ok
+reject with icmp type prot-unreachable;ok
+reject with icmp type port-unreachable;ok;reject
+reject with icmp type net-prohibited;ok
+reject with icmp type host-prohibited;ok
+reject with icmp type admin-prohibited;ok
+
+reject with icmp type no-route;fail
+reject with icmpv6 type no-route;fail
diff --git a/tests/regression/ip6/reject.t b/tests/regression/ip6/reject.t
index b49c50b..60dec90 100644
--- a/tests/regression/ip6/reject.t
+++ b/tests/regression/ip6/reject.t
@@ -1,5 +1,12 @@
 *ip6;test-ip6
-*inet;test-inet
 :output;type filter hook output priority 0
 
 reject;ok
+reject with icmpv6 type no-route;ok
+reject with icmpv6 type admin-prohibited;ok
+reject with icmpv6 type addr-unreachable;ok
+reject with icmpv6 type port-unreachable;ok;reject
+reject with tcp reset;ok;ip6 nexthdr 6 reject with tcp reset
+
+reject with icmpv6 type host-unreachable;fail
+reject with icmp type host-unreachable;fail
-- 
1.7.10.4

Re: [nft PATCH 1/4 v2] evaluate: fix a crash if we specify ether type or meta nfproto in reject

[Pablo Neira Ayuso]

On Fri, Oct 17, 2014 at 02:24:34PM +0200, Alvaro Neira Ayuso wrote:
> If we use a rule:
> nft add rule bridge filter input \
> 	ether type ip reject with icmp type host-unreachable
> 
> or this:
> 
> nft add rule inet filter input \
> 	meta nfproto ipv4 reject with icmp type host-unreachable
> 
> we have a segfault because we add a network dependency when we already have
> network context.
> 
> Signed-off-by: Alvaro Neira Ayuso <alvaroneay@gmail.com>
> ---
> [changes in v2]
> * Fixed a incorrect refactor when we check the family in bridge
> 
>  src/evaluate.c |   57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 56 insertions(+), 1 deletion(-)
> 
> diff --git a/src/evaluate.c b/src/evaluate.c
> index 83ef749..4b7bda9 100644
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -19,6 +19,7 @@
>  #include <linux/netfilter/nf_tables.h>
>  #include <netinet/ip_icmp.h>
>  #include <netinet/icmp6.h>
> +#include <net/ethernet.h>
>  
>  #include <expression.h>
>  #include <statement.h>
> @@ -1193,6 +1194,8 @@ static int stmt_reject_gen_dependency(struct eval_ctx *ctx, struct stmt *stmt,
>  		BUG("cannot generate reject dependency for type %d",
>  		    stmt->reject.type);
>  	}
> +	if (payload == NULL)
> +		return 0;

Why this check?

>  	if (payload_gen_dependency(ctx, payload, &nstmt) < 0)
>  		return -1;

Re: [nft PATCH 3/4 v2] delinearize: list the icmpx reason with the string associated

[Pablo Neira Ayuso]

On Fri, Oct 17, 2014 at 02:24:36PM +0200, Alvaro Neira Ayuso wrote:
> If you add the rule:
>   nft add rule inet filter input reject with icmpx type host-unreachable
>   nft list table inet filter
> 
> shows:
>   table inet filter {
> 	chain input {
> 		reject with icmpx type 2
> 	}
>   }

Applied, thanks.

Re: [nft PATCH 1/4 v2] evaluate: fix a crash if we specify ether type or meta nfproto in reject

[Álvaro Neira Ayuso]

El 17/10/14 14:55, Pablo Neira Ayuso escribió:
> On Fri, Oct 17, 2014 at 02:24:34PM +0200, Alvaro Neira Ayuso wrote:
>> If we use a rule:
>> nft add rule bridge filter input \
>> 	ether type ip reject with icmp type host-unreachable
>>
>> or this:
>>
>> nft add rule inet filter input \
>> 	meta nfproto ipv4 reject with icmp type host-unreachable
>>
>> we have a segfault because we add a network dependency when we already have
>> network context.
>>
>> Signed-off-by: Alvaro Neira Ayuso <alvaroneay@gmail.com>
>> ---
>> [changes in v2]
>> * Fixed a incorrect refactor when we check the family in bridge
>>
>>   src/evaluate.c |   57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 56 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/evaluate.c b/src/evaluate.c
>> index 83ef749..4b7bda9 100644
>> --- a/src/evaluate.c
>> +++ b/src/evaluate.c
>> @@ -19,6 +19,7 @@
>>   #include <linux/netfilter/nf_tables.h>
>>   #include <netinet/ip_icmp.h>
>>   #include <netinet/icmp6.h>
>> +#include <net/ethernet.h>
>>
>>   #include <expression.h>
>>   #include <statement.h>
>> @@ -1193,6 +1194,8 @@ static int stmt_reject_gen_dependency(struct eval_ctx *ctx, struct stmt *stmt,
>>   		BUG("cannot generate reject dependency for type %d",
>>   		    stmt->reject.type);
>>   	}
>> +	if (payload == NULL)
>> +		return 0;
>
> Why this check?

If we already have context, the previously functions return a NULL 
payload. Therefore, if we try to create a dependency with this NULL 
payload, we have a crash.

>
>>   	if (payload_gen_dependency(ctx, payload, &nstmt) < 0)
>>   		return -1;

--
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

Re: [nft PATCH 1/4 v2] evaluate: fix a crash if we specify ether type or meta nfproto in reject

[Pablo Neira Ayuso]

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

On Fri, Oct 17, 2014 at 03:02:07PM +0200, Álvaro Neira Ayuso wrote:
> El 17/10/14 14:55, Pablo Neira Ayuso escribió:
> >On Fri, Oct 17, 2014 at 02:24:34PM +0200, Alvaro Neira Ayuso wrote:
> >>If we use a rule:
> >>nft add rule bridge filter input \
> >>	ether type ip reject with icmp type host-unreachable
> >>
> >>or this:
> >>
> >>nft add rule inet filter input \
> >>	meta nfproto ipv4 reject with icmp type host-unreachable
> >>
> >>we have a segfault because we add a network dependency when we already have
> >>network context.
> >>
> >>Signed-off-by: Alvaro Neira Ayuso <alvaroneay@gmail.com>
> >>---
> >>[changes in v2]
> >>* Fixed a incorrect refactor when we check the family in bridge
> >>
> >>  src/evaluate.c |   57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >>  1 file changed, 56 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/src/evaluate.c b/src/evaluate.c
> >>index 83ef749..4b7bda9 100644
> >>--- a/src/evaluate.c
> >>+++ b/src/evaluate.c
> >>@@ -19,6 +19,7 @@
> >>  #include <linux/netfilter/nf_tables.h>
> >>  #include <netinet/ip_icmp.h>
> >>  #include <netinet/icmp6.h>
> >>+#include <net/ethernet.h>
> >>
> >>  #include <expression.h>
> >>  #include <statement.h>
> >>@@ -1193,6 +1194,8 @@ static int stmt_reject_gen_dependency(struct eval_ctx *ctx, struct stmt *stmt,
> >>  		BUG("cannot generate reject dependency for type %d",
> >>  		    stmt->reject.type);
> >>  	}
> >>+	if (payload == NULL)
> >>+		return 0;
> >
> >Why this check?
> 
> If we already have context, the previously functions return a NULL
> payload. Therefore, if we try to create a dependency with this NULL
> payload, we have a crash.

I prefer if you can change the return value logic in
reject_payload_gen_dependency*() to:

  1: payload dependency was created
  0: no payload dependency needed
 -1: error

See patch attached.

[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 1531 bytes --]

diff --git a/src/evaluate.c b/src/evaluate.c
index d61d76b..71dc767 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -1140,9 +1140,10 @@ static int reject_payload_gen_dependency_tcp(struct eval_ctx *ctx,
 	desc = ctx->pctx.protocol[PROTO_BASE_TRANSPORT_HDR].desc;
 	if (desc != NULL)
 		return 0;
+
 	*payload = payload_expr_alloc(&stmt->location, &proto_tcp,
 				      TCPHDR_UNSPEC);
-	return 0;
+	return 1;
 }
 
 static int reject_payload_gen_dependency_family(struct eval_ctx *ctx,
@@ -1171,7 +1172,7 @@ static int reject_payload_gen_dependency_family(struct eval_ctx *ctx,
 	default:
 		BUG("unknown reject family");
 	}
-	return 0;
+	return 1;
 }
 
 static int stmt_reject_gen_dependency(struct eval_ctx *ctx, struct stmt *stmt,
@@ -1179,21 +1180,21 @@ static int stmt_reject_gen_dependency(struct eval_ctx *ctx, struct stmt *stmt,
 {
 	struct expr *payload = NULL;
 	struct stmt *nstmt;
+	int ret;
 
 	switch (stmt->reject.type) {
 	case NFT_REJECT_TCP_RST:
-		if (reject_payload_gen_dependency_tcp(ctx, stmt, &payload) < 0)
-			return -1;
+		ret = reject_payload_gen_dependency_tcp(ctx, stmt, &payload);
 		break;
 	case NFT_REJECT_ICMP_UNREACH:
-		if (reject_payload_gen_dependency_family(ctx, stmt,
-							 &payload) < 0)
-			return -1;
+		ret = reject_payload_gen_dependency_family(ctx, stmt, &payload);
 		break;
 	default:
 		BUG("cannot generate reject dependency for type %d",
 		    stmt->reject.type);
 	}
+	if (ret <= 0)
+		return ret;
 
 	if (payload_gen_dependency(ctx, payload, &nstmt) < 0)
 		return -1;

Re: [nft PATCH 1/4 v2] evaluate: fix a crash if we specify ether type or meta nfproto in reject

[Álvaro Neira Ayuso]

El 17/10/14 15:38, Pablo Neira Ayuso escribió:
> On Fri, Oct 17, 2014 at 03:02:07PM +0200, Álvaro Neira Ayuso wrote:
>> El 17/10/14 14:55, Pablo Neira Ayuso escribió:
>>> On Fri, Oct 17, 2014 at 02:24:34PM +0200, Alvaro Neira Ayuso wrote:
>>>> If we use a rule:
>>>> nft add rule bridge filter input \
>>>> 	ether type ip reject with icmp type host-unreachable
>>>>
>>>> or this:
>>>>
>>>> nft add rule inet filter input \
>>>> 	meta nfproto ipv4 reject with icmp type host-unreachable
>>>>
>>>> we have a segfault because we add a network dependency when we already have
>>>> network context.
>>>>
>>>> Signed-off-by: Alvaro Neira Ayuso <alvaroneay@gmail.com>
>>>> ---
>>>> [changes in v2]
>>>> * Fixed a incorrect refactor when we check the family in bridge
>>>>
>>>>   src/evaluate.c |   57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>   1 file changed, 56 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/evaluate.c b/src/evaluate.c
>>>> index 83ef749..4b7bda9 100644
>>>> --- a/src/evaluate.c
>>>> +++ b/src/evaluate.c
>>>> @@ -19,6 +19,7 @@
>>>>   #include <linux/netfilter/nf_tables.h>
>>>>   #include <netinet/ip_icmp.h>
>>>>   #include <netinet/icmp6.h>
>>>> +#include <net/ethernet.h>
>>>>
>>>>   #include <expression.h>
>>>>   #include <statement.h>
>>>> @@ -1193,6 +1194,8 @@ static int stmt_reject_gen_dependency(struct eval_ctx *ctx, struct stmt *stmt,
>>>>   		BUG("cannot generate reject dependency for type %d",
>>>>   		    stmt->reject.type);
>>>>   	}
>>>> +	if (payload == NULL)
>>>> +		return 0;
>>>
>>> Why this check?
>>
>> If we already have context, the previously functions return a NULL
>> payload. Therefore, if we try to create a dependency with this NULL
>> payload, we have a crash.
>
> I prefer if you can change the return value logic in
> reject_payload_gen_dependency*() to:
>
>    1: payload dependency was created
>    0: no payload dependency needed
>   -1: error
>
> See patch attached.
>

Nice idea. Looks good to me.
--
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

Re: [nft PATCH 2/4 v2] evaluate: fix a crash if we check the transport protocol

[Pablo Neira Ayuso]

On Fri, Oct 17, 2014 at 02:24:35PM +0200, Alvaro Neira Ayuso wrote:
> Example:
> 
> nft add rule inet filter input meta l4proto udp reject with tcp reset
> 
> When we check if the transport protocol is tcp, we use the network context.
> If we don't have this network context, we have a crash.
> 
> Signed-off-by: Alvaro Neira Ayuso <alvaroneay@gmail.com>
> ---
> [no changes in v2]
> 
>  src/evaluate.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/src/evaluate.c b/src/evaluate.c
> index 4b7bda9..2f71e9b 100644
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -1339,6 +1339,13 @@ static int stmt_evaluate_reset(struct eval_ctx *ctx, struct stmt *stmt)
>  	if (desc == NULL)
>  		return 0;
>  
> +	if (base == NULL) {
> +		if (strcmp(desc->name, "tcp") == 0)
> +			return 0;
> +		else
> +			return stmt_error(ctx, stmt,
> +				 "you cannot use tcp reset with this protocol");
> +	}

Can you give a try to this?

        if (base == NULL &&
            ctx->table.handle.family == NFPROTO_INET)
                base = &proto_inet_service;

>  	protonum = proto_find_num(base, desc);
>  	switch (protonum) {
>  	case IPPROTO_TCP:
> -- 
> 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

Re: [nft PATCH 2/4 v2] evaluate: fix a crash if we check the transport protocol

[Álvaro Neira Ayuso]

El 20/10/14 10:59, Pablo Neira Ayuso escribió:
> On Fri, Oct 17, 2014 at 02:24:35PM +0200, Alvaro Neira Ayuso wrote:
>> Example:
>>
>> nft add rule inet filter input meta l4proto udp reject with tcp reset
>>
>> When we check if the transport protocol is tcp, we use the network context.
>> If we don't have this network context, we have a crash.
>>
>> Signed-off-by: Alvaro Neira Ayuso <alvaroneay@gmail.com>
>> ---
>> [no changes in v2]
>>
>>   src/evaluate.c |    7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/src/evaluate.c b/src/evaluate.c
>> index 4b7bda9..2f71e9b 100644
>> --- a/src/evaluate.c
>> +++ b/src/evaluate.c
>> @@ -1339,6 +1339,13 @@ static int stmt_evaluate_reset(struct eval_ctx *ctx, struct stmt *stmt)
>>   	if (desc == NULL)
>>   		return 0;
>>
>> +	if (base == NULL) {
>> +		if (strcmp(desc->name, "tcp") == 0)
>> +			return 0;
>> +		else
>> +			return stmt_error(ctx, stmt,
>> +				 "you cannot use tcp reset with this protocol");
>> +	}
>
> Can you give a try to this?
>
>          if (base == NULL &&
>              ctx->table.handle.family == NFPROTO_INET)
>                  base = &proto_inet_service;

It works. That was another solution that I thought. But we don't need to 
compare the family because the base can be NULL only with Inet and 
Bridge tables.

>
>>   	protonum = proto_find_num(base, desc);
>>   	switch (protonum) {
>>   	case IPPROTO_TCP:
>> --
>> 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

--
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

Re: [nft PATCH 2/4 v2] evaluate: fix a crash if we check the transport protocol

[Pablo Neira Ayuso]

On Mon, Oct 20, 2014 at 11:40:17AM +0200, Álvaro Neira Ayuso wrote:
> El 20/10/14 10:59, Pablo Neira Ayuso escribió:
> >On Fri, Oct 17, 2014 at 02:24:35PM +0200, Alvaro Neira Ayuso wrote:
> >>Example:
> >>
> >>nft add rule inet filter input meta l4proto udp reject with tcp reset
> >>
> >>When we check if the transport protocol is tcp, we use the network context.
> >>If we don't have this network context, we have a crash.
> >>
> >>Signed-off-by: Alvaro Neira Ayuso <alvaroneay@gmail.com>
> >>---
> >>[no changes in v2]
> >>
> >>  src/evaluate.c |    7 +++++++
> >>  1 file changed, 7 insertions(+)
> >>
> >>diff --git a/src/evaluate.c b/src/evaluate.c
> >>index 4b7bda9..2f71e9b 100644
> >>--- a/src/evaluate.c
> >>+++ b/src/evaluate.c
> >>@@ -1339,6 +1339,13 @@ static int stmt_evaluate_reset(struct eval_ctx *ctx, struct stmt *stmt)
> >>  	if (desc == NULL)
> >>  		return 0;
> >>
> >>+	if (base == NULL) {
> >>+		if (strcmp(desc->name, "tcp") == 0)
> >>+			return 0;
> >>+		else
> >>+			return stmt_error(ctx, stmt,
> >>+				 "you cannot use tcp reset with this protocol");
> >>+	}
> >
> >Can you give a try to this?
> >
> >         if (base == NULL &&
> >             ctx->table.handle.family == NFPROTO_INET)
> >                 base = &proto_inet_service;
> 
> It works. That was another solution that I thought. But we don't
> need to compare the family because the base can be NULL only with
> Inet and Bridge tables.

OK, but better you still check for bridge and inet there. We may
introduce changes later on that may easily break this code because of
this assumption.
--
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

Re: [nft PATCH 2/4 v2] evaluate: fix a crash if we check the transport protocol

[Álvaro Neira Ayuso]

El 20/10/14 11:46, Pablo Neira Ayuso escribió:
> On Mon, Oct 20, 2014 at 11:40:17AM +0200, Álvaro Neira Ayuso wrote:
>> El 20/10/14 10:59, Pablo Neira Ayuso escribió:
>>> On Fri, Oct 17, 2014 at 02:24:35PM +0200, Alvaro Neira Ayuso wrote:
>>>> Example:
>>>>
>>>> nft add rule inet filter input meta l4proto udp reject with tcp reset
>>>>
>>>> When we check if the transport protocol is tcp, we use the network context.
>>>> If we don't have this network context, we have a crash.
>>>>
>>>> Signed-off-by: Alvaro Neira Ayuso <alvaroneay@gmail.com>
>>>> ---
>>>> [no changes in v2]
>>>>
>>>>   src/evaluate.c |    7 +++++++
>>>>   1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/src/evaluate.c b/src/evaluate.c
>>>> index 4b7bda9..2f71e9b 100644
>>>> --- a/src/evaluate.c
>>>> +++ b/src/evaluate.c
>>>> @@ -1339,6 +1339,13 @@ static int stmt_evaluate_reset(struct eval_ctx *ctx, struct stmt *stmt)
>>>>   	if (desc == NULL)
>>>>   		return 0;
>>>>
>>>> +	if (base == NULL) {
>>>> +		if (strcmp(desc->name, "tcp") == 0)
>>>> +			return 0;
>>>> +		else
>>>> +			return stmt_error(ctx, stmt,
>>>> +				 "you cannot use tcp reset with this protocol");
>>>> +	}
>>>
>>> Can you give a try to this?
>>>
>>>          if (base == NULL &&
>>>              ctx->table.handle.family == NFPROTO_INET)
>>>                  base = &proto_inet_service;
>>
>> It works. That was another solution that I thought. But we don't
>> need to compare the family because the base can be NULL only with
>> Inet and Bridge tables.
>
> OK, but better you still check for bridge and inet there. We may
> introduce changes later on that may easily break this code because of
> this assumption.
>

Perfect. That's true. Thanks Pablo.
--
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