[PATCH libnetfilter_queue] checksum: Fix UDP checksum calculation

[PATCH libnetfilter_queue] checksum: Fix UDP checksum calculation

[Pablo Neira Ayuso]

The level 4 protocol is part of the UDP and TCP calculations.
nfq_checksum_tcpudp_ipv4() was using IPPROTO_TCP in this calculation,
which gave the wrong answer for UDP.

Based on patch from Alin Nastac, and patch description from Duncan Roe.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/extra/checksum.c | 9 +++++----
 src/extra/tcp.c      | 4 ++--
 src/extra/udp.c      | 4 ++--
 src/internal.h       | 5 +++--
 4 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/src/extra/checksum.c b/src/extra/checksum.c
index f367f75bbef3..4d52a998dc82 100644
--- a/src/extra/checksum.c
+++ b/src/extra/checksum.c
@@ -35,7 +35,7 @@ uint16_t nfq_checksum(uint32_t sum, uint16_t *buf, int size)
 	return (uint16_t)(~sum);
 }
 
-uint16_t nfq_checksum_tcpudp_ipv4(struct iphdr *iph)
+uint16_t nfq_checksum_tcpudp_ipv4(struct iphdr *iph, uint16_t protonum)
 {
 	uint32_t sum = 0;
 	uint32_t iph_len = iph->ihl*4;
@@ -46,13 +46,14 @@ uint16_t nfq_checksum_tcpudp_ipv4(struct iphdr *iph)
 	sum += (iph->saddr) & 0xFFFF;
 	sum += (iph->daddr >> 16) & 0xFFFF;
 	sum += (iph->daddr) & 0xFFFF;
-	sum += htons(IPPROTO_TCP);
+	sum += htons(protonum);
 	sum += htons(len);
 
 	return nfq_checksum(sum, (uint16_t *)payload, len);
 }
 
-uint16_t nfq_checksum_tcpudp_ipv6(struct ip6_hdr *ip6h, void *transport_hdr)
+uint16_t nfq_checksum_tcpudp_ipv6(struct ip6_hdr *ip6h, void *transport_hdr,
+				  uint16_t protonum)
 {
 	uint32_t sum = 0;
 	uint32_t hdr_len = (uint32_t *)transport_hdr - (uint32_t *)ip6h;
@@ -68,7 +69,7 @@ uint16_t nfq_checksum_tcpudp_ipv6(struct ip6_hdr *ip6h, void *transport_hdr)
 		sum += (ip6h->ip6_dst.s6_addr16[i] >> 16) & 0xFFFF;
 		sum += (ip6h->ip6_dst.s6_addr16[i]) & 0xFFFF;
 	}
-	sum += htons(IPPROTO_TCP);
+	sum += htons(protonum);
 	sum += htons(ip6h->ip6_plen);
 
 	return nfq_checksum(sum, (uint16_t *)payload, len);
diff --git a/src/extra/tcp.c b/src/extra/tcp.c
index d1cd79d9de67..a66f3924145b 100644
--- a/src/extra/tcp.c
+++ b/src/extra/tcp.c
@@ -96,7 +96,7 @@ nfq_tcp_compute_checksum_ipv4(struct tcphdr *tcph, struct iphdr *iph)
 {
 	/* checksum field in header needs to be zero for calculation. */
 	tcph->check = 0;
-	tcph->check = nfq_checksum_tcpudp_ipv4(iph);
+	tcph->check = nfq_checksum_tcpudp_ipv4(iph, IPPROTO_TCP);
 }
 EXPORT_SYMBOL(nfq_tcp_compute_checksum_ipv4);
 
@@ -110,7 +110,7 @@ nfq_tcp_compute_checksum_ipv6(struct tcphdr *tcph, struct ip6_hdr *ip6h)
 {
 	/* checksum field in header needs to be zero for calculation. */
 	tcph->check = 0;
-	tcph->check = nfq_checksum_tcpudp_ipv6(ip6h, tcph);
+	tcph->check = nfq_checksum_tcpudp_ipv6(ip6h, tcph, IPPROTO_TCP);
 }
 EXPORT_SYMBOL(nfq_tcp_compute_checksum_ipv6);
 
diff --git a/src/extra/udp.c b/src/extra/udp.c
index 8c44a66e8209..c48a179dfccb 100644
--- a/src/extra/udp.c
+++ b/src/extra/udp.c
@@ -96,7 +96,7 @@ nfq_udp_compute_checksum_ipv4(struct udphdr *udph, struct iphdr *iph)
 {
 	/* checksum field in header needs to be zero for calculation. */
 	udph->check = 0;
-	udph->check = nfq_checksum_tcpudp_ipv4(iph);
+	udph->check = nfq_checksum_tcpudp_ipv4(iph, IPPROTO_UDP);
 }
 EXPORT_SYMBOL(nfq_udp_compute_checksum_ipv4);
 
@@ -115,7 +115,7 @@ nfq_udp_compute_checksum_ipv6(struct udphdr *udph, struct ip6_hdr *ip6h)
 {
 	/* checksum field in header needs to be zero for calculation. */
 	udph->check = 0;
-	udph->check = nfq_checksum_tcpudp_ipv6(ip6h, udph);
+	udph->check = nfq_checksum_tcpudp_ipv6(ip6h, udph, IPPROTO_UDP);
 }
 EXPORT_SYMBOL(nfq_udp_compute_checksum_ipv6);
 
diff --git a/src/internal.h b/src/internal.h
index 558d267bf602..d648bfe112e8 100644
--- a/src/internal.h
+++ b/src/internal.h
@@ -15,8 +15,9 @@ struct iphdr;
 struct ip6_hdr;
 
 uint16_t nfq_checksum(uint32_t sum, uint16_t *buf, int size);
-uint16_t nfq_checksum_tcpudp_ipv4(struct iphdr *iph);
-uint16_t nfq_checksum_tcpudp_ipv6(struct ip6_hdr *ip6h, void *transport_hdr);
+uint16_t nfq_checksum_tcpudp_ipv4(struct iphdr *iph, uint16_t protonum);
+uint16_t nfq_checksum_tcpudp_ipv6(struct ip6_hdr *ip6h, void *transport_hdr,
+				  uint16_t protonum);
 
 struct pkt_buff {
 	uint8_t *mac_header;
-- 
2.11.0

Re: [PATCH libnetfilter_queue] checksum: Fix UDP checksum calculation

[Alin Năstac]

Hi Pablo,

On Mon, Sep 30, 2019 at 4:29 PM Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> The level 4 protocol is part of the UDP and TCP calculations.
> nfq_checksum_tcpudp_ipv4() was using IPPROTO_TCP in this calculation,
> which gave the wrong answer for UDP.
>
> Based on patch from Alin Nastac, and patch description from Duncan Roe.
>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

There was another issue that my patch fixed, on big endian platform
checksum is incorrectly computed when payload length is odd. You have
to include this changes as well in order to fix this:
--- a/src/extra/checksum.c
+++ b/src/extra/checksum.c
@@ -11,6 +11,7 @@

 #include <stdio.h>
 #include <stdbool.h>
+#include <endian.h>
 #include <arpa/inet.h>
 #include <netinet/ip.h>
 #include <netinet/ip6.h>
@@ -26,8 +27,13 @@ uint16_t nfq_checksum(uint32_t sum, uint16_t *buf, int size)
  sum += *buf++;
  size -= sizeof(uint16_t);
  }
- if (size)
- sum += *(uint8_t *)buf;
+ if (size) {
+#if __BYTE_ORDER == __BIG_ENDIAN
+ sum += (uint16_t)*(uint8_t *)buf << 8;
+#else
+ sum += (uint16_t)*(uint8_t *)buf;
+#endif
+ }

  sum = (sum >> 16) + (sum & 0xffff);
  sum += (sum >>16);