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..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 @@ -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,39 @@ 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 { + // 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 + } + + /** + * 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..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 @@ -497,6 +497,128 @@ 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 `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")))) + 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"))))