Fix DNS forward/cache path for relay-driven poisoning - #21853
Fix DNS forward/cache path for relay-driven poisoning#21853Pushpenderrathore wants to merge 4 commits into
Conversation
Three issues surfaced while running the DNS server under the Kerberos relay coercion workflow (identified during jheysel's ESC8 testing): - Rex::Proto::DNS::Cache#cache_record raised on any forwarded record whose name did not match MATCH_HOSTNAME, killing the dispatch thread. Skip the non-cacheable record instead. - Server#default_dispatch_request duplicated the request with Dnsruby::Message#dup, which is shallow, so req.question and forward.question shared one Array. Deleting a cache-served question from the forwarded packet also emptied the original request's question list. Give forward its own copy. - The response was built by mutating the decoded request's @answer via instance_variable_set and re-encoding it, which appended the answer bytes past the packet end so clients decoded zero answers ("bad DNS packet" on Windows). Build a fresh Dnsruby::Message for the response instead. Also handle the udp recvfrom returning an explicit source port so the reply goes back to the right host/port across socket variants. Adds regression specs for the shallow-dup question handling and the answer-encoding fix.
40fbf90 to
b0d920f
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes DNS forwarding, caching, response encoding, and UDP reply addressing for relay-driven poisoning.
Changes:
- Skips invalid-hostname cache records.
- Safely separates forwarded questions and rebuilds DNS responses.
- Supports alternate UDP
recvfromreturn shapes.
Impact Analysis:
- Blast radius: Medium; affects default DNS forwarding and UDP listeners.
- Data and contract effects: Changes DNS response construction and cache behavior; RD is not currently preserved.
- Rollback and test focus: Reversible; validate DNS flags, cache rejection, and both UDP address formats.
File summaries
| File | Description |
|---|---|
lib/rex/proto/dns/cache.rb |
Skips non-cacheable hostnames. |
lib/rex/proto/dns/server.rb |
Fixes forwarding, response encoding, and UDP source handling. |
spec/lib/rex/proto/dns/server_spec.rb |
Adds forwarding and encoding regressions. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| unless record.name.to_s.match(MATCH_HOSTNAME) | ||
| raise "Invalid record for cache entry (invalid hostname) - #{record.inspect}" | ||
| return # skip non-cacheable record: " - #{record.inspect}" |
There was a problem hiding this comment.
Added coverage in the new spec/lib/rex/proto/dns/cache_spec.rb: with the monitor active it feeds cache_record a record whose name fails MATCH_HOSTNAME (an underscore SRV-style label) and asserts it neither raises nor gets added, alongside a control that a valid-hostname record is cached. Resolved in 6ad348f.
| buf, addr, source_port = self.udp_sock.recvfrom(65535) | ||
| if source_port | ||
| host, port = addr, source_port | ||
| else | ||
| host, port = addr[3], addr[1] |
There was a problem hiding this comment.
Added two #monitor_listener specs covering both recvfrom return shapes: the three-value (data, host, port) form used by Rex sockets and the two-value (data, sockaddr array) form from a raw UDPSocket, each asserting the mock client is addressed to the extracted host and port. Resolved in 52e6d90.
| resp = Dnsruby::Message.new | ||
| resp.header.id = req.header.id | ||
| resp.header.qr = true | ||
| resp.header.ra = req.header.rd |
There was a problem hiding this comment.
Good catch, fixed. The fresh response now echoes RD via resp.header.rd = req.header.rd. One note: header.recursive= is not defined in the pinned dnsruby 1.73.1 (Header exposes rd/ra accessors only), so I used header.rd=. I also derive RA from whether a forwarder is configured (ra = !fwd_res.nil?) rather than from the client RD, since RA advertises server capability. Specs assert RD is echoed and RA reflects the forwarder. Resolved in 76fd57a (fix) plus 52e6d90 (specs).
The fresh Dnsruby::Message dropped the request RD bit and set RA from the client RD. Echo RD back and derive RA from whether a forwarder is configured, so recursive queries get a spec-correct header.
|
Follow-up verification. Specs are now at 18 examples, 0 failures (spec/lib/rex/proto/dns/server_spec.rb plus the new spec/lib/rex/proto/dns/cache_spec.rb). I also lab-tested the fix end to end by running Rex::Proto::DNS::Server over real UDP in front of a public resolver and driving it with live queries:
Before/after on the crash itself: with the old raise, that same TXT query terminated the listener thread (Invalid record for cache entry (invalid hostname) on the real DMARC record at server.rb:183) and the client timed out. With the skip it returns normally. That is the thread-death this PR fixes. |
Description
Three fixes to the core
Rex::Proto::DNSforward/cache path that surface once the DNS server is used as a selective poisoner in front of a real upstream resolver (the Kerberos relay coercion workflow in #21693). Identified during @jheysel-r7's ESC8 relay testing.Each is small and independent:
Cache#cache_recordcrashed on non-cacheable forwarded records. Real upstream responses carry records whose names do not matchMATCH_HOSTNAME; caching them raised and killed the dispatch thread. Skip the record instead of raising.Shallow
dupshared the question array.Dnsruby::Message#dupis shallow, soreq.questionandforward.questionreferenced the sameArray. Deleting a cache-served question from the forwarded packet also emptied the original request's question list, so the echoed response lost its questions.forwardnow gets its own copy.Mutating a decoded request's
@answerproduced a malformed packet. Building the reply byinstance_variable_set(:@answer, ...)on the decoded request and re-encoding appended the answer bytes after the packet end, so clients decoded zero answers (surfaces as "Bad DNS packet" on Windows). The response is now built as a freshDnsruby::Message.Also handles
udp_sock.recvfromreturning an explicit source port so the reply is addressed to the right host/port across socket variants.Verification
Both non-obvious bugs (2 and 3) were confirmed empirically against the pinned
dnsrubybefore and after the fix, and are covered by new regression specs.The existing coverage from #21784 (empty-forward response) still passes.
Related