[PATCH v2] netfilter: Parse ICMPv6 redirects

[PATCH v2] netfilter: Parse ICMPv6 redirects

[Alin Nastac]

Extract IPv6 packet that triggered the sending of redirect message from
ICMPv6 Redirected Header option and check if conntrack table contain such
connection. Mark redirect packet as RELATED if a matching connection is found.

Signed-off-by: Alin Nastac <alin.nastac@gmail.com>
---
 net/ipv6/netfilter/nf_conntrack_proto_icmpv6.c | 87 ++++++++++++++++++++++++--
 1 file changed, 82 insertions(+), 5 deletions(-)

diff --git a/net/ipv6/netfilter/nf_conntrack_proto_icmpv6.c b/net/ipv6/netfilter/nf_conntrack_proto_icmpv6.c
index d2c2ccb..f7bae5c 100644
--- a/net/ipv6/netfilter/nf_conntrack_proto_icmpv6.c
+++ b/net/ipv6/netfilter/nf_conntrack_proto_icmpv6.c
@@ -144,7 +144,7 @@ static bool icmpv6_new(struct nf_conn *ct, const struct sk_buff *skb,
 static int
 icmpv6_error_message(struct net *net, struct nf_conn *tmpl,
 		     struct sk_buff *skb,
-		     unsigned int icmp6off,
+		     unsigned int inneripv6off,
 		     unsigned int hooknum)
 {
 	struct nf_conntrack_tuple intuple, origtuple;
@@ -157,9 +157,7 @@ static bool icmpv6_new(struct nf_conn *ct, const struct sk_buff *skb,
 
 	/* Are they talking about one of our connections? */
 	if (!nf_ct_get_tuplepr(skb,
-			       skb_network_offset(skb)
-				+ sizeof(struct ipv6hdr)
-				+ sizeof(struct icmp6hdr),
+			       inneripv6off,
 			       PF_INET6, net, &origtuple)) {
 		pr_debug("icmpv6_error: Can't get tuple\n");
 		return -NF_ACCEPT;
@@ -194,6 +192,82 @@ static bool icmpv6_new(struct nf_conn *ct, const struct sk_buff *skb,
 }
 
 static int
+icmpv6_redirect_message(struct net *net, struct nf_conn *tmpl,
+		     struct sk_buff *skb,
+		     unsigned int dataoff,
+		     unsigned int hooknum)
+{
+	const struct in6_addr *dst;
+	struct in6_addr _dst;
+	const struct nd_opt_hdr *opt;
+	struct nd_opt_hdr _opt;
+	const struct ipv6hdr *iph;
+	struct ipv6hdr _iph;
+
+	/* skip target address */
+	dataoff += sizeof(_dst);
+
+	/* read destination address */
+	dst = skb_header_pointer(skb, dataoff, sizeof(_dst), &_dst);
+	if (dst == NULL) {
+		if (LOG_INVALID(net, IPPROTO_ICMPV6))
+			nf_log_packet(net, PF_INET6, 0, skb, NULL, NULL, NULL,
+			      "nf_ct_icmpv6: short redirect packet ");
+		return -NF_ACCEPT;
+	}
+	if (ipv6_addr_is_multicast(dst)) {
+		if (LOG_INVALID(net, IPPROTO_ICMPV6))
+			nf_log_packet(net, PF_INET6, 0, skb, NULL, NULL, NULL,
+			      "nf_ct_icmpv6: redirect destination address is multicast ");
+		return -NF_ACCEPT;
+	}
+	dataoff += sizeof(_dst);
+
+	/* find redirected header */
+	while (1) {
+		opt = skb_header_pointer(skb, dataoff, sizeof(_opt), &_opt);
+		if (opt == NULL) {
+			if (LOG_INVALID(net, IPPROTO_ICMPV6))
+				nf_log_packet(net, PF_INET6, 0, skb, NULL, NULL, NULL,
+				      "nf_ct_icmpv6: invalid redirect option ");
+			return -NF_ACCEPT;
+		}
+		if (opt->nd_opt_len == 0) {
+			if (LOG_INVALID(net, IPPROTO_ICMPV6))
+				nf_log_packet(net, PF_INET6, 0, skb, NULL, NULL, NULL,
+				      "nf_ct_icmpv6: invalid redirect option length ");
+			return -NF_ACCEPT;
+		}
+
+		if (opt->nd_opt_type == ND_OPT_REDIRECT_HDR) {
+			dataoff += 8;
+			break;
+		}
+
+		dataoff += opt->nd_opt_len << 3;
+	}
+
+	/* read redirect header */
+	iph = skb_header_pointer(skb, dataoff, sizeof(_iph), &_iph);
+	if (iph == NULL) {
+		if (LOG_INVALID(net, IPPROTO_ICMPV6))
+			nf_log_packet(net, PF_INET6, 0, skb, NULL, NULL, NULL,
+			      "nf_ct_icmpv6: short redirect header ");
+		return -NF_ACCEPT;
+	}
+
+	/* validate destination address */
+	if (!ipv6_addr_equal(&iph->daddr, dst)) {
+		if (LOG_INVALID(net, IPPROTO_ICMPV6))
+			nf_log_packet(net, PF_INET6, 0, skb, NULL, NULL, NULL,
+			      "nf_ct_icmpv6: redirect destination address not matching destination address of redirect header ");
+		return -NF_ACCEPT;
+	}
+	
+	return icmpv6_error_message(net, tmpl, skb, dataoff, hooknum);
+}
+
+static int
 icmpv6_error(struct net *net, struct nf_conn *tmpl,
 	     struct sk_buff *skb, unsigned int dataoff,
 	     u8 pf, unsigned int hooknum)
@@ -225,9 +299,12 @@ static bool icmpv6_new(struct nf_conn *ct, const struct sk_buff *skb,
 		nf_conntrack_get(skb_nfct(skb));
 		return NF_ACCEPT;
 	}
+	dataoff += sizeof(struct icmp6hdr);
 
 	/* is not error message ? */
-	if (icmp6h->icmp6_type >= 128)
+	if (icmp6h->icmp6_type == NDISC_REDIRECT)
+		return icmpv6_redirect_message(net, tmpl, skb, dataoff, hooknum);
+	else if (icmp6h->icmp6_type >= 128)
 		return NF_ACCEPT;
 
 	return icmpv6_error_message(net, tmpl, skb, dataoff, hooknum);
-- 
1.7.12.4

Re: [PATCH v2] netfilter: Parse ICMPv6 redirects

[Pablo Neira Ayuso]

Hi Alin,

On Tue, Mar 07, 2017 at 11:00:43AM +0100, Alin Nastac wrote:
> Extract IPv6 packet that triggered the sending of redirect message from
> ICMPv6 Redirected Header option and check if conntrack table contain such
> connection. Mark redirect packet as RELATED if a matching connection is found.

I think we need a sysctl to enable this on demand, eg.
'nf_conntrack_icmpv6_accept_redirects'

This is changing the default behaviour, my main concern here is that
filtering policies not accepting redirects will now make it via
RELATED.

I would prefer the user explicitly requests this.

Let me know, thanks!

Re: [PATCH v2] netfilter: Parse ICMPv6 redirects

[Alin Năstac]

Hi Pablo,

On Mon, Mar 13, 2017 at 1:40 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Tue, Mar 07, 2017 at 11:00:43AM +0100, Alin Nastac wrote:
>> Extract IPv6 packet that triggered the sending of redirect message from
>> ICMPv6 Redirected Header option and check if conntrack table contain such
>> connection. Mark redirect packet as RELATED if a matching connection is found.
>
> I think we need a sysctl to enable this on demand, eg.
> 'nf_conntrack_icmpv6_accept_redirects'
>
> This is changing the default behaviour, my main concern here is that
> filtering policies not accepting redirects will now make it via
> RELATED.

net/ipv4/netfilter/nf_conntrack_proto_icmp.c  give RELATED status to
all ICMP redirect messages that refer to valid conntracks. Why would
ICMPv6 redirect case be any different? Would you implement a similar
sysctl switch for ICMP redirect RELATED state? And if you do, would
you accept to enable these switches by default?

IMHO ICMPv6 redirects were not included in the original
nf_conntrack_proto_icmpv6.c due to relative complexity of their
format.

Re: [PATCH v2] netfilter: Parse ICMPv6 redirects

[Pablo Neira Ayuso]

On Mon, Mar 13, 2017 at 02:17:39PM +0100, Alin Năstac wrote:
> Hi Pablo,
> 
> On Mon, Mar 13, 2017 at 1:40 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Tue, Mar 07, 2017 at 11:00:43AM +0100, Alin Nastac wrote:
> >> Extract IPv6 packet that triggered the sending of redirect message from
> >> ICMPv6 Redirected Header option and check if conntrack table contain such
> >> connection. Mark redirect packet as RELATED if a matching connection is found.
> >
> > I think we need a sysctl to enable this on demand, eg.
> > 'nf_conntrack_icmpv6_accept_redirects'
> >
> > This is changing the default behaviour, my main concern here is that
> > filtering policies not accepting redirects will now make it via
> > RELATED.
> 
> net/ipv4/netfilter/nf_conntrack_proto_icmp.c  give RELATED status to
> all ICMP redirect messages that refer to valid conntracks. Why would
> ICMPv6 redirect case be any different?

That's very valid argument, but we have this asymmetry for long time
ago, basically since the beginning. As I said, I have concerns on
changing this default behaviour without an explicit knob. This
behaviour change will go through inadvertently for many people.

> Would you implement a similar sysctl switch for ICMP redirect
> RELATED state? And if you do, would you accept to enable these
> switches by default?

I don't think we shouldn't enable this by default. We have tried to be
conservative on that side so far. Is it a problem there to enable this
via sysctl.conf?

Am I missing any requirement there that is not described in your patch
description?

Thanks.

Re: [PATCH v2] netfilter: Parse ICMPv6 redirects

[Alin Năstac]

On Mon, Mar 13, 2017 at 2:44 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Mon, Mar 13, 2017 at 02:17:39PM +0100, Alin Năstac wrote:
>> Hi Pablo,
>>
>> On Mon, Mar 13, 2017 at 1:40 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> > On Tue, Mar 07, 2017 at 11:00:43AM +0100, Alin Nastac wrote:
>> >> Extract IPv6 packet that triggered the sending of redirect message from
>> >> ICMPv6 Redirected Header option and check if conntrack table contain such
>> >> connection. Mark redirect packet as RELATED if a matching connection is found.
>> >
>> > I think we need a sysctl to enable this on demand, eg.
>> > 'nf_conntrack_icmpv6_accept_redirects'
>> >
>> > This is changing the default behaviour, my main concern here is that
>> > filtering policies not accepting redirects will now make it via
>> > RELATED.
>>
>> net/ipv4/netfilter/nf_conntrack_proto_icmp.c  give RELATED status to
>> all ICMP redirect messages that refer to valid conntracks. Why would
>> ICMPv6 redirect case be any different?
>
> That's very valid argument, but we have this asymmetry for long time
> ago, basically since the beginning. As I said, I have concerns on
> changing this default behaviour without an explicit knob. This
> behaviour change will go through inadvertently for many people.

People should not rely on buggy behaviour to keep them safe. Imagine
for instance there is a bug that prevents packets sent by HTTP servers
to match "-m conntrack --state ESTABLISHED" rules. Would you add a fix
that is operational only when an obscure procfs knob gets enabled?

Redirects are supposed to be sent to on-link hosts, so all we want in
fact is to allow these packets on INPUT. Would it be OK to restrict
RELATED status to redirects originated from link-local addresses? This
will be in line with RFC 4861 requirement that source address of
ICMPv6 redirects must be in link-local scope.

>> Would you implement a similar sysctl switch for ICMP redirect
>> RELATED state? And if you do, would you accept to enable these
>> switches by default?
>
> I don't think we shouldn't enable this by default. We have tried to be
> conservative on that side so far. Is it a problem there to enable this
> via sysctl.conf?

Nothing except that netfilter will still fail to find ICMPv6 redirects
as RELATED unless you know where to look. Those who want to allow
redirects will likely allow them explicitly rather than take the
trouble to find the switch in the procfs.

Re: [PATCH v2] netfilter: Parse ICMPv6 redirects

[Pablo Neira Ayuso]

On Mon, Mar 13, 2017 at 03:17:22PM +0100, Alin Năstac wrote:
> On Mon, Mar 13, 2017 at 2:44 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Mon, Mar 13, 2017 at 02:17:39PM +0100, Alin Năstac wrote:
> >> Hi Pablo,
> >>
> >> On Mon, Mar 13, 2017 at 1:40 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >> > On Tue, Mar 07, 2017 at 11:00:43AM +0100, Alin Nastac wrote:
> >> >> Extract IPv6 packet that triggered the sending of redirect message from
> >> >> ICMPv6 Redirected Header option and check if conntrack table contain such
> >> >> connection. Mark redirect packet as RELATED if a matching connection is found.
> >> >
> >> > I think we need a sysctl to enable this on demand, eg.
> >> > 'nf_conntrack_icmpv6_accept_redirects'
> >> >
> >> > This is changing the default behaviour, my main concern here is that
> >> > filtering policies not accepting redirects will now make it via
> >> > RELATED.
> >>
> >> net/ipv4/netfilter/nf_conntrack_proto_icmp.c  give RELATED status to
> >> all ICMP redirect messages that refer to valid conntracks. Why would
> >> ICMPv6 redirect case be any different?
> >
> > That's very valid argument, but we have this asymmetry for long time
> > ago, basically since the beginning. As I said, I have concerns on
> > changing this default behaviour without an explicit knob. This
> > behaviour change will go through inadvertently for many people.
> 
> People should not rely on buggy behaviour to keep them safe. Imagine
> for instance there is a bug that prevents packets sent by HTTP servers
> to match "-m conntrack --state ESTABLISHED" rules. Would you add a fix
> that is operational only when an obscure procfs knob gets enabled?

Come on, this behaviour has been there for more than 10 years...

> Redirects are supposed to be sent to on-link hosts, so all we want in
> fact is to allow these packets on INPUT. Would it be OK to restrict
> RELATED status to redirects originated from link-local addresses? This
> will be in line with RFC 4861 requirement that source address of
> ICMPv6 redirects must be in link-local scope.

I think restricting this to link-local, if possible, would be fine.

> >> Would you implement a similar sysctl switch for ICMP redirect
> >> RELATED state? And if you do, would you accept to enable these
> >> switches by default?
> >
> > I don't think we shouldn't enable this by default. We have tried to be
> > conservative on that side so far. Is it a problem there to enable this
> > via sysctl.conf?
> 
> Nothing except that netfilter will still fail to find ICMPv6 redirects
> as RELATED unless you know where to look. Those who want to allow
> redirects will likely allow them explicitly rather than take the
> trouble to find the switch in the procfs.

We can play games on predicting what people will do.

However, fact is that we have not handled icmpv6 nd-redirect as
RELATED for more than 10 years, so I think this behaviour qualifies as
feature not as bug ;).

Look at this from a different angle: User A upgrades its old kernel
with stateful ip6tables ruleset, things will start working in a
different way with no prior advice. That is not good.

A patch for iptables manpage would be good too to document this.

Re: [PATCH v2] netfilter: Parse ICMPv6 redirects

[Alin Năstac]

On Mon, Mar 13, 2017 at 6:00 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Mon, Mar 13, 2017 at 03:17:22PM +0100, Alin Năstac wrote:
>> On Mon, Mar 13, 2017 at 2:44 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> > On Mon, Mar 13, 2017 at 02:17:39PM +0100, Alin Năstac wrote:
>> >> Hi Pablo,
>> >>
>> >> On Mon, Mar 13, 2017 at 1:40 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> >> > On Tue, Mar 07, 2017 at 11:00:43AM +0100, Alin Nastac wrote:
>> >> >> Extract IPv6 packet that triggered the sending of redirect message from
>> >> >> ICMPv6 Redirected Header option and check if conntrack table contain such
>> >> >> connection. Mark redirect packet as RELATED if a matching connection is found.
>> >> >
>> >> > I think we need a sysctl to enable this on demand, eg.
>> >> > 'nf_conntrack_icmpv6_accept_redirects'
>> >> >
>> >> > This is changing the default behaviour, my main concern here is that
>> >> > filtering policies not accepting redirects will now make it via
>> >> > RELATED.
>> >>
>> >> net/ipv4/netfilter/nf_conntrack_proto_icmp.c  give RELATED status to
>> >> all ICMP redirect messages that refer to valid conntracks. Why would
>> >> ICMPv6 redirect case be any different?
>> >
>> > That's very valid argument, but we have this asymmetry for long time
>> > ago, basically since the beginning. As I said, I have concerns on
>> > changing this default behaviour without an explicit knob. This
>> > behaviour change will go through inadvertently for many people.
>>
>> People should not rely on buggy behaviour to keep them safe. Imagine
>> for instance there is a bug that prevents packets sent by HTTP servers
>> to match "-m conntrack --state ESTABLISHED" rules. Would you add a fix
>> that is operational only when an obscure procfs knob gets enabled?
>
> Come on, this behaviour has been there for more than 10 years...
>
>> Redirects are supposed to be sent to on-link hosts, so all we want in
>> fact is to allow these packets on INPUT. Would it be OK to restrict
>> RELATED status to redirects originated from link-local addresses? This
>> will be in line with RFC 4861 requirement that source address of
>> ICMPv6 redirects must be in link-local scope.
>
> I think restricting this to link-local, if possible, would be fine.

I take it would be fine but still not enough. :-(

Fine, drop this patch then. Better allow ICMPv6 redirects through an
ip6tables INPUT rule placed in front of the usual
   -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT
than having to patch the kernel and use sysctl to alter default behaviour.

I only wish you told me earlier not to waste time on fixing a bug that
no one is interested to take it in.