Re: [musl] gethostbyname2_r returns invalid IPv6 address if DNS server replies IPv4 address

[Rich Felker <dalias@libc.org>]

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

On Wed, Oct 19, 2022 at 11:44:45AM +0800, Tom Shen wrote:
> Thanks for replying!
> 
> > gethostbyname2 is legacy and has lots of inherent problems, so this
> > really should be changed to use getaddrinfo. If nothing else, such a
> > change would make it finish twice as fast.
> 
> Ya, it is obsolete as stated in Linux doc. But a third-party service is
> using "getent hosts" which calls this API.

Right. I'm saying the maintainers of this version of getent should be
informed that they're using a backwards nonstandard function here and
that it would work better with getaddrinfo.

> > Are you saying that the configured nameserver (coredns) is responding
> >   with an answer packet for a different question (A instead of AAAA)
> >   than the query packet asked for? I'm confused why it would do that.
> 
> >   It sounds like this is also wrong behavior by coredns. If it's
> >   rewriting AAAA queries to A for the sake of determining if the name
> >   exists vs NxDomain, that's fine for the outgoing query it performs,
> >   but instead of returning the answer to the rewritten query back to the
> >   asker as an answer that mismatches the question, it should just be
> >   removing the answer section to make it into a NODATA answer (if it's
> >   not already NxDomain).
> 
> Yes. In CoreDNS, we can configure it to rewrite the request type AAAA to A,
> so that the response will be IPv4 family and address. This is a CoreDNS
> feature. I cannot find anything in the DNS standards against it so far.

I inquired about this and got the response that there might not be an
explicit rule against it, "much like no RFC prohibits you from eating
glass". :-) I don't see how the behavior makes any sense though. There
is a risk that strict stub resolvers could consider these packets
non-answers and timeout/fail rather than returning a response, and
like you found, a chance that buggy stub resolvers not doing
sufficient validation could misinterpret the answers. I'm not clear
where these wrong-type answers could give useful results, especially
not anything more-useful than just returning NODATA in place of the v6
result you want to suppress.

> >   The expected behavior of a process receiving such an answer would be
> >   to ignore it as bogus, but maybe we're not doing that, and then
> >   wrongly returning answers that don't match the type we asked for?
> 
> Unfortunately, we return invalid IPv6 address as stated in the previous
> email. But, maybe it's not good to mention it here, glibc can handle this
> situation.
> 
> > I'd rather fix this at the layer the actual bug (missing validation)
> > is at.
> 
> So, let me try to suggest a fix in __lookup_name or musl team would help to
> take a look? So far I cannot find a way to create a pull request to musl
> project.

You can send patches to the list in plain diff or git format-patch
form. Attachments are preferred over inline but inline is okay too if
you're sure your mailer won't mess them up. But I have a proposed fix
here. The last hunk needs to be applied manually since it has context
affected by other DNS work I'm about to push (reversal of the for loop
direction).

Rich

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

diff --git a/src/network/lookup_name.c b/src/network/lookup_name.c
index 3eda65a7..57585b8e 100644
--- a/src/network/lookup_name.c
+++ b/src/network/lookup_name.c
@@ -102,6 +102,7 @@ struct dpc_ctx {
 	struct address *addrs;
 	char *canon;
 	int cnt;
+	int rrtype;
 };
 
 #define RR_A 1
@@ -115,12 +116,14 @@ static int dns_parse_callback(void *c, int rr, const void *data, int len, const
 	if (ctx->cnt >= MAXADDRS) return -1;
 	switch (rr) {
 	case RR_A:
+		if (rr != ctx->rrtype) return 0;
 		if (len != 4) return -1;
 		ctx->addrs[ctx->cnt].family = AF_INET;
 		ctx->addrs[ctx->cnt].scopeid = 0;
 		memcpy(ctx->addrs[ctx->cnt++].addr, data, 4);
 		break;
 	case RR_AAAA:
+		if (rr != ctx->rrtype) return 0;
 		if (len != 16) return -1;
 		ctx->addrs[ctx->cnt].family = AF_INET6;
 		ctx->addrs[ctx->cnt].scopeid = 0;
@@ -140,7 +143,7 @@ static int name_from_dns(struct address buf[static MAXADDRS], char canon[static
 	unsigned char qbuf[2][280], abuf[2][768];
 	const unsigned char *qp[2] = { qbuf[0], qbuf[1] };
 	unsigned char *ap[2] = { abuf[0], abuf[1] };
-	int qlens[2], alens[2];
+	int qlens[2], alens[2], qtypes[2];
 	int i, nq = 0;
 	struct dpc_ctx ctx = { .addrs = buf, .canon = canon };
 	static const struct { int af; int rr; } afrr[2] = {
@@ -154,6 +157,7 @@ static int name_from_dns(struct address buf[static MAXADDRS], char canon[static
 				0, 0, 0, qbuf[nq], sizeof *qbuf);
 			if (qlens[nq] == -1)
 				return 0;
+			qtypes[nq] = afrr[i].rr;
 			qbuf[nq][3] = 0; /* don't need AD flag */
 			/* Ensure query IDs are distinct. */
 			if (nq && qbuf[nq][0] == qbuf[0][0])
@@ -171,8 +175,10 @@ static int name_from_dns(struct address buf[static MAXADDRS], char canon[static
 		if ((abuf[i][3] & 15) != 0) return EAI_FAIL;
 	}
 
-	for (i=nq-1; i>=0; i--)
+	for (i=nq-1; i>=0; i--) {
+		ctx.rrtype = qtypes[i];
 		__dns_parse(abuf[i], alens[i], dns_parse_callback, &ctx);
+	}
 
 	if (ctx.cnt) return ctx.cnt;
 	return EAI_NODATA;