NetBSD Problem Report #52578

From www@NetBSD.org  Thu Sep 28 19:44:11 2017
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 2C3F97A1FF
	for <gnats-bugs@gnats.NetBSD.org>; Thu, 28 Sep 2017 19:44:11 +0000 (UTC)
Message-Id: <20170928194409.EDD8C7A297@mollari.NetBSD.org>
Date: Thu, 28 Sep 2017 19:44:09 +0000 (UTC)
From: bemasc@google.com
Reply-To: bemasc@google.com
To: gnats-bugs@NetBSD.org
Subject: gethostbyname has EDNS fallback but getaddrinfo does not
X-Send-Pr-Version: www-1.0

>Number:         52578
>Category:       lib
>Synopsis:       gethostbyname has EDNS fallback but getaddrinfo does not
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    jdolecek
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Thu Sep 28 19:45:00 +0000 2017
>Closed-Date:    Wed Oct 25 18:04:26 +0000 2017
>Last-Modified:  Wed Oct 25 18:04:26 +0000 2017
>Originator:     Benjamin M. Schwartz
>Release:        master
>Organization:
Google LLC
>Environment:
I diagnosed this issuing on Android, which uses a fork of NetBSD's getaddrinfo.
>Description:
The EDNS0 extension allow dns clients (stub resolvers) to signal some additional metadata to the recursive resolver, especially the client's largest supported response size.  However, some old and broken DNS resolvers would return SERVFAIL or similar upon encountering any EDNS0 extension.  BIND therefore includes fallback logic (in the res_nquery function) to retry failed requests without EDNS0.  This logic is also present in NetBSD's resolver (based on BIND), and applies correctly to gethostbyname.

However, getaddrinfo doesn't use res_nquery.  Instead, it uses a function called res_queryN, which contains a modified copy of res_nquery.  This copy likely predates the introduction of the EDNS fallback mechanism, and has not been kept up to date.  As a result, getaddrinfo does not have the same fallback behavior as gethostbyname.

The obvious solution is to sync res_queryN to the current res_nquery, reducing version skew and making getaddrinfo's behavior match gethostbyname's.
>How-To-Repeat:

>Fix:
Apologies for the git-formatted patch.  I'm not used to CVS.

From e046666a4141b85585ebc6a673de83e43bdef009 Mon Sep 17 00:00:00 2001
From: Ben Schwartz <bemasc@google.com>
Date: Thu, 28 Sep 2017 10:36:44 -0400
Subject: [PATCH 1/1] Extend EDNS retry logic to getaddrinfo

Currently, if EDNS is enabled, but the query fails,
gethostbyname will retry without EDNS.  This behavior
comes directly from upstream BIND, in the res_nquery
function.  However, the corresponding behavior is not
present in res_queryN, a copy of res_nquery that is
used by getaddrinfo.  This change extends res_queryN
to match res_nquery's retry behavior.

This change also sets the AD bit when DNSSEC is enabled
(RFC 6840 Section 5.7).
---
 lib/libc/net/getaddrinfo.c    | 17 ++++++++++++++++-
 lib/libc/resolv/res_mkquery.c |  1 +
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/lib/libc/net/getaddrinfo.c b/lib/libc/net/getaddrinfo.c
index 27146aeae..6f1c60137 100644
--- a/lib/libc/net/getaddrinfo.c
+++ b/lib/libc/net/getaddrinfo.c
@@ -2558,8 +2558,12 @@ res_queryN(const char *name, /* domain name */ struct res_target *target,
 		int class, type;
 		u_char *answer;
 		int anslen;
+		u_int oflags;

 		hp = (HEADER *)(void *)t->answer;
+		oflags = res->_flags;
+
+again:
 		hp->rcode = NOERROR;	/* default */

 		/* make it easier... */
@@ -2575,7 +2579,8 @@ res_queryN(const char *name, /* domain name */ struct res_target *target,
 		n = res_nmkquery(res, QUERY, name, class, type, NULL, 0, NULL,
 		    buf, (int)sizeof(buf));
 #ifdef RES_USE_EDNS0
-		if (n > 0 && (res->options & RES_USE_EDNS0) != 0)
+		if (n > 0 && (res->_flags & RES_F_EDNS0ERR) == 0 &&
+		    (res->options & (RES_USE_EDNS0|RES_USE_DNSSEC)) != 0)
 			n = res_nopt(res, n, buf, (int)sizeof(buf), anslen);
 #endif
 		if (n <= 0) {
@@ -2600,6 +2605,16 @@ res_queryN(const char *name, /* domain name */ struct res_target *target,

 		if (n < 0 || hp->rcode != NOERROR || ntohs(hp->ancount) == 0) {
 			rcode = hp->rcode;	/* record most recent error */
+#ifdef RES_USE_EDNS0
+			/* if the query choked with EDNS0, retry without EDNS0 */
+			if ((res->options & (RES_USE_EDNS0|RES_USE_DNSSEC)) != 0 &&
+			    ((oflags ^ res->_flags) & RES_F_EDNS0ERR) != 0) {
+				res->_flags |= RES_F_EDNS0ERR;
+				if (res->options & RES_DEBUG)
+					printf(";; res_nquery: retry without EDNS0\n");
+				goto again;
+			}
+#endif
 #ifdef DEBUG
 			if (res->options & RES_DEBUG)
 				printf(";; rcode = %u, ancount=%u\n", hp->rcode,
diff --git a/lib/libc/resolv/res_mkquery.c b/lib/libc/resolv/res_mkquery.c
index c7a17ead6..242ce0b14 100644
--- a/lib/libc/resolv/res_mkquery.c
+++ b/lib/libc/resolv/res_mkquery.c
@@ -142,6 +142,7 @@ res_nmkquery(res_state statp,
 	hp->id = htons(statp->id);
 	hp->opcode = op;
 	hp->rd = (statp->options & RES_RECURSE) != 0U;
+	hp->ad = (statp->options & RES_USE_DNSSEC) != 0U;
 	hp->rcode = NOERROR;
 	cp = buf + HFIXEDSZ;
 	ep = buf + buflen;
-- 
2.14.2.822.g60be5d43e6-goog

>Release-Note:

>Audit-Trail:
From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/52578 CVS commit: src/lib/libc/resolv
Date: Thu, 28 Sep 2017 19:32:01 -0400

 Module Name:	src
 Committed By:	christos
 Date:		Thu Sep 28 23:32:01 UTC 2017

 Modified Files:
 	src/lib/libc/resolv: res_mkquery.c

 Log Message:
 PR/52578: Benjamin M. Schwartz Set the AD bit when DNSSEC is enabled
 (RFC 6840 Section 5.7).


 To generate a diff of this commit:
 cvs rdiff -u -r1.15 -r1.16 src/lib/libc/resolv/res_mkquery.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/52578 CVS commit: src/lib/libc/net
Date: Thu, 28 Sep 2017 20:04:33 -0400

 Module Name:	src
 Committed By:	christos
 Date:		Fri Sep 29 00:04:33 UTC 2017

 Modified Files:
 	src/lib/libc/net: getaddrinfo.c

 Log Message:
 PR/52578: Benjamin M. Schwartz: sync the internal copy of res_nquery for
 getaddrinfo, with the real version so that we handle EDNS fallback.


 To generate a diff of this commit:
 cvs rdiff -u -r1.115 -r1.116 src/lib/libc/net/getaddrinfo.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

Responsible-Changed-From-To: lib-bug-people->jdolecek
Responsible-Changed-By: jdolecek@NetBSD.org
Responsible-Changed-When: Tue, 10 Oct 2017 17:37:37 +0000
Responsible-Changed-Why:
I'll take care about the pullup bureaucracy.


State-Changed-From-To: open->pending-pullups
State-Changed-By: jdolecek@NetBSD.org
State-Changed-When: Tue, 10 Oct 2017 17:37:37 +0000
State-Changed-Why:
Your fix was committed, waiting for pullups for netbsd-8 branch.


From: "Soren Jacobsen" <snj@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/52578 CVS commit: [netbsd-8] src/lib/libc
Date: Wed, 25 Oct 2017 06:56:41 +0000

 Module Name:	src
 Committed By:	snj
 Date:		Wed Oct 25 06:56:41 UTC 2017

 Modified Files:
 	src/lib/libc/net [netbsd-8]: getaddrinfo.c
 	src/lib/libc/resolv [netbsd-8]: res_mkquery.c

 Log Message:
 Pull up following revision(s) (requested by jdolecek in ticket #317):
 	lib/libc/net/getaddrinfo.c: revision 1.116
 	lib/libc/resolv/res_mkquery.c: revision 1.16
 PR/52578: Benjamin M. Schwartz Set the AD bit when DNSSEC is enabled
 (RFC 6840 Section 5.7).
 --
 PR/52578: Benjamin M. Schwartz: sync the internal copy of res_nquery for
 getaddrinfo, with the real version so that we handle EDNS fallback.


 To generate a diff of this commit:
 cvs rdiff -u -r1.115 -r1.115.6.1 src/lib/libc/net/getaddrinfo.c
 cvs rdiff -u -r1.15 -r1.15.8.1 src/lib/libc/resolv/res_mkquery.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: pending-pullups->closed
State-Changed-By: jdolecek@NetBSD.org
State-Changed-When: Wed, 25 Oct 2017 18:04:26 +0000
State-Changed-Why:
Pullup to netbsd-8 branch is done. Thanks for report and patch!


>Unformatted:

Home	PR Database Search