connman.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] dnsproxy: Add input validation checks
@ 2022-01-25  9:00 Daniel Wagner
  2022-01-25  9:00 ` [PATCH 1/5] main: Use g_strdup for online_check_ipv{4,6}_url config Daniel Wagner
                   ` (5 more replies)
  0 siblings, 6 replies; 7+ messages in thread
From: Daniel Wagner @ 2022-01-25  9:00 UTC (permalink / raw)
  To: connman; +Cc: 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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/5] main: Use g_strdup for online_check_ipv{4,6}_url config
  2022-01-25  9:00 [PATCH 0/5] dnsproxy: Add input validation checks Daniel Wagner
@ 2022-01-25  9:00 ` Daniel Wagner
  2022-01-25  9:00 ` [PATCH 2/5] dnsproxy: Update TCP length header Daniel Wagner
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Daniel Wagner @ 2022-01-25  9:00 UTC (permalink / raw)
  To: connman; +Cc: 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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 2/5] dnsproxy: Update TCP length header
  2022-01-25  9:00 [PATCH 0/5] dnsproxy: Add input validation checks Daniel Wagner
  2022-01-25  9:00 ` [PATCH 1/5] main: Use g_strdup for online_check_ipv{4,6}_url config Daniel Wagner
@ 2022-01-25  9:00 ` Daniel Wagner
  2022-01-25  9:00 ` [PATCH 3/5] dnsproxy: Validate input data before using them Daniel Wagner
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Daniel Wagner @ 2022-01-25  9:00 UTC (permalink / raw)
  To: connman; +Cc: Matthias Gerstner

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 3/5] dnsproxy: Validate input data before using them
  2022-01-25  9:00 [PATCH 0/5] dnsproxy: Add input validation checks Daniel Wagner
  2022-01-25  9:00 ` [PATCH 1/5] main: Use g_strdup for online_check_ipv{4,6}_url config Daniel Wagner
  2022-01-25  9:00 ` [PATCH 2/5] dnsproxy: Update TCP length header Daniel Wagner
@ 2022-01-25  9:00 ` Daniel Wagner
  2022-01-25  9:00 ` [PATCH 4/5] dnsproxy: Avoid 100 % busy loop in TCP server case Daniel Wagner
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Daniel Wagner @ 2022-01-25  9:00 UTC (permalink / raw)
  To: connman; +Cc: 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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 4/5] dnsproxy: Avoid 100 % busy loop in TCP server case
  2022-01-25  9:00 [PATCH 0/5] dnsproxy: Add input validation checks Daniel Wagner
                   ` (2 preceding siblings ...)
  2022-01-25  9:00 ` [PATCH 3/5] dnsproxy: Validate input data before using them Daniel Wagner
@ 2022-01-25  9:00 ` Daniel Wagner
  2022-01-25  9:00 ` [PATCH 5/5] dnsproxy: Keep timeout in TCP case even after connection is established Daniel Wagner
  2022-01-25  9:10 ` [PATCH 0/5] dnsproxy: Add input validation checks Daniel Wagner
  5 siblings, 0 replies; 7+ messages in thread
From: Daniel Wagner @ 2022-01-25  9:00 UTC (permalink / raw)
  To: connman; +Cc: Matthias Gerstner

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 5/5] dnsproxy: Keep timeout in TCP case even after connection is established
  2022-01-25  9:00 [PATCH 0/5] dnsproxy: Add input validation checks Daniel Wagner
                   ` (3 preceding siblings ...)
  2022-01-25  9:00 ` [PATCH 4/5] dnsproxy: Avoid 100 % busy loop in TCP server case Daniel Wagner
@ 2022-01-25  9:00 ` Daniel Wagner
  2022-01-25  9:10 ` [PATCH 0/5] dnsproxy: Add input validation checks Daniel Wagner
  5 siblings, 0 replies; 7+ messages in thread
From: Daniel Wagner @ 2022-01-25  9:00 UTC (permalink / raw)
  To: connman; +Cc: Matthias Gerstner

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 0/5] dnsproxy: Add input validation checks
  2022-01-25  9:00 [PATCH 0/5] dnsproxy: Add input validation checks Daniel Wagner
                   ` (4 preceding siblings ...)
  2022-01-25  9:00 ` [PATCH 5/5] dnsproxy: Keep timeout in TCP case even after connection is established Daniel Wagner
@ 2022-01-25  9:10 ` Daniel Wagner
  5 siblings, 0 replies; 7+ messages in thread
From: Daniel Wagner @ 2022-01-25  9:10 UTC (permalink / raw)
  To: connman, 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>

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2022-01-25  9:11 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-25  9:00 [PATCH 0/5] dnsproxy: Add input validation checks Daniel Wagner
2022-01-25  9:00 ` [PATCH 1/5] main: Use g_strdup for online_check_ipv{4,6}_url config Daniel Wagner
2022-01-25  9:00 ` [PATCH 2/5] dnsproxy: Update TCP length header Daniel Wagner
2022-01-25  9:00 ` [PATCH 3/5] dnsproxy: Validate input data before using them Daniel Wagner
2022-01-25  9:00 ` [PATCH 4/5] dnsproxy: Avoid 100 % busy loop in TCP server case Daniel Wagner
2022-01-25  9:00 ` [PATCH 5/5] dnsproxy: Keep timeout in TCP case even after connection is established Daniel Wagner
2022-01-25  9:10 ` [PATCH 0/5] dnsproxy: Add input validation checks Daniel Wagner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).