alexdalitz / dnsruby

Dnsruby is a feature-complete DNS(SEC) client for Ruby, as used by many of the world's largest DNS registries and the OpenDNSSEC project
Other
194 stars 77 forks source link

incorrect TXT segment parsing #170

Closed and0x000 closed 3 years ago

and0x000 commented 3 years ago

The TXT parsing function fails to recognized segmented records, that consist of escaped and non-escaped strings.

Example:

@ IN TXT "a" b

This is a legal TXT record, consisting of the 2 segments a and b. Running this through the TXT parser, it gives me an array only containing a.

minimal working example:

~ pry
[1] pry(main)> require 'dnsruby'
=> true
[2] pry(main)> Dnsruby::RR::TXT.parse 'a b'
=> ["a", "b"] # OK
[3] pry(main)> Dnsruby::RR::TXT.parse '"a" "b"'
=> ["a", "b"] # OK
[4] pry(main)> Dnsruby::RR::TXT.parse '"a" b'
=> ["a"] # NOK
and0x000 commented 3 years ago

The problem seems to be caused by this section in the code

[...]
          next if seen_strings && !in_string # resolves to true
[...]

In the iteration, where b would have to be added to the array, seen_strings is true and in_string is false, resulting in the call to next, skipping b.

alexdalitz commented 3 years ago

Oh dear how terrifically embarrassing! Thanks for the report. I’ll try to get this fixed over the weekend

Sent from my iPhone

On 18 Dec 2020, at 14:30, and0x000 notifications@github.com wrote:

 The problem seems to be caused by this section in the code

[...] next if seen_strings && !in_string # resolves to true [...] In the iteration, where b would have to be added to the array, seen_strings is true and in_string is false, resulting in the call to next, skipping b.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

and0x000 commented 3 years ago

Oh dear how terrifically embarrassing!

It's hardly embarrassing, considering what a f***ed up mess TXT records are to parse/validate. I'm glad someone else accepted this challenge.

On the topic of being a mess: When fixing this, please be aware that "a" b (with whitespace in between) is valid TXT data, whereas "a"b (without whitespace in between) is not ... this gave me headaches to the point of resignation when I tried to fix this myself. Not entirely sure thought, if receiving/handling possibly incorrect DNS data is in the scope of this gem.

alexdalitz commented 3 years ago

OK, I've pushed a fix which parses your examples. To be honest, it's far from clear whether this is actually correct behaviour or not!

and0x000 commented 3 years ago

OK, I've pushed a fix which parses your examples.

Thank you very much.

To be honest, it's far from clear whether this is actually correct behaviour or not!

Not sure to what point exactly you are refering to, but there are indeed differences across implementations for what constitudes a legit TXT record.

Probably way beyond the scope of this issue but: I cross-tested with named-checkzone (bind9), nsd-checkzone and kzonecheck (knot) for zonefile validation. Interestingly enough, only named-checkzone accepts a txt record without whitespace between string "a" and b. WTF. All 3 are solid DNS server implementations so a majority vote of what constitutes for valid would be plausible, if it weren't for the fact, that bind is the refference implementation.