diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavHttp.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavHttp.kt index ba520c5..8426bd6 100644 --- a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavHttp.kt +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavHttp.kt @@ -1,7 +1,6 @@ package de.jeanlucmakiola.caldav import at.bitfire.dav4jvm.BasicDigestAuthHandler -import at.bitfire.dav4jvm.UrlUtils import okhttp3.HttpUrl import okhttp3.Interceptor import okhttp3.OkHttpClient @@ -63,12 +62,24 @@ object CalDavHttp { origin: HttpUrl, ): OkHttpClient { val handler = BasicDigestAuthHandler( - // ⚠️ The **registrable** domain, not the host. The handler compares - // its `domain` against UrlUtils.hostToDomain(request host), which - // keeps only the last two labels — so passing "cloud.example.com" - // compares it to "example.com", never matches, and the credential is - // withheld from every request. That is every self-hosted Nextcloud. - domain = UrlUtils.hostToDomain(origin.host), + // ⚠️ The **registrable** domain, not the host, and it must be + // derived exactly as the handler derives it for each request — pass + // "cloud.example.com" where the handler computes "example.com" and + // the credential is withheld from every request. That is every + // self-hosted Nextcloud. + // + // Public-suffix list, not a last-two-labels split: the split scopes + // cloud.example.co.uk to co.uk and 192.168.1.10 to 1.10, offering + // the password preemptively to strangers. `topPrivateDomain()` is + // null for an IP literal or a single-label host, where the exact + // host is the only safe scope. + // + // What this still trusts: two hosts under one registrable domain + // have one owner. That is what a cross-host `calendar-home-set` + // needs — iCloud answers on caldav.icloud.com and serves from + // pNN-caldav.icloud.com — so a breached dav.example.com can still + // claim evil.example.com. Narrowing further costs iCloud. + domain = origin.topPrivateDomain() ?: origin.host, username = username, password = password, // Never preemptively over cleartext. The handler already gates its diff --git a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalDavHttpTest.kt b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalDavHttpTest.kt index 8e4d29d..53b187a 100644 --- a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalDavHttpTest.kt +++ b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalDavHttpTest.kt @@ -12,9 +12,8 @@ class CalDavHttpTest { @Test fun `the auth handler is scoped to the registrable domain, not the host`() { - // ⚠️ The handler compares its `domain` against - // UrlUtils.hostToDomain(request host), which keeps only the last two - // labels. Passing the full host means the comparison is + // ⚠️ The handler derives the registrable domain of each request host and + // compares. Passing the full host means the comparison is // "cloud.example.com" == "example.com" — never true — and the credential // is withheld from every single request. That is every self-hosted // Nextcloud, silently answering 401 forever. @@ -54,6 +53,98 @@ class CalDavHttpTest { assertThat(authorised?.header("Authorization")).isNotNull() } + @Test + fun `a multi-label public suffix is not mistaken for a domain`() { + // ⚠️ A last-two-labels split scopes this to "co.uk" and then offers the + // password preemptively to any host under it — one an attacker can buy a + // certificate for. This is the headline case. + val client = CalDavHttp.authenticated( + "Agendula", + "user", + "pw", + "https://cloud.example.co.uk/dav/".toHttpUrl(), + ) + val handler = client.networkInterceptors.filterIsInstance().single() + assertThat(handler.domain).isEqualTo("example.co.uk") + + val stranger = handler.authenticateRequest( + Request.Builder().url("https://attacker.co.uk/dav/").build(), + null, + ) + assertThat(stranger).isNull() + + val ours = handler.authenticateRequest( + Request.Builder().url("https://cloud.example.co.uk/dav/").build(), + null, + ) + assertThat(ours?.header("Authorization")).isNotNull() + } + + @Test + fun `a free-subdomain host does not trust its neighbours`() { + // The PSL private section covers the dynamic-DNS providers self-hosters + // actually use, where the neighbour is a stranger with a free account. + val client = CalDavHttp.authenticated( + "Agendula", + "user", + "pw", + "https://myhome.duckdns.org/dav/".toHttpUrl(), + ) + val handler = client.networkInterceptors.filterIsInstance().single() + assertThat(handler.domain).isEqualTo("myhome.duckdns.org") + + val neighbour = handler.authenticateRequest( + Request.Builder().url("https://evil.duckdns.org/dav/").build(), + null, + ) + assertThat(neighbour).isNull() + } + + @Test + fun `a bare address is scoped to itself, not to its last two labels`() { + // ⚠️ topPrivateDomain() is null here, and null means *no restriction* to + // the handler — so the fallback has to be the exact host. A split would + // scope 192.168.1.10 to "1.10". + val client = CalDavHttp.authenticated( + "Agendula", + "user", + "pw", + "https://192.168.1.10/dav/".toHttpUrl(), + ) + val handler = client.networkInterceptors.filterIsInstance().single() + assertThat(handler.domain).isEqualTo("192.168.1.10") + + val ours = handler.authenticateRequest( + Request.Builder().url("https://192.168.1.10/dav/").build(), + null, + ) + assertThat(ours?.header("Authorization")).isNotNull() + + val other = handler.authenticateRequest( + Request.Builder().url("https://10.20.1.10/dav/").build(), + null, + ) + assertThat(other).isNull() + } + + @Test + fun `a single-label host is scoped to itself`() { + val client = CalDavHttp.authenticated( + "Agendula", + "user", + "pw", + "https://localhost:8443/dav/".toHttpUrl(), + ) + val handler = client.networkInterceptors.filterIsInstance().single() + assertThat(handler.domain).isEqualTo("localhost") + + val elsewhere = handler.authenticateRequest( + Request.Builder().url("https://notlocalhost/dav/").build(), + null, + ) + assertThat(elsewhere).isNull() + } + @Test fun `an unrelated domain gets nothing`() { val client = CalDavHttp.authenticated("Agendula", "user", "pw", origin) diff --git a/dav/PROVENANCE.md b/dav/PROVENANCE.md index d2f279d..0a9abf8 100644 --- a/dav/PROVENANCE.md +++ b/dav/PROVENANCE.md @@ -180,3 +180,37 @@ downgrade still throws. Found against a real server, not by reading: `cloud.jeanlucmakiola.de` returns `301 → http://cloud.jeanlucmakiola.de/remote.php/dav/`. + +## Change 8 — the credential is scoped by the public-suffix list, not by a label split + +`BasicDigestAuthHandler` gated every request on +`domain.equals(UrlUtils.hostToDomain(request host))`, and `hostToDomain` is a +pure last-two-labels split with no public-suffix knowledge. So a server at +`cloud.example.co.uk` scoped the credential to `co.uk`, one at +`myhome.duckdns.org` to `duckdns.org`, and a self-hoster at `192.168.1.10` to +`1.10`. + +⚠️ **The handler adds `Authorization: Basic` preemptively**, before any +challenge, to the first HTTPS request to any host that passes that gate. So the +scope is not merely recorded — it is the set of hosts that receive the app +password unprompted. + +That is reachable. `ServiceDiscovery` builds candidate origins from SRV targets +without requiring the target to lie inside the queried domain, over plain UDP +DNS with no DNSSEC. Correct scoping forces an on-path attacker to obtain a +certificate for a name inside the victim's own registrable domain, which is +infeasible; `co.uk` scoping lets them point the SRV at a domain they own and +hold a legitimate certificate for. + +Now: `request.url.topPrivateDomain() ?: request.url.host`. OkHttp bundles the +public-suffix list including its private section, so the dynamic-DNS providers +self-hosters actually use are covered. `topPrivateDomain()` is null for an IP +literal, a single-label host, and a host that *is* a public suffix — and null +means *no restriction* to this handler, so it falls back to the exact host. + +The caller must derive the scope the same way, which is why +`CalDavHttp.authenticated` changed with it: a mismatch withholds the credential +from every request rather than leaking it. + +`UrlUtils.hostToDomain` and its test are left alone — after this it has no +production callers, and keeping it keeps the resync diff small. diff --git a/dav/src/main/kotlin/at/bitfire/dav4jvm/BasicDigestAuthHandler.kt b/dav/src/main/kotlin/at/bitfire/dav4jvm/BasicDigestAuthHandler.kt index 54a8399..2cdf129 100644 --- a/dav/src/main/kotlin/at/bitfire/dav4jvm/BasicDigestAuthHandler.kt +++ b/dav/src/main/kotlin/at/bitfire/dav4jvm/BasicDigestAuthHandler.kt @@ -25,7 +25,12 @@ import java.util.concurrent.atomic.AtomicInteger * Usage: Set as authenticator *and* as network interceptor. */ class BasicDigestAuthHandler( - /** Authenticate only against hosts ending with this domain (may be null, which means no restriction) */ + /** + * Authenticate only against hosts sharing this registrable domain, as + * [HttpUrl.topPrivateDomain] derives it — or, for a host that has none + * (an IP literal, `localhost`, a host that *is* a public suffix), only + * against that exact host. Null means no restriction. + */ val domain: String?, val username: String, @@ -61,7 +66,14 @@ class BasicDigestAuthHandler( fun authenticateRequest(request: Request, response: Response?): Request? { domain?.let { val host = request.url.host - if (!domain.equals(UrlUtils.hostToDomain(host), true)) { + // ⚠️ The public-suffix list, not a last-two-labels split. Splitting + // scopes `cloud.example.co.uk` to `co.uk` and `192.168.1.10` to + // `1.10`, handing the credential preemptively to any host that + // matches — including one an attacker can buy a certificate for. + // `topPrivateDomain()` is null for hosts with no registrable domain, + // and null here would mean *no* restriction, so it falls back to the + // exact host. + if (!domain.equals(request.url.topPrivateDomain() ?: host, true)) { Dav4jvm.log.warning("Not authenticating against $host because it doesn't belong to $domain") return null }