diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavHttp.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavHttp.kt index 8426bd6..93c750a 100644 --- a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavHttp.kt +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavHttp.kt @@ -82,9 +82,8 @@ object CalDavHttp { domain = origin.topPrivateDomain() ?: origin.host, username = username, password = password, - // Never preemptively over cleartext. The handler already gates its - // own preemptive path on isHttps; stating it is cheap insurance. - insecurePreemptive = false, + // Never over cleartext, challenged or not. + insecureBasic = false, ) return base(userAgent) .authenticator(handler) diff --git a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalDavHttpTest.kt b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalDavHttpTest.kt index 53b187a..f3cc412 100644 --- a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalDavHttpTest.kt +++ b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalDavHttpTest.kt @@ -3,7 +3,9 @@ package de.jeanlucmakiola.caldav import at.bitfire.dav4jvm.BasicDigestAuthHandler import com.google.common.truth.Truth.assertThat import okhttp3.HttpUrl.Companion.toHttpUrl +import okhttp3.Protocol import okhttp3.Request +import okhttp3.Response import org.junit.Test class CalDavHttpTest { @@ -171,6 +173,35 @@ class CalDavHttpTest { ).isNull() } + @Test + fun `cleartext carries no credential even when the server asks for one`() { + val client = CalDavHttp.authenticated("Agendula", "user", "pw", origin) + val handler = client.networkInterceptors.filterIsInstance().single() + val request = Request.Builder().url("http://cloud.example.com/dav/").build() + + // ⚠️ Gating only the preemptive path leaves this open: the server just + // has to ask. Basic is the password, in a header every hop can read. + assertThat(handler.authenticateRequest(request, challenge(request, "Basic"))).isNull() + } + + @Test + fun `a challenge over TLS is still answered`() { + val client = CalDavHttp.authenticated("Agendula", "user", "pw", origin) + val handler = client.networkInterceptors.filterIsInstance().single() + val request = Request.Builder().url("https://cloud.example.com/dav/").build() + + val authorised = handler.authenticateRequest(request, challenge(request, "Basic")) + assertThat(authorised?.header("Authorization")).startsWith("Basic") + } + + private fun challenge(request: Request, scheme: String) = Response.Builder() + .request(request) + .protocol(Protocol.HTTP_1_1) + .code(401) + .message("Authentication required") + .header("WWW-Authenticate", "$scheme realm=\"dav\"") + .build() + @Test fun `derived clients share one connection pool`() { // A fresh OkHttpClient per probe gives each its own pool and dispatcher diff --git a/dav/PROVENANCE.md b/dav/PROVENANCE.md index 0a9abf8..ff29a7a 100644 --- a/dav/PROVENANCE.md +++ b/dav/PROVENANCE.md @@ -214,3 +214,31 @@ 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. + +## Change 9 — Basic is refused over cleartext even when the server asks for it + +`insecurePreemptive` gated only the preemptive branch. A plain-HTTP server +answering 401 with a `Basic` challenge still got +`Authorization: Basic ` in the clear — the flag's name was +accurate and its coverage was not. + +The gate now sits on the Basic *emission*, so both paths are covered by one +condition, and the flag is renamed `insecureBasic` to say what it actually +permits. Digest is deliberately untouched: it never puts the password on the +wire, and refusing it would break a LAN server the day the documented +per-account cleartext opt-in ships. + +Not currently reachable in the app — `network_security_config.xml` sets +`cleartextTrafficPermitted="false"`, so OkHttp throws before the request is +written, and nothing passes `allowCleartext = true`. Fixed anyway: `:caldav` is +a plain JVM module earmarked for reuse, where the Android policy does not apply, +and the mitigation would evaporate silently the day that opt-in is wired up. + +⚠️ **Upstream's `BasicDigestAuthHandlerTest.testBasic` was amended** — it +asserted exactly this behaviour, using `http://example.com` with a Basic +challenge and expecting the header. Its URL is now `https://`, and the +cleartext cases it used to cover are pinned explicitly by +`cleartextBasicIsRefusedEvenWhenChallenged`, +`cleartextBasicIsSentWhenExplicitlyAllowed` and `cleartextDigestIsStillAnswered`. +This is the one upstream test this port deliberately changes rather than +inherits. diff --git a/dav/src/main/kotlin/at/bitfire/dav4jvm/BasicDigestAuthHandler.kt b/dav/src/main/kotlin/at/bitfire/dav4jvm/BasicDigestAuthHandler.kt index 2cdf129..561d71a 100644 --- a/dav/src/main/kotlin/at/bitfire/dav4jvm/BasicDigestAuthHandler.kt +++ b/dav/src/main/kotlin/at/bitfire/dav4jvm/BasicDigestAuthHandler.kt @@ -36,7 +36,11 @@ class BasicDigestAuthHandler( val username: String, val password: String, - val insecurePreemptive: Boolean = false + /** + * Allow Basic over cleartext — both preemptively and in answer to a + * challenge. Off by default: the header is the password. + */ + val insecureBasic: Boolean = false ): Authenticator, Interceptor { companion object { @@ -82,7 +86,7 @@ class BasicDigestAuthHandler( if (response == null) { // we're not processing a 401 response - if (basicAuth == null && digestAuth == null && (request.isHttps || insecurePreemptive)) { + if (basicAuth == null && digestAuth == null && (request.isHttps || insecureBasic)) { Dav4jvm.log.fine("Trying Basic auth preemptively") basicAuth = Challenge("Basic", "") } @@ -124,6 +128,16 @@ class BasicDigestAuthHandler( } basicAuth != null -> { + // ⚠️ Gated here, not only on the preemptive path above. Basic + // *is* the password, in a header any hop can read, so a plain + // HTTP server answering 401 with a Basic challenge must not be + // able to ask for it. Digest is left alone: it never transmits + // the password, and refusing it would kill a LAN server the day + // the cleartext opt-in ships. + if (!request.isHttps && !insecureBasic) { + Dav4jvm.log.warning("Refusing to send Basic credentials over cleartext to ${request.url}") + return null + } Dav4jvm.log.fine("Adding Basic authorization header for ${request.url}") /* In RFC 2617 (obsolete), there was no encoding for credentials defined, although diff --git a/dav/src/test/kotlin/at/bitfire/dav4jvm/BasicDigestAuthHandlerTest.kt b/dav/src/test/kotlin/at/bitfire/dav4jvm/BasicDigestAuthHandlerTest.kt index a33a284..139de0b 100644 --- a/dav/src/test/kotlin/at/bitfire/dav4jvm/BasicDigestAuthHandlerTest.kt +++ b/dav/src/test/kotlin/at/bitfire/dav4jvm/BasicDigestAuthHandlerTest.kt @@ -18,11 +18,15 @@ import org.junit.Test class BasicDigestAuthHandlerTest { + // ⚠️ Amended from upstream: https, not http. Basic over cleartext is now + // refused whether or not it was challenged — see PROVENANCE change 9. The + // cleartext behaviour this used to assert is pinned by + // `cleartextBasicIsRefusedEvenWhenChallenged` below. @Test fun testBasic() { var authenticator = BasicDigestAuthHandler(null, "user", "password") val original = Request.Builder() - .url("http://example.com") + .url("https://example.com") .build() var response = Builder() .request(original) @@ -42,6 +46,55 @@ class BasicDigestAuthHandlerTest { assertEquals("Basic dXNlcm5hbWU6cGHDn3dvcmQ=", request!!.header("Authorization")) } + @Test + fun cleartextBasicIsRefusedEvenWhenChallenged() { + val authenticator = BasicDigestAuthHandler(null, "user", "password") + val original = Request.Builder().url("http://example.com").build() + val response = Builder() + .request(original) + .protocol(Protocol.HTTP_1_1) + .code(401).message("Authentication required") + .header("WWW-Authenticate", "Basic realm=\"WallyWorld\"") + .build() + + assertNull(authenticator.authenticateRequest(original, response)) + } + + @Test + fun cleartextBasicIsSentWhenExplicitlyAllowed() { + val authenticator = BasicDigestAuthHandler(null, "user", "password", insecureBasic = true) + val original = Request.Builder().url("http://example.com").build() + val response = Builder() + .request(original) + .protocol(Protocol.HTTP_1_1) + .code(401).message("Authentication required") + .header("WWW-Authenticate", "Basic realm=\"WallyWorld\"") + .build() + + val request = authenticator.authenticateRequest(original, response) + assertEquals("Basic dXNlcjpwYXNzd29yZA==", request!!.header("Authorization")) + } + + @Test + fun cleartextDigestIsStillAnswered() { + // Digest never puts the password on the wire, so gating it would break a + // LAN server for nothing. + val authenticator = BasicDigestAuthHandler(null, "user", "password") + val original = Request.Builder().url("http://example.com").build() + val response = Builder() + .request(original) + .protocol(Protocol.HTTP_1_1) + .code(401).message("Authentication required") + .header( + "WWW-Authenticate", + "Digest realm=\"WallyWorld\", nonce=\"abc\", qop=\"auth\"", + ) + .build() + + val request = authenticator.authenticateRequest(original, response) + assertTrue(request!!.header("Authorization")!!.startsWith("Digest")) + } + @Test fun testDigestRFCExample() { // use cnonce from example