Bug 836 - unbound could echo back EDNS options in an error response
unbound could echo back EDNS options in an error response
Status: RESOLVED FIXED
Product: unbound
Classification: Unclassified
Component: server
unspecified
Other All
: P5 enhancement
Assigned To: unbound team
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2016-09-22 00:24 CEST by JINMEI Tatuya
Modified: 2016-09-27 14:22 CEST (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description JINMEI Tatuya 2016-09-22 00:24:10 CEST
If, for example, you send a query in the CH class that is not subject
to any special handling (like "version.bind"), unbound normally
returns a REFUSED response primarily because of the lack of a root
hint.  In this case it includes all and any EDNS options that the
query contains:

% dig @127.0.0.1 examle.com aaaa ch +ednsopt=65001:01020304
...
;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 4096
; OPT=65001: 01 02 03 04 ("....")

I'm not sure if it's intentional, but this is inconsistent with many
other cases including a normal response.

Implementation-wise, the difference is that the case where the options
are echoed back is handled in the following block of mesh.c:mesh_send_reply():

	} else if(rcode) {
		m->s.qinfo.qname = r->qname;
		error_encode(r->query_reply.c->buffer, rcode, &m->s.qinfo,
			r->qid, r->qflags, &r->edns);
		comm_point_send_reply(&r->query_reply);

In many (perhaps all) other cases, edns_opt_inplace_reply() is called
before error_encode(), and EDNS options in the query are cleared:

int edns_opt_inplace_reply(struct edns_data* edns, struct regional* region)
{
	(void)region;
	/* remove all edns options from the reply, because only the
	 * options that we understand should be in the reply
	 * (sec 6.1.2 RFC 6891) */
	edns->opt_list = NULL;
	return 1;
}

Actually, it's not really clear to me whether 6.1.2 of RFC6891
specifies this behavior, but the inclusion would at least break
draft-ietf-dnsop-edns-key-tag.  If nothing else, unless there's
a specific reason for behaving differently, it would be better to make
it consistent.
Comment 1 Wouter Wijngaards 2016-09-27 14:22:30 CEST
Hi Jinmei,

Thank you very much for finding this code path.  It really should inplace reply or clear the option list.  So as to not echo the EDNS options, for EDNS compliance.

Patched like this, where it calls the inplace reply, but if that fails, clears the option list so there is no echoing going on.

Index: services/mesh.c
===================================================================
--- services/mesh.c	(revision 3867)
+++ services/mesh.c	(working copy)
@@ -878,6 +878,8 @@
 		comm_point_send_reply(&r->query_reply);
 	} else if(rcode) {
 		m->s.qinfo.qname = r->qname;
+		if(!edns_opt_inplace_reply(&r->edns, m->s.region))
+			r->edns.opt_list = NULL;
 		error_encode(r->query_reply.c->buffer, rcode, &m->s.qinfo,
 			r->qid, r->qflags, &r->edns);
 		comm_point_send_reply(&r->query_reply);

Best regards, Wouter