[PATCH nft 1/2] src: allow filtering on L2 header in inet family

[PATCH nft 1/2] src: allow filtering on L2 header in inet family

[Florian Westphal]

Error: conflicting protocols specified: inet vs. ether
tcp dport 22 iiftype ether ether saddr 00:0f:54:0c:11:4
                                   ^^^^^^^^^^^

This allows the implicit inet proto dependency to get replaced
by an ethernet one.

This is possible since by the time we detect the conflict the
meta dependency for the network protocol has already been added.

So we only need to add another dependency on the Linklayer frame type.

Closes: http://bugzilla.netfilter.org/show_bug.cgi?id=981
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/meta.h |  1 +
 src/evaluate.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
 src/meta.c     | 24 +++++++++++++++++++++++-
 src/payload.c  |  8 +-------
 4 files changed, 71 insertions(+), 9 deletions(-)

diff --git a/include/meta.h b/include/meta.h
index 459221f..40652e3 100644
--- a/include/meta.h
+++ b/include/meta.h
@@ -26,4 +26,5 @@ struct meta_template {
 extern struct expr *meta_expr_alloc(const struct location *loc,
 				    enum nft_meta_keys key);
 
+struct stmt *meta_stmt_meta_iiftype(const struct location *loc, uint16_t type);
 #endif /* NFTABLES_META_H */
diff --git a/src/evaluate.c b/src/evaluate.c
index 581f364..165d360 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -286,6 +286,34 @@ conflict_resolution_gen_dependency(struct eval_ctx *ctx, int protocol,
 	return 0;
 }
 
+/* dependency supersede.
+ *
+ * 'inet' is a 'phony' l2 dependeny used by NFPROTO_INET to fulfill network
+ * header dependency, i.e. ensure that 'ip saddr 1.2.3.4' only sees ip headers.
+ *
+ * If a match expression that depends on a particular L2 header, e.g. ethernet,
+ * is used, we thus get a conflict since we already have a l2 header dependency.
+ *
+ * But in the inet case we can just ignore the conflict since only another
+ * restriction is added, and these are not mutually exclusive.
+ *
+ * Example: inet filter in ip saddr 1.2.3.4 ether saddr a:b:c:d:e:f
+ *
+ * ip saddr adds meta dependency on ipv4 packets
+ * ether saddr adds another dependeny on ethernet frames.
+ */
+static bool supersede_dep(const struct proto_desc *have,
+			  struct expr *payload)
+{
+	if (payload->payload.base != PROTO_BASE_LL_HDR || have->length)
+		return false;
+
+	if (have != &proto_inet)
+		return false;
+
+	return true;
+}
+
 static bool resolve_protocol_conflict(struct eval_ctx *ctx,
 				      struct expr *payload)
 {
@@ -305,7 +333,24 @@ static bool resolve_protocol_conflict(struct eval_ctx *ctx,
 	if (payload->payload.base != h->base)
 		return false;
 
-	assert(desc->length);
+	if (supersede_dep(desc, payload)) {
+		uint16_t type;
+
+		if (proto_dev_type(payload->payload.desc, &type) < 0)
+			return expr_error(ctx->msgs, payload,
+					  "protocol specification is invalid "
+					  "for this family");
+
+		nstmt = meta_stmt_meta_iiftype(&payload->location, type);
+		if (stmt_evaluate(ctx, nstmt) < 0)
+			return expr_error(ctx->msgs, payload,
+					  "dependency statement is invalid");
+
+		list_add_tail(&nstmt->list, &ctx->stmt->list);
+		ctx->pctx.protocol[base].desc = payload->payload.desc;
+		return true;
+	}
+
 	if (base < PROTO_BASE_MAX) {
 		const struct proto_desc *next = ctx->pctx.protocol[base + 1].desc;
 
diff --git a/src/meta.c b/src/meta.c
index bfc1258..0b52fda 100644
--- a/src/meta.c
+++ b/src/meta.c
@@ -19,6 +19,8 @@
 #include <net/if_arp.h>
 #include <pwd.h>
 #include <grp.h>
+#include <arpa/inet.h>
+#include <linux/netfilter.h>
 #include <linux/pkt_sched.h>
 #include <linux/if_packet.h>
 
@@ -468,7 +470,7 @@ static void meta_expr_pctx_update(struct proto_ctx *ctx,
 
 	switch (left->meta.key) {
 	case NFT_META_IIFTYPE:
-		if (h->base < PROTO_BASE_NETWORK_HDR)
+		if (h->base < PROTO_BASE_NETWORK_HDR && ctx->family != NFPROTO_INET)
 			return;
 
 		desc = proto_dev_desc(mpz_get_uint16(right->value));
@@ -572,3 +574,23 @@ static void __init meta_init(void)
 	datatype_register(&devgroup_type);
 	datatype_register(&pkttype_type);
 }
+
+/*
+ * @expr:	payload expression
+ * @res:	dependency expression
+ *
+ * Generate a NFT_META_IIFTYPE expression to check for ethernet frames.
+ * Only works on input path.
+ */
+struct stmt *meta_stmt_meta_iiftype(const struct location *loc, uint16_t type)
+{
+	struct expr *dep, *left, *right;
+
+	left = meta_expr_alloc(loc, NFT_META_IIFTYPE);
+	right = constant_expr_alloc(loc, &arphrd_type,
+				    BYTEORDER_HOST_ENDIAN,
+				    2 * BITS_PER_BYTE, &type);
+
+	dep = relational_expr_alloc(loc, OP_EQ, left, right);
+	return expr_stmt_alloc(&dep->location, dep);
+}
diff --git a/src/payload.c b/src/payload.c
index 23afa2f..b75527a 100644
--- a/src/payload.c
+++ b/src/payload.c
@@ -183,13 +183,7 @@ int payload_gen_dependency(struct eval_ctx *ctx, const struct expr *expr,
 					  "protocol specification is invalid "
 					  "for this family");
 
-		left = meta_expr_alloc(&expr->location, NFT_META_IIFTYPE);
-		right = constant_expr_alloc(&expr->location, &arphrd_type,
-					    BYTEORDER_HOST_ENDIAN,
-					    2 * BITS_PER_BYTE, &type);
-
-		dep = relational_expr_alloc(&expr->location, OP_EQ, left, right);
-		stmt = expr_stmt_alloc(&dep->location, dep);
+		stmt = meta_stmt_meta_iiftype(&expr->location, type);
 		if (stmt_evaluate(ctx, stmt) < 0) {
 			return expr_error(ctx->msgs, expr,
 					  "dependency statement is invalid");
-- 
2.0.5

[PATCH nft 2/2] rule: don't reorder protocol payload expressions when merging

[Florian Westphal]

An instruction like

 bridge filter input ip saddr 1.2.3.4 ether saddr a:b:c:d:e:f

is displayed as

unknown unknown 0x1020304 [invalid type] ether saddr 00:0f:54:0c:11:04 ether type ip

.. because the (implicit) 'ether type ip' that is injected before the
network header match gets merged into the ether saddr instruction.

This inverts the merge in case the merge candidate contains
a next header protocol field.

After this change, the rule will be displayed as

bridge filter input ether saddr a:b:c:d:e:f ip saddr 1.2.3.4

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/rule.c | 49 ++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 42 insertions(+), 7 deletions(-)

diff --git a/src/rule.c b/src/rule.c
index 92b83f0..3c526cc 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -1179,30 +1179,65 @@ static int payload_match_stmt_cmp(const void *p1, const void *p2)
 
 static void payload_do_merge(struct stmt *sa[], unsigned int n)
 {
-	struct expr *last, *this, *expr;
+	struct expr *last, *this, *expr1, *expr2;
 	struct stmt *stmt;
-	unsigned int i;
+	unsigned int i, j;
 
 	qsort(sa, n, sizeof(sa[0]), payload_match_stmt_cmp);
 
 	last = sa[0]->expr;
-	for (i = 1; i < n; i++) {
+	for (j = 0, i = 1; i < n; i++) {
 		stmt = sa[i];
 		this = stmt->expr;
 
 		if (!payload_is_adjacent(last->left, this->left) ||
 		    last->op != this->op) {
 			last = this;
+			j = i;
 			continue;
 		}
 
-		expr = payload_expr_join(last->left, this->left);
+		expr1 = payload_expr_join(last->left, this->left);
+		expr2 = constant_expr_join(last->right, this->right);
+
+		/* We can merge last into this, but we can't replace
+		 * the statement associated with this if it does contain
+		 * a higher level protocol.
+		 *
+		 * ether type ip ip saddr X ether saddr Y
+		 * ... can be changed to
+		 * ether type ip ether saddr Y ip saddr X
+		 * ... but not
+		 * ip saddr X ether type ip ether saddr Y
+		 *
+		 * The latter form means we perform ip saddr test before
+		 * ensuring ip dependency, plus it makes decoding harder
+		 * since we don't know the type of the network header
+		 * right away.
+		 *
+		 * So, if we're about to replace a statement
+		 * containing a protocol identifier, just swap this and last
+		 * and replace the other one (i.e., replace 'load ether type ip'
+		 * with the combined 'load both ether type and saddr') and not
+		 * the other way around.
+		 */
+		if (this->left->flags & EXPR_F_PROTOCOL) {
+			struct expr *tmp = last;
+
+			last = this;
+			this = tmp;
+
+			expr1->flags |= EXPR_F_PROTOCOL;
+			stmt = sa[j];
+			assert(stmt->expr == this);
+			j = i;
+		}
+
 		expr_free(last->left);
-		last->left = expr;
+		last->left = expr1;
 
-		expr = constant_expr_join(last->right, this->right);
 		expr_free(last->right);
-		last->right = expr;
+		last->right = expr2;
 
 		list_del(&stmt->list);
 		stmt_free(stmt);
-- 
2.0.5

Re: [PATCH nft 2/2] rule: don't reorder protocol payload expressions when merging

[Florian Westphal]

Florian Westphal <fw@strlen.de> wrote:
> An instruction like
> 
>  bridge filter input ip saddr 1.2.3.4 ether saddr a:b:c:d:e:f
> 
> is displayed as
> 
> unknown unknown 0x1020304 [invalid type] ether saddr 00:0f:54:0c:11:04 ether type ip
> 
> .. because the (implicit) 'ether type ip' that is injected before the
> network header match gets merged into the ether saddr instruction.
> 
> This inverts the merge in case the merge candidate contains
> a next header protocol field.
> 
> After this change, the rule will be displayed as
> 
> bridge filter input ether saddr a:b:c:d:e:f ip saddr 1.2.3.4

One side-effect with this approach is that it will reorder the
instructions depending on the family dependencies.

So, f.e.
tcp dport 22 ip daddr 1.2.3.4 ether saddr 00:0f:54:0c:11:4

will be added/displayed as
tcp dport 22 ether saddr 00:0f:54:0c:11:04 ip daddr 1.2.3.4

for bridge family, but as

tcp dport 22 ip daddr 1.2.3.4 ether saddr 00:0f:54:0c:11:04

for ip and inet.

In the bridge case we do need a 'is this ip' dependency test, so we
now merge the ether saddr x into the (implicitly inserted)
'ether protocol ip' expression.

ip family doesn't have such a dependency, since network header is
always ipv4.

For INET the dependency is expressed via 'meta load nfproto', so
no payload merge takes place either.

I pushed a rebased version including tests to
https://git.breakpoint.cc/cgit/fw/nftables.git/log/?h=ether_tests_02

Re: [PATCH nft 2/2] rule: don't reorder protocol payload expressions when merging

[Pablo Neira Ayuso]

On Sat, Sep 26, 2015 at 05:14:03AM +0200, Florian Westphal wrote:
> An instruction like
> 
>  bridge filter input ip saddr 1.2.3.4 ether saddr a:b:c:d:e:f
> 
> is displayed as
> 
> unknown unknown 0x1020304 [invalid type] ether saddr 00:0f:54:0c:11:04 ether type ip
> 
> .. because the (implicit) 'ether type ip' that is injected before the
> network header match gets merged into the ether saddr instruction.
> 
> This inverts the merge in case the merge candidate contains
> a next header protocol field.
> 
> After this change, the rule will be displayed as
> 
> bridge filter input ether saddr a:b:c:d:e:f ip saddr 1.2.3.4
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>

Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

Re: [PATCH nft 1/2] src: allow filtering on L2 header in inet family

[Pablo Neira Ayuso]

On Sat, Sep 26, 2015 at 05:14:02AM +0200, Florian Westphal wrote:
> Error: conflicting protocols specified: inet vs. ether
> tcp dport 22 iiftype ether ether saddr 00:0f:54:0c:11:4
>                                    ^^^^^^^^^^^
> 
> This allows the implicit inet proto dependency to get replaced
> by an ethernet one.
> 
> This is possible since by the time we detect the conflict the
> meta dependency for the network protocol has already been added.
> 
> So we only need to add another dependency on the Linklayer frame type.
> 
> Closes: http://bugzilla.netfilter.org/show_bug.cgi?id=981
> Signed-off-by: Florian Westphal <fw@strlen.de>

Acked-by: Pablo Neira Ayuso <pablo@netfilter.org>

P.S: You push this Florian, thanks.