Skip to content

[FIX] SSL handling#4861

Merged
jesmrec merged 3 commits into
masterfrom
fix/ssl_handling
May 26, 2026
Merged

[FIX] SSL handling#4861
jesmrec merged 3 commits into
masterfrom
fix/ssl_handling

Conversation

@jesmrec
Copy link
Copy Markdown
Contributor

@jesmrec jesmrec commented May 19, 2026

From

opencloud-eu/android#121
opencloud-eu/android#126

  • Host is now correctly verified with a KnownServersHostnameVerifier that checks if:

    • Host certificate is "acceptable" through the network library
        if (mDelegate.verify(hostname, session)) {
            return true;
        }
      
    • If not, it checks if host certificate was previously accepted and exists in the certificate store
       return NetworkUtils.isCertInKnownServersStore(peerCerts[0], mContext);
      
  • If a SslPeerUnverifiedException raises, it is enclosed in a CertificateCombinedException to be handled separately from a generic SSLException

     CertificateCombinedException sslPeerUnverifiedException = new CertificateCombinedException(lastCert);
     sslPeerUnverifiedException.setSslPeerUnverifiedException((SSLPeerUnverifiedException) e);
    
  • When SslPeerUnverifiedException raises, the certificate accepting dialog turns into an alert

       if (m509Certificate == null) {
           ok.setText(android.R.string.ok);
           mView.findViewById(R.id.question).setVisibility(View.GONE);
           cancel.setVisibility(View.GONE);
       }
    
  • Certificate and connection are aborted

     ((OnSslUntrustedCertListener) getActivity()).onCancelCertificate();
    

Related Issues

App:

  • Add changelog files for the fixed issues in folder changelog/unreleased. More info here
  • Add feature to Release Notes in ReleaseNotesViewModel.kt creating a new ReleaseNote() with String resources (if required)

QA

@jesmrec jesmrec added this to the 4.8.1 - Current milestone May 19, 2026
@jesmrec jesmrec changed the title [FIX] ssl handling [FIX] SSL handling May 19, 2026
@jesmrec jesmrec force-pushed the fix/ssl_handling branch from af2e071 to 7f30c21 Compare May 19, 2026 12:25
@jesmrec jesmrec force-pushed the fix/ssl_handling branch from 7f30c21 to 5e3a5f8 Compare May 20, 2026 07:48
@jesmrec
Copy link
Copy Markdown
Contributor Author

jesmrec commented May 20, 2026

Some checks done with badssl.com

Test case URL Checkpoints Expected result Comments
Valid https flow https://badssl.com/ 1. KnownServersHostnameVerifier.verify(...)
2. mDelegate.verify(hostname, session)
3. Final result of verify(...)
1. Reached ✅
2. mDelegate.verify(...) == true
3. KnownServersHostnameVerifier.verify(...) == true
3. No fallback to isCertInKnownServersStore(...)
3. No SSL dialog ✅
The badssl.com is intentionally valid and is used as the valid https case.
Hostname not verified https://wrong.host.badssl.com/ 1. mDelegate.verify(hostname, session)
2. NetworkUtils.isCertInKnownServersStore(...)
3. getCertificateAlias(cert)
4. e instanceof SSLPeerUnverifiedException
5. mException / mCode
6. SslUntrustedCertDialog.m509Certificate
7. Final button action
1. mDelegate.verify(...) == false
2. isCertInKnownServersStore(...) == false
3. getCertificateAlias(cert) == null
4. e instanceof SSLPeerUnverifiedException == true
5. mException instanceof CertificateCombinedException
5. mCode == SSL_RECOVERABLE_PEER_UNVERIFIED
6. m509Certificate == null
6. Dialog shows only OK
7. Pressing OK calls onCancelCertificate()
This is the main scenario. The certificate does not cover wrong.host.badssl.com, so the app must not allow the user to save/trust it from the dialog and it is aborted.
Untrusted certificate, hostname correct https://self-signed.badssl.com/ 1. mDelegate.verify(hostname, session)
2. e instanceof SSLPeerUnverifiedException
3. SslUntrustedCertDialog.m509Certificate
4. Final button action if user accepts
1. mDelegate.verify(...) == true
1. No fallback to isCertInKnownServersStore(...) from hostname verifier ✅
2. e instanceof SSLPeerUnverifiedException == false
3. m509Certificate != null
4. If user accepts: addCertToKnownServersStore(...) and onSavedCertificate() is called ✅
The hostname is correct, but the certificate is self-signed. This should be treated as a classic trust problem, not as a peer/hostname verification failure.
Certificate validity error, hostname correct https://expired.badssl.com/ 1. mDelegate.verify(hostname, session)
2. e instanceof SSLPeerUnverifiedException
3. SslUntrustedCertDialog.m509Certificate
4. UI Dialog
1. mDelegate.verify(...) == true
1. No fallback to isCertInKnownServersStore(...) from hostname verifier ✅
2. e instanceof SSLPeerUnverifiedException == false
3. m509Certificate != null
4. Dialog to continue with connection ✅
The hostname matches, but the certificate is expired. This validates that certificate validity errors are not confused with peer/hostname failures.
Untrusted root CA, hostname correct https://untrusted-root.badssl.com/ 1. mDelegate.verify(hostname, session)
2. e instanceof SSLPeerUnverifiedException
3. SslUntrustedCertDialog.m509Certificate
4. UI dialog
1. mDelegate.verify(...) == true
1. No fallback to isCertInKnownServersStore(...) from hostname verifier ✅
2. e instanceof SSLPeerUnverifiedException == false
3. m509Certificate != null
4. Dialog to continue with connection ✅
The hostname matches, but the root CA is not trusted. This validates that trust-chain errors are not confused with peer/hostname failures.
Missing intermediate certificate, hostname correct https://incomplete-chain.badssl.com/ 1. mDelegate.verify(hostname, session)
2. e instanceof SSLPeerUnverifiedException
3. SslUntrustedCertDialog.m509Certificate
4. UI dialog
1. mDelegate.verify(...) == true
1. No fallback to isCertInKnownServersStore(...) from hostname verifier ✅
2. e instanceof SSLPeerUnverifiedException == false
3. If dialog appears: m509Certificate != null
4. Dialog to continue with connection ✅
The hostname matches, but the certificate chain is incomplete. This validates that chain-building errors are not confused with peer/hostname failures.

@joragua joragua force-pushed the fix/ssl_handling branch from e39c9dd to 3b3a275 Compare May 26, 2026 12:06
@jesmrec jesmrec marked this pull request as ready for review May 26, 2026 12:12
@jesmrec jesmrec closed this May 26, 2026
@jesmrec jesmrec reopened this May 26, 2026
@joragua joragua assigned jesmrec and unassigned jesmrec May 26, 2026
@jesmrec jesmrec closed this May 26, 2026
@jesmrec jesmrec reopened this May 26, 2026
@jesmrec jesmrec force-pushed the fix/ssl_handling branch from 6e0b6cb to 3b3a275 Compare May 26, 2026 13:19
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR tightens SSL/TLS handling by replacing the permissive hostname verifier with logic that validates the hostname normally, and falls back to trusting previously accepted server certificates. It also improves propagation/UI handling of SSLPeerUnverifiedException so the app can present an appropriate alert and abort the connection/certificate flow.

Changes:

  • Replace the always-true hostname verifier with KnownServersHostnameVerifier that delegates to standard hostname checks and falls back to the known-servers certificate store.
  • Wrap SSLPeerUnverifiedException into CertificateCombinedException (carrying the last seen certificate) to route it through the existing recoverable SSL error path.
  • Update the untrusted certificate dialog behavior for hostname-not-verified cases and add a security changelog entry.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
owncloudComLibrary/src/main/java/com/owncloud/android/lib/common/operations/RemoteOperationResult.java Wrap SSLPeerUnverifiedException into CertificateCombinedException using a thread-local last certificate.
owncloudComLibrary/src/main/java/com/owncloud/android/lib/common/network/NetworkUtils.java Add helper to check if a certificate exists in the known-servers store.
owncloudComLibrary/src/main/java/com/owncloud/android/lib/common/network/KnownServersHostnameVerifier.java New hostname verifier that delegates to default verification and falls back to known-servers cert store.
owncloudComLibrary/src/main/java/com/owncloud/android/lib/common/network/AdvancedX509TrustManager.java Track “last certificate” in a ThreadLocal for later error wrapping.
owncloudComLibrary/src/main/java/com/owncloud/android/lib/common/http/HttpClient.java Wire the new hostname verifier into OkHttp client configuration.
owncloudApp/src/main/java/com/owncloud/android/ui/dialog/SslUntrustedCertDialog.java Adjust dialog UI/flow when hostname verification fails (peer unverified).
changelog/unreleased/4861 Add changelog entry for SSL verification/trusted host handling improvements.
Comments suppressed due to low confidence (1)

owncloudComLibrary/src/main/java/com/owncloud/android/lib/common/network/AdvancedX509TrustManager.java:102

  • checkServerTrusted guards certificates with a null/length check, but then immediately dereferences certificates[0] regardless. As written, a null/empty chain will still crash with NPE/ArrayIndexOutOfBounds. Either enforce the contract at the top (return/throw) or update all subsequent uses to handle null/empty safely.
    public void checkServerTrusted(X509Certificate[] certificates, String authType) {
        if (certificates != null && certificates.length > 0) {
            sLastCert.set(certificates[0]);
        }
        if (!isKnownServer(certificates[0])) {
            CertificateCombinedException result = new CertificateCombinedException(certificates[0]);
            try {
                certificates[0].checkValidity();

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +2 to +16
* ownCloud Android client application
*
* Copyright (C) 2026 ownCloud GmbH.
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2,
* as published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
Comment on lines +21 to +37
import android.content.Context;
import okhttp3.internal.tls.OkHostnameVerifier;
import timber.log.Timber;
import javax.net.ssl.HostnameVerifier;
import javax.net.ssl.SSLPeerUnverifiedException;
import javax.net.ssl.SSLSession;
import java.security.cert.Certificate;
import java.security.cert.X509Certificate;

public class KnownServersHostnameVerifier implements HostnameVerifier {

private final Context mContext;
private final HostnameVerifier mDelegate;

public KnownServersHostnameVerifier(Context context) {
this(context, OkHostnameVerifier.INSTANCE);
}
Comment on lines +89 to 99
public static final ThreadLocal<X509Certificate> sLastCert = new ThreadLocal<>();

/**
* @see javax.net.ssl.X509TrustManager#checkServerTrusted(X509Certificate[],
* String authType)
*/
public void checkServerTrusted(X509Certificate[] certificates, String authType) {
if (certificates != null && certificates.length > 0) {
sLastCert.set(certificates[0]);
}
if (!isKnownServer(certificates[0])) {
@jesmrec jesmrec merged commit 8715a24 into master May 26, 2026
14 of 22 checks passed
@jesmrec jesmrec deleted the fix/ssl_handling branch May 26, 2026 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants