[PATCH net-next v4 0/3] path mtu hardening patches

[PATCH net-next v4 0/3] path mtu hardening patches

[hannes]

Hi all!

After a lot of back and forth I want to propose these changes regarding
path mtu hardening and give an outline why I think this is the best way
how to proceed:

This set contains the following patches:
* ipv4: introduce ip_dst_mtu_maybe_forward and protect forwarding path against pmtu spoofing
* ipv6: introduce ip6_dst_mtu_forward and protect forwarding path with it
* ipv4: introduce hardened ip_no_pmtu_disc mode

The first one switches the forwarding path of IPv4 to use the interface
mtu by default and ignore a possible discovered path mtu. It provides
a sysctl to switch back to the original behavior (see discussion below).

The second patch does the same thing unconditionally for IPv6. I don't
provide a knob for IPv6 to switch to original behavior (please see
below).

The third patch introduces a hardened pmtu mode, where only pmtu
information are accepted where the protocol is able to do more stringent
checks on the icmp piggyback payload (please see the patch commit msg
for further details).

Why is this change necessary?

First of all, RFC 1191 4. Router specification says:
"When a router is unable to forward a datagram because it exceeds the
 MTU of the next-hop network and its Don't Fragment bit is set, the
 router is required to return an ICMP Destination Unreachable message
 to the source of the datagram, with the Code indicating
 "fragmentation needed and DF set". ..."

For some time now fragmentation has been considered problematic, e.g.:
* http://www.hpl.hp.com/techreports/Compaq-DEC/WRL-87-3.pdf
* http://tools.ietf.org/search/rfc4963

Most of them seem to agree that fragmentation should be avoided because
of efficiency, data corruption or security concerns.

Recently it was shown possible that correctly guessing IP ids could lead
to data injection on DNS packets:
<https://sites.google.com/site/hayashulman/files/fragmentation-poisoning.pdf>

While we can try to completly stop fragmentation on the end host
(this is e.g. implemented via IP_PMTUDISC_INTERFACE), we cannot stop
fragmentation completly on the forwarding path. On the end host the
application has to deal with MTUs and has to choose fallback methods
if fragmentation could be an attack vector. This is already the case for
most DNS software, where a maximum UDP packet size can be configured. But
until recently they had no control over local fragmentation and could
thus emit fragmented packets.

On the forwarding path we can just try to delay the fragmentation to
the last hop where this is really necessary. Current kernel already does
that but only because routers don't receive feedback of path mtus, these are
only send back to the end host system. But it is possible to maliciously
insert path mtu inforamtion via ICMP packets which have an icmp echo_reply
payload, because we cannot validate those notifications against local
sockets. DHCP clients which establish an any-bound RAW-socket could also
start processing unwanted fragmentation-needed packets.

Why does IPv4 has a knob to revert to old behavior while IPv6 doesn't?

IPv4 does fragmentation on the path while IPv6 does always respond with
packet-too-big errors. The interface MTU will always be greater than
the path MTU information. So we would discard packets we could actually
forward because of malicious information. After this change we would
let the hop, which really could not forward the packet, notify the host
of this problem.

IPv4 allowes fragmentation mid-path. In case someone does use a software
which tries to discover such paths and assumes that the kernel is handling
the discovered pmtu information automatically. This should be an extremly
rare case, but because I could not exclude the possibility this knob is
provided. Also this software could insert non-locked mtu information
into the kernel. We cannot distinguish that from path mtu information
currently. Premature fragmentation could solve some problems in wrongly
configured networks, thus this switch is provided.

One frag-needed packet could reduce the path mtu down to 522 bytes
(route/min_pmtu).

Misc:

IPv6 neighbor discovery could advertise mtu information for an
interface. These information update the ipv6-specific interface mtu and
thus get used by the forwarding path.

Tunnel and xfrm output path will still honour path mtu and also respond
with Packet-too-Big or fragmentation-needed errors if needed.

Changelog for all patches:
# ipv4: introduce ip_dst_mtu_maybe_forward and protect forwarding path against pmtu spoofing
v2)
* enabled ip_forward_use_pmtu by default
* reworded
v3)
* disabled ip_forward_use_pmtu by default
* reworded
v4)
* renamed ip_dst_mtu_secure to ip_dst_mtu_maybe_forward
* updated changelog accordingly
* removed unneeded !!(... & ...) double negations

# ipv6: introduce ip6_dst_mtu_forward and protect forwarding path with it
v2)
* by default we honour pmtu information
3)
* only honor interface mtu
* rewritten and simplified
* no knob to fall back to old mode any more

# ipv4: introduce hardened ip_no_pmtu_disc mode
v2)
* reworded Documentation

Diffstat:
 Documentation/networking/ip-sysctl.txt | 26 +++++++++++++++++++++++++-
 include/net/ip.h                       | 33 +++++++++++++++++++++++++++++++++
 include/net/netns/ipv4.h               |  1 +
 include/net/protocol.h                 |  7 ++++++-
 include/net/route.h                    | 19 +++----------------
 net/dccp/ipv4.c                        |  1 +
 net/ipv4/af_inet.c                     |  1 +
 net/ipv4/icmp.c                        | 28 ++++++++++++++++++++++++----
 net/ipv4/ip_forward.c                  |  7 +++++--
 net/ipv4/ip_output.c                   |  8 +++++---
 net/ipv4/route.c                       |  3 ---
 net/ipv4/sysctl_net_ipv4.c             |  7 +++++++
 net/ipv6/ip6_output.c                  | 23 ++++++++++++++++++++++-
 net/sctp/protocol.c                    |  1 +
 14 files changed, 134 insertions(+), 31 deletions(-)

Thanks,

  Hannes

[PATCH net-next v4 1/3] ipv4: introduce ip_dst_mtu_maybe_forward and protect forwarding path against pmtu spoofing

[hannes]

From: Hannes Frederic Sowa <hannes@stressinduktion.org>

While forwarding we should not use the protocol path mtu to calculate
the mtu for a forwarded packet but instead use the interface mtu.

We mark forwarded skbs in ip_forward with IPSKB_FORWARDED, which was
introduced for multicast forwarding. But as it does not conflict with
our usage in unicast code path it is perfect for reuse.

I moved the functions ip_sk_accept_pmtu, ip_sk_use_pmtu and ip_skb_dst_mtu
along with the new ip_dst_mtu_maybe_forward to net/ip.h to fix circular
dependencies because of IPSKB_FORWARDED.

Because someone might have written a software which does probe
destinations manually and expects the kernel to honour those path mtus
I introduced a new per-namespace "ip_forward_use_pmtu" knob so someone
can disable this new behaviour. We also still use mtus which are locked on a
route for forwarding.

The reason for this change is, that path mtus information can be injected
into the kernel via e.g. icmp_err protocol handler without verification
of local sockets. As such, this could cause the IPv4 forwarding path to
wrongfully emit fragmentation needed notifications or start to fragment
packets along a path.

Tunnel and ipsec output paths clear IPCB again, thus IPSKB_FORWARDED
won't be set and further fragmentation logic will use the path mtu to
determine the fragmentation size. They also recheck packet size with
help of path mtu discovery and report appropriate errors.

Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: David Miller <davem@davemloft.net>
Cc: John Heffner <johnwheffner@gmail.com>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 Documentation/networking/ip-sysctl.txt | 13 +++++++++++++
 include/net/ip.h                       | 33 +++++++++++++++++++++++++++++++++
 include/net/netns/ipv4.h               |  1 +
 include/net/route.h                    | 19 +++----------------
 net/ipv4/ip_forward.c                  |  7 +++++--
 net/ipv4/ip_output.c                   |  8 +++++---
 net/ipv4/route.c                       |  3 ---
 net/ipv4/sysctl_net_ipv4.c             |  7 +++++++
 8 files changed, 67 insertions(+), 24 deletions(-)

diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
index 7373115..0d71fa9 100644
--- a/Documentation/networking/ip-sysctl.txt
+++ b/Documentation/networking/ip-sysctl.txt
@@ -32,6 +32,19 @@ ip_no_pmtu_disc - INTEGER
 min_pmtu - INTEGER
 	default 552 - minimum discovered Path MTU
 
+ip_forward_use_pmtu - BOOLEAN
+	By default we don't trust protocol path MTUs while forwarding
+	because they could be easily forged and can lead to unwanted
+	fragmentation by the router.
+	You only need to enable this if you have user-space software
+	which tries to discover path mtus by itself and depends on the
+	kernel honoring this information. This is normally not the
+	case.
+	Default: 0 (disabled)
+	Possible values:
+	0 - disabled
+	1 - enabled
+
 route/max_size - INTEGER
 	Maximum number of routes allowed in the kernel.  Increase
 	this when using large numbers of interfaces and/or routes.
diff --git a/include/net/ip.h b/include/net/ip.h
index 5356644..0dab95c 100644
--- a/include/net/ip.h
+++ b/include/net/ip.h
@@ -263,6 +263,39 @@ int ip_dont_fragment(struct sock *sk, struct dst_entry *dst)
 		 !(dst_metric_locked(dst, RTAX_MTU)));
 }
 
+static inline bool ip_sk_accept_pmtu(const struct sock *sk)
+{
+	return inet_sk(sk)->pmtudisc != IP_PMTUDISC_INTERFACE;
+}
+
+static inline bool ip_sk_use_pmtu(const struct sock *sk)
+{
+	return inet_sk(sk)->pmtudisc < IP_PMTUDISC_PROBE;
+}
+
+static inline unsigned int ip_dst_mtu_maybe_forward(const struct dst_entry *dst,
+						    bool forwarding)
+{
+	struct net *net = dev_net(dst->dev);
+
+	if (net->ipv4.sysctl_ip_fwd_use_pmtu ||
+	    dst_metric_locked(dst, RTAX_MTU) ||
+	    !forwarding)
+		return dst_mtu(dst);
+
+	return min(dst->dev->mtu, IP_MAX_MTU);
+}
+
+static inline unsigned int ip_skb_dst_mtu(const struct sk_buff *skb)
+{
+	if (!skb->sk || ip_sk_use_pmtu(skb->sk)) {
+		bool forwarding = IPCB(skb)->flags & IPSKB_FORWARDED;
+		return ip_dst_mtu_maybe_forward(skb_dst(skb), forwarding);
+	} else {
+		return min(skb_dst(skb)->dev->mtu, IP_MAX_MTU);
+	}
+}
+
 void __ip_select_ident(struct iphdr *iph, struct dst_entry *dst, int more);
 
 static inline void ip_select_ident(struct sk_buff *skb, struct dst_entry *dst, struct sock *sk)
diff --git a/include/net/netns/ipv4.h b/include/net/netns/ipv4.h
index 929a668..80f500a 100644
--- a/include/net/netns/ipv4.h
+++ b/include/net/netns/ipv4.h
@@ -70,6 +70,7 @@ struct netns_ipv4 {
 
 	int sysctl_tcp_ecn;
 	int sysctl_ip_no_pmtu_disc;
+	int sysctl_ip_fwd_use_pmtu;
 
 	kgid_t sysctl_ping_group_range[2];
 
diff --git a/include/net/route.h b/include/net/route.h
index 638e3eb..9d1f423 100644
--- a/include/net/route.h
+++ b/include/net/route.h
@@ -36,6 +36,9 @@
 #include <linux/cache.h>
 #include <linux/security.h>
 
+/* IPv4 datagram length is stored into 16bit field (tot_len) */
+#define IP_MAX_MTU	0xFFFFU
+
 #define RTO_ONLINK	0x01
 
 #define RT_CONN_FLAGS(sk)   (RT_TOS(inet_sk(sk)->tos) | sock_flag(sk, SOCK_LOCALROUTE))
@@ -311,20 +314,4 @@ static inline int ip4_dst_hoplimit(const struct dst_entry *dst)
 	return hoplimit;
 }
 
-static inline bool ip_sk_accept_pmtu(const struct sock *sk)
-{
-	return inet_sk(sk)->pmtudisc != IP_PMTUDISC_INTERFACE;
-}
-
-static inline bool ip_sk_use_pmtu(const struct sock *sk)
-{
-	return inet_sk(sk)->pmtudisc < IP_PMTUDISC_PROBE;
-}
-
-static inline int ip_skb_dst_mtu(const struct sk_buff *skb)
-{
-	return (!skb->sk || ip_sk_use_pmtu(skb->sk)) ?
-	       dst_mtu(skb_dst(skb)) : skb_dst(skb)->dev->mtu;
-}
-
 #endif	/* _ROUTE_H */
diff --git a/net/ipv4/ip_forward.c b/net/ipv4/ip_forward.c
index 694de3b..e9f1217 100644
--- a/net/ipv4/ip_forward.c
+++ b/net/ipv4/ip_forward.c
@@ -54,6 +54,7 @@ static int ip_forward_finish(struct sk_buff *skb)
 
 int ip_forward(struct sk_buff *skb)
 {
+	u32 mtu;
 	struct iphdr *iph;	/* Our header */
 	struct rtable *rt;	/* Route we use */
 	struct ip_options *opt	= &(IPCB(skb)->opt);
@@ -88,11 +89,13 @@ int ip_forward(struct sk_buff *skb)
 	if (opt->is_strictroute && rt->rt_uses_gateway)
 		goto sr_failed;
 
-	if (unlikely(skb->len > dst_mtu(&rt->dst) && !skb_is_gso(skb) &&
+	IPCB(skb)->flags |= IPSKB_FORWARDED;
+	mtu = ip_dst_mtu_maybe_forward(&rt->dst, true);
+	if (unlikely(skb->len > mtu && !skb_is_gso(skb) &&
 		     (ip_hdr(skb)->frag_off & htons(IP_DF))) && !skb->local_df) {
 		IP_INC_STATS(dev_net(rt->dst.dev), IPSTATS_MIB_FRAGFAILS);
 		icmp_send(skb, ICMP_DEST_UNREACH, ICMP_FRAG_NEEDED,
-			  htonl(dst_mtu(&rt->dst)));
+			  htonl(mtu));
 		goto drop;
 	}
 
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index df18461..9a78804 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -449,6 +449,7 @@ int ip_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *))
 	__be16 not_last_frag;
 	struct rtable *rt = skb_rtable(skb);
 	int err = 0;
+	bool forwarding = IPCB(skb)->flags & IPSKB_FORWARDED;
 
 	dev = rt->dst.dev;
 
@@ -458,12 +459,13 @@ int ip_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *))
 
 	iph = ip_hdr(skb);
 
+	mtu = ip_dst_mtu_maybe_forward(&rt->dst, forwarding);
 	if (unlikely(((iph->frag_off & htons(IP_DF)) && !skb->local_df) ||
 		     (IPCB(skb)->frag_max_size &&
-		      IPCB(skb)->frag_max_size > dst_mtu(&rt->dst)))) {
+		      IPCB(skb)->frag_max_size > mtu))) {
 		IP_INC_STATS(dev_net(dev), IPSTATS_MIB_FRAGFAILS);
 		icmp_send(skb, ICMP_DEST_UNREACH, ICMP_FRAG_NEEDED,
-			  htonl(ip_skb_dst_mtu(skb)));
+			  htonl(mtu));
 		kfree_skb(skb);
 		return -EMSGSIZE;
 	}
@@ -473,7 +475,7 @@ int ip_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *))
 	 */
 
 	hlen = iph->ihl * 4;
-	mtu = dst_mtu(&rt->dst) - hlen;	/* Size of data space */
+	mtu = mtu - hlen;	/* Size of data space */
 #ifdef CONFIG_BRIDGE_NETFILTER
 	if (skb->nf_bridge)
 		mtu -= nf_bridge_mtu_reduction(skb);
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index f8da282..25071b4 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -112,9 +112,6 @@
 #define RT_FL_TOS(oldflp4) \
 	((oldflp4)->flowi4_tos & (IPTOS_RT_MASK | RTO_ONLINK))
 
-/* IPv4 datagram length is stored into 16bit field (tot_len) */
-#define IP_MAX_MTU	0xFFFF
-
 #define RT_GC_TIMEOUT (300*HZ)
 
 static int ip_rt_max_size;
diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
index 1d2480a..44eba05 100644
--- a/net/ipv4/sysctl_net_ipv4.c
+++ b/net/ipv4/sysctl_net_ipv4.c
@@ -831,6 +831,13 @@ static struct ctl_table ipv4_net_table[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec
 	},
+	{
+		.procname	= "ip_forward_use_pmtu",
+		.data		= &init_net.ipv4.sysctl_ip_fwd_use_pmtu,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec,
+	},
 	{ }
 };
 
-- 
1.8.4.2

[PATCH net-next v3 2/3] ipv6: introduce ip6_dst_mtu_forward and protect forwarding path with it

[hannes]

From: Hannes Frederic Sowa <hannes@stressinduktion.org>

In the IPv6 forwarding path we are only concerend about the outgoing
interface MTU, but also respect locked MTUs on routes. Tunnel provider
or IPSEC already have to recheck and if needed send PtB notifications
to the sending host in case the data does not fit into the packet with
added headers (we only know the final header sizes there, while also
using path MTU information).

The reason for this change is, that path MTU information can be injected
into the kernel via e.g. icmp_err protocol handler without verification
of local sockets. As such, this could cause the IPv6 forwarding path to
wrongfully emit Packet-too-Big errors and drop IPv6 packets.

Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: David Miller <davem@davemloft.net>
Cc: John Heffner <johnwheffner@gmail.com>
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 net/ipv6/ip6_output.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index d1de956..ef02b26 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -321,6 +321,27 @@ static inline int ip6_forward_finish(struct sk_buff *skb)
 	return dst_output(skb);
 }
 
+static unsigned int ip6_dst_mtu_forward(const struct dst_entry *dst)
+{
+	unsigned int mtu;
+	struct inet6_dev *idev;
+
+	if (dst_metric_locked(dst, RTAX_MTU)) {
+		mtu = dst_metric_raw(dst, RTAX_MTU);
+		if (mtu)
+			return mtu;
+	}
+
+	mtu = IPV6_MIN_MTU;
+	rcu_read_lock();
+	idev = __in6_dev_get(dst->dev);
+	if (idev)
+		mtu = idev->cnf.mtu6;
+	rcu_read_unlock();
+
+	return mtu;
+}
+
 int ip6_forward(struct sk_buff *skb)
 {
 	struct dst_entry *dst = skb_dst(skb);
@@ -441,7 +462,7 @@ int ip6_forward(struct sk_buff *skb)
 		}
 	}
 
-	mtu = dst_mtu(dst);
+	mtu = ip6_dst_mtu_forward(dst);
 	if (mtu < IPV6_MIN_MTU)
 		mtu = IPV6_MIN_MTU;
 
-- 
1.8.4.2

[PATCH net-next v2 3/3] ipv4: introduce hardened ip_no_pmtu_disc mode

[hannes]

From: Hannes Frederic Sowa <hannes@stressinduktion.org>

This new ip_no_pmtu_disc mode only allowes fragmentation-needed errors
to be honored by protocols which do more stringent validation on the
ICMP's packet payload. This knob is useful for people who e.g. want to
run an unmodified DNS server in a namespace where they need to use pmtu
for TCP connections (as they are used for zone transfers or fallback
for requests) but don't want to use possibly spoofed UDP pmtu information.

Currently the whitelisted protocols are TCP, SCTP and DCCP as they check
if the returned packet is in the window or if the association is valid.

Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: David Miller <davem@davemloft.net>
Cc: John Heffner <johnwheffner@gmail.com>
Suggested-by: Florian Weimer <fweimer@redhat.com>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 Documentation/networking/ip-sysctl.txt | 13 ++++++++++++-
 include/net/protocol.h                 |  7 ++++++-
 net/dccp/ipv4.c                        |  1 +
 net/ipv4/af_inet.c                     |  1 +
 net/ipv4/icmp.c                        | 28 ++++++++++++++++++++++++----
 net/sctp/protocol.c                    |  1 +
 6 files changed, 45 insertions(+), 6 deletions(-)

diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
index 0d71fa9..c97932c 100644
--- a/Documentation/networking/ip-sysctl.txt
+++ b/Documentation/networking/ip-sysctl.txt
@@ -26,7 +26,18 @@ ip_no_pmtu_disc - INTEGER
 	discarded. Outgoing frames are handled the same as in mode 1,
 	implicitly setting IP_PMTUDISC_DONT on every created socket.
 
-	Possible values: 0-2
+	Mode 3 is a hardend pmtu discover mode. The kernel will only
+	accept fragmentation-needed errors if the underlying protocol
+	can verify them besides a plain socket lookup. Current
+	protocols for which pmtu events will be honored are TCP, SCTP
+	and DCCP as they verify e.g. the sequence number or the
+	association. This mode should not be enabled globally but is
+	only intended to secure e.g. name servers in namespaces where
+	TCP path mtu must still work but path MTU information of other
+	protocols should be discarded. If enabled globally this mode
+	could break other protocols.
+
+	Possible values: 0-3
 	Default: FALSE
 
 min_pmtu - INTEGER
diff --git a/include/net/protocol.h b/include/net/protocol.h
index fbf7676..0e5f866 100644
--- a/include/net/protocol.h
+++ b/include/net/protocol.h
@@ -43,7 +43,12 @@ struct net_protocol {
 	int			(*handler)(struct sk_buff *skb);
 	void			(*err_handler)(struct sk_buff *skb, u32 info);
 	unsigned int		no_policy:1,
-				netns_ok:1;
+				netns_ok:1,
+				/* does the protocol do more stringent
+				 * icmp tag validation than simple
+				 * socket lookup?
+				 */
+				icmp_strict_tag_validation:1;
 };
 
 #if IS_ENABLED(CONFIG_IPV6)
diff --git a/net/dccp/ipv4.c b/net/dccp/ipv4.c
index 88299c2..22b5d81 100644
--- a/net/dccp/ipv4.c
+++ b/net/dccp/ipv4.c
@@ -989,6 +989,7 @@ static const struct net_protocol dccp_v4_protocol = {
 	.err_handler	= dccp_v4_err,
 	.no_policy	= 1,
 	.netns_ok	= 1,
+	.icmp_strict_tag_validation = 1,
 };
 
 static const struct proto_ops inet_dccp_ops = {
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 6268a47..ecd2c3f 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -1545,6 +1545,7 @@ static const struct net_protocol tcp_protocol = {
 	.err_handler	=	tcp_v4_err,
 	.no_policy	=	1,
 	.netns_ok	=	1,
+	.icmp_strict_tag_validation = 1,
 };
 
 static const struct net_protocol udp_protocol = {
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index fb3c563..0134663 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -668,6 +668,16 @@ static void icmp_socket_deliver(struct sk_buff *skb, u32 info)
 	rcu_read_unlock();
 }
 
+static bool icmp_tag_validation(int proto)
+{
+	bool ok;
+
+	rcu_read_lock();
+	ok = rcu_dereference(inet_protos[proto])->icmp_strict_tag_validation;
+	rcu_read_unlock();
+	return ok;
+}
+
 /*
  *	Handle ICMP_DEST_UNREACH, ICMP_TIME_EXCEED, ICMP_QUENCH, and
  *	ICMP_PARAMETERPROB.
@@ -705,12 +715,22 @@ static void icmp_unreach(struct sk_buff *skb)
 		case ICMP_PORT_UNREACH:
 			break;
 		case ICMP_FRAG_NEEDED:
-			if (net->ipv4.sysctl_ip_no_pmtu_disc == 2) {
-				goto out;
-			} else if (net->ipv4.sysctl_ip_no_pmtu_disc) {
+			/* for documentation of the ip_no_pmtu_disc
+			 * values please see
+			 * Documentation/networking/ip-sysctl.txt
+			 */
+			switch (net->ipv4.sysctl_ip_no_pmtu_disc) {
+			default:
 				LIMIT_NETDEBUG(KERN_INFO pr_fmt("%pI4: fragmentation needed and DF set\n"),
 					       &iph->daddr);
-			} else {
+				break;
+			case 2:
+				goto out;
+			case 3:
+				if (!icmp_tag_validation(iph->protocol))
+					goto out;
+				/* fall through */
+			case 0:
 				info = ntohs(icmph->un.frag.mtu);
 				if (!info)
 					goto out;
diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
index 34b7726..7c16108 100644
--- a/net/sctp/protocol.c
+++ b/net/sctp/protocol.c
@@ -1030,6 +1030,7 @@ static const struct net_protocol sctp_protocol = {
 	.err_handler = sctp_v4_err,
 	.no_policy   = 1,
 	.netns_ok    = 1,
+	.icmp_strict_tag_validation = 1,
 };
 
 /* IPv4 address related functions.  */
-- 
1.8.4.2

Re: [PATCH net-next v4 0/3] path mtu hardening patches

[David Miller]

From: hannes@stressinduktion.org
Date: Thu,  9 Jan 2014 10:01:14 +0100

> After a lot of back and forth I want to propose these changes regarding
> path mtu hardening and give an outline why I think this is the best way
> how to proceed:

I'm not going to fight this any more even though I still disagree with
these changes.  John Heffner has not provided a coherent strong
argument for not doing this, in fact the counter arguments were
extremely vague.

I am pretty sure that now my worst fears will be realized and every
single distribution will not use the kernel's default, and everyone
will get this behavior rather than adminstrators making well informed
decisions about how to defend against these kind of situations when
enabling routing, or whether they'd even be exposed to the issue at
all in a particular setup.

Such is life.

Re: [PATCH net-next v4 0/3] path mtu hardening patches

[John Heffner]

On Mon, Jan 13, 2014 at 2:25 PM, David Miller <davem@davemloft.net> wrote:
> From: hannes@stressinduktion.org
> Date: Thu,  9 Jan 2014 10:01:14 +0100
>
>> After a lot of back and forth I want to propose these changes regarding
>> path mtu hardening and give an outline why I think this is the best way
>> how to proceed:
>
> I'm not going to fight this any more even though I still disagree with
> these changes.  John Heffner has not provided a coherent strong
> argument for not doing this, in fact the counter arguments were
> extremely vague.
>
> I am pretty sure that now my worst fears will be realized and every
> single distribution will not use the kernel's default, and everyone
> will get this behavior rather than adminstrators making well informed
> decisions about how to defend against these kind of situations when
> enabling routing, or whether they'd even be exposed to the issue at
> all in a particular setup.
>
> Such is life.

My only comment would be not to look to me as the only source of
reason not to include this change.  I've been largely disconnected
from Linux development for several years and don't have time to get
into a protracted discussion on this topic.

FWIW, I still have doubts as to whether this is the best approach to
solving the underlying problem.  I still haven't heard any reason why
firewall rules and other administrative best practices, such as using
separate management and forwarding interfaces on a router, don't
practically solve this problem.  I'd also be curious to hear what
dedicated routing operating systems do, and why I haven't heard about
widespread fragmentation DoS attacks.

That said, this probably won't deeply break anything, but adds yet
more complexity in the core of the network stack...

  -John

Re: [PATCH net-next v4 0/3] path mtu hardening patches

[Hannes Frederic Sowa]

On Mon, Jan 13, 2014 at 02:35:31PM -0500, John Heffner wrote:
> On Mon, Jan 13, 2014 at 2:25 PM, David Miller <davem@davemloft.net> wrote:
> > From: hannes@stressinduktion.org
> > Date: Thu,  9 Jan 2014 10:01:14 +0100
> >
> >> After a lot of back and forth I want to propose these changes regarding
> >> path mtu hardening and give an outline why I think this is the best way
> >> how to proceed:
> >
> > I'm not going to fight this any more even though I still disagree with
> > these changes.  John Heffner has not provided a coherent strong
> > argument for not doing this, in fact the counter arguments were
> > extremely vague.
> >
> > I am pretty sure that now my worst fears will be realized and every
> > single distribution will not use the kernel's default, and everyone
> > will get this behavior rather than adminstrators making well informed
> > decisions about how to defend against these kind of situations when
> > enabling routing, or whether they'd even be exposed to the issue at
> > all in a particular setup.
> >
> > Such is life.

:/

> My only comment would be not to look to me as the only source of
> reason not to include this change.  I've been largely disconnected
> from Linux development for several years and don't have time to get
> into a protracted discussion on this topic.
> 
> FWIW, I still have doubts as to whether this is the best approach to
> solving the underlying problem.  I still haven't heard any reason why
> firewall rules and other administrative best practices, such as using

Because we currently cannot easily filter icmp payloads and check whether
it is in a response for a local socket or a malicious one.

> separate management and forwarding interfaces on a router, don't
> practically solve this problem.

I don't think this is practiable, especially in times of small devices
doing routing (e.g. smartphones).

> I'd also be curious to hear what
> dedicated routing operating systems do, and why I haven't heard about
> widespread fragmentation DoS attacks.

My old Cisco didn't honour those pmtu packets (at least in default
configuration) and FreeBSD only accepts pmtu information for TCP sockets
where it also verifies the sequence number. It does not react to pmtu
notifications in response to icmp or udp payloads.

Routing path does use the pmtu values on FreeBSD, though. But it is much
harder to inject path mtu packets there because, as said, they are only
accepted for tcp.

> That said, this probably won't deeply break anything, but adds yet
> more complexity in the core of the network stack...

Actually I feel a bit emberrased how these changes went in. At first I
thought they wouldn't be that much of a problem. I'll try to improve my
communiation skills and try to listen more carefully.

Thank you, David and John!

Greetings,

  Hannes

Re: [PATCH net-next v4 0/3] path mtu hardening patches

[John Heffner]

On Mon, Jan 13, 2014 at 3:42 PM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> On Mon, Jan 13, 2014 at 02:35:31PM -0500, John Heffner wrote:
>> My only comment would be not to look to me as the only source of
>> reason not to include this change.  I've been largely disconnected
>> from Linux development for several years and don't have time to get
>> into a protracted discussion on this topic.
>>
>> FWIW, I still have doubts as to whether this is the best approach to
>> solving the underlying problem.  I still haven't heard any reason why
>> firewall rules and other administrative best practices, such as using
>
> Because we currently cannot easily filter icmp payloads and check whether
> it is in a response for a local socket or a malicious one.
>
>> separate management and forwarding interfaces on a router, don't
>> practically solve this problem.
>
> I don't think this is practiable, especially in times of small devices
> doing routing (e.g. smartphones).
>
>> I'd also be curious to hear what
>> dedicated routing operating systems do, and why I haven't heard about
>> widespread fragmentation DoS attacks.
>
> My old Cisco didn't honour those pmtu packets (at least in default
> configuration) and FreeBSD only accepts pmtu information for TCP sockets
> where it also verifies the sequence number. It does not react to pmtu
> notifications in response to icmp or udp payloads.
>
> Routing path does use the pmtu values on FreeBSD, though. But it is much
> harder to inject path mtu packets there because, as said, they are only
> accepted for tcp.

Would it be sufficient to allow Linux to be configured in a way that
matches FreeBSD's behavior?  (I believe you can do this easily with
stateful firewall rules now, or possibly in the ICMP processing code
with a sysctl switch.)  I feel this would be a much cleaner approach.

Thanks,
  -John

Re: [PATCH net-next v4 0/3] path mtu hardening patches

[Hannes Frederic Sowa]

On Mon, Jan 13, 2014 at 04:08:22PM -0500, John Heffner wrote:
> On Mon, Jan 13, 2014 at 3:42 PM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
> > On Mon, Jan 13, 2014 at 02:35:31PM -0500, John Heffner wrote:
> >> My only comment would be not to look to me as the only source of
> >> reason not to include this change.  I've been largely disconnected
> >> from Linux development for several years and don't have time to get
> >> into a protracted discussion on this topic.
> >>
> >> FWIW, I still have doubts as to whether this is the best approach to
> >> solving the underlying problem.  I still haven't heard any reason why
> >> firewall rules and other administrative best practices, such as using
> >
> > Because we currently cannot easily filter icmp payloads and check whether
> > it is in a response for a local socket or a malicious one.
> >
> >> separate management and forwarding interfaces on a router, don't
> >> practically solve this problem.
> >
> > I don't think this is practiable, especially in times of small devices
> > doing routing (e.g. smartphones).
> >
> >> I'd also be curious to hear what
> >> dedicated routing operating systems do, and why I haven't heard about
> >> widespread fragmentation DoS attacks.
> >
> > My old Cisco didn't honour those pmtu packets (at least in default
> > configuration) and FreeBSD only accepts pmtu information for TCP sockets
> > where it also verifies the sequence number. It does not react to pmtu
> > notifications in response to icmp or udp payloads.
> >
> > Routing path does use the pmtu values on FreeBSD, though. But it is much
> > harder to inject path mtu packets there because, as said, they are only
> > accepted for tcp.
> 
> Would it be sufficient to allow Linux to be configured in a way that
> matches FreeBSD's behavior?  (I believe you can do this easily with
> stateful firewall rules now, or possibly in the ICMP processing code
> with a sysctl switch.)  I feel this would be a much cleaner approach.

Actually, this is part of this series. The hardened path mtu mode provides
exactly that (Patch 3).

But because we cannot switch this on by default, I also protected the
forwarding path. UDP path mtu discovery has been too long available on
Linux and, I guess, a lot of applications, especially running on routers,
depend on that.

Greetings,

  Hannes

Re: [PATCH net-next v4 0/3] path mtu hardening patches

[John Heffner]

On Mon, Jan 13, 2014 at 4:28 PM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
> On Mon, Jan 13, 2014 at 04:08:22PM -0500, John Heffner wrote:
>> Would it be sufficient to allow Linux to be configured in a way that
>> matches FreeBSD's behavior?  (I believe you can do this easily with
>> stateful firewall rules now, or possibly in the ICMP processing code
>> with a sysctl switch.)  I feel this would be a much cleaner approach.
>
> Actually, this is part of this series. The hardened path mtu mode provides
> exactly that (Patch 3).
>
> But because we cannot switch this on by default, I also protected the
> forwarding path. UDP path mtu discovery has been too long available on
> Linux and, I guess, a lot of applications, especially running on routers,
> depend on that.

Perhaps I misunderstood your description of FreeBSD then.  It seems
hard for me to believe that MTU discovery for UDP is broken by default
in FreeBSD.  It was not as of a couple years ago...

The nice thing about stateful firewall rules is that they give you
fine-grained policies over which ICMP messages you want to trust, and
can filter out messages that don't match "connections" with existing
state across a wide variety of protocols (including TCP, UDP and
ICMP).

  -John

Re: [PATCH net-next v4 0/3] path mtu hardening patches

[Hannes Frederic Sowa]

On Mon, Jan 13, 2014 at 04:50:38PM -0500, John Heffner wrote:
> On Mon, Jan 13, 2014 at 4:28 PM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
> > On Mon, Jan 13, 2014 at 04:08:22PM -0500, John Heffner wrote:
> >> Would it be sufficient to allow Linux to be configured in a way that
> >> matches FreeBSD's behavior?  (I believe you can do this easily with
> >> stateful firewall rules now, or possibly in the ICMP processing code
> >> with a sysctl switch.)  I feel this would be a much cleaner approach.
> >
> > Actually, this is part of this series. The hardened path mtu mode provides
> > exactly that (Patch 3).
> >
> > But because we cannot switch this on by default, I also protected the
> > forwarding path. UDP path mtu discovery has been too long available on
> > Linux and, I guess, a lot of applications, especially running on routers,
> > depend on that.
> 
> Perhaps I misunderstood your description of FreeBSD then.  It seems
> hard for me to believe that MTU discovery for UDP is broken by default
> in FreeBSD.  It was not as of a couple years ago...

I have to admit, I have not tested that. But I could not find an icmp handler
dealing with pmtu updates in the UDP protocol path. Neither did the icmp
layer.

TCP and sctp do handle the PRC_MSGSIZE callbacks and update the path
mtu on the route.

Maybe I overlooked something and I also did not check the history just
their current subversion checkout.

> The nice thing about stateful firewall rules is that they give you
> fine-grained policies over which ICMP messages you want to trust, and
> can filter out messages that don't match "connections" with existing
> state across a wide variety of protocols (including TCP, UDP and
> ICMP).

I really don't like to depend on firewalling to do that. Especially on
big routers one can use the routing table to protect interfaces for
management and thus don't need to introduce stateful firewalling to
realize a secure router setup which could cause performance degradation,
especially with lots of small and shortlived flows (e.g. UDP/DNS).

Greetings,

  Hannes

Re: [PATCH net-next v4 0/3] path mtu hardening patches

[David Miller]

From: John Heffner <johnwheffner@gmail.com>
Date: Mon, 13 Jan 2014 16:50:38 -0500

> Perhaps I misunderstood your description of FreeBSD then.  It seems
> hard for me to believe that MTU discovery for UDP is broken by default
> in FreeBSD.  It was not as of a couple years ago...

FreeBSD never implemented PMTU for anything other than TCP.

In fact, outside of Linux and perhaps Solaris, very few networking
stacks have implemented UDP PMTU.

Re: [PATCH net-next v4 0/3] path mtu hardening patches

[Hannes Frederic Sowa]

On Mon, Jan 13, 2014 at 11:03:56PM +0100, Hannes Frederic Sowa wrote:
> I really don't like to depend on firewalling to do that. Especially on
> big routers one can use the routing table to protect interfaces for
> management and thus don't need to introduce stateful firewalling to
> realize a secure router setup which could cause performance degradation,
> especially with lots of small and shortlived flows (e.g. UDP/DNS).

This may get better if maybe some work is put into bringing this patch
forward: http://comments.gmane.org/gmane.linux.network/268758

Greetings,

  Hannes

Re: [PATCH net-next v4 0/3] path mtu hardening patches

[Florian Westphal]

Hannes Frederic Sowa <hannes@stressinduktion.org> wrote:
> On Mon, Jan 13, 2014 at 11:03:56PM +0100, Hannes Frederic Sowa wrote:
> > I really don't like to depend on firewalling to do that. Especially on
> > big routers one can use the routing table to protect interfaces for
> > management and thus don't need to introduce stateful firewalling to
> > realize a secure router setup which could cause performance degradation,
> > especially with lots of small and shortlived flows (e.g. UDP/DNS).
> 
> This may get better if maybe some work is put into bringing this patch
> forward: http://comments.gmane.org/gmane.linux.network/268758

Jesper Brouer is working on this.

But, why do you even need stateful firewalling for filtering?
Isn't -m socket enough?

[ sorry if you already explained, might have missed it when search
archive ]

Re: [PATCH net-next v4 0/3] path mtu hardening patches

[Hannes Frederic Sowa]

On Mon, Jan 13, 2014 at 11:48:35PM +0100, Florian Westphal wrote:
> Hannes Frederic Sowa <hannes@stressinduktion.org> wrote:
> > On Mon, Jan 13, 2014 at 11:03:56PM +0100, Hannes Frederic Sowa wrote:
> > > I really don't like to depend on firewalling to do that. Especially on
> > > big routers one can use the routing table to protect interfaces for
> > > management and thus don't need to introduce stateful firewalling to
> > > realize a secure router setup which could cause performance degradation,
> > > especially with lots of small and shortlived flows (e.g. UDP/DNS).
> > 
> > This may get better if maybe some work is put into bringing this patch
> > forward: http://comments.gmane.org/gmane.linux.network/268758
> 
> Jesper Brouer is working on this.

Cool!

> But, why do you even need stateful firewalling for filtering?
> Isn't -m socket enough?
>
> [ sorry if you already explained, might have missed it when search
> archive ]

That would solve the tests I actually did, because the module doesn't let
ICMP packets with ICMP packet payload through (maybe this could
be bad for people using ping to debug pmtu problems on routers. this is
a bit far fetched, but I actually did ;) )

Myself, I don't trust socket lookup on unconnected UDP sockets any more as
you only need to spray such pMTU packets in a limited port range against
a box. Given that many routers currently also provide DNS services on
a random but outgoing port or are equipped with a whole bunch of services.

As soon as you have tunnels on a router, this wouldn't work either,
because you have to accept pmtu information on a non-socket in this case,
also ipsec.

I guess there are more special cases.

I think it is possible to come up with an iptables setup which is suitable
to protect a system in a special constellation. But, IMHO, it is better
to strictly follow the RFC and don't use path mtu in forwarding.

Greetings,

  Hannes