From f1a73b29af59cc2488c35cbf858793a0f0a59342 Mon Sep 17 00:00:00 2001 From: Marcel Hibbe Date: Fri, 4 Sep 2026 15:52:35 +0200 Subject: [PATCH 1/2] fix(ssl): accept self-signed certificates without SAN once manually trusted The hostname verifier rejected any certificate lacking a matching Subject Alternative Name, even after the user explicitly trusted that exact certificate via the certificate dialog. Fall back to the manual trust store when the strict SAN check fails, so legacy self-signed certificates that only set a Common Name work once accepted. Assisted-by: Claude Code:claude-sonnet-5 Signed-off-by: Marcel Hibbe --- .../talk/utils/ssl/TrustManager.java | 25 +++++++++++-------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/utils/ssl/TrustManager.java b/app/src/main/java/com/nextcloud/talk/utils/ssl/TrustManager.java index 1cb96ac956e..f83a02baeba 100644 --- a/app/src/main/java/com/nextcloud/talk/utils/ssl/TrustManager.java +++ b/app/src/main/java/com/nextcloud/talk/utils/ssl/TrustManager.java @@ -164,19 +164,24 @@ private HostnameVerifier(javax.net.ssl.HostnameVerifier defaultHostNameVerifier) @Override public boolean verify(String s, SSLSession sslSession) { + try { + X509Certificate[] certificates = (X509Certificate[]) sslSession.getPeerCertificates(); + if (certificates.length == 0) { + return false; + } - if (defaultHostNameVerifier.verify(s, sslSession)) { - try { - X509Certificate[] certificates = (X509Certificate[]) sslSession.getPeerCertificates(); - if (certificates.length > 0 && isCertInTrustStore(certificates, s)) { - return true; - } - } catch (SSLPeerUnverifiedException e) { - Log.d(TAG, "Couldn't get certificate for host name verification"); + if (defaultHostNameVerifier.verify(s, sslSession)) { + return true; } - } - return false; + // Certificate has no (matching) Subject Alternative Name, e.g. legacy self-signed + // certificates that only set the Common Name. Accept it anyway if the user already + // explicitly trusted this exact certificate via the certificate dialog. + return isCertInTrustStore(certificates[0]); + } catch (SSLPeerUnverifiedException e) { + Log.d(TAG, "Couldn't get certificate for host name verification"); + return false; + } } } From 124f3c6987b77668d783466328cea8a0fdf0c274 Mon Sep 17 00:00:00 2001 From: Marcel Hibbe Date: Fri, 4 Sep 2026 16:23:19 +0200 Subject: [PATCH 2/2] test(ssl): add unit tests for TrustManager hostname verification Covers the hostname verifier fallback: a certificate without a matching SAN is rejected unless the user already trusted it manually, a certificate with a matching SAN is accepted, and missing/unreadable peer certificates are rejected. Verified these tests fail against the pre-fix TrustManager. Adds okhttp-tls as a test dependency to generate real self-signed certificates (with and without a SAN) instead of certificate mocks, since KeyStore.setCertificateEntry needs an encodable certificate. Assisted-by: Claude Code:claude-sonnet-5 Signed-off-by: Marcel Hibbe --- app/build.gradle.kts | 1 + .../talk/utils/ssl/TrustManagerTest.kt | 152 ++++++++++++++++++ 2 files changed, 153 insertions(+) create mode 100644 app/src/test/java/com/nextcloud/talk/utils/ssl/TrustManagerTest.kt diff --git a/app/build.gradle.kts b/app/build.gradle.kts index a600a095172..343007c1326 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -389,6 +389,7 @@ dependencies { testImplementation("com.google.crypto.tink:tink:1.23.0") testImplementation("androidx.room:room-testing:$roomVersion") testImplementation("com.squareup.okhttp3:mockwebserver:$okhttpVersion") + testImplementation("com.squareup.okhttp3:okhttp-tls:$okhttpVersion") testImplementation("com.google.dagger:hilt-android-testing:2.60.1") testImplementation("org.robolectric:robolectric:4.16.1") // conscrypt-android provides Android JNI libs only; the openjdk-uber variant bundles diff --git a/app/src/test/java/com/nextcloud/talk/utils/ssl/TrustManagerTest.kt b/app/src/test/java/com/nextcloud/talk/utils/ssl/TrustManagerTest.kt new file mode 100644 index 00000000000..54fed42ef7d --- /dev/null +++ b/app/src/test/java/com/nextcloud/talk/utils/ssl/TrustManagerTest.kt @@ -0,0 +1,152 @@ +/* + * Nextcloud Talk - Android Client + * + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: GPL-3.0-or-later + */ +package com.nextcloud.talk.utils.ssl + +import android.app.Application +import android.content.Context +import androidx.test.core.app.ApplicationProvider +import com.nextcloud.talk.application.NextcloudTalkApplication +import okhttp3.tls.HeldCertificate +import org.junit.After +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.mockito.kotlin.mock +import org.mockito.kotlin.whenever +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config +import org.robolectric.util.ReflectionHelpers +import java.security.cert.X509Certificate +import javax.net.ssl.HostnameVerifier +import javax.net.ssl.SSLPeerUnverifiedException +import javax.net.ssl.SSLSession + +/** + * Regression tests for the hostname verifier returned by [TrustManager.getHostnameVerifier]. + * + * Before the fix, a certificate without a matching Subject Alternative Name (e.g. legacy + * self-signed certificates that only set a Common Name) was always rejected, even after the + * user had explicitly trusted that exact certificate via the certificate dialog + * ([TrustManager.addCertInTrustStore]). + */ +@RunWith(RobolectricTestRunner::class) +@Config(application = Application::class, sdk = [33]) +class TrustManagerTest { + + private lateinit var trustManager: TrustManager + + @Before + fun setUp() { + // TrustManager() needs NextcloudTalkApplication.sharedApplication for Context.getDir(). + // Attach a real context without running NextcloudTalkApplication.onCreate(), which would + // pull in Dagger, WorkManager and WebRTC init that this test doesn't need. + val fakeApplication = NextcloudTalkApplication() + ReflectionHelpers.callInstanceMethod( + fakeApplication, + "attachBaseContext", + ReflectionHelpers.ClassParameter.from(Context::class.java, ApplicationProvider.getApplicationContext()) + ) + setSharedApplication(fakeApplication) + + trustManager = TrustManager() + } + + @After + fun tearDown() { + setSharedApplication(null) + } + + // NextcloudTalkApplication.sharedApplication's setter is `protected` (only meant to be set + // from onCreate()/onTerminate()); reflection is the only way in from a test. + private fun setSharedApplication(application: NextcloudTalkApplication?) { + ReflectionHelpers.callInstanceMethod( + NextcloudTalkApplication.Companion, + "setSharedApplication", + ReflectionHelpers.ClassParameter.from(NextcloudTalkApplication::class.java, application) + ) + } + + @Test + fun `verify accepts connection when default hostname verifier passes`() { + // By the time the hostname verifier runs, checkServerTrusted() has already let the TLS + // handshake through for this exact certificate (CA-trusted, or previously trusted here). + val certificate = selfSignedCertificate(host = "example.com", withSan = true) + trustManager.addCertInTrustStore(certificate) + + val sslSession = sessionWithCertificate(certificate) + val defaultVerifier = mock() + whenever(defaultVerifier.verify("example.com", sslSession)).thenReturn(true) + + val hostnameVerifier = trustManager.getHostnameVerifier(defaultVerifier) + + assertTrue(hostnameVerifier.verify("example.com", sslSession)) + } + + @Test + fun `verify rejects legacy certificate without SAN that was never manually trusted`() { + val certificate = selfSignedCertificate(host = "192.168.178.162", withSan = false) + val sslSession = sessionWithCertificate(certificate) + val defaultVerifier = mock() + whenever(defaultVerifier.verify("192.168.178.162", sslSession)).thenReturn(false) + + val hostnameVerifier = trustManager.getHostnameVerifier(defaultVerifier) + + assertFalse(hostnameVerifier.verify("192.168.178.162", sslSession)) + } + + @Test + fun `verify accepts legacy certificate without SAN once the user manually trusted it`() { + val certificate = selfSignedCertificate(host = "192.168.178.162", withSan = false) + trustManager.addCertInTrustStore(certificate) + + val sslSession = sessionWithCertificate(certificate) + val defaultVerifier = mock() + whenever(defaultVerifier.verify("192.168.178.162", sslSession)).thenReturn(false) + + val hostnameVerifier = trustManager.getHostnameVerifier(defaultVerifier) + + assertTrue(hostnameVerifier.verify("192.168.178.162", sslSession)) + } + + @Test + fun `verify rejects connection when there are no peer certificates`() { + val sslSession = mock() + whenever(sslSession.peerCertificates).thenReturn(arrayOf()) + val defaultVerifier = mock() + + val hostnameVerifier = trustManager.getHostnameVerifier(defaultVerifier) + + assertFalse(hostnameVerifier.verify("example.com", sslSession)) + } + + @Test + fun `verify rejects connection when peer certificates cannot be read`() { + val sslSession = mock() + whenever(sslSession.peerCertificates).thenThrow(SSLPeerUnverifiedException("no certificates")) + val defaultVerifier = mock() + + val hostnameVerifier = trustManager.getHostnameVerifier(defaultVerifier) + + assertFalse(hostnameVerifier.verify("example.com", sslSession)) + } + + private fun selfSignedCertificate(host: String, withSan: Boolean): X509Certificate { + val builder = HeldCertificate.Builder().commonName(host) + if (withSan) { + builder.addSubjectAlternativeName(host) + } + return builder.build().certificate + } + + private fun sessionWithCertificate(certificate: X509Certificate): SSLSession { + val sslSession = mock() + whenever(sslSession.peerCertificates).thenReturn(arrayOf(certificate)) + return sslSession + } +}