[PATCH next] netfilter: reject: don't send icmp error if packet has invalid checksum

[PATCH next] netfilter: reject: don't send icmp error if packet has invalid checksum

[Florian Westphal]

tcp resets are never emitted if the packet that triggers the
reject/reset has an invalid checksum.

For icmp error responses there was no such check.
It allows to distinguish icmp response generated via

iptables -I INPUT -p udp --dport 42 -j REJECT

and those emitted by network stack (won't respond if csum is invalid,
REJECT does).

Arguably its possible to avoid this by using conntrack and only using
REJECT with -m conntrack NEW/RELATED.

However, this doesn't work when connection tracking is not in use or
when using nf_conntrack_checksum=0.

Furthermore, sending errors in response to invalid csums doesn't make
much sense so just add similar test as in nf_send_reset.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/net/netfilter/ipv4/nf_reject.h |  6 +-----
 include/net/netfilter/ipv6/nf_reject.h | 11 ++---------
 net/ipv4/netfilter/ipt_REJECT.c        | 17 +++++++++--------
 net/ipv4/netfilter/nf_reject_ipv4.c    | 12 ++++++++++++
 net/ipv4/netfilter/nft_reject_ipv4.c   |  3 ++-
 net/ipv6/netfilter/nf_reject_ipv6.c    | 29 +++++++++++++++++++++++++++++
 net/netfilter/nft_reject_inet.c        |  6 ++++--
 7 files changed, 59 insertions(+), 25 deletions(-)

diff --git a/include/net/netfilter/ipv4/nf_reject.h b/include/net/netfilter/ipv4/nf_reject.h
index 03e928a..8641275 100644
--- a/include/net/netfilter/ipv4/nf_reject.h
+++ b/include/net/netfilter/ipv4/nf_reject.h
@@ -5,11 +5,7 @@
 #include <net/ip.h>
 #include <net/icmp.h>
 
-static inline void nf_send_unreach(struct sk_buff *skb_in, int code)
-{
-	icmp_send(skb_in, ICMP_DEST_UNREACH, code, 0);
-}
-
+void nf_send_unreach(struct sk_buff *skb_in, int code, int hook);
 void nf_send_reset(struct sk_buff *oldskb, int hook);
 
 const struct tcphdr *nf_reject_ip_tcphdr_get(struct sk_buff *oldskb,
diff --git a/include/net/netfilter/ipv6/nf_reject.h b/include/net/netfilter/ipv6/nf_reject.h
index 23216d4..0ae445d 100644
--- a/include/net/netfilter/ipv6/nf_reject.h
+++ b/include/net/netfilter/ipv6/nf_reject.h
@@ -3,15 +3,8 @@
 
 #include <linux/icmpv6.h>
 
-static inline void
-nf_send_unreach6(struct net *net, struct sk_buff *skb_in, unsigned char code,
-	     unsigned int hooknum)
-{
-	if (hooknum == NF_INET_LOCAL_OUT && skb_in->dev == NULL)
-		skb_in->dev = net->loopback_dev;
-
-	icmpv6_send(skb_in, ICMPV6_DEST_UNREACH, code, 0);
-}
+void nf_send_unreach6(struct net *net, struct sk_buff *skb_in, unsigned char code,
+		      unsigned int hooknum);
 
 void nf_send_reset6(struct net *net, struct sk_buff *oldskb, int hook);
 
diff --git a/net/ipv4/netfilter/ipt_REJECT.c b/net/ipv4/netfilter/ipt_REJECT.c
index 8f48f55..87907d4 100644
--- a/net/ipv4/netfilter/ipt_REJECT.c
+++ b/net/ipv4/netfilter/ipt_REJECT.c
@@ -34,31 +34,32 @@ static unsigned int
 reject_tg(struct sk_buff *skb, const struct xt_action_param *par)
 {
 	const struct ipt_reject_info *reject = par->targinfo;
+	int hook = par->hooknum;
 
 	switch (reject->with) {
 	case IPT_ICMP_NET_UNREACHABLE:
-		nf_send_unreach(skb, ICMP_NET_UNREACH);
+		nf_send_unreach(skb, ICMP_NET_UNREACH, hook);
 		break;
 	case IPT_ICMP_HOST_UNREACHABLE:
-		nf_send_unreach(skb, ICMP_HOST_UNREACH);
+		nf_send_unreach(skb, ICMP_HOST_UNREACH, hook);
 		break;
 	case IPT_ICMP_PROT_UNREACHABLE:
-		nf_send_unreach(skb, ICMP_PROT_UNREACH);
+		nf_send_unreach(skb, ICMP_PROT_UNREACH, hook);
 		break;
 	case IPT_ICMP_PORT_UNREACHABLE:
-		nf_send_unreach(skb, ICMP_PORT_UNREACH);
+		nf_send_unreach(skb, ICMP_PORT_UNREACH, hook);
 		break;
 	case IPT_ICMP_NET_PROHIBITED:
-		nf_send_unreach(skb, ICMP_NET_ANO);
+		nf_send_unreach(skb, ICMP_NET_ANO, hook);
 		break;
 	case IPT_ICMP_HOST_PROHIBITED:
-		nf_send_unreach(skb, ICMP_HOST_ANO);
+		nf_send_unreach(skb, ICMP_HOST_ANO, hook);
 		break;
 	case IPT_ICMP_ADMIN_PROHIBITED:
-		nf_send_unreach(skb, ICMP_PKT_FILTERED);
+		nf_send_unreach(skb, ICMP_PKT_FILTERED, hook);
 		break;
 	case IPT_TCP_RESET:
-		nf_send_reset(skb, par->hooknum);
+		nf_send_reset(skb, hook);
 	case IPT_ICMP_ECHOREPLY:
 		/* Doesn't happen. */
 		break;
diff --git a/net/ipv4/netfilter/nf_reject_ipv4.c b/net/ipv4/netfilter/nf_reject_ipv4.c
index 536da7b..b3e50c8 100644
--- a/net/ipv4/netfilter/nf_reject_ipv4.c
+++ b/net/ipv4/netfilter/nf_reject_ipv4.c
@@ -164,4 +164,16 @@ void nf_send_reset(struct sk_buff *oldskb, int hook)
 }
 EXPORT_SYMBOL_GPL(nf_send_reset);
 
+void nf_send_unreach(struct sk_buff *skb_in, int code, int hook)
+{
+	struct iphdr *iph = ip_hdr(skb_in);
+
+	if (iph->frag_off & htons(IP_OFFSET))
+		return;
+
+	if (nf_ip_checksum(skb_in, hook, ip_hdrlen(skb_in), iph->protocol) == 0)
+		icmp_send(skb_in, ICMP_DEST_UNREACH, code, 0);
+}
+EXPORT_SYMBOL_GPL(nf_send_unreach);
+
 MODULE_LICENSE("GPL");
diff --git a/net/ipv4/netfilter/nft_reject_ipv4.c b/net/ipv4/netfilter/nft_reject_ipv4.c
index d729542..16a5d4d 100644
--- a/net/ipv4/netfilter/nft_reject_ipv4.c
+++ b/net/ipv4/netfilter/nft_reject_ipv4.c
@@ -27,7 +27,8 @@ static void nft_reject_ipv4_eval(const struct nft_expr *expr,
 
 	switch (priv->type) {
 	case NFT_REJECT_ICMP_UNREACH:
-		nf_send_unreach(pkt->skb, priv->icmp_code);
+		nf_send_unreach(pkt->skb, priv->icmp_code,
+				pkt->ops->hooknum);
 		break;
 	case NFT_REJECT_TCP_RST:
 		nf_send_reset(pkt->skb, pkt->ops->hooknum);
diff --git a/net/ipv6/netfilter/nf_reject_ipv6.c b/net/ipv6/netfilter/nf_reject_ipv6.c
index d05b364..d6ea8fa 100644
--- a/net/ipv6/netfilter/nf_reject_ipv6.c
+++ b/net/ipv6/netfilter/nf_reject_ipv6.c
@@ -208,4 +208,33 @@ void nf_send_reset6(struct net *net, struct sk_buff *oldskb, int hook)
 }
 EXPORT_SYMBOL_GPL(nf_send_reset6);
 
+static bool reject6_csum_ok(struct sk_buff *skb, int hook)
+{
+	const struct ipv6hdr *ip6h = ipv6_hdr(skb);
+	int thoff;
+	__be16 fo;
+	u8 proto;
+
+	proto = ip6h->nexthdr;
+	thoff = ipv6_skip_exthdr(skb, ((u8*)(ip6h+1) - skb->data), &proto, &fo);
+
+	if (thoff < 0 || (fo & htons(~0x7)) != 0)
+		return false;
+
+	return nf_ip6_checksum(skb, hook, thoff, proto) == 0;
+}
+
+void nf_send_unreach6(struct net *net, struct sk_buff *skb_in,
+		      unsigned char code, unsigned int hooknum)
+{
+	if (!reject6_csum_ok(skb_in, hooknum))
+		return;
+
+	if (hooknum == NF_INET_LOCAL_OUT && skb_in->dev == NULL)
+		skb_in->dev = net->loopback_dev;
+
+	icmpv6_send(skb_in, ICMPV6_DEST_UNREACH, code, 0);
+}
+EXPORT_SYMBOL_GPL(nf_send_unreach6);
+
 MODULE_LICENSE("GPL");
diff --git a/net/netfilter/nft_reject_inet.c b/net/netfilter/nft_reject_inet.c
index 7b5f9d5..9287711 100644
--- a/net/netfilter/nft_reject_inet.c
+++ b/net/netfilter/nft_reject_inet.c
@@ -28,14 +28,16 @@ static void nft_reject_inet_eval(const struct nft_expr *expr,
 	case NFPROTO_IPV4:
 		switch (priv->type) {
 		case NFT_REJECT_ICMP_UNREACH:
-			nf_send_unreach(pkt->skb, priv->icmp_code);
+			nf_send_unreach(pkt->skb, priv->icmp_code,
+					pkt->ops->hooknum);
 			break;
 		case NFT_REJECT_TCP_RST:
 			nf_send_reset(pkt->skb, pkt->ops->hooknum);
 			break;
 		case NFT_REJECT_ICMPX_UNREACH:
 			nf_send_unreach(pkt->skb,
-					nft_reject_icmp_code(priv->icmp_code));
+					nft_reject_icmp_code(priv->icmp_code),
+					pkt->ops->hooknum);
 			break;
 		}
 		break;
-- 
2.0.5

Re: [PATCH next] netfilter: reject: don't send icmp error if packet has invalid checksum

[Pablo Neira Ayuso]

On Thu, Jan 29, 2015 at 10:59:46AM +0100, Florian Westphal wrote:
> tcp resets are never emitted if the packet that triggers the
> reject/reset has an invalid checksum.
> 
> For icmp error responses there was no such check.
> It allows to distinguish icmp response generated via
> 
> iptables -I INPUT -p udp --dport 42 -j REJECT
> 
> and those emitted by network stack (won't respond if csum is invalid,
> REJECT does).
> 
> Arguably its possible to avoid this by using conntrack and only using
> REJECT with -m conntrack NEW/RELATED.
> 
> However, this doesn't work when connection tracking is not in use or
> when using nf_conntrack_checksum=0.
> 
> Furthermore, sending errors in response to invalid csums doesn't make
> much sense so just add similar test as in nf_send_reset.

Could you also review net/bridge/netfilter/nft_reject_bridge.c?

Thanks.

Re: [PATCH next] netfilter: reject: don't send icmp error if packet has invalid checksum

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > For icmp error responses there was no such check.
> > It allows to distinguish icmp response generated via
> > 
> > iptables -I INPUT -p udp --dport 42 -j REJECT
> > 
> > and those emitted by network stack (won't respond if csum is invalid,
> > REJECT does).
> > 
> > Arguably its possible to avoid this by using conntrack and only using
> > REJECT with -m conntrack NEW/RELATED.
> > 
> > However, this doesn't work when connection tracking is not in use or
> > when using nf_conntrack_checksum=0.
> > 
> > Furthermore, sending errors in response to invalid csums doesn't make
> > much sense so just add similar test as in nf_send_reset.
> 
> Could you also review net/bridge/netfilter/nft_reject_bridge.c?

Looks like the ipv6 part doesn't check them either, I'll see how to best
address this (ipv4 part looks good).

Thanks.

Re: [PATCH next] netfilter: reject: don't send icmp error if packet has invalid checksum

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Thu, Jan 29, 2015 at 10:59:46AM +0100, Florian Westphal wrote:
> > tcp resets are never emitted if the packet that triggers the
> > reject/reset has an invalid checksum.
> > 
> > For icmp error responses there was no such check.
> > It allows to distinguish icmp response generated via
> > 
> > iptables -I INPUT -p udp --dport 42 -j REJECT
> > 
> > and those emitted by network stack (won't respond if csum is invalid,
> > REJECT does).
> > 
> > Arguably its possible to avoid this by using conntrack and only using
> > REJECT with -m conntrack NEW/RELATED.
> > 
> > However, this doesn't work when connection tracking is not in use or
> > when using nf_conntrack_checksum=0.
> > 
> > Furthermore, sending errors in response to invalid csums doesn't make
> > much sense so just add similar test as in nf_send_reset.
> 
> Could you also review net/bridge/netfilter/nft_reject_bridge.c?

Sorry that this took some time.  Gist is that this has a few issues at
the moment, its not working for me.

Some issues that I found:
Doesn't work in INPUT since skb->dev is the bridge port, so we end
with NULL br_port_get_rcu (no crash, br_deliver is noop).

After fixing INPUT issue, nft reject from bridge works if
nf-call-iptables is on, most likely due to extra pskb_trim_rcsum
done by bridge_netfilter before PREROUTING invocation for ipv4.

Adding an explicit call to pskb_trim_rcsum() seemed to make reject
work from bridge layer in both prerouting and input.

Untested further possible issues:

seems nf*_ip_checksum() only works for UDP or TCP, not with
e.g. SCTP or UDPLITE.

Unfortunately I'll be unavailable next week due to meetings & devconf conference
in Brno.  I'm sending what I have at the moment.  If someone wants to
pick that up as a starting point, please go ahead.

Otherwise I'll get back to this on Feb 10th or so.

Thanks,
Florian

This makes bridge reject (good-checksum) udp packets in both input and
prerouting (nf-call-iptables off).  ipv6 is untested.

diff --git a/net/bridge/netfilter/nft_reject_bridge.c b/net/bridge/netfilter/nft_reject_bridge.c
--- a/net/bridge/netfilter/nft_reject_bridge.c
+++ b/net/bridge/netfilter/nft_reject_bridge.c
@@ -36,7 +36,14 @@ static void nft_reject_br_push_etherhdr(struct sk_buff *oldskb,
 	skb_pull(nskb, ETH_HLEN);
 }
 
-static void nft_reject_br_send_v4_tcp_reset(struct sk_buff *oldskb, int hook)
+/* We cannot use oldskb->dev, since it either is the bridge port
+ * (NF_BRIDGE PREROUTING) OR the bridge device (NF_BRIDGE INPUT).
+ *
+ * We must use bridge netfilter indevice instead.
+ */
+static void nft_reject_br_send_v4_tcp_reset(struct sk_buff *oldskb,
+					    const struct net_device *dev,
+					    int hook)
 {
 	struct sk_buff *nskb;
 	struct iphdr *niph;
@@ -65,11 +72,12 @@ static void nft_reject_br_send_v4_tcp_reset(struct sk_buff *oldskb, int hook)
 
 	nft_reject_br_push_etherhdr(oldskb, nskb);
 
-	br_deliver(br_port_get_rcu(oldskb->dev), nskb);
+	br_deliver(br_port_get_rcu(dev), nskb);
 }
 
-static void nft_reject_br_send_v4_unreach(struct sk_buff *oldskb, int hook,
-					  u8 code)
+static void nft_reject_br_send_v4_unreach(struct sk_buff *oldskb,
+					  const struct net_device *dev,
+					  int hook, u8 code)
 {
 	struct sk_buff *nskb;
 	struct iphdr *niph;
@@ -91,7 +99,10 @@ static void nft_reject_br_send_v4_unreach(struct sk_buff *oldskb, int hook,
 	if (!pskb_may_pull(oldskb, len))
 		return;
 
-	if (nf_ip_checksum(oldskb, hook, ip_hdrlen(oldskb), 0))
+	if (pskb_trim_rcsum(oldskb, htons(ip_hdr(oldskb)->tot_len)))
+		return;
+
+	if (nf_ip_checksum(oldskb, hook, ip_hdrlen(oldskb), ip_hdr(oldskb)->protocol))
 		return;
 
 	nskb = alloc_skb(sizeof(struct iphdr) + sizeof(struct icmphdr) +
@@ -120,11 +131,13 @@ static void nft_reject_br_send_v4_unreach(struct sk_buff *oldskb, int hook,
 
 	nft_reject_br_push_etherhdr(oldskb, nskb);
 
-	br_deliver(br_port_get_rcu(oldskb->dev), nskb);
+	br_deliver(br_port_get_rcu(dev), nskb);
 }
 
 static void nft_reject_br_send_v6_tcp_reset(struct net *net,
-					    struct sk_buff *oldskb, int hook)
+					    struct sk_buff *oldskb,
+					    const struct net_device *dev,
+					    int hook)
 {
 	struct sk_buff *nskb;
 	const struct tcphdr *oth;
@@ -152,12 +165,13 @@ static void nft_reject_br_send_v6_tcp_reset(struct net *net,
 
 	nft_reject_br_push_etherhdr(oldskb, nskb);
 
-	br_deliver(br_port_get_rcu(oldskb->dev), nskb);
+	br_deliver(br_port_get_rcu(dev), nskb);
 }
 
 static void nft_reject_br_send_v6_unreach(struct net *net,
-					  struct sk_buff *oldskb, int hook,
-					  u8 code)
+					  struct sk_buff *oldskb,
+					  const struct net_device *dev,
+					  int hook, u8 code)
 {
 	struct sk_buff *nskb;
 	struct ipv6hdr *nip6h;
@@ -205,7 +219,7 @@ static void nft_reject_br_send_v6_unreach(struct net *net,
 
 	nft_reject_br_push_etherhdr(oldskb, nskb);
 
-	br_deliver(br_port_get_rcu(oldskb->dev), nskb);
+	br_deliver(br_port_get_rcu(dev), nskb);
 }
 
 static void nft_reject_bridge_eval(const struct nft_expr *expr,
@@ -224,16 +238,16 @@ static void nft_reject_bridge_eval(const struct nft_expr *expr,
 	case htons(ETH_P_IP):
 		switch (priv->type) {
 		case NFT_REJECT_ICMP_UNREACH:
-			nft_reject_br_send_v4_unreach(pkt->skb,
+			nft_reject_br_send_v4_unreach(pkt->skb, pkt->in,
 						      pkt->ops->hooknum,
 						      priv->icmp_code);
 			break;
 		case NFT_REJECT_TCP_RST:
-			nft_reject_br_send_v4_tcp_reset(pkt->skb,
+			nft_reject_br_send_v4_tcp_reset(pkt->skb, pkt->in,
 							pkt->ops->hooknum);
 			break;
 		case NFT_REJECT_ICMPX_UNREACH:
-			nft_reject_br_send_v4_unreach(pkt->skb,
+			nft_reject_br_send_v4_unreach(pkt->skb, pkt->in,
 						      pkt->ops->hooknum,
 						      nft_reject_icmp_code(priv->icmp_code));
 			break;
@@ -242,16 +256,16 @@ static void nft_reject_bridge_eval(const struct nft_expr *expr,
 	case htons(ETH_P_IPV6):
 		switch (priv->type) {
 		case NFT_REJECT_ICMP_UNREACH:
-			nft_reject_br_send_v6_unreach(net, pkt->skb,
+			nft_reject_br_send_v6_unreach(net, pkt->skb, pkt->in,
 						      pkt->ops->hooknum,
 						      priv->icmp_code);
 			break;
 		case NFT_REJECT_TCP_RST:
-			nft_reject_br_send_v6_tcp_reset(net, pkt->skb,
+			nft_reject_br_send_v6_tcp_reset(net, pkt->skb, pkt->in,
 							pkt->ops->hooknum);
 			break;
 		case NFT_REJECT_ICMPX_UNREACH:
-			nft_reject_br_send_v6_unreach(net, pkt->skb,
+			nft_reject_br_send_v6_unreach(net, pkt->skb, pkt->in,
 						      pkt->ops->hooknum,
 						      nft_reject_icmpv6_code(priv->icmp_code));
 			break;

Re: [PATCH next] netfilter: reject: don't send icmp error if packet has invalid checksum

[Pablo Neira Ayuso]

On Sat, Jan 31, 2015 at 10:47:17PM +0100, Florian Westphal wrote:
> Some issues that I found:
> Doesn't work in INPUT since skb->dev is the bridge port, so we end
> with NULL br_port_get_rcu (no crash, br_deliver is noop).
> 
> After fixing INPUT issue, nft reject from bridge works if
> nf-call-iptables is on, most likely due to extra pskb_trim_rcsum
> done by bridge_netfilter before PREROUTING invocation for ipv4.
> 
> Adding an explicit call to pskb_trim_rcsum() seemed to make reject
> work from bridge layer in both prerouting and input.

Will you send me formal submission for this?

> Untested further possible issues:
> 
> seems nf*_ip_checksum() only works for UDP or TCP, not with
> e.g. SCTP or UDPLITE.

I guess you'll have to add checks in that function for all transport
protocols that we support, may require some ifdefs. We may still reply
to unknown transport protocols depending on the iptables
configuration, but there's little we can do on that.

Thanks Florian.

Re: [PATCH next] netfilter: reject: don't send icmp error if packet has invalid checksum

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Sat, Jan 31, 2015 at 10:47:17PM +0100, Florian Westphal wrote:
> > Some issues that I found:
> > Doesn't work in INPUT since skb->dev is the bridge port, so we end
> > with NULL br_port_get_rcu (no crash, br_deliver is noop).
> > 
> > After fixing INPUT issue, nft reject from bridge works if
> > nf-call-iptables is on, most likely due to extra pskb_trim_rcsum
> > done by bridge_netfilter before PREROUTING invocation for ipv4.
> > 
> > Adding an explicit call to pskb_trim_rcsum() seemed to make reject
> > work from bridge layer in both prerouting and input.
> 
> Will you send me formal submission for this?

I considered it ugly kludge but given we're dropping anyway
it should be ok.  So yes, I'll send you a formal patch, but not this
week, sorry :-|

> > Untested further possible issues:
> > 
> > seems nf*_ip_checksum() only works for UDP or TCP, not with
> > e.g. SCTP or UDPLITE.
> 
> I guess you'll have to add checks in that function for all transport
> protocols that we support, may require some ifdefs.

Seems ifdefs are not enough, e.g. sctp would need libcrc32c module :-(

Not sure what to do at the moment, perhaps only do csum validation for
UDP and TCP, and rely on ipv4 csum alone for the rest?

For ipv6, this would obviously mean no csum validation except
for tcp and udp.