* [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 related [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 related [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 related [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 related [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 related [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).