[PATCH 0/5] dnsproxy: Add input validation checks

[PATCH 0/5] dnsproxy: Add input validation checks

[Daniel Wagner]

Matthias Gerstner was busy testing the dnsproxy code and found a bunch
of bugs. With a lot of input and help from him we came up with the
following patches.

dnsproxy.c is in a pretty bad shape and needs a complete rewrite with
proper tests added. This will take time and brave soul to do so.
Though things aren't that bad as there is the option to use
systemd-resolved as DNS proxy. I haven't looked at the code of it but
I am pretty sure it is way better than dnsproxy.c :)

Anyway, I'd like to thank Matthias for his excellent support and
contributions fixing up these bugs.

Thanks,
Daniel

Daniel Wagner (2):
  main: Use g_strdup for online_check_ipv{4,6}_url config
  dnsproxy: Validate input data before using them

Matthias Gerstner (3):
  dnsproxy: Update TCP length header
  dnsproxy: Avoid 100 % busy loop in TCP server case
  dnsproxy: Keep timeout in TCP case even after connection is
    established

 src/dnsproxy.c | 51 ++++++++++++++++++++++++++++++++++++++++----------
 src/main.c     | 11 +++++++++--
 2 files changed, 50 insertions(+), 12 deletions(-)

-- 
2.34.1

[PATCH 1/5] main: Use g_strdup for online_check_ipv{4,6}_url config

[Daniel Wagner]

We are using g_free() on ConnMan's exist path, hence we would try to
free non malloc memory.
---
 src/main.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/main.c b/src/main.c
index b8c9560af177..e209cf261300 100644
--- a/src/main.c
+++ b/src/main.c
@@ -127,8 +127,8 @@ static struct {
 	.vendor_class_id = NULL,
 	.enable_online_check = true,
 	.enable_online_to_ready_transition = false,
-	.online_check_ipv4_url = DEFAULT_ONLINE_CHECK_IPV4_URL,
-	.online_check_ipv6_url = DEFAULT_ONLINE_CHECK_IPV6_URL,
+	.online_check_ipv4_url = NULL,
+	.online_check_ipv6_url = NULL,
 	.online_check_initial_interval = DEFAULT_ONLINE_CHECK_INITIAL_INTERVAL,
 	.online_check_max_interval = DEFAULT_ONLINE_CHECK_MAX_INTERVAL,
 	.auto_connect_roaming_services = false,
@@ -503,6 +503,9 @@ static void parse_config(GKeyFile *config)
 					CONF_ONLINE_CHECK_IPV4_URL, &error);
 	if (!error)
 		connman_settings.online_check_ipv4_url = string;
+	else
+		connman_settings.online_check_ipv4_url =
+			g_strdup(DEFAULT_ONLINE_CHECK_IPV4_URL);
 
 	g_clear_error(&error);
 
@@ -510,6 +513,10 @@ static void parse_config(GKeyFile *config)
 					CONF_ONLINE_CHECK_IPV6_URL, &error);
 	if (!error)
 		connman_settings.online_check_ipv6_url = string;
+	else
+		connman_settings.online_check_ipv6_url =
+			g_strdup(DEFAULT_ONLINE_CHECK_IPV6_URL);
+
 
 	g_clear_error(&error);
 
-- 
2.34.1

[PATCH 2/5] dnsproxy: Update TCP length header

[Daniel Wagner]

From: Matthias Gerstner <mgerstner@suse.de>

---
 src/dnsproxy.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index fbbee0413f8f..cdfafbc292f2 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -2166,6 +2166,9 @@ static int forward_dns_reply(unsigned char *reply, int reply_len, int protocol,
 			err = sendto(sk, req->resp, req->resplen, 0,
 				&req->sa, req->sa_len);
 	} else {
+		uint16_t tcp_len = htons(req->resplen - 2);
+		/* correct TCP message length */
+		memcpy(req->resp, &tcp_len, sizeof(tcp_len));
 		sk = req->client_sk;
 		err = send(sk, req->resp, req->resplen, MSG_NOSIGNAL);
 	}
-- 
2.34.1

[PATCH 3/5] dnsproxy: Validate input data before using them

[Daniel Wagner]

dnsproxy is not validating various input data. Add a bunch of checks.

Fixes: CVE-2022-23097
Fixes: CVE-2022-23096
---
 src/dnsproxy.c | 31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)

diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index cdfafbc292f2..c027bcb972c4 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -1951,6 +1951,12 @@ static int forward_dns_reply(unsigned char *reply, int reply_len, int protocol,
 
 	if (offset < 0)
 		return offset;
+	if (reply_len < 0)
+		return -EINVAL;
+	if (reply_len < offset + 1)
+		return -EINVAL;
+	if ((size_t)reply_len < sizeof(struct domain_hdr))
+		return -EINVAL;
 
 	hdr = (void *)(reply + offset);
 	dns_id = reply[offset] | reply[offset + 1] << 8;
@@ -1986,23 +1992,31 @@ static int forward_dns_reply(unsigned char *reply, int reply_len, int protocol,
 		 */
 		if (req->append_domain && ntohs(hdr->qdcount) == 1) {
 			uint16_t domain_len = 0;
-			uint16_t header_len;
+			uint16_t header_len, payload_len;
 			uint16_t dns_type, dns_class;
 			uint8_t host_len, dns_type_pos;
 			char uncompressed[NS_MAXDNAME], *uptr;
 			char *ptr, *eom = (char *)reply + reply_len;
+			char *domain;
 
 			/*
 			 * ptr points to the first char of the hostname.
 			 * ->hostname.domain.net
 			 */
 			header_len = offset + sizeof(struct domain_hdr);
+			if (reply_len < header_len)
+				return -EINVAL;
+			payload_len = reply_len - header_len;
+
 			ptr = (char *)reply + header_len;
 
 			host_len = *ptr;
+			domain = ptr + 1 + host_len;
+			if (domain > eom)
+				return -EINVAL;
+
 			if (host_len > 0)
-				domain_len = strnlen(ptr + 1 + host_len,
-						reply_len - header_len);
+				domain_len = strnlen(domain, eom - domain);
 
 			/*
 			 * If the query type is anything other than A or AAAA,
@@ -2011,6 +2025,8 @@ static int forward_dns_reply(unsigned char *reply, int reply_len, int protocol,
 			 */
 			dns_type_pos = host_len + 1 + domain_len + 1;
 
+			if (ptr + (dns_type_pos + 3) > eom)
+				return -EINVAL;
 			dns_type = ptr[dns_type_pos] << 8 |
 							ptr[dns_type_pos + 1];
 			dns_class = ptr[dns_type_pos + 2] << 8 |
@@ -2040,6 +2056,8 @@ static int forward_dns_reply(unsigned char *reply, int reply_len, int protocol,
 				int new_len, fixed_len;
 				char *answers;
 
+				if (len > payload_len)
+					return -EINVAL;
 				/*
 				 * First copy host (without domain name) into
 				 * tmp buffer.
@@ -2054,6 +2072,8 @@ static int forward_dns_reply(unsigned char *reply, int reply_len, int protocol,
 				 * Copy type and class fields of the question.
 				 */
 				ptr += len + domain_len + 1;
+				if (ptr + NS_QFIXEDSZ > eom)
+					return -EINVAL;
 				memcpy(uptr, ptr, NS_QFIXEDSZ);
 
 				/*
@@ -2063,6 +2083,8 @@ static int forward_dns_reply(unsigned char *reply, int reply_len, int protocol,
 				uptr += NS_QFIXEDSZ;
 				answers = uptr;
 				fixed_len = answers - uncompressed;
+				if (ptr + offset > eom)
+					return -EINVAL;
 
 				/*
 				 * We then uncompress the result to buffer
@@ -2257,8 +2279,7 @@ static gboolean udp_server_event(GIOChannel *channel, GIOCondition condition,
 
 	len = recv(sk, buf, sizeof(buf), 0);
 
-	if (len >= 12)
-		forward_dns_reply(buf, len, IPPROTO_UDP, data);
+	forward_dns_reply(buf, len, IPPROTO_UDP, data);
 
 	return TRUE;
 }
-- 
2.34.1

[PATCH 4/5] dnsproxy: Avoid 100 % busy loop in TCP server case

[Daniel Wagner]

From: Matthias Gerstner <mgerstner@suse.de>

Once the TCP socket is connected and until the remote server is
responding (if ever) ConnMan executes a 100 % CPU loop, since
the connected socket will always be writable (G_IO_OUT).

To fix this, modify the watch after the connection is established to
remove the G_IO_OUT from the callback conditions.

Fixes: CVE-2022-23098
---
 src/dnsproxy.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index c027bcb972c4..1ccf36a95a35 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -2360,6 +2360,18 @@ static gboolean tcp_server_event(GIOChannel *channel, GIOCondition condition,
 			}
 		}
 
+		/*
+		 * Remove the G_IO_OUT flag from the watch, otherwise we end
+		 * up in a busy loop, because the socket is constantly writable.
+		 *
+		 * There seems to be no better way in g_io to do that than
+		 * re-adding the watch.
+		 */
+		g_source_remove(server->watch);
+		server->watch = g_io_add_watch(server->channel,
+			G_IO_IN | G_IO_HUP | G_IO_NVAL | G_IO_ERR,
+			tcp_server_event, server);
+
 		server->connected = true;
 		server_list = g_slist_append(server_list, server);
 
-- 
2.34.1

[PATCH 5/5] dnsproxy: Keep timeout in TCP case even after connection is established

[Daniel Wagner]

From: Matthias Gerstner <mgerstner@suse.de>

If an outgoing TCP connection succeeds but the remote server never sends
back any data then currently the TCP connection will never be
terminated by connmand.

To prevent this keep the connection timeout of 30 seconds active even
after the connection has been established.
---
 src/dnsproxy.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/src/dnsproxy.c b/src/dnsproxy.c
index 1ccf36a95a35..cf1d36c74496 100644
--- a/src/dnsproxy.c
+++ b/src/dnsproxy.c
@@ -2375,11 +2375,6 @@ static gboolean tcp_server_event(GIOChannel *channel, GIOCondition condition,
 		server->connected = true;
 		server_list = g_slist_append(server_list, server);
 
-		if (server->timeout > 0) {
-			g_source_remove(server->timeout);
-			server->timeout = 0;
-		}
-
 		for (list = request_list; list; ) {
 			struct request_data *req = list->data;
 			int status;
-- 
2.34.1

Re: [PATCH 0/5] dnsproxy: Add input validation checks

[Daniel Wagner]

On Tue, 25 Jan 2022 10:00:21 +0100, Daniel Wagner wrote:
> Matthias Gerstner was busy testing the dnsproxy code and found a bunch
> of bugs. With a lot of input and help from him we came up with the
> following patches.
> 
> dnsproxy.c is in a pretty bad shape and needs a complete rewrite with
> proper tests added. This will take time and brave soul to do so.
> Though things aren't that bad as there is the option to use
> systemd-resolved as DNS proxy. I haven't looked at the code of it but
> I am pretty sure it is way better than dnsproxy.c :)
> 
> [...]

Applied, thanks!

[1/5] main: Use g_strdup for online_check_ipv{4,6}_url config
      commit: 8bed0e22cb59468e773b247724a114d6764bd0a6
[2/5] dnsproxy: Update TCP length header
      commit: f65b6c233dd9f91723ea6993dca59fcf303d001b
[3/5] dnsproxy: Validate input data before using them
      commit: e5a313736e13c90d19085e953a26256a198e4950
[4/5] dnsproxy: Avoid 100 % busy loop in TCP server case
      commit: d8708b85c1e8fe25af7803e8a20cf20e7201d8a4
[5/5] dnsproxy: Keep timeout in TCP case even after connection is established
      commit: 5c34313a196515c80fe78a2862ad78174b985be5

Best regards,
-- 
Daniel Wagner <wagi@monom.org>