From 604aa87ccf35ac1beafc3e5cd5e3573cda04dcf7 Mon Sep 17 00:00:00 2001 From: Joe Steele Date: Sun, 25 Aug 2013 15:43:36 -0400 Subject: [PATCH] Fix erroneous SSL certificate warnings If you attempted to use SSL to connect to a server that speaks STARTTLS, you should get an SSL protocol error. Instead, you were likely to get an "Unrecognized Certificate" error that shows you an unrelated certificate chain and asks you to accept it or reject it. Neither action would work because the actual problem had nothing to do with certificates. The unrelated certificate chain that popped up had been statically stored when validating a prior connection to a different server. With this patch, certificate chains are no longer stored statically when validating server connections. Issue 5886 is an example of a user experiencing this problem. --- .../setup/AccountSetupCheckSettings.java | 11 +++--- .../k9/mail/CertificateChainException.java | 34 +++++++++++++++++++ .../mail/CertificateValidationException.java | 18 ++++++++++ .../k9/mail/store/TrustManagerFactory.java | 26 ++++++-------- 4 files changed, 69 insertions(+), 20 deletions(-) create mode 100644 src/com/fsck/k9/mail/CertificateChainException.java diff --git a/src/com/fsck/k9/activity/setup/AccountSetupCheckSettings.java b/src/com/fsck/k9/activity/setup/AccountSetupCheckSettings.java index a92461e26..136d32717 100644 --- a/src/com/fsck/k9/activity/setup/AccountSetupCheckSettings.java +++ b/src/com/fsck/k9/activity/setup/AccountSetupCheckSettings.java @@ -159,11 +159,12 @@ public class AccountSetupCheckSettings extends K9Activity implements OnClickList } catch (final CertificateValidationException cve) { Log.e(K9.LOG_TAG, "Error while testing settings", cve); + X509Certificate[] chain = cve.getCertChain(); // Avoid NullPointerException in acceptKeyDialog() - if (TrustManagerFactory.getLastCertChain() != null) { + if (chain != null) { acceptKeyDialog( R.string.account_setup_failed_dlg_certificate_message_fmt, - cve); + cve, chain); } else { showErrorDialog( R.string.account_setup_failed_dlg_server_message_fmt, @@ -232,16 +233,16 @@ public class AccountSetupCheckSettings extends K9Activity implements OnClickList } }); } - private void acceptKeyDialog(final int msgResId, final Object... args) { + + private void acceptKeyDialog(final int msgResId, + final CertificateValidationException ex, final X509Certificate[] chain) { mHandler.post(new Runnable() { public void run() { if (mDestroyed) { return; } - final X509Certificate[] chain = TrustManagerFactory.getLastCertChain(); String exMessage = "Unknown Error"; - Exception ex = ((Exception)args[0]); if (ex != null) { if (ex.getCause() != null) { if (ex.getCause().getCause() != null) { diff --git a/src/com/fsck/k9/mail/CertificateChainException.java b/src/com/fsck/k9/mail/CertificateChainException.java new file mode 100644 index 000000000..f7ce58f4f --- /dev/null +++ b/src/com/fsck/k9/mail/CertificateChainException.java @@ -0,0 +1,34 @@ +package com.fsck.k9.mail; + +import java.security.cert.CertificateException; +import java.security.cert.X509Certificate; + +/** + * A {@link CertificateException} extension that provides access to + * the pertinent certificate chain. + * + */ +public class CertificateChainException extends CertificateException { + + private static final long serialVersionUID = 1103894512106650107L; + private X509Certificate[] mCertChain; + + public CertificateChainException(String msg, X509Certificate[] chain) { + super(msg); + setCertChain(chain); + } + + public CertificateChainException(CertificateException ce, + X509Certificate[] chain) { + super.initCause(ce); + setCertChain(chain); + } + + public void setCertChain(X509Certificate[] chain) { + mCertChain = chain; + } + public X509Certificate[] getCertChain() { + return mCertChain; + } + +} diff --git a/src/com/fsck/k9/mail/CertificateValidationException.java b/src/com/fsck/k9/mail/CertificateValidationException.java index 2b25dd3d0..d1e9b7c8e 100644 --- a/src/com/fsck/k9/mail/CertificateValidationException.java +++ b/src/com/fsck/k9/mail/CertificateValidationException.java @@ -3,9 +3,11 @@ package com.fsck.k9.mail; import java.security.cert.CertPathValidatorException; import java.security.cert.CertificateException; +import java.security.cert.X509Certificate; public class CertificateValidationException extends MessagingException { public static final long serialVersionUID = -1; + private X509Certificate[] mCertChain; public CertificateValidationException(String message) { super(message); @@ -25,6 +27,22 @@ public class CertificateValidationException extends MessagingException { throwable = throwable.getCause(); } + if (throwable instanceof CertificateChainException) { + mCertChain = ((CertificateChainException) throwable).getCertChain(); + } return throwable != null; } + + /** + * If the cause of this {@link CertificateValidationException} was a + * {@link CertificateChainException}, then the offending chain is available + * for return. + * + * @return An {@link X509Certificate X509Certificate[]} containing the Cert. + * chain, or else null. + */ + public X509Certificate[] getCertChain() { + needsUserAttention(); + return mCertChain; + } } \ No newline at end of file diff --git a/src/com/fsck/k9/mail/store/TrustManagerFactory.java b/src/com/fsck/k9/mail/store/TrustManagerFactory.java index f6a907080..7b508c3bb 100644 --- a/src/com/fsck/k9/mail/store/TrustManagerFactory.java +++ b/src/com/fsck/k9/mail/store/TrustManagerFactory.java @@ -6,6 +6,8 @@ import android.content.Context; import android.util.Log; import com.fsck.k9.K9; import com.fsck.k9.helper.DomainNameChecker; +import com.fsck.k9.mail.CertificateChainException; + import org.apache.commons.io.IOUtils; import javax.net.ssl.TrustManager; @@ -28,8 +30,6 @@ public final class TrustManagerFactory { private static X509TrustManager unsecureTrustManager; private static X509TrustManager localTrustManager; - private static X509Certificate[] lastCertChain = null; - private static File keyStoreFile; private static KeyStore keyStore; @@ -77,13 +77,15 @@ public final class TrustManagerFactory { public void checkServerTrusted(X509Certificate[] chain, String authType) throws CertificateException { - // FIXME: Using a static field to store the certificate chain is a bad idea. Instead - // create a CertificateException subclass and store the chain there. - TrustManagerFactory.setLastCertChain(chain); try { defaultTrustManager.checkServerTrusted(chain, authType); } catch (CertificateException e) { - localTrustManager.checkServerTrusted(new X509Certificate[] {chain[0]}, authType); + try { + localTrustManager.checkServerTrusted( + new X509Certificate[] { chain[0] }, authType); + } catch (CertificateException ce) { + throw new CertificateChainException(ce, chain); + } } if (!DomainNameChecker.match(chain[0], mHost)) { try { @@ -94,8 +96,9 @@ public final class TrustManagerFactory { } catch (KeyStoreException e) { throw new CertificateException("Certificate cannot be verified; KeyStore Exception: " + e); } - throw new CertificateException("Certificate domain name does not match " - + mHost); + throw new CertificateChainException( + "Certificate domain name does not match " + mHost, + chain); } } @@ -170,13 +173,6 @@ public final class TrustManagerFactory { return keyStore; } - public static void setLastCertChain(X509Certificate[] chain) { - lastCertChain = chain; - } - public static X509Certificate[] getLastCertChain() { - return lastCertChain; - } - public static void addCertificateChain(String alias, X509Certificate[] chain) throws CertificateException { try { javax.net.ssl.TrustManagerFactory tmf = javax.net.ssl.TrustManagerFactory.getInstance("X509");