Bugzilla – Full Text Bug Listing
|Summary:||unbound could (still) echo back EDNS options in an error response|
|Product:||unbound||Reporter:||JINMEI Tatuya <jtatuya>|
|Component:||server||Assignee:||unbound team <unbound-team>|
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