DNSKEY RR with only three fields

[mattias-p]

Hi guys!

In Zonemaster we ran into an assertion error in ldns when we encountered an (arguably) unreasonable response. The response contained a DNSKEY RR with an empty public key field. This caused the wireformat parser to return an ldns_rr with only three rdata fields instead of the usual four. In our code we expected the fourth field of DNSKEY RRs to always be there, so we just accessed it and boom.

In rr.c I found a declaration stating that an DNSKEY RR should always have 4 rdata fields. This agrees with my intuition. So AFAICT there's a bug somewhere in the wireformat parser.

I might add that we simply working around the problem on our side by adding a guard that checks the number of fields of the DNSKEY RR before accessing the public key field.

[wtoorop]

Hi @mattias-p Did the key come from querying the internet? ldns uses the definition to produce errors for missing rdata fields when parsing presentation format, but not with wireformat. I.e:

$ echo "example.com. DNSKEY \# 4 01010308" | ldns-read-zone 
example.com.    3600    IN  DNSKEY  257 3 8 ;{id = 1033 (ksk), size = 0b}

The rational for this is that ldns should return whatever it got from the "wire" even if it is broken, because unlike presentation format files, it may not be under the control of the ldns user. Do you think that's wrong?

We could also create a new ldns function that would asses a packet, rrset or rr and report on the issues it found. It could already report on the correctness of the number of rdata fields, but perhaps also other things, such as the length of SHA rdata fields. named-checkzone does these kind of checks, but we've until now always took the more permissive approach...

[mattias-p]

Hi @wtoorop! Yes, we got it in a response from the internet. The issue was reported in zonemaster/zonemaster-ldns#110. However the zone was fixed on the internet before I got time to work on it, so I synthesized a DNS response with an empty public key. (First I tried to construct one using ldns (through Zonemaster::LDNS) but that didn't work. So I constructed one using https://pypi.org/project/dnslib/ instead.)

Anyway, this is what our code looked like before:

// This only works when the public key is non-empty, which we can't be sure of since
// we're parsing DNS data from the internet
ldns_rdf *rdf = ldns_rr_rdf(dnskey_rr, 3);
size_t keysize = ldns_rdf_size(rdf);

But now I'm changing it to look like this:

// This always works
size_t keysize = 0;
if (ldns_rr_rd_count(dnskey_rr) > 3) {
    ldns_rdf *rdf = ldns_rr_rdf(dnskey_rr, 3);
    keysize = ldns_rdf_size(rdf);
}

To me the former is more reasonable since the public key field isn't optional according to the RFC 4034. Instead the field count check is only needed because of an idiosyncrasy in ldns. To me it would be better if the wireformat parser returned DNSKEY RRs with four rdata fields no matter if the public key field is empty or non-empty.

And since you ask, here are my thoughts on the general design of DNS libraries:

From my perspective the ideal would be to have an API that would allow me to both parse and programmatically construct any DNS data that could be represented in a technically valid DNS packet on the wire. But if I tried to construct something that couldn't be represented on the wire I'd to get a compile-time error. Or if it isn't feasible to make every error a compile-time error I'd settle for run-time errors where needed.

The ability to parse any valid data that comes over the wire - even if it's nonsense - is crucial for applications lite Zonemaster. The ability to programmatically construct any valid data that could be sent over the wire - even if it's nonsense - is also very useful. I already mentioned synthesizing DNS responses. And sometimes when I want to test the error handling behavior of a DNS client I wish I had a DNS server that could be provoked to give any kind of erroneous response. The ability to construct nonsense DNS data would be helpful there too.

For people who only want to push correct DNS messages I guess a validation facility could be useful so they can fail early. But personally I'm spending a lot more time thinking about stuff that's already broken.

[mattias-p]

I've realized a couple of things since my last post. I realized that:

• an DNSKEY rdata with an empty public key field isn't even valid,
• ldns accepts it anyway and returns as many fields as there is data for, and
• ldns is more generous in what it accepts than I expected.

My current idea of how to deal with the situation is check all RRs of all responses and filter out any RRs that don't have valid rdata field counts.

The edge case that tripped us up in the first place is still interesting though. It seems safe to ignore DNSKEY RRs with three rdata fields because they're invalid anyway, but I'm thinking there may be other RR types that end with a variable length field that could be empty without being invalid. In such cases the reasonable thing seems to be to add an empty rdata field to the ldns_rr just to make sure the field count is right and that calling code can access it.

What do you think about this?

[wtoorop]

The edge case that tripped us up in the first place is still interesting though. It seems safe to ignore DNSKEY RRs with three rdata fields because they're invalid anyway, but I'm thinking there may be other RR types that end with a variable length field that could be empty without being invalid. In such cases the reasonable thing seems to be to add an empty rdata field to the ldns_rr just to make sure the field count is right and that calling code can access it.

What do you think about this?

In case of missing variable length last rdata field create an empty variable length rdf. I like that! But you don't want this for DNSKEYs?

[matsduf]

How does LDNS treat unassigned or private RR types when it comes to RDATA?

[wtoorop]

How does LDNS treat unassigned or private RR types when it comes to RDATA?

As unknown type format, like this:

$ echo "example.com. TYPE123 \# 17 736f6d65207264617461206669656c6473" | ldns-read-zone
example.com.    3600    IN  TYPE123 \# 17 736f6d65207264617461206669656c6473

[mattias-p]

In case of missing variable length last rdata field create an empty variable length rdf. I like that! But you don't want this for DNSKEYs?

Yeah, I guess we want it for DNSKEY RRs too. (We just don't strictly need it.)

In Zonemaster we'll need to add a filter for weeding out RRs with invalid RDF counts for any RR type. Additionally we'll need to add some RR type specific checks. E.g. we won't be accepting DNSKEY RRs with empty pubilc key fields. In practice it won't matter much if the DNSKEY RR is ignored because of low RDF count or because of small public key field size.

Though I guess if the DNSKEY RRs always had four RDFs we'd have the option to emit better errors/warnings.