From 527176c6eb0f30ce0d6439e0564dd6f985bed8d7 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Thu, 25 Jun 2026 05:12:20 +0000 Subject: [PATCH 1/7] Add functional review findings backlog Co-authored-by: Gerold K. --- REVIEW_FINDINGS.md | 231 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 231 insertions(+) create mode 100644 REVIEW_FINDINGS.md diff --git a/REVIEW_FINDINGS.md b/REVIEW_FINDINGS.md new file mode 100644 index 0000000..0f24027 --- /dev/null +++ b/REVIEW_FINDINGS.md @@ -0,0 +1,231 @@ +# REVIEW_FINDINGS.md + +## Summary + +FilterDNS is a single .NET console DNS proxy. It pulls zones from an upstream master over AXFR, rewrites SOA/NS records, optionally filters private A/AAAA data, stores file-backed zone history, then serves AXFR/IXFR to slaves and ACL-gated health-check queries. The highest-risk paths are DNS wire parsing, ACL/range matching, zone history persistence, IXFR diffing, and cache-update side effects. The codebase builds cleanly, but several normal DNS scenarios can still produce corrupt transfers, refused transfers, or silent operational failures. + +Total findings: 19 + +- Critical: 3 +- High: 6 +- Medium: 8 +- Low: 2 + +Fix first: + +1. Fix persisted history RDATA loss before trusting IXFR after restart. +2. Replace the shared CIDR matcher before relying on whitelists or private-IP filtering. +3. Fix IXFR/RRset diff identity so duplicate records and TTL changes are not lost. +4. Normalize zone names at configuration load so mixed-case zones do not break external DNS paths. +5. Remove cache-update side effects from health checks and transfer-triggered refreshes. + +## Findings + +### [SEV-01] Persisted history drops RDATA and can corrupt IXFR after restart + +- **Severity:** Critical +- **Confidence:** Confirmed +- **Location:** `FilterDns/Cache/ZoneHistoryStorage.cs:26-34`, `FilterDns/Cache/ZoneHistoryStorage.cs:481-530`, `FilterDns/Dns/IxfrResponseBuilder.cs:378-455` +- **What's wrong:** JSON history declares `RawRdataBase64`, but `ConvertToJson` never writes it and `ConvertFromJson` restores `OriginalRecord = null`. `IxfrResponseBuilder.SerializeRdataWithoutCompression` returns empty RDATA when a loaded non-SOA/non-NS record has no `OriginalRecord`. +- **How it manifests:** After restart, persisted A, AAAA, MX, TXT, CNAME, SRV, PTR, CAA, and unknown record versions cannot be serialized correctly in IXFR. Slaves requesting IXFR from old persisted serials can receive malformed records or record deletions even though in-memory history worked before restart. +- **Fix instructions:** In `ZoneHistoryStorage`, persist canonical RDATA for every `FilteredRecord`. Add fields or a raw base64 payload that round-trips all served record types. In `ConvertFromJson`, restore enough type-specific data for `RecordComparer` and `IxfrResponseBuilder` to serialize identical RDATA. If a legacy history file lacks RDATA, mark that zone history unsafe for IXFR and force AXFR until rebuilt. Add a regression test that saves and reloads a zone with A, AAAA, MX, TXT, CNAME, CAA, SOA, and NS records, then builds an IXFR and asserts every RDATA length and payload is non-empty and correct. +- **Risk / blast radius:** Touches history file format, IXFR serialization, diffing, and any existing persisted files. Coordinate with [SEV-02] because both affect record identity. +- **Depends on:** none + +### [SEV-02] IXFR diff keys collapse duplicate RRsets and omit TTL changes + +- **Severity:** Critical +- **Confidence:** Confirmed +- **Location:** `FilterDns/Cache/RecordComparer.cs:43-52`, `FilterDns/Cache/RecordComparer.cs:119-128`, `FilterDns/Cache/RecordComparer.cs:154-172`, `FilterDns/Cache/RecordComparer.cs:227-239`, `FilterDns/Xfer/ZoneDiffCalculator.cs:45-80` +- **What's wrong:** `ZoneDiffCalculator` uses one dictionary entry per record key. When `OriginalRecord` is null, A/AAAA and unknown-type RDATA hashes become the hash of an empty string, so multiple records at the same owner/type/class collapse into one key. TTL is not included in equality or keys, so TTL-only updates are invisible. +- **How it manifests:** Round-robin A/AAAA, multiple MX/TXT values, or loaded history records can be dropped from IXFR diffs. A record with only a TTL change is never sent as delete+add, so slaves keep stale TTLs until AXFR. +- **Fix instructions:** Redesign record identity as a multiset over canonical owner, type, class, TTL, and canonical RDATA. Do not store duplicate RRset members in a single dictionary value; count duplicates or store lists per key. Update `RecordComparer.RecordsEqual` and `GetRecordKey` to use the same canonical data as IXFR serialization. Add tests for two A records on one name, two MX records with different exchanges, same RDATA with changed TTL, and records loaded from persisted history. +- **Risk / blast radius:** Changes IXFR output for all record types and depends on accurate RDATA persistence from [SEV-01]. +- **Depends on:** [SEV-01] + +### [SEV-03] Custom CIDR matching breaks ACLs and private-IP filtering + +- **Severity:** Critical +- **Confidence:** Confirmed +- **Location:** `FilterDns/Filter/RecordFilter.cs:195-204`, `FilterDns/Filter/RecordFilter.cs:223-272`, `FilterDns/Whitelist/IpWhitelist.cs:10-27`, `FilterDns/Whitelist/IpWhitelist.cs:52-81` +- **What's wrong:** Both `RecordFilter` and `IpWhitelist` compute `(prefixLength + 7) / 8`, compare that many full bytes, then also read the next byte for partial prefixes. Non-byte-aligned prefixes such as `/25`, `/17`, `/7`, and `/10` are evaluated incorrectly; exact network addresses can hit `IndexOutOfRangeException`. Invalid prefix lengths are accepted. +- **How it manifests:** Whitelisted clients inside valid CIDRs can be denied transfers or health checks. Custom private ranges can fail to match private records, leaking internal addresses into public zones. A probe against `192.168.1.0/25` confirmed `192.168.1.1` is denied and `192.168.1.0` throws. +- **Fix instructions:** Replace both implementations with one tested CIDR helper. Compare `prefixLength / 8` full bytes, then mask only the remaining bits in the last partial byte. Validate prefix ranges as 0-32 for IPv4 and 0-128 for IPv6, and reject address-family mismatches. Normalize IPv4-mapped IPv6 addresses before matching. Add tests for `/0`, `/8`, `/12`, `/17`, `/25`, `/32`, `/7`, `/10`, `/64`, `/128`, invalid negative prefixes, and too-large prefixes. +- **Risk / blast radius:** Affects XFER whitelist, health-check ACL, and custom private-IP filtering. Existing deployments with broken CIDRs may change behavior immediately. +- **Depends on:** none + +### [SEV-04] Hardened DNS compression parsing leaves the offset at the pointed-to name + +- **Severity:** High +- **Confidence:** Confirmed +- **Location:** `FilterDns/Dns/DnsMessageParser.cs:334-368`, `FilterDns/Dns/DnsMessageParser.cs:414-417` +- **What's wrong:** In the hardening branch of `ReadDomainName`, compression pointers are followed, but `jumped` and `jumpOffset` are never set. After resolving a compressed name, the caller resumes reading at the target name terminator instead of after the original two-byte pointer. +- **How it manifests:** Normal compressed DNS records are misparsed. A minimal IXFR-style authority SOA with owner `C0 0C` parsed as record type `IXFR` instead of `SOA`, so client serial extraction returned null and would fall back to AXFR. +- **Fix instructions:** In the hardening branch, on the first pointer, set `jumpOffset = offset + 1` and `jumped = true` before following the pointer. Continue loop-depth validation as now, and after name completion restore `offset = jumpOffset`. Add parser tests for compressed question references in answer, authority, and additional sections, including IXFR authority SOA serial extraction. +- **Risk / blast radius:** Central DNS parser used by UDP NOTIFY, health checks, TCP transfers, and NOTIFY response parsing. +- **Depends on:** none + +### [SEV-05] Mixed-case configured zone names break NOTIFY, transfers, and health checks + +- **Severity:** High +- **Confidence:** Confirmed +- **Location:** `FilterDns/Proxy/DnsProxyServer.cs:107-115`, `FilterDns/Xfer/XferHandler.cs:665-676`, `FilterDns/Xfer/XferHandler.cs:771-783`, `FilterDns/Xfer/XferHandler.cs:942-965` +- **What's wrong:** `_zones`, `_notifySenders`, and `_zoneUpdateSemaphores` are keyed with `zoneConfig.Name` exactly as configured. External request paths lower-case query names before lookup. A configured `Example.com` key is not found for `example.com`. +- **How it manifests:** Polling may work internally while AXFR/IXFR requests are refused as unknown zone, upstream NOTIFY is ignored, and health checks return NXDOMAIN. +- **Fix instructions:** Add one canonical-zone-name helper used at configuration load and every lookup: trim trailing dot, lower-case invariant, and consider IDNA if intended. Store `_zones`, `_notifySenders`, `_zoneUpdateSemaphores`, cache keys, and history keys using the canonical value. Preserve display names only for logs/config. Add tests with `Zones[].Name = "Example.COM."` and requests for `example.com` and `www.example.com`. +- **Risk / blast radius:** Touches all dictionaries keyed by zone name plus on-disk history filenames. Coordinate with migration behavior for existing mixed-case history files. +- **Depends on:** none + +### [SEV-06] Health-check cache misses update history and notify slaves + +- **Severity:** High +- **Confidence:** Confirmed +- **Location:** `FilterDns/Xfer/XferHandler.cs:803-824`, `FilterDns/Xfer/XferHandler.cs:1400-1443`, `FilterDns/Proxy/DnsProxyServer.cs:340-380` +- **What's wrong:** A health-check query from an allowed IP fetches upstream on cache miss, then calls `UpdateCacheAndNotifyAsync`. That callback updates history and sends NOTIFY to all slaves, even though the trigger was only a read-style health probe. +- **How it manifests:** A load balancer or monitor can trigger upstream AXFRs, zone history writes, and slave NOTIFY storms after startup or cache eviction. Slaves can initiate transfers because a health check asked for one record. +- **Fix instructions:** Split cache population from propagation. For health checks, fetch and filter data only for the response or populate cache without calling `onZoneUpdatedDuringTransfer` and without NOTIFY. If cache population is desired, serialize it through the same per-zone update lock and only notify slaves when an actual upstream poll/authorized NOTIFY detects a serial change. Add a test that clears cache, runs one health-check query, and asserts no `NotifySender.NotifyAllAsync` call occurs. +- **Risk / blast radius:** Touches health check behavior, cache warmup, and notification semantics. +- **Depends on:** none + +### [SEV-07] Transfer-triggered refreshes race poll/NOTIFY updates and notify slaves mid-transfer + +- **Severity:** High +- **Confidence:** Confirmed +- **Location:** `FilterDns/Proxy/DnsProxyServer.cs:516-531`, `FilterDns/Xfer/XferHandler.cs:1001-1036`, `FilterDns/Xfer/XferHandler.cs:1071-1105`, `FilterDns/Xfer/XferHandler.cs:1400-1443` +- **What's wrong:** Poll and upstream-NOTIFY updates are serialized by `_zoneUpdateSemaphores`, but inbound AXFR/IXFR refreshes call `UpdateCacheAndNotifyAsync` without that lock. They also notify all slaves while the requesting slave may still be in the middle of its transfer. +- **How it manifests:** Concurrent transfer, poll, and upstream NOTIFY paths can double-fetch, update cache/history out of order, create duplicate NOTIFYs, or send slaves into competing transfers. This is most likely during rapid serial changes or cold cache. +- **Fix instructions:** Route all cache/history mutations through one per-zone update service or semaphore shared by `DnsProxyServer` and `XferHandler`. Add a trigger reason parameter so slave-transfer refreshes can update cache/history without immediate NOTIFY, or can schedule debounced NOTIFY after the initiating transfer completes. Add a concurrency test with one transfer-triggered refresh and one poll update and assert serial/history order is deterministic. +- **Risk / blast radius:** Affects update orchestration, history consistency, NOTIFY timing, and slave synchronization. +- **Depends on:** none + +### [SEV-08] IXFR calculates diffs from live history after validating a snapshot + +- **Severity:** High +- **Confidence:** Confirmed +- **Location:** `FilterDns/Xfer/XferHandler.cs:1552-1613`, `FilterDns/Xfer/ZoneDiffCalculator.cs:95-110` +- **What's wrong:** `HandleIxfrRequestAsync` snapshots history for validation, but then passes the live `ZoneHistory` to `ZoneDiffCalculator.CalculateDiffSequence`. Concurrent adds/prunes/saves can change the versions used for the actual diff after validation succeeds. +- **How it manifests:** Under concurrent updates, IXFR can be built from a different history set than the one validated. Slaves may receive missing, extra, or inconsistent diff sequences, or fall back intermittently. +- **Fix instructions:** Change `CalculateDiffSequence` to accept an immutable `Dictionary` snapshot or a list of copied versions. Ensure both validation and diff calculation use the same snapshot and copied record lists. Add a test that mutates live history after validation but before diff calculation and asserts IXFR output still uses the original snapshot. +- **Risk / blast radius:** IXFR behavior and history APIs. This complements [SEV-07] but can be fixed independently inside the IXFR read path. +- **Depends on:** none + +### [SEV-09] Minimum record count is enforced after cache, history, and NOTIFY + +- **Severity:** High +- **Confidence:** Confirmed +- **Location:** `FilterDns/Proxy/DnsProxyServer.cs:627-699`, `FilterDns/Xfer/XferHandler.cs:1178-1200` +- **What's wrong:** Poll/NOTIFY refreshes cache filtered records and notify slaves without applying `MinimumZoneRecordCount`. The minimum is only checked later when a slave requests a transfer. +- **How it manifests:** With `MinimumZoneRecordCount = 10`, the proxy can cache a 3-record zone, notify slaves, then refuse the slave transfer. Verification may clear history and resend NOTIFY, but the transfer remains refused, creating a stuck recovery loop. +- **Fix instructions:** Enforce `MinimumZoneRecordCount` before cache update, history update, NOTIFY, and verification scheduling. If a fetched filtered zone is below the configured minimum, keep the last good cache, do not notify slaves, report a zone failure, and serve the previous good zone or REFUSED according to policy. Add tests for below-minimum startup fetch, poll update, NOTIFY-triggered update, and transfer-triggered refresh. +- **Risk / blast radius:** Changes reliability behavior for intentionally tiny zones; allow `0` to disable as documented. +- **Depends on:** none + +### [SEV-10] CAA records are serialized with an invalid extra value-length byte + +- **Severity:** Medium +- **Confidence:** Confirmed +- **Location:** `FilterDns/Dns/DnsRecordBuilder.cs:377-399`, `FilterDns/Dns/IxfrResponseBuilder.cs:437-445` +- **What's wrong:** RFC 6844 CAA RDATA is `flags`, `tag length`, `tag`, then `value` as the remaining bytes. Both serializers add an extra `value length` byte before the value. +- **How it manifests:** AXFR, IXFR, and health-check answers containing CAA records are not wire-compatible. Slaves or validators may reject or misread CAA policy. +- **Fix instructions:** Remove the extra `data.Add((byte)valueBytes.Length)` from both CAA serializers. Update comments to state that only the tag is length-prefixed. Add a wire-format test for `0 issue "letsencrypt.org"` and assert the RDATA bytes are `00 05 69 73 73 75 65 ...` with no value-length octet. +- **Risk / blast radius:** Affects only CAA output; update any tests that encoded the old behavior. +- **Depends on:** none + +### [SEV-11] AXFR size limit can abort after sending a partial transfer + +- **Severity:** Medium +- **Confidence:** Confirmed +- **Location:** `FilterDns/Xfer/XferHandler.cs:1207-1230`, `FilterDns/Xfer/XferHandler.cs:1261-1308` +- **What's wrong:** There is a rough pre-check based on `recordCount * 100`, then exact byte checks while streaming. If the exact count exceeds `MaxZoneTransferSizeBytes` after the opening SOA or a later record, the method returns without sending a DNS error or a complete trailing SOA. +- **How it manifests:** Slaves receive an incomplete AXFR stream and may hang, retry, or install no zone. Logs show an abort, but the client sees a truncated transfer rather than a clean refusal. +- **Fix instructions:** Compute exact transfer size before writing any AXFR messages, or refuse before streaming when the configured limit would be exceeded. If exact precomputation is too expensive, remove the mid-stream partial-send path and prefer conservative pre-refusal. Add a test with a low size limit and assert the first response is REFUSED or SERVFAIL with no partial SOA/record stream. +- **Risk / blast radius:** Affects very large zones and defensive limits. +- **Depends on:** none + +### [SEV-12] NOTIFY receive sends NOERROR before validating zone and source + +- **Severity:** Medium +- **Confidence:** Confirmed +- **Location:** `FilterDns/Xfer/XferHandler.cs:658-676`, `FilterDns/Xfer/XferHandler.cs:680-735` +- **What's wrong:** The UDP NOTIFY handler sends `NoError` immediately for any SOA NOTIFY question, then checks whether the zone exists and whether the sender matches the configured upstream. +- **How it manifests:** Unauthorized or unknown-zone NOTIFY senders receive a successful DNS response even though the proxy ignores the update. An upstream with a source-address mismatch may believe the proxy accepted the NOTIFY. +- **Fix instructions:** First select the zone, validate source address, and decide the response code. Send exactly one response per NOTIFY message. Use `Refused` or `NotAuth`-equivalent policy for unauthorized/unknown zones, and `NoError` only after acceptance. Add tests for valid upstream, wrong upstream, unknown zone, and multiple SOA questions. +- **Risk / blast radius:** Changes DNS response behavior for NOTIFY clients. +- **Depends on:** [SEV-05] + +### [SEV-13] UDP concurrency limiting silently drops DNS packets + +- **Severity:** Medium +- **Confidence:** Confirmed +- **Location:** `FilterDns/Xfer/XferHandler.cs:215-229` +- **What's wrong:** When `MaxConcurrentUdpRequests` is saturated, the server logs and `continue`s without any DNS response. +- **How it manifests:** Health checks time out instead of receiving SERVFAIL/REFUSED. NOTIFY senders see packet loss and may retry in bursts, worsening load. +- **Fix instructions:** For parseable DNS packets, send a minimal `ServFail` or `Refused` response using the request ID before dropping work. If parsing is too expensive under saturation, document an intentional drop policy and exempt health-check/NOTIFY response expectations. Add a load test that saturates the semaphore and asserts clients receive deterministic DNS errors. +- **Risk / blast radius:** Affects overload behavior and monitoring reliability. +- **Depends on:** none + +### [SEV-14] Verification mismatch recovery does not schedule a retry check + +- **Severity:** Medium +- **Confidence:** Confirmed +- **Location:** `FilterDns/Verify/SlaveVerificationService.cs:110-146`, `FilterDns/Verify/SlaveVerificationService.cs:181-187` +- **What's wrong:** On mismatch within retry budget, the service may clear history and resend NOTIFY, then exits. It does not schedule another delayed verification for the same expected serial. The retry counter only advances if a later external `ScheduleVerification` call happens. +- **How it manifests:** If the zone is quiet after a mismatch, slaves can remain wrong indefinitely even though logs say retry attempt N of M. +- **Fix instructions:** After re-NOTIFY, schedule a delayed verification for the same zone/serial/record count with backoff until success or `maxRetries`. Keep one pending verification per zone and cancel/replace it only when a newer serial is scheduled. Add tests where the first verification mismatches, the second succeeds, and no new zone update occurs. +- **Risk / blast radius:** Affects recovery timing and NOTIFY volume. +- **Depends on:** none + +### [SEV-15] Self-restart verification and window-limit config is not wired + +- **Severity:** Medium +- **Confidence:** Confirmed +- **Location:** `FilterDns/Config/Configuration.cs:82-118`, `FilterDns/Recovery/SelfRestartService.cs:70-87`, `FilterDns/Recovery/SelfRestartService.cs:146-205` +- **What's wrong:** `MaxRestartsInWindow` and `RestartWindowSeconds` are configured but never enforced. `ReportVerificationFailure` implements per-zone verification failure counting, but no verification code calls it. +- **How it manifests:** A flapping service can restart indefinitely after uptime threshold. Per-zone verification failure thresholds do not work as described; only a later `ReportCriticalError` path can contribute to restart logic. +- **Fix instructions:** Persist or otherwise track restart timestamps across process exits, then enforce `MaxRestartsInWindow` within `RestartWindowSeconds` before `Environment.Exit`. Call `ReportVerificationFailure` from slave verification mismatch paths or remove the unused config/API and document the actual behavior. Add tests for restart suppression after N restarts and for verification mismatch triggering the configured threshold. +- **Risk / blast radius:** Affects operational recovery and service-manager behavior. +- **Depends on:** none + +### [SEV-16] Startup/export configuration validation misses important contract checks + +- **Severity:** Medium +- **Confidence:** Confirmed +- **Location:** `FilterDns/Program.cs:47-65`, `FilterDns/Program.cs:148-196`, `FilterDns/Program.cs:223-256`, `FilterDns/Upstream/UpstreamClient.cs:14-20` +- **What's wrong:** Server startup validation only checks zones, `Ns1`/`Ns2`, and zone-name syntax. Export skips validation entirely. Invalid log levels throw `Enum.Parse` exceptions, malformed upstream/listen addresses fail later, duplicate zones overwrite silently, and invalid private ranges can silently keep records public. +- **How it manifests:** Misconfiguration is discovered at runtime with unclear exceptions or masked behavior. `export` can run with config that the server would reject, and it uses the CLI zone spelling for upstream fetch/filtering instead of the canonical configured name. +- **Fix instructions:** Create a shared validator used by both server and export. Validate log levels with `Enum.TryParse`, upstream as host/IP plus port, listen address/port, `IxfrResponseMode`, duplicate canonical zone names, email required fields, slave ports, whitelist entries, and private ranges. After export zone lookup, use `zoneConfig.Name` for fetch/filter/export. Add negative config tests for each invalid field and assert the error names the bad key. +- **Risk / blast radius:** Startup behavior becomes stricter; may reject configs that previously limped along. +- **Depends on:** [SEV-03], [SEV-05] + +### [SEV-17] History save can mutate live history and replace files non-atomically + +- **Severity:** Medium +- **Confidence:** Confirmed +- **Location:** `FilterDns/Cache/ZoneHistoryStorage.cs:281-291`, `FilterDns/Cache/ZoneHistoryStorage.cs:339-347`, `FilterDns/Cache/ZoneHistory.cs:143-176` +- **What's wrong:** `SaveAsync` prunes the live `ZoneHistory` object as a side effect, while `UpdateZoneHistoryAsync` already prunes by effective history depth. The disk replace deletes the old file before moving the temp file into place. +- **How it manifests:** A save can remove versions during a concurrent IXFR read. A crash between delete and move leaves no valid JSON history file, losing IXFR history on restart. +- **Fix instructions:** Make `SaveAsync` serialize a copied/pruned view rather than mutating the live history. Keep pruning ownership in one place. Replace files atomically with `File.Replace` when a target exists, or with a same-directory atomic move that never deletes the last valid file first. Add tests for saving while history has more than max versions and for simulated replace failure preserving the old file. +- **Risk / blast radius:** Affects persistence, retention, and IXFR availability. +- **Depends on:** none + +### [SEV-18] Path containment and symlink checks fail open in storage hardening + +- **Severity:** Low +- **Confidence:** Confirmed +- **Location:** `FilterDns/Cache/ZoneHistoryStorage.cs:190-215`, `FilterDns/Cache/ZoneHistoryStorage.cs:221-240` +- **What's wrong:** `ValidatePathWithinDirectory` uses a plain string prefix check, so `/var/filterdns/data-backup/file` starts with `/var/filterdns/data`. `IsSymlink` returns false if attribute inspection fails. +- **How it manifests:** Security hardening can accept paths outside the intended directory in prefix-collision layouts and can fail open when symlink metadata cannot be read. +- **Fix instructions:** Normalize base and target with `Path.GetFullPath`, ensure the base path has a trailing separator, and compare with path-segment boundaries. If symlink inspection fails under hardening, fail closed. Add tests for sibling-prefix paths, symlink files, symlink directories, and inspection exceptions. +- **Risk / blast radius:** Mostly affects hardened file-operation guarantees and unusual filesystem layouts. +- **Depends on:** none + +### [SEV-19] Critical paths have no automated tests + +- **Severity:** Low +- **Confidence:** Confirmed +- **Location:** `FilterDns.sln:5`, `FilterDns/FilterDns.csproj:1-20` +- **What's wrong:** The solution contains only the executable project and no test project. The codebase has complex DNS parser, CIDR, history persistence, IXFR diffing, and transfer orchestration logic with no regression coverage. +- **How it manifests:** Parser regressions, wire-format mistakes, history corruption, and concurrency bugs can ship while `dotnet build` remains green. +- **Fix instructions:** Add a focused test project to the solution. Start with unit tests for `DnsMessageParser`, `IpWhitelist`, `RecordFilter` private-range behavior via public filtering, `ZoneHistoryStorage` round-trips, `RecordComparer`, `ZoneDiffCalculator`, and CAA wire serialization. Add a small integration harness for AXFR/IXFR using in-memory or loopback DNS packets. Keep tests deterministic and not dependent on external DNS. +- **Risk / blast radius:** Adds test infrastructure only; should not change runtime behavior. +- **Depends on:** none From f5c0e86e9d920cb37ff3439f2f2c7236e403372a Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Thu, 25 Jun 2026 05:41:33 +0000 Subject: [PATCH 2/7] Fix critical DNS history and CIDR bugs Co-authored-by: Gerold K. --- FilterDns.Tests/CriticalFindingTests.cs | 330 ++++++++++++++++++++++++ FilterDns.Tests/FilterDns.Tests.csproj | 26 ++ FilterDns.sln | 8 +- FilterDns/Cache/RecordComparer.cs | 159 +----------- FilterDns/Cache/ZoneHistoryStorage.cs | 50 +++- FilterDns/Dns/DnsRecordBuilder.cs | 12 +- FilterDns/Dns/IxfrResponseBuilder.cs | 5 + FilterDns/Dns/RDataSerializer.cs | 183 +++++++++++++ FilterDns/Filter/RecordFilter.cs | 27 +- FilterDns/Net/CidrRange.cs | 71 +++++ FilterDns/Whitelist/IpWhitelist.cs | 32 +-- REVIEW_FINDINGS.md | 10 + 12 files changed, 697 insertions(+), 216 deletions(-) create mode 100644 FilterDns.Tests/CriticalFindingTests.cs create mode 100644 FilterDns.Tests/FilterDns.Tests.csproj create mode 100644 FilterDns/Dns/RDataSerializer.cs create mode 100644 FilterDns/Net/CidrRange.cs diff --git a/FilterDns.Tests/CriticalFindingTests.cs b/FilterDns.Tests/CriticalFindingTests.cs new file mode 100644 index 0000000..7c24b22 --- /dev/null +++ b/FilterDns.Tests/CriticalFindingTests.cs @@ -0,0 +1,330 @@ +using System.Net; +using DnsClient; +using DnsClient.Protocol; +using FilterDns.Cache; +using FilterDns.Config; +using FilterDns.Dns; +using FilterDns.Filter; +using FilterDns.Whitelist; +using FilterDns.Xfer; + +namespace FilterDns.Tests; + +public class CriticalFindingTests +{ + [Fact] + public void IpWhitelist_AllowsAddressesInsideNonByteAlignedIpv4Cidr() + { + var whitelist = new IpWhitelist(["192.168.1.0/25"]); + + Assert.True(whitelist.Allows(IPAddress.Parse("192.168.1.0"))); + Assert.True(whitelist.Allows(IPAddress.Parse("192.168.1.1"))); + Assert.True(whitelist.Allows(IPAddress.Parse("192.168.1.64"))); + Assert.False(whitelist.Allows(IPAddress.Parse("192.168.1.128"))); + } + + [Fact] + public void RecordFilter_FiltersCustomNonByteAlignedPrivateIpv6Range() + { + var config = new ZoneConfig + { + Name = "example.com", + Ns1 = "ns1.example.com", + Ns2 = "ns2.example.com", + FilterPrivateIPs = true, + PrivateIPRanges = ["fc00::/7"] + }; + var records = new DnsResourceRecord[] + { + new AaaaRecord( + new ResourceRecordInfo("secret.example.com.", ResourceRecordType.AAAA, QueryClass.IN, 300, 16), + IPAddress.Parse("fd00::1")) + }; + + var filtered = RecordFilter.ApplyFilters(records, config, "example.com"); + + Assert.DoesNotContain(filtered, record => record.DomainName == "secret.example.com."); + } + + [Fact] + public async Task ZoneHistoryStorage_RoundTripsARecordRdataForLoadedHistory() + { + var dataDirectory = Path.Combine(Path.GetTempPath(), $"filterdns-tests-{Guid.NewGuid():N}"); + var storage = new ZoneHistoryStorage(dataDirectory, exportBindZoneFiles: false); + var history = new ZoneHistory("example.com"); + var record = new FilteredRecord + { + DomainName = "www.example.com.", + RecordType = ResourceRecordType.A, + RecordClass = QueryClass.IN, + TimeToLive = 300, + OriginalRecord = new ARecord( + new ResourceRecordInfo("www.example.com.", ResourceRecordType.A, QueryClass.IN, 300, 4), + IPAddress.Parse("203.0.113.10")) + }; + history.AddVersion(1, [record]); + + await storage.SaveAsync(history); + var loaded = await storage.LoadAsync("example.com"); + var loadedRecord = Assert.Single(loaded!.GetVersion(1)!.Records); + + var response = DnsRecordBuilder.BuildQueryResponse( + new DnsMessage + { + Id = 0x1234, + Questions = + [ + new FilterDns.Dns.DnsQuestion + { + Name = "www.example.com.", + QueryType = DnsQueryType.A, + QueryClass = DnsQueryClass.IN + } + ] + }, + [loadedRecord]); + + var rdata = ExtractFirstAnswerRdata(response); + Assert.Equal(IPAddress.Parse("203.0.113.10").GetAddressBytes(), rdata); + } + + [Fact] + public async Task ZoneHistoryStorage_RoundTripsCommonRecordTypeRdataForLoadedHistory() + { + var dataDirectory = Path.Combine(Path.GetTempPath(), $"filterdns-tests-{Guid.NewGuid():N}"); + var storage = new ZoneHistoryStorage(dataDirectory, exportBindZoneFiles: false); + var records = new List + { + RecordFromOriginal( + new AaaaRecord( + new ResourceRecordInfo("v6.example.com.", ResourceRecordType.AAAA, QueryClass.IN, 300, 16), + IPAddress.Parse("2001:db8::10"))), + RecordFromOriginal( + new MxRecord( + new ResourceRecordInfo("example.com.", ResourceRecordType.MX, QueryClass.IN, 300, 0), + 10, + DnsString.Parse("mail.example.com."))), + RecordFromOriginal( + new TxtRecord( + new ResourceRecordInfo("txt.example.com.", ResourceRecordType.TXT, QueryClass.IN, 300, 0), + ["hello"], + ["hello"])), + RecordFromOriginal( + new CNameRecord( + new ResourceRecordInfo("alias.example.com.", ResourceRecordType.CNAME, QueryClass.IN, 300, 0), + DnsString.Parse("www.example.com."))), + RecordFromOriginal( + new CaaRecord( + new ResourceRecordInfo("example.com.", ResourceRecordType.CAA, QueryClass.IN, 300, 0), + 0, + "issue", + "letsencrypt.org")), + new() + { + DomainName = "example.com.", + RecordType = ResourceRecordType.SOA, + RecordClass = QueryClass.IN, + TimeToLive = 300, + SoaData = new SoaRecordData + { + MName = "ns1.example.com.", + RName = "admin.example.com.", + Serial = 1, + Refresh = 3600, + Retry = 600, + Expire = 604800, + Minimum = 300 + } + }, + new() + { + DomainName = "example.com.", + RecordType = ResourceRecordType.NS, + RecordClass = QueryClass.IN, + TimeToLive = 300, + NsName = "ns1.example.com." + } + }; + var expectedRdata = records.ToDictionary( + record => $"{record.DomainName}|{record.RecordType}", + RDataSerializer.Serialize); + var history = new ZoneHistory("example.com"); + history.AddVersion(1, records); + + await storage.SaveAsync(history); + var loaded = await storage.LoadAsync("example.com"); + + Assert.NotNull(loaded); + foreach (var loadedRecord in loaded.GetVersion(1)!.Records) + { + Assert.Equal( + expectedRdata[$"{loadedRecord.DomainName}|{loadedRecord.RecordType}"], + RDataSerializer.Serialize(loadedRecord)); + } + } + + [Fact] + public async Task ZoneHistoryStorage_RejectsUnsupportedRecordWithoutSerializableRdataOnSave() + { + var dataDirectory = Path.Combine(Path.GetTempPath(), $"filterdns-tests-{Guid.NewGuid():N}"); + var storage = new ZoneHistoryStorage(dataDirectory, exportBindZoneFiles: false); + var history = new ZoneHistory("example.com"); + history.AddVersion(1, + [ + new FilteredRecord + { + DomainName = "unknown.example.com.", + RecordType = (ResourceRecordType)65000, + RecordClass = QueryClass.IN, + TimeToLive = 300 + } + ]); + + await Assert.ThrowsAsync(() => storage.SaveAsync(history)); + } + + [Fact] + public async Task ZoneHistoryStorage_RejectsLegacyHistoryWithoutRequiredRdata() + { + var dataDirectory = Path.Combine(Path.GetTempPath(), $"filterdns-tests-{Guid.NewGuid():N}"); + var historyDirectory = Path.Combine(dataDirectory, "history"); + Directory.CreateDirectory(historyDirectory); + await File.WriteAllTextAsync( + Path.Combine(historyDirectory, "example_com.json"), + """ + [ + { + "serial": 1, + "timestamp": "2026-06-25T00:00:00Z", + "records": [ + { + "domainName": "www.example.com.", + "recordType": 1, + "recordClass": 1, + "timeToLive": 300 + } + ] + } + ] + """); + var storage = new ZoneHistoryStorage(dataDirectory, exportBindZoneFiles: false); + + var loaded = await storage.LoadAsync("example.com"); + + Assert.Null(loaded); + } + + [Fact] + public void ZoneDiffCalculator_TreatsTtlChangeAsDeleteAndAdd() + { + var oldRecord = new FilteredRecord + { + DomainName = "www.example.com.", + RecordType = ResourceRecordType.A, + RecordClass = QueryClass.IN, + TimeToLive = 300, + OriginalRecord = new ARecord( + new ResourceRecordInfo("www.example.com.", ResourceRecordType.A, QueryClass.IN, 300, 4), + IPAddress.Parse("203.0.113.10")) + }; + var newRecord = new FilteredRecord + { + DomainName = "www.example.com.", + RecordType = ResourceRecordType.A, + RecordClass = QueryClass.IN, + TimeToLive = 600, + OriginalRecord = new ARecord( + new ResourceRecordInfo("www.example.com.", ResourceRecordType.A, QueryClass.IN, 600, 4), + IPAddress.Parse("203.0.113.10")) + }; + + var diff = ZoneDiffCalculator.CalculateDiff([oldRecord], 1, [newRecord], 2); + + Assert.Single(diff.DeletedRecords); + Assert.Single(diff.AddedRecords); + } + + [Fact] + public void ZoneDiffCalculator_DistinguishesLoadedRecordsByRawRdata() + { + var oldRecords = new List + { + LoadedARecord("www.example.com.", 300, "203.0.113.10"), + LoadedARecord("www.example.com.", 300, "203.0.113.11") + }; + var newRecords = new List + { + LoadedARecord("www.example.com.", 300, "203.0.113.10") + }; + + var diff = ZoneDiffCalculator.CalculateDiff(oldRecords, 1, newRecords, 2); + + var deleted = Assert.Single(diff.DeletedRecords); + Assert.Equal(IPAddress.Parse("203.0.113.11").GetAddressBytes(), deleted.RawRData); + Assert.Empty(diff.AddedRecords); + } + + private static byte[] ExtractFirstAnswerRdata(byte[] message) + { + var offset = 12; + SkipDomainName(message, ref offset); + offset += 4; + SkipDomainName(message, ref offset); + offset += 2; // type + offset += 2; // class + offset += 4; // ttl + var rdLength = ReadUInt16(message, ref offset); + return message.Skip(offset).Take(rdLength).ToArray(); + } + + private static FilteredRecord LoadedARecord(string name, int ttl, string address) + { + return new FilteredRecord + { + DomainName = name, + RecordType = ResourceRecordType.A, + RecordClass = QueryClass.IN, + TimeToLive = ttl, + RawRData = IPAddress.Parse(address).GetAddressBytes() + }; + } + + private static FilteredRecord RecordFromOriginal(DnsResourceRecord record) + { + return new FilteredRecord + { + DomainName = record.DomainName.Value, + RecordType = record.RecordType, + RecordClass = record.RecordClass, + TimeToLive = record.TimeToLive, + OriginalRecord = record + }; + } + + private static void SkipDomainName(byte[] message, ref int offset) + { + while (offset < message.Length) + { + var length = message[offset++]; + if (length == 0) + { + return; + } + + if ((length & 0xC0) == 0xC0) + { + offset++; + return; + } + + offset += length; + } + } + + private static ushort ReadUInt16(byte[] message, ref int offset) + { + var value = (ushort)((message[offset] << 8) | message[offset + 1]); + offset += 2; + return value; + } +} diff --git a/FilterDns.Tests/FilterDns.Tests.csproj b/FilterDns.Tests/FilterDns.Tests.csproj new file mode 100644 index 0000000..698f4ff --- /dev/null +++ b/FilterDns.Tests/FilterDns.Tests.csproj @@ -0,0 +1,26 @@ + + + + net10.0 + enable + enable + false + + + + + + + + + + + + + + + + + + + \ No newline at end of file diff --git a/FilterDns.sln b/FilterDns.sln index f74b874..3c66830 100644 --- a/FilterDns.sln +++ b/FilterDns.sln @@ -1,9 +1,12 @@ + Microsoft Visual Studio Solution File, Format Version 12.00 # Visual Studio Version 18 VisualStudioVersion = 18.0.0.0 MinimumVisualStudioVersion = 10.0.40219.1 Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "FilterDns", "FilterDns\FilterDns.csproj", "{A1B2C3D4-E5F6-4A5B-8C9D-0E1F2A3B4C5D}" EndProject +Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "FilterDns.Tests", "FilterDns.Tests\FilterDns.Tests.csproj", "{10439BEF-E845-4782-9AF7-5BE664EB0984}" +EndProject Global GlobalSection(SolutionConfigurationPlatforms) = preSolution Debug|Any CPU = Debug|Any CPU @@ -14,9 +17,12 @@ Global {A1B2C3D4-E5F6-4A5B-8C9D-0E1F2A3B4C5D}.Debug|Any CPU.Build.0 = Debug|Any CPU {A1B2C3D4-E5F6-4A5B-8C9D-0E1F2A3B4C5D}.Release|Any CPU.ActiveCfg = Release|Any CPU {A1B2C3D4-E5F6-4A5B-8C9D-0E1F2A3B4C5D}.Release|Any CPU.Build.0 = Release|Any CPU + {10439BEF-E845-4782-9AF7-5BE664EB0984}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {10439BEF-E845-4782-9AF7-5BE664EB0984}.Debug|Any CPU.Build.0 = Debug|Any CPU + {10439BEF-E845-4782-9AF7-5BE664EB0984}.Release|Any CPU.ActiveCfg = Release|Any CPU + {10439BEF-E845-4782-9AF7-5BE664EB0984}.Release|Any CPU.Build.0 = Release|Any CPU EndGlobalSection GlobalSection(SolutionProperties) = preSolution HideSolutionNode = FALSE EndGlobalSection EndGlobal - diff --git a/FilterDns/Cache/RecordComparer.cs b/FilterDns/Cache/RecordComparer.cs index 1fd2129..1900cc3 100644 --- a/FilterDns/Cache/RecordComparer.cs +++ b/FilterDns/Cache/RecordComparer.cs @@ -1,8 +1,7 @@ -using System.Net; -using System.Net.Sockets; using System.Security.Cryptography; using System.Text; using DnsClient.Protocol; +using FilterDns.Dns; using FilterDns.Filter; namespace FilterDns.Cache; @@ -32,6 +31,9 @@ public static bool RecordsEqual(FilteredRecord a, FilteredRecord b) if (a.RecordClass != b.RecordClass) return false; + if (a.TimeToLive != b.TimeToLive) + return false; + // Compare RDATA based on record type return CompareRdata(a, b); } @@ -48,7 +50,7 @@ public static string GetRecordKey(FilteredRecord record) var domainName = record.DomainName.TrimEnd('.').ToLowerInvariant(); var rdataHash = GetRdataHash(record); - return $"{domainName}|{(int)record.RecordType}|{(int)record.RecordClass}|{rdataHash}"; + return $"{domainName}|{(int)record.RecordType}|{(int)record.RecordClass}|{record.TimeToLive}|{rdataHash}"; } /// @@ -79,23 +81,6 @@ private static bool CompareRdata(FilteredRecord a, FilteredRecord b) return string.Equals(aNsName, bNsName, StringComparison.OrdinalIgnoreCase); } - // A records: compare IP address - if (a.RecordType == ResourceRecordType.A) - { - var aIp = ExtractIpAddress(a, AddressFamily.InterNetwork); - var bIp = ExtractIpAddress(b, AddressFamily.InterNetwork); - return aIp != null && bIp != null && aIp.Equals(bIp); - } - - // AAAA records: compare IPv6 address - if (a.RecordType == ResourceRecordType.AAAA) - { - var aIp = ExtractIpAddress(a, AddressFamily.InterNetworkV6); - var bIp = ExtractIpAddress(b, AddressFamily.InterNetworkV6); - return aIp != null && bIp != null && aIp.Equals(bIp); - } - - // For other record types, compare raw RDATA return CompareRawRdata(a, b); } @@ -116,14 +101,6 @@ private static string GetRdataHash(FilteredRecord record) return ComputeHash(nsName ?? string.Empty); } - if (record.RecordType == ResourceRecordType.A || record.RecordType == ResourceRecordType.AAAA) - { - var ip = ExtractIpAddress(record, - record.RecordType == ResourceRecordType.A ? AddressFamily.InterNetwork : AddressFamily.InterNetworkV6); - return ComputeHash(ip?.ToString() ?? string.Empty); - } - - // For other types, use raw RDATA var rawRdata = GetRawRdata(record); return ComputeHash(Convert.ToBase64String(rawRdata)); } @@ -153,60 +130,7 @@ private static bool CompareRawRdata(FilteredRecord a, FilteredRecord b) /// private static byte[] GetRawRdata(FilteredRecord record) { - if (record.OriginalRecord != null) - { - // Try to get raw RDATA from original record using reflection - var rawRdata = TryGetRawRdataFromOriginal(record.OriginalRecord); - if (rawRdata != null && rawRdata.Length > 0) - { - return rawRdata; - } - } - - // Fallback: serialize based on known types - return record.RecordType switch - { - ResourceRecordType.SOA when record.SoaData != null => SerializeSoaRdata(record.SoaData), - ResourceRecordType.NS when record.NsName != null => SerializeNsRdata(record.NsName), - _ => Array.Empty() - }; - } - - /// - /// Tries to extract raw RDATA from DnsResourceRecord using reflection. - /// - private static byte[]? TryGetRawRdataFromOriginal(DnsResourceRecord record) - { - try - { - // Try to access Raw property - var rawProperty = record.GetType().GetProperty("Raw", - System.Reflection.BindingFlags.Public | System.Reflection.BindingFlags.Instance); - if (rawProperty?.PropertyType == typeof(byte[])) - { - var rawValue = rawProperty.GetValue(record) as byte[]; - if (rawValue != null && rawValue.Length > 0) - { - // Extract RDATA portion (skip name, type, class, TTL - typically 12-16 bytes) - // This is a simplified approach - full parsing would be more accurate - if (rawValue.Length > 16) - { - // Find RDATA length (2 bytes after TTL) - var rdataLength = (rawValue[rawValue.Length - 2] << 8) | rawValue[rawValue.Length - 1]; - // For now, return the last portion which should contain RDATA - // In practice, we'd need proper DNS record parsing - return rawValue; - } - return rawValue; - } - } - } - catch - { - // Reflection failed, fall back to type-specific serialization - } - - return null; + return RDataSerializer.Serialize(record); } /// @@ -221,77 +145,6 @@ private static byte[] GetRawRdata(FilteredRecord record) return null; } - /// - /// Extracts IP address from record. - /// - private static IPAddress? ExtractIpAddress(FilteredRecord record, AddressFamily family) - { - if (record.OriginalRecord is ARecord aRecord && family == AddressFamily.InterNetwork) - { - return aRecord.Address; - } - - if (record.OriginalRecord is AaaaRecord aaaaRecord && family == AddressFamily.InterNetworkV6) - { - return aaaaRecord.Address; - } - - return null; - } - - /// - /// Serializes SOA RDATA. - /// - private static byte[] SerializeSoaRdata(SoaRecordData soa) - { - var data = new List(); - WriteDomainName(data, soa.MName); - WriteDomainName(data, soa.RName); - WriteUInt32(data, soa.Serial); - WriteUInt32(data, soa.Refresh); - WriteUInt32(data, soa.Retry); - WriteUInt32(data, soa.Expire); - WriteUInt32(data, soa.Minimum); - return data.ToArray(); - } - - /// - /// Serializes NS RDATA. - /// - private static byte[] SerializeNsRdata(string nsName) - { - var data = new List(); - WriteDomainName(data, nsName); - return data.ToArray(); - } - - /// - /// Writes a domain name in DNS format. - /// - private static void WriteDomainName(List data, string name) - { - var parts = name.TrimEnd('.').Split('.'); - foreach (var part in parts) - { - if (string.IsNullOrEmpty(part)) continue; - var partBytes = Encoding.UTF8.GetBytes(part); - data.Add((byte)partBytes.Length); - data.AddRange(partBytes); - } - data.Add(0); // Terminator - } - - /// - /// Writes a 32-bit unsigned integer in network byte order. - /// - private static void WriteUInt32(List data, uint value) - { - data.Add((byte)(value >> 24)); - data.Add((byte)(value >> 16)); - data.Add((byte)(value >> 8)); - data.Add((byte)(value & 0xFF)); - } - /// /// Computes a SHA256 hash of the input string and returns it as a hex string. /// diff --git a/FilterDns/Cache/ZoneHistoryStorage.cs b/FilterDns/Cache/ZoneHistoryStorage.cs index 40b48a4..94c3767 100644 --- a/FilterDns/Cache/ZoneHistoryStorage.cs +++ b/FilterDns/Cache/ZoneHistoryStorage.cs @@ -2,9 +2,11 @@ using System.Text.Json; using System.Text; using System.Security.Cryptography; +using DnsClient.Protocol; using FilterDns.Export; using FilterDns.Filter; using FilterDns.Config; +using FilterDns.Dns; using Microsoft.Extensions.Logging; namespace FilterDns.Cache; @@ -503,12 +505,16 @@ private FilteredRecordJson ConvertToJson(FilteredRecord record) }; } - // For records with OriginalRecord, we can't fully serialize them, - // so we'll need to reconstruct them on load from the zone data - // For now, we'll store a placeholder - in practice, we'd need to serialize - // the RDATA properly based on record type - // This is a limitation: we can't fully restore OriginalRecord from JSON - // But for IXFR purposes, we mainly need FilteredRecord data which we can restore + var rawRdata = RDataSerializer.Serialize(record); + if (rawRdata.Length > 0) + { + json.RawRdataBase64 = Convert.ToBase64String(rawRdata); + } + else if (RequiresRawRdata(json)) + { + throw new InvalidOperationException( + $"Record {record.DomainName} type {record.RecordType} cannot be persisted without raw RDATA"); + } return json; } @@ -520,13 +526,20 @@ private FilteredRecordJson ConvertToJson(FilteredRecord record) /// private FilteredRecord ConvertFromJson(FilteredRecordJson json) { + if (string.IsNullOrEmpty(json.RawRdataBase64) && RequiresRawRdata(json)) + { + throw new InvalidDataException( + $"Persisted record {json.DomainName} type {json.RecordType} is missing raw RDATA and cannot be safely used for IXFR"); + } + var record = new FilteredRecord { DomainName = json.DomainName, - RecordType = (DnsClient.Protocol.ResourceRecordType)json.RecordType, + RecordType = (ResourceRecordType)json.RecordType, RecordClass = (DnsClient.QueryClass)json.RecordClass, TimeToLive = json.TimeToLive, NsName = json.NsName, + RawRData = string.IsNullOrEmpty(json.RawRdataBase64) ? null : Convert.FromBase64String(json.RawRdataBase64), OriginalRecord = null! // Cannot restore from JSON }; @@ -547,6 +560,17 @@ private FilteredRecord ConvertFromJson(FilteredRecordJson json) return record; } + private static bool RequiresRawRdata(FilteredRecordJson json) + { + var recordType = (ResourceRecordType)json.RecordType; + return recordType switch + { + ResourceRecordType.SOA => json.SoaData == null, + ResourceRecordType.NS => string.IsNullOrEmpty(json.NsName), + _ => true + }; + } + /// /// Gets the directory path for BIND zone files. /// @@ -688,7 +712,7 @@ private string CalculateZoneVersionHash(uint serial, List re // Include a hash of record data for stronger integrity var recordsHash = string.Join("|", records.Select(r => - $"{r.DomainName}:{r.RecordType}:{r.RecordClass}:{r.TimeToLive}")); + $"{r.DomainName}:{r.RecordType}:{r.RecordClass}:{r.TimeToLive}:{r.NsName}:{r.RawRdataBase64}:{SerializeSoaForHash(r.SoaData)}")); hashInput += $"|{recordsHash}"; @@ -696,4 +720,14 @@ private string CalculateZoneVersionHash(uint serial, List re var hashBytes = SHA256.HashData(inputBytes); return Convert.ToHexString(hashBytes).ToLowerInvariant(); } + + private static string SerializeSoaForHash(SoaRecordDataJson? soaData) + { + if (soaData == null) + { + return string.Empty; + } + + return $"{soaData.MName}:{soaData.RName}:{soaData.Serial}:{soaData.Refresh}:{soaData.Retry}:{soaData.Expire}:{soaData.Minimum}"; + } } diff --git a/FilterDns/Dns/DnsRecordBuilder.cs b/FilterDns/Dns/DnsRecordBuilder.cs index 3cff54c..e5a1a05 100644 --- a/FilterDns/Dns/DnsRecordBuilder.cs +++ b/FilterDns/Dns/DnsRecordBuilder.cs @@ -144,7 +144,11 @@ private static void WriteRecordWithCompression( private static byte[] SerializeRdata(FilteredRecord record) { - if (record.SoaData != null) + if (record.RawRData is { Length: > 0 }) + { + return record.RawRData.ToArray(); + } + else if (record.SoaData != null) { return SerializeSoa(record.SoaData); } @@ -164,7 +168,11 @@ private static byte[] SerializeRdataWithCompression( Dictionary nameCompression, string zoneName) { - if (record.SoaData != null) + if (record.RawRData is { Length: > 0 }) + { + return record.RawRData.ToArray(); + } + else if (record.SoaData != null) { return SerializeSoaWithCompression(record.SoaData, nameCompression, zoneName); } diff --git a/FilterDns/Dns/IxfrResponseBuilder.cs b/FilterDns/Dns/IxfrResponseBuilder.cs index 47684b1..5f7a46f 100644 --- a/FilterDns/Dns/IxfrResponseBuilder.cs +++ b/FilterDns/Dns/IxfrResponseBuilder.cs @@ -365,6 +365,11 @@ private static byte[] SerializeRdata( /// private static byte[] SerializeRdataWithoutCompression(FilteredRecord record) { + if (record.RawRData is { Length: > 0 }) + { + return record.RawRData.ToArray(); + } + if (record.SoaData != null) { return SerializeSoaRdataWithoutCompression(record.SoaData); diff --git a/FilterDns/Dns/RDataSerializer.cs b/FilterDns/Dns/RDataSerializer.cs new file mode 100644 index 0000000..475e339 --- /dev/null +++ b/FilterDns/Dns/RDataSerializer.cs @@ -0,0 +1,183 @@ +using System.Reflection; +using System.Text; +using DnsClient.Protocol; +using FilterDns.Filter; + +namespace FilterDns.Dns; + +public static class RDataSerializer +{ + public static byte[] Serialize(FilteredRecord record) + { + if (record.RawRData is { Length: > 0 }) + { + return record.RawRData.ToArray(); + } + + if (record.SoaData != null) + { + return SerializeSoa(record.SoaData); + } + + if (record.NsName != null) + { + return SerializeDomainName(record.NsName); + } + + if (record.OriginalRecord == null) + { + return Array.Empty(); + } + + return Serialize(record.OriginalRecord); + } + + public static byte[] Serialize(DnsResourceRecord record) + { + var rawRdata = TryGetRawRdata(record); + if (rawRdata is { Length: > 0 }) + { + return rawRdata; + } + + return record switch + { + SoaRecord soa => SerializeSoa(soa), + NsRecord ns => SerializeDomainName(ns.NSDName.Value), + ARecord a => a.Address.GetAddressBytes(), + AaaaRecord aaaa => aaaa.Address.GetAddressBytes(), + MxRecord mx => SerializeMx(mx), + CNameRecord cname => SerializeDomainName(cname.CanonicalName.Value), + TxtRecord txt => SerializeTxt(txt), + SrvRecord srv => SerializeSrv(srv), + PtrRecord ptr => SerializeDomainName(ptr.PtrDomainName.Value), + CaaRecord caa => SerializeCaa(caa), + _ => Array.Empty() + }; + } + + private static byte[] SerializeSoa(SoaRecordData soa) + { + var data = new List(); + DnsMessageParser.WriteDomainName(data, soa.MName); + DnsMessageParser.WriteDomainName(data, soa.RName); + DnsMessageParser.WriteUInt32(data, soa.Serial); + DnsMessageParser.WriteUInt32(data, soa.Refresh); + DnsMessageParser.WriteUInt32(data, soa.Retry); + DnsMessageParser.WriteUInt32(data, soa.Expire); + DnsMessageParser.WriteUInt32(data, soa.Minimum); + return data.ToArray(); + } + + private static byte[] SerializeSoa(SoaRecord soa) + { + var data = new List(); + DnsMessageParser.WriteDomainName(data, soa.MName.Value); + DnsMessageParser.WriteDomainName(data, soa.RName.Value); + DnsMessageParser.WriteUInt32(data, (uint)soa.Serial); + DnsMessageParser.WriteUInt32(data, (uint)soa.Refresh); + DnsMessageParser.WriteUInt32(data, (uint)soa.Retry); + DnsMessageParser.WriteUInt32(data, (uint)soa.Expire); + DnsMessageParser.WriteUInt32(data, (uint)soa.Minimum); + return data.ToArray(); + } + + private static byte[] SerializeMx(MxRecord mx) + { + var data = new List(); + DnsMessageParser.WriteUInt16(data, (ushort)mx.Preference); + DnsMessageParser.WriteDomainName(data, mx.Exchange.Value); + return data.ToArray(); + } + + private static byte[] SerializeTxt(TxtRecord txt) + { + var data = new List(); + foreach (var text in txt.Text) + { + var bytes = Encoding.UTF8.GetBytes(text); + data.Add((byte)bytes.Length); + data.AddRange(bytes); + } + return data.ToArray(); + } + + private static byte[] SerializeSrv(SrvRecord srv) + { + var data = new List(); + DnsMessageParser.WriteUInt16(data, (ushort)srv.Priority); + DnsMessageParser.WriteUInt16(data, (ushort)srv.Weight); + DnsMessageParser.WriteUInt16(data, (ushort)srv.Port); + DnsMessageParser.WriteDomainName(data, srv.Target.Value); + return data.ToArray(); + } + + private static byte[] SerializeCaa(CaaRecord caa) + { + var data = new List(); + data.Add((byte)caa.Flags); + var tagBytes = Encoding.ASCII.GetBytes(caa.Tag); + data.Add((byte)tagBytes.Length); + data.AddRange(tagBytes); + data.AddRange(Encoding.UTF8.GetBytes(caa.Value)); + return data.ToArray(); + } + + private static byte[] SerializeDomainName(string name) + { + var data = new List(); + DnsMessageParser.WriteDomainName(data, name); + return data.ToArray(); + } + + private static byte[]? TryGetRawRdata(DnsResourceRecord record) + { + try + { + var rawProperty = record.GetType().GetProperty("Raw", BindingFlags.Public | BindingFlags.Instance); + if (rawProperty?.PropertyType == typeof(byte[])) + { + var rawValue = rawProperty.GetValue(record) as byte[]; + if (rawValue is { Length: > 0 }) + { + return rawValue; + } + } + + var fields = record.GetType().GetFields(BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.Public); + foreach (var field in fields) + { + if ((field.Name.Contains("Raw") || field.Name.Contains("Rdata") || field.Name.Contains("Data")) + && field.FieldType == typeof(byte[])) + { + var value = field.GetValue(record) as byte[]; + if (value is { Length: > 0 }) + { + return value; + } + } + } + + var baseType = record.GetType().BaseType; + while (baseType != null && baseType != typeof(object)) + { + var baseRawProperty = baseType.GetProperty("Raw", BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance); + if (baseRawProperty?.PropertyType == typeof(byte[])) + { + var rawValue = baseRawProperty.GetValue(record) as byte[]; + if (rawValue is { Length: > 0 }) + { + return rawValue; + } + } + baseType = baseType.BaseType; + } + } + catch + { + return null; + } + + return null; + } +} diff --git a/FilterDns/Filter/RecordFilter.cs b/FilterDns/Filter/RecordFilter.cs index fdf6095..c172ff8 100644 --- a/FilterDns/Filter/RecordFilter.cs +++ b/FilterDns/Filter/RecordFilter.cs @@ -3,6 +3,7 @@ using DnsClient; using DnsClient.Protocol; using FilterDns.Config; +using FilterDns.Net; namespace FilterDns.Filter; @@ -16,6 +17,7 @@ public class FilteredRecord public DnsResourceRecord OriginalRecord { get; set; } = null!; public SoaRecordData? SoaData { get; set; } public string? NsName { get; set; } + public byte[]? RawRData { get; set; } } public class SoaRecordData @@ -246,30 +248,7 @@ private static bool IsInRange(IPAddress address, string range) /// private static bool IsInCidrRange(IPAddress address, IPAddress networkAddress, int prefixLength) { - var addressBytes = address.GetAddressBytes(); - var networkBytes = networkAddress.GetAddressBytes(); - - if (addressBytes.Length != networkBytes.Length) - return false; - - var byteLength = (prefixLength + 7) / 8; // Number of bytes to compare - - for (int i = 0; i < byteLength; i++) - { - if (addressBytes[i] != networkBytes[i]) - return false; - } - - // Check remaining bits if prefix length is not byte-aligned - if (prefixLength % 8 != 0) - { - var bitsInLastByte = prefixLength % 8; - var mask = (byte)(0xFF << (8 - bitsInLastByte)); - if ((addressBytes[byteLength] & mask) != (networkBytes[byteLength] & mask)) - return false; - } - - return true; + return new CidrRange(networkAddress, prefixLength).Contains(address); } /// diff --git a/FilterDns/Net/CidrRange.cs b/FilterDns/Net/CidrRange.cs new file mode 100644 index 0000000..6fe43c3 --- /dev/null +++ b/FilterDns/Net/CidrRange.cs @@ -0,0 +1,71 @@ +using System.Net; +using System.Net.Sockets; + +namespace FilterDns.Net; + +public sealed class CidrRange +{ + private readonly byte[] _networkBytes; + private readonly int _fullBytes; + private readonly int _remainingBits; + + public CidrRange(IPAddress networkAddress, int prefixLength) + { + var normalizedNetworkAddress = NormalizeAddress(networkAddress); + ValidatePrefixLength(normalizedNetworkAddress, prefixLength); + + _networkBytes = normalizedNetworkAddress.GetAddressBytes(); + _fullBytes = prefixLength / 8; + _remainingBits = prefixLength % 8; + } + + public bool Contains(IPAddress address) + { + var normalizedAddress = NormalizeAddress(address); + var addressBytes = normalizedAddress.GetAddressBytes(); + + if (addressBytes.Length != _networkBytes.Length) + { + return false; + } + + for (var i = 0; i < _fullBytes; i++) + { + if (addressBytes[i] != _networkBytes[i]) + { + return false; + } + } + + if (_remainingBits == 0) + { + return true; + } + + var mask = (byte)(0xFF << (8 - _remainingBits)); + return (addressBytes[_fullBytes] & mask) == (_networkBytes[_fullBytes] & mask); + } + + private static IPAddress NormalizeAddress(IPAddress address) + { + return address.IsIPv4MappedToIPv6 ? address.MapToIPv4() : address; + } + + private static void ValidatePrefixLength(IPAddress address, int prefixLength) + { + var maxPrefixLength = address.AddressFamily switch + { + AddressFamily.InterNetwork => 32, + AddressFamily.InterNetworkV6 => 128, + _ => throw new ArgumentException($"Unsupported address family: {address.AddressFamily}", nameof(address)) + }; + + if (prefixLength < 0 || prefixLength > maxPrefixLength) + { + throw new ArgumentOutOfRangeException( + nameof(prefixLength), + prefixLength, + $"Prefix length must be between 0 and {maxPrefixLength} for {address.AddressFamily}"); + } + } +} diff --git a/FilterDns/Whitelist/IpWhitelist.cs b/FilterDns/Whitelist/IpWhitelist.cs index c324827..10c5908 100644 --- a/FilterDns/Whitelist/IpWhitelist.cs +++ b/FilterDns/Whitelist/IpWhitelist.cs @@ -1,5 +1,6 @@ using System.Net; using System.Net.Sockets; +using FilterDns.Net; namespace FilterDns.Whitelist; @@ -44,41 +45,16 @@ public bool Allows(IPAddress ip) private class IPNetwork { - private readonly IPAddress _networkAddress; - private readonly int _prefixLength; - private readonly byte[] _networkBytes; - private readonly int _byteLength; + private readonly CidrRange _range; public IPNetwork(IPAddress address, int prefixLength) { - _networkAddress = address; - _prefixLength = prefixLength; - _networkBytes = address.GetAddressBytes(); - _byteLength = (prefixLength + 7) / 8; // Number of bytes to compare + _range = new CidrRange(address, prefixLength); } public bool Contains(IPAddress address) { - var addressBytes = address.GetAddressBytes(); - if (addressBytes.Length != _networkBytes.Length) - return false; - - for (int i = 0; i < _byteLength; i++) - { - if (addressBytes[i] != _networkBytes[i]) - return false; - } - - // Check remaining bits if prefix length is not byte-aligned - if (_prefixLength % 8 != 0) - { - var bitsInLastByte = _prefixLength % 8; - var mask = (byte)(0xFF << (8 - bitsInLastByte)); - if ((addressBytes[_byteLength] & mask) != (_networkBytes[_byteLength] & mask)) - return false; - } - - return true; + return _range.Contains(address); } } } diff --git a/REVIEW_FINDINGS.md b/REVIEW_FINDINGS.md index 0f24027..ec2a776 100644 --- a/REVIEW_FINDINGS.md +++ b/REVIEW_FINDINGS.md @@ -19,11 +19,19 @@ Fix first: 4. Normalize zone names at configuration load so mixed-case zones do not break external DNS paths. 5. Remove cache-update side effects from health checks and transfer-triggered refreshes. +### Critical Fix Progress + +- [x] **SEV-01:** Fixed in this branch. Added persisted raw RDATA round-trip coverage in `FilterDns.Tests/CriticalFindingTests.cs`. +- [x] **SEV-02:** Fixed in this branch. Added diff coverage for TTL-only changes and loaded same-name A records with distinct RDATA. +- [x] **SEV-03:** Fixed in this branch. Added CIDR coverage for non-byte-aligned IPv4 whitelist ranges and custom IPv6 private ranges. +- [ ] **SEV-04+ follow-ups:** Not started in this pass. + ## Findings ### [SEV-01] Persisted history drops RDATA and can corrupt IXFR after restart - **Severity:** Critical +- **Status:** Fixed in this branch - **Confidence:** Confirmed - **Location:** `FilterDns/Cache/ZoneHistoryStorage.cs:26-34`, `FilterDns/Cache/ZoneHistoryStorage.cs:481-530`, `FilterDns/Dns/IxfrResponseBuilder.cs:378-455` - **What's wrong:** JSON history declares `RawRdataBase64`, but `ConvertToJson` never writes it and `ConvertFromJson` restores `OriginalRecord = null`. `IxfrResponseBuilder.SerializeRdataWithoutCompression` returns empty RDATA when a loaded non-SOA/non-NS record has no `OriginalRecord`. @@ -35,6 +43,7 @@ Fix first: ### [SEV-02] IXFR diff keys collapse duplicate RRsets and omit TTL changes - **Severity:** Critical +- **Status:** Fixed in this branch - **Confidence:** Confirmed - **Location:** `FilterDns/Cache/RecordComparer.cs:43-52`, `FilterDns/Cache/RecordComparer.cs:119-128`, `FilterDns/Cache/RecordComparer.cs:154-172`, `FilterDns/Cache/RecordComparer.cs:227-239`, `FilterDns/Xfer/ZoneDiffCalculator.cs:45-80` - **What's wrong:** `ZoneDiffCalculator` uses one dictionary entry per record key. When `OriginalRecord` is null, A/AAAA and unknown-type RDATA hashes become the hash of an empty string, so multiple records at the same owner/type/class collapse into one key. TTL is not included in equality or keys, so TTL-only updates are invisible. @@ -46,6 +55,7 @@ Fix first: ### [SEV-03] Custom CIDR matching breaks ACLs and private-IP filtering - **Severity:** Critical +- **Status:** Fixed in this branch - **Confidence:** Confirmed - **Location:** `FilterDns/Filter/RecordFilter.cs:195-204`, `FilterDns/Filter/RecordFilter.cs:223-272`, `FilterDns/Whitelist/IpWhitelist.cs:10-27`, `FilterDns/Whitelist/IpWhitelist.cs:52-81` - **What's wrong:** Both `RecordFilter` and `IpWhitelist` compute `(prefixLength + 7) / 8`, compare that many full bytes, then also read the next byte for partial prefixes. Non-byte-aligned prefixes such as `/25`, `/17`, `/7`, and `/10` are evaluated incorrectly; exact network addresses can hit `IndexOutOfRangeException`. Invalid prefix lengths are accepted. From cf2406ea51a88c2eba33873a2138bcc54c477c67 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Thu, 25 Jun 2026 05:46:38 +0000 Subject: [PATCH 3/7] Archive invalid zone history on load Co-authored-by: Gerold K. --- FilterDns.Tests/CriticalFindingTests.cs | 9 +++- FilterDns/Cache/ZoneHistoryStorage.cs | 56 ++++++++++++++++++++++--- REVIEW_FINDINGS.md | 2 +- 3 files changed, 59 insertions(+), 8 deletions(-) diff --git a/FilterDns.Tests/CriticalFindingTests.cs b/FilterDns.Tests/CriticalFindingTests.cs index 7c24b22..8fcd35a 100644 --- a/FilterDns.Tests/CriticalFindingTests.cs +++ b/FilterDns.Tests/CriticalFindingTests.cs @@ -1,4 +1,5 @@ using System.Net; +using System.IO.Compression; using DnsClient; using DnsClient.Protocol; using FilterDns.Cache; @@ -189,8 +190,9 @@ public async Task ZoneHistoryStorage_RejectsLegacyHistoryWithoutRequiredRdata() var dataDirectory = Path.Combine(Path.GetTempPath(), $"filterdns-tests-{Guid.NewGuid():N}"); var historyDirectory = Path.Combine(dataDirectory, "history"); Directory.CreateDirectory(historyDirectory); + var historyFilePath = Path.Combine(historyDirectory, "example_com.json"); await File.WriteAllTextAsync( - Path.Combine(historyDirectory, "example_com.json"), + historyFilePath, """ [ { @@ -212,6 +214,11 @@ await File.WriteAllTextAsync( var loaded = await storage.LoadAsync("example.com"); Assert.Null(loaded); + Assert.False(File.Exists(historyFilePath)); + var archivePath = Assert.Single(Directory.GetFiles(Path.Combine(historyDirectory, "invalid"), "example_com_*.zip")); + using var archive = ZipFile.OpenRead(archivePath); + Assert.Contains(archive.Entries, entry => entry.FullName == "example_com.json"); + Assert.Contains(archive.Entries, entry => entry.FullName == "reason.txt"); } [Fact] diff --git a/FilterDns/Cache/ZoneHistoryStorage.cs b/FilterDns/Cache/ZoneHistoryStorage.cs index 94c3767..bd12fc4 100644 --- a/FilterDns/Cache/ZoneHistoryStorage.cs +++ b/FilterDns/Cache/ZoneHistoryStorage.cs @@ -2,6 +2,7 @@ using System.Text.Json; using System.Text; using System.Security.Cryptography; +using System.IO.Compression; using DnsClient.Protocol; using FilterDns.Export; using FilterDns.Filter; @@ -412,6 +413,7 @@ public async Task SaveAsync(ZoneHistory history) { _logger?.LogWarning("Failed to deserialize zone history for {ZoneName} from {FilePath}", zoneName, filePath); + QuarantineInvalidHistoryFile(filePath, zoneName, "deserialized history was null"); return null; } @@ -428,12 +430,8 @@ public async Task SaveAsync(ZoneHistory history) var calculatedHash = CalculateZoneVersionHash(versionJson.Serial, versionJson.Records, versionJson.Timestamp); if (calculatedHash != versionJson.Hash) { - _logger?.LogWarning( - "Zone history integrity check failed for {ZoneName} version {Serial}: hash mismatch. Expected {ExpectedHash}, got {CalculatedHash}", - zoneName, versionJson.Serial, versionJson.Hash, calculatedHash); - - // Reject corrupted version - don't add to history - continue; + throw new InvalidDataException( + $"Zone history integrity check failed for {zoneName} version {versionJson.Serial}: hash mismatch. Expected {versionJson.Hash}, got {calculatedHash}"); } } @@ -450,10 +448,56 @@ public async Task SaveAsync(ZoneHistory history) { _logger?.LogError(ex, "Failed to load zone history for {ZoneName} from {FilePath}", zoneName, filePath); + QuarantineInvalidHistoryFile(filePath, zoneName, ex.Message); return null; } } + private void QuarantineInvalidHistoryFile(string filePath, string zoneName, string reason) + { + if (!File.Exists(filePath)) + { + return; + } + + try + { + var historyDir = Path.Combine(_dataDirectory, "history"); + var invalidDir = Path.Combine(historyDir, "invalid"); + Directory.CreateDirectory(invalidDir); + ValidatePathWithinDirectory(invalidDir, _dataDirectory); + + var sanitizedZoneName = SanitizeZoneName(zoneName); + var timestamp = DateTime.UtcNow.ToString("yyyyMMddHHmmssfff"); + var archivePath = Path.Combine(invalidDir, $"{sanitizedZoneName}_{timestamp}.zip"); + ValidatePathWithinDirectory(archivePath, _dataDirectory); + + using (var archive = ZipFile.Open(archivePath, ZipArchiveMode.Create)) + { + archive.CreateEntryFromFile(filePath, Path.GetFileName(filePath), CompressionLevel.Optimal); + var reasonEntry = archive.CreateEntry("reason.txt", CompressionLevel.Optimal); + using var writer = new StreamWriter(reasonEntry.Open(), Encoding.UTF8); + writer.WriteLine($"Zone: {zoneName}"); + writer.WriteLine($"OriginalPath: {filePath}"); + writer.WriteLine($"ArchivedAtUtc: {DateTime.UtcNow:O}"); + writer.WriteLine($"Reason: {reason}"); + } + + File.Delete(filePath); + _logger?.LogWarning( + "Archived invalid zone history for {ZoneName} to {ArchivePath} and removed active file {FilePath}", + zoneName, archivePath, filePath); + } + catch (Exception archiveEx) + { + _logger?.LogError( + archiveEx, + "Failed to archive invalid zone history for {ZoneName} at {FilePath}; leaving active file in place", + zoneName, + filePath); + } + } + /// /// Deletes the history file for a zone. /// diff --git a/REVIEW_FINDINGS.md b/REVIEW_FINDINGS.md index ec2a776..f3d6fcc 100644 --- a/REVIEW_FINDINGS.md +++ b/REVIEW_FINDINGS.md @@ -21,7 +21,7 @@ Fix first: ### Critical Fix Progress -- [x] **SEV-01:** Fixed in this branch. Added persisted raw RDATA round-trip coverage in `FilterDns.Tests/CriticalFindingTests.cs`. +- [x] **SEV-01:** Fixed in this branch. Added persisted raw RDATA round-trip coverage in `FilterDns.Tests/CriticalFindingTests.cs`; invalid legacy history files are now archived to compressed backups and removed from the active history path. - [x] **SEV-02:** Fixed in this branch. Added diff coverage for TTL-only changes and loaded same-name A records with distinct RDATA. - [x] **SEV-03:** Fixed in this branch. Added CIDR coverage for non-byte-aligned IPv4 whitelist ranges and custom IPv6 private ranges. - [ ] **SEV-04+ follow-ups:** Not started in this pass. From 84ce726214b464d0022448f5f477b2b68a6ab888 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Thu, 25 Jun 2026 06:08:57 +0000 Subject: [PATCH 4/7] Fix high severity DNS proxy issues Co-authored-by: Gerold K. --- FilterDns.Tests/HighFindingTests.cs | 296 +++++++++++++++++++++++++++ FilterDns/Cache/ZoneHistory.cs | 15 +- FilterDns/Dns/DnsMessageParser.cs | 6 + FilterDns/Proxy/DnsProxyServer.cs | 130 ++++++++---- FilterDns/Xfer/XferHandler.cs | 70 ++++--- FilterDns/Xfer/ZoneDiffCalculator.cs | 51 ++++- REVIEW_FINDINGS.md | 15 +- 7 files changed, 496 insertions(+), 87 deletions(-) create mode 100644 FilterDns.Tests/HighFindingTests.cs diff --git a/FilterDns.Tests/HighFindingTests.cs b/FilterDns.Tests/HighFindingTests.cs new file mode 100644 index 0000000..218bc90 --- /dev/null +++ b/FilterDns.Tests/HighFindingTests.cs @@ -0,0 +1,296 @@ +using System.Net; +using DnsClient; +using DnsClient.Protocol; +using FilterDns.Cache; +using FilterDns.Config; +using FilterDns.Dns; +using FilterDns.Filter; +using FilterDns.Proxy; +using FilterDns.Xfer; +using Microsoft.Extensions.Logging; + +namespace FilterDns.Tests; + +public class HighFindingTests +{ + [Fact] + public void DnsMessageParser_ParsesCompressedAuthoritySoaWithHardening() + { + var message = BuildIxfrRequestWithCompressedAuthoritySoa(clientSerial: 42); + + var parsed = DnsMessageParser.Parse(message); + var serial = DnsMessageParser.ExtractIxfrClientSerial(parsed, message); + + var authority = Assert.Single(parsed.Authority); + Assert.Equal(ResourceRecordType.SOA, authority.RecordType); + Assert.Equal((uint)42, serial); + } + + [Fact] + public void ZoneDiffCalculator_UsesSnapshotDictionaryInsteadOfLiveHistory() + { + var history = new ZoneHistory("example.com"); + history.AddVersion(1, [ARecord("www.example.com.", 300, "203.0.113.1")]); + history.AddVersion(2, [ARecord("www.example.com.", 300, "203.0.113.2")]); + var snapshot = history.GetAllVersions(); + history.Clear(); + + var diffs = ZoneDiffCalculator.CalculateDiffSequence( + snapshot, + fromSerial: 1, + toSerial: 2); + + var diff = Assert.Single(diffs); + Assert.Equal(1u, diff.FromSerial); + Assert.Equal(2u, diff.ToSerial); + Assert.Single(diff.DeletedRecords); + Assert.Single(diff.AddedRecords); + } + + [Fact] + public void ZoneDiffCalculator_PreservesSnapshotSerialWraparoundOrder() + { + var snapshot = new Dictionary + { + [uint.MaxValue] = new() + { + Serial = uint.MaxValue, + Records = [ARecord("www.example.com.", 300, "203.0.113.255")] + }, + [0] = new() + { + Serial = 0, + Records = [ARecord("www.example.com.", 300, "203.0.113.0")] + }, + [1] = new() + { + Serial = 1, + Records = [ARecord("www.example.com.", 300, "203.0.113.1")] + } + }; + + var diffs = ZoneDiffCalculator.CalculateDiffSequence(snapshot, uint.MaxValue, 1); + + Assert.Collection( + diffs, + diff => + { + Assert.Equal(uint.MaxValue, diff.FromSerial); + Assert.Equal(0u, diff.ToSerial); + }, + diff => + { + Assert.Equal(0u, diff.FromSerial); + Assert.Equal(1u, diff.ToSerial); + }); + } + + [Fact] + public async Task TransferRefresh_DoesNotDowngradeNewerCachedSerial() + { + using var loggerFactory = LoggerFactory.Create(_ => { }); + var proxy = CreateProxy(loggerFactory, minimumZoneRecordCount: 0); + var cache = GetPrivateField(proxy, "_cache"); + cache.UpdateZone("example.com", ZoneRecords(serial: 3)); + + await InvokeTransferRefreshAsync(proxy, ZoneRecords(serial: 2)); + + Assert.Equal((uint)3, cache.GetSerial("example.com")); + } + + [Fact] + public async Task TransferRefresh_AllowsWrappedSerialAfterUintMax() + { + using var loggerFactory = LoggerFactory.Create(_ => { }); + var proxy = CreateProxy(loggerFactory, minimumZoneRecordCount: 0); + var cache = GetPrivateField(proxy, "_cache"); + cache.UpdateZone("example.com", ZoneRecords(serial: uint.MaxValue)); + + await InvokeTransferRefreshAsync(proxy, ZoneRecords(serial: 1)); + + Assert.Equal((uint)1, cache.GetSerial("example.com")); + } + + [Fact] + public void XferHandler_TreatsWrappedUpstreamSerialAsNewer() + { + var method = typeof(XferHandler).GetMethod( + "IsSerialNewer", + System.Reflection.BindingFlags.Static | System.Reflection.BindingFlags.NonPublic); + Assert.NotNull(method); + + Assert.True((bool)method.Invoke(null, [1u, uint.MaxValue])!); + Assert.False((bool)method.Invoke(null, [uint.MaxValue, 1u])!); + } + + [Fact] + public async Task TransferRefresh_DoesNotCacheZoneBelowMinimumRecordCount() + { + using var loggerFactory = LoggerFactory.Create(_ => { }); + var proxy = CreateProxy(loggerFactory, minimumZoneRecordCount: 3); + var cache = GetPrivateField(proxy, "_cache"); + + await InvokeTransferRefreshAsync(proxy, [SoaRecord(serial: 2)]); + + Assert.Null(cache.GetZone("example.com")); + } + + [Fact] + public void DnsProxyServer_NormalizesConfiguredZoneNamesForExternalLookups() + { + using var loggerFactory = LoggerFactory.Create(_ => { }); + var proxy = CreateProxy(loggerFactory, zoneName: "Example.COM."); + + var zones = GetPrivateField>( + proxy, + "_zones"); + + var entry = Assert.Single(zones); + Assert.Equal("example.com", entry.Key); + Assert.Equal("example.com", entry.Value.Config.Name); + } + + private static FilteredRecord ARecord(string name, int ttl, string address) + { + return new FilteredRecord + { + DomainName = name, + RecordType = ResourceRecordType.A, + RecordClass = QueryClass.IN, + TimeToLive = ttl, + RawRData = IPAddress.Parse(address).GetAddressBytes() + }; + } + + private static List ZoneRecords(uint serial) + { + return + [ + SoaRecord(serial), + new() + { + DomainName = "example.com.", + RecordType = ResourceRecordType.NS, + RecordClass = QueryClass.IN, + TimeToLive = 300, + NsName = "ns1.example.com." + }, + ARecord("www.example.com.", 300, $"203.0.113.{serial % 250}") + ]; + } + + private static FilteredRecord SoaRecord(uint serial) + { + return new FilteredRecord + { + DomainName = "example.com.", + RecordType = ResourceRecordType.SOA, + RecordClass = QueryClass.IN, + TimeToLive = 300, + SoaData = new SoaRecordData + { + MName = "ns1.example.com.", + RName = "admin.example.com.", + Serial = serial, + Refresh = 3600, + Retry = 600, + Expire = 604800, + Minimum = 300 + } + }; + } + + private static DnsProxyServer CreateProxy( + ILoggerFactory loggerFactory, + int minimumZoneRecordCount = 0, + string zoneName = "example.com") + { + return new DnsProxyServer( + new AppConfiguration + { + Server = new ServerConfig + { + DataDirectory = Path.Combine(Path.GetTempPath(), $"filterdns-high-tests-{Guid.NewGuid():N}") + }, + Zones = + [ + new ZoneConfig + { + Name = zoneName, + Upstream = "127.0.0.1:5300", + Ns1 = "ns1.example.com", + Ns2 = "ns2.example.com", + MinimumZoneRecordCount = minimumZoneRecordCount + } + ] + }, + loggerFactory.CreateLogger(), + loggerFactory); + } + + private static async Task InvokeTransferRefreshAsync(DnsProxyServer proxy, List records) + { + var method = typeof(DnsProxyServer).GetMethod( + "HandleZoneUpdatedDuringTransferAsync", + System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic); + Assert.NotNull(method); + var zones = GetPrivateField>( + proxy, + "_zones"); + var zoneConfig = zones["example.com"].Config; + + var task = Assert.IsAssignableFrom(method.Invoke( + proxy, + [ + "example.com", + zoneConfig, + records, + records.First(r => r.RecordType == ResourceRecordType.SOA).SoaData!.Serial, + null, + 0u, + CancellationToken.None + ])!); + await task; + } + + private static T GetPrivateField(object instance, string fieldName) + { + var field = instance.GetType().GetField(fieldName, System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic); + Assert.NotNull(field); + return Assert.IsType(field.GetValue(instance)); + } + + private static byte[] BuildIxfrRequestWithCompressedAuthoritySoa(uint clientSerial) + { + var data = new List(); + DnsMessageParser.WriteUInt16(data, 0x1234); + DnsMessageParser.WriteUInt16(data, 0x0000); + DnsMessageParser.WriteUInt16(data, 1); // QDCOUNT + DnsMessageParser.WriteUInt16(data, 0); // ANCOUNT + DnsMessageParser.WriteUInt16(data, 1); // NSCOUNT + DnsMessageParser.WriteUInt16(data, 0); // ARCOUNT + + DnsMessageParser.WriteDomainName(data, "example.com."); + DnsMessageParser.WriteUInt16(data, (ushort)DnsQueryType.IXFR); + DnsMessageParser.WriteUInt16(data, (ushort)DnsQueryClass.IN); + + DnsMessageParser.WriteUInt16(data, 0xC00C); // compressed owner points at question name + DnsMessageParser.WriteUInt16(data, (ushort)DnsQueryType.SOA); + DnsMessageParser.WriteUInt16(data, (ushort)DnsQueryClass.IN); + DnsMessageParser.WriteUInt32(data, 60); + + var rdata = new List(); + DnsMessageParser.WriteDomainName(rdata, "ns1.example.com."); + DnsMessageParser.WriteDomainName(rdata, "admin.example.com."); + DnsMessageParser.WriteUInt32(rdata, clientSerial); + DnsMessageParser.WriteUInt32(rdata, 3600); + DnsMessageParser.WriteUInt32(rdata, 600); + DnsMessageParser.WriteUInt32(rdata, 604800); + DnsMessageParser.WriteUInt32(rdata, 300); + + DnsMessageParser.WriteUInt16(data, (ushort)rdata.Count); + data.AddRange(rdata); + + return data.ToArray(); + } +} diff --git a/FilterDns/Cache/ZoneHistory.cs b/FilterDns/Cache/ZoneHistory.cs index fd0122a..639d83a 100644 --- a/FilterDns/Cache/ZoneHistory.cs +++ b/FilterDns/Cache/ZoneHistory.cs @@ -71,16 +71,13 @@ public List GetVersionsBetween(uint fromSerial, uint toSerial) { // Wraparound case: fromSerial is after wraparound // Get versions from fromSerial to uint.MaxValue, then from 0 to toSerial - // Use explicit range checks to avoid including keys in the middle range - foreach (var kvp in _versions.OrderBy(v => v.Key)) + foreach (var kvp in _versions.Where(v => v.Key >= fromSerial).OrderBy(v => v.Key)) { - // Include keys from fromSerial to max uint32, OR from 0 to toSerial - // The second condition ensures we don't include keys between (toSerial+1) and (fromSerial-1) - if ((kvp.Key >= fromSerial && kvp.Key <= uint.MaxValue) || - (kvp.Key >= 0 && kvp.Key <= toSerial)) - { - result.Add(kvp.Value); - } + result.Add(kvp.Value); + } + foreach (var kvp in _versions.Where(v => v.Key <= toSerial).OrderBy(v => v.Key)) + { + result.Add(kvp.Value); } } else diff --git a/FilterDns/Dns/DnsMessageParser.cs b/FilterDns/Dns/DnsMessageParser.cs index ca50afd..91c3100 100644 --- a/FilterDns/Dns/DnsMessageParser.cs +++ b/FilterDns/Dns/DnsMessageParser.cs @@ -348,6 +348,12 @@ private static string ReadDomainName(byte[] data, ref int offset, SecurityConfig throw new FormatException("Domain name parsing failed: buffer overflow while reading compression pointer"); } + if (!jumped) + { + jumpOffset = offset + 1; + jumped = true; + } + var newOffset = ((length & 0x3F) << 8) | data[offset]; // Check for compression pointer loops diff --git a/FilterDns/Proxy/DnsProxyServer.cs b/FilterDns/Proxy/DnsProxyServer.cs index a4429f2..b281a4e 100644 --- a/FilterDns/Proxy/DnsProxyServer.cs +++ b/FilterDns/Proxy/DnsProxyServer.cs @@ -82,6 +82,9 @@ public DnsProxyServer( // Build zones and whitelists foreach (var zoneConfig in config.Zones) { + var originalZoneName = zoneConfig.Name; + var zoneName = NormalizeZoneName(originalZoneName); + zoneConfig.Name = zoneName; var whitelistEntries = new List(zoneConfig.XferWhitelist); // Filter out invalid slaves and automatically add valid slave IPs to whitelist @@ -100,19 +103,19 @@ public DnsProxyServer( else { _logger.LogWarning("Skipping invalid slave configuration for zone {Zone}: Ip={Ip}, Port={Port}", - zoneConfig.Name, slave.Ip, slave.Port); + originalZoneName, slave.Ip, slave.Port); } } var whitelist = new IpWhitelist(whitelistEntries); - _zones[zoneConfig.Name] = (zoneConfig, whitelist); + _zones[zoneName] = (zoneConfig, whitelist); // Create notify sender for this zone using only valid slaves var notifyLogger = _loggerFactory.CreateLogger(); - _notifySenders[zoneConfig.Name] = new NotifySender(validSlaves, notifyLogger); + _notifySenders[zoneName] = new NotifySender(validSlaves, notifyLogger); // Create semaphore for serializing zone updates (1 concurrent update per zone) - _zoneUpdateSemaphores[zoneConfig.Name] = new SemaphoreSlim(1, 1); + _zoneUpdateSemaphores[zoneName] = new SemaphoreSlim(1, 1); } // Create verification service (shared across all zones) @@ -120,6 +123,11 @@ public DnsProxyServer( _verificationService = new SlaveVerificationService(verificationLogger, emailAlertService, _selfRestartService); } + private static string NormalizeZoneName(string zoneName) + { + return zoneName.TrimEnd('.').ToLowerInvariant(); + } + protected override async Task ExecuteAsync(CancellationToken stoppingToken) { _logger.LogInformation("Starting DNS Proxy Server"); @@ -323,10 +331,9 @@ private async Task HandleNotifyFromUpstreamAsync( } /// - /// Callback for when XferHandler updates the cache during zone transfer handling. - /// This ensures zone history is updated and other slaves are notified when the cache - /// is updated outside of the normal polling/NOTIFY flow. - /// CRITICAL: Receives old zone info to maintain history continuity for IXFR. + /// Callback for when XferHandler refreshes a zone during transfer handling. + /// Cache and history updates are serialized with poll/NOTIFY updates, but slave + /// NOTIFY is intentionally skipped to avoid triggering competing transfers mid-request. /// private async Task HandleZoneUpdatedDuringTransferAsync( string zoneName, @@ -337,51 +344,49 @@ private async Task HandleZoneUpdatedDuringTransferAsync( uint oldSerial, CancellationToken cancellationToken) { - _logger.LogDebug("Zone {Zone} updated during transfer handling (serial: {OldSerial} -> {NewSerial}), updating history and notifying slaves", - zoneName, oldSerial, newSerial); - - // CRITICAL: Save the OLD version to history FIRST (before adding new version) - // This ensures IXFR can calculate diffs from the old serial to the new serial - if (oldRecords != null && oldSerial > 0 && oldSerial != newSerial) + var semaphore = _zoneUpdateSemaphores.GetOrAdd(zoneName, _ => new SemaphoreSlim(1, 1)); + await semaphore.WaitAsync(cancellationToken); + try { - await SaveCurrentVersionToHistoryAsync(zoneName, zoneConfig, oldRecords, oldSerial); - _logger.LogDebug("Zone {Zone}: Saved old version (serial {OldSerial}) to history for IXFR support", - zoneName, oldSerial); - } + if (IsBelowMinimumRecordCount(zoneName, zoneConfig, newRecords, "TransferUpdate")) + { + return; + } - // Update zone history for IXFR support - add the new version - await UpdateZoneHistoryAsync(zoneName, zoneConfig, newRecords, newSerial); + var currentCachedZone = _cache.GetZone(zoneName); + if (currentCachedZone != null && IsSerialNewer(currentCachedZone.Serial, newSerial)) + { + _logger.LogInformation( + "TransferUpdate: Zone {Zone} refresh serial {RefreshSerial} is older than cached serial {CachedSerial}; skipping stale cache/history update", + zoneName, newSerial, currentCachedZone.Serial); + return; + } - // Record this update for rapid update detection - RecordUpdateTimestamp(zoneName); + var effectiveOldRecords = oldRecords ?? currentCachedZone?.Records; + var effectiveOldSerial = oldSerial > 0 ? oldSerial : currentCachedZone?.Serial ?? 0; - // Send NOTIFY to all slaves - if (_notifySenders.TryGetValue(zoneName, out var notifySender)) - { - // Small delay to ensure zone is fully ready before sending NOTIFY - await Task.Delay(Constants.NotifyDelayMs, cancellationToken); + _logger.LogDebug( + "Zone {Zone} refreshed during transfer handling (serial: {OldSerial} -> {NewSerial}), updating cache/history without slave NOTIFY", + zoneName, effectiveOldSerial, newSerial); - var logPrefix = "TransferUpdate"; - - // Check if we're in a rapid update period - if (IsRapidUpdatePeriod(zoneName)) + if (effectiveOldRecords != null && effectiveOldSerial > 0 && effectiveOldSerial != newSerial) { - var recentCount = GetRecentUpdateCount(zoneName); - _logger.LogInformation( - "{Prefix}: Zone {Zone} is in rapid update period ({RecentCount} updates in last {Window}s), using rate-limited NOTIFY", - logPrefix, zoneName, recentCount, Constants.RapidUpdateWindowSeconds); + await SaveCurrentVersionToHistoryAsync(zoneName, zoneConfig, effectiveOldRecords, effectiveOldSerial); + _logger.LogDebug("Zone {Zone}: Saved old version (serial {OldSerial}) to history for IXFR support", + zoneName, effectiveOldSerial); } - // Use rate-limited NOTIFY to prevent flooding slaves - await SendNotifyToSlavesWithRateLimitAsync(zoneName, zoneConfig, notifySender, logPrefix, cancellationToken); + _cache.UpdateZone(zoneName, newRecords); + await UpdateZoneHistoryAsync(zoneName, zoneConfig, newRecords, newSerial); + RecordUpdateTimestamp(zoneName); _logger.LogInformation( - "{Prefix}: Zone {Zone} serial {OldSerial} -> {NewSerial} - history updated and slaves notified", - logPrefix, zoneName, oldSerial, newSerial); + "TransferUpdate: Zone {Zone} serial {OldSerial} -> {NewSerial} - cache/history updated without slave NOTIFY", + zoneName, effectiveOldSerial, newSerial); } - else + finally { - _logger.LogDebug("Zone {Zone} has no configured slaves, skipping NOTIFY", zoneName); + semaphore.Release(); } } @@ -635,6 +640,11 @@ private async Task UpdateZoneFromUpstreamAsync( _selfRestartService.ReportZoneFailure(zoneName, "Zone filtered to empty records"); return; // Don't update cache with empty data } + + if (IsBelowMinimumRecordCount(zoneName, zoneConfig, filteredRecords, logPrefix)) + { + return; + } // Calculate statistics for filtered records var filteredStats = CalculateFilteredZoneStatistics(filteredRecords, upstreamSerial); @@ -1184,6 +1194,44 @@ private async Task UpdateZoneHistoryAsync( } } + private bool IsBelowMinimumRecordCount( + string zoneName, + ZoneConfig zoneConfig, + List records, + string logPrefix) + { + var minimumRecordCount = zoneConfig.MinimumZoneRecordCount ?? 3; + if (minimumRecordCount <= 0 || records.Count >= minimumRecordCount) + { + return false; + } + + _logger.LogError( + "{Prefix}: Zone {Zone} has only {RecordCount} filtered records (minimum: {MinimumCount}); keeping existing cache and skipping history/NOTIFY", + logPrefix, zoneName, records.Count, minimumRecordCount); + _selfRestartService.ReportZoneFailure( + zoneName, + $"Filtered record count {records.Count} below minimum {minimumRecordCount}"); + return true; + } + + private static bool IsSerialNewer(uint candidateSerial, uint referenceSerial) + { + if (candidateSerial == referenceSerial) + { + return false; + } + + const ulong halfSerialSpace = 2147483648UL; + + if (candidateSerial > referenceSerial) + { + return candidateSerial - (ulong)referenceSerial < halfSerialSpace; + } + + return referenceSerial - (ulong)candidateSerial > halfSerialSpace; + } + /// /// Gets zone history for a zone. Used by XferHandler. /// diff --git a/FilterDns/Xfer/XferHandler.cs b/FilterDns/Xfer/XferHandler.cs index 54a3617..41024be 100644 --- a/FilterDns/Xfer/XferHandler.cs +++ b/FilterDns/Xfer/XferHandler.cs @@ -23,7 +23,8 @@ public class XferHandler private readonly IpWhitelist? _healthCheckWhitelist; private readonly Func? _onNotifyReceived; private readonly Func? _getHistory; - // Callback signature: zoneName, zoneConfig, newRecords, newSerial, oldRecords, oldSerial, cancellationToken + // Callback signature: zoneName, zoneConfig, newRecords, newSerial, oldRecords, oldSerial, cancellationToken. + // The proxy implementation updates cache/history under its per-zone lock without notifying slaves. private readonly Func, uint, List?, uint, CancellationToken, Task>? _onZoneUpdatedDuringTransfer; private readonly string _ixfrResponseMode; // "Incremental" or "FullZone" private readonly SecurityConfig? _securityConfig; @@ -820,7 +821,6 @@ private async Task HandleHealthCheckQueryAsync( { var upstreamRecords = await upstreamClient.FetchZoneAsync(zoneConfig.Name, cancellationToken); zoneRecords = RecordFilter.ApplyFilters(upstreamRecords, zoneConfig, zoneConfig.Name); - await UpdateCacheAndNotifyAsync(zoneConfig.Name, zoneConfig, zoneRecords, cancellationToken); } catch (Exception ex) { @@ -1001,7 +1001,7 @@ private async Task HandleZoneTransferRequestAsync( var upstreamSerial = await upstreamClient.GetSoaSerialAsync(zoneConfig.Name, cancellationToken); // If upstream has newer serial, fetch fresh data - if (upstreamSerial > cachedZone.Serial) + if (IsSerialNewer(upstreamSerial, cachedZone.Serial)) { _logger.LogInformation( "Zone transfer: Cached data for {Zone} is stale (cached: {CachedSerial}, upstream: {UpstreamSerial}), fetching latest from upstream {Upstream}", @@ -1033,7 +1033,7 @@ private async Task HandleZoneTransferRequestAsync( // Update cache with latest data and notify other slaves // This ensures future requests get the latest data and slaves are kept in sync - await UpdateCacheAndNotifyAsync(zoneConfig.Name, zoneConfig, zoneRecords, cancellationToken); + await UpdateCacheAndHistoryAsync(zoneConfig.Name, zoneConfig, zoneRecords, cancellationToken); } else if (upstreamSerial == cachedZone.Serial) { @@ -1102,7 +1102,7 @@ private async Task HandleZoneTransferRequestAsync( zoneConfig.Name, upstreamRecords.Count, zoneRecords.Count); // Update cache with latest data and notify other slaves - await UpdateCacheAndNotifyAsync(zoneConfig.Name, zoneConfig, zoneRecords, cancellationToken); + await UpdateCacheAndHistoryAsync(zoneConfig.Name, zoneConfig, zoneRecords, cancellationToken); } catch (Exception ex) { @@ -1392,12 +1392,10 @@ private async Task HandleZoneTransferRequestAsync( } /// - /// Updates the cache and notifies about zone updates during transfer handling. - /// This ensures zone history is updated and other slaves are notified when the cache - /// is updated during AXFR/IXFR handling. - /// CRITICAL: Captures the old zone BEFORE updating to ensure history continuity for IXFR. + /// Updates cache/history through the proxy during transfer handling without notifying slaves. + /// The proxy callback owns locking so transfer-triggered refreshes serialize with poll/NOTIFY updates. /// - private async Task UpdateCacheAndNotifyAsync( + private async Task UpdateCacheAndHistoryAsync( string zoneName, ZoneConfig zoneConfig, List records, @@ -1408,39 +1406,32 @@ private async Task UpdateCacheAndNotifyAsync( .FirstOrDefault(r => r.RecordType == ResourceRecordType.SOA) ?.SoaData?.Serial ?? 0; - // CRITICAL: Get the old cached zone BEFORE updating - // This is needed to save the old version to history for IXFR support - var oldCachedZone = _cache.GetZone(zoneName); - var oldSerial = oldCachedZone?.Serial ?? 0; - var oldRecords = oldCachedZone?.Records; - - // Update cache - _cache.UpdateZone(zoneName, records); - - // Notify about the update (updates history and sends NOTIFY to other slaves) if (_onZoneUpdatedDuringTransfer != null && newSerial > 0) { try { - // Pass old zone info so the callback can save it to history first await _onZoneUpdatedDuringTransfer( zoneName, zoneConfig, records, newSerial, - oldRecords, - oldSerial, + null, + 0, cancellationToken); _logger.LogDebug( - "Zone {Zone} updated during transfer handling (serial: {OldSerial} -> {NewSerial}), history updated and slaves notified", - zoneName, oldSerial, newSerial); + "Zone {Zone} refreshed during transfer handling (serial: {NewSerial}), cache/history update delegated to proxy", + zoneName, newSerial); } catch (Exception ex) { - _logger.LogWarning(ex, "Failed to notify about zone update during transfer handling for zone {Zone}", zoneName); - // Don't fail the transfer - the cache is already updated + _logger.LogWarning(ex, "Failed to update cache/history during transfer handling for zone {Zone}", zoneName); + // Don't fail the transfer; the fetched records can still be served to the current requester. } } + else + { + _cache.UpdateZone(zoneName, records); + } } /// @@ -1607,7 +1598,7 @@ private async Task HandleIxfrRequestAsync( // This ensures we include the final diff from last history version to current cache // Using snapshot ensures consistency even if cache is updated during IXFR handling var diffs = ZoneDiffCalculator.CalculateDiffSequence( - history, + historySnapshot, clientSerial.Value, snapshotSerial.Value, snapshotRecords); // Pass snapshot records for final diff calculation @@ -1971,7 +1962,7 @@ private async Task HandleSoaQueryOnTcpAsync( var upstreamSerial = await upstreamClient.GetSoaSerialAsync(zoneConfig.Name, cancellationToken); // If upstream has newer serial, fetch fresh data - if (upstreamSerial > cachedZone.Serial) + if (IsSerialNewer(upstreamSerial, cachedZone.Serial)) { _logger.LogDebug( "SOA query: Cached data for {Zone} is stale (cached: {CachedSerial}, upstream: {UpstreamSerial}), fetching latest", @@ -1979,7 +1970,7 @@ private async Task HandleSoaQueryOnTcpAsync( var upstreamRecords = await upstreamClient.FetchZoneAsync(zoneConfig.Name, cancellationToken); zoneRecords = RecordFilter.ApplyFilters(upstreamRecords, zoneConfig, zoneConfig.Name); - await UpdateCacheAndNotifyAsync(zoneConfig.Name, zoneConfig, zoneRecords, cancellationToken); + await UpdateCacheAndHistoryAsync(zoneConfig.Name, zoneConfig, zoneRecords, cancellationToken); } else { @@ -2010,7 +2001,7 @@ private async Task HandleSoaQueryOnTcpAsync( { var upstreamRecords = await upstreamClient.FetchZoneAsync(zoneConfig.Name, cancellationToken); zoneRecords = RecordFilter.ApplyFilters(upstreamRecords, zoneConfig, zoneConfig.Name); - await UpdateCacheAndNotifyAsync(zoneConfig.Name, zoneConfig, zoneRecords, cancellationToken); + await UpdateCacheAndHistoryAsync(zoneConfig.Name, zoneConfig, zoneRecords, cancellationToken); } catch (Exception ex) { @@ -2092,5 +2083,22 @@ public void Stop() _tcpConnectionSemaphore?.Dispose(); _udpRequestSemaphore?.Dispose(); } + + private static bool IsSerialNewer(uint candidateSerial, uint referenceSerial) + { + if (candidateSerial == referenceSerial) + { + return false; + } + + const ulong halfSerialSpace = 2147483648UL; + + if (candidateSerial > referenceSerial) + { + return candidateSerial - (ulong)referenceSerial < halfSerialSpace; + } + + return referenceSerial - (ulong)candidateSerial > halfSerialSpace; + } } diff --git a/FilterDns/Xfer/ZoneDiffCalculator.cs b/FilterDns/Xfer/ZoneDiffCalculator.cs index 903e799..61d1725 100644 --- a/FilterDns/Xfer/ZoneDiffCalculator.cs +++ b/FilterDns/Xfer/ZoneDiffCalculator.cs @@ -101,13 +101,32 @@ public static List CalculateDiffSequence( if (history == null) throw new ArgumentNullException(nameof(history)); - var diffs = new List(); - // Get all versions between fromSerial and toSerial var versions = history.GetVersionsBetween(fromSerial, toSerial); - // Sort by serial to ensure correct order - versions = versions.OrderBy(v => v.Serial).ToList(); + return CalculateDiffSequence(versions, fromSerial, toSerial, currentZoneRecords); + } + + public static List CalculateDiffSequence( + Dictionary historySnapshot, + uint fromSerial, + uint toSerial, + List? currentZoneRecords = null) + { + if (historySnapshot == null) + throw new ArgumentNullException(nameof(historySnapshot)); + + var versions = GetVersionsBetween(historySnapshot, fromSerial, toSerial); + return CalculateDiffSequence(versions, fromSerial, toSerial, currentZoneRecords); + } + + private static List CalculateDiffSequence( + List versions, + uint fromSerial, + uint toSerial, + List? currentZoneRecords) + { + var diffs = new List(); // If the fromSerial version doesn't exist, we can't calculate incremental diffs // This should be handled by the caller (fallback to AXFR) @@ -191,6 +210,30 @@ public static List CalculateDiffSequence( return diffs; } + private static List GetVersionsBetween( + Dictionary historySnapshot, + uint fromSerial, + uint toSerial) + { + if (fromSerial > toSerial) + { + return historySnapshot + .Where(kvp => kvp.Key >= fromSerial) + .OrderBy(kvp => kvp.Key) + .Concat(historySnapshot + .Where(kvp => kvp.Key <= toSerial) + .OrderBy(kvp => kvp.Key)) + .Select(kvp => kvp.Value) + .ToList(); + } + + return historySnapshot + .Where(kvp => kvp.Key >= fromSerial && kvp.Key <= toSerial) + .OrderBy(kvp => kvp.Key) + .Select(kvp => kvp.Value) + .ToList(); + } + /// /// Calculates a cumulative diff from fromSerial to toSerial. /// This combines all intermediate changes into a single diff. diff --git a/REVIEW_FINDINGS.md b/REVIEW_FINDINGS.md index f3d6fcc..3a3dc69 100644 --- a/REVIEW_FINDINGS.md +++ b/REVIEW_FINDINGS.md @@ -19,12 +19,17 @@ Fix first: 4. Normalize zone names at configuration load so mixed-case zones do not break external DNS paths. 5. Remove cache-update side effects from health checks and transfer-triggered refreshes. -### Critical Fix Progress +### Fix Progress - [x] **SEV-01:** Fixed in this branch. Added persisted raw RDATA round-trip coverage in `FilterDns.Tests/CriticalFindingTests.cs`; invalid legacy history files are now archived to compressed backups and removed from the active history path. - [x] **SEV-02:** Fixed in this branch. Added diff coverage for TTL-only changes and loaded same-name A records with distinct RDATA. - [x] **SEV-03:** Fixed in this branch. Added CIDR coverage for non-byte-aligned IPv4 whitelist ranges and custom IPv6 private ranges. -- [ ] **SEV-04+ follow-ups:** Not started in this pass. +- [x] **SEV-04:** Fixed in this branch. Added compressed IXFR authority SOA parser coverage. +- [x] **SEV-05:** Fixed in this branch. Added mixed-case configured-zone normalization coverage. +- [x] **SEV-06:** Fixed in this branch. Health-check cache misses now fetch/filter for the response without cache/history/NOTIFY propagation. +- [x] **SEV-07:** Fixed in this branch. Transfer-triggered refreshes update cache/history through the proxy's per-zone lock and skip slave NOTIFY. +- [x] **SEV-08:** Fixed in this branch. Added snapshot-based IXFR diff coverage. +- [x] **SEV-09:** Fixed in this branch. Minimum record count is checked before poll/NOTIFY/transfer-refresh cache/history updates. ## Findings @@ -67,6 +72,7 @@ Fix first: ### [SEV-04] Hardened DNS compression parsing leaves the offset at the pointed-to name - **Severity:** High +- **Status:** Fixed in this branch - **Confidence:** Confirmed - **Location:** `FilterDns/Dns/DnsMessageParser.cs:334-368`, `FilterDns/Dns/DnsMessageParser.cs:414-417` - **What's wrong:** In the hardening branch of `ReadDomainName`, compression pointers are followed, but `jumped` and `jumpOffset` are never set. After resolving a compressed name, the caller resumes reading at the target name terminator instead of after the original two-byte pointer. @@ -78,6 +84,7 @@ Fix first: ### [SEV-05] Mixed-case configured zone names break NOTIFY, transfers, and health checks - **Severity:** High +- **Status:** Fixed in this branch - **Confidence:** Confirmed - **Location:** `FilterDns/Proxy/DnsProxyServer.cs:107-115`, `FilterDns/Xfer/XferHandler.cs:665-676`, `FilterDns/Xfer/XferHandler.cs:771-783`, `FilterDns/Xfer/XferHandler.cs:942-965` - **What's wrong:** `_zones`, `_notifySenders`, and `_zoneUpdateSemaphores` are keyed with `zoneConfig.Name` exactly as configured. External request paths lower-case query names before lookup. A configured `Example.com` key is not found for `example.com`. @@ -89,6 +96,7 @@ Fix first: ### [SEV-06] Health-check cache misses update history and notify slaves - **Severity:** High +- **Status:** Fixed in this branch - **Confidence:** Confirmed - **Location:** `FilterDns/Xfer/XferHandler.cs:803-824`, `FilterDns/Xfer/XferHandler.cs:1400-1443`, `FilterDns/Proxy/DnsProxyServer.cs:340-380` - **What's wrong:** A health-check query from an allowed IP fetches upstream on cache miss, then calls `UpdateCacheAndNotifyAsync`. That callback updates history and sends NOTIFY to all slaves, even though the trigger was only a read-style health probe. @@ -100,6 +108,7 @@ Fix first: ### [SEV-07] Transfer-triggered refreshes race poll/NOTIFY updates and notify slaves mid-transfer - **Severity:** High +- **Status:** Fixed in this branch - **Confidence:** Confirmed - **Location:** `FilterDns/Proxy/DnsProxyServer.cs:516-531`, `FilterDns/Xfer/XferHandler.cs:1001-1036`, `FilterDns/Xfer/XferHandler.cs:1071-1105`, `FilterDns/Xfer/XferHandler.cs:1400-1443` - **What's wrong:** Poll and upstream-NOTIFY updates are serialized by `_zoneUpdateSemaphores`, but inbound AXFR/IXFR refreshes call `UpdateCacheAndNotifyAsync` without that lock. They also notify all slaves while the requesting slave may still be in the middle of its transfer. @@ -111,6 +120,7 @@ Fix first: ### [SEV-08] IXFR calculates diffs from live history after validating a snapshot - **Severity:** High +- **Status:** Fixed in this branch - **Confidence:** Confirmed - **Location:** `FilterDns/Xfer/XferHandler.cs:1552-1613`, `FilterDns/Xfer/ZoneDiffCalculator.cs:95-110` - **What's wrong:** `HandleIxfrRequestAsync` snapshots history for validation, but then passes the live `ZoneHistory` to `ZoneDiffCalculator.CalculateDiffSequence`. Concurrent adds/prunes/saves can change the versions used for the actual diff after validation succeeds. @@ -122,6 +132,7 @@ Fix first: ### [SEV-09] Minimum record count is enforced after cache, history, and NOTIFY - **Severity:** High +- **Status:** Fixed in this branch - **Confidence:** Confirmed - **Location:** `FilterDns/Proxy/DnsProxyServer.cs:627-699`, `FilterDns/Xfer/XferHandler.cs:1178-1200` - **What's wrong:** Poll/NOTIFY refreshes cache filtered records and notify slaves without applying `MinimumZoneRecordCount`. The minimum is only checked later when a slave requests a transfer. From 996c7569bd601847bfa748b299776c48d87b0657 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Thu, 25 Jun 2026 06:27:06 +0000 Subject: [PATCH 5/7] Fix medium severity DNS proxy issues Co-authored-by: Gerold K. --- FilterDns.Tests/MediumFindingTests.cs | 399 +++++++++++++++++++ FilterDns/Cache/ZoneHistoryStorage.cs | 22 +- FilterDns/Config/ConfigurationValidator.cs | 223 +++++++++++ FilterDns/Dns/DnsRecordBuilder.cs | 3 +- FilterDns/Dns/IxfrResponseBuilder.cs | 1 - FilterDns/Program.cs | 3 +- FilterDns/Proxy/DnsProxyServer.cs | 5 +- FilterDns/Recovery/SelfRestartService.cs | 93 ++++- FilterDns/Verify/SlaveVerificationService.cs | 45 ++- FilterDns/Xfer/XferHandler.cs | 242 ++++++----- REVIEW_FINDINGS.md | 16 + 11 files changed, 927 insertions(+), 125 deletions(-) create mode 100644 FilterDns.Tests/MediumFindingTests.cs create mode 100644 FilterDns/Config/ConfigurationValidator.cs diff --git a/FilterDns.Tests/MediumFindingTests.cs b/FilterDns.Tests/MediumFindingTests.cs new file mode 100644 index 0000000..908f0f3 --- /dev/null +++ b/FilterDns.Tests/MediumFindingTests.cs @@ -0,0 +1,399 @@ +using System.Net; +using DnsClient; +using DnsClient.Protocol; +using FilterDns.Cache; +using FilterDns.Config; +using FilterDns.Dns; +using FilterDns.Filter; +using FilterDns.Recovery; +using FilterDns.Xfer; +using Microsoft.Extensions.Logging; + +namespace FilterDns.Tests; + +public class MediumFindingTests +{ + [Fact] + public void DnsRecordBuilder_SerializesCaaWithoutValueLengthByte() + { + var caa = RecordFromOriginal(new CaaRecord( + new ResourceRecordInfo("example.com.", ResourceRecordType.CAA, QueryClass.IN, 300, 0), + 0, + "issue", + "letsencrypt.org")); + var response = DnsRecordBuilder.BuildQueryResponse( + Query("example.com.", (DnsQueryType)257), + [caa]); + + var rdata = ExtractFirstAnswerRdata(response); + + Assert.Equal( + new byte[] { 0, 5, (byte)'i', (byte)'s', (byte)'s', (byte)'u', (byte)'e' } + .Concat("letsencrypt.org"u8.ToArray()), + rdata); + } + + [Fact] + public void IxfrResponseBuilder_SerializesCaaWithoutValueLengthByte() + { + var currentSoa = SoaRecord(serial: 2); + var oldSoa = SoaRecord(serial: 1); + var caa = RecordFromOriginal(new CaaRecord( + new ResourceRecordInfo("example.com.", ResourceRecordType.CAA, QueryClass.IN, 300, 0), + 0, + "issue", + "letsencrypt.org")); + var result = IxfrResponseBuilder.BuildIxfrResponse( + [ + new ZoneDiff + { + FromSerial = 1, + ToSerial = 2, + AddedRecords = [caa] + } + ], + currentSoa, + 0x1234, + new FilterDns.Dns.DnsQuestion + { + Name = "example.com.", + QueryType = DnsQueryType.IXFR, + QueryClass = DnsQueryClass.IN + }, + serial => serial == 1 ? oldSoa : currentSoa); + + Assert.True(result.Success, result.FailureReason); + var caaMessage = result.Messages.Single(message => + ExtractAnswerType(message) == (ushort)ResourceRecordType.CAA); + var rdata = ExtractFirstAnswerRdata(caaMessage); + + Assert.Equal( + new byte[] { 0, 5, (byte)'i', (byte)'s', (byte)'s', (byte)'u', (byte)'e' } + .Concat("letsencrypt.org"u8.ToArray()), + rdata); + } + + [Fact] + public async Task ZoneHistoryStorage_SaveDoesNotPruneLiveHistory() + { + var dataDirectory = Path.Combine(Path.GetTempPath(), $"filterdns-medium-tests-{Guid.NewGuid():N}"); + var storage = new ZoneHistoryStorage( + dataDirectory, + exportBindZoneFiles: false, + securityConfig: new SecurityConfig { MaxZoneVersionsPerZone = 1 }); + var history = new ZoneHistory("example.com"); + history.AddVersion(1, [SoaRecord(1)]); + history.AddVersion(2, [SoaRecord(2)]); + + await storage.SaveAsync(history); + + Assert.True(history.HasVersion(1)); + Assert.True(history.HasVersion(2)); + } + + [Fact] + public void ConfigurationValidator_RejectsInvalidLogLevel() + { + var config = ValidConfig(); + config.Server.Logging = new LoggingConfig { DefaultLevel = "Info" }; + + Assert.Throws(() => ConfigurationValidator.Validate(config)); + } + + [Fact] + public void ConfigurationValidator_RejectsDuplicateZoneNamesAfterNormalization() + { + var config = ValidConfig(); + config.Zones.Add(new ZoneConfig + { + Name = "Example.COM.", + Upstream = "127.0.0.1:53", + Ns1 = "ns1.example.com", + Ns2 = "ns2.example.com" + }); + + Assert.Throws(() => ConfigurationValidator.Validate(config)); + } + + [Fact] + public void ConfigurationValidator_RejectsInvalidPrivateIpRange() + { + var config = ValidConfig(); + config.Zones[0].PrivateIPRanges = ["192.168.1.0/99"]; + + Assert.Throws(() => ConfigurationValidator.Validate(config)); + } + + [Fact] + public void XferHandler_CalculatesExactAxfrTransferSizeBeforeStreaming() + { + var method = typeof(XferHandler).GetMethod( + "CalculateAxfrTransferSizeBytes", + System.Reflection.BindingFlags.Static | System.Reflection.BindingFlags.NonPublic); + Assert.NotNull(method); + var records = new List + { + SoaRecord(1), + new() + { + DomainName = "large.example.com.", + RecordType = ResourceRecordType.TXT, + RecordClass = QueryClass.IN, + TimeToLive = 300, + RawRData = Enumerable.Repeat((byte)'a', 220).Prepend((byte)220).ToArray() + } + }; + + var exactSize = Assert.IsType(method.Invoke(null, [records, (ushort)0x1234, "example.com"])); + + Assert.True(exactSize > records.Count * 100L); + } + + [Fact] + public void XferHandler_ChoosesNotifyResponseAfterZoneAndSourceValidation() + { + var method = typeof(XferHandler).GetMethod( + "GetNotifyResponseCode", + System.Reflection.BindingFlags.Static | System.Reflection.BindingFlags.NonPublic); + Assert.NotNull(method); + var zones = new Dictionary + { + ["example.com"] = ( + new ZoneConfig + { + Name = "example.com", + Upstream = "192.0.2.1:53", + Ns1 = "ns1.example.com", + Ns2 = "ns2.example.com" + }, + new FilterDns.Whitelist.IpWhitelist([])) + }; + + Assert.Equal( + FilterDns.Dns.DnsResponseCode.NoError, + method.Invoke(null, [zones, "example.com", IPAddress.Parse("192.0.2.1")])); + Assert.Equal( + FilterDns.Dns.DnsResponseCode.Refused, + method.Invoke(null, [zones, "example.com", IPAddress.Parse("192.0.2.2")])); + Assert.Equal( + FilterDns.Dns.DnsResponseCode.Refused, + method.Invoke(null, [zones, "missing.example", IPAddress.Parse("192.0.2.1")])); + } + + [Fact] + public void XferHandler_BuildsServFailResponseForUdpSaturation() + { + var method = typeof(XferHandler).GetMethod( + "BuildUdpSaturationResponse", + System.Reflection.BindingFlags.Static | System.Reflection.BindingFlags.NonPublic); + Assert.NotNull(method); + + var response = Assert.IsType(method.Invoke(null, [new byte[] { 0x12, 0x34 }])); + + Assert.Equal(0x12, response[0]); + Assert.Equal(0x34, response[1]); + Assert.Equal((byte)FilterDns.Dns.DnsResponseCode.ServFail, (byte)(response[3] & 0x0F)); + } + + [Fact] + public void SelfRestartService_SuppressesRestartWhenWindowLimitReached() + { + var now = new DateTime(2026, 6, 25, 0, 0, 0, DateTimeKind.Utc); + var exits = 0; + using var loggerFactory = LoggerFactory.Create(_ => { }); + var service = new SelfRestartService( + new SelfRestartConfig + { + MinimumUptimeBeforeRestartSeconds = 0, + MaxRestartsInWindow = 1, + RestartWindowSeconds = 3600 + }, + loggerFactory.CreateLogger(), + exitAction: _ => exits++, + utcNow: () => now, + existingRestartAttempts: [now.AddMinutes(-5)]); + + service.TriggerRestart("test"); + + Assert.False(service.IsRestartTriggered); + Assert.Equal(0, exits); + } + + [Fact] + public void SelfRestartService_LoadsRestartWindowFromPersistedHistory() + { + var now = new DateTime(2026, 6, 25, 0, 0, 0, DateTimeKind.Utc); + var historyPath = Path.Combine(Path.GetTempPath(), $"filterdns-restarts-{Guid.NewGuid():N}.txt"); + File.WriteAllLines(historyPath, [now.AddMinutes(-5).ToString("O")]); + var exits = 0; + using var loggerFactory = LoggerFactory.Create(_ => { }); + var service = new SelfRestartService( + new SelfRestartConfig + { + MinimumUptimeBeforeRestartSeconds = 0, + MaxRestartsInWindow = 1, + RestartWindowSeconds = 3600 + }, + loggerFactory.CreateLogger(), + exitAction: _ => exits++, + utcNow: () => now, + restartHistoryFilePath: historyPath); + + service.TriggerRestart("test"); + + Assert.False(service.IsRestartTriggered); + Assert.Equal(0, exits); + } + + [Fact] + public void DnsProxyServer_ConfiguresSelfRestartHistoryUnderDataDirectory() + { + var dataDirectory = Path.Combine(Path.GetTempPath(), $"filterdns-restart-data-{Guid.NewGuid():N}"); + using var loggerFactory = LoggerFactory.Create(_ => { }); + var config = ValidConfig(); + config.Server.DataDirectory = dataDirectory; + var proxy = new FilterDns.Proxy.DnsProxyServer( + config, + loggerFactory.CreateLogger(), + loggerFactory); + + var restartService = GetPrivateField(proxy, "_selfRestartService"); + var restartHistoryPath = GetPrivateField(restartService, "_restartHistoryFilePath"); + + Assert.Equal(Path.Combine(dataDirectory, "self-restart-history.txt"), restartHistoryPath); + } + + private static DnsMessage Query(string name, DnsQueryType type) + { + return new DnsMessage + { + Id = 0x1234, + Questions = + [ + new FilterDns.Dns.DnsQuestion + { + Name = name, + QueryType = type, + QueryClass = DnsQueryClass.IN + } + ] + }; + } + + private static AppConfiguration ValidConfig() + { + return new AppConfiguration + { + Server = new ServerConfig + { + ListenAddress = "127.0.0.1", + ListenPort = 5353, + LogLevel = "Information", + HealthCheckAcl = ["127.0.0.1"], + Logging = new LoggingConfig() + }, + Zones = + [ + new ZoneConfig + { + Name = "example.com", + Upstream = "127.0.0.1:53", + Ns1 = "ns1.example.com", + Ns2 = "ns2.example.com", + PrivateIPRanges = ["10.0.0.0/8"], + Slaves = [new SlaveConfig { Ip = "127.0.0.1", Port = 53 }], + XferWhitelist = ["127.0.0.1"] + } + ] + }; + } + + private static FilteredRecord RecordFromOriginal(DnsResourceRecord record) + { + return new FilteredRecord + { + DomainName = record.DomainName.Value, + RecordType = record.RecordType, + RecordClass = record.RecordClass, + TimeToLive = record.TimeToLive, + OriginalRecord = record + }; + } + + private static FilteredRecord SoaRecord(uint serial) + { + return new FilteredRecord + { + DomainName = "example.com.", + RecordType = ResourceRecordType.SOA, + RecordClass = QueryClass.IN, + TimeToLive = 300, + SoaData = new SoaRecordData + { + MName = "ns1.example.com.", + RName = "admin.example.com.", + Serial = serial, + Refresh = 3600, + Retry = 600, + Expire = 604800, + Minimum = 300 + } + }; + } + + private static ushort ExtractAnswerType(byte[] message) + { + var offset = 12; + SkipDomainName(message, ref offset); + offset += 4; + SkipDomainName(message, ref offset); + return ReadUInt16(message, ref offset); + } + + private static byte[] ExtractFirstAnswerRdata(byte[] message) + { + var offset = 12; + SkipDomainName(message, ref offset); + offset += 4; + SkipDomainName(message, ref offset); + offset += 2; + offset += 2; + offset += 4; + var rdLength = ReadUInt16(message, ref offset); + return message.Skip(offset).Take(rdLength).ToArray(); + } + + private static void SkipDomainName(byte[] message, ref int offset) + { + while (offset < message.Length) + { + var length = message[offset++]; + if (length == 0) + { + return; + } + + if ((length & 0xC0) == 0xC0) + { + offset++; + return; + } + + offset += length; + } + } + + private static ushort ReadUInt16(byte[] message, ref int offset) + { + var value = (ushort)((message[offset] << 8) | message[offset + 1]); + offset += 2; + return value; + } + + private static T GetPrivateField(object instance, string fieldName) + { + var field = instance.GetType().GetField(fieldName, System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic); + Assert.NotNull(field); + return Assert.IsType(field.GetValue(instance)); + } +} diff --git a/FilterDns/Cache/ZoneHistoryStorage.cs b/FilterDns/Cache/ZoneHistoryStorage.cs index bd12fc4..1de1e13 100644 --- a/FilterDns/Cache/ZoneHistoryStorage.cs +++ b/FilterDns/Cache/ZoneHistoryStorage.cs @@ -281,17 +281,18 @@ public async Task SaveAsync(ZoneHistory history) try { - // Check version count limit and prune if needed var allVersions = history.GetAllVersions().Values.ToList(); if (hardeningEnabled && allVersions.Count > maxVersions) { _logger?.LogWarning( - "Zone history for {ZoneName} has {VersionCount} versions, exceeds maximum ({MaxVersions}). Pruning oldest versions.", + "Zone history for {ZoneName} has {VersionCount} versions, exceeds maximum ({MaxVersions}). Saving only the newest versions.", history.ZoneName, allVersions.Count, maxVersions); - - // Prune oldest versions to stay within limit - history.PruneOldVersions(maxVersions); - allVersions = history.GetAllVersions().Values.ToList(); + + allVersions = allVersions + .OrderByDescending(v => v.Timestamp) + .ThenByDescending(v => v.Serial) + .Take(maxVersions) + .ToList(); } // Convert to JSON format @@ -342,12 +343,15 @@ public async Task SaveAsync(ZoneHistory history) // Write to temp file await File.WriteAllBytesAsync(tempFilePath, jsonBytes); - // Atomic rename + // Atomic replace without deleting the last known-good history first. if (File.Exists(filePath)) { - File.Delete(filePath); + File.Replace(tempFilePath, filePath, null); + } + else + { + File.Move(tempFilePath, filePath); } - File.Move(tempFilePath, filePath); // Also save each version as a BIND format zone file for human reference (if enabled) if (_exportBindZoneFiles) diff --git a/FilterDns/Config/ConfigurationValidator.cs b/FilterDns/Config/ConfigurationValidator.cs new file mode 100644 index 0000000..36f7ba9 --- /dev/null +++ b/FilterDns/Config/ConfigurationValidator.cs @@ -0,0 +1,223 @@ +using System.Net; +using System.Text.RegularExpressions; +using FilterDns.Net; +using FilterDns.Whitelist; +using Microsoft.Extensions.Logging; + +namespace FilterDns.Config; + +public static class ConfigurationValidator +{ + private static readonly string[] IxfrModes = ["Incremental", "FullZone"]; + + public static void Validate(AppConfiguration config) + { + if (config.Zones == null || config.Zones.Count == 0) + { + throw new InvalidOperationException("No zones configured"); + } + + ValidateServer(config.Server); + + var seenZones = new HashSet(StringComparer.OrdinalIgnoreCase); + var hardeningEnabled = config.Server.Security?.SecurityHardeningEnabled ?? true; + foreach (var zone in config.Zones) + { + ValidateZone(zone, config.Server.Security, hardeningEnabled); + var canonicalName = zone.Name.TrimEnd('.').ToLowerInvariant(); + if (!seenZones.Add(canonicalName)) + { + throw new InvalidOperationException($"Duplicate zone configured: {zone.Name}"); + } + } + } + + private static void ValidateServer(ServerConfig server) + { + if (!IPAddress.TryParse(server.ListenAddress, out _)) + { + throw new InvalidOperationException($"Server.ListenAddress is invalid: {server.ListenAddress}"); + } + + ValidatePort(server.ListenPort, "Server.ListenPort"); + ValidateLogLevel(server.LogLevel, "Server.LogLevel"); + + var logging = server.Logging ?? new LoggingConfig + { + DefaultLevel = server.LogLevel, + OperationsLevel = server.LogLevel, + DebugLevel = server.LogLevel + }; + ValidateLogLevel(logging.DefaultLevel, "Server.Logging.DefaultLevel"); + ValidateLogLevel(logging.OperationsLevel, "Server.Logging.OperationsLevel"); + ValidateLogLevel(logging.DebugLevel, "Server.Logging.DebugLevel"); + foreach (var category in logging.Categories) + { + ValidateLogLevel(category.Value, $"Server.Logging.Categories[{category.Key}]"); + } + + if (!IxfrModes.Contains(server.IxfrResponseMode, StringComparer.OrdinalIgnoreCase)) + { + throw new InvalidOperationException($"Server.IxfrResponseMode must be one of: {string.Join(", ", IxfrModes)}"); + } + + _ = new IpWhitelist(server.HealthCheckAcl ?? []); + + if (server.Email?.Enabled == true) + { + ValidateEmail(server.Email); + } + } + + private static void ValidateZone(ZoneConfig zone, SecurityConfig? securityConfig, bool hardeningEnabled) + { + if (string.IsNullOrEmpty(zone.Ns1) || string.IsNullOrEmpty(zone.Ns2)) + { + throw new InvalidOperationException($"Zone {zone.Name} must have NS1 and NS2 configured"); + } + + if (hardeningEnabled) + { + ValidateZoneName(zone.Name, securityConfig); + } + + ValidateEndpoint(zone.Upstream, $"Zone {zone.Name}.Upstream"); + _ = new IpWhitelist(zone.XferWhitelist ?? []); + foreach (var slave in zone.Slaves) + { + if (!slave.IsValid()) + { + throw new InvalidOperationException($"Zone {zone.Name} has invalid slave: {slave.Ip}:{slave.Port}"); + } + } + + foreach (var range in zone.PrivateIPRanges ?? []) + { + ValidateIpOrCidr(range, $"Zone {zone.Name}.PrivateIPRanges"); + } + } + + private static void ValidateEndpoint(string endpoint, string key) + { + var parts = endpoint.Split(':'); + if (parts.Length == 0 || !IPAddress.TryParse(parts[0], out _)) + { + throw new InvalidOperationException($"{key} must start with a valid IP address"); + } + + if (parts.Length > 1) + { + if (!int.TryParse(parts[1], out var port)) + { + throw new InvalidOperationException($"{key} has invalid port: {parts[1]}"); + } + ValidatePort(port, key); + } + } + + private static void ValidateIpOrCidr(string value, string key) + { + if (IPAddress.TryParse(value, out _)) + { + return; + } + + var parts = value.Split('/'); + if (parts.Length != 2 || !IPAddress.TryParse(parts[0], out var networkIp) || !int.TryParse(parts[1], out var prefixLength)) + { + throw new InvalidOperationException($"{key} contains invalid IP/CIDR: {value}"); + } + + try + { + _ = new CidrRange(networkIp, prefixLength); + } + catch (ArgumentException ex) + { + throw new InvalidOperationException($"{key} contains invalid IP/CIDR: {value}", ex); + } + } + + private static void ValidatePort(int port, string key) + { + if (port <= 0 || port > 65535) + { + throw new InvalidOperationException($"{key} must be between 1 and 65535"); + } + } + + private static void ValidateLogLevel(string level, string key) + { + if (!Enum.TryParse(level, ignoreCase: true, out _)) + { + throw new InvalidOperationException($"{key} has invalid log level '{level}'"); + } + } + + private static void ValidateEmail(EmailConfig email) + { + if (string.IsNullOrWhiteSpace(email.SmtpServer)) + { + throw new InvalidOperationException("Server.Email.SmtpServer is required when email is enabled"); + } + ValidatePort(email.SmtpPort, "Server.Email.SmtpPort"); + if (string.IsNullOrWhiteSpace(email.FromAddress)) + { + throw new InvalidOperationException("Server.Email.FromAddress is required when email is enabled"); + } + if (string.IsNullOrWhiteSpace(email.ToAddress)) + { + throw new InvalidOperationException("Server.Email.ToAddress is required when email is enabled"); + } + } + + private static void ValidateZoneName(string zoneName, SecurityConfig? securityConfig) + { + if (string.IsNullOrWhiteSpace(zoneName)) + { + throw new ArgumentException("Zone name cannot be null or empty", nameof(zoneName)); + } + + var maxZoneNameLength = securityConfig?.MaxZoneNameLength ?? 253; + var normalized = zoneName.TrimEnd('.').ToLowerInvariant(); + if (normalized.Length > maxZoneNameLength) + { + throw new ArgumentException($"Zone name length ({normalized.Length}) exceeds maximum ({maxZoneNameLength})", nameof(zoneName)); + } + if (string.IsNullOrEmpty(normalized)) + { + throw new ArgumentException("Zone name cannot be empty or only dots", nameof(zoneName)); + } + if (normalized.Contains("..")) + { + throw new ArgumentException("Zone name contains consecutive dots", nameof(zoneName)); + } + + var labels = normalized.Split('.'); + foreach (var label in labels) + { + if (string.IsNullOrEmpty(label)) + { + throw new ArgumentException("Zone name contains empty label", nameof(zoneName)); + } + if (label.Length > 63) + { + throw new ArgumentException($"Label '{label}' length ({label.Length}) exceeds maximum (63)", nameof(zoneName)); + } + if (label.StartsWith('-') || label.EndsWith('-')) + { + throw new ArgumentException($"Label '{label}' cannot start or end with hyphen", nameof(zoneName)); + } + if (!Regex.IsMatch(label, @"^[a-z0-9_-]+$", RegexOptions.IgnoreCase)) + { + throw new ArgumentException($"Label '{label}' contains invalid characters (only alphanumeric, hyphen, underscore allowed)", nameof(zoneName)); + } + } + + var byteLength = System.Text.Encoding.UTF8.GetByteCount(normalized); + if (byteLength > 253) + { + throw new ArgumentException($"Zone name byte length ({byteLength}) exceeds maximum (253)", nameof(zoneName)); + } + } +} diff --git a/FilterDns/Dns/DnsRecordBuilder.cs b/FilterDns/Dns/DnsRecordBuilder.cs index e5a1a05..8d4abac 100644 --- a/FilterDns/Dns/DnsRecordBuilder.cs +++ b/FilterDns/Dns/DnsRecordBuilder.cs @@ -398,11 +398,10 @@ private static byte[] SerializeCaa(CaaRecord caa) data.Add((byte)tagBytes.Length); data.AddRange(tagBytes); - // Value (length-prefixed binary data) + // Value is the remaining RDATA bytes; only the tag is length-prefixed. // CAA value is stored as a string in DnsClient, but RFC allows binary // We'll encode it as UTF-8 bytes var valueBytes = Encoding.UTF8.GetBytes(caa.Value); - data.Add((byte)valueBytes.Length); data.AddRange(valueBytes); return data.ToArray(); diff --git a/FilterDns/Dns/IxfrResponseBuilder.cs b/FilterDns/Dns/IxfrResponseBuilder.cs index 5f7a46f..24303fa 100644 --- a/FilterDns/Dns/IxfrResponseBuilder.cs +++ b/FilterDns/Dns/IxfrResponseBuilder.cs @@ -446,7 +446,6 @@ private static byte[] SerializeRdataWithoutCompression(FilteredRecord record) data.Add((byte)tagBytes.Length); data.AddRange(tagBytes); var valueBytes = System.Text.Encoding.UTF8.GetBytes(caa.Value); - data.Add((byte)valueBytes.Length); data.AddRange(valueBytes); return data.ToArray(); diff --git a/FilterDns/Program.cs b/FilterDns/Program.cs index cd6ae13..8bb9abc 100644 --- a/FilterDns/Program.cs +++ b/FilterDns/Program.cs @@ -45,7 +45,7 @@ static async Task Main(string[] args) } // Validate configuration - ValidateConfiguration(appConfig); + ConfigurationValidator.Validate(appConfig); // Configure logging var loggingConfig = appConfig.Server.Logging ?? new LoggingConfig(); @@ -170,6 +170,7 @@ private static async Task HandleExportCommandAsync(string[] args) var appConfig = new AppConfiguration(); configuration.Bind(appConfig); + ConfigurationValidator.Validate(appConfig); // Find zone configuration var zoneConfig = appConfig.Zones?.FirstOrDefault(z => diff --git a/FilterDns/Proxy/DnsProxyServer.cs b/FilterDns/Proxy/DnsProxyServer.cs index b281a4e..5714c72 100644 --- a/FilterDns/Proxy/DnsProxyServer.cs +++ b/FilterDns/Proxy/DnsProxyServer.cs @@ -67,13 +67,14 @@ public DnsProxyServer( _notifySenders = new Dictionary(); _zoneHistories = new ConcurrentDictionary(); _zoneUpdateSemaphores = new ConcurrentDictionary(); + var dataDirectory = config.Server.DataDirectory ?? "./data"; // Initialize self-restart service for recovery from unrecoverable situations var selfRestartLogger = _loggerFactory.CreateLogger(); - _selfRestartService = new SelfRestartService(config.Server.SelfRestart, selfRestartLogger); + var restartHistoryPath = Path.Combine(dataDirectory, "self-restart-history.txt"); + _selfRestartService = new SelfRestartService(config.Server.SelfRestart, selfRestartLogger, restartHistoryFilePath: restartHistoryPath); // Initialize history storage if data directory is configured - var dataDirectory = config.Server.DataDirectory ?? "./data"; var historyLogger = _loggerFactory.CreateLogger(); var exportBindZoneFiles = config.Server.ExportBindZoneFiles; var securityConfig = config.Server.Security; diff --git a/FilterDns/Recovery/SelfRestartService.cs b/FilterDns/Recovery/SelfRestartService.cs index 65766a9..46527dd 100644 --- a/FilterDns/Recovery/SelfRestartService.cs +++ b/FilterDns/Recovery/SelfRestartService.cs @@ -13,6 +13,10 @@ public class SelfRestartService { private readonly SelfRestartConfig _config; private readonly ILogger _logger; + private readonly Action _exitAction; + private readonly Func _utcNow; + private readonly Queue _restartAttempts; + private readonly string? _restartHistoryFilePath; private readonly DateTime _startTime; private readonly ConcurrentDictionary _zoneFailureCounts = new(); private readonly ConcurrentDictionary _verificationFailureCounts = new(); @@ -20,11 +24,21 @@ public class SelfRestartService private bool _restartTriggered = false; private readonly object _restartLock = new(); - public SelfRestartService(SelfRestartConfig? config, ILogger logger) + public SelfRestartService( + SelfRestartConfig? config, + ILogger logger, + Action? exitAction = null, + Func? utcNow = null, + IEnumerable? existingRestartAttempts = null, + string? restartHistoryFilePath = null) { _config = config ?? new SelfRestartConfig(); _logger = logger; - _startTime = DateTime.UtcNow; + _exitAction = exitAction ?? Environment.Exit; + _utcNow = utcNow ?? (() => DateTime.UtcNow); + _restartHistoryFilePath = restartHistoryFilePath ?? Path.Combine(AppContext.BaseDirectory, "self-restart-history.txt"); + _restartAttempts = new Queue(existingRestartAttempts ?? LoadRestartAttempts(_restartHistoryFilePath)); + _startTime = _utcNow(); if (_config.Enabled) { @@ -162,7 +176,8 @@ public void TriggerRestart(string reason) } // Check minimum uptime - var uptime = (DateTime.UtcNow - _startTime).TotalSeconds; + var now = _utcNow(); + var uptime = (now - _startTime).TotalSeconds; if (uptime < _config.MinimumUptimeBeforeRestartSeconds) { _logger.LogWarning( @@ -172,6 +187,16 @@ public void TriggerRestart(string reason) return; } + if (RestartWindowLimitReached(now)) + { + _logger.LogError( + "Self-restart requested but restart window limit reached | MaxRestarts={MaxRestarts} | WindowSeconds={WindowSeconds} | Reason={Reason}", + _config.MaxRestartsInWindow, _config.RestartWindowSeconds, reason); + return; + } + + _restartAttempts.Enqueue(now); + SaveRestartAttempts(); _restartTriggered = true; _logger.LogError( @@ -192,18 +217,74 @@ public void TriggerRestart(string reason) _config.RestartExitCode); // Exit with specific code - service manager should restart us - Environment.Exit(_config.RestartExitCode); + _exitAction(_config.RestartExitCode); } catch (Exception ex) { _logger.LogError(ex, "Error during self-restart sequence"); // Force exit anyway - Environment.Exit(_config.RestartExitCode); + _exitAction(_config.RestartExitCode); } }); } } + private bool RestartWindowLimitReached(DateTime now) + { + if (_config.MaxRestartsInWindow <= 0) + { + return false; + } + + var cutoff = now.AddSeconds(-_config.RestartWindowSeconds); + while (_restartAttempts.Count > 0 && _restartAttempts.Peek() < cutoff) + { + _restartAttempts.Dequeue(); + } + + return _restartAttempts.Count >= _config.MaxRestartsInWindow; + } + + private static IEnumerable LoadRestartAttempts(string? path) + { + if (string.IsNullOrWhiteSpace(path) || !File.Exists(path)) + { + return []; + } + + try + { + return File.ReadAllLines(path) + .Select(line => DateTime.TryParse(line, null, System.Globalization.DateTimeStyles.RoundtripKind, out var timestamp) + ? timestamp + : (DateTime?)null) + .Where(timestamp => timestamp.HasValue) + .Select(timestamp => timestamp!.Value) + .ToList(); + } + catch + { + return []; + } + } + + private void SaveRestartAttempts() + { + if (string.IsNullOrWhiteSpace(_restartHistoryFilePath)) + { + return; + } + + try + { + File.WriteAllLines(_restartHistoryFilePath, _restartAttempts.Select(timestamp => timestamp.ToString("O"))); + } + catch (Exception ex) + { + _logger.LogWarning(ex, "Failed to persist self-restart history to {Path}", _restartHistoryFilePath); + } + } + /// /// Gets current failure statistics. /// @@ -212,7 +293,7 @@ public void TriggerRestart(string reason) return ( _consecutiveGlobalFailures, _zoneFailureCounts.Count, - DateTime.UtcNow - _startTime + _utcNow() - _startTime ); } diff --git a/FilterDns/Verify/SlaveVerificationService.cs b/FilterDns/Verify/SlaveVerificationService.cs index 87b314b..2fd140e 100644 --- a/FilterDns/Verify/SlaveVerificationService.cs +++ b/FilterDns/Verify/SlaveVerificationService.cs @@ -143,6 +143,42 @@ public void ScheduleVerification( _logger.LogWarning(ex, "Failed to re-trigger NOTIFY for zone {Zone} after mismatch", zoneName); } } + + _ = Task.Run(async () => + { + try + { + while (_pendingVerifications.TryGetValue(zoneName, out var pendingCts) && ReferenceEquals(pendingCts, cts)) + { + await Task.Delay(10, cancellationToken); + } + + if (_pendingVerifications.ContainsKey(zoneName)) + { + _logger.LogDebug( + "Skipping retry verification for zone {Zone} because a newer verification is already pending", + zoneName); + return; + } + + ScheduleVerification( + zoneName, + sentSerial, + sentRecordCount, + slaves, + delaySeconds, + recordCountTolerance, + notifySender, + cancellationToken, + clearHistoryCallback, + clearHistoryOnMismatch, + maxRetries); + } + catch (OperationCanceledException) + { + _logger.LogDebug("Retry verification scheduling cancelled for zone {Zone}", zoneName); + } + }, CancellationToken.None); } else { @@ -197,11 +233,11 @@ await _emailAlertService.SendCriticalAlertAsync( finally { // Clean up - only dispose after all tasks complete - if (_pendingVerifications.TryRemove(zoneName, out var removedCts)) + if (_pendingVerifications.TryRemove(new KeyValuePair(zoneName, cts))) { try { - removedCts.Dispose(); + cts.Dispose(); } catch (ObjectDisposedException) { @@ -332,6 +368,11 @@ private async Task VerifySlaveWithResultAsync( string.Join(", ", mismatchTypes)); } + _selfRestartService?.ReportVerificationFailure( + zoneName, + $"{slaveIp}:{slave.Port}", + string.Join(",", mismatchTypes)); + // Send email alert for verification issue if (_emailAlertService != null) { diff --git a/FilterDns/Xfer/XferHandler.cs b/FilterDns/Xfer/XferHandler.cs index 41024be..ea93bc3 100644 --- a/FilterDns/Xfer/XferHandler.cs +++ b/FilterDns/Xfer/XferHandler.cs @@ -219,6 +219,9 @@ public async Task StartAsync(CancellationToken cancellationToken) { if (!await _udpRequestSemaphore.WaitAsync(0, cancellationToken)) { + var overloadResponse = BuildUdpSaturationResponse(result.Buffer); + await _udpClient.SendAsync(overloadResponse, result.RemoteEndPoint, cancellationToken); + // Audit log if (_securityConfig?.EnableAuditLogging == true && _securityConfig?.AuditLogFailedTransfers == true) { @@ -659,82 +662,60 @@ private async Task HandleUdpRequestAsync(byte[] data, IPEndPoint remoteEndPoint, // Handle NOTIFY messages if (request.OpCode == DnsOpCode.Notify) { - foreach (var question in request.Questions) + var soaQuestion = request.Questions.FirstOrDefault(q => q.QueryType == DnsQueryType.SOA); + if (soaQuestion != null) { - if (question.QueryType == DnsQueryType.SOA) + var zoneName = soaQuestion.Name.TrimEnd('.'); + var zoneNameLookup = zoneName.ToLowerInvariant(); + + _logger.LogInformation("Received NOTIFY for zone {Zone} from {RemoteEndPoint}", + zoneName, remoteEndPoint); + + var responseCode = GetNotifyResponseCode(_zones, zoneNameLookup, remoteEndPoint.Address); + var response = DnsMessageParser.BuildResponse(request, responseCode); + await _udpClient.SendAsync(response, remoteEndPoint, cancellationToken); + + if (responseCode != DnsResponseCode.NoError) { - var zoneName = question.Name.TrimEnd('.'); - var zoneNameLookup = zoneName.ToLowerInvariant(); - - _logger.LogInformation("Received NOTIFY for zone {Zone} from {RemoteEndPoint}", - zoneName, remoteEndPoint); - - // Send positive response immediately - var response = DnsMessageParser.BuildResponse(request, DnsResponseCode.NoError); - await _udpClient.SendAsync(response, remoteEndPoint, cancellationToken); - - // Find zone configuration - if (_zones.TryGetValue(zoneNameLookup, out var zoneEntry)) + if (_securityConfig?.EnableAuditLogging == true && _securityConfig?.AuditLogUnauthorizedNotify == true) { - var (zoneConfig, _) = zoneEntry; - - // Check if NOTIFY is from upstream master - var upstreamParts = zoneConfig.Upstream.Split(':'); - var upstreamIp = IPAddress.Parse(upstreamParts[0]); - - if (remoteEndPoint.Address.Equals(upstreamIp)) - { - _logger.LogInformation( - "NOTIFY from upstream master {Upstream} for zone {Zone}, triggering zone transfer and slave notification", - zoneConfig.Upstream, zoneName); - - // Trigger zone update and slave notification - if (_onNotifyReceived != null) - { - _ = Task.Run(async () => - { - try - { - await _onNotifyReceived(zoneName, zoneConfig, cancellationToken); - } - catch (Exception ex) - { - _logger.LogError(ex, - "Error processing NOTIFY-triggered update for zone {Zone} from upstream {Upstream}", - zoneName, zoneConfig.Upstream); - } - }, cancellationToken); - } - else - { - _logger.LogWarning( - "NOTIFY received from upstream but no handler configured for zone {Zone}", - zoneName); - } - } - else - { - // Audit log unauthorized NOTIFY - if (_securityConfig?.EnableAuditLogging == true && _securityConfig?.AuditLogUnauthorizedNotify == true) - { - _logger.LogInformation( - "[AUDIT] Unauthorized NOTIFY: Zone {Zone} from {RemoteEndPoint} (not from upstream {Upstream})", - zoneName, remoteEndPoint, zoneConfig.Upstream); - } - else - { - _logger.LogDebug( - "NOTIFY received for zone {Zone} from {RemoteEndPoint} (not from upstream {Upstream}), ignoring", - zoneName, remoteEndPoint, zoneConfig.Upstream); - } - } + _logger.LogInformation( + "[AUDIT] Refused NOTIFY: Zone {Zone} from {RemoteEndPoint}", + zoneName, remoteEndPoint); } else { - _logger.LogWarning( - "NOTIFY received for unknown zone {Zone} from {RemoteEndPoint}", - zoneName, remoteEndPoint); + _logger.LogDebug("Refused NOTIFY for zone {Zone} from {RemoteEndPoint}", zoneName, remoteEndPoint); } + return; + } + + var (zoneConfig, _) = _zones[zoneNameLookup]; + _logger.LogInformation( + "NOTIFY from upstream master {Upstream} for zone {Zone}, triggering zone transfer and slave notification", + zoneConfig.Upstream, zoneName); + + if (_onNotifyReceived != null) + { + _ = Task.Run(async () => + { + try + { + await _onNotifyReceived(zoneNameLookup, zoneConfig, cancellationToken); + } + catch (Exception ex) + { + _logger.LogError(ex, + "Error processing NOTIFY-triggered update for zone {Zone} from upstream {Upstream}", + zoneName, zoneConfig.Upstream); + } + }, cancellationToken); + } + else + { + _logger.LogWarning( + "NOTIFY received from upstream but no handler configured for zone {Zone}", + zoneName); } } } @@ -1207,14 +1188,13 @@ private async Task HandleZoneTransferRequestAsync( // Check zone transfer size limit if hardening enabled if (hardeningEnabled && _securityConfig != null) { - // Estimate transfer size (rough estimate: ~100 bytes per record average) - var estimatedSize = recordCount * 100L; + var estimatedSize = CalculateAxfrTransferSizeBytes(zoneRecords, request.Id, zoneConfig.Name); var maxSize = _securityConfig.MaxZoneTransferSizeBytes; if (estimatedSize > maxSize) { _logger.LogWarning( - "Zone transfer rejected: Zone {Zone} estimated size ({EstimatedSize} bytes) exceeds maximum ({MaxSize} bytes)", + "Zone transfer rejected: Zone {Zone} exact size ({EstimatedSize} bytes) exceeds maximum ({MaxSize} bytes)", zoneName, estimatedSize, maxSize); // Audit log @@ -1264,21 +1244,6 @@ private async Task HandleZoneTransferRequestAsync( var bytes = await SendRecordAsync(stream, soaRecord, request.Id, zoneConfig.Name, effectiveToken); bytesTransferred += bytes; - // Check transfer size limit during transfer - if (hardeningEnabled && _securityConfig != null && bytesTransferred > _securityConfig.MaxZoneTransferSizeBytes) - { - _logger.LogWarning( - "Zone transfer aborted: Zone {Zone} transfer size ({BytesTransferred} bytes) exceeds maximum ({MaxSize} bytes)", - zoneName, bytesTransferred, _securityConfig.MaxZoneTransferSizeBytes); - - if (_securityConfig.EnableAuditLogging == true && _securityConfig.AuditLogFailedTransfers == true) - { - _logger.LogInformation( - "[AUDIT] Zone transfer aborted: Zone {Zone} from {RemoteEndPoint} - Transfer size ({BytesTransferred} bytes) exceeds limit ({MaxSize} bytes)", - zoneName, remoteEndPoint, bytesTransferred, _securityConfig.MaxZoneTransferSizeBytes); - } - return; - } } // Send all other records @@ -1291,21 +1256,6 @@ private async Task HandleZoneTransferRequestAsync( bytesTransferred += bytes; recordsSent++; - // Check transfer size limit during transfer - if (hardeningEnabled && _securityConfig != null && bytesTransferred > _securityConfig.MaxZoneTransferSizeBytes) - { - _logger.LogWarning( - "Zone transfer aborted: Zone {Zone} transfer size ({BytesTransferred} bytes) exceeds maximum ({MaxSize} bytes)", - zoneName, bytesTransferred, _securityConfig.MaxZoneTransferSizeBytes); - - if (_securityConfig.EnableAuditLogging == true && _securityConfig.AuditLogFailedTransfers == true) - { - _logger.LogInformation( - "[AUDIT] Zone transfer aborted: Zone {Zone} from {RemoteEndPoint} - Transfer size ({BytesTransferred} bytes) exceeds limit ({MaxSize} bytes)", - zoneName, remoteEndPoint, bytesTransferred, _securityConfig.MaxZoneTransferSizeBytes); - } - return; - } } } @@ -1934,6 +1884,44 @@ private async Task SendRecordAsync( return 2 + response.Length; } + private static long CalculateAxfrTransferSizeBytes( + List records, + ushort queryId, + string zoneName) + { + var soaRecord = records.FirstOrDefault(r => r.RecordType == ResourceRecordType.SOA); + long size = 0; + + if (soaRecord != null) + { + size += CalculateTcpRecordMessageSize(soaRecord, queryId, zoneName); + } + + foreach (var record in records) + { + if (record.RecordType != ResourceRecordType.SOA) + { + size += CalculateTcpRecordMessageSize(record, queryId, zoneName); + } + } + + if (soaRecord != null) + { + size += CalculateTcpRecordMessageSize(soaRecord, queryId, zoneName); + } + + return size; + } + + private static long CalculateTcpRecordMessageSize( + FilteredRecord record, + ushort queryId, + string zoneName) + { + var response = DnsRecordBuilder.BuildZoneTransferResponse([record], queryId, zoneName); + return 2L + response.Length; + } + private async Task HandleSoaQueryOnTcpAsync( NetworkStream stream, IPEndPoint remoteEndPoint, @@ -2100,5 +2088,55 @@ private static bool IsSerialNewer(uint candidateSerial, uint referenceSerial) return referenceSerial - (ulong)candidateSerial > halfSerialSpace; } + + private static DnsResponseCode GetNotifyResponseCode( + Dictionary zones, + string zoneName, + IPAddress remoteAddress) + { + var zoneLookup = zoneName.TrimEnd('.').ToLowerInvariant(); + if (!zones.TryGetValue(zoneLookup, out var zoneEntry)) + { + return DnsResponseCode.Refused; + } + + var upstreamParts = zoneEntry.Config.Upstream.Split(':'); + if (upstreamParts.Length == 0 || !IPAddress.TryParse(upstreamParts[0], out var upstreamIp)) + { + return DnsResponseCode.Refused; + } + + return AddressesEqual(remoteAddress, upstreamIp) ? DnsResponseCode.NoError : DnsResponseCode.Refused; + } + + private static bool AddressesEqual(IPAddress left, IPAddress right) + { + if (left.Equals(right)) + { + return true; + } + + if (left.IsIPv4MappedToIPv6) + { + left = left.MapToIPv4(); + } + if (right.IsIPv4MappedToIPv6) + { + right = right.MapToIPv4(); + } + + return left.Equals(right); + } + + private static byte[] BuildUdpSaturationResponse(byte[] requestData) + { + ushort messageId = 0; + if (requestData.Length >= 2) + { + messageId = (ushort)((requestData[0] << 8) | requestData[1]); + } + + return DnsMessageParser.BuildResponse(new DnsMessage { Id = messageId }, DnsResponseCode.ServFail); + } } diff --git a/REVIEW_FINDINGS.md b/REVIEW_FINDINGS.md index 3a3dc69..3910fdc 100644 --- a/REVIEW_FINDINGS.md +++ b/REVIEW_FINDINGS.md @@ -30,6 +30,14 @@ Fix first: - [x] **SEV-07:** Fixed in this branch. Transfer-triggered refreshes update cache/history through the proxy's per-zone lock and skip slave NOTIFY. - [x] **SEV-08:** Fixed in this branch. Added snapshot-based IXFR diff coverage. - [x] **SEV-09:** Fixed in this branch. Minimum record count is checked before poll/NOTIFY/transfer-refresh cache/history updates. +- [x] **SEV-10:** Fixed in this branch. Added CAA wire-format coverage for query and IXFR responses. +- [x] **SEV-11:** Fixed in this branch. AXFR uses exact preflight sizing before streaming. +- [x] **SEV-12:** Fixed in this branch. NOTIFY responses are selected after zone/source validation. +- [x] **SEV-13:** Fixed in this branch. UDP saturation now sends a SERVFAIL response. +- [x] **SEV-14:** Fixed in this branch. Verification mismatches now schedule retry verification after re-NOTIFY. +- [x] **SEV-15:** Fixed in this branch. Verification mismatches report to self-restart and restart window limits are enforced. +- [x] **SEV-16:** Fixed in this branch. Startup/export use shared configuration validation. +- [x] **SEV-17:** Fixed in this branch. History save no longer prunes the live model and replaces files atomically. ## Findings @@ -144,6 +152,7 @@ Fix first: ### [SEV-10] CAA records are serialized with an invalid extra value-length byte - **Severity:** Medium +- **Status:** Fixed in this branch - **Confidence:** Confirmed - **Location:** `FilterDns/Dns/DnsRecordBuilder.cs:377-399`, `FilterDns/Dns/IxfrResponseBuilder.cs:437-445` - **What's wrong:** RFC 6844 CAA RDATA is `flags`, `tag length`, `tag`, then `value` as the remaining bytes. Both serializers add an extra `value length` byte before the value. @@ -155,6 +164,7 @@ Fix first: ### [SEV-11] AXFR size limit can abort after sending a partial transfer - **Severity:** Medium +- **Status:** Fixed in this branch - **Confidence:** Confirmed - **Location:** `FilterDns/Xfer/XferHandler.cs:1207-1230`, `FilterDns/Xfer/XferHandler.cs:1261-1308` - **What's wrong:** There is a rough pre-check based on `recordCount * 100`, then exact byte checks while streaming. If the exact count exceeds `MaxZoneTransferSizeBytes` after the opening SOA or a later record, the method returns without sending a DNS error or a complete trailing SOA. @@ -166,6 +176,7 @@ Fix first: ### [SEV-12] NOTIFY receive sends NOERROR before validating zone and source - **Severity:** Medium +- **Status:** Fixed in this branch - **Confidence:** Confirmed - **Location:** `FilterDns/Xfer/XferHandler.cs:658-676`, `FilterDns/Xfer/XferHandler.cs:680-735` - **What's wrong:** The UDP NOTIFY handler sends `NoError` immediately for any SOA NOTIFY question, then checks whether the zone exists and whether the sender matches the configured upstream. @@ -177,6 +188,7 @@ Fix first: ### [SEV-13] UDP concurrency limiting silently drops DNS packets - **Severity:** Medium +- **Status:** Fixed in this branch - **Confidence:** Confirmed - **Location:** `FilterDns/Xfer/XferHandler.cs:215-229` - **What's wrong:** When `MaxConcurrentUdpRequests` is saturated, the server logs and `continue`s without any DNS response. @@ -188,6 +200,7 @@ Fix first: ### [SEV-14] Verification mismatch recovery does not schedule a retry check - **Severity:** Medium +- **Status:** Fixed in this branch - **Confidence:** Confirmed - **Location:** `FilterDns/Verify/SlaveVerificationService.cs:110-146`, `FilterDns/Verify/SlaveVerificationService.cs:181-187` - **What's wrong:** On mismatch within retry budget, the service may clear history and resend NOTIFY, then exits. It does not schedule another delayed verification for the same expected serial. The retry counter only advances if a later external `ScheduleVerification` call happens. @@ -199,6 +212,7 @@ Fix first: ### [SEV-15] Self-restart verification and window-limit config is not wired - **Severity:** Medium +- **Status:** Fixed in this branch - **Confidence:** Confirmed - **Location:** `FilterDns/Config/Configuration.cs:82-118`, `FilterDns/Recovery/SelfRestartService.cs:70-87`, `FilterDns/Recovery/SelfRestartService.cs:146-205` - **What's wrong:** `MaxRestartsInWindow` and `RestartWindowSeconds` are configured but never enforced. `ReportVerificationFailure` implements per-zone verification failure counting, but no verification code calls it. @@ -210,6 +224,7 @@ Fix first: ### [SEV-16] Startup/export configuration validation misses important contract checks - **Severity:** Medium +- **Status:** Fixed in this branch - **Confidence:** Confirmed - **Location:** `FilterDns/Program.cs:47-65`, `FilterDns/Program.cs:148-196`, `FilterDns/Program.cs:223-256`, `FilterDns/Upstream/UpstreamClient.cs:14-20` - **What's wrong:** Server startup validation only checks zones, `Ns1`/`Ns2`, and zone-name syntax. Export skips validation entirely. Invalid log levels throw `Enum.Parse` exceptions, malformed upstream/listen addresses fail later, duplicate zones overwrite silently, and invalid private ranges can silently keep records public. @@ -221,6 +236,7 @@ Fix first: ### [SEV-17] History save can mutate live history and replace files non-atomically - **Severity:** Medium +- **Status:** Fixed in this branch - **Confidence:** Confirmed - **Location:** `FilterDns/Cache/ZoneHistoryStorage.cs:281-291`, `FilterDns/Cache/ZoneHistoryStorage.cs:339-347`, `FilterDns/Cache/ZoneHistory.cs:143-176` - **What's wrong:** `SaveAsync` prunes the live `ZoneHistory` object as a side effect, while `UpdateZoneHistoryAsync` already prunes by effective history depth. The disk replace deletes the old file before moving the temp file into place. From 63c5b9c6c87c58dc2c6da5cef72df2f8bbc4a8cb Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Thu, 25 Jun 2026 06:56:08 +0000 Subject: [PATCH 6/7] Fix low severity storage hardening issues Co-authored-by: Gerold K. --- FilterDns.Tests/LowFindingTests.cs | 50 +++++++++++++++++++++++++++ FilterDns/Cache/ZoneHistoryStorage.cs | 14 +++++--- REVIEW_FINDINGS.md | 4 +++ 3 files changed, 64 insertions(+), 4 deletions(-) create mode 100644 FilterDns.Tests/LowFindingTests.cs diff --git a/FilterDns.Tests/LowFindingTests.cs b/FilterDns.Tests/LowFindingTests.cs new file mode 100644 index 0000000..f8fa1cc --- /dev/null +++ b/FilterDns.Tests/LowFindingTests.cs @@ -0,0 +1,50 @@ +using System.Security; +using FilterDns.Cache; +using FilterDns.Config; + +namespace FilterDns.Tests; + +public class LowFindingTests +{ + [Fact] + public void ZoneHistoryStorage_RejectsSiblingPathWithSamePrefix() + { + var baseDirectory = Path.Combine(Path.GetTempPath(), $"filterdns-low-{Guid.NewGuid():N}", "data"); + var siblingDirectory = baseDirectory + "-backup"; + Directory.CreateDirectory(baseDirectory); + Directory.CreateDirectory(siblingDirectory); + var storage = new ZoneHistoryStorage(baseDirectory, exportBindZoneFiles: false); + + Assert.Throws(() => + InvokeValidatePath(storage, Path.Combine(siblingDirectory, "example.json"), baseDirectory)); + } + + [Fact] + public void ZoneHistoryStorage_TreatsBrokenSymlinkAsUnsafeWhenHardeningEnabled() + { + var baseDirectory = Path.Combine(Path.GetTempPath(), $"filterdns-low-{Guid.NewGuid():N}"); + Directory.CreateDirectory(baseDirectory); + var symlinkPath = Path.Combine(baseDirectory, "broken-link.json"); + File.CreateSymbolicLink(symlinkPath, Path.Combine(baseDirectory, "missing-target.json")); + var storage = new ZoneHistoryStorage(baseDirectory, exportBindZoneFiles: false); + + Assert.Throws(() => InvokeValidatePath(storage, symlinkPath, baseDirectory)); + } + + private static void InvokeValidatePath(ZoneHistoryStorage storage, string filePath, string baseDirectory) + { + var method = typeof(ZoneHistoryStorage).GetMethod( + "ValidatePathWithinDirectory", + System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic); + Assert.NotNull(method); + + try + { + method.Invoke(storage, [filePath, baseDirectory]); + } + catch (System.Reflection.TargetInvocationException ex) when (ex.InnerException != null) + { + throw ex.InnerException; + } + } +} diff --git a/FilterDns/Cache/ZoneHistoryStorage.cs b/FilterDns/Cache/ZoneHistoryStorage.cs index 1de1e13..248f10f 100644 --- a/FilterDns/Cache/ZoneHistoryStorage.cs +++ b/FilterDns/Cache/ZoneHistoryStorage.cs @@ -198,12 +198,14 @@ private void ValidatePathWithinDirectory(string filePath, string baseDirectory) return; // Skip validation if hardening disabled } - // Resolve to absolute paths var absoluteFilePath = Path.GetFullPath(filePath); var absoluteBaseDir = Path.GetFullPath(baseDirectory); + var baseWithSeparator = absoluteBaseDir.EndsWith(Path.DirectorySeparatorChar) + ? absoluteBaseDir + : absoluteBaseDir + Path.DirectorySeparatorChar; - // Ensure the file path is within the base directory - if (!absoluteFilePath.StartsWith(absoluteBaseDir, StringComparison.Ordinal)) + if (!absoluteFilePath.Equals(absoluteBaseDir, StringComparison.Ordinal) + && !absoluteFilePath.StartsWith(baseWithSeparator, StringComparison.Ordinal)) { throw new System.Security.SecurityException($"Path '{filePath}' escapes data directory '{baseDirectory}'"); } @@ -238,7 +240,11 @@ private bool IsSymlink(string path) } catch { - // If we can't check, assume it's not a symlink (fail open for compatibility) + if (_securityConfig?.SecurityHardeningEnabled ?? true) + { + throw new System.Security.SecurityException($"Unable to inspect path '{path}' for symlinks"); + } + return false; } diff --git a/REVIEW_FINDINGS.md b/REVIEW_FINDINGS.md index 3910fdc..c812851 100644 --- a/REVIEW_FINDINGS.md +++ b/REVIEW_FINDINGS.md @@ -38,6 +38,8 @@ Fix first: - [x] **SEV-15:** Fixed in this branch. Verification mismatches report to self-restart and restart window limits are enforced. - [x] **SEV-16:** Fixed in this branch. Startup/export use shared configuration validation. - [x] **SEV-17:** Fixed in this branch. History save no longer prunes the live model and replaces files atomically. +- [x] **SEV-18:** Fixed in this branch. Path containment now checks segment boundaries and symlink inspection fails closed under hardening. +- [x] **SEV-19:** Fixed in this branch. Added `FilterDns.Tests` with regression coverage for the audited critical, high, medium, and low paths. ## Findings @@ -248,6 +250,7 @@ Fix first: ### [SEV-18] Path containment and symlink checks fail open in storage hardening - **Severity:** Low +- **Status:** Fixed in this branch - **Confidence:** Confirmed - **Location:** `FilterDns/Cache/ZoneHistoryStorage.cs:190-215`, `FilterDns/Cache/ZoneHistoryStorage.cs:221-240` - **What's wrong:** `ValidatePathWithinDirectory` uses a plain string prefix check, so `/var/filterdns/data-backup/file` starts with `/var/filterdns/data`. `IsSymlink` returns false if attribute inspection fails. @@ -259,6 +262,7 @@ Fix first: ### [SEV-19] Critical paths have no automated tests - **Severity:** Low +- **Status:** Fixed in this branch - **Confidence:** Confirmed - **Location:** `FilterDns.sln:5`, `FilterDns/FilterDns.csproj:1-20` - **What's wrong:** The solution contains only the executable project and no test project. The codebase has complex DNS parser, CIDR, history persistence, IXFR diffing, and transfer orchestration logic with no regression coverage. From f47cbb5c83760854eeb19b48c06692ac607f2c68 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Thu, 25 Jun 2026 07:16:03 +0000 Subject: [PATCH 7/7] Address final review findings Co-authored-by: Gerold K. --- FilterDns.Tests/HighFindingTests.cs | 13 +++ FilterDns.Tests/LowFindingTests.cs | 16 ++++ FilterDns/Cache/ZoneHistoryStorage.cs | 38 +++++++- FilterDns/Program.cs | 129 -------------------------- FilterDns/Proxy/DnsProxyServer.cs | 32 +++---- REVIEW_FINDINGS.md | 10 +- 6 files changed, 82 insertions(+), 156 deletions(-) diff --git a/FilterDns.Tests/HighFindingTests.cs b/FilterDns.Tests/HighFindingTests.cs index 218bc90..d3b1663 100644 --- a/FilterDns.Tests/HighFindingTests.cs +++ b/FilterDns.Tests/HighFindingTests.cs @@ -123,6 +123,19 @@ public void XferHandler_TreatsWrappedUpstreamSerialAsNewer() Assert.False((bool)method.Invoke(null, [uint.MaxValue, 1u])!); } + [Fact] + public void DnsProxyServer_OnlyFetchesWhenUpstreamSerialIsNewer() + { + var method = typeof(DnsProxyServer).GetMethod( + "ShouldFetchFromUpstream", + System.Reflection.BindingFlags.Static | System.Reflection.BindingFlags.NonPublic); + Assert.NotNull(method); + + Assert.False((bool)method.Invoke(null, [3u, true, 2u])!); + Assert.True((bool)method.Invoke(null, [uint.MaxValue, true, 1u])!); + Assert.True((bool)method.Invoke(null, [3u, false, 2u])!); + } + [Fact] public async Task TransferRefresh_DoesNotCacheZoneBelowMinimumRecordCount() { diff --git a/FilterDns.Tests/LowFindingTests.cs b/FilterDns.Tests/LowFindingTests.cs index f8fa1cc..79835be 100644 --- a/FilterDns.Tests/LowFindingTests.cs +++ b/FilterDns.Tests/LowFindingTests.cs @@ -31,6 +31,22 @@ public void ZoneHistoryStorage_TreatsBrokenSymlinkAsUnsafeWhenHardeningEnabled() Assert.Throws(() => InvokeValidatePath(storage, symlinkPath, baseDirectory)); } + [Fact] + public void ZoneHistoryStorage_RejectsSymlinkedAncestorDirectory() + { + var root = Path.Combine(Path.GetTempPath(), $"filterdns-low-{Guid.NewGuid():N}"); + var dataDirectory = Path.Combine(root, "data"); + var outsideDirectory = Path.Combine(root, "outside"); + Directory.CreateDirectory(dataDirectory); + Directory.CreateDirectory(outsideDirectory); + var historyLink = Path.Combine(dataDirectory, "history"); + Directory.CreateSymbolicLink(historyLink, outsideDirectory); + var storage = new ZoneHistoryStorage(dataDirectory, exportBindZoneFiles: false); + + Assert.Throws(() => + InvokeValidatePath(storage, Path.Combine(historyLink, "example_com.json"), dataDirectory)); + } + private static void InvokeValidatePath(ZoneHistoryStorage storage, string filePath, string baseDirectory) { var method = typeof(ZoneHistoryStorage).GetMethod( diff --git a/FilterDns/Cache/ZoneHistoryStorage.cs b/FilterDns/Cache/ZoneHistoryStorage.cs index 248f10f..a15cd0d 100644 --- a/FilterDns/Cache/ZoneHistoryStorage.cs +++ b/FilterDns/Cache/ZoneHistoryStorage.cs @@ -213,13 +213,47 @@ private void ValidatePathWithinDirectory(string filePath, string baseDirectory) // Check for symlinks if protection is enabled if (_securityConfig?.EnableSymlinkProtection ?? true) { - if (IsSymlink(filePath)) + if (ContainsSymlinkInPath(absoluteFilePath, absoluteBaseDir)) { - throw new System.Security.SecurityException($"Path '{filePath}' is a symlink, which is not allowed"); + throw new System.Security.SecurityException($"Path '{filePath}' contains a symlink, which is not allowed"); } } } + private bool ContainsSymlinkInPath(string absoluteFilePath, string absoluteBaseDir) + { + var baseFullPath = Path.GetFullPath(absoluteBaseDir).TrimEnd(Path.DirectorySeparatorChar); + var current = Path.GetFullPath(absoluteFilePath); + + var pathsToCheck = new Stack(); + while (!string.IsNullOrEmpty(current) && current.Length >= baseFullPath.Length) + { + pathsToCheck.Push(current); + if (string.Equals(current.TrimEnd(Path.DirectorySeparatorChar), baseFullPath, StringComparison.Ordinal)) + { + break; + } + + var parent = Path.GetDirectoryName(current); + if (string.Equals(parent, current, StringComparison.Ordinal)) + { + break; + } + current = parent ?? string.Empty; + } + + while (pathsToCheck.Count > 0) + { + var path = pathsToCheck.Pop(); + if ((File.Exists(path) || Directory.Exists(path)) && IsSymlink(path)) + { + return true; + } + } + + return false; + } + /// /// Checks if a path is a symlink. /// diff --git a/FilterDns/Program.cs b/FilterDns/Program.cs index 8bb9abc..80d79a4 100644 --- a/FilterDns/Program.cs +++ b/FilterDns/Program.cs @@ -12,7 +12,6 @@ using Serilog.Extensions.Logging; using System.Net; using System.Text.Json; -using System.Text.RegularExpressions; namespace FilterDns; @@ -221,134 +220,6 @@ private static async Task HandleExportCommandAsync(string[] args) } } - private static void ValidateConfiguration(AppConfiguration config) - { - if (config.Zones == null || config.Zones.Count == 0) - { - throw new InvalidOperationException("No zones configured"); - } - - var securityConfig = config.Server.Security; - var hardeningEnabled = securityConfig?.SecurityHardeningEnabled ?? true; - - foreach (var zone in config.Zones) - { - if (string.IsNullOrEmpty(zone.Ns1) || string.IsNullOrEmpty(zone.Ns2)) - { - throw new InvalidOperationException($"Zone {zone.Name} must have NS1 and NS2 configured"); - } - - // Validate zone name if hardening enabled - if (hardeningEnabled) - { - try - { - ValidateZoneName(zone.Name, securityConfig); - } - catch (ArgumentException ex) - { - throw new InvalidOperationException($"Invalid zone name '{zone.Name}': {ex.Message}", ex); - } - } - else - { - Console.WriteLine($"Warning: Zone name validation skipped for '{zone.Name}' (SecurityHardeningEnabled=false)"); - } - } - } - - /// - /// Validates a zone name according to RFC 1035 DNS name format. - /// - private static void ValidateZoneName(string zoneName, SecurityConfig? securityConfig) - { - if (string.IsNullOrWhiteSpace(zoneName)) - { - throw new ArgumentException("Zone name cannot be null or empty", nameof(zoneName)); - } - - var maxZoneNameLength = securityConfig?.MaxZoneNameLength ?? 253; - var hardeningEnabled = securityConfig?.SecurityHardeningEnabled ?? true; - - if (!hardeningEnabled) - { - return; // Skip validation if hardening disabled - } - - // Normalize: remove trailing dot if present, convert to lowercase - var normalized = zoneName.TrimEnd('.').ToLowerInvariant(); - - // Check length (RFC 1035: max 253 bytes for FQDN, but we use characters) - // DNS names are typically ASCII, so character count approximates byte count - if (normalized.Length > maxZoneNameLength) - { - throw new ArgumentException($"Zone name length ({normalized.Length}) exceeds maximum ({maxZoneNameLength})", nameof(zoneName)); - } - - // Check for empty after normalization - if (string.IsNullOrEmpty(normalized)) - { - throw new ArgumentException("Zone name cannot be empty or only dots", nameof(zoneName)); - } - - // RFC 1035: DNS name format validation - // - Each label: 1-63 characters, alphanumeric and hyphen - // - Labels separated by dots - // - Total length: max 253 characters (for FQDN) - // - Cannot start or end with hyphen in a label - // - Cannot have consecutive dots - - // Check for consecutive dots - if (normalized.Contains("..")) - { - throw new ArgumentException("Zone name contains consecutive dots", nameof(zoneName)); - } - - // Split into labels - var labels = normalized.Split('.'); - - if (labels.Length == 0) - { - throw new ArgumentException("Zone name has no labels", nameof(zoneName)); - } - - // Validate each label - foreach (var label in labels) - { - if (string.IsNullOrEmpty(label)) - { - throw new ArgumentException("Zone name contains empty label", nameof(zoneName)); - } - - // Label length: 1-63 characters (RFC 1035) - if (label.Length > 63) - { - throw new ArgumentException($"Label '{label}' length ({label.Length}) exceeds maximum (63)", nameof(zoneName)); - } - - // Label must start and end with alphanumeric (cannot start/end with hyphen) - if (label.StartsWith('-') || label.EndsWith('-')) - { - throw new ArgumentException($"Label '{label}' cannot start or end with hyphen", nameof(zoneName)); - } - - // Label characters: alphanumeric and hyphen (RFC 1035) - // Note: We allow underscore for compatibility, though not strictly RFC 1035 - if (!Regex.IsMatch(label, @"^[a-z0-9_-]+$", RegexOptions.IgnoreCase)) - { - throw new ArgumentException($"Label '{label}' contains invalid characters (only alphanumeric, hyphen, underscore allowed)", nameof(zoneName)); - } - } - - // Check total length in bytes (approximate - ASCII characters are 1 byte) - // For internationalized domain names, this is more complex, but we'll use character count as approximation - var byteLength = System.Text.Encoding.UTF8.GetByteCount(normalized); - if (byteLength > 253) - { - throw new ArgumentException($"Zone name byte length ({byteLength}) exceeds maximum (253)", nameof(zoneName)); - } - } - private static Serilog.Events.LogEventLevel ConvertLogLevel(LogLevel logLevel) { return logLevel switch diff --git a/FilterDns/Proxy/DnsProxyServer.cs b/FilterDns/Proxy/DnsProxyServer.cs index 5714c72..27562bb 100644 --- a/FilterDns/Proxy/DnsProxyServer.cs +++ b/FilterDns/Proxy/DnsProxyServer.cs @@ -556,25 +556,13 @@ private async Task UpdateZoneFromUpstreamAsync( var cachedSerial = _cache.GetSerial(zoneName); var cachedZoneCheck = _cache.GetZone(zoneName); - // If cache is empty (after restart), always fetch to populate it - // This prevents empty zones on slaves when they initiate transfers after receiving NOTIFY - if (cachedSerial == null || cachedZoneCheck == null || cachedZoneCheck.Records.Count == 0) + var cacheHasRecords = cachedZoneCheck != null && cachedZoneCheck.Records.Count > 0; + if (!ShouldFetchFromUpstream(cachedSerial, cacheHasRecords, upstreamSerial)) { - _logger.LogInformation("{Prefix}: Zone {Zone} cache is empty or invalid, fetching from upstream (serial: {Serial})", - logPrefix, zoneName, upstreamSerial); - // Continue to fetch below - } - // If serial hasn't changed, skip the update but still notify slaves if this was triggered by NOTIFY - // This ensures slaves are notified even if our serial check shows no change (handles race conditions) - else if (cachedSerial == upstreamSerial) - { - _logger.LogInformation("{Prefix}: Zone {Zone} serial unchanged ({Serial}), no update needed", - logPrefix, zoneName, upstreamSerial); + _logger.LogInformation("{Prefix}: Zone {Zone} upstream serial {UpstreamSerial} is not newer than cached serial {CachedSerial}, no update needed", + logPrefix, zoneName, upstreamSerial, cachedSerial ?? 0); - // If triggered by NOTIFY, still send NOTIFY to slaves to ensure they check for updates - // This handles cases where upstream sent NOTIFY but our serial check shows unchanged - // This prevents empty zones on slaves when they initiate transfers after receiving NOTIFY - if (triggeredByNotify) + if (triggeredByNotify && cachedSerial == upstreamSerial) { if (_notifySenders.TryGetValue(zoneName, out var notifySenderForUnchanged)) { @@ -1233,6 +1221,16 @@ private static bool IsSerialNewer(uint candidateSerial, uint referenceSerial) return referenceSerial - (ulong)candidateSerial > halfSerialSpace; } + private static bool ShouldFetchFromUpstream(uint? cachedSerial, bool cacheHasRecords, uint upstreamSerial) + { + if (cachedSerial == null || !cacheHasRecords) + { + return true; + } + + return IsSerialNewer(upstreamSerial, cachedSerial.Value); + } + /// /// Gets zone history for a zone. Used by XferHandler. /// diff --git a/REVIEW_FINDINGS.md b/REVIEW_FINDINGS.md index c812851..1f2b7f1 100644 --- a/REVIEW_FINDINGS.md +++ b/REVIEW_FINDINGS.md @@ -2,7 +2,7 @@ ## Summary -FilterDNS is a single .NET console DNS proxy. It pulls zones from an upstream master over AXFR, rewrites SOA/NS records, optionally filters private A/AAAA data, stores file-backed zone history, then serves AXFR/IXFR to slaves and ACL-gated health-check queries. The highest-risk paths are DNS wire parsing, ACL/range matching, zone history persistence, IXFR diffing, and cache-update side effects. The codebase builds cleanly, but several normal DNS scenarios can still produce corrupt transfers, refused transfers, or silent operational failures. +FilterDNS is a single .NET console DNS proxy. It pulls zones from an upstream master over AXFR, rewrites SOA/NS records, optionally filters private A/AAAA data, stores file-backed zone history, then serves AXFR/IXFR to slaves and ACL-gated health-check queries. This audit found 19 issues across DNS wire parsing, ACL/range matching, zone history persistence, IXFR diffing, cache-update side effects, recovery, validation, and storage hardening. All findings listed below are fixed in this branch and covered by focused regression tests where practical. Total findings: 19 @@ -11,13 +11,7 @@ Total findings: 19 - Medium: 8 - Low: 2 -Fix first: - -1. Fix persisted history RDATA loss before trusting IXFR after restart. -2. Replace the shared CIDR matcher before relying on whitelists or private-IP filtering. -3. Fix IXFR/RRset diff identity so duplicate records and TTL changes are not lost. -4. Normalize zone names at configuration load so mixed-case zones do not break external DNS paths. -5. Remove cache-update side effects from health checks and transfer-triggered refreshes. +Fix status: all findings are complete in this branch. ### Fix Progress