Enhance SSL handling and testing#121
Conversation
…etter certificate validation, remove hostname verifier in HttpClient, and add HttpClientTlsTest for SSL peer verification scenarios.
There was a problem hiding this comment.
Pull request overview
This PR closes a transport security gap in the Android client by restoring default TLS hostname verification in OkHttp, while keeping the app’s existing “recoverable SSL” handling path intact (including UI behavior) and adding a regression test.
Changes:
- Removed the always-true
hostnameVerifierso OkHttp performs standard hostname validation again. - Mapped
SSLPeerUnverifiedExceptionintoCertificateCombinedExceptioninRemoteOperationResultto preserve the existing SSL recoverable flow. - Added Robolectric/MockWebServer coverage for hostname mismatch rejection and the exception mapping.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| opencloudComLibrary/src/main/java/eu/opencloud/android/lib/common/http/HttpClient.java | Stops disabling hostname verification by removing the permissive verifier. |
| opencloudComLibrary/src/main/java/eu/opencloud/android/lib/common/operations/RemoteOperationResult.java | Wraps hostname verification failures into CertificateCombinedException with the expected result code. |
| opencloudApp/src/main/java/eu/opencloud/android/ui/dialog/SslUntrustedCertDialog.java | Adjusts UI/flow so hostname mismatch is shown as SSL-related but not treated as “trust and store cert”. |
| opencloudComLibrary/src/test/java/eu/opencloud/android/lib/common/http/HttpClientTlsTest.kt | Adds regression tests for rejecting wrong-hostname certs and for exception wrapping behavior. |
| opencloudComLibrary/build.gradle | Adds okhttp-tls test dependency to generate test certificates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
opencloudApp/src/main/java/eu/opencloud/android/ui/dialog/SslUntrustedCertDialog.java
Outdated
Show resolved
Hide resolved
opencloudApp/src/main/java/eu/opencloud/android/ui/dialog/SslUntrustedCertDialog.java
Outdated
Show resolved
Hide resolved
...mLibrary/src/main/java/eu/opencloud/android/lib/common/operations/RemoteOperationResult.java
Show resolved
Hide resolved
opencloudComLibrary/src/test/java/eu/opencloud/android/lib/common/http/HttpClientTlsTest.kt
Show resolved
Hide resolved
…ntrustedCertDialog.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ntrustedCertDialog.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
I would treat this as CVE-worthy and rate it as CVSS 3.1 7.4 (High). The root cause is that the Android client disables TLS hostname verification globally. Certificate chain validation still runs, but the client accepts a certificate that is valid for a different hostname. In practice, this allows an attacker in a network position to impersonate the backend for any flow that uses this client, including login, token exchange, sync, upload, and download. I would not classify it as Critical because exploitation still requires a successful on-path position and the primary impact is confidentiality and integrity rather than availability. That said, the impact on protected backend communication is broad enough that a High severity rating is justified. Proposed vector: CVSS:3.1/AV:N/AC:H/PR:N/UI:N/S:U/C:H/I:H/A:N = 7.4 |
…rox80/android into fix/tls-hostname-verification
|
Seems to exist since 2018 even: owncloud/android-library@e6bdfab#diff-683f994be72f7281f159ef0fda43ca0e14182804390d3aca7239941c58c59f60R78 |
|
I tested this with https://wrong.host.badssl.com/ but it does not actually show the certificate in UI, @zerox80 04-02 18:33:12.669 11909 11955 D (RemoteOperationResult.java:120): Create RemoteOperationResult from exception. 
|
…er and update RemoteOperationResult to utilize it for SSL exceptions
|
its fixed |
Summary
This fixes a TLS validation bug in the Android client.
HttpClientwas explicitly disabling hostname verification for every HTTPS connection. Certificate chain validation was still active, but the client would accept a certificate for the wrong host. That weakens transport security across all backend communication, including login, token-based authentication, sync, upload, and download.The fix restores normal hostname verification and keeps the existing SSL error handling intact so hostname mismatches are still surfaced through the app's recoverable certificate flow instead of failing in an inconsistent way.
What changed
HttpClientso OkHttp uses its default hostname checks again.SSLPeerUnverifiedExceptioninCertificateCombinedExceptioninsideRemoteOperationResultto preserve the app's existing SSL error handling path.SslUntrustedCertDialogso hostname mismatches are shown as an SSL problem, but are not treated like a certificate that can simply be trusted and stored.Why this approach
The main issue was not certificate trust itself, but the missing hostname check. Removing the custom verifier is the narrowest and correct fix.
There was one follow-up issue to address at the same time: once hostname verification is re-enabled, the app can receive
SSLPeerUnverifiedExceptionagain. Some parts of the current flow assume recoverable SSL issues arrive asCertificateCombinedException, so the error mapping had to be kept consistent to avoid regressions in the login and file-operation UI.Testing
HttpClientTlsTestcovering hostname mismatch rejection and the recoverable exception mapping.Risk
The behavioral change is intentional: servers with certificates whose CN or SAN does not match the requested host will now fail instead of being accepted.
That may expose misconfigured environments that happened to work before, but it is required to close the TLS gap.