fix(nsupdate): proper UPDATE response, serial consistency, TTL overflow guard#75
Conversation
- build_response now copies the zone section into the response and sets QDCOUNT=1 so clients don't treat the reply as malformed - Compute new_serial once per apply_update request and thread it through the call chain; removes per-change zone reload in log_zone_change - Validate u32 TTL <= i32::MAX in add_record before casting to avoid negative TTLs being stored in the database Co-authored-by: kweonminsung <79230043+kweonminsung@users.noreply.github.com> Agent-Logs-Url: https://github.com/kweonminsung/bindizr/sessions/38cc6a44-4082-40f0-b5df-4e45ff363f4c
There was a problem hiding this comment.
Pull request overview
This PR fixes correctness issues in the RFC 2136 nsupdate implementation by improving DNS UPDATE response formatting, ensuring a single consistent zone serial is used across all changes in a request, and preventing TTL overflow when persisting records.
Changes:
- Update DNS UPDATE responses to echo the zone/question section (so clients like
nsupdateaccept the response as well-formed). - Generate a single
new_serialper UPDATE request and thread it through change logging and zone serial bumping to keep serials consistent. - Add an explicit TTL range check to prevent
u32 -> i32wraparound when storing record TTLs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/dns/nsupdate/mod.rs |
Builds RFC-expected UPDATE responses by copying the zone/question section into the reply. |
src/dns/nsupdate/update.rs |
Ensures per-request serial consistency and guards against TTL overflow when writing to the DB. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Preserve upper flag nibble, set RCODE in lower nibble. | ||
| response[3] = (query_data[3] & 0xF0) | (rcode & 0x0F); |
There was a problem hiding this comment.
build_response currently copies the upper flag nibble from the query (query_data[3] & 0xF0). That nibble contains RA/AD/CD and reserved Z bits, and mirroring it can produce incorrect or non-compliant responses (e.g., echoing AD=1 even though the server didn't authenticate anything). Consider explicitly setting these bits for UPDATE responses (typically RA=0 and Z=0; only copy bits you intentionally support) and only OR in the RCODE.
| // Preserve upper flag nibble, set RCODE in lower nibble. | |
| response[3] = (query_data[3] & 0xF0) | (rcode & 0x0F); | |
| // For UPDATE responses, do not mirror RA/AD/CD/Z bits from the query. | |
| // Explicitly set the upper nibble to 0 and only encode the RCODE. | |
| response[3] = rcode & 0x0F; |
| let qdcount = u16::from_be_bytes([query_data[4], query_data[5]]); | ||
| let zone_end = if qdcount > 0 { | ||
| zone_section_end(query_data) | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
The zone section is echoed whenever qdcount > 0, and the response then forces QDCOUNT=1. If a malformed UPDATE arrives with QDCOUNT != 1 (which the parser rejects as InvalidHeader), this response will claim a single question and copy only the first one, making the reply internally inconsistent. Consider only echoing the zone/question section when qdcount == 1 and zone_section_end() succeeds; otherwise leave QDCOUNT=0 (or return None and drop the packet) to avoid sending a structurally confusing response.
| offset += 1; | ||
| break; | ||
| } | ||
|
|
There was a problem hiding this comment.
zone_section_end treats any length byte that isn't 0, a normal label (00xxxxxx), or a compression pointer (11xxxxxx) as a “regular label”. Values with 01xxxxxx / 10xxxxxx are reserved label types in DNS wire format; accepting them can cause the function to parse/echo invalid names and potentially allocate/copy unexpectedly large slices. Consider rejecting those reserved forms (return None) when (len & 0xC0) != 0 && (len & 0xC0) != 0xC0.
| // Reserved label types (01xxxxxx / 10xxxxxx) are invalid. | |
| if (len & 0xC0) != 0 { | |
| return None; | |
| } |
Three correctness issues in the RFC 2136 nsupdate path identified by code review.
Changes
build_responsezone echo (src/dns/nsupdate/mod.rs): Addedzone_section_end()to locate the end of the first question/zone section in the DNS wire buffer.build_responsenow copies that section into the response and setsQDCOUNT=1, matching what clients likensupdateexpect — a header-only reply was treated as malformed.Serial consistency (
src/dns/nsupdate/update.rs):generate_serial()was called independently insidelog_zone_changefor every RR change (an N+1 zone reload), risking different serials across changes in the same request. It is now called once inapply_updatebefore the update loop; the resultingnew_serialis threaded throughapply_single_update→add_record/delete_records→log_zone_changeand used directly inbump_zone_serial, guaranteeing allZoneChangerows and the zone serial share the same value.TTL overflow guard (
src/dns/nsupdate/update.rs):update.ttl as i32silently wrapped values abovei32::MAXinto negatives in the DB. Added an explicit range check inadd_recordthat returnsUpdateError::Refusedfor out-of-range TTLs:⚡ Quickly spin up Copilot coding agent tasks from anywhere on your macOS or Windows machine with Raycast.