inet_dns.erl incorrectly handles mDNS high flag bit

[jimdigriz]

Describe the bug

The class field in in a DNS header is incorrectly decoded, the details of the values for this field maintained by IANA.

For unicast requests, it has the range 0->65535 whilst for multicast the value of class may only be in the range 0->32767; this change in behaviour is described in RFC6762 section 22.

If you try to use a private range class, the decoded record has its class returned with the top bit masked out.

To Reproduce

Send a request using:

$ dig @127.0.0.1 -p 55353 example.com TYPE65280 CLASS65280

Erlang wise, this effectively becomes:

1> rr(inet_dns).
2> Packet = <<203,85,1,32,0,1,0,0,0,0,0,1,7,101,120,97,109,112,108,101,3,99,111,109,0,255,0,255,0,0,0,41,4,208,0,0,0,0,0,12,0,10,0,8,45,34,16,83,207,221,146,158>>.
3> inet_dns:decode(Packet).
{ok,#dns_rec{header = #dns_header{id = 52053,qr = false,
                                  opcode = query,aa = false,tc = false,rd = true,ra = false,
                                  pr = false,rcode = 0},
             qdlist = [#dns_query{domain = "example.com",type = 65280,
                                  class = 32512,unicast_response = true}],
             anlist = [],nslist = [],
             arlist = [#dns_rr_opt{domain = ".",type = opt,
                                   udp_payload_size = 1232,ext_rcode = 0,version = 0,z = 0,
                                   data = <<0,10,0,8,45,34,16,83,207,221,146,158>>,
                                   do = false}]}}

Expected behavior

Class is returned as the value 65280; supporting classes above 32767 for unicast queries and responses.

Affected versions

Using OTP27 (master branch) as of 0164d3db05

Additional context

The fix for me here is unclear.

My understanding is there is nothing in a DNS packet that states "I am multicast", and the receiver should determine this by inspecting that the destination IP/port is 224.0.0.251:5353 (RFC6762, section 5+6) which means the logic in lib/kernel/src/inet_dns.erl:{encode,decode}_class() should really be lurking in the application as it is the only place that is aware of this.

So the problem is that OTP assumes this high bit is always 'unicast-response' when really it is only for mDNS traffic.

@RaimoNiskanen though added this in de80584b05c565952bf0cc7513203d40e89f2b11 for some reason and in this form, I am assuming it was reasons of "it pays our salary's"?

If this must be done in inet_dns.erl, one alternative is to set the default value of the flag to undefined and only give it a boolean value through inferring that queries were for local. (and if you really want to get fuzzy the link-local reverse zones).

Personally, I think de80584b05c565952bf0cc7513203d40e89f2b11 should be reverted and that logic pushed into the application, but that horse may have long since bolted and is no longer a feasible option.

Another option is @RaimoNiskanen points out like RFC6762 section 22 says, "only only 3+2 classes are registered, just use some other random number lower than 32768" or alternatively use #dns_query.unicast_response and #dns_rr.func in my application to reverse the breakage using a reversed logic of "not mDNS so repair...".

[jimdigriz]

My workaround for now is to have a wrapping function:

-module(inet_dns_utils).

-export([decode/1, decode/2]).
-export([encode/1, encode/2]).

-include_lib("kernel/src/inet_dns.hrl"). 

decode(Packet, MDNS) ->
        {ok, DnsRec} = inet_dns:decode(Packet),
        MDNS andalso {ok, DnsRec} orelse decode_mdns_fixup(DnsRec).
decode(Packet) ->
        decode(Packet, false).

%%% included for symmetry
encode(Packet, _MDNS) ->
        inet_dns:encode(Packet).
encode(Packet) ->
        encode(Packet, false).

%%% internal functions

%%% https://github.com/erlang/otp/issues/7718
%%% unfortunately inet_dns:{dns_query,rr} obliterate #dns_query.unicast_response
%%% and #dns_rr.func so we have to interact with the undocumented record directly
decode_mdns_fixup(DnsRec = #dns_rec{ qdlist = QDList0, anlist = ANList0, nslist = NSList0, arlist = ARList0}) ->
        QDList = [ decode_mdns_fixup2(RR) || RR <- QDList0 ],
        ANList = [ decode_mdns_fixup2(RR) || RR <- ANList0 ],
        NSList = [ decode_mdns_fixup2(RR) || RR <- NSList0 ],
        ARList = [ decode_mdns_fixup2(RR) || RR <- ARList0 ],
        {ok, DnsRec#dns_rec{ qdlist = QDList, anlist = ANList, nslist = NSList, arlist = ARList }}.

-define(UNICAST_RESPONSE, 16#8000).
decode_mdns_fixup2(RR = #dns_query{ class = C, unicast_response = true }) ->
        RR#dns_query{ class = C bor ?UNICAST_RESPONSE, unicast_response = false };
decode_mdns_fixup2(RR = #dns_rr{ class = C, func = true }) ->
        RR#dns_rr{ class = C bor ?UNICAST_RESPONSE, func = false };
decode_mdns_fixup2(RR) ->
        RR.

[RaimoNiskanen]

@RaimoNiskanen though added this in de80584 for some reason and in this form, I am assuming it was reasons of "it pays our salary's"?

The reason was more "what is the simplest fix to handle mDNS in a decent way", which apparently introduced a bug for private range classes, maybe subconsciously thinking that "classes 32768..65545 - nobody uses that"...

[RaimoNiskanen]

Personally, I think de80584 should be reverted and that logic pushed into the application, but that horse may have long since bolted and is no longer a feasible option.

I'd say so too. Reverting that fix would force the application into knowing how to decode RR content when handling mDNS. I don't think that is an option.

[RaimoNiskanen]

Looking just at the Summary of Differences it seems reasonable to follow in your track and add two functions inet_dns:decode(Buffer, MDNS_flag) and inet_dns:encode(Q, MDNS_flag). This little problem may just be the tip of an mDNS iceberg...

[jimdigriz]

Reverting that fix would force the application into knowing how to decode RR content when handling mDNS

Those summary of changes make it pretty client the client has no choice but to know it is processing mDNS.

mDNS is a different protocol that happens to be on the wire compatible with another related protocol called 'DNS'.

Have you seen anything using unicast_response? https://github.com/shortishly/mdns/, https://github.com/Licenser/erlang-mdns-server and https://github.com/nerves-networking/mdns_lite do not, I have been unable to find anything else that does but of course this does not mean something out there does not.

Instead of MDNS_flag, you may want to just provide an atom of which RFC you want the decoder/encoder to conform to? For example, QDCOUNT greater than one is technically allowed, that is about to change. Not 100% convinced a single 'conforming' flag is the right shape here (maybe a list, or something else), but want to put that idea out there to see if it has merit; similar to all those REST APIs where you get to pin the version of the API you want to communicate using. Would mean as bugs are uncovered or new features needed they can be added with little fanfare.

I would suggest trying to go back to being a plain boring DNS packer encoder/decoder and punt all the work to the application, which for mDNS anyone implementing it has to already do (bit masking the class is the least of their worries).

[RaimoNiskanen]

Those summary of changes make it pretty client the client has no choice but to know it is processing mDNS.

Yes. The intention is that the caller of inet_dns that must know it is processing mDNS, since it knows how the packet was received / how it will be sent.

Have you seen anything using unicast_response?

As you say; it is impossible to know if something out there does

Instead of MDNS_flag, you may want to just provide an atom of which RFC you want the decoder/encoder to conform to?

My plan was to start with a boolean flag, and when/if more needs surface, change the same argument into a property list where Flag::boolean() then translates into [{mdns,Flag}].

Having RFC:s as flags sounds scary, since a future RFC may come with somethin new for example for mDNS. A name we invent ourselves like mdns can have the semantics we define ourselves.

I would suggest trying to go back to being a plain boring DNS packer encoder/decoder and punt all the work to the application, which for mDNS anyone implementing it has to already do (bit masking the class is the least of their worries).

I would assume that having the content of the RRs decoded/encoded for mDNS, as for DNS would be useful, since the protocols "happens to be on the wire compatible". Otherwise, for DNS you get IP addresses decoded, domains compressed and so forth, and for mDNS,... nothing. Is it even possible to do domain label decompression of the raw binary format, in a nice way?

[jimdigriz]

Instead of MDNS_flag, you may want to just provide an atom of which RFC you want the decoder/encoder to conform to?

My plan was to start with a boolean flag, and when/if more needs surface, change the same argument into a property list where Flag::boolean() then translates into [{mdns,Flag}].

Okay, I am a fan of proplists.

Having RFC:s as flags sounds scary, since a future RFC may come with somethin new for example for mDNS. A name we invent ourselves like mdns can have the semantics we define ourselves.

Sounds sensible, the aim is to instead flag for variants/flavours of DNS packet from the base specification.

Just to mix things up a bit, does the encoder/decoder behaviour get a say here?

Picking the example of SRV RR's, which is not too hard to firmly decide on, but highlights some conflicting behaviour:

• RFC2052 (unicast) allows target to have name compression
• RFC2782 (unicast) forbids target to have name compression
• RFC6762 (mDNS) requires name compression for SRV
• draft-ietf-dnssd-srp-23 (unicast) requires support of name compression for SRV (section 3.2.5.4)

As we are discussing inet_dns which is undocumented behaviour, I suppose a reasonable answer is "use your own DNS library". :)

Otherwise, for DNS you get IP addresses decoded, domains compressed and so forth, and for mDNS,... nothing.

I think you mis-understand. The packet should still be decoded, I just do not see a reason to do the special (for example) mDNS only step of interpreting the high bit of the class; the application is having to do so much other work to mask the high bit of class does not really help the developer in a meaningful way.

Is it even possible to do domain label decompression of the raw binary format, in a nice way?

Define 'nice'. Your existing decode_name probably as good as you can get without making things awkward elsewhere,

Just be glad the 'unfeature' of pointers to later bytes has been dead for ~20 years....

[RaimoNiskanen]

Sounds sensible, the aim is to instead flag for variants/flavours of DNS packet from the base specification.

Precisely.

As we are discussing inet_dns which is undocumented behaviour, I suppose a reasonable answer is "use your own DNS library". :)

Even though it is undocumented, which is mostly just because it hasn't been done, and because it hasn't been decided exactly what to support; we still strive for it to be useful.

We should try to get as far as possible with safe defaults, and "be strict when you encode but accepting when you decode", then add a flag for things useful enough. And for too esoteric wishes we can always say "use your own DNS library".

I think you mis-understand. The packet should still be decoded, I just do not see a reason to do the special (for example) mDNS only step of interpreting the high bit of the class; ...

This is what I was getting at. If decode_class(Class) doesn't recognize the class as in high bit set on class IN, never heard of mDNS, return a numeric class. Then decode_data/4 doesn't try to decode the data so the RR record gets a numeric class and binary data.

Therefore decode_class(Class, Mdns) can know when it is an unknown numeric private class from when it is a known class with high bit set and return a symbolic class, which makes decode_data/4 decode the record content.

The intention is to decode known records and leave unknown records (numeric class) undecoded.

[jimdigriz]

This is what I was getting at. If decode_class(Class) doesn't recognize the class as in high bit set on class IN, never heard of mDNS, return a numeric class. Then decode_data/4 doesn't try to decode the data so the RR record gets a numeric class and binary data.

I see what I missed there, thanks for pointing it out.

[RaimoNiskanen]

So, does adding decode(Buffer, Mdns) and encode(Q, Mdns) as suggested in my PR #7905 seem like a good enough solution to you?

[jimdigriz]

So, does adding decode(Buffer, Mdns) and encode(Q, Mdns) as suggested in my PR #7905 seem like a good enough solution to you?

Sure, I am a big fan of 'good enough' :)

[RaimoNiskanen]

Merged; thank you for reporting this issue!