Always verify hostname/domain

There might be corner cases where it is required to use self signed
certificates. However there should be no corner cases where it is
required to use a wrong domain name. This commit swaps out the
MemorizingHostnameVerifier that let users accept wrong domains with the
standard XmppDomainVerifier.

closes #4066

5 files changed, 18 insertions, 236 deletions

diff --git a/src/main/java/eu/siacs/conversations/entities/Account.java b/src/main/java/eu/siacs/conversations/entities/Account.java
index 969a9c765..0796a1c00 100644
--- a/src/main/java/eu/siacs/conversations/entities/Account.java
+++ b/src/main/java/eu/siacs/conversations/entities/Account.java
@@ -635,6 +635,7 @@ public class Account extends AbstractEntity implements AvatarService.Avatarable
         REGISTRATION_INVALID_TOKEN(true,false),
         REGISTRATION_PASSWORD_TOO_WEAK(true, false),
         TLS_ERROR,
+        TLS_ERROR_DOMAIN,
         INCOMPATIBLE_SERVER,
         TOR_NOT_AVAILABLE,
         DOWNGRADE_ATTACK,
@@ -701,6 +702,8 @@ public class Account extends AbstractEntity implements AvatarService.Avatarable
                     return R.string.account_status_regis_invalid_token;
                 case TLS_ERROR:
                     return R.string.account_status_tls_error;
+                case TLS_ERROR_DOMAIN:
+                    return R.string.account_status_tls_error_domain;
                 case INCOMPATIBLE_SERVER:
                     return R.string.account_status_incompatible_server;
                 case TOR_NOT_AVAILABLE:
diff --git a/src/main/java/eu/siacs/conversations/http/HttpConnectionManager.java b/src/main/java/eu/siacs/conversations/http/HttpConnectionManager.java
index a16242be0..7ee1da466 100644
--- a/src/main/java/eu/siacs/conversations/http/HttpConnectionManager.java
+++ b/src/main/java/eu/siacs/conversations/http/HttpConnectionManager.java
@@ -124,7 +124,6 @@ public class HttpConnectionManager extends AbstractConnectionManager {
 
     private void setupTrustManager(final OkHttpClient.Builder builder, final boolean interactive) {
         final X509TrustManager trustManager;
-        final HostnameVerifier hostnameVerifier = mXmppConnectionService.getMemorizingTrustManager().wrapHostnameVerifier(new StrictHostnameVerifier(), interactive);
         if (interactive) {
             trustManager = mXmppConnectionService.getMemorizingTrustManager().getInteractive();
         } else {
@@ -133,7 +132,7 @@ public class HttpConnectionManager extends AbstractConnectionManager {
         try {
             final SSLSocketFactory sf = new TLSSocketFactory(new X509TrustManager[]{trustManager}, mXmppConnectionService.getRNG());
             builder.sslSocketFactory(sf, trustManager);
-            builder.hostnameVerifier(hostnameVerifier);
+            builder.hostnameVerifier(new StrictHostnameVerifier());
         } catch (final KeyManagementException | NoSuchAlgorithmException ignored) {
         }
     }
diff --git a/src/main/java/eu/siacs/conversations/services/MemorizingTrustManager.java b/src/main/java/eu/siacs/conversations/services/MemorizingTrustManager.java
index e94c1c5c0..2f22911c1 100644
--- a/src/main/java/eu/siacs/conversations/services/MemorizingTrustManager.java
+++ b/src/main/java/eu/siacs/conversations/services/MemorizingTrustManager.java
@@ -48,10 +48,8 @@ import org.json.JSONArray;
 import org.json.JSONException;
 import org.json.JSONObject;
 
-import java.io.BufferedReader;
 import java.io.File;
 import java.io.FileInputStream;
-import java.io.FileNotFoundException;
 import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
@@ -64,26 +62,20 @@ import java.security.cert.Certificate;
 import java.security.cert.CertificateEncodingException;
 import java.security.cert.CertificateException;
 import java.security.cert.CertificateExpiredException;
-import java.security.cert.CertificateParsingException;
 import java.security.cert.X509Certificate;
 import java.text.SimpleDateFormat;
 import java.util.ArrayList;
-import java.util.Collection;
 import java.util.Enumeration;
 import java.util.List;
-import java.util.Locale;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import java.util.regex.Pattern;
 
-import javax.net.ssl.HostnameVerifier;
-import javax.net.ssl.SSLSession;
 import javax.net.ssl.TrustManager;
 import javax.net.ssl.TrustManagerFactory;
 import javax.net.ssl.X509TrustManager;
 
 import eu.siacs.conversations.R;
-import eu.siacs.conversations.crypto.DomainHostnameVerifier;
 import eu.siacs.conversations.entities.MTMDecision;
 import eu.siacs.conversations.http.HttpConnectionManager;
 import eu.siacs.conversations.persistance.FileBackend;
@@ -101,12 +93,10 @@ import eu.siacs.conversations.ui.MemorizingActivity;
  */
 public class MemorizingTrustManager {
 
-
     final static String DECISION_INTENT = "de.duenndns.ssl.DECISION";
     public final static String DECISION_INTENT_ID = DECISION_INTENT + ".decisionId";
     public final static String DECISION_INTENT_CERT = DECISION_INTENT + ".cert";
     public final static String DECISION_TITLE_ID = DECISION_INTENT + ".titleId";
-    final static String DECISION_INTENT_CHOICE = DECISION_INTENT + ".decisionChoice";
     final static String NO_TRUST_ANCHOR = "Trust anchor for certification path not found.";
     private static final Pattern PATTERN_IPV4 = Pattern.compile("\\A(25[0-5]|2[0-4]\\d|[0-1]?\\d?\\d)(\\.(25[0-5]|2[0-4]\\d|[0-1]?\\d?\\d)){3}\\z");
     private static final Pattern PATTERN_IPV6_HEX4DECCOMPRESSED = Pattern.compile("\\A((?:[0-9A-Fa-f]{1,4}(?::[0-9A-Fa-f]{1,4})*)?) ::((?:[0-9A-Fa-f]{1,4}:)*)(25[0-5]|2[0-4]\\d|[0-1]?\\d?\\d)(\\.(25[0-5]|2[0-4]\\d|[0-1]?\\d?\\d)){3}\\z");
@@ -114,7 +104,6 @@ public class MemorizingTrustManager {
     private static final Pattern PATTERN_IPV6_HEXCOMPRESSED = Pattern.compile("\\A((?:[0-9A-Fa-f]{1,4}(?::[0-9A-Fa-f]{1,4})*)?)::((?:[0-9A-Fa-f]{1,4}(?::[0-9A-Fa-f]{1,4})*)?)\\z");
     private static final Pattern PATTERN_IPV6 = Pattern.compile("\\A(?:[0-9a-fA-F]{1,4}:){7}[0-9a-fA-F]{1,4}\\z");
     private final static Logger LOGGER = Logger.getLogger(MemorizingTrustManager.class.getName());
-    private final static int NOTIFICATION_ID = 100509;
     static String KEYSTORE_DIR = "KeyStore";
     static String KEYSTORE_FILE = "KeyStore.bks";
     private static int decisionId = 0;
@@ -168,20 +157,6 @@ public class MemorizingTrustManager {
         this.defaultTrustManager = getTrustManager(null);
     }
 
-    /**
-     * Changes the path for the KeyStore file.
-     * <p>
-     * The actual filename relative to the app's directory will be
-     * <code>app_<i>dirname</i>/<i>filename</i></code>.
-     *
-     * @param dirname  directory to store the KeyStore.
-     * @param filename file name for the KeyStore.
-     */
-    public static void setKeyStoreFile(String dirname, String filename) {
-        KEYSTORE_DIR = dirname;
-        KEYSTORE_FILE = filename;
-    }
-
     private static boolean isIp(final String server) {
         return server != null && (
                 PATTERN_IPV4.matcher(server).matches()
@@ -217,9 +192,7 @@ public class MemorizingTrustManager {
             MessageDigest md = MessageDigest.getInstance(digest);
             md.update(cert.getEncoded());
             return hexString(md.digest());
-        } catch (java.security.cert.CertificateEncodingException e) {
-            return e.getMessage();
-        } catch (java.security.NoSuchAlgorithmException e) {
+        } catch (CertificateEncodingException | NoSuchAlgorithmException e) {
             return e.getMessage();
         }
     }
@@ -240,7 +213,7 @@ public class MemorizingTrustManager {
         }
     }
 
-    void init(Context m) {
+    void init(final Context m) {
         master = m;
         masterHandler = new Handler(m.getMainLooper());
         notificationManager = (NotificationManager) master.getSystemService(Context.NOTIFICATION_SERVICE);
@@ -264,36 +237,6 @@ public class MemorizingTrustManager {
     }
 
     /**
-     * Binds an Activity to the MTM for displaying the query dialog.
-     * <p>
-     * This is useful if your connection is run from a service that is
-     * triggered by user interaction -- in such cases the activity is
-     * visible and the user tends to ignore the service notification.
-     * <p>
-     * You should never have a hidden activity bound to MTM! Use this
-     * function in onResume() and @see unbindDisplayActivity in onPause().
-     *
-     * @param act Activity to be bound
-     */
-    public void bindDisplayActivity(AppCompatActivity act) {
-        foregroundAct = act;
-    }
-
-    /**
-     * Removes an Activity from the MTM display stack.
-     * <p>
-     * Always call this function when the Activity added with
-     * {@link #bindDisplayActivity(AppCompatActivity)} is hidden.
-     *
-     * @param act Activity to be unbound
-     */
-    public void unbindDisplayActivity(AppCompatActivity act) {
-        // do not remove if it was overridden by a different activity
-        if (foregroundAct == act)
-            foregroundAct = null;
-    }
-
-    /**
      * Get a list of all certificate aliases stored in MTM.
      *
      * @return an {@link Enumeration} of all certificates
@@ -308,21 +251,6 @@ public class MemorizingTrustManager {
     }
 
     /**
-     * Get a certificate for a given alias.
-     *
-     * @param alias the certificate's alias as returned by {@link #getCertificates()}.
-     * @return the certificate associated with the alias or <tt>null</tt> if none found.
-     */
-    public Certificate getCertificate(String alias) {
-        try {
-            return appKeyStore.getCertificate(alias);
-        } catch (KeyStoreException e) {
-            // this should never happen, however...
-            throw new RuntimeException(e);
-        }
-    }
-
-    /**
      * Removes the given certificate from MTMs key store.
      *
      * <p>
@@ -340,32 +268,6 @@ public class MemorizingTrustManager {
         keyStoreUpdated();
     }
 
-    /**
-     * Creates a new hostname verifier supporting user interaction.
-     *
-     * <p>This method creates a new {@link HostnameVerifier} that is bound to
-     * the given instance of {@link MemorizingTrustManager}, and leverages an
-     * existing {@link HostnameVerifier}. The returned verifier performs the
-     * following steps, returning as soon as one of them succeeds:
-     * /p>
-     * <ol>
-     * <li>Success, if the wrapped defaultVerifier accepts the certificate.</li>
-     * <li>Success, if the server certificate is stored in the keystore under the given hostname.</li>
-     * <li>Ask the user and return accordingly.</li>
-     * <li>Failure on exception.</li>
-     * </ol>
-     *
-     * @param defaultVerifier the {@link HostnameVerifier} that should perform the actual check
-     * @return a new hostname verifier using the MTM's key store
-     * @throws IllegalArgumentException if the defaultVerifier parameter is null
-     */
-    public DomainHostnameVerifier wrapHostnameVerifier(final HostnameVerifier defaultVerifier, final boolean interactive) {
-        if (defaultVerifier == null)
-            throw new IllegalArgumentException("The default verifier may not be null");
-
-        return new MemorizingHostnameVerifier(defaultVerifier, interactive);
-    }
-
     X509TrustManager getTrustManager(KeyStore ks) {
         try {
             TrustManagerFactory tmf = TrustManagerFactory.getInstance("X509");
@@ -452,16 +354,8 @@ public class MemorizingTrustManager {
         }
     }
 
-    private boolean isExpiredException(Throwable e) {
-        do {
-            if (e instanceof CertificateExpiredException)
-                return true;
-            e = e.getCause();
-        } while (e != null);
-        return false;
-    }
 
-    public void checkCertTrusted(X509Certificate[] chain, String authType, String domain, boolean isServer, boolean interactive)
+    private void checkCertTrusted(X509Certificate[] chain, String authType, String domain, boolean isServer, boolean interactive)
             throws CertificateException {
         LOGGER.log(Level.FINE, "checkCertTrusted(" + chain + ", " + authType + ", " + isServer + ")");
         try {
@@ -470,13 +364,8 @@ public class MemorizingTrustManager {
                 appTrustManager.checkServerTrusted(chain, authType);
             else
                 appTrustManager.checkClientTrusted(chain, authType);
-        } catch (CertificateException ae) {
+        } catch (final CertificateException ae) {
             LOGGER.log(Level.FINER, "checkCertTrusted: appTrustManager failed", ae);
-            // if the cert is stored in our appTrustManager, we ignore expiredness
-            if (isExpiredException(ae)) {
-                LOGGER.log(Level.INFO, "checkCertTrusted: accepting expired certificate from keystore");
-                return;
-            }
             if (isCertKnown(chain[0])) {
                 LOGGER.log(Level.INFO, "checkCertTrusted: accepting cert already stored in keystore");
                 return;
@@ -673,40 +562,6 @@ public class MemorizingTrustManager {
         return si.toString();
     }
 
-    private String hostNameMessage(X509Certificate cert, String hostname) {
-        StringBuffer si = new StringBuffer();
-
-        si.append(master.getString(R.string.mtm_hostname_mismatch, hostname));
-        si.append("\n\n");
-        try {
-            Collection<List<?>> sans = cert.getSubjectAlternativeNames();
-            if (sans == null) {
-                si.append(cert.getSubjectDN());
-                si.append("\n");
-            } else for (List<?> altName : sans) {
-                Object name = altName.get(1);
-                if (name instanceof String) {
-                    si.append("[");
-                    si.append(altName.get(0));
-                    si.append("] ");
-                    si.append(name);
-                    si.append("\n");
-                }
-            }
-        } catch (CertificateParsingException e) {
-            e.printStackTrace();
-            si.append("<Parsing error: ");
-            si.append(e.getLocalizedMessage());
-            si.append(">\n");
-        }
-        si.append("\n");
-        si.append(master.getString(R.string.mtm_connect_anyway));
-        si.append("\n\n");
-        si.append(master.getString(R.string.mtm_cert_details));
-        certDetails(si, cert);
-        return si.toString();
-    }
-
     /**
      * Returns the top-most entry of the activity stack.
      *
@@ -764,17 +619,6 @@ public class MemorizingTrustManager {
         }
     }
 
-    boolean interactHostname(X509Certificate cert, String hostname) {
-        switch (interact(hostNameMessage(cert, hostname), R.string.mtm_accept_servername)) {
-            case MTMDecision.DECISION_ALWAYS:
-                storeCert(hostname, cert);
-            case MTMDecision.DECISION_ONCE:
-                return true;
-            default:
-                return false;
-        }
-    }
-
     public X509TrustManager getNonInteractive(String domain) {
         return new NonInteractiveMemorizingTrustManager(domain);
     }
@@ -791,57 +635,6 @@ public class MemorizingTrustManager {
         return new InteractiveMemorizingTrustManager(null);
     }
 
-    class MemorizingHostnameVerifier implements DomainHostnameVerifier {
-        private final HostnameVerifier defaultVerifier;
-        private final boolean interactive;
-
-        public MemorizingHostnameVerifier(HostnameVerifier wrapped, boolean interactive) {
-            this.defaultVerifier = wrapped;
-            this.interactive = interactive;
-        }
-
-        @Override
-        public boolean verify(String domain, String hostname, SSLSession session) {
-            LOGGER.log(Level.FINE, "hostname verifier for " + domain + ", trying default verifier first");
-            // if the default verifier accepts the hostname, we are done
-            if (defaultVerifier instanceof DomainHostnameVerifier) {
-                if (((DomainHostnameVerifier) defaultVerifier).verify(domain, hostname, session)) {
-                    return true;
-                }
-            } else {
-                if (defaultVerifier.verify(domain, session)) {
-                    return true;
-                }
-            }
-
-
-            // otherwise, we check if the hostname is an alias for this cert in our keystore
-            try {
-                X509Certificate cert = (X509Certificate) session.getPeerCertificates()[0];
-                //Log.d(TAG, "cert: " + cert);
-                if (cert.equals(appKeyStore.getCertificate(domain.toLowerCase(Locale.US)))) {
-                    LOGGER.log(Level.FINE, "certificate for " + domain + " is in our keystore. accepting.");
-                    return true;
-                } else {
-                    LOGGER.log(Level.FINE, "server " + domain + " provided wrong certificate, asking user.");
-                    if (interactive) {
-                        return interactHostname(cert, domain);
-                    } else {
-                        return false;
-                    }
-                }
-            } catch (Exception e) {
-                e.printStackTrace();
-                return false;
-            }
-        }
-
-        @Override
-        public boolean verify(String domain, SSLSession sslSession) {
-            return verify(domain, null, sslSession);
-        }
-    }
-
     private class NonInteractiveMemorizingTrustManager implements X509TrustManager {
 
         private final String domain;
diff --git a/src/main/java/eu/siacs/conversations/xmpp/XmppConnection.java b/src/main/java/eu/siacs/conversations/xmpp/XmppConnection.java
index 6baa65256..cd9cc2832 100644
--- a/src/main/java/eu/siacs/conversations/xmpp/XmppConnection.java
+++ b/src/main/java/eu/siacs/conversations/xmpp/XmppConnection.java
@@ -428,7 +428,7 @@ public class XmppConnection implements Runnable {
         return tag != null && tag.isStart("stream");
     }
 
-    private TlsFactoryVerifier getTlsFactoryVerifier() throws NoSuchAlgorithmException, KeyManagementException, IOException {
+    private SSLSocketFactory getSSLSocketFactory() throws NoSuchAlgorithmException, KeyManagementException {
         final SSLContext sc = SSLSocketHelper.getSSLContext();
         final MemorizingTrustManager trustManager = this.mXmppConnectionService.getMemorizingTrustManager();
         final KeyManager[] keyManager;
@@ -439,9 +439,7 @@ public class XmppConnection implements Runnable {
         }
         final String domain = account.getServer();
         sc.init(keyManager, new X509TrustManager[]{mInteractive ? trustManager.getInteractive(domain) : trustManager.getNonInteractive(domain)}, mXmppConnectionService.getRNG());
-        final SSLSocketFactory factory = sc.getSocketFactory();
-        final DomainHostnameVerifier verifier = trustManager.wrapHostnameVerifier(new XmppDomainVerifier(), mInteractive);
-        return new TlsFactoryVerifier(factory, verifier);
+        return sc.getSocketFactory();
     }
 
     @Override
@@ -816,21 +814,22 @@ public class XmppConnection implements Runnable {
     }
 
     private SSLSocket upgradeSocketToTls(final Socket socket) throws IOException {
-        final TlsFactoryVerifier tlsFactoryVerifier;
+        final SSLSocketFactory sslSocketFactory;
         try {
-            tlsFactoryVerifier = getTlsFactoryVerifier();
+            sslSocketFactory = getSSLSocketFactory();
         } catch (final NoSuchAlgorithmException | KeyManagementException e) {
             throw new StateChangingException(Account.State.TLS_ERROR);
         }
         final InetAddress address = socket.getInetAddress();
-        final SSLSocket sslSocket = (SSLSocket) tlsFactoryVerifier.factory.createSocket(socket, address.getHostAddress(), socket.getPort(), true);
+        final SSLSocket sslSocket = (SSLSocket) sslSocketFactory.createSocket(socket, address.getHostAddress(), socket.getPort(), true);
         SSLSocketHelper.setSecurity(sslSocket);
         SSLSocketHelper.setHostname(sslSocket, IDN.toASCII(account.getServer()));
         SSLSocketHelper.setApplicationProtocol(sslSocket, "xmpp-client");
-        if (!tlsFactoryVerifier.verifier.verify(account.getServer(), this.verifiedHostname, sslSocket.getSession())) {
-            Log.d(Config.LOGTAG, account.getJid().asBareJid() + ": TLS certificate verification failed");
+        final XmppDomainVerifier xmppDomainVerifier = new XmppDomainVerifier();
+        if (!xmppDomainVerifier.verify(account.getServer(), this.verifiedHostname, sslSocket.getSession())) {
+            Log.d(Config.LOGTAG, account.getJid().asBareJid() + ": TLS certificate domain verification failed");
             FileBackend.close(sslSocket);
-            throw new StateChangingException(Account.State.TLS_ERROR);
+            throw new StateChangingException(Account.State.TLS_ERROR_DOMAIN);
         }
         return sslSocket;
     }
@@ -1738,19 +1737,6 @@ public class XmppConnection implements Runnable {
         UNKNOWN
     }
 
-    private static class TlsFactoryVerifier {
-        private final SSLSocketFactory factory;
-        private final DomainHostnameVerifier verifier;
-
-        TlsFactoryVerifier(final SSLSocketFactory factory, final DomainHostnameVerifier verifier) throws IOException {
-            this.factory = factory;
-            this.verifier = verifier;
-            if (factory == null || verifier == null) {
-                throw new IOException("could not setup ssl");
-            }
-        }
-    }
-
     private class MyKeyManager implements X509KeyManager {
         @Override
         public String chooseClientAlias(String[] strings, Principal[] principals, Socket socket) {
diff --git a/src/main/res/values/strings.xml b/src/main/res/values/strings.xml
index 1b4b56e7d..2db8b1da3 100644
--- a/src/main/res/values/strings.xml
+++ b/src/main/res/values/strings.xml
@@ -163,6 +163,7 @@
     <string name="account_status_regis_not_sup">Registration not supported by server</string>
     <string name="account_status_regis_invalid_token">Invalid registration token</string>
     <string name="account_status_tls_error">TLS negotiation failed</string>
+    <string name="account_status_tls_error_domain">Domain not verifiable</string>
     <string name="account_status_policy_violation">Policy violation</string>
     <string name="account_status_incompatible_server">Incompatible server</string>
     <string name="account_status_stream_error">Stream error</string>