From fd6a663c81e16d908a5c4d3eab08360e1c952cfe Mon Sep 17 00:00:00 2001 From: Konrad Kollnig <5175206+kasnder@users.noreply.github.com> Date: Sat, 22 Aug 2026 12:07:10 +0200 Subject: [PATCH] Normalise DNS qnames to lowercase on the database write path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../eu/faircode/netguard/DatabaseHelper.java | 48 ++++++++++++++++--- .../DatabaseHelperDnsAttributionTest.java | 18 +++++++ .../netguard/DatabaseHelperMigrationTest.java | 37 ++++++++++++-- 3 files changed, 93 insertions(+), 10 deletions(-) diff --git a/app/src/main/java/eu/faircode/netguard/DatabaseHelper.java b/app/src/main/java/eu/faircode/netguard/DatabaseHelper.java index e4eb8fea0..a2e0c4d37 100644 --- a/app/src/main/java/eu/faircode/netguard/DatabaseHelper.java +++ b/app/src/main/java/eu/faircode/netguard/DatabaseHelper.java @@ -42,6 +42,7 @@ import java.util.HashMap; import java.util.LinkedHashMap; import java.util.List; +import java.util.Locale; import java.util.Map; import java.util.concurrent.locks.ReentrantReadWriteLock; @@ -84,7 +85,7 @@ public Cursor getHosts() { private static final String TAG = "TrackerControl.Database"; private static final String DB_NAME = "Netguard"; - private static final int DB_VERSION = 22; + private static final int DB_VERSION = 23; private static boolean once = true; private static List logChangedListeners = new ArrayList<>(); @@ -447,6 +448,23 @@ public void onUpgrade(SQLiteDatabase db, int oldVersion, int newVersion) { db.execSQL("ALTER TABLE access ADD COLUMN uncertain INTEGER"); oldVersion = 22; } + if (oldVersion < 23) { + // Remove case-variant duplicates before lowercasing because idx_dns is UNIQUE. + // Keep the freshest row for each DNS identity, breaking time ties by ID. + db.execSQL("DELETE FROM dns WHERE ID NOT IN (" + + "SELECT MAX(d.ID) FROM dns d" + + " JOIN (" + + "SELECT lower(qname) AS qname, lower(aname) AS aname, resource, MAX(time) AS time" + + " FROM dns" + + " GROUP BY lower(qname), lower(aname), resource" + + ") latest ON lower(d.qname) = latest.qname" + + " AND lower(d.aname) = latest.aname" + + " AND d.resource = latest.resource" + + " AND d.time = latest.time" + + " GROUP BY latest.qname, latest.aname, latest.resource)"); + db.execSQL("UPDATE dns SET qname = lower(qname), aname = lower(aname)"); + oldVersion = 23; + } if (oldVersion == DB_VERSION) { db.setVersion(oldVersion); @@ -935,7 +953,7 @@ public Cursor getAccess(int uid) { // There is a segmented index on uid // There is no index on time for write performance String query = "SELECT a.ID AS _id, a.*"; - query += ", (SELECT COUNT(DISTINCT d.qname) FROM dns d WHERE d.resource IN (SELECT d1.resource FROM dns d1 WHERE d1.qname = a.daddr)) count"; + query += ", (SELECT COUNT(DISTINCT d.qname) FROM dns d WHERE d.resource IN (SELECT d1.resource FROM dns d1 WHERE d1.qname = lower(a.daddr))) count"; query += " FROM access a"; query += " WHERE a.uid = ?"; query += " ORDER BY a.time DESC"; @@ -1053,6 +1071,10 @@ public Cursor getRecentTrackerActivity() { // DNS + private static String lower(String name) { + return (name == null ? null : name.toLowerCase(Locale.ROOT)); + } + public boolean insertDns(ResourceRecord rr) { lock.writeLock().lock(); try { @@ -1069,12 +1091,21 @@ public boolean insertDns(ResourceRecord rr) { cv.put("time", rr.Time); cv.put("ttl", ttl * 1000L); + // DNS names are case-insensitive, but the tracker and hosts lookups + // are keyed in lowercase (see TrackerList.findTracker). Storing the + // wire case verbatim made a server-chosen CNAME case — or a resolver + // using 0x20 randomisation — a distinct row under the BINARY-collated + // idx_dns, splitting one domain across several rows and making + // getQAName report it as several qnames sharing an IP. + String qname = lower(rr.QName); + String aname = lower(rr.AName); + int rows = db.update("dns", cv, "qname = ? AND aname = ? AND resource = ?", - new String[] { rr.QName, rr.AName, rr.Resource }); + new String[] { qname, aname, rr.Resource }); if (rows == 0) { - cv.put("qname", rr.QName); - cv.put("aname", rr.AName); + cv.put("qname", qname); + cv.put("aname", aname); cv.put("resource", rr.Resource); if (db.insert("dns", null, cv) == -1) @@ -1199,6 +1230,7 @@ public Cursor getAlternateQNames(String qname) { lock.readLock().lock(); try { SQLiteDatabase db = this.getReadableDatabase(); + qname = lower(qname); String query = "SELECT DISTINCT d2.qname"; query += " FROM dns d1"; query += " JOIN dns d2"; @@ -1216,6 +1248,7 @@ public Cursor getAName(String qname, boolean alive) { lock.readLock().lock(); try { SQLiteDatabase db = this.getReadableDatabase(); + qname = lower(qname); String query = "SELECT d.qname, d.aname, d.time, d.ttl"; query += " FROM dns d"; query += " WHERE d.qname = ?"; @@ -1248,13 +1281,16 @@ public Cursor getAccessDns(String dname) { lock.readLock().lock(); try { SQLiteDatabase db = this.getReadableDatabase(); + // dns.qname is stored lowercase; access.daddr is written from it, so + // the parameter is matched against the indexed column as-is. + dname = lower(dname); // There is a segmented index on dns.qname // There is an index on access.daddr and access.block String query = "SELECT a.uid, a.version, a.protocol, a.daddr, d.resource, a.dport, a.block, d.time, d.ttl"; query += " FROM access AS a"; query += " LEFT JOIN dns AS d"; - query += " ON d.qname = a.daddr"; + query += " ON d.qname = lower(a.daddr)"; query += " WHERE a.block >= 0"; query += " AND (d.time IS NULL OR d.time + d.ttl >= " + now + ")"; if (dname != null) diff --git a/app/src/test/java/eu/faircode/netguard/DatabaseHelperDnsAttributionTest.java b/app/src/test/java/eu/faircode/netguard/DatabaseHelperDnsAttributionTest.java index 2cfbbf13d..a1561e898 100644 --- a/app/src/test/java/eu/faircode/netguard/DatabaseHelperDnsAttributionTest.java +++ b/app/src/test/java/eu/faircode/netguard/DatabaseHelperDnsAttributionTest.java @@ -59,6 +59,24 @@ public void repeatedObservationsOfSameQnameCollapseToFreshestRow() { } } + @Test + public void caseVariantQnamesShareOneRowAndDoNotLookUncertain() { + DatabaseHelper dh = DatabaseHelper.getInstance(RuntimeEnvironment.getApplication()); + dh.clearDns(); + + String ip = "203.0.113.25"; + dh.insertDns(rr(1_000L, "Graph.Facebook.Com", "Alias.Example.Com", ip, 3600)); + dh.insertDns(rr(5_000L, "graph.facebook.com", "alias.example.com", ip, 3600)); + + try (Cursor c = dh.getQAName(-1, ip, false)) { + assertEquals("case variants must share one stored qname", 1, c.getCount()); + assertTrue(c.moveToFirst()); + assertEquals("graph.facebook.com", c.getString(c.getColumnIndexOrThrow("qname"))); + assertEquals("alias.example.com", c.getString(c.getColumnIndexOrThrow("aname"))); + assertEquals(5_000L, c.getLong(c.getColumnIndexOrThrow("time"))); + } + } + @Test public void aliveFilterAppliesBeforeDedup() { DatabaseHelper dh = DatabaseHelper.getInstance(RuntimeEnvironment.getApplication()); diff --git a/app/src/test/java/eu/faircode/netguard/DatabaseHelperMigrationTest.java b/app/src/test/java/eu/faircode/netguard/DatabaseHelperMigrationTest.java index f2f8c13af..320c5fe4b 100644 --- a/app/src/test/java/eu/faircode/netguard/DatabaseHelperMigrationTest.java +++ b/app/src/test/java/eu/faircode/netguard/DatabaseHelperMigrationTest.java @@ -35,14 +35,18 @@ public void upgradeFrom21AddsUncertainWithoutLosingAccessRows() { database.execSQL("ALTER TABLE access ADD COLUMN sent INTEGER"); database.execSQL("ALTER TABLE access ADD COLUMN received INTEGER"); database.execSQL("ALTER TABLE access ADD COLUMN connections INTEGER"); + database.execSQL("CREATE TABLE dns (" + + "ID INTEGER PRIMARY KEY AUTOINCREMENT, time INTEGER NOT NULL, " + + "qname TEXT NOT NULL, aname TEXT NOT NULL, resource TEXT NOT NULL, ttl INTEGER)"); + database.execSQL("CREATE UNIQUE INDEX idx_dns ON dns(qname, aname, resource)"); database.execSQL("INSERT INTO access " + "(uid, version, protocol, daddr, dport, time, allowed, block, sent, received, connections) " + "VALUES (1001, 4, 6, 'tracker.example', 443, 123456, 0, 1, 10, 20, 3)"); database.setVersion(21); - helper.onUpgrade(database, 21, 22); + helper.onUpgrade(database, 21, 23); - assertEquals(22, database.getVersion()); + assertEquals(23, database.getVersion()); assertTrue(columnExists("access", "uncertain")); try (Cursor cursor = database.rawQuery("SELECT * FROM access WHERE uid = 1001", null)) { assertTrue(cursor.moveToFirst()); @@ -70,9 +74,9 @@ public void upgradeFrom16PreservesDataAndBuildsCurrentSchema() { + "VALUES (654321, 'example.org', 'alias.example.org', '1.2.3.4', 60)"); database.setVersion(16); - helper.onUpgrade(database, 16, 22); + helper.onUpgrade(database, 16, 23); - assertEquals(22, database.getVersion()); + assertEquals(23, database.getVersion()); assertTrue(columnExists("access", "sent")); assertTrue(columnExists("access", "received")); assertTrue(columnExists("access", "connections")); @@ -96,6 +100,31 @@ public void upgradeFrom16PreservesDataAndBuildsCurrentSchema() { } } + @Test + public void upgradeFrom22NormalizesAndDeduplicatesDns() { + database.execSQL("CREATE TABLE dns (" + + "ID INTEGER PRIMARY KEY AUTOINCREMENT, time INTEGER NOT NULL, " + + "qname TEXT NOT NULL, aname TEXT NOT NULL, resource TEXT NOT NULL, ttl INTEGER)"); + database.execSQL("CREATE UNIQUE INDEX idx_dns ON dns(qname, aname, resource)"); + database.execSQL("INSERT INTO dns (time, qname, aname, resource, ttl) " + + "VALUES (1000, 'Graph.Facebook.Com', 'Alias.Example.Com', '203.0.113.25', 60)"); + database.execSQL("INSERT INTO dns (time, qname, aname, resource, ttl) " + + "VALUES (5000, 'graph.facebook.com', 'alias.example.com', '203.0.113.25', 60)"); + database.setVersion(22); + + helper.onUpgrade(database, 22, 23); + + assertEquals(23, database.getVersion()); + try (Cursor cursor = database.rawQuery("SELECT qname, aname, resource, time FROM dns", null)) { + assertEquals(1, cursor.getCount()); + assertTrue(cursor.moveToFirst()); + assertEquals("graph.facebook.com", cursor.getString(0)); + assertEquals("alias.example.com", cursor.getString(1)); + assertEquals("203.0.113.25", cursor.getString(2)); + assertEquals(5000L, cursor.getLong(3)); + } + } + private static void createVersion16AccessTable(SQLiteDatabase db) { db.execSQL("CREATE TABLE access (" + "ID INTEGER PRIMARY KEY AUTOINCREMENT, uid INTEGER NOT NULL, "