Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DNS Protocol Compliance #5

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

DNS Protocol Compliance #5

wants to merge 1 commit into from

Conversation

Kolkman
Copy link

@Kolkman Kolkman commented Oct 25, 2022

When trying to debug some code with 'dig' I noticed that the code would return a SERVFAIL (or other configured error code) because of the EDNS in the additional section. I rewrote the code to be able to answer queries with EDNS on. Makes troubleshooting with dig much easier.

Also, reply packet handling was such that a SERFVAIL would return a few extra bytes in the packet (easy to observe with wireshark).

The code would return an A resource record also when the QTYPE was not A. I fixed that for other QTYPE SERVAIL is now returned.

Finally the domain name parsing might not terminate if the question domain name in the packet is crafted to have no nul-byte terminating it, potentially leading to DOS.

All this was fixed with rewriting some of the internal and private functions.

Fix: reply with malformed SERVFAIL packets, i.e. with an additional couple of bytes in the packet. Now: clean DNS packet boundary.
Fix: code would reply with A record also for other QTYPES. Now: will answer only A type queries,
Fix: code would return SERVAIL when EDNS was set on request (and copy all its content as extraneous packet content). Now: cleanly ignores EDNS0
Fix: Added check on parsing of QNAME to prevent potential problems with corrupt QNAMES

@benpeart
Copy link

benpeart commented Nov 1, 2022

I'm happy to see these fixes given how frequently this library is used by other libraries. I have one request, please update the version in library.json with this change. I've noticed that platform.io isn't picking up the correct version with the ESP32 fixes guestisp made back in May 2022 as those changes weren't properly versioned either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants