mailing list of musl libc
 help / color / mirror / code / Atom feed
* [musl] Out-of-bounds reads in DNS response parsing
@ 2023-02-23 20:34 Alexey Izbyshev
  0 siblings, 0 replies; only message in thread
From: Alexey Izbyshev @ 2023-02-23 20:34 UTC (permalink / raw)
  To: musl

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

Hi,

I've found several issues with DNS response parsing that can result in 
getaddrinfo/getnameinfo reading unininitialized or (nearby) 
out-of-bounds data on stack and returning garbage. The issues are 
described in the attached patches.

Alexey

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-fix-out-of-bounds-reads-in-__dns_parse.patch --]
[-- Type: text/x-diff; name=0001-fix-out-of-bounds-reads-in-__dns_parse.patch, Size: 2247 bytes --]

From bbbbd53761b484422c7031fe97ad7ea92a0bdb04 Mon Sep 17 00:00:00 2001
From: Alexey Izbyshev <izbyshev@ispras.ru>
Date: Sat, 28 Jan 2023 00:17:37 +0300
Subject: [PATCH 1/2] fix out-of-bounds reads in __dns_parse
Mail-Followup-To: musl@lists.openwall.com

There are several issues with range checks in this function:

* The question section parsing loop can read up to two out-of-bounds
  bytes before doing the range check and bailing out.

* The answer section parsing loop, in addition to the same issue as
  above, uses the wrong length in the range check that doesn't prevent
  OOB reads when computing len later.

* The len range check before calling the callback is off by 10. Also,
  p+len can overflow in a (probably theoretical) case when p is within
  2^16 from UINTPTR_MAX.

Because __dns_parse is used only with stack-allocated buffers, such
small overreads can't result in a segfault. The first two also don't
affect the function result, but the last one may result in getaddrinfo
incorrectly succeeding and returning up to 10 bytes past the
response buffer as a part of the IP address, and in (canon) name
returned by getaddrinfo/getnameinfo being affected by memory past the
response buffer (because dn_expand might interpret it as a pointer).
---
 src/network/dns_parse.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/network/dns_parse.c b/src/network/dns_parse.c
index e6ee19d9..320df60d 100644
--- a/src/network/dns_parse.c
+++ b/src/network/dns_parse.c
@@ -15,17 +15,17 @@ int __dns_parse(const unsigned char *r, int rlen, int (*callback)(void *, int, c
 	if (qdcount+ancount > 64) return -1;
 	while (qdcount--) {
 		while (p-r < rlen && *p-1U < 127) p++;
-		if (*p>193 || (*p==193 && p[1]>254) || p>r+rlen-6)
+		if (p>r+rlen-6 || *p>193 || (*p==193 && p[1]>254))
 			return -1;
 		p += 5 + !!*p;
 	}
 	while (ancount--) {
 		while (p-r < rlen && *p-1U < 127) p++;
-		if (*p>193 || (*p==193 && p[1]>254) || p>r+rlen-6)
+		if (p>r+rlen-12 || *p>193 || (*p==193 && p[1]>254))
 			return -1;
 		p += 1 + !!*p;
 		len = p[8]*256 + p[9];
-		if (p+len > r+rlen) return -1;
+		if (len+10 > r+rlen-p) return -1;
 		if (callback(ctx, p[1], p+10, len, r) < 0) return -1;
 		p += 10 + len;
 	}
-- 
2.39.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-prevent-CNAME-PTR-parsing-from-reading-data-past-the.patch --]
[-- Type: text/x-diff; name=0002-prevent-CNAME-PTR-parsing-from-reading-data-past-the.patch, Size: 3783 bytes --]

From 1eb7fd50d7c045c1203f700cb08169308435ae72 Mon Sep 17 00:00:00 2001
From: Alexey Izbyshev <izbyshev@ispras.ru>
Date: Sun, 29 Jan 2023 19:46:51 +0300
Subject: [PATCH 2/2] prevent CNAME/PTR parsing from reading data past the
 response end
Mail-Followup-To: musl@lists.openwall.com

DNS parsing callbacks pass the response buffer end instead of the actual
response end to dn_expand, so a malformed DNS response can use message
compression to make dn_expand jump past the response end and attempt to
parse uninitialized parts of that buffer, which might succeed and return
garbage.
---
 src/network/dns_parse.c   | 4 ++--
 src/network/getnameinfo.c | 4 ++--
 src/network/lookup.h      | 2 +-
 src/network/lookup_name.c | 4 ++--
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/network/dns_parse.c b/src/network/dns_parse.c
index 320df60d..7f83e791 100644
--- a/src/network/dns_parse.c
+++ b/src/network/dns_parse.c
@@ -1,7 +1,7 @@
 #include <string.h>
 #include "lookup.h"
 
-int __dns_parse(const unsigned char *r, int rlen, int (*callback)(void *, int, const void *, int, const void *), void *ctx)
+int __dns_parse(const unsigned char *r, int rlen, int (*callback)(void *, int, const void *, int, const void *, int), void *ctx)
 {
 	int qdcount, ancount;
 	const unsigned char *p;
@@ -26,7 +26,7 @@ int __dns_parse(const unsigned char *r, int rlen, int (*callback)(void *, int, c
 		p += 1 + !!*p;
 		len = p[8]*256 + p[9];
 		if (len+10 > r+rlen-p) return -1;
-		if (callback(ctx, p[1], p+10, len, r) < 0) return -1;
+		if (callback(ctx, p[1], p+10, len, r, rlen) < 0) return -1;
 		p += 10 + len;
 	}
 	return 0;
diff --git a/src/network/getnameinfo.c b/src/network/getnameinfo.c
index 949e1811..080d3c06 100644
--- a/src/network/getnameinfo.c
+++ b/src/network/getnameinfo.c
@@ -108,10 +108,10 @@ static void reverse_services(char *buf, int port, int dgram)
 	__fclose_ca(f);
 }
 
-static int dns_parse_callback(void *c, int rr, const void *data, int len, const void *packet)
+static int dns_parse_callback(void *c, int rr, const void *data, int len, const void *packet, int plen)
 {
 	if (rr != RR_PTR) return 0;
-	if (__dn_expand(packet, (const unsigned char *)packet + 512,
+	if (__dn_expand(packet, (const unsigned char *)packet + plen,
 	    data, c, 256) <= 0)
 		*(char *)c = 0;
 	return 0;
diff --git a/src/network/lookup.h b/src/network/lookup.h
index ef662725..54b2f8b5 100644
--- a/src/network/lookup.h
+++ b/src/network/lookup.h
@@ -50,6 +50,6 @@ hidden int __lookup_ipliteral(struct address buf[static 1], const char *name, in
 hidden int __get_resolv_conf(struct resolvconf *, char *, size_t);
 hidden int __res_msend_rc(int, const unsigned char *const *, const int *, unsigned char *const *, int *, int, const struct resolvconf *);
 
-hidden int __dns_parse(const unsigned char *, int, int (*)(void *, int, const void *, int, const void *), void *);
+hidden int __dns_parse(const unsigned char *, int, int (*)(void *, int, const void *, int, const void *, int), void *);
 
 #endif
diff --git a/src/network/lookup_name.c b/src/network/lookup_name.c
index 5f6867cb..f268bcda 100644
--- a/src/network/lookup_name.c
+++ b/src/network/lookup_name.c
@@ -111,13 +111,13 @@ struct dpc_ctx {
 
 #define ABUF_SIZE 768
 
-static int dns_parse_callback(void *c, int rr, const void *data, int len, const void *packet)
+static int dns_parse_callback(void *c, int rr, const void *data, int len, const void *packet, int plen)
 {
 	char tmp[256];
 	int family;
 	struct dpc_ctx *ctx = c;
 	if (rr == RR_CNAME) {
-		if (__dn_expand(packet, (const unsigned char *)packet + ABUF_SIZE,
+		if (__dn_expand(packet, (const unsigned char *)packet + plen,
 		    data, tmp, sizeof tmp) > 0 && is_valid_hostname(tmp))
 			strcpy(ctx->canon, tmp);
 		return 0;
-- 
2.39.1


^ permalink raw reply	[flat|nested] only message in thread

only message in thread, other threads:[~2023-02-23 20:34 UTC | newest]

Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-23 20:34 [musl] Out-of-bounds reads in DNS response parsing Alexey Izbyshev

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/musl/

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).