Bug 1190 - unbound could (still) echo back EDNS options in an error response
unbound could (still) echo back EDNS options in an error response
Status: ASSIGNED
Product: unbound
Classification: Unclassified
Component: server
unspecified
Other All
: P5 major
Assigned To: unbound team
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2016-12-16 19:26 CET by JINMEI Tatuya
Modified: 2016-12-23 13:54 CET (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-12-16 19:26:35 CET
I noticed the fix to bug #836
(https://www.nlnetlabs.nl/bugs-script/show_bug.cgi?id=836) was
incomplete.  error_encode() is called from many other places, and in
most cases edns_opt_inplace_reply() isn't called beforehand, so the
same awkward echo back can happen in other cases.  I confirmed this at
least for some local zone cases.  If you have this configuration:

	local-zone: example.com always_nxdomain

and send a query for it with a custom EDNS option:

% dig @127.0.0.1 example.com +ednsopt=65001:01020304

you'll get the option echoed back in the NXDOMAIN response.  That's
the same if you specify always_refuse instead of always_nxdomain.  I
suspect some of the less likely cases for SERVFAILs have the same
issue.  If we fix bug #836 by suppressing the echo back, I believe we
should do the same thing for other cases, too.  Since error_encode()
is called from many places, it might be better to include the
suppression logic inside error_encode() itself, rather than fix all
calllers.

A couple of notes you may want to know:

- in the above example of always_nxdomain, if you also provide
  local-data SOA for example.com, the options aren't echo back.  This
  is because error_encode() isn't used in this case:

		if(z->soa)
			return local_encode(qinfo, env, edns, buf, temp, 
				z->soa, 0, rcode);

  But it looks like local_encode() could explicitly insert some EDNS
  options.  In that sense we might actually want to make it possible
  for error_encode() to insert EDNS options in some specific cases.

- if we choose to update error_encode() so it clears the EDNS options,
  you'll probably need to be careful for the 'else if(rcode)' case of
  mesh_send_reply(), since the passed 'edns' can be reused.
Comment 1 Ralph Dolmans 2016-12-23 13:54:26 CET
Hi JINMEI,

Again many thanks for reporting a bug!

I prefer not to do the suppression inside error_encode() itself because, with our new EDNS framework, the EDNS handling in localzones can be different from other places in unbound. inplace_cb_reply_local_call() should be called before all error_encode() calls in localzone.c. To do so I created local_error_encode().

Regards,
-- Ralph