1
0
mirror of https://github.com/moparisthebest/k-9 synced 2024-11-27 11:42:16 -05:00

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.
This commit is contained in:
Joe Steele 2013-08-25 15:43:36 -04:00
parent 7aeaa46fe6
commit 604aa87ccf
4 changed files with 69 additions and 20 deletions

View File

@ -159,11 +159,12 @@ public class AccountSetupCheckSettings extends K9Activity implements OnClickList
} catch (final CertificateValidationException cve) { } catch (final CertificateValidationException cve) {
Log.e(K9.LOG_TAG, "Error while testing settings", cve); Log.e(K9.LOG_TAG, "Error while testing settings", cve);
X509Certificate[] chain = cve.getCertChain();
// Avoid NullPointerException in acceptKeyDialog() // Avoid NullPointerException in acceptKeyDialog()
if (TrustManagerFactory.getLastCertChain() != null) { if (chain != null) {
acceptKeyDialog( acceptKeyDialog(
R.string.account_setup_failed_dlg_certificate_message_fmt, R.string.account_setup_failed_dlg_certificate_message_fmt,
cve); cve, chain);
} else { } else {
showErrorDialog( showErrorDialog(
R.string.account_setup_failed_dlg_server_message_fmt, 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() { mHandler.post(new Runnable() {
public void run() { public void run() {
if (mDestroyed) { if (mDestroyed) {
return; return;
} }
final X509Certificate[] chain = TrustManagerFactory.getLastCertChain();
String exMessage = "Unknown Error"; String exMessage = "Unknown Error";
Exception ex = ((Exception)args[0]);
if (ex != null) { if (ex != null) {
if (ex.getCause() != null) { if (ex.getCause() != null) {
if (ex.getCause().getCause() != null) { if (ex.getCause().getCause() != null) {

View File

@ -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;
}
}

View File

@ -3,9 +3,11 @@ package com.fsck.k9.mail;
import java.security.cert.CertPathValidatorException; import java.security.cert.CertPathValidatorException;
import java.security.cert.CertificateException; import java.security.cert.CertificateException;
import java.security.cert.X509Certificate;
public class CertificateValidationException extends MessagingException { public class CertificateValidationException extends MessagingException {
public static final long serialVersionUID = -1; public static final long serialVersionUID = -1;
private X509Certificate[] mCertChain;
public CertificateValidationException(String message) { public CertificateValidationException(String message) {
super(message); super(message);
@ -25,6 +27,22 @@ public class CertificateValidationException extends MessagingException {
throwable = throwable.getCause(); throwable = throwable.getCause();
} }
if (throwable instanceof CertificateChainException) {
mCertChain = ((CertificateChainException) throwable).getCertChain();
}
return throwable != null; 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;
}
} }

View File

@ -6,6 +6,8 @@ import android.content.Context;
import android.util.Log; import android.util.Log;
import com.fsck.k9.K9; import com.fsck.k9.K9;
import com.fsck.k9.helper.DomainNameChecker; import com.fsck.k9.helper.DomainNameChecker;
import com.fsck.k9.mail.CertificateChainException;
import org.apache.commons.io.IOUtils; import org.apache.commons.io.IOUtils;
import javax.net.ssl.TrustManager; import javax.net.ssl.TrustManager;
@ -28,8 +30,6 @@ public final class TrustManagerFactory {
private static X509TrustManager unsecureTrustManager; private static X509TrustManager unsecureTrustManager;
private static X509TrustManager localTrustManager; private static X509TrustManager localTrustManager;
private static X509Certificate[] lastCertChain = null;
private static File keyStoreFile; private static File keyStoreFile;
private static KeyStore keyStore; private static KeyStore keyStore;
@ -77,13 +77,15 @@ public final class TrustManagerFactory {
public void checkServerTrusted(X509Certificate[] chain, String authType) public void checkServerTrusted(X509Certificate[] chain, String authType)
throws CertificateException { 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 { try {
defaultTrustManager.checkServerTrusted(chain, authType); defaultTrustManager.checkServerTrusted(chain, authType);
} catch (CertificateException e) { } 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)) { if (!DomainNameChecker.match(chain[0], mHost)) {
try { try {
@ -94,8 +96,9 @@ public final class TrustManagerFactory {
} catch (KeyStoreException e) { } catch (KeyStoreException e) {
throw new CertificateException("Certificate cannot be verified; KeyStore Exception: " + e); throw new CertificateException("Certificate cannot be verified; KeyStore Exception: " + e);
} }
throw new CertificateException("Certificate domain name does not match " throw new CertificateChainException(
+ mHost); "Certificate domain name does not match " + mHost,
chain);
} }
} }
@ -170,13 +173,6 @@ public final class TrustManagerFactory {
return keyStore; 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 { public static void addCertificateChain(String alias, X509Certificate[] chain) throws CertificateException {
try { try {
javax.net.ssl.TrustManagerFactory tmf = javax.net.ssl.TrustManagerFactory.getInstance("X509"); javax.net.ssl.TrustManagerFactory tmf = javax.net.ssl.TrustManagerFactory.getInstance("X509");