[musl] Erroneous rejection of pointers in __dns_parse

[musl] Erroneous rejection of pointers in __dns_parse

[Markus Wichmann]

Hi all,

__dns_parse() must skip over all domain names in the package as part of
its operation, and it also checks if the domain names end in a pointer,
and the pointer has an offset larger than 510, because then it also
returns failure immediately. That is probably from before the TCP merge,
when the response buffer was a fixed 512 bytes. Now it is 768, so
pointers can have an offset of up to 766. Except they cannot have an
offset larger than rlen-2 in any case.

I am not quite sure what the point of invalid pointer detection in
__dns_parse() is, given that if the name ever actually matters,
__dn_expand() will reject it in its operation. But the hardcoded limit
in __dns_parse() means that packages from TCP cannot contain pointers
that reference the last third of the buffer.

On a related note, I see that a malformed packet can send __dn_expand()
into an infinite loop: If a pointer points to another pointer, they can
form a loop. The loop can be arbitrarily complex, so history tracking
would do no good. I think it would be a good idea to reject pointers to
pointers in that function. Because then every pointer must cause at
least two bytes to be written to the destination buffer, so it would be
exhausted at some point, and that's also an abort condition.

Ciao,
Markus

Re: [musl] Erroneous rejection of pointers in __dns_parse

[Rich Felker]

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

On Sun, Jul 16, 2023 at 08:58:04AM +0200, Markus Wichmann wrote:
> Hi all,
> 
> __dns_parse() must skip over all domain names in the package as part of
> its operation, and it also checks if the domain names end in a pointer,
> and the pointer has an offset larger than 510, because then it also
> returns failure immediately. That is probably from before the TCP merge,
> when the response buffer was a fixed 512 bytes. Now it is 768, so
> pointers can have an offset of up to 766. Except they cannot have an
> offset larger than rlen-2 in any case.

Following commit 12590c8bbd04ea484cee86812e2258fbdfca0e59, does the
attached fix seem ok?

> I am not quite sure what the point of invalid pointer detection in
> __dns_parse() is, given that if the name ever actually matters,
> __dn_expand() will reject it in its operation. But the hardcoded limit
> in __dns_parse() means that packages from TCP cannot contain pointers
> that reference the last third of the buffer.
> 
> On a related note, I see that a malformed packet can send __dn_expand()
> into an infinite loop: If a pointer points to another pointer, they can
> form a loop. The loop can be arbitrarily complex, so history tracking
> would do no good. I think it would be a good idea to reject pointers to
> pointers in that function. Because then every pointer must cause at
> least two bytes to be written to the destination buffer, so it would be
> exhausted at some point, and that's also an abort condition.

The comment on line 11 indicates how the loop is precluded. Do you
think it's incorrect?

Rich

[-- Attachment #2: dns_parse.diff --]
[-- Type: text/plain, Size: 667 bytes --]

diff --git a/src/network/dns_parse.c b/src/network/dns_parse.c
index 7f83e791..ea1ec126 100644
--- a/src/network/dns_parse.c
+++ b/src/network/dns_parse.c
@@ -15,13 +15,13 @@ 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>r+rlen-6 || *p>193 || (*p==193 && p[1]>254))
+		if (p>r+rlen-6)
 			return -1;
 		p += 5 + !!*p;
 	}
 	while (ancount--) {
 		while (p-r < rlen && *p-1U < 127) p++;
-		if (p>r+rlen-12 || *p>193 || (*p==193 && p[1]>254))
+		if (p>r+rlen-12)
 			return -1;
 		p += 1 + !!*p;
 		len = p[8]*256 + p[9];

Re: [musl] Erroneous rejection of pointers in __dns_parse

[Markus Wichmann]

Am Mon, Jul 17, 2023 at 02:48:18PM -0400 schrieb Rich Felker:
> Following commit 12590c8bbd04ea484cee86812e2258fbdfca0e59, does the
> attached fix seem ok?
>

Yes, this does solve the problem.

> The comment on line 11 indicates how the loop is precluded. Do you
> think it's incorrect?
>

I somehow totally missed that line. Of course, that does limit the
number of iterations correctly.

Ciao,
Markus