diff --git a/app/src/main/java/eu/faircode/netguard/DatabaseHelper.java b/app/src/main/java/eu/faircode/netguard/DatabaseHelper.java index 64546ae1..cfb1f140 100644 --- a/app/src/main/java/eu/faircode/netguard/DatabaseHelper.java +++ b/app/src/main/java/eu/faircode/netguard/DatabaseHelper.java @@ -1189,7 +1189,18 @@ public String getQName(int uid, String ip) { } } - public Cursor getQAName(int uid, String ip, boolean alive) { + /** + * DNS evidence for an IP, freshest qname first, one row per qname. + * + *

Expired rows are always excluded. Both callers — the runtime + * blocking decision in {@code blockKnownTracker()} and the UI + * classification in {@code ServiceSinkhole.log()} — must answer "is this + * IP shared, and whose is it?" from the same row set. When the UI alone + * saw the full history (see issue #759), it drew the shared-IP marker and + * the ALLOWED/BLOCKED text from evidence the blocker had already + * discarded, so the log contradicted what actually happened. + */ + public Cursor getQAName(int uid, String ip) { long now = new Date().getTime(); lock.readLock().lock(); try { @@ -1197,9 +1208,7 @@ public Cursor getQAName(int uid, String ip, boolean alive) { readableDb = this.getReadableDatabase(); SQLiteDatabase db = readableDb; String escapedIp = ip.replace("'", "''"); - String aliveFilter = alive - ? " AND (d.time IS NULL OR d.time + d.ttl >= " + now + ")" - : ""; + String aliveFilter = " AND (d.time IS NULL OR d.time + d.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 diff --git a/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java b/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java index b6b7c723..2309cb5e 100644 --- a/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java +++ b/app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java @@ -999,7 +999,7 @@ private void log(Packet packet, int connection, boolean interactive) { int uncertain = DatabaseHelper.ACCESS_UNCERTAIN_NONE; boolean isTracker = false; - try (Cursor lookup = dh.getQAName(packet.uid, packet.daddr, false)) { + try (Cursor lookup = dh.getQAName(packet.uid, packet.daddr)) { uncertain = (lookup != null && lookup.getCount() > 1) ? DatabaseHelper.ACCESS_UNCERTAIN_SHARED_IP : DatabaseHelper.ACCESS_UNCERTAIN_NONE; @@ -2697,7 +2697,7 @@ private boolean blockKnownTracker(String daddr, int uid) { boolean sawFirstRow = false; boolean sawTrackerEvidence = false; boolean sawNonTrackerEvidence = false; - try (Cursor lookup = dh.getQAName(uid, daddr, true)) { + try (Cursor lookup = dh.getQAName(uid, daddr)) { // Loop through all fresh DNS candidates for this IP and only fail closed // when ambiguous tracker blocking is enabled or the evidence is tracker-only. if (lookup != null) { diff --git a/app/src/test/java/eu/faircode/netguard/DatabaseHelperDnsAttributionTest.java b/app/src/test/java/eu/faircode/netguard/DatabaseHelperDnsAttributionTest.java index a1561e89..9a01302f 100644 --- a/app/src/test/java/eu/faircode/netguard/DatabaseHelperDnsAttributionTest.java +++ b/app/src/test/java/eu/faircode/netguard/DatabaseHelperDnsAttributionTest.java @@ -20,6 +20,8 @@ @RunWith(RobolectricTestRunner.class) public class DatabaseHelperDnsAttributionTest { + private static final long NOW = System.currentTimeMillis(); + @Test public void mostRecentlyObservedQnameIsReturnedFirst() { DatabaseHelper dh = DatabaseHelper.getInstance(RuntimeEnvironment.getApplication()); @@ -27,10 +29,10 @@ public void mostRecentlyObservedQnameIsReturnedFirst() { 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)); + dh.insertDns(rr(NOW - 2_000L, "aaa-old.example.com", "aaa-old.example.com", ip, 3600)); + dh.insertDns(rr(NOW - 1_000L, "zzz-new.example.com", "zzz-new.example.com", ip, 3600)); - try (Cursor c = dh.getQAName(-1, ip, false)) { + try (Cursor c = dh.getQAName(-1, ip)) { assertEquals(2, c.getCount()); assertTrue(c.moveToFirst()); assertEquals("zzz-new.example.com", c.getString(c.getColumnIndexOrThrow("qname"))); @@ -46,16 +48,16 @@ public void repeatedObservationsOfSameQnameCollapseToFreshestRow() { 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)); + dh.insertDns(rr(NOW - 5_000L, "tracker.example.com", "old-cname.example.com", ip, 3600)); + dh.insertDns(rr(NOW - 1_000L, "tracker.example.com", "new-cname.example.com", ip, 3600)); - try (Cursor c = dh.getQAName(-1, ip, false)) { + try (Cursor c = dh.getQAName(-1, ip)) { 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"))); + assertEquals(NOW - 1_000L, c.getLong(c.getColumnIndexOrThrow("time"))); } } @@ -65,15 +67,15 @@ public void caseVariantQnamesShareOneRowAndDoNotLookUncertain() { 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)); + dh.insertDns(rr(NOW - 5_000L, "Graph.Facebook.Com", "Alias.Example.Com", ip, 3600)); + dh.insertDns(rr(NOW - 1_000L, "graph.facebook.com", "alias.example.com", ip, 3600)); - try (Cursor c = dh.getQAName(-1, ip, false)) { + try (Cursor c = dh.getQAName(-1, ip)) { 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"))); + assertEquals(NOW - 1_000L, c.getLong(c.getColumnIndexOrThrow("time"))); } } @@ -84,7 +86,7 @@ public void aliveFilterAppliesBeforeDedup() { String ip = "203.0.113.30"; // The freshest observation has already expired; an older one is still - // alive. With alive=true the expired row must not shadow the alive one. + // alive. The expired row must not shadow the alive one. // Rows are inserted directly because insertDns() clamps the TTL to the // "ttl" preference floor, which would keep the fresh row alive. long now = System.currentTimeMillis(); @@ -97,13 +99,45 @@ public void aliveFilterAppliesBeforeDedup() { + (now - 5_000) + ", 't2.example.com', 'expired-cname.example.com', '" + ip + "', 1000)"); - try (Cursor c = dh.getQAName(-1, ip, true)) { + try (Cursor c = dh.getQAName(-1, ip)) { assertEquals(1, c.getCount()); assertTrue(c.moveToFirst()); assertEquals("alive-cname.example.com", c.getString(c.getColumnIndexOrThrow("aname"))); } } + /** + * Issue #759: an expired qname must not make an IP look shared. The UI + * derived its shared-IP marker from a row set that still contained + * expired evidence while the blocker had already dropped it, so the log + * flagged an ambiguity the blocking decision never saw. + */ + @Test + public void expiredQnameDoesNotMakeIpLookShared() { + DatabaseHelper dh = DatabaseHelper.getInstance(RuntimeEnvironment.getApplication()); + dh.clearDns(); + + String ip = "203.0.113.40"; + // Two different qnames on one IP, but only one is still alive. Rows are + // inserted directly because insertDns() clamps the TTL to the "ttl" + // preference floor, which would keep the expired one alive. + long now = System.currentTimeMillis(); + dh.getWritableDatabase().execSQL( + "INSERT INTO dns (time, qname, aname, resource, ttl) VALUES (" + + (now - 10_000) + ", 'alive.example.com', 'alive.example.com', '" + + ip + "', 3600000)"); + dh.getWritableDatabase().execSQL( + "INSERT INTO dns (time, qname, aname, resource, ttl) VALUES (" + + (now - 5_000) + ", 'expired.example.com', 'expired.example.com', '" + + ip + "', 1000)"); + + try (Cursor c = dh.getQAName(-1, ip)) { + assertEquals("an expired qname must not count towards shared-IP", 1, c.getCount()); + assertTrue(c.moveToFirst()); + assertEquals("alive.example.com", c.getString(c.getColumnIndexOrThrow("qname"))); + } + } + private static ResourceRecord rr(long time, String qname, String aname, String resource, int ttl) { ResourceRecord rr = new ResourceRecord(); rr.Time = time;