From 2b1cfe119d36e6b98a36134cc07449fa8a818e10 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 07:47:05 +0000 Subject: [PATCH] Attribute shared-IP DNS evidence by recency, not alphabetically (#655) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getQAName() previously deduped and ordered candidate qnames for a shared IP alphabetically, so blockKnownTracker() and the traffic-log attribution in ServiceSinkhole.log() always locked onto whichever qname sorted first lexically, regardless of when it was actually resolved. Order by most recent observation instead, so the freshest DNS evidence — the one most likely to correspond to the connection actually being made — wins. --- .../eu/faircode/netguard/DatabaseHelper.java | 30 +++++--- .../DatabaseHelperDnsAttributionTest.java | 71 +++++++++++++++++++ 2 files changed, 93 insertions(+), 8 deletions(-) create mode 100644 app/src/test/java/eu/faircode/netguard/DatabaseHelperDnsAttributionTest.java diff --git a/app/src/main/java/eu/faircode/netguard/DatabaseHelper.java b/app/src/main/java/eu/faircode/netguard/DatabaseHelper.java index b76622fa..5623fad8 100644 --- a/app/src/main/java/eu/faircode/netguard/DatabaseHelper.java +++ b/app/src/main/java/eu/faircode/netguard/DatabaseHelper.java @@ -1164,14 +1164,28 @@ public Cursor getQAName(int uid, String ip, boolean alive) { if (readableDb == null) readableDb = this.getReadableDatabase(); SQLiteDatabase db = readableDb; - // There is a segmented index on resource - String query = "SELECT d.qname, d.aname, d.time, d.ttl"; - query += " FROM dns AS d"; - query += " WHERE d.resource = '" + ip.replace("'", "''") + "'"; - if (alive) - query += " AND (d.time IS NULL OR d.time + d.ttl >= " + now + ")"; - query += " GROUP BY d.qname"; // remove duplicates - query += " ORDER BY d.qname"; + String escapedIp = ip.replace("'", "''"); + String aliveFilter = alive + ? " AND (%1$s.time IS NULL OR %1$s.time + %1$s.ttl >= " + now + ")" + : ""; + // There is a segmented index on resource. A shared IP can carry + // DNS evidence for several qnames; keep only the most recently + // observed row per qname (dedup) and order qnames by recency, so + // the freshest resolution — most likely tied to the connection + // that's actually being made now — is attributed first instead + // of an alphabetically-first but possibly stale one. + String query = "SELECT d.qname, d.aname, d.time, d.ttl" + + " FROM dns AS d" + + " WHERE d.resource = '" + escapedIp + "'" + + String.format(aliveFilter, "d") + + " AND d.ID = (" + + " SELECT d2.ID FROM dns AS d2" + + " WHERE d2.resource = d.resource AND d2.qname = d.qname" + + String.format(aliveFilter, "d2") + + " ORDER BY d2.time DESC, d2.ID DESC" + + " LIMIT 1" + + " )" + + " ORDER BY d.time DESC, d.ID DESC"; return db.rawQuery(query, new String[] {}); } finally { lock.readLock().unlock(); diff --git a/app/src/test/java/eu/faircode/netguard/DatabaseHelperDnsAttributionTest.java b/app/src/test/java/eu/faircode/netguard/DatabaseHelperDnsAttributionTest.java new file mode 100644 index 00000000..e7878f97 --- /dev/null +++ b/app/src/test/java/eu/faircode/netguard/DatabaseHelperDnsAttributionTest.java @@ -0,0 +1,71 @@ +package eu.faircode.netguard; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +import android.database.Cursor; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.robolectric.RobolectricTestRunner; +import org.robolectric.RuntimeEnvironment; + +/** + * Covers getQAName's ordering: when a shared IP carries DNS evidence for + * several qnames (see issue #655), the most recently observed qname should + * be attributed first rather than the alphabetically-first one, and a qname + * with several observed rows (e.g. distinct CNAME targets) should collapse + * to its single freshest row. + */ +@RunWith(RobolectricTestRunner.class) +public class DatabaseHelperDnsAttributionTest { + + @Test + public void mostRecentlyObservedQnameIsReturnedFirst() { + DatabaseHelper dh = DatabaseHelper.getInstance(RuntimeEnvironment.getApplication()); + dh.clearDns(); + + String ip = "203.0.113.10"; + // Alphabetically "aaa..." would sort first, but "zzz..." was resolved later. + dh.insertDns(rr(1_000L, "aaa-old.example.com", "aaa-old.example.com", ip, 3600)); + dh.insertDns(rr(2_000L, "zzz-new.example.com", "zzz-new.example.com", ip, 3600)); + + try (Cursor c = dh.getQAName(-1, ip, false)) { + assertEquals(2, c.getCount()); + assertTrue(c.moveToFirst()); + assertEquals("zzz-new.example.com", c.getString(c.getColumnIndexOrThrow("qname"))); + assertTrue(c.moveToNext()); + assertEquals("aaa-old.example.com", c.getString(c.getColumnIndexOrThrow("qname"))); + } + } + + @Test + public void repeatedObservationsOfSameQnameCollapseToFreshestRow() { + DatabaseHelper dh = DatabaseHelper.getInstance(RuntimeEnvironment.getApplication()); + dh.clearDns(); + + String ip = "203.0.113.20"; + // Same qname, two distinct CNAME targets observed at different times. + dh.insertDns(rr(1_000L, "tracker.example.com", "old-cname.example.com", ip, 3600)); + dh.insertDns(rr(5_000L, "tracker.example.com", "new-cname.example.com", ip, 3600)); + + try (Cursor c = dh.getQAName(-1, ip, false)) { + assertEquals("duplicate rows for the same qname must collapse to one", 1, c.getCount()); + assertTrue(c.moveToFirst()); + assertEquals("tracker.example.com", c.getString(c.getColumnIndexOrThrow("qname"))); + assertEquals("the freshest row's aname should win", + "new-cname.example.com", c.getString(c.getColumnIndexOrThrow("aname"))); + assertEquals(5_000L, c.getLong(c.getColumnIndexOrThrow("time"))); + } + } + + private static ResourceRecord rr(long time, String qname, String aname, String resource, int ttl) { + ResourceRecord rr = new ResourceRecord(); + rr.Time = time; + rr.QName = qname; + rr.AName = aname; + rr.Resource = resource; + rr.TTL = ttl; + return rr; + } +}