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.

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,6 +269,12 @@ public class AccountSetupBasics extends K9Activity
     @Override
     public void onActivityResult(int requestCode, int resultCode, Intent data) {
         if (resultCode == RESULT_OK) {
+            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);
@@ -273,6 +282,7 @@ public class AccountSetupBasics extends K9Activity
                 finish();
             }
         }
+    }
 
     private void onManualSetup() {
         String email = mEmailView.getText().toString();

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 CheckDirection mDirection;
-
-    private boolean mCheckOutgoing;
 
     private boolean mCanceled;
 
     private boolean mDestroyed;
 
-    public static void actionCheckSettings(Activity context, Account account,
+    public static void actionCheckSettings(Activity context, Account account, CheckDirection direction) {
-                                           boolean checkIncoming, boolean checkOutgoing) {
         Intent i = new Intent(context, AccountSetupCheckSettings.class);
         i.putExtra(EXTRA_ACCOUNT, account.getUuid());
-        i.putExtra(EXTRA_CHECK_INCOMING, checkIncoming);
+        i.putExtra(EXTRA_CHECK_DIRECTION, direction);
-        i.putExtra(EXTRA_CHECK_OUTGOING, checkOutgoing);
         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);
+        mDirection = (CheckDirection) getIntent().getSerializableExtra(EXTRA_CHECK_DIRECTION);
-        mCheckOutgoing = getIntent().getBooleanExtra(EXTRA_CHECK_OUTGOING, false);
 
         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();
+                            Uri uri = null;
-                            if (mCheckIncoming) {
+                            if (mDirection.equals(CheckDirection.INCOMING)) {
-                                alias = alias + ".incoming";
+                                uri = Uri.parse(mAccount.getStoreUri());
+                            } else {
+                                uri = Uri.parse(mAccount.getTransportUri());
                             }
-                            if (mCheckOutgoing) {
+                            TrustManagerFactory.addCertificate(uri.getHost(), uri.getPort(), chain[0]);
-                                alias = alias + ".outgoing";
-                            }
-                            TrustManagerFactory.addCertificateChain(alias, chain);
                         } 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(

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

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);

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());
-        }
+        } else {
-        if (outgoing) {
             nm.cancel(null, K9.CERTIFICATE_EXCEPTION_NOTIFICATION_OUTGOING + account.getAccountNumber());
         }
     }

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[] {
+                            sslContext
-                                                TrustManagerFactory.get(mSettings.getHost(), secure)
+                                    .init(null,
-                                            }, new SecureRandom());
+                                            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[] {
+                        sslContext.init(null,
-                                            TrustManagerFactory.get(mSettings.getHost(), secure)
+                                new TrustManager[] { TrustManagerFactory.get(
-                                        }, new SecureRandom());
+                                        mSettings.getHost(),
+                                        mSettings.getPort(), secure) },
+                                new SecureRandom());
                         mSocket = TrustedSocketFactory.createSocket(sslContext, mSocket,
                                 mSettings.getHost(), mSettings.getPort(), true);
                         mSocket.setSoTimeout(Store.SOCKET_READ_TIMEOUT);

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[] {
+                    sslContext.init(null,
-                                        TrustManagerFactory.get(mHost, secure)
+                            new TrustManager[] { TrustManagerFactory.get(mHost,
-                                    }, new SecureRandom());
+                                    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[] {
+                        sslContext.init(null,
-                                            TrustManagerFactory.get(mHost, secure)
+                                new TrustManager[] { TrustManagerFactory.get(
-                                        }, new SecureRandom());
+                                        mHost, mPort, secure) },
+                                new SecureRandom());
                         mSocket = TrustedSocketFactory.createSocket(sslContext, mSocket, mHost,
                                 mPort, true);
                         mSocket.setSoTimeout(Store.SOCKET_READ_TIMEOUT);

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<String, SecureX509TrustManager>();
 
         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)) {
+            if (mTrustManager.containsKey(key)) {
-                trustManager = mTrustManager.get(host);
+                trustManager = mTrustManager.get(key);
             } else {
-                trustManager = new SecureX509TrustManager(host);
+                trustManager = new SecureX509TrustManager(host, port);
-                mTrustManager.put(host, trustManager);
+                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();
+                    Certificate storedCert = keyStore
-                    if ((dn != null) && (dn.equalsIgnoreCase(keyStore.getCertificateAlias(chain[0])))) {
+                            .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) {
+    public static X509TrustManager get(String host, int port, boolean secure) {
-        return secure ? SecureX509TrustManager.getInstance(host) :
+        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(getCertKey(host, port), certificate);
-                keyStore.setCertificateEntry
-                (element.getSubjectDN().toString(), element);
-            }
 
             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;
+    }
 }

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();

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);

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[] {
+                        sslContext.init(null,
-                                            TrustManagerFactory.get(mHost, secure)
+                                new TrustManager[] { TrustManagerFactory.get(
-                                        }, new SecureRandom());
+                                        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[] {
+                    sslContext.init(null,
-                                        TrustManagerFactory.get(mHost, secure)
+                            new TrustManager[] { TrustManagerFactory.get(mHost,
-                                    }, new SecureRandom());
+                                    mPort, secure) }, new SecureRandom());
                     mSocket = TrustedSocketFactory.createSocket(sslContext, mSocket, mHost,
                               mPort, true);
                     mIn = new PeekableInputStream(new BufferedInputStream(mSocket.getInputStream(),