Normalise DNS qnames to lowercase on the database write path - #772
Merged
Conversation
DNS names are case-insensitive, but insertDns() stored qname/aname with their on-the-wire case while matching on the exact (qname, aname, resource) triple. Since idx_dns is BINARY-collated, a server-chosen CNAME case — or a resolver using 0x20 randomisation — produced several distinct rows for one domain. getQAName() groups by qname, so ServiceSinkhole.log() then read count > 1 and marked the access row ACCESS_UNCERTAIN_SHARED_IP, showing the user a spurious shared-IP marker on an unambiguous domain. The lookup side already normalised (TrackerList.findTracker, hosts keys), so only the write side disagreed. Lowercase qname/aname in insertDns() and in the qname parameters of getAName/getAlternateQNames/getAccessDns, and compare dns.qname against lower(access.daddr) in the joins so pre-existing mixed-case access rows still match. The WHERE clause keeps the plain a.daddr = ? comparison so idx_access_daddr is still used — prepareUidIPFilters runs on every new DNS record. DB_VERSION 23 deduplicates and lowercases existing dns rows, keeping the freshest row per (lower(qname), lower(aname), resource); the dedup has to run before the UPDATE because idx_dns is UNIQUE. Fixes #756 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The defect
DNS names are case-insensitive, but the two halves of the DNS store disagreed about it.
DatabaseHelper.insertDns()(app/src/main/java/eu/faircode/netguard/DatabaseHelper.java:1072-1078on master) UPDATEd on the exact(qname, aname, resource)string triple and INSERTedrr.QName/rr.ANameverbatim. The schema'sCREATE UNIQUE INDEX idx_dns ON dns(qname, aname, resource)(DatabaseHelper.java:294, recreated in the v18 migration at:429) is BINARY-collated, so case variants of one domain became distinct rows.TrackerList.findTrackerlowercases (app/src/main/java/net/kollnig/missioncontrol/data/TrackerList.java:121) andServiceSinkhole.prepareHostsBlockedlowercases hosts.txt keys (ServiceSinkhole.java:2167).getQANamedoesGROUP BY d.qname(DatabaseHelper.java:1185-1189), so those variants counted as several distinct qnames for one IP.User-visible symptom (default configuration)
ServiceSinkhole.log()computesuncertain = getQAName(...).getCount() > 1 ? ACCESS_UNCERTAIN_SHARED_IP : NONE(ServiceSinkhole.java:960-968), andlog_appdefaults totrue, sodh.updateAccess(packet, dname, -1, uncertain)runs (:1035). A perfectly unambiguous domain therefore picked up the spurious "shared IP / uncertain" marker in the UI, and fed the mixed-evidence classification that decides whether an IP counts as an ambiguous shared host in Standard mode. The realistic trigger is server-chosen CNAME case inaname, rather than DNS 0x20 query-case randomisation.The issue text points at
get_qnameinapp/src/main/jni/netguard/dns.c; that reference is stale — DNS parsing now lives in Rust (wgbridge-rs/tc-dns/src/message.rs). Preserving wire case in the parser is correct and is unchanged here. The fix is purely on the Java write path.What changed
app/src/main/java/eu/faircode/netguard/DatabaseHelper.java:insertDns()lowercasesqname/anamewithLocale.ROOTbefore both the UPDATE match and the INSERT, via a null-safelower()helper (the columns areNOT NULL, so a null still fails as a caught SQLite error rather than an NPE thrown out of the nativednsResolvedcallback).resourceis an IP literal and is left alone.getAName(),getAlternateQNames()andgetAccessDns()are lowercased to match the now-lowercase column —dnsResolved()callsprepareUidIPFilters(rr.QName)with the wire-case name.dns.qnameagainstaccess.daddr(getAccess's shared-IP count subquery andgetAccessDns's LEFT JOIN) compare againstlower(a.daddr), so access rows written before this change still resolve. TheWHEREclause deliberately keeps the plaina.daddr = ?comparison soidx_access_daddris still usable —prepareUidIPFiltersruns on every new DNS record, andlower()there would have turned that into a full scan of a table that is never pruned. Battery is a first-class constraint in this repo.Migration: yes,
DB_VERSION22 -> 23. Thednstable is TTL-expiring, so duplicates would eventually age out on their own — butttldefaults to a 259200 s (3 day) floor and users refresh the same domains constantly, so "eventually" can mean the false uncertainty marker sticks around for days on exactly the domains a user looks at most. The migration deletes case-variant duplicates first (keeping the freshest row per(lower(qname), lower(aname), resource), ties broken byMAX(ID)) and only then runsUPDATE dns SET qname = lower(qname), aname = lower(aname); the dedup must precede the update becauseidx_dnsis UNIQUE and a naive lowercase would hit a constraint violation where a lowercase twin already exists. SQLite'slower()is ASCII-only, which is exactly right for DNS names. It is a one-off, two-statement pass over a small table.Test evidence
DatabaseHelperDnsAttributionTest.caseVariantQnamesShareOneRowAndDoNotLookUncertain— insertsGraph.Facebook.Com/Alias.Example.Comandgraph.facebook.com/alias.example.comfor the same IP, then assertsgetQANamereturns one row (not two, which is what drove the falseACCESS_UNCERTAIN_SHARED_IP), with the lowercase names and the freshest timestamp.DatabaseHelperMigrationTest.upgradeFrom22NormalizesAndDeduplicatesDns— seeds a v22dnstable with the same two case variants, runsonUpgrade(22, 23), asserts a single lowercase row survives carrying the newertime. The two existing migration tests were extended to target 23.Reverting only
DatabaseHelper.javaand re-running these tests fails withcase variants must share one stored qname expected:<1> but was:<2>, confirming the test reproduces the reported defect.Deliberately out of scope
access.daddris not normalised or deduplicated. A domain observed in mixed case before this change can leave a stale access row alongside the new lowercase one; the joins above still resolve it, and normalisingaccessmeans a second dedup againstUNIQUE INDEX idx_access(uid, version, protocol, daddr, dport)for a cosmetic legacy artefact.Fixes #756