[nft PATCH 0/4 RFC] Support IPv6 AH header matches

[nft PATCH 0/4 RFC] Support IPv6 AH header matches

[Phil Sutter]

The following series allows users to match on IPv6 AH header fields. I
consider this a bit of a hack since it's the "cheap" solution. As to why
this is, let me first picture the problem once again:

AH header is an extension header in IPv6 land. This means in order to
find it, one has to use ip6_find_hdr() (in kernel space) since
pkt->xt.thoff (which payload expression uses) points past extension
headers in IPv6 packets. So while the existing AH header match for IPv4
uses payload expression, a theoretical one for IPv6 has to use exthdr
expression.

Expressions are constructed in user space at rule creation time, so it
is not always clear which packet family they will get applied to (see
inet or bridge family tables).

I see two alternatives for solving this situation: The better one is to
allow the kernel to choose the right expression (payload or exthdr)
depending on IP address family when searching the AH header. The other
one is presented here: Userspace demands the user to clarify which IP
address family an AH header match should apply to, so it can create the
right expression for the job.

For the sake of simplicity, in this implementation I went without some
kind of placeholder expression but just convert the parser-generated
payload expression into an exthdr one if protocol context states layer 3
is IPv6.

I looked at netlink debug output and the following commands seem to turn
out right:

| $ nft add rule ip t c ah spi 2	# ip table family
| $ nft add rule ip6 t c ah spi 2	# ip6 table family
| $ nft add rule inet t c ip6 version 6 ah spi 2
| $ nft add rule inet t c ip version 4 ah spi 2

There are still a few oddities here: E.g. 'meta protocol ip6' is not
sufficient since that doesn't create a protocol dependency, hence why I
used the somewhat redundant 'ip6 version 6' match instead. So if we
decide to use the solution proposed here, I'd suggest do patch up meta
expression to generate the dependency as well.

Phil Sutter (4):
  payload: Carry template number around for internal use
  exthdr: Align max templates count with payload expr
  exthdr: Define AH header description
  payload: Convert AH header expression to exthdr for IPv6

 include/expression.h |  1 +
 include/exthdr.h     |  3 ++-
 src/evaluate.c       | 30 ++++++++++++++++++++++++++++++
 src/exthdr.c         | 16 ++++++++++++++++
 src/payload.c        |  1 +
 5 files changed, 50 insertions(+), 1 deletion(-)

-- 
2.11.0

[nft PATCH 1/4 RFC] payload: Carry template number around for internal use

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/expression.h | 1 +
 src/payload.c        | 1 +
 2 files changed, 2 insertions(+)

diff --git a/include/expression.h b/include/expression.h
index 9ba87e8273a3a..1b31160713750 100644
--- a/include/expression.h
+++ b/include/expression.h
@@ -277,6 +277,7 @@ struct expr {
 			const struct proto_hdr_template	*tmpl;
 			enum proto_bases		base;
 			unsigned int			offset;
+			unsigned int			type;
 		} payload;
 		struct {
 			/* EXPR_EXTHDR */
diff --git a/src/payload.c b/src/payload.c
index 55128fee14986..4e353e8e5c8b0 100644
--- a/src/payload.c
+++ b/src/payload.c
@@ -145,6 +145,7 @@ struct expr *payload_expr_alloc(const struct location *loc,
 	expr->payload.tmpl   = tmpl;
 	expr->payload.base   = base;
 	expr->payload.offset = tmpl->offset;
+	expr->payload.type   = type;
 
 	return expr;
 }
-- 
2.11.0

[nft PATCH 2/4 RFC] exthdr: Align max templates count with payload expr

[Phil Sutter]

This is for convenience only, so that when translating a payload
expression into an exthdr one we can't accidentally address data out of
bounds.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/exthdr.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/exthdr.h b/include/exthdr.h
index a2647ee142dcd..a5522ea5b5e5d 100644
--- a/include/exthdr.h
+++ b/include/exthdr.h
@@ -14,7 +14,7 @@
 struct exthdr_desc {
 	const char			*name;
 	uint8_t				type;
-	struct proto_hdr_template	templates[10];
+	struct proto_hdr_template	templates[PROTO_HDRS_MAX];
 };
 
 extern struct expr *exthdr_expr_alloc(const struct location *loc,
-- 
2.11.0

[nft PATCH 3/4 RFC] exthdr: Define AH header description

[Phil Sutter]

This is not directly used from parsing stage since AH header match is
defined there as payload expression. Instead it will be used when
converting that payload expression into an exthdr one for IPv6.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/exthdr.h |  1 +
 src/exthdr.c     | 16 ++++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/include/exthdr.h b/include/exthdr.h
index a5522ea5b5e5d..0f9c17222a47a 100644
--- a/include/exthdr.h
+++ b/include/exthdr.h
@@ -89,5 +89,6 @@ extern const struct exthdr_desc exthdr_rt2;
 extern const struct exthdr_desc exthdr_frag;
 extern const struct exthdr_desc exthdr_dst;
 extern const struct exthdr_desc exthdr_mh;
+extern const struct exthdr_desc exthdr_ah;
 
 #endif /* NFTABLES_EXTHDR_H */
diff --git a/src/exthdr.c b/src/exthdr.c
index c8599f233132a..c08b39a0ee8d2 100644
--- a/src/exthdr.c
+++ b/src/exthdr.c
@@ -104,6 +104,7 @@ static const struct exthdr_desc *exthdr_protocols[IPPROTO_MAX] = {
 	[IPPROTO_FRAGMENT]	= &exthdr_frag,
 	[IPPROTO_DSTOPTS]	= &exthdr_dst,
 	[IPPROTO_MH]		= &exthdr_mh,
+	[IPPROTO_AH]		= &exthdr_ah,
 };
 
 const struct exthdr_desc *exthdr_find_proto(uint8_t proto)
@@ -344,6 +345,21 @@ const struct exthdr_desc exthdr_mh = {
 	},
 };
 
+#define AH_FIELD(__name, __member, __dtype) \
+	HDR_TEMPLATE(__name, __dtype, struct ip_auth_hdr, __member)
+
+const struct exthdr_desc exthdr_ah = {
+	.name		= "ah",
+	.type		= IPPROTO_AH,
+	.templates	= {
+		[AHHDR_NEXTHDR]		= AH_FIELD("nexthdr", nexthdr, &inet_protocol_type),
+		[AHHDR_HDRLENGTH]	= AH_FIELD("hdrlength", hdrlen, &integer_type),
+		[AHHDR_RESERVED]	= AH_FIELD("reserved", reserved, &integer_type),
+		[AHHDR_SPI]		= AH_FIELD("spi", spi, &integer_type),
+		[AHHDR_SEQUENCE]	= AH_FIELD("sequence", seq_no, &integer_type),
+	},
+};
+
 static void __init exthdr_init(void)
 {
 	datatype_register(&mh_type_type);
-- 
2.11.0

[nft PATCH 4/4 RFC] payload: Convert AH header expression to exthdr for IPv6

[Phil Sutter]

If protocol context specifies IPv6 on network layer, convert the payload
expression at hand into an exthdr one. OTOH, if network layer protocol
is not IPv4, bail out with an error since it is not clear what the user
wants in this case.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/evaluate.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/src/evaluate.c b/src/evaluate.c
index 27cee98916db0..9b28d5266135d 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -633,10 +633,40 @@ static bool payload_needs_adjustment(const struct expr *expr)
 	       expr->len % BITS_PER_BYTE != 0;
 }
 
+static int proto_desc2proto(const struct proto_desc *proto_desc)
+{
+	int i;
+
+	for (i = 0; i < NFPROTO_NUMPROTO; i++) {
+		if (hook_proto_desc[i].desc == proto_desc)
+			return i;
+	}
+
+	return NFPROTO_NUMPROTO;
+}
+
 static int expr_evaluate_payload(struct eval_ctx *ctx, struct expr **exprp)
 {
 	struct expr *expr = *exprp;
 
+	if (expr->payload.desc == &proto_ah) {
+		struct proto_desc *desc =
+			ctx->pctx.protocol[PROTO_BASE_NETWORK_HDR].desc;
+		switch (proto_desc2proto(desc)) {
+		case NFPROTO_IPV4:
+			break;
+		case NFPROTO_IPV6:
+			expr = exthdr_expr_alloc(&expr->location, &exthdr_ah,
+						 expr->payload.type);
+			expr_free(*exprp);
+			*exprp = expr;
+			return expr_evaluate(ctx, exprp);
+			break;
+		default:
+			return expr_error(ctx->msgs, expr, "ah header match requires address family to be either IPv4 or IPv6 (got %d)", proto_desc2proto(desc));
+		}
+	}
+
 	if (__expr_evaluate_payload(ctx, expr) < 0)
 		return -1;
 
-- 
2.11.0

Re: [nft PATCH 0/4 RFC] Support IPv6 AH header matches

[Pablo Neira Ayuso]

On Mon, May 22, 2017 at 06:49:32PM +0200, Phil Sutter wrote:
> The following series allows users to match on IPv6 AH header fields. I
> consider this a bit of a hack since it's the "cheap" solution. As to why
> this is, let me first picture the problem once again:
> 
> AH header is an extension header in IPv6 land. This means in order to
> find it, one has to use ip6_find_hdr() (in kernel space) since
> pkt->xt.thoff (which payload expression uses) points past extension
> headers in IPv6 packets. So while the existing AH header match for IPv4
> uses payload expression, a theoretical one for IPv6 has to use exthdr
> expression.
> 
> Expressions are constructed in user space at rule creation time, so it
> is not always clear which packet family they will get applied to (see
> inet or bridge family tables).
> 
> I see two alternatives for solving this situation: The better one is to
> allow the kernel to choose the right expression (payload or exthdr)
> depending on IP address family when searching the AH header. The other
> one is presented here: Userspace demands the user to clarify which IP
> address family an AH header match should apply to, so it can create the
> right expression for the job.
> 
> For the sake of simplicity, in this implementation I went without some
> kind of placeholder expression but just convert the parser-generated
> payload expression into an exthdr one if protocol context states layer 3
> is IPv6.
> 
> I looked at netlink debug output and the following commands seem to turn
> out right:
> 
> | $ nft add rule ip t c ah spi 2	# ip table family
> | $ nft add rule ip6 t c ah spi 2	# ip6 table family
> | $ nft add rule inet t c ip6 version 6 ah spi 2
> | $ nft add rule inet t c ip version 4 ah spi 2
> 
> There are still a few oddities here: E.g. 'meta protocol ip6' is not
> sufficient since that doesn't create a protocol dependency, hence why I
> used the somewhat redundant 'ip6 version 6' match instead. So if we
> decide to use the solution proposed here, I'd suggest do patch up meta
> expression to generate the dependency as well.

We fixed this from the kernel side, so both IPv4 and IPv6 meta l4proto
says ah. And transport offsets are set to the AH header.

So the code we already have should work out of the box.

Am I missing anything?