netfilter: reject: skip csum verification for protocols that don't support it

Message ID 1549631724-20326-1-git-send-email-alin.nastac@technicolor.com
State Changes Requested
Delegated to: Pablo Neira
Headers show
Series
  • netfilter: reject: skip csum verification for protocols that don't support it
Related show

Commit Message

Alin Năstac Feb. 8, 2019, 1:15 p.m.
From: Alin Nastac <alin.nastac@gmail.com>

Some protocols have other means to verify the payload integrity
(AH, ESP, SCTP) while others are incompatible with nf_ip(6)_checksum
implementation because checksum is either optional or might be
partial (UDPLITE, DCCP, GRE). Because nf_ip(6)_checksum was used
to validate the packets, ip(6)tables REJECT rules were not capable
to generate ICMP(v6) errors for the protocols mentioned above.

This commit also fixes the incorrect pseudo-header protocol used
for IPv4 packets that carry other transport protocols than TCP or
UDP (pseudo-header used protocol 0 iso the proper value).

Signed-off-by: Alin Nastac <alin.nastac@gmail.com>
---
 net/ipv4/netfilter/nf_reject_ipv4.c | 33 ++++++++++++++++++++++++---------
 net/ipv6/netfilter/nf_reject_ipv6.c | 18 ++++++++++++++++++
 2 files changed, 42 insertions(+), 9 deletions(-)

Comments

Florian Westphal Feb. 8, 2019, 1:25 p.m. | #1
Alin Nastac <alin.nastac@gmail.com> wrote:
> From: Alin Nastac <alin.nastac@gmail.com>
> 
> Some protocols have other means to verify the payload integrity
> (AH, ESP, SCTP) while others are incompatible with nf_ip(6)_checksum
> implementation because checksum is either optional or might be
> partial (UDPLITE, DCCP, GRE). Because nf_ip(6)_checksum was used
> to validate the packets, ip(6)tables REJECT rules were not capable
> to generate ICMP(v6) errors for the protocols mentioned above.
> 
> This commit also fixes the incorrect pseudo-header protocol used
> for IPv4 packets that carry other transport protocols than TCP or
> UDP (pseudo-header used protocol 0 iso the proper value).
> 
> Signed-off-by: Alin Nastac <alin.nastac@gmail.com>

Acked-by: Florian Westphal <fw@strlen.de>
Pablo Neira Ayuso Feb. 11, 2019, 11:45 p.m. | #2
On Fri, Feb 08, 2019 at 02:15:24PM +0100, Alin Nastac wrote:
> From: Alin Nastac <alin.nastac@gmail.com>
> 
> Some protocols have other means to verify the payload integrity
> (AH, ESP, SCTP) while others are incompatible with nf_ip(6)_checksum
> implementation because checksum is either optional or might be
> partial (UDPLITE, DCCP, GRE). Because nf_ip(6)_checksum was used
> to validate the packets, ip(6)tables REJECT rules were not capable
> to generate ICMP(v6) errors for the protocols mentioned above.
> 
> This commit also fixes the incorrect pseudo-header protocol used
> for IPv4 packets that carry other transport protocols than TCP or
> UDP (pseudo-header used protocol 0 iso the proper value).

Applied, thanks Alin.
Pablo Neira Ayuso Feb. 11, 2019, 11:50 p.m. | #3
Hi Alin,

On Tue, Feb 12, 2019 at 12:45:51AM +0100, Pablo Neira Ayuso wrote:
> On Fri, Feb 08, 2019 at 02:15:24PM +0100, Alin Nastac wrote:
> > From: Alin Nastac <alin.nastac@gmail.com>
> > 
> > Some protocols have other means to verify the payload integrity
> > (AH, ESP, SCTP) while others are incompatible with nf_ip(6)_checksum
> > implementation because checksum is either optional or might be
> > partial (UDPLITE, DCCP, GRE). Because nf_ip(6)_checksum was used
> > to validate the packets, ip(6)tables REJECT rules were not capable
> > to generate ICMP(v6) errors for the protocols mentioned above.
> > 
> > This commit also fixes the incorrect pseudo-header protocol used
> > for IPv4 packets that carry other transport protocols than TCP or
> > UDP (pseudo-header used protocol 0 iso the proper value).

Sorry, I just realized that we are not updating:

net/bridge/netfilter/nft_reject_bridge.c

Probably we can place this:

+       proto = iph->protocol;
+       switch (proto) {
+               /* Protocols with other integrity checks. */
+               case IPPROTO_AH:
+               case IPPROTO_ESP:
+               case IPPROTO_SCTP:
+
+               /* Protocols with partial checksums. */
+               case IPPROTO_UDPLITE:
+               case IPPROTO_DCCP:
+
+               /* Protocols with optional checksums. */
+               case IPPROTO_GRE:
+                       goto send_unreach;
        }

into an inline function in include/net/netfilter/nf_reject.h and use
it from these three spots?

Thanks!
Alin Năstac Feb. 12, 2019, 6:25 a.m. | #4
Hi Pablo,

On Tue, Feb 12, 2019 at 12:50 AM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> Hi Alin,
>
> On Tue, Feb 12, 2019 at 12:45:51AM +0100, Pablo Neira Ayuso wrote:
> > On Fri, Feb 08, 2019 at 02:15:24PM +0100, Alin Nastac wrote:
> > > From: Alin Nastac <alin.nastac@gmail.com>
> > >
> > > Some protocols have other means to verify the payload integrity
> > > (AH, ESP, SCTP) while others are incompatible with nf_ip(6)_checksum
> > > implementation because checksum is either optional or might be
> > > partial (UDPLITE, DCCP, GRE). Because nf_ip(6)_checksum was used
> > > to validate the packets, ip(6)tables REJECT rules were not capable
> > > to generate ICMP(v6) errors for the protocols mentioned above.
> > >
> > > This commit also fixes the incorrect pseudo-header protocol used
> > > for IPv4 packets that carry other transport protocols than TCP or
> > > UDP (pseudo-header used protocol 0 iso the proper value).
>
> Sorry, I just realized that we are not updating:
>
> net/bridge/netfilter/nft_reject_bridge.c
>
> Probably we can place this:
>
> +       proto = iph->protocol;
> +       switch (proto) {
> +               /* Protocols with other integrity checks. */
> +               case IPPROTO_AH:
> +               case IPPROTO_ESP:
> +               case IPPROTO_SCTP:
> +
> +               /* Protocols with partial checksums. */
> +               case IPPROTO_UDPLITE:
> +               case IPPROTO_DCCP:
> +
> +               /* Protocols with optional checksums. */
> +               case IPPROTO_GRE:
> +                       goto send_unreach;
>         }
>
> into an inline function in include/net/netfilter/nf_reject.h and use
> it from these three spots?

The pseudo-header proto=0 issue must also be addressed in
net/bridge/netfilter/nft_reject_bridge.c.

I see you haven't pushed yet my commit. Do you want me to issue the
2nd version of this patch?
Pablo Neira Ayuso Feb. 12, 2019, 10:19 a.m. | #5
Hi Alin,

On Tue, Feb 12, 2019 at 07:25:29AM +0100, Alin Năstac wrote:
> The pseudo-header proto=0 issue must also be addressed in
> net/bridge/netfilter/nft_reject_bridge.c.
> 
> I see you haven't pushed yet my commit. Do you want me to issue the
> 2nd version of this patch?

You choose:

1) I push out your original patch and I wait for a follow up patch to
   adjust nft_reject_bridge.c

2) I wait for a v2 that fixes things in one go.

Let me know your preference.

Thanks.
Alin Năstac Feb. 12, 2019, 10:21 a.m. | #6
Hi Pablo,

On Tue, Feb 12, 2019 at 11:20 AM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> Hi Alin,
>
> On Tue, Feb 12, 2019 at 07:25:29AM +0100, Alin Năstac wrote:
> > The pseudo-header proto=0 issue must also be addressed in
> > net/bridge/netfilter/nft_reject_bridge.c.
> >
> > I see you haven't pushed yet my commit. Do you want me to issue the
> > 2nd version of this patch?
>
> You choose:
>
> 1) I push out your original patch and I wait for a follow up patch to
>    adjust nft_reject_bridge.c
>
> 2) I wait for a v2 that fixes things in one go.
>
> Let me know your preference.

I will send you later today the v2 version of this patch.
Alin Năstac Feb. 12, 2019, 2:38 p.m. | #7
Hi Pablo,

On Tue, Feb 12, 2019 at 11:21 AM Alin Năstac <alin.nastac@gmail.com> wrote:
> I will send you later today the v2 version of this patch.

I have problems with the inline function defined in
include/net/netfilter/nft_reject.h. Mu CONFIG_NF_TABLES is disabled
and I get the following error when I try to include it in
nf_reject_ipv{4,6}.c:
include/net/netfilter/nf_tables.h:898:12: error: 'const struct net'
has no member named 'nft'
 return net->nft.gencursor + 1 == 1 ? 1 : 0;

Should I create a new include/net/netfilter/nf_reject.h file and
include it in both include/net/netfilter/ipv{4,6}/nf_reject.h?

Patch

diff --git a/net/ipv4/netfilter/nf_reject_ipv4.c b/net/ipv4/netfilter/nf_reject_ipv4.c
index aa8304c..9225b7f 100644
--- a/net/ipv4/netfilter/nf_reject_ipv4.c
+++ b/net/ipv4/netfilter/nf_reject_ipv4.c
@@ -178,18 +178,33 @@  void nf_send_unreach(struct sk_buff *skb_in, int code, int hook)
 	if (iph->frag_off & htons(IP_OFFSET))
 		return;
 
-	if (skb_csum_unnecessary(skb_in)) {
-		icmp_send(skb_in, ICMP_DEST_UNREACH, code, 0);
-		return;
+	if (skb_csum_unnecessary(skb_in))
+		goto send_unreach;
+
+	/* Skip checksum verification for protocols that don't use
+	 * 16-bit one's complement checksum of the entire payload.
+	 */
+	proto = iph->protocol;
+	switch (proto) {
+		/* Protocols with other integrity checks. */
+		case IPPROTO_AH:
+		case IPPROTO_ESP:
+		case IPPROTO_SCTP:
+
+		/* Protocols with partial checksums. */
+		case IPPROTO_UDPLITE:
+		case IPPROTO_DCCP:
+
+		/* Protocols with optional checksums. */
+		case IPPROTO_GRE:
+			goto send_unreach;
 	}
 
-	if (iph->protocol == IPPROTO_TCP || iph->protocol == IPPROTO_UDP)
-		proto = iph->protocol;
-	else
-		proto = 0;
+	if (nf_ip_checksum(skb_in, hook, ip_hdrlen(skb_in), proto))
+		return;
 
-	if (nf_ip_checksum(skb_in, hook, ip_hdrlen(skb_in), proto) == 0)
-		icmp_send(skb_in, ICMP_DEST_UNREACH, code, 0);
+ send_unreach:
+	icmp_send(skb_in, ICMP_DEST_UNREACH, code, 0);
 }
 EXPORT_SYMBOL_GPL(nf_send_unreach);
 
diff --git a/net/ipv6/netfilter/nf_reject_ipv6.c b/net/ipv6/netfilter/nf_reject_ipv6.c
index b9c8a76..2af014e 100644
--- a/net/ipv6/netfilter/nf_reject_ipv6.c
+++ b/net/ipv6/netfilter/nf_reject_ipv6.c
@@ -233,6 +233,24 @@  static bool reject6_csum_ok(struct sk_buff *skb, int hook)
 	if (thoff < 0 || thoff >= skb->len || (fo & htons(~0x7)) != 0)
 		return false;
 
+	/* Skip protocols that don't use 16-bit one's complement checksum
+	 * of the entire payload.
+	 */
+	switch (proto) {
+		/* Protocols with other integrity checks. */
+		case IPPROTO_AH:
+		case IPPROTO_ESP:
+		case IPPROTO_SCTP:
+
+		/* Protocols with partial checksums. */
+		case IPPROTO_UDPLITE:
+		case IPPROTO_DCCP:
+
+		/* Protocols with optional checksums. */
+		case IPPROTO_GRE:
+			return true;
+	}
+
 	return nf_ip6_checksum(skb, hook, thoff, proto) == 0;
 }