[PATCH v5] resolver: mitigate bad interactions concering inconsistent DNS search domains with ndots usage

[PATCH v5] resolver: mitigate bad interactions concering inconsistent DNS search domains with ndots usage

[William Pitcock]

When using kubernetes, the search path is configured like so:

  - containername.prod.env.whatever
  - prod.env.whatever
  - env.whatever
  - clusterwide domain

Kubernetes also typically configures the container's resolver with "options ndots:5", which
causes it to look for public domains in the private DNS domains (presumably so their DNS
records can be overridden somehow).

In certain cases where the Kubernetes guest is configured with a clusterwide domain that is
hosted by a certain large CDN provider (*ahem* Cloudflare), the resolver may process
erroneous replies sent from that CDN provider that have an empty A/AAAA record set.

Accordingly, if we detect a configuration that is exotic, as a mitigation, we force all DNS
queries to behave as if they were AF_UNSPEC and return only the records that were actually
requested (either A or AAAA).
---
 src/network/lookup_name.c | 35 ++++++++++++++++++++++++++++++-----
 1 file changed, 30 insertions(+), 5 deletions(-)

diff --git a/src/network/lookup_name.c b/src/network/lookup_name.c
index 209c20f0..b28b6208 100644
--- a/src/network/lookup_name.c
+++ b/src/network/lookup_name.c
@@ -95,7 +95,9 @@ static int name_from_hosts(struct address buf[static MAXADDRS], char canon[stati
 struct dpc_ctx {
 	struct address *addrs;
 	char *canon;
+	int wanted;
 	int cnt;
+	int recordcnt[2];
 };
 
 int __dns_parse(const unsigned char *, int, int (*)(void *, int, const void *, int, const void *), void *);
@@ -115,12 +117,16 @@ static int dns_parse_callback(void *c, int rr, const void *data, int len, const
 	switch (rr) {
 	case RR_A:
 		if (len != 4) return -1;
+		ctx->recordcnt[0]++;
+		if (ctx->wanted && rr != ctx->wanted) return 0;
 		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 (len != 16) return -1;
+		ctx->recordcnt[1]++;
+		if (ctx->wanted && rr != ctx->wanted) return 0;
 		ctx->addrs[ctx->cnt].family = AF_INET6;
 		ctx->addrs[ctx->cnt].scopeid = 0;
 		memcpy(ctx->addrs[ctx->cnt++].addr, data, 16);
@@ -134,7 +140,7 @@ static int dns_parse_callback(void *c, int rr, const void *data, int len, const
 	return 0;
 }
 
-static int name_from_dns(struct address buf[static MAXADDRS], char canon[static 256], const char *name, int family, const struct resolvconf *conf)
+static int name_from_dns(struct address buf[static MAXADDRS], char canon[static 256], const char *name, int family, const struct resolvconf *conf, int searchpath_len)
 {
 	unsigned char qbuf[2][280], abuf[2][512];
 	const unsigned char *qp[2] = { qbuf[0], qbuf[1] };
@@ -148,7 +154,12 @@ static int name_from_dns(struct address buf[static MAXADDRS], char canon[static
 	};
 
 	for (i=0; i<2; i++) {
-		if (family != afrr[i].af) {
+		if (family && family != afrr[i].af) ctx.wanted = afrr[i].rr;
+
+		/* If we are using search paths longer than 1 domain, or we have changed the
+		 * ndots setting to be greater than 1, then we need to always treat the query
+		 * as if it were AF_UNSPEC to ensure results are consistent. */
+		if (family != afrr[i].af || searchpath_len > 1 || conf->ndots > 1) {
 			qlens[nq] = __res_mkquery(0, name, 1, afrr[i].rr,
 				0, 0, 0, qbuf[nq], sizeof *qbuf);
 			if (qlens[nq] == -1)
@@ -165,7 +176,14 @@ static int name_from_dns(struct address buf[static MAXADDRS], char canon[static
 
 	if (ctx.cnt) return ctx.cnt;
 	if (alens[0] < 4 || (abuf[0][3] & 15) == 2) return EAI_AGAIN;
-	if ((abuf[0][3] & 15) == 0) return EAI_NONAME;
+	if ((abuf[0][3] & 15) == 0) {
+		/* A certain large CDN provider's DNS service erroneously responds to queries with
+		 * a NOERROR(0) response code, while also returning an empty record set. Accordingly,
+		 * check for this and handle it as we would an NXDOMAIN(3) if the record set is empty
+		 * for both A and AAAA records. */
+		if (nq == 2 && (ctx.recordcnt[0] + ctx.recordcnt[1]) == 0) return 0;
+		else return EAI_NONAME;
+	}
 	if ((abuf[0][3] & 15) == 3) return 0;
 	return EAI_FAIL;
 }
@@ -175,6 +193,7 @@ static int name_from_dns_search(struct address buf[static MAXADDRS], char canon[
 	char search[256];
 	struct resolvconf conf;
 	size_t l, dots;
+	int searchpath_len = 0;
 	char *p, *z;
 
 	if (__get_resolv_conf(&conf, search, sizeof search) < 0) return -1;
@@ -184,6 +203,12 @@ static int name_from_dns_search(struct address buf[static MAXADDRS], char canon[
 	for (dots=l=0; name[l]; l++) if (name[l]=='.') dots++;
 	if (dots >= conf.ndots || name[l-1]=='.') *search = 0;
 
+	/* Count the number of domains in the search path. */
+	if (*search) {
+		size_t n;
+		for (searchpath_len = n = 0; search[n]; n++) if (isspace(search[n])) searchpath_len++;
+	}
+
 	/* This can never happen; the caller already checked length. */
 	if (l >= 256) return EAI_NONAME;
 
@@ -201,13 +226,13 @@ static int name_from_dns_search(struct address buf[static MAXADDRS], char canon[
 		if (z-p < 256 - l - 1) {
 			memcpy(canon+l+1, p, z-p);
 			canon[z-p+1+l] = 0;
-			int cnt = name_from_dns(buf, canon, canon, family, &conf);
+			int cnt = name_from_dns(buf, canon, canon, family, &conf, searchpath_len);
 			if (cnt) return cnt;
 		}
 	}
 
 	canon[l] = 0;
-	return name_from_dns(buf, canon, name, family, &conf);
+	return name_from_dns(buf, canon, name, family, &conf, searchpath_len);
 }
 
 static const struct policy {
-- 
2.16.3

Re: [PATCH v5] resolver: mitigate bad interactions concering inconsistent DNS search domains with ndots usage

[Eric Pruitt]

On Sat, Mar 31, 2018 at 09:40:04AM +0000, William Pitcock wrote:
> In certain cases where the Kubernetes guest is configured with a clusterwide domain that is
> hosted by a certain large CDN provider (*ahem* Cloudflare), the resolver may process
> erroneous replies sent from that CDN provider that have an empty A/AAAA record set.
> [...]
> -	if ((abuf[0][3] & 15) == 0) return EAI_NONAME;
> +	if ((abuf[0][3] & 15) == 0) {
> +		/* A certain large CDN provider's DNS service erroneously responds to queries with
> +		 * a NOERROR(0) response code, while also returning an empty record set. Accordingly,
> +		 * check for this and handle it as we would an NXDOMAIN(3) if the record set is empty
> +		 * for both A and AAAA records. */
> +		if (nq == 2 && (ctx.recordcnt[0] + ctx.recordcnt[1]) == 0) return 0;
> +		else return EAI_NONAME;

If you're going to call out Cloudflare in the commit message, why not do
it in the code comment, too? If someone runs into this later without
having read this mailing list post and they're using a release copy of
musl (something without revision history like a tar ball), poorly
obscuring Cloudflare's name just adds unnecessary friction to debugging
the problem.

Eric

Re: [PATCH v5] resolver: mitigate bad interactions concering inconsistent DNS search domains with ndots usage

[Rich Felker]

On Sat, Mar 31, 2018 at 10:22:32AM -0700, Eric Pruitt wrote:
> On Sat, Mar 31, 2018 at 09:40:04AM +0000, William Pitcock wrote:
> > In certain cases where the Kubernetes guest is configured with a clusterwide domain that is
> > hosted by a certain large CDN provider (*ahem* Cloudflare), the resolver may process
> > erroneous replies sent from that CDN provider that have an empty A/AAAA record set.
> > [...]
> > -	if ((abuf[0][3] & 15) == 0) return EAI_NONAME;
> > +	if ((abuf[0][3] & 15) == 0) {
> > +		/* A certain large CDN provider's DNS service erroneously responds to queries with
> > +		 * a NOERROR(0) response code, while also returning an empty record set. Accordingly,
> > +		 * check for this and handle it as we would an NXDOMAIN(3) if the record set is empty
> > +		 * for both A and AAAA records. */
> > +		if (nq == 2 && (ctx.recordcnt[0] + ctx.recordcnt[1]) == 0) return 0;
> > +		else return EAI_NONAME;
> 
> If you're going to call out Cloudflare in the commit message, why not do
> it in the code comment, too? If someone runs into this later without
> having read this mailing list post and they're using a release copy of
> musl (something without revision history like a tar ball), poorly
> obscuring Cloudflare's name just adds unnecessary friction to debugging
> the problem.

Generally names of services/users/products are not put in either the
commit log or comments in musl. The main exception is toolchain
(compiler, binutils) stuff where we're talking about explicit
interactions with those components.

Rich