[PATCH RESEND nf-next] netfilter: add support for matching IPv4 options

[PATCH RESEND nf-next] netfilter: add support for matching IPv4 options

[Stephen Suryaputra]

This is the kernel change for the overall changes with this description:
Add capability to have rules matching IPv4 options. This is developed
mainly to support dropping of IP packets with loose and/or strict source
route route options. Nevertheless, the implementation include others and
ability to get specific fields in the option.

v2: Fix style issues. Make this work with NFPROTO_INET (inet tables),
    NFPROTO_BRIDGE and the NFPROTO_NETDEV families. Check skb->protocol.
    Remove ability to input IP header offset for ipv4_find_option()
    function (all per Pablo Neira Ayuso).

Signed-off-by: Stephen Suryaputra <ssuryaextr@gmail.com>
---
 include/net/inet_sock.h                  |   2 +-
 include/uapi/linux/netfilter/nf_tables.h |   2 +
 net/ipv4/ip_options.c                    |   2 +
 net/netfilter/nft_exthdr.c               | 133 +++++++++++++++++++++++
 4 files changed, 138 insertions(+), 1 deletion(-)

diff --git a/include/net/inet_sock.h b/include/net/inet_sock.h
index e8eef85006aa..8db4f8639a33 100644
--- a/include/net/inet_sock.h
+++ b/include/net/inet_sock.h
@@ -55,7 +55,7 @@ struct ip_options {
 			ts_needaddr:1;
 	unsigned char	router_alert;
 	unsigned char	cipso;
-	unsigned char	__pad2;
+	unsigned char	end;
 	unsigned char	__data[0];
 };
 
diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
index 505393c6e959..168d741f42c5 100644
--- a/include/uapi/linux/netfilter/nf_tables.h
+++ b/include/uapi/linux/netfilter/nf_tables.h
@@ -730,10 +730,12 @@ enum nft_exthdr_flags {
  *
  * @NFT_EXTHDR_OP_IPV6: match against ipv6 extension headers
  * @NFT_EXTHDR_OP_TCP: match against tcp options
+ * @NFT_EXTHDR_OP_IPV4: match against ipv4 options
  */
 enum nft_exthdr_op {
 	NFT_EXTHDR_OP_IPV6,
 	NFT_EXTHDR_OP_TCPOPT,
+	NFT_EXTHDR_OP_IPV4,
 	__NFT_EXTHDR_OP_MAX
 };
 #define NFT_EXTHDR_OP_MAX	(__NFT_EXTHDR_OP_MAX - 1)
diff --git a/net/ipv4/ip_options.c b/net/ipv4/ip_options.c
index 3db31bb9df50..fc0e694aa97c 100644
--- a/net/ipv4/ip_options.c
+++ b/net/ipv4/ip_options.c
@@ -272,6 +272,7 @@ int __ip_options_compile(struct net *net,
 	for (l = opt->optlen; l > 0; ) {
 		switch (*optptr) {
 		case IPOPT_END:
+			opt->end = optptr - iph;
 			for (optptr++, l--; l > 0; optptr++, l--) {
 				if (*optptr != IPOPT_END) {
 					*optptr = IPOPT_END;
@@ -473,6 +474,7 @@ int __ip_options_compile(struct net *net,
 		*info = htonl((pp_ptr-iph)<<24);
 	return -EINVAL;
 }
+EXPORT_SYMBOL(__ip_options_compile);
 
 int ip_options_compile(struct net *net,
 		       struct ip_options *opt, struct sk_buff *skb)
diff --git a/net/netfilter/nft_exthdr.c b/net/netfilter/nft_exthdr.c
index a940c9fd9045..4155a32fade7 100644
--- a/net/netfilter/nft_exthdr.c
+++ b/net/netfilter/nft_exthdr.c
@@ -62,6 +62,125 @@ static void nft_exthdr_ipv6_eval(const struct nft_expr *expr,
 	regs->verdict.code = NFT_BREAK;
 }
 
+/* find the offset to specified option or the header beyond the options
+ * if target < 0.
+ *
+ * If target header is found, its offset is set in *offset and return option
+ * number. Otherwise, return negative error.
+ *
+ * If the first fragment doesn't contain the End of Options it is considered
+ * invalid.
+ */
+static int ipv4_find_option(struct net *net, struct sk_buff *skb,
+			    unsigned int *offset, int target,
+			    unsigned short *fragoff, int *flags)
+{
+	unsigned char optbuf[sizeof(struct ip_options) + 41];
+	struct ip_options *opt = (struct ip_options *)optbuf;
+	struct iphdr *iph, _iph;
+	unsigned int start;
+	bool found = false;
+	__be32 info;
+	int optlen;
+
+	if (fragoff)
+		*fragoff = 0;
+
+	iph = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
+	if (!iph || iph->version != 4)
+		return -EBADMSG;
+	start = sizeof(struct iphdr);
+
+	optlen = iph->ihl * 4 - (int)sizeof(struct iphdr);
+	if (optlen <= 0)
+		return -ENOENT;
+
+	memset(opt, 0, sizeof(struct ip_options));
+	/* Copy the options since __ip_options_compile() modifies
+	 * the options. Get one byte beyond the option for target < 0
+	 */
+	if (skb_copy_bits(skb, start, opt->__data, optlen + 1))
+		return -EBADMSG;
+	opt->optlen = optlen;
+
+	if (__ip_options_compile(net, opt, NULL, &info))
+		return -EBADMSG;
+
+	switch (target) {
+	case IPOPT_SSRR:
+	case IPOPT_LSRR:
+		if (!opt->srr)
+			break;
+		found = target == IPOPT_SSRR ? opt->is_strictroute :
+					       !opt->is_strictroute;
+		if (found)
+			*offset = opt->srr + start;
+		break;
+	case IPOPT_RR:
+		if (opt->rr)
+			break;
+		*offset = opt->rr + start;
+		found = true;
+		break;
+	case IPOPT_RA:
+		if (opt->router_alert)
+			break;
+		*offset = opt->router_alert + start;
+		found = true;
+		break;
+	default:
+		/* Either not supported or not a specific search, treated as
+		 * found
+		 */
+		found = true;
+		if (target >= 0) {
+			target = -EOPNOTSUPP;
+			break;
+		}
+		if (opt->end) {
+			*offset = opt->end + start;
+			target = IPOPT_END;
+		} else {
+			/* Point to beyond the options. */
+			*offset = optlen + start;
+			target = opt->__data[optlen];
+		}
+	}
+	if (!found)
+		target = -ENOENT;
+	return target;
+}
+
+static void nft_exthdr_ipv4_eval(const struct nft_expr *expr,
+				 struct nft_regs *regs,
+				 const struct nft_pktinfo *pkt)
+{
+	struct nft_exthdr *priv = nft_expr_priv(expr);
+	u32 *dest = &regs->data[priv->dreg];
+	struct sk_buff *skb = pkt->skb;
+	unsigned int offset;
+	int err;
+
+	if (skb->protocol != htons(ETH_P_IP))
+		goto err;
+
+	err = ipv4_find_option(nft_net(pkt), skb, &offset, priv->type, NULL, NULL);
+	if (priv->flags & NFT_EXTHDR_F_PRESENT) {
+		*dest = (err >= 0);
+		return;
+	} else if (err < 0) {
+		goto err;
+	}
+	offset += priv->offset;
+
+	dest[priv->len / NFT_REG32_SIZE] = 0;
+	if (skb_copy_bits(pkt->skb, offset, dest, priv->len) < 0)
+		goto err;
+	return;
+err:
+	regs->verdict.code = NFT_BREAK;
+}
+
 static void *
 nft_tcp_header_pointer(const struct nft_pktinfo *pkt,
 		       unsigned int len, void *buffer, unsigned int *tcphdr_len)
@@ -360,6 +479,14 @@ static const struct nft_expr_ops nft_exthdr_ipv6_ops = {
 	.dump		= nft_exthdr_dump,
 };
 
+static const struct nft_expr_ops nft_exthdr_ipv4_ops = {
+	.type		= &nft_exthdr_type,
+	.size		= NFT_EXPR_SIZE(sizeof(struct nft_exthdr)),
+	.eval		= nft_exthdr_ipv4_eval,
+	.init		= nft_exthdr_init,
+	.dump		= nft_exthdr_dump,
+};
+
 static const struct nft_expr_ops nft_exthdr_tcp_ops = {
 	.type		= &nft_exthdr_type,
 	.size		= NFT_EXPR_SIZE(sizeof(struct nft_exthdr)),
@@ -400,6 +527,12 @@ nft_exthdr_select_ops(const struct nft_ctx *ctx,
 		if (tb[NFTA_EXTHDR_DREG])
 			return &nft_exthdr_ipv6_ops;
 		break;
+	case NFT_EXTHDR_OP_IPV4:
+		if (ctx->family != NFPROTO_IPV6) {
+			if (tb[NFTA_EXTHDR_DREG])
+				return &nft_exthdr_ipv4_ops;
+		}
+		break;
 	}
 
 	return ERR_PTR(-EOPNOTSUPP);
-- 
2.17.1

Re: [PATCH RESEND nf-next] netfilter: add support for matching IPv4 options

[Pablo Neira Ayuso]

On Tue, Jun 11, 2019 at 08:09:12AM -0400, Stephen Suryaputra wrote:
[...]
> diff --git a/net/netfilter/nft_exthdr.c b/net/netfilter/nft_exthdr.c
> index a940c9fd9045..4155a32fade7 100644
> --- a/net/netfilter/nft_exthdr.c
> +++ b/net/netfilter/nft_exthdr.c
> @@ -62,6 +62,125 @@ static void nft_exthdr_ipv6_eval(const struct nft_expr *expr,
>  	regs->verdict.code = NFT_BREAK;
>  }
>  
> +/* find the offset to specified option or the header beyond the options
> + * if target < 0.
> + *
> + * If target header is found, its offset is set in *offset and return option
> + * number. Otherwise, return negative error.
> + *
> + * If the first fragment doesn't contain the End of Options it is considered
> + * invalid.
> + */
> +static int ipv4_find_option(struct net *net, struct sk_buff *skb,
> +			    unsigned int *offset, int target,
> +			    unsigned short *fragoff, int *flags)

flags is never used, please remove it.

> +{
> +	unsigned char optbuf[sizeof(struct ip_options) + 41];

In other parts of the kernel this is + 40:

net/ipv4/cipso_ipv4.c:  unsigned char optbuf[sizeof(struct ip_options) + 40];

here it is + 41.

> +	struct ip_options *opt = (struct ip_options *)optbuf;
> +	struct iphdr *iph, _iph;
> +	unsigned int start;
> +	bool found = false;
> +	__be32 info;
> +	int optlen;
> +
> +	if (fragoff)
> +		*fragoff = 0;

fragoff is set and never used. Please, remove this parameter.

> +	iph = skb_header_pointer(skb, 0, sizeof(_iph), &_iph);
> +	if (!iph || iph->version != 4)
> +		return -EBADMSG;
> +	start = sizeof(struct iphdr);
> +
> +	optlen = iph->ihl * 4 - (int)sizeof(struct iphdr);
> +	if (optlen <= 0)
> +		return -ENOENT;
> +
> +	memset(opt, 0, sizeof(struct ip_options));
> +	/* Copy the options since __ip_options_compile() modifies
> +	 * the options. Get one byte beyond the option for target < 0

How does this "one byte beyond the option" trick works?

> +	 */
> +	if (skb_copy_bits(skb, start, opt->__data, optlen + 1))
> +		return -EBADMSG;
> +	opt->optlen = optlen;
> +
> +	if (__ip_options_compile(net, opt, NULL, &info))
> +		return -EBADMSG;
> +
> +	switch (target) {
> +	case IPOPT_SSRR:
> +	case IPOPT_LSRR:
> +		if (!opt->srr)
> +			break;
> +		found = target == IPOPT_SSRR ? opt->is_strictroute :
> +					       !opt->is_strictroute;
> +		if (found)
> +			*offset = opt->srr + start;
> +		break;
> +	case IPOPT_RR:
> +		if (opt->rr)
> +			break;
> +		*offset = opt->rr + start;
> +		found = true;
> +		break;
> +	case IPOPT_RA:
> +		if (opt->router_alert)
> +			break;
> +		*offset = opt->router_alert + start;
> +		found = true;
> +		break;
> +	default:
> +		/* Either not supported or not a specific search, treated as
> +		 * found
> +		 */
> +		found = true;
> +		if (target >= 0) {
> +			target = -EOPNOTSUPP;
> +			break;
> +		}
> +		if (opt->end) {
> +			*offset = opt->end + start;
> +			target = IPOPT_END;

May I ask, what's the purpose of IPOPT_END? :-)

> +		} else {
> +			/* Point to beyond the options. */
> +			*offset = optlen + start;
> +			target = opt->__data[optlen];
> +		}
> +	}
> +	if (!found)
> +		target = -ENOENT;
> +	return target;

nitpick: Probably replace code above.

        return found ? target : -ENOENT;

Apart from the above, this looks good to me.

Thanks!

Re: [PATCH RESEND nf-next] netfilter: add support for matching IPv4 options

[Stephen Suryaputra]

On Tue, Jun 18, 2019 at 05:31:12PM +0200, Pablo Neira Ayuso wrote:
> > +{
> > +	unsigned char optbuf[sizeof(struct ip_options) + 41];
> 
> In other parts of the kernel this is + 40:
> 
> net/ipv4/cipso_ipv4.c:  unsigned char optbuf[sizeof(struct ip_options) + 40];
> 
> here it is + 41.
>
> ...
>
> > +	/* Copy the options since __ip_options_compile() modifies
> > +	 * the options. Get one byte beyond the option for target < 0
> 
> How does this "one byte beyond the option" trick works?

I used ipv6_find_hdr() as a reference. There if target is set to less
than 0, then the offset points to the byte beyond the extension header.
In this function, it points to the byte beyond the option. I wanted to
be as close as a working code as possible. Also, why +41 instead of +40.

> > +		if (opt->end) {
> > +			*offset = opt->end + start;
> > +			target = IPOPT_END;
> 
> May I ask, what's the purpose of IPOPT_END? :-)

My understanding is that in ipv6_find_hdr() if the nexthdr is
NEXTHDR_NONE, then that's the one being returned. The same here: target
is the return value.

> Apart from the above, this looks good to me.

AOK for other comments. I can spin another version.

Thank you,

Stephen.

Re: [PATCH RESEND nf-next] netfilter: add support for matching IPv4 options

[Pablo Neira Ayuso]

On Tue, Jun 18, 2019 at 10:13:55AM -0400, Stephen Suryaputra wrote:
> On Tue, Jun 18, 2019 at 05:31:12PM +0200, Pablo Neira Ayuso wrote:
> > > +{
> > > +	unsigned char optbuf[sizeof(struct ip_options) + 41];
> > 
> > In other parts of the kernel this is + 40:
> > 
> > net/ipv4/cipso_ipv4.c:  unsigned char optbuf[sizeof(struct ip_options) + 40];
> > 
> > here it is + 41.
> >
> > ...
> >
> > > +	/* Copy the options since __ip_options_compile() modifies
> > > +	 * the options. Get one byte beyond the option for target < 0
> > 
> > How does this "one byte beyond the option" trick works?
> 
> I used ipv6_find_hdr() as a reference. There if target is set to less
> than 0, then the offset points to the byte beyond the extension header.
> In this function, it points to the byte beyond the option. I wanted to
> be as close as a working code as possible. Also, why +41 instead of +40.

OK. But this is never used in this new extension, priv->type is always
set. I mean, we already have a pointer to the transport header via
nft_pktinfo->xt.thoff.

> > > +		if (opt->end) {
> > > +			*offset = opt->end + start;
> > > +			target = IPOPT_END;
> > 
> > May I ask, what's the purpose of IPOPT_END? :-)
> 
> My understanding is that in ipv6_find_hdr() if the nexthdr is
> NEXTHDR_NONE, then that's the one being returned. The same here: target
> is the return value.

Code that falls under the default: case that deals with IPOPT_END is
never exercised, right?

priv->type is always set to >= 0, so the opt->end never happens.
Hence, we can remove the chunk in net/ipv4/ip_options.c.

> > Apart from the above, this looks good to me.
> 
> AOK for other comments. I can spin another version.

Please, do.

Thanks for explaining. I understand motivation is to mimic
ipv6_find_hdr() which is a good idea indeed... if this functions
becomes used away from the netfilter tree at some point that would be
a good pattern to extend it. However, so far - please correct me if
I'm mistaken - for the requirements of nft_exthdr to support IPv4
options, we don't seem to need it, so I would prefer to start with the
bare minimum code that nft_exthdr needs, if possible.

Thanks!

Re: [PATCH RESEND nf-next] netfilter: add support for matching IPv4 options

[Pablo Neira Ayuso]

On Tue, Jun 11, 2019 at 08:09:12AM -0400, Stephen Suryaputra wrote:
[...]
> diff --git a/net/netfilter/nft_exthdr.c b/net/netfilter/nft_exthdr.c
> index a940c9fd9045..4155a32fade7 100644
> --- a/net/netfilter/nft_exthdr.c
> +++ b/net/netfilter/nft_exthdr.c
[...]
> +static int ipv4_find_option(struct net *net, struct sk_buff *skb,
> +			    unsigned int *offset, int target,
> +			    unsigned short *fragoff, int *flags)
> +{
[...]
> +	switch (target) {
> +	case IPOPT_SSRR:
> +	case IPOPT_LSRR:
[...]
> +	case IPOPT_RR:
[...]
> +	case IPOPT_RA:
[...]
> +	default:
> +		/* Either not supported or not a specific search, treated as
> +		 * found
> +		 */
> +		found = true;
> +		if (target >= 0) {
> +			target = -EOPNOTSUPP;
> +			break;
> +		}

Rules with this options will load fine:

ip option eol type 1
ip option noop type 1
ip option sec type 1
ip option timestamp type 1
ip option rr type 1
ip option sid type 1

However, they will not ever match I think.

found is set to true, but target is set to EOPNOTSUPP, then...

[...]
> +static void nft_exthdr_ipv4_eval(const struct nft_expr *expr,
> +				 struct nft_regs *regs,
> +				 const struct nft_pktinfo *pkt)
> +{
> +	struct nft_exthdr *priv = nft_expr_priv(expr);
> +	u32 *dest = &regs->data[priv->dreg];
> +	struct sk_buff *skb = pkt->skb;
> +	unsigned int offset;
> +	int err;
> +
> +	if (skb->protocol != htons(ETH_P_IP))
> +		goto err;
> +
> +	err = ipv4_find_option(nft_net(pkt), skb, &offset, priv->type, NULL, NULL);

... ipv4_find_option() returns -EOPNOTSUPP which says header does
not exist.

> +	if (priv->flags & NFT_EXTHDR_F_PRESENT) {
> +		*dest = (err >= 0);
> +		return;
> +	} else if (err < 0) {
> +		goto err;
> +	}

Re: [PATCH RESEND nf-next] netfilter: add support for matching IPv4 options

[Stephen Suryaputra]

On Wed, Jun 19, 2019 at 07:18:32PM +0200, Pablo Neira Ayuso wrote:
> 
> Rules with this options will load fine:
> 
> ip option eol type 1
> ip option noop type 1
> ip option sec type 1
> ip option timestamp type 1
> ip option rr type 1
> ip option sid type 1
> 
> However, they will not ever match I think.
> 
> found is set to true, but target is set to EOPNOTSUPP, then...
> 
> [...]
> > +	err = ipv4_find_option(nft_net(pkt), skb, &offset, priv->type, NULL, NULL);
> 
> ... ipv4_find_option() returns -EOPNOTSUPP which says header does
> not exist.
> 
Yes. My goal in writing this is mainly to block loose and/or strict
source routing. The system also will need to block RA and RR. Others are
not fully supported since we (my employer) don't need it. They can be
added later on if desired...

Re: [PATCH RESEND nf-next] netfilter: add support for matching IPv4 options

[Pablo Neira Ayuso]

On Wed, Jun 19, 2019 at 01:58:02PM -0400, Stephen Suryaputra wrote:
> On Wed, Jun 19, 2019 at 07:18:32PM +0200, Pablo Neira Ayuso wrote:
> > 
> > Rules with this options will load fine:
> > 
> > ip option eol type 1
> > ip option noop type 1
> > ip option sec type 1
> > ip option timestamp type 1
> > ip option rr type 1
> > ip option sid type 1
> > 
> > However, they will not ever match I think.
> > 
> > found is set to true, but target is set to EOPNOTSUPP, then...
> > 
> > [...]
> > > +	err = ipv4_find_option(nft_net(pkt), skb, &offset, priv->type, NULL, NULL);
> > 
> > ... ipv4_find_option() returns -EOPNOTSUPP which says header does
> > not exist.
> > 
> Yes. My goal in writing this is mainly to block loose and/or strict
> source routing. The system also will need to block RA and RR. Others are
> not fully supported since we (my employer) don't need it. They can be
> added later on if desired...

OK, that's fine. Then I'd suggest you remove support from eol, noop,
sec, timestamp and sid from the userspace patches.

Thanks!