Revamp dns.c -- catch up with 9 years, fix some bugs, add some features
#39
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I asked Claude code to get `src/dns` to build again. I've not yet reviewed this commit. Do not use w/o reviewing!
I asked Claude code to get `contrib` to build again. I've not yet reviewed this commit. Do not use w/o reviewing!
Fuzzing found several issues:
1. Integer Overflow in TTL Parsing (src/dns.c:2584)
(Low-severity)
- Issue: Left shift 0xff << 24 caused signed integer overflow
- Fix: Cast to (unsigned) before shifting: (unsigned)(0xff & P->data[p + 0]) << 24
2. RDATA Bounds Validation Added to SSHFP parser.
(Remote crash DoS.)
Added validation if (rr->rd.len > P->end - rr->rd.p) return
DNS_EILLEGAL to dns_sshfp_parse().
3. dns_any_parse() Bounds Validation (src/dns.c:5190)
(Remote crash DoS.)
- Issue: Unknown RR types were copied without bounds checking
- Fix: Added if (rr->rd.len > P->end - rr->rd.p) return DNS_EILLEGAL
The fuzzer will be included in a later commit.
Overall the impact on any actual users -are there any?- would be a
remote DoS / crasher due to over-reading past the end of heap objects.
There are no over-writes, therefore no classic buffer overflows.
These over-reads do not affect at least domainnames in RDATA. Therefore
there is no way to leak over-read data to DNS servers.
I asked Claude code to add support for the AD and CD bits. I've not yet reviewed this commit. Do not use w/o reviewing!
…ypes (Claude-coded) I asked Claude code to add support for a bunch of RRtypes. Included are tests that include validating dns.c's results with dig(1). I've not yet reviewed this commit. Do not use w/o reviewing!
Claude-coded, with human guidance. Partly reviewed.
nicowilliams
changed the title
dns.c -- catch up with 9 yearsdns.c -- catch up with 9 years, fix some bugs, add some features
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR mainly makes
dns.cbuild with more warnings enabled, adds GitHub Actions workflows, fixes a few minor bugs, and adds some functionality, including support for several more RR types, AD/CD bit, fuzzing, etc.I used an LLM (Claude) for much of this, but I drove it carefully and reviewed most of its output -- I can attach my
dns.c-relevant sessions if you like.My intent is to use
dns.cin various open source projects such as Heimdal (which I co-maintain and have for some 14 years now).Hoping you're doing well!
FYI the reason I did this work is that I needed a DNS library that could do async I/O and had an easy-to-use API. I looked quite a bit and this was by far the best library I found. I like the quality of
dns.c, and I ran it past @dukhovni, who is a DNS expert and a DANE champion, and after being skeptical at first he pronounced it fairly solid with one caveat: that nowadays one must not write a full recursor because there are too many options like DoT, DoH, and DNS over QUIC. Well, I've no intent on using the full recursor functionality indns.c, but certainly for this contribution I'm also not removing it!