Bug 3397 - cachedb could return a partial CNAME chain
cachedb could return a partial CNAME chain
Status: RESOLVED FIXED
Product: unbound
Classification: Unclassified
Component: server
unspecified
Other All
: P5 normal
Assigned To: unbound team
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2017-12-20 19:31 CET by JINMEI Tatuya
Modified: 2018-01-22 15:31 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 2017-12-20 19:31:19 CET
(This is confirmed for svn trunk revision 4429.)

If you configure unbound with cachedb:

server:
[...]
	module-config: "cachedb iterator"

and configure an authoritative server with example.com zone:

a 60 IN CNAME b
b 5 IN AAAA 2001:db8::1

Then query unbound for a.example.com/AAAA.  Of course it should return a complete response:

;; flags: qr rd ra; QUERY: 1, ANSWER: 2, AUTHORITY: 1, ADDITIONAL: 1
;; ANSWER SECTION:
a.example.com.		60	IN	CNAME	b.example.com.
b.example.com.		5	IN	AAAA	2001:db8::1

Wait until the AAAA RRset expires, and repeat the query.  You'll now see a partial CNAME chain:

;; flags: qr rd; QUERY: 1, ANSWER: 1, AUTHORITY: 0, ADDITIONAL: 1
;; WARNING: recursion requested but not available
;; ANSWER SECTION:
a.example.com.		51	IN	CNAME	b.example.com.

(It also doesn't set the RA bit, which seems awkward, but that's not the main point here).

The reason for having the partial chain is because cachedb_intcache_lookup() uses whatever returned from dns_cache_lookup().  In the above scenario, dns_cache_lookup() notices the message cache entry expires and falls back to the CNAME, builds a message from it and returns it.

Furthermore, if validator is specified on top of cachedb:
	module-config: "validator cachedb iterator"
it will cache this partial chain in the in-memory cache, and subsequent queries will directly hit that entry (although even if not the end result would be almost the same except for the RA bit).

A similar nuisance can happen with DNAME.  If you have this in example.com:

o 60 IN DNAME example.org.

also set up example.org with this:

x 5 IN AAAA 2001:db8::2

and query unbound for x.o.example.com/AAAA, it first returns a complete chain with synthesized CNAME:

;; ANSWER SECTION:
o.example.com.		60	IN	DNAME	example.org.
x.o.example.com.	60	IN	CNAME	x.example.org.
x.example.org.		5	IN	AAAA	2001:db8::2

but once the AAAA expires, it will return:

;; ANSWER SECTION:
x.o.example.com.	43	IN	CNAME	x.example.org.

To prevent this, you'll need to do something like the following:

- just don't bother to call dns_cache_lookup from the cachedb module.
  it can result in some suboptimal behavior (otherwise-unnecessary
  extcache lookup) in some rare cases, but would be the simplest solution, or
- use a similar logic as the 'if(msg)' block of iterator.c:processInitRequest(), or
- at least check if the answer is incomplete and ignore it if so

By the way, the standard iterator module basically doesn't have this
problem thanks to the 'if(msg)' check, but in the case of DNAME it can
still produce an awkward answer.  If you only enable iterator with the same
authoritative setup, send a query for x.o.example.com/AAAA, wait until
the final AAAA expires, and repeat the query, then you'll see this:

;; ANSWER SECTION:
x.o.example.com.	54	IN	CNAME	x.example.org.
x.example.org.		5	IN	AAAA	2001:db8::2

This is because dns_cache_lookup() finds the cached DNAME but
synth_dname_msg() ignores it unless it's DNSSEC-signed:

	/* only allow validated (with DNSSEC) DNAMEs used from cache 
	 * for insecure DNAMEs, query again. */

while still finds the synthesized CNAME (successfully) and uses it:

	/* see if we have CNAME for this domain,
	 * but not for DS records (which are part of the parent) */
	if( qtype != LDNS_RR_TYPE_DS &&
	   (rrset=rrset_cache_lookup(env->rrset_cache, qname, qnamelen, 
		LDNS_RR_TYPE_CNAME, qclass, 0, now, 0))) {
[...]

In practice, most stub resolvers don't care, so it wouldn't cause a big
operational trouble.  But it still doesn't seem to be the sensible behavior
to me, and you may want to fix it too.
Comment 1 Wouter Wijngaards 2018-01-18 16:42:03 CET
Hi Jinmei,

Thanks for the report, so I think the following may be a clean solution.

In the dns_cache_lookup() include a parameter that says 'no_partial', that is true for the cachedb call.  When that is true, in the lookup it won't lookup the individual DNAME or CNAME records and construct a message for it.  It would lookup from the message cache (a whole result).  That should both be performant, and very simple.

For the iterator alone case, I would want to change the rule that a DNAME won't be picked up if its unsigned, unless a CNAME is already cached for it.  And then pick up the DNAME and the CNAME.  So that the iterator will then correctly display the DNAME, and also pick it from cache.

If that is too security worrisome (not sure if its a problem), then I would just omit the CNAME pick up when a DNAME was refused due to not being signed.  Then it can re-lookup the DNAME and synth the CNAME from it.  But if such a CNAME already exists, then perhaps this is not optimal, or even adds security, and the previous case is better.

Best regards, Wouter
Comment 2 JINMEI Tatuya 2018-01-18 16:55:42 CET
The proposed change to dns_cache_lookup() looks good to me.

As for the iterator case, I also tend to agree on changing the rule -
I actually wondered about the rationale of the rule.  But I don't have
a strong opinion; I just happened to notice some awkward behavior so I
reported it.  I'd leave the resolution to you.
Comment 3 Wouter Wijngaards 2018-01-22 15:31:06 CET
Hi Jinmei,

Thank you very much for the report of the awkward behaviour, I have committed code to fix it!

Best regards, Wouter