Date: 2025-01-27
Reviewer: Security Specialist
Version: 1.0
This security review identified 15 security issues ranging from Critical to Medium severity. The application is a DNS master proxy server that handles zone transfers (AXFR/IXFR), NOTIFY messages, and health check queries. While the application implements basic IP whitelisting, several critical vulnerabilities could lead to denial of service, information disclosure, and potential remote code execution.
Severity: CRITICAL
Status: ✅ FIXED (2025-01-27)
Location: FilterDns/Dns/DnsMessageParser.cs
Issues:
- Compression Pointer Loop: The
ReadDomainNamefunction doesn't limit compression pointer jumps, allowing infinite loops or buffer overflows - No Bounds Checking: Domain name parsing doesn't validate offset stays within buffer bounds
- No Maximum Message Size: TCP messages can be up to 65535 bytes, but no validation prevents maliciously crafted messages
- Unlimited Recursion: Compression pointers can create deep recursion without limits
Code Reference:
// Lines 158-198: ReadDomainName lacks proper bounds checking
while (offset < data.Length)
{
var length = data[offset++];
// No check if offset exceeds bounds after increment
if ((length & 0xC0) == 0xC0)
{
offset = ((length & 0x3F) << 8) | data[offset]; // Can jump to invalid offset
// No loop detection - can create infinite loops
}
}Impact: Remote code execution via buffer overflow, or denial of service via infinite loops
Recommendation:
- Add maximum compression pointer depth (e.g., 10 jumps)
- Validate all offsets before array access
- Add loop detection for compression pointers
- Enforce maximum domain name length (255 bytes per RFC 1035)
Implementation Notes:
- ✅ Added
SecurityConfigclass with configurable limits (MaxCompressionPointerDepth,MaxDomainNameLength,MaxLabelLength,MaxMessageCounts) - ✅ Refactored
ReadDomainNameto track visited offsets usingHashSet<int>to detect compression pointer loops - ✅ Added compression depth counter (max 10 by default, configurable)
- ✅ Added
ValidateBounds()helper method for all buffer access operations - ✅ Updated
ReadUInt16,ReadUInt32to validate bounds before reading - ✅ Added validation for message counts (
qdCount,anCount,nsCount,arCount) with configurable maximum - ✅ Wrapped
Parse()method in try-catch to returnFormatExceptionfor malformed messages - ✅ Updated
ReadResourceRecord,SkipResourceRecord, andExtractSerialFromSoaRecordwith bounds checking - ✅ Added loop detection to
ExtractSerialFromSoaRecordfor domain name parsing within RDATA - ✅ All limits are configurable via
appsettings.jsonSecurity section - ✅ Master switch
SecurityHardeningEnabledallows disabling hardening (not recommended for production) - ✅ Default behavior: Hardening enabled with RFC 1035 compliant limits
Severity: CRITICAL
Status: ✅ FIXED (2025-01-27)
Location: FilterDns/Cache/ZoneHistoryStorage.cs
Issue: Zone name sanitization is insufficient. Only replaces ., , /, \ but doesn't prevent:
- Unicode normalization attacks
- Path traversal sequences (
..) - Null bytes
- Other special filesystem characters
Code Reference:
// Lines 82-87: Weak sanitization
var sanitizedZoneName = zoneName
.Replace(".", "_")
.Replace(" ", "_")
.Replace("/", "_")
.Replace("\\", "_");
// Missing: .., null bytes, unicode, etc.Impact: Arbitrary file read/write outside intended directory, potential for code execution if zone files are executed
Recommendation:
- Use
Path.GetFileName()to extract only filename - Whitelist allowed characters (alphanumeric, underscore, hyphen)
- Reject any zone name containing path separators or
.. - Use
Path.Combine()with validation to ensure paths stay within data directory
Implementation Notes:
- ✅ Created
SanitizeZoneName()method with comprehensive validation:- Checks for path traversal sequences (
..) - Rejects path separators (
/,\) - Rejects null bytes
- Normalizes Unicode using NFKC normalization
- Whitelist validation: only allows alphanumeric, underscore, hyphen, dot
- Configurable maximum zone name length (default: 253 characters)
- Checks for path traversal sequences (
- ✅ Created
ValidatePathWithinDirectory()method:- Uses
Path.GetFullPath()to resolve paths - Verifies resolved path starts with data directory (case-sensitive on Linux)
- Throws
SecurityExceptionif path escapes data directory
- Uses
- ✅ Added
ValidateDataDirectoryPath()to constructor:- Validates data directory path is absolute
- Rejects paths containing
.. - Ensures path is a directory, not a file
- ✅ Updated
GetHistoryFilePath()andGetZoneFilePath()to use new validation - ✅ Added symlink protection via
IsSymlink()method:- Checks
FileAttributes.ReparsePointflag - Configurable via
EnableSymlinkProtectionsetting - Rejects symlinks in data directory when enabled
- Checks
- ✅ All file operations validate paths before access
- ✅ Master switch
SecurityHardeningEnabledallows disabling hardening (not recommended) - ✅ Default behavior: Hardening enabled with strict path validation
Severity: CRITICAL
Status: ✅ FIXED (2025-01-27)
Location: FilterDns/Xfer/XferHandler.cs
Issues:
- No limit on concurrent TCP connections
- No limit on concurrent UDP requests
- TCP connections can be held open indefinitely
- No timeout on zone transfer operations
- Large zone transfers can consume unlimited memory
Code Reference:
// Lines 57-92: No connection limits
while (!cancellationToken.IsCancellationRequested)
{
var tcpClient = await _tcpListener.AcceptTcpClientAsync(cancellationToken);
_ = Task.Run(async () => { /* No timeout, no resource limits */ });
}Impact: Denial of service - attacker can exhaust file descriptors, memory, or CPU by opening many connections or initiating large transfers
Recommendation:
- Implement connection pool with maximum concurrent connections (e.g., 100)
- Add per-IP rate limiting (e.g., max 10 connections per IP)
- Set TCP connection timeouts (e.g., 5 minutes)
- Limit zone transfer size (e.g., max 10MB per transfer)
- Implement circuit breaker pattern for repeated failures
Implementation Notes:
- ✅ Added
SecurityConfigproperties:MaxConcurrentTcpConnections(default: 100),MaxConcurrentUdpRequests(default: 200),MaxConnectionsPerIp(default: 10),TcpConnectionTimeoutSeconds(default: 300),ZoneTransferTimeoutSeconds(default: 600),MaxZoneTransferSizeBytes(default: 10MB) - ✅ Implemented
SemaphoreSlimfor TCP and UDP connection limiting inXferHandler - ✅ Added
ConcurrentDictionary<IPAddress, int>for per-IP connection tracking - ✅ Wrapped TCP connection acceptance with semaphore and per-IP limit checks
- ✅ Wrapped UDP request handling with semaphore
- ✅ Set TCP client receive/send timeouts on accepted connections
- ✅ Added
CancellationTokenSourcewith timeout for zone transfers - ✅ Track zone transfer size during transfer and reject if exceeds limit
- ✅ Added periodic cleanup timer for connection tracking (every 5 minutes) to prevent memory leaks
- ✅ Audit logging when limits are exceeded (if
AuditLogFailedTransfersenabled) - ✅ All limits configurable via
appsettings.jsonSecurity section - ✅ Master switch
SecurityHardeningEnabledcontrols all DoS protection features - ✅ Default behavior: Hardening enabled, all limits active
Severity: CRITICAL
Status: ✅ FIXED (2025-01-27)
Location: FilterDns/Dns/DnsMessageParser.cs:ReadDomainName()
Issue: Compression pointers can create loops (pointer A points to B, B points to A) causing infinite loops or stack overflow
Code Reference:
// Lines 172-181: No loop detection
if ((length & 0xC0) == 0xC0)
{
if (!jumped)
{
jumpOffset = offset + 1;
jumped = true;
}
offset = ((length & 0x3F) << 8) | data[offset];
continue; // Can loop forever if compression pointers form cycle
}Impact: Denial of service - single malicious DNS packet can hang or crash the server
Recommendation:
- Track visited offsets in a set
- Limit maximum compression pointer depth (RFC 1035 recommends 10)
- Reject messages with compression pointer loops
Implementation Notes:
- ✅ Added
HashSet<int> visitedOffsetsto track all compression pointer jumps - ✅ Added compression depth counter with configurable maximum (default: 10, RFC 1035 compliant)
- ✅ Detects compression pointer loops by checking if offset was already visited
- ✅ Throws
FormatExceptionimmediately when loop detected - ✅ Validates compression pointer doesn't point outside buffer bounds
- ✅ Applied same loop detection to
ExtractSerialFromSoaRecord()for domain names in RDATA - ✅ Configurable via
SecurityConfig.MaxCompressionPointerDepthinappsettings.json - ✅ Master switch
SecurityHardeningEnabledallows disabling (not recommended) - ✅ Default behavior: Hardening enabled, max depth 10, loop detection active
Severity: HIGH
Location: FilterDns/Xfer/XferHandler.cs:HandleUdpRequestAsync()
Issue: NOTIFY messages are only validated by source IP address. No cryptographic authentication (TSIG) or message integrity checking.
Code Reference:
// Lines 318-322: Only IP-based validation
var upstreamIp = IPAddress.Parse(upstreamParts[0]);
if (remoteEndPoint.Address.Equals(upstreamIp))
{
// Accept NOTIFY - no cryptographic verification
}Impact: IP spoofing attacks can trigger unnecessary zone transfers and NOTIFY storms
Recommendation:
- Implement TSIG (Transaction SIGnature) support for NOTIFY authentication
- Add rate limiting on NOTIFY messages per source IP
- Log and alert on NOTIFY messages from non-configured upstreams
Severity: HIGH
Location: FilterDns/Xfer/XferHandler.cs:HandleHealthCheckQueryAsync()
Issue: Health check queries return full zone data. If HealthCheckAcl is misconfigured or compromised, zone data can be leaked.
Code Reference:
// Lines 375-498: Health checks return full zone records
var matchingRecords = MatchQueryToRecords(question, zoneRecords, foundZoneName);
// Returns actual zone data including all recordsImpact: Zone enumeration and data exfiltration if ACL is misconfigured
Recommendation:
- Limit health check responses to specific record types (e.g., only SOA)
- Don't return actual zone data, only confirm zone exists
- Add audit logging for all health check queries
- Consider removing health check feature or making it opt-in with warnings
Severity: HIGH
Location: FilterDns/Dns/DnsMessageParser.cs:Parse()
Issues:
- Question count (
qdCount) not validated against actual message size - Authority count (
nsCount) can be manipulated to cause out-of-bounds access - No validation that counts match actual records in message
- Domain names not validated for length or format
Code Reference:
// Lines 77-80: Counts read without validation
var qdCount = ReadUInt16(data, ref offset);
var anCount = ReadUInt16(data, ref offset);
var nsCount = ReadUInt16(data, ref offset);
var arCount = ReadUInt16(data, ref offset);
// No check if these counts are reasonable or match message sizeImpact: Buffer overflows, out-of-bounds reads, or crashes from malformed messages
Recommendation:
- Validate counts are reasonable (e.g., qdCount <= 10, nsCount <= 50)
- Verify counts match actual records parsed
- Validate domain names are valid DNS names (RFC 1035)
- Reject messages with invalid field combinations
Severity: HIGH
Location: FilterDns/Cache/ZoneHistoryStorage.cs
Issue: No limits on zone history file size. Malicious or misconfigured zones can create arbitrarily large JSON files consuming disk space.
Code Reference:
// Lines 97-168: No size limits on history files
var json = JsonSerializer.Serialize(versionsJson, options);
var jsonBytes = System.Text.Encoding.UTF8.GetBytes(json);
await File.WriteAllBytesAsync(tempFilePath, jsonBytes);
// No check on jsonBytes.LengthImpact: Disk space exhaustion, memory exhaustion when loading large history files
Recommendation:
- Limit maximum history file size (e.g., 100MB)
- Limit maximum number of zone versions per zone
- Monitor disk usage and alert when thresholds exceeded
- Implement file size checks before writing
Severity: MEDIUM
Location: FilterDns/filter-dns.service
Issue: Service runs as root user, violating principle of least privilege.
Code Reference:
[Service]
User=root # Should be non-privileged user
Impact: Any vulnerability that leads to code execution runs with root privileges
Recommendation:
- Create dedicated user (e.g.,
filterdns) - Run service as non-root user
- Ensure data directory is writable by service user
- Use capabilities (CAP_NET_BIND_SERVICE) if port 53 requires privileges
Severity: MEDIUM
Location: Entire codebase
Issue: All zone transfers occur over plaintext TCP/UDP. No support for DNS-over-TLS (DoT) or DNS-over-HTTPS (DoH).
Impact: Zone data can be intercepted and modified in transit (man-in-the-middle attacks)
Recommendation:
- Implement DNS-over-TLS (RFC 7858) for zone transfers
- Add TSIG support for message authentication
- Document security implications of plaintext transfers
- Consider implementing DNS-over-HTTPS for health checks
Severity: MEDIUM
Status: ✅ FIXED (2025-01-27)
Location: Multiple files
Issue: Zone names are not fully validated. Accepts any string without checking:
- Valid DNS name format (RFC 1035)
- Maximum length (253 bytes)
- Invalid characters
- Case sensitivity handling
Impact: Potential for injection attacks, configuration errors, or crashes
Recommendation:
- Validate zone names match RFC 1035 format
- Reject zone names longer than 253 bytes
- Normalize zone names consistently (lowercase, trailing dot handling)
- Add validation in configuration loading
Implementation Notes:
- ✅ Created
ValidateZoneName()helper method inProgram.cswith RFC 1035 validation - ✅ Validates label length (1-63 characters per label)
- ✅ Validates total zone name length (max 253 bytes)
- ✅ Validates label format: alphanumeric, hyphen, underscore (allows underscore for compatibility)
- ✅ Rejects labels starting/ending with hyphen
- ✅ Rejects consecutive dots
- ✅ Normalizes zone names: removes trailing dot, converts to lowercase
- ✅ Validates during configuration loading in
ValidateConfiguration() - ✅ Uses
SecurityConfig.MaxZoneNameLengthfor configurable limit (default: 253) - ✅ Master switch
SecurityHardeningEnabledcontrols validation (skips with warning if disabled) - ✅ Default behavior: Hardening enabled, RFC 1035 compliant validation active
Severity: MEDIUM
Status: ✅ FIXED (2025-01-27)
Location: FilterDns/Cache/ZoneCache.cs
Issue: No validation that cached zone data hasn't been tampered with. No integrity checks on zone history files.
Impact: Corrupted zone data could be served to slaves if cache or history files are modified
Recommendation:
- Add checksums/hashes to zone history files
- Validate zone data integrity on load
- Detect and alert on unexpected zone data changes
- Implement read-only mode for zone cache after initial load
Implementation Notes:
- ✅ Added
Hashfield toZoneVersionJsonfor SHA-256 integrity hashes - ✅ Implemented
CalculateZoneVersionHash()method usingSHA256.HashData() - ✅ Hash includes serial, timestamp, record count, and record data for strong integrity protection
- ✅ Hash is calculated and stored when saving zone history (if
EnableCacheIntegrityChecksenabled) - ✅ Hash is validated when loading zone history - corrupted versions are rejected
- ✅ Added
SecurityConfigproperties:MaxZoneHistoryFileSizeBytes(default: 100MB),MaxZoneVersionsPerZone(default: 100),EnableCacheIntegrityChecks(default: true) - ✅ File size limit enforced before writing - throws exception if exceeds limit
- ✅ Version count limit enforced - automatically prunes oldest versions using
ZoneHistory.PruneOldVersions() - ✅ Integrity check failures are logged as warnings
- ✅ All settings configurable via
appsettings.jsonSecurity section - ✅ Master switch
SecurityHardeningEnabledcontrols integrity checks (skips with warning if disabled) - ✅ Default behavior: Hardening enabled, integrity checks active, file size and version limits enforced
Severity: MEDIUM
Status: ✅ FIXED (2025-01-27)
Location: FilterDns/Dns/DnsMessageParser.cs
Issue: Parsing errors can crash the server or leak information. No graceful degradation for malformed messages.
Code Reference:
// Various parsing functions don't handle all edge cases
// Exceptions can propagate and crash connection handlersImpact: Denial of service via malformed packets, information disclosure via error messages
Recommendation:
- Wrap all parsing in try-catch blocks
- Return REFUSED or SERVFAIL for malformed messages
- Log parsing errors without exposing internal details
- Implement message validation before parsing
Implementation Notes:
- ✅ Added comprehensive error handling in
HandleTcpConnectionAsync():- Catches
FormatExceptionfrom parsing and sendsFormErrresponse with proper message ID - Catches unexpected exceptions and sends
ServFailresponse - Continues handling other queries on same connection (doesn't break on parsing errors)
- Extracts message ID from buffer when possible for proper error response
- Catches
- ✅ Enhanced error handling in
HandleUdpRequestAsync():- Added catch for unexpected exceptions (not just
FormatException) - Sends
ServFailresponse for unexpected errors - Extracts message ID from buffer for proper error response
- Added catch for unexpected exceptions (not just
- ✅ Improved error messages in
DnsMessageParser.Parse():- Removed inner exception from
FormatExceptionto prevent stack trace exposure - All error messages are generic and don't expose internal implementation details
- Removed inner exception from
- ✅ Added message length validation before parsing:
- Validates message length is not zero
- Validates message length doesn't exceed DNS protocol limit (65535)
- Logs warnings for invalid message lengths
- ✅ Error logging improvements:
- Logs error type and message without full stack traces
- Uses generic error messages that don't expose internal details
- All parsing errors are logged at appropriate levels (Warning for malformed, Error for unexpected)
- ✅ All parsing operations now gracefully handle errors:
- TCP connections: Send error response and continue handling other queries
- UDP requests: Send error response and return
- No crashes or connection drops on parsing errors
- ✅ Default behavior: All error handling active, graceful degradation for malformed messages
Severity: MEDIUM
Status: ✅ FIXED (2025-01-27)
Location: Entire codebase
Issue: Security-relevant events are not logged:
- Failed zone transfer attempts
- NOTIFY messages from unauthorized sources
- Health check queries
- Configuration changes
Impact: Inability to detect attacks or investigate security incidents
Recommendation:
- Add comprehensive audit logging
- Log all zone transfer attempts (success and failure)
- Log all NOTIFY messages with source IP
- Log configuration file changes
- Implement log rotation and retention policies
Implementation Notes:
- ✅ Added
SecurityConfigproperties:EnableAuditLogging(default: true),AuditLogFailedTransfers(default: true),AuditLogUnauthorizedNotify(default: true),AuditLogHealthCheckQueries(default: true),AuditLogConfigChanges(default: true) - ✅ Implemented audit logging in
XferHandler.cs:- Failed zone transfers: logs rejection reason, source IP, zone name, transfer size/timeout violations
- Unauthorized NOTIFY: logs source IP, zone name, expected upstream IP
- Health check queries: logs query type, query name, source IP
- ✅ Implemented audit logging in
Program.cs:- Configuration changes: logs when configuration is loaded from
appsettings.json
- Configuration changes: logs when configuration is loaded from
- ✅ All audit logs use
[AUDIT]prefix for easy filtering - ✅ Structured logging with consistent format including source IP, zone name, event type
- ✅ Individual flags control specific audit event types
- ✅ Audit logging is separate from hardening (still logs even if
SecurityHardeningEnabled=false) - ✅ Can be disabled entirely via
EnableAuditLogging=false - ✅ All settings configurable via
appsettings.jsonSecurity section - ✅ Default behavior: Audit logging enabled for all event types
Severity: MEDIUM
Location: FilterDns/Cache/ZoneHistoryStorage.cs
Issue: File operations don't check for symlinks. Attacker could create symlinks to overwrite sensitive files.
Code Reference:
// Lines 131-138: No symlink checking
await File.WriteAllBytesAsync(tempFilePath, jsonBytes);
File.Move(tempFilePath, filePath);
// If tempFilePath or filePath is a symlink, could overwrite other filesImpact: Arbitrary file overwrite if attacker can control zone names or data directory
Recommendation:
- Check for symlinks before file operations
- Use
File.GetAttributes()to detect symlinks - Create files with restrictive permissions (600)
- Use atomic operations that prevent symlink races
Severity: LOW
Status: ✅ FIXED (2025-01-27)
Location: FilterDns/Notify/NotifySender.cs
Issue: Uses Random.Shared which is cryptographically weak. Message IDs could be predictable.
Code Reference:
// Line 46: Weak randomness
var messageId = (ushort)Random.Shared.Next(0, 65535);Recommendation: Use RandomNumberGenerator for cryptographically secure random numbers
Implementation Notes:
- ✅ Replaced
Random.Shared.Next(0, 65535)withRandomNumberGenerator.GetInt32(0, 65536)inNotifySender.cs - ✅ Added
using System.Security.Cryptography;import - ✅ Now uses cryptographically secure random number generation for all NOTIFY message IDs
- ✅ No configuration needed - always uses cryptographic RNG (code quality improvement, not hardening feature)
Severity: LOW
Status: ✅ FIXED (2025-01-27)
Location: FilterDns/Xfer/XferHandler.cs
Issue: Zone transfers can take unlimited time, holding connections open indefinitely.
Recommendation: Add configurable timeout (e.g., 10 minutes) for zone transfers
Implementation Notes:
- ✅ Added
ZoneTransferTimeoutSecondstoSecurityConfig(default: 600 seconds = 10 minutes) - ✅ Implemented timeout using
CancellationTokenSource.CreateLinkedTokenSource()with timeout inHandleZoneTransferRequestAsync() - ✅ Timeout applies to both AXFR and IXFR transfers
- ✅ On timeout, transfer is aborted gracefully, connection closed, and audit event logged
- ✅ If
SecurityHardeningEnabled=false, uses very long timeout (effectively no timeout) or no timeout - ✅ Configurable via
appsettings.jsonSecurity section - ✅ Default behavior: Hardening enabled, 10-minute timeout active
The following security features are well-implemented:
- ✅ IP whitelisting for zone transfers
- ✅ Automatic slave IP whitelisting
- ✅ Health check ACL
- ✅ NOTIFY source IP validation
- ✅ Systemd security hardening (NoNewPrivileges, PrivateTmp)
- ✅ Atomic file writes for zone history
- ✅ Input validation on slave configuration
- ✅ Zone transfer validation (SOA start/end)
- Fix DNS message parser buffer overflow and loop vulnerabilities
- Implement proper path traversal protection
- Add connection rate limiting and DoS protection
- Add compression pointer loop detection
- Implement TSIG for NOTIFY authentication
- Restrict health check query responses
- Add input validation on all DNS message fields
- Implement file size limits for zone history
- Run service as non-root user
- Add TLS/DTLS support
- Implement comprehensive audit logging
- Add zone name validation
- Implement DNS-over-TLS for zone transfers
- Add checksums to zone history files
- Implement advanced rate limiting and DDoS protection
- Fuzzing: Use DNS fuzzing tools (e.g., American Fuzzy Lop) to test message parser
- Penetration Testing: Test zone transfer and NOTIFY functionality with malicious inputs
- Load Testing: Test DoS resistance with high connection rates
- Security Scanning: Run static analysis tools (e.g., SonarQube, CodeQL)
While FilterDNS implements basic security controls, several critical vulnerabilities require immediate attention. The DNS message parser is particularly vulnerable to buffer overflows and DoS attacks. Path traversal and lack of rate limiting also pose significant risks. It is recommended to address all Critical and High severity issues before deploying to production.
Report End