From 440f5495a4390d646ca29d740a1e5f6a11ce73b7 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 16 May 2026 13:17:08 +0000 Subject: [PATCH 1/2] fix(dns): reject loopback poison + dirty cache on restore drop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SurgeDns was faithfully caching whatever the system resolver returned — including 127.0.0.1 / ::1 / 0.0.0.0 — for 24-48h plus an on-disk snapshot. A single bad answer (captive portal, ad-blocker DNS, transient VPN hiccup) could leave the user unable to reach any non-loopback relay for days: connection attempts go to their own loopback, fail, and the next lookup re-resolves through the same source. - Filter loopback/any-local addresses out of lookupAndCache and restore, unless the hostname is itself a loopback name (localhost, .localhost subdomains, or 127.0.0.1 / ::1 literals) so user-configured local relays keep working. - Mark cache dirty when restore drops poisoned on-disk entries so the next save rewrites the snapshot without them — otherwise the bad entries would be re-restored on every cold start. --- .../amethyst/service/okhttp/SurgeDns.kt | 66 ++++++++-- .../amethyst/service/okhttp/SurgeDnsTest.kt | 113 ++++++++++++++++++ 2 files changed, 172 insertions(+), 7 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/okhttp/SurgeDns.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/okhttp/SurgeDns.kt index 50bf1449f2..5e442efee4 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/okhttp/SurgeDns.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/okhttp/SurgeDns.kt @@ -139,7 +139,15 @@ class SurgeDns( private fun lookupAndCache(host: String): List = try { - val addresses = delegate.lookup(host).ifEmpty { throw UnknownHostException(host) } + val raw = delegate.lookup(host).ifEmpty { throw UnknownHostException(host) } + val addresses = + if (isLoopbackHostname(host)) { + raw + } else { + raw + .filterNot { it.isLoopbackAddress || it.isAnyLocalAddress } + .ifEmpty { throw UnknownHostException(host) } + } putPositive(host, addresses) addresses } catch (e: UnknownHostException) { @@ -233,22 +241,35 @@ class SurgeDns( /** * Restore from a previously taken snapshot. Entries already expired on disk are dropped. * Existing in-memory entries are NOT overwritten (so a fresh lookup that completes before - * restore lands keeps its newer answer). + * restore lands keeps its newer answer). Loopback / any-local addresses (127/8, ::1, 0.0.0.0) + * are filtered out for non-loopback hostnames — they're poison left over from a captive + * portal, ad-blocker DNS, or VPN hiccup at snapshot time. When such addresses are dropped + * the cache is marked dirty so the next persist re-writes the snapshot without them. */ fun restore(records: List) { val now = System.currentTimeMillis() + var droppedPoisoned = false for (record in records) { if (record.expiresAtMillis <= now) continue - val addresses = - record.addresses.mapNotNull { literal -> - // getByName on a numeric literal parses without doing DNS. - runCatching { InetAddress.getByName(literal) }.getOrNull() + val key = record.hostname.lowercase(Locale.ROOT) + val allowLoopback = isLoopbackHostname(key) + val addresses = ArrayList(record.addresses.size) + for (literal in record.addresses) { + // getByName on a numeric literal parses without doing DNS. + val addr = runCatching { InetAddress.getByName(literal) }.getOrNull() ?: continue + if (!allowLoopback && (addr.isLoopbackAddress || addr.isAnyLocalAddress)) { + droppedPoisoned = true + continue } + addresses += addr + } if (addresses.isNotEmpty()) { - val key = record.hostname.lowercase(Locale.ROOT) cache.putIfAbsent(key, Entry(addresses, record.expiresAtMillis)) } } + // Dirty the cache so the next save rewrites the snapshot without the poisoned entries + // — otherwise they'd be re-restored on every cold start. + if (droppedPoisoned) dirty.set(true) } /** True if the cache has changed since the last [tryClearDirty] / [markDirty] / construction. */ @@ -267,6 +288,37 @@ class SurgeDns( dirty.set(true) } + /** + * True if [host] is itself a loopback identifier — the user-configured `localhost`-style relay + * case where 127.0.0.1 / ::1 in the answer is legitimate and must not be filtered as poison. + * Accepts the DNS name `localhost` and any `.localhost` subdomain (RFC 6761), and IP literals + * that parse to a loopback or any-local address (127.0.0.0/8, ::1, 0.0.0.0). [host] is assumed + * already lowercased. + */ + private fun isLoopbackHostname(host: String): Boolean { + if (host == "localhost" || host.endsWith(".localhost")) return true + val literal = parseIpLiteral(host) ?: return false + return literal.isLoopbackAddress || literal.isAnyLocalAddress + } + + /** + * Parses [host] as a numeric IP literal without ever triggering a DNS lookup. Returns null for + * anything that doesn't structurally look like an IPv4 or IPv6 literal — we must not hand a + * plain hostname to [InetAddress.getByName] from inside the resolver. + */ + private fun parseIpLiteral(host: String): InetAddress? { + val candidate = + if (host.startsWith("[") && host.endsWith("]")) { + host.substring(1, host.length - 1) + } else { + host + } + val looksLikeIpv4 = candidate.isNotEmpty() && candidate.all { it.isDigit() || it == '.' } && candidate.contains('.') + val looksLikeIpv6 = candidate.contains(':') + if (!looksLikeIpv4 && !looksLikeIpv6) return null + return runCatching { InetAddress.getByName(candidate) }.getOrNull() + } + private class Entry( val addresses: List, val expiresAtMillis: Long, diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/okhttp/SurgeDnsTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/okhttp/SurgeDnsTest.kt index 92384ab578..692116756a 100644 --- a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/okhttp/SurgeDnsTest.kt +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/okhttp/SurgeDnsTest.kt @@ -497,6 +497,119 @@ class SurgeDnsTest { assertThrows(UnknownHostException::class.java) { dns.lookup("a.example") } } + @Test + fun `loopback address from upstream is rejected for non-loopback host`() { + val upstream = CountingDns(mapOf("relay.example" to listOf(ip("127.0.0.1")))) + val dns = SurgeDns(delegate = upstream) + + assertThrows(UnknownHostException::class.java) { dns.lookup("relay.example") } + assertFalse("Rejected answer must not be cached as positive", dns.isDirty()) + } + + @Test + fun `any-local address from upstream is rejected for non-loopback host`() { + val upstream = CountingDns(mapOf("relay.example" to listOf(ip("0.0.0.0")))) + val dns = SurgeDns(delegate = upstream) + + assertThrows(UnknownHostException::class.java) { dns.lookup("relay.example") } + } + + @Test + fun `loopback addresses are stripped but routable addresses are kept`() { + val upstream = + CountingDns(mapOf("relay.example" to listOf(ip("127.0.0.1"), ip("5.6.7.8")))) + val dns = SurgeDns(delegate = upstream) + + val result = dns.lookup("relay.example") + assertEquals(listOf(ip("5.6.7.8")), result) + } + + @Test + fun `localhost hostname keeps loopback answers`() { + val upstream = CountingDns(mapOf("localhost" to listOf(ip("127.0.0.1")))) + val dns = SurgeDns(delegate = upstream) + + assertEquals(listOf(ip("127.0.0.1")), dns.lookup("localhost")) + } + + @Test + fun `dot-localhost subdomain keeps loopback answers`() { + val upstream = CountingDns(mapOf("relay.localhost" to listOf(ip("127.0.0.1")))) + val dns = SurgeDns(delegate = upstream) + + assertEquals(listOf(ip("127.0.0.1")), dns.lookup("relay.localhost")) + } + + @Test + fun `ipv4 loopback literal keeps loopback answers`() { + val upstream = CountingDns(mapOf("127.0.0.1" to listOf(ip("127.0.0.1")))) + val dns = SurgeDns(delegate = upstream) + + assertEquals(listOf(ip("127.0.0.1")), dns.lookup("127.0.0.1")) + } + + @Test + fun `ipv6 loopback literal keeps loopback answers`() { + val upstream = CountingDns(mapOf("::1" to listOf(ip("::1")))) + val dns = SurgeDns(delegate = upstream) + + assertEquals(listOf(ip("::1")), dns.lookup("::1")) + } + + @Test + fun `restore filters loopback poison and marks cache dirty`() { + val upstream = CountingDns(mapOf("relay.example" to listOf(ip("5.6.7.8")))) + val dns = SurgeDns(delegate = upstream) + + val expiresAt = System.currentTimeMillis() + 60_000 + dns.restore(listOf(DnsCacheRecord("relay.example", listOf("127.0.0.1"), expiresAt))) + + assertTrue("Dropping poison must dirty the cache so the snapshot is rewritten", dns.isDirty()) + // Cache should not hold the poisoned entry, so the next lookup hits upstream. + assertEquals(listOf(ip("5.6.7.8")), dns.lookup("relay.example")) + assertEquals(1, upstream.calls("relay.example")) + } + + @Test + fun `restore keeps non-loopback addresses when poison is mixed in`() { + val upstream = CountingDns(mapOf("relay.example" to listOf(ip("9.9.9.9")))) + val dns = SurgeDns(delegate = upstream) + + val expiresAt = System.currentTimeMillis() + 60_000 + dns.restore( + listOf( + DnsCacheRecord("relay.example", listOf("127.0.0.1", "5.6.7.8"), expiresAt), + ), + ) + + assertTrue("Dropping one poisoned address still dirties the cache", dns.isDirty()) + assertEquals(listOf(ip("5.6.7.8")), dns.lookup("relay.example")) + assertEquals("Restored survivors should serve without upstream", 0, upstream.calls("relay.example")) + } + + @Test + fun `restore keeps loopback for localhost host`() { + val upstream = CountingDns(emptyMap()) + val dns = SurgeDns(delegate = upstream) + + val expiresAt = System.currentTimeMillis() + 60_000 + dns.restore(listOf(DnsCacheRecord("localhost", listOf("127.0.0.1"), expiresAt))) + + assertFalse("Loopback entry for localhost is legitimate, not poison", dns.isDirty()) + assertEquals(listOf(ip("127.0.0.1")), dns.lookup("localhost")) + } + + @Test + fun `restore of clean snapshot does not dirty the cache`() { + val upstream = CountingDns(emptyMap()) + val dns = SurgeDns(delegate = upstream) + + val expiresAt = System.currentTimeMillis() + 60_000 + dns.restore(listOf(DnsCacheRecord("relay.example", listOf("5.6.7.8"), expiresAt))) + + assertFalse("Clean restore must not dirty the cache", dns.isDirty()) + } + @Test fun `refresh executor rejection cleans up the inflight slot`() { val upstream = CountingDns(mapOf("a.example" to listOf(ip("1.2.3.4")))) From c36c9ccf43f62cbe6ee6902b499142ea7e7518cd Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 16 May 2026 13:37:02 +0000 Subject: [PATCH 2/2] fix(dns): treat trailing-dot FQDN form of localhost as loopback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RFC 1034: `localhost.` and `localhost` are the same name — the trailing dot just marks the FQDN form. Without this, an upstream answer of 127.0.0.1 for `localhost.` (or `relay.localhost.`) would get filtered as poison, breaking user-configured local relays addressed in FQDN form. --- .../vitorpamplona/amethyst/service/okhttp/SurgeDns.kt | 6 ++++-- .../amethyst/service/okhttp/SurgeDnsTest.kt | 9 +++++++++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/okhttp/SurgeDns.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/okhttp/SurgeDns.kt index 5e442efee4..126c3587e1 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/okhttp/SurgeDns.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/okhttp/SurgeDns.kt @@ -296,8 +296,10 @@ class SurgeDns( * already lowercased. */ private fun isLoopbackHostname(host: String): Boolean { - if (host == "localhost" || host.endsWith(".localhost")) return true - val literal = parseIpLiteral(host) ?: return false + // RFC 1034: a trailing dot is the FQDN form (e.g. `localhost.`), still the same name. + val name = host.trimEnd('.') + if (name == "localhost" || name.endsWith(".localhost")) return true + val literal = parseIpLiteral(name) ?: return false return literal.isLoopbackAddress || literal.isAnyLocalAddress } diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/okhttp/SurgeDnsTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/okhttp/SurgeDnsTest.kt index 692116756a..d2578126b9 100644 --- a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/okhttp/SurgeDnsTest.kt +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/okhttp/SurgeDnsTest.kt @@ -540,6 +540,15 @@ class SurgeDnsTest { assertEquals(listOf(ip("127.0.0.1")), dns.lookup("relay.localhost")) } + @Test + fun `trailing-dot FQDN form of localhost keeps loopback answers`() { + // RFC 1034: `localhost.` is the same name as `localhost`, just in FQDN form. + val upstream = CountingDns(mapOf("localhost." to listOf(ip("127.0.0.1")))) + val dns = SurgeDns(delegate = upstream) + + assertEquals(listOf(ip("127.0.0.1")), dns.lookup("localhost.")) + } + @Test fun `ipv4 loopback literal keeps loopback answers`() { val upstream = CountingDns(mapOf("127.0.0.1" to listOf(ip("127.0.0.1"))))