From a4440b404234215151617f43c349bae276c544bc Mon Sep 17 00:00:00 2001 From: Joe Steele Date: Sat, 23 Nov 2013 13:26:57 -0500 Subject: [PATCH] Fix inadequate certificate validation Proper host name validation was not being performed for certificates kept in the local keystore. If an attacker could convince a user to accept and store an attacker's certificate, then that certificate could be used for MITM attacks, giving the attacker access to all connections to all servers in all accounts in K-9. This commit changes how the certificates are stored. Previously, an entire certificate chain was stored for a server (and any of those certificates in the chain were available for validating signatures on certificates received when connecting). Now just the single certificate for the server is stored. This commit changes how locally stored certificates are retrieved. They can only be retrieved using the host:port that the user configured for the server. This also fixes issue 1326. Users can now use different certificates for different servers on the same host (listening to different ports). The above changes mean that users might have to re-accept certificates that they had previously accepted and are still using (but only if the certificate's Subject doesn't match the host that they are connecting to). This commit modifies AccountSetupBasics so that it now calls AccountSetupCheckSettings twice -- once for checking the incoming settings and once for the outgoing settings. Otherwise, an exception could occur while checking incoming settings, the user could say continue (or the user could accept a certificate key), and the outgoing settings would not be checked. This also helps with determining if a certificate exception was for the incoming or outgoing server, which is needed if the user decides to add the certificate to the keystore. --- .../k9/activity/setup/AccountSetupBasics.java | 22 +++++++--- .../setup/AccountSetupCheckSettings.java | 41 +++++++++---------- .../activity/setup/AccountSetupIncoming.java | 3 +- .../activity/setup/AccountSetupOutgoing.java | 5 ++- .../k9/controller/MessagingController.java | 8 ++-- src/com/fsck/k9/mail/store/ImapStore.java | 17 +++++--- src/com/fsck/k9/mail/store/Pop3Store.java | 13 +++--- .../k9/mail/store/TrustManagerFactory.java | 36 +++++++++------- .../k9/mail/store/WebDavSocketFactory.java | 4 +- src/com/fsck/k9/mail/store/WebDavStore.java | 2 +- .../fsck/k9/mail/transport/SmtpTransport.java | 13 +++--- 11 files changed, 93 insertions(+), 71 deletions(-) diff --git a/src/com/fsck/k9/activity/setup/AccountSetupBasics.java b/src/com/fsck/k9/activity/setup/AccountSetupBasics.java index 435a35d24..f5f106c64 100644 --- a/src/com/fsck/k9/activity/setup/AccountSetupBasics.java +++ b/src/com/fsck/k9/activity/setup/AccountSetupBasics.java @@ -17,6 +17,7 @@ import android.widget.Button; import android.widget.EditText; import com.fsck.k9.*; import com.fsck.k9.activity.K9Activity; +import com.fsck.k9.activity.setup.AccountSetupCheckSettings.CheckDirection; import com.fsck.k9.helper.Utility; import java.io.Serializable; import java.io.UnsupportedEncodingException; @@ -46,6 +47,7 @@ public class AccountSetupBasics extends K9Activity private Provider mProvider; private EmailAddressValidator mEmailValidator = new EmailAddressValidator(); + private boolean mCheckedIncoming = false; public static void actionNewAccount(Context context) { Intent i = new Intent(context, AccountSetupBasics.class); @@ -229,7 +231,8 @@ public class AccountSetupBasics extends K9Activity } else if (incomingUri.toString().startsWith("pop3")) { mAccount.setDeletePolicy(Account.DELETE_POLICY_NEVER); } - AccountSetupCheckSettings.actionCheckSettings(this, mAccount, true, true); + // Check incoming here. Then check outgoing in onActivityResult() + AccountSetupCheckSettings.actionCheckSettings(this, mAccount, CheckDirection.INCOMING); } catch (UnsupportedEncodingException enc) { // This really shouldn't happen since the encoding is hardcoded to UTF-8 Log.e(K9.LOG_TAG, "Couldn't urlencode username or password.", enc); @@ -266,11 +269,18 @@ public class AccountSetupBasics extends K9Activity @Override public void onActivityResult(int requestCode, int resultCode, Intent data) { if (resultCode == RESULT_OK) { - mAccount.setDescription(mAccount.getEmail()); - mAccount.save(Preferences.getPreferences(this)); - K9.setServicesEnabled(this); - AccountSetupNames.actionSetNames(this, mAccount); - finish(); + if (!mCheckedIncoming) { + //We've successfully checked incoming. Now check outgoing. + mCheckedIncoming = true; + AccountSetupCheckSettings.actionCheckSettings(this, mAccount, CheckDirection.OUTGOING); + } else { + //We've successfully checked outgoing as well. + mAccount.setDescription(mAccount.getEmail()); + mAccount.save(Preferences.getPreferences(this)); + K9.setServicesEnabled(this); + AccountSetupNames.actionSetNames(this, mAccount); + finish(); + } } } diff --git a/src/com/fsck/k9/activity/setup/AccountSetupCheckSettings.java b/src/com/fsck/k9/activity/setup/AccountSetupCheckSettings.java index 5941f7369..6e7fd88cd 100644 --- a/src/com/fsck/k9/activity/setup/AccountSetupCheckSettings.java +++ b/src/com/fsck/k9/activity/setup/AccountSetupCheckSettings.java @@ -47,9 +47,12 @@ public class AccountSetupCheckSettings extends K9Activity implements OnClickList private static final String EXTRA_ACCOUNT = "account"; - private static final String EXTRA_CHECK_INCOMING = "checkIncoming"; + private static final String EXTRA_CHECK_DIRECTION ="checkDirection"; - private static final String EXTRA_CHECK_OUTGOING = "checkOutgoing"; + public enum CheckDirection { + INCOMING, + OUTGOING + } private Handler mHandler = new Handler(); @@ -59,20 +62,16 @@ public class AccountSetupCheckSettings extends K9Activity implements OnClickList private Account mAccount; - private boolean mCheckIncoming; - - private boolean mCheckOutgoing; + private CheckDirection mDirection; private boolean mCanceled; private boolean mDestroyed; - public static void actionCheckSettings(Activity context, Account account, - boolean checkIncoming, boolean checkOutgoing) { + public static void actionCheckSettings(Activity context, Account account, CheckDirection direction) { Intent i = new Intent(context, AccountSetupCheckSettings.class); i.putExtra(EXTRA_ACCOUNT, account.getUuid()); - i.putExtra(EXTRA_CHECK_INCOMING, checkIncoming); - i.putExtra(EXTRA_CHECK_OUTGOING, checkOutgoing); + i.putExtra(EXTRA_CHECK_DIRECTION, direction); context.startActivityForResult(i, ACTIVITY_REQUEST_CODE); } @@ -89,8 +88,7 @@ public class AccountSetupCheckSettings extends K9Activity implements OnClickList String accountUuid = getIntent().getStringExtra(EXTRA_ACCOUNT); mAccount = Preferences.getPreferences(this).getAccount(accountUuid); - mCheckIncoming = getIntent().getBooleanExtra(EXTRA_CHECK_INCOMING, false); - mCheckOutgoing = getIntent().getBooleanExtra(EXTRA_CHECK_OUTGOING, false); + mDirection = (CheckDirection) getIntent().getSerializableExtra(EXTRA_CHECK_DIRECTION); new Thread() { @Override @@ -108,9 +106,9 @@ public class AccountSetupCheckSettings extends K9Activity implements OnClickList final MessagingController ctrl = MessagingController.getInstance(getApplication()); ctrl.clearCertificateErrorNotifications(AccountSetupCheckSettings.this, - mAccount, mCheckIncoming, mCheckOutgoing); + mAccount, mDirection); - if (mCheckIncoming) { + if (mDirection.equals(CheckDirection.INCOMING)) { store = mAccount.getRemoteStore(); if (store instanceof WebDavStore) { @@ -133,7 +131,7 @@ public class AccountSetupCheckSettings extends K9Activity implements OnClickList finish(); return; } - if (mCheckOutgoing) { + if (mDirection.equals(CheckDirection.OUTGOING)) { if (!(mAccount.getRemoteStore() instanceof WebDavStore)) { setMessage(R.string.account_setup_check_settings_check_outgoing_msg); } @@ -366,21 +364,20 @@ public class AccountSetupCheckSettings extends K9Activity implements OnClickList new DialogInterface.OnClickListener() { public void onClick(DialogInterface dialog, int which) { try { - String alias = mAccount.getUuid(); - if (mCheckIncoming) { - alias = alias + ".incoming"; + Uri uri = null; + if (mDirection.equals(CheckDirection.INCOMING)) { + uri = Uri.parse(mAccount.getStoreUri()); + } else { + uri = Uri.parse(mAccount.getTransportUri()); } - if (mCheckOutgoing) { - alias = alias + ".outgoing"; - } - TrustManagerFactory.addCertificateChain(alias, chain); + TrustManagerFactory.addCertificate(uri.getHost(), uri.getPort(), chain[0]); } catch (CertificateException e) { showErrorDialog( R.string.account_setup_failed_dlg_certificate_message_fmt, e.getMessage() == null ? "" : e.getMessage()); } AccountSetupCheckSettings.actionCheckSettings(AccountSetupCheckSettings.this, mAccount, - mCheckIncoming, mCheckOutgoing); + mDirection); } }) .setNegativeButton( diff --git a/src/com/fsck/k9/activity/setup/AccountSetupIncoming.java b/src/com/fsck/k9/activity/setup/AccountSetupIncoming.java index c76b6289f..4ad0a2cf9 100644 --- a/src/com/fsck/k9/activity/setup/AccountSetupIncoming.java +++ b/src/com/fsck/k9/activity/setup/AccountSetupIncoming.java @@ -16,6 +16,7 @@ import android.widget.CompoundButton.OnCheckedChangeListener; import com.fsck.k9.*; import com.fsck.k9.activity.K9Activity; +import com.fsck.k9.activity.setup.AccountSetupCheckSettings.CheckDirection; import com.fsck.k9.helper.Utility; import com.fsck.k9.mail.ConnectionSecurity; import com.fsck.k9.mail.ServerSettings; @@ -437,7 +438,7 @@ public class AccountSetupIncoming extends K9Activity implements OnClickListener mAccount.setCompression(Account.TYPE_OTHER, mCompressionOther.isChecked()); mAccount.setSubscribedFoldersOnly(mSubscribedFoldersOnly.isChecked()); - AccountSetupCheckSettings.actionCheckSettings(this, mAccount, true, false); + AccountSetupCheckSettings.actionCheckSettings(this, mAccount, CheckDirection.INCOMING); } catch (Exception e) { failure(e); } diff --git a/src/com/fsck/k9/activity/setup/AccountSetupOutgoing.java b/src/com/fsck/k9/activity/setup/AccountSetupOutgoing.java index 6f71545fe..460c5bdb4 100644 --- a/src/com/fsck/k9/activity/setup/AccountSetupOutgoing.java +++ b/src/com/fsck/k9/activity/setup/AccountSetupOutgoing.java @@ -15,6 +15,7 @@ import android.widget.*; import android.widget.CompoundButton.OnCheckedChangeListener; import com.fsck.k9.*; import com.fsck.k9.activity.K9Activity; +import com.fsck.k9.activity.setup.AccountSetupCheckSettings.CheckDirection; import com.fsck.k9.helper.Utility; import com.fsck.k9.mail.transport.SmtpTransport; @@ -95,7 +96,7 @@ public class AccountSetupOutgoing extends K9Activity implements OnClickListener, try { if (new URI(mAccount.getStoreUri()).getScheme().startsWith("webdav")) { mAccount.setTransportUri(mAccount.getStoreUri()); - AccountSetupCheckSettings.actionCheckSettings(this, mAccount, false, true); + AccountSetupCheckSettings.actionCheckSettings(this, mAccount, CheckDirection.OUTGOING); } } catch (URISyntaxException e) { // TODO Auto-generated catch block @@ -311,7 +312,7 @@ public class AccountSetupOutgoing extends K9Activity implements OnClickListener, uri = new URI(smtpSchemes[securityType], userInfo, mServerView.getText().toString(), Integer.parseInt(mPortView.getText().toString()), null, null, null); mAccount.setTransportUri(uri.toString()); - AccountSetupCheckSettings.actionCheckSettings(this, mAccount, false, true); + AccountSetupCheckSettings.actionCheckSettings(this, mAccount, CheckDirection.OUTGOING); } catch (UnsupportedEncodingException enc) { // This really shouldn't happen since the encoding is hardcoded to UTF-8 Log.e(K9.LOG_TAG, "Couldn't urlencode username or password.", enc); diff --git a/src/com/fsck/k9/controller/MessagingController.java b/src/com/fsck/k9/controller/MessagingController.java index 33c457353..7de35d667 100644 --- a/src/com/fsck/k9/controller/MessagingController.java +++ b/src/com/fsck/k9/controller/MessagingController.java @@ -53,6 +53,7 @@ import com.fsck.k9.activity.FolderList; import com.fsck.k9.activity.MessageList; import com.fsck.k9.activity.MessageReference; import com.fsck.k9.activity.NotificationDeleteConfirmation; +import com.fsck.k9.activity.setup.AccountSetupCheckSettings.CheckDirection; import com.fsck.k9.activity.setup.AccountSetupIncoming; import com.fsck.k9.activity.setup.AccountSetupOutgoing; import com.fsck.k9.cache.EmailProviderCache; @@ -2671,14 +2672,13 @@ public class MessagingController implements Runnable { } public void clearCertificateErrorNotifications(Context context, - final Account account, boolean incoming, boolean outgoing) { + final Account account, CheckDirection direction) { final NotificationManager nm = (NotificationManager) context.getSystemService(Context.NOTIFICATION_SERVICE); - if (incoming) { + if (direction.equals(CheckDirection.INCOMING)) { nm.cancel(null, K9.CERTIFICATE_EXCEPTION_NOTIFICATION_INCOMING + account.getAccountNumber()); - } - if (outgoing) { + } else { nm.cancel(null, K9.CERTIFICATE_EXCEPTION_NOTIFICATION_OUTGOING + account.getAccountNumber()); } } diff --git a/src/com/fsck/k9/mail/store/ImapStore.java b/src/com/fsck/k9/mail/store/ImapStore.java index a71a95aaf..8a0bbf14a 100644 --- a/src/com/fsck/k9/mail/store/ImapStore.java +++ b/src/com/fsck/k9/mail/store/ImapStore.java @@ -2446,9 +2446,12 @@ public class ImapStore extends Store { connectionSecurity == CONNECTION_SECURITY_SSL_OPTIONAL) { SSLContext sslContext = SSLContext.getInstance("TLS"); boolean secure = connectionSecurity == CONNECTION_SECURITY_SSL_REQUIRED; - sslContext.init(null, new TrustManager[] { - TrustManagerFactory.get(mSettings.getHost(), secure) - }, new SecureRandom()); + sslContext + .init(null, + new TrustManager[] { TrustManagerFactory.get( + mSettings.getHost(), + mSettings.getPort(), secure) }, + new SecureRandom()); mSocket = TrustedSocketFactory.createSocket(sslContext); } else { mSocket = new Socket(); @@ -2501,9 +2504,11 @@ public class ImapStore extends Store { SSLContext sslContext = SSLContext.getInstance("TLS"); boolean secure = mSettings.getConnectionSecurity() == CONNECTION_SECURITY_TLS_REQUIRED; - sslContext.init(null, new TrustManager[] { - TrustManagerFactory.get(mSettings.getHost(), secure) - }, new SecureRandom()); + sslContext.init(null, + new TrustManager[] { TrustManagerFactory.get( + mSettings.getHost(), + mSettings.getPort(), secure) }, + new SecureRandom()); mSocket = TrustedSocketFactory.createSocket(sslContext, mSocket, mSettings.getHost(), mSettings.getPort(), true); mSocket.setSoTimeout(Store.SOCKET_READ_TIMEOUT); diff --git a/src/com/fsck/k9/mail/store/Pop3Store.java b/src/com/fsck/k9/mail/store/Pop3Store.java index 26e37d1dd..e4138452b 100644 --- a/src/com/fsck/k9/mail/store/Pop3Store.java +++ b/src/com/fsck/k9/mail/store/Pop3Store.java @@ -327,9 +327,9 @@ public class Pop3Store extends Store { mConnectionSecurity == CONNECTION_SECURITY_SSL_OPTIONAL) { SSLContext sslContext = SSLContext.getInstance("TLS"); final boolean secure = mConnectionSecurity == CONNECTION_SECURITY_SSL_REQUIRED; - sslContext.init(null, new TrustManager[] { - TrustManagerFactory.get(mHost, secure) - }, new SecureRandom()); + sslContext.init(null, + new TrustManager[] { TrustManagerFactory.get(mHost, + mPort, secure) }, new SecureRandom()); mSocket = TrustedSocketFactory.createSocket(sslContext); } else { mSocket = new Socket(); @@ -356,9 +356,10 @@ public class Pop3Store extends Store { SSLContext sslContext = SSLContext.getInstance("TLS"); boolean secure = mConnectionSecurity == CONNECTION_SECURITY_TLS_REQUIRED; - sslContext.init(null, new TrustManager[] { - TrustManagerFactory.get(mHost, secure) - }, new SecureRandom()); + sslContext.init(null, + new TrustManager[] { TrustManagerFactory.get( + mHost, mPort, secure) }, + new SecureRandom()); mSocket = TrustedSocketFactory.createSocket(sslContext, mSocket, mHost, mPort, true); mSocket.setSoTimeout(Store.SOCKET_READ_TIMEOUT); diff --git a/src/com/fsck/k9/mail/store/TrustManagerFactory.java b/src/com/fsck/k9/mail/store/TrustManagerFactory.java index 7b508c3bb..37c9f32dc 100644 --- a/src/com/fsck/k9/mail/store/TrustManagerFactory.java +++ b/src/com/fsck/k9/mail/store/TrustManagerFactory.java @@ -18,6 +18,7 @@ import java.io.IOException; import java.security.KeyStore; import java.security.KeyStoreException; import java.security.NoSuchAlgorithmException; +import java.security.cert.Certificate; import java.security.cert.CertificateException; import java.security.cert.X509Certificate; import java.util.HashMap; @@ -53,18 +54,21 @@ public final class TrustManagerFactory { new HashMap(); private final String mHost; + private final int mPort; - private SecureX509TrustManager(String host) { + private SecureX509TrustManager(String host, int port) { mHost = host; + mPort = port; } - public synchronized static X509TrustManager getInstance(String host) { + public synchronized static X509TrustManager getInstance(String host, int port) { + String key = getCertKey(host, port); SecureX509TrustManager trustManager; - if (mTrustManager.containsKey(host)) { - trustManager = mTrustManager.get(host); + if (mTrustManager.containsKey(key)) { + trustManager = mTrustManager.get(key); } else { - trustManager = new SecureX509TrustManager(host); - mTrustManager.put(host, trustManager); + trustManager = new SecureX509TrustManager(host, port); + mTrustManager.put(key, trustManager); } return trustManager; @@ -89,8 +93,9 @@ public final class TrustManagerFactory { } if (!DomainNameChecker.match(chain[0], mHost)) { try { - String dn = chain[0].getSubjectDN().toString(); - if ((dn != null) && (dn.equalsIgnoreCase(keyStore.getCertificateAlias(chain[0])))) { + Certificate storedCert = keyStore + .getCertificate(getCertKey(mHost, mPort)); + if (storedCert != null && storedCert.equals(chain[0])) { return; } } catch (KeyStoreException e) { @@ -164,8 +169,8 @@ public final class TrustManagerFactory { private TrustManagerFactory() { } - public static X509TrustManager get(String host, boolean secure) { - return secure ? SecureX509TrustManager.getInstance(host) : + public static X509TrustManager get(String host, int port, boolean secure) { + return secure ? SecureX509TrustManager.getInstance(host, port) : unsecureTrustManager; } @@ -173,13 +178,10 @@ public final class TrustManagerFactory { return keyStore; } - public static void addCertificateChain(String alias, X509Certificate[] chain) throws CertificateException { + public static void addCertificate(String host, int port, X509Certificate certificate) throws CertificateException { try { javax.net.ssl.TrustManagerFactory tmf = javax.net.ssl.TrustManagerFactory.getInstance("X509"); - for (X509Certificate element : chain) { - keyStore.setCertificateEntry - (element.getSubjectDN().toString(), element); - } + keyStore.setCertificateEntry(getCertKey(host, port), certificate); tmf.init(keyStore); TrustManager[] tms = tmf.getTrustManagers(); @@ -211,4 +213,8 @@ public final class TrustManagerFactory { Log.e(LOG_TAG, "Key Store exception while initializing TrustManagerFactory ", e); } } + + private static String getCertKey(String host, int port) { + return host + ":" + port; + } } diff --git a/src/com/fsck/k9/mail/store/WebDavSocketFactory.java b/src/com/fsck/k9/mail/store/WebDavSocketFactory.java index 2d4f959ed..b465dcf38 100644 --- a/src/com/fsck/k9/mail/store/WebDavSocketFactory.java +++ b/src/com/fsck/k9/mail/store/WebDavSocketFactory.java @@ -26,10 +26,10 @@ public class WebDavSocketFactory implements LayeredSocketFactory { private SSLSocketFactory mSocketFactory; private org.apache.http.conn.ssl.SSLSocketFactory mSchemeSocketFactory; - public WebDavSocketFactory(String host, boolean secure) throws NoSuchAlgorithmException, KeyManagementException { + public WebDavSocketFactory(String host, int port, boolean secure) throws NoSuchAlgorithmException, KeyManagementException { SSLContext sslContext = SSLContext.getInstance("TLS"); sslContext.init(null, new TrustManager[] { - TrustManagerFactory.get(host, secure) + TrustManagerFactory.get(host, port, secure) }, new SecureRandom()); mSocketFactory = sslContext.getSocketFactory(); mSchemeSocketFactory = org.apache.http.conn.ssl.SSLSocketFactory.getSocketFactory(); diff --git a/src/com/fsck/k9/mail/store/WebDavStore.java b/src/com/fsck/k9/mail/store/WebDavStore.java index b3f8d5f24..dd8b399ed 100644 --- a/src/com/fsck/k9/mail/store/WebDavStore.java +++ b/src/com/fsck/k9/mail/store/WebDavStore.java @@ -1079,7 +1079,7 @@ public class WebDavStore extends Store { SchemeRegistry reg = mHttpClient.getConnectionManager().getSchemeRegistry(); try { - Scheme s = new Scheme("https", new WebDavSocketFactory(mHost, mSecure), 443); + Scheme s = new Scheme("https", new WebDavSocketFactory(mHost, 443, mSecure), 443); reg.register(s); } catch (NoSuchAlgorithmException nsa) { Log.e(K9.LOG_TAG, "NoSuchAlgorithmException in getHttpClient: " + nsa); diff --git a/src/com/fsck/k9/mail/transport/SmtpTransport.java b/src/com/fsck/k9/mail/transport/SmtpTransport.java index daf147326..33602a37d 100644 --- a/src/com/fsck/k9/mail/transport/SmtpTransport.java +++ b/src/com/fsck/k9/mail/transport/SmtpTransport.java @@ -242,9 +242,10 @@ public class SmtpTransport extends Transport { mConnectionSecurity == CONNECTION_SECURITY_SSL_OPTIONAL) { SSLContext sslContext = SSLContext.getInstance("TLS"); boolean secure = mConnectionSecurity == CONNECTION_SECURITY_SSL_REQUIRED; - sslContext.init(null, new TrustManager[] { - TrustManagerFactory.get(mHost, secure) - }, new SecureRandom()); + sslContext.init(null, + new TrustManager[] { TrustManagerFactory.get( + mHost, mPort, secure) }, + new SecureRandom()); mSocket = TrustedSocketFactory.createSocket(sslContext); mSocket.connect(socketAddress, SOCKET_CONNECT_TIMEOUT); } else { @@ -301,9 +302,9 @@ public class SmtpTransport extends Transport { SSLContext sslContext = SSLContext.getInstance("TLS"); boolean secure = mConnectionSecurity == CONNECTION_SECURITY_TLS_REQUIRED; - sslContext.init(null, new TrustManager[] { - TrustManagerFactory.get(mHost, secure) - }, new SecureRandom()); + sslContext.init(null, + new TrustManager[] { TrustManagerFactory.get(mHost, + mPort, secure) }, new SecureRandom()); mSocket = TrustedSocketFactory.createSocket(sslContext, mSocket, mHost, mPort, true); mIn = new PeekableInputStream(new BufferedInputStream(mSocket.getInputStream(),