[PATCH] udp: Fix port and address checks for DNS forwarder

[PATCH] udp: Fix port and address checks for DNS forwarder

[Stefano Brivio]

First off, as we swap endianness for source ports in
udp_fill_data_v{4,6}(), we want host endianness, not network
endianness. It doesn't actually matter if we use htons() or ntohs()
here, but the current version is confusing.

In the IPv4 path, when we remap DNS answers, we already swapped the
endianness as needed for the source port: don't swap it again,
otherwise we'll not map DNS answers for IPv4.

In the IPv6 path, when we remap DNS answers, we want to check that
they came from our upstream DNS server, not the one configured via
--dns-forward (which doesn't even need to exist for this
functionality to work).
---
 udp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/udp.c b/udp.c
index cac9c65..4b201d3 100644
--- a/udp.c
+++ b/udp.c
@@ -678,7 +678,7 @@ static void udp_sock_fill_data_v4(const struct ctx *c, int n,
 	b->iph.tot_len = htons(ip_len);
 
 	src = ntohl(b->s_in.sin_addr.s_addr);
-	src_port = htons(b->s_in.sin_port);
+	src_port = ntohs(b->s_in.sin_port);
 
 	if (src >> IN_CLASSA_NSHIFT == IN_LOOPBACKNET ||
 	    src == INADDR_ANY || src == ntohl(c->ip4.addr_seen)) {
@@ -693,7 +693,7 @@ static void udp_sock_fill_data_v4(const struct ctx *c, int n,
 
 		bitmap_set(udp_act[V4][UDP_ACT_TAP], src_port);
 	} else if (c->ip4.dns_fwd &&
-		   src == ntohl(c->ip4.dns[0]) && ntohs(src_port) == 53) {
+		   src == htonl(c->ip4.dns[0]) && src_port == 53) {
 		b->iph.saddr = c->ip4.dns_fwd;
 	} else {
 		b->iph.saddr = b->s_in.sin_addr.s_addr;
@@ -795,7 +795,7 @@ static void udp_sock_fill_data_v6(const struct ctx *c, int n,
 
 		bitmap_set(udp_act[V6][UDP_ACT_TAP], src_port);
 	} else if (!IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns_fwd) &&
-		   IN6_ARE_ADDR_EQUAL(src, &c->ip6.dns_fwd) && src_port == 53) {
+		   IN6_ARE_ADDR_EQUAL(src, &c->ip6.dns[0]) && src_port == 53) {
 		b->ip6h.daddr = c->ip6.addr_seen;
 		b->ip6h.saddr = c->ip6.dns_fwd;
 	} else {
-- 
@@ -678,7 +678,7 @@ static void udp_sock_fill_data_v4(const struct ctx *c, int n,
 	b->iph.tot_len = htons(ip_len);
 
 	src = ntohl(b->s_in.sin_addr.s_addr);
-	src_port = htons(b->s_in.sin_port);
+	src_port = ntohs(b->s_in.sin_port);
 
 	if (src >> IN_CLASSA_NSHIFT == IN_LOOPBACKNET ||
 	    src == INADDR_ANY || src == ntohl(c->ip4.addr_seen)) {
@@ -693,7 +693,7 @@ static void udp_sock_fill_data_v4(const struct ctx *c, int n,
 
 		bitmap_set(udp_act[V4][UDP_ACT_TAP], src_port);
 	} else if (c->ip4.dns_fwd &&
-		   src == ntohl(c->ip4.dns[0]) && ntohs(src_port) == 53) {
+		   src == htonl(c->ip4.dns[0]) && src_port == 53) {
 		b->iph.saddr = c->ip4.dns_fwd;
 	} else {
 		b->iph.saddr = b->s_in.sin_addr.s_addr;
@@ -795,7 +795,7 @@ static void udp_sock_fill_data_v6(const struct ctx *c, int n,
 
 		bitmap_set(udp_act[V6][UDP_ACT_TAP], src_port);
 	} else if (!IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns_fwd) &&
-		   IN6_ARE_ADDR_EQUAL(src, &c->ip6.dns_fwd) && src_port == 53) {
+		   IN6_ARE_ADDR_EQUAL(src, &c->ip6.dns[0]) && src_port == 53) {
 		b->ip6h.daddr = c->ip6.addr_seen;
 		b->ip6h.saddr = c->ip6.dns_fwd;
 	} else {
-- 
2.35.1

Re: [PATCH] udp: Fix port and address checks for DNS forwarder

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2436 bytes --]

On Mon, Oct 10, 2022 at 10:21:09AM +0200, Stefano Brivio wrote:
> First off, as we swap endianness for source ports in
> udp_fill_data_v{4,6}(), we want host endianness, not network
> endianness. It doesn't actually matter if we use htons() or ntohs()
> here, but the current version is confusing.
> 
> In the IPv4 path, when we remap DNS answers, we already swapped the
> endianness as needed for the source port: don't swap it again,
> otherwise we'll not map DNS answers for IPv4.
> 
> In the IPv6 path, when we remap DNS answers, we want to check that
> they came from our upstream DNS server, not the one configured via
> --dns-forward (which doesn't even need to exist for this
> functionality to work).

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

..although you appear to have forgotten your S-o-b on this one.

> ---
>  udp.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/udp.c b/udp.c
> index cac9c65..4b201d3 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -678,7 +678,7 @@ static void udp_sock_fill_data_v4(const struct ctx *c, int n,
>  	b->iph.tot_len = htons(ip_len);
>  
>  	src = ntohl(b->s_in.sin_addr.s_addr);
> -	src_port = htons(b->s_in.sin_port);
> +	src_port = ntohs(b->s_in.sin_port);
>  
>  	if (src >> IN_CLASSA_NSHIFT == IN_LOOPBACKNET ||
>  	    src == INADDR_ANY || src == ntohl(c->ip4.addr_seen)) {
> @@ -693,7 +693,7 @@ static void udp_sock_fill_data_v4(const struct ctx *c, int n,
>  
>  		bitmap_set(udp_act[V4][UDP_ACT_TAP], src_port);
>  	} else if (c->ip4.dns_fwd &&
> -		   src == ntohl(c->ip4.dns[0]) && ntohs(src_port) == 53) {
> +		   src == htonl(c->ip4.dns[0]) && src_port == 53) {
>  		b->iph.saddr = c->ip4.dns_fwd;
>  	} else {
>  		b->iph.saddr = b->s_in.sin_addr.s_addr;
> @@ -795,7 +795,7 @@ static void udp_sock_fill_data_v6(const struct ctx *c, int n,
>  
>  		bitmap_set(udp_act[V6][UDP_ACT_TAP], src_port);
>  	} else if (!IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns_fwd) &&
> -		   IN6_ARE_ADDR_EQUAL(src, &c->ip6.dns_fwd) && src_port == 53) {
> +		   IN6_ARE_ADDR_EQUAL(src, &c->ip6.dns[0]) && src_port == 53) {
>  		b->ip6h.daddr = c->ip6.addr_seen;
>  		b->ip6h.saddr = c->ip6.dns_fwd;
>  	} else {

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]