From bc58fb0fbdf22c9518579d13c5d511f37d34367d Mon Sep 17 00:00:00 2001 From: Daniel Gultsch Date: Fri, 30 Apr 2021 09:53:19 +0200 Subject: [PATCH] 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 --- .../siacs/conversations/entities/Account.java | 3 + .../http/HttpConnectionManager.java | 3 +- .../services/MemorizingTrustManager.java | 215 +----------------- .../conversations/xmpp/XmppConnection.java | 32 +-- src/main/res/values/strings.xml | 1 + 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. - *

- * The actual filename relative to the app's directory will be - * app_dirname/filename. - * - * @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); @@ -263,36 +236,6 @@ public class MemorizingTrustManager { appKeyStore = loadAppKeyStore(); } - /** - * Binds an Activity to the MTM for displaying the query dialog. - *

- * 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. - *

- * 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. - *

- * 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. * @@ -307,21 +250,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 null 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. * @@ -340,32 +268,6 @@ public class MemorizingTrustManager { keyStoreUpdated(); } - /** - * Creates a new hostname verifier supporting user interaction. - * - *

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> - *

    - *
  1. Success, if the wrapped defaultVerifier accepts the certificate.
    2. - *
  2. Success, if the server certificate is stored in the keystore under the given hostname.
    3. - *
  3. Ask the user and return accordingly.
    4. - *
  4. Failure on exception.
    5. - *
- * - * @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> 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("\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 @@ Registration not supported by server Invalid registration token TLS negotiation failed + Domain not verifiable Policy violation Incompatible server Stream error