From 93030c218c62cf0a0a8ea48746db1452fa34033c Mon Sep 17 00:00:00 2001 From: Florian Schmaus Date: Mon, 10 Feb 2014 12:07:39 +0100 Subject: [PATCH] Remove ServerTrustManager The implementation of ServerTrustManger contains a security vulnerability, which could lead to unauthorized certificates being erroneously trusted. SMACK-410 --- .../smack/ConnectionConfiguration.java | 185 ---------- .../smack/ServerTrustManager.java | 342 ------------------ .../jivesoftware/smack/XMPPConnection.java | 3 +- 3 files changed, 1 insertion(+), 529 deletions(-) delete mode 100644 source/org/jivesoftware/smack/ServerTrustManager.java diff --git a/source/org/jivesoftware/smack/ConnectionConfiguration.java b/source/org/jivesoftware/smack/ConnectionConfiguration.java index 396acea3e..8e2f6b30c 100644 --- a/source/org/jivesoftware/smack/ConnectionConfiguration.java +++ b/source/org/jivesoftware/smack/ConnectionConfiguration.java @@ -28,7 +28,6 @@ import javax.net.SocketFactory; import javax.net.ssl.SSLContext; import javax.security.auth.callback.CallbackHandler; -import java.io.File; import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -55,17 +54,9 @@ public class ConnectionConfiguration implements Cloneable { private int port; protected List hostAddresses; - private String truststorePath; - private String truststoreType; - private String truststorePassword; private String keystorePath; private String keystoreType; private String pkcs11Library; - private boolean verifyChainEnabled = false; - private boolean verifyRootCAEnabled = false; - private boolean selfSignedCertificateEnabled = false; - private boolean expiredCertificatesCheckEnabled = false; - private boolean notMatchingDomainCheckEnabled = false; private SSLContext customSSLContext; private boolean compressionEnabled = false; @@ -203,18 +194,6 @@ public class ConnectionConfiguration implements Cloneable { this.serviceName = serviceName; this.proxy = proxy; - // Build the default path to the cacert truststore file. By default we are - // going to use the file located in $JREHOME/lib/security/cacerts. - String javaHome = System.getProperty("java.home"); - StringBuilder buffer = new StringBuilder(); - buffer.append(javaHome).append(File.separator).append("lib"); - buffer.append(File.separator).append("security"); - buffer.append(File.separator).append("cacerts"); - truststorePath = buffer.toString(); - // Set the default store type - truststoreType = "jks"; - // Set the default password of the cacert file that is "changeit" - truststorePassword = "changeit"; keystorePath = System.getProperty("javax.net.ssl.keyStore"); keystoreType = "jks"; pkcs11Library = "pkcs11.config"; @@ -287,66 +266,6 @@ public class ConnectionConfiguration implements Cloneable { this.securityMode = securityMode; } - /** - * Retuns the path to the trust store file. The trust store file contains the root - * certificates of several well known CAs. By default, will attempt to use the - * the file located in $JREHOME/lib/security/cacerts. - * - * @return the path to the truststore file. - */ - public String getTruststorePath() { - return truststorePath; - } - - /** - * Sets the path to the trust store file. The truststore file contains the root - * certificates of several well?known CAs. By default Smack is going to use - * the file located in $JREHOME/lib/security/cacerts. - * - * @param truststorePath the path to the truststore file. - */ - public void setTruststorePath(String truststorePath) { - this.truststorePath = truststorePath; - } - - /** - * Returns the trust store type, or null if it's not set. - * - * @return the trust store type. - */ - public String getTruststoreType() { - return truststoreType; - } - - /** - * Sets the trust store type. - * - * @param truststoreType the trust store type. - */ - public void setTruststoreType(String truststoreType) { - this.truststoreType = truststoreType; - } - - /** - * Returns the password to use to access the trust store file. It is assumed that all - * certificates share the same password in the trust store. - * - * @return the password to use to access the truststore file. - */ - public String getTruststorePassword() { - return truststorePassword; - } - - /** - * Sets the password to use to access the trust store file. It is assumed that all - * certificates share the same password in the trust store. - * - * @param truststorePassword the password to use to access the truststore file. - */ - public void setTruststorePassword(String truststorePassword) { - this.truststorePassword = truststorePassword; - } - /** * Retuns the path to the keystore file. The key store file contains the * certificates that may be used to authenticate the client to the server, @@ -408,110 +327,6 @@ public class ConnectionConfiguration implements Cloneable { this.pkcs11Library = pkcs11Library; } - /** - * Returns true if the whole chain of certificates presented by the server are going to - * be checked. By default the certificate chain is not verified. - * - * @return true if the whole chaing of certificates presented by the server are going to - * be checked. - */ - public boolean isVerifyChainEnabled() { - return verifyChainEnabled; - } - - /** - * Sets if the whole chain of certificates presented by the server are going to - * be checked. By default the certificate chain is not verified. - * - * @param verifyChainEnabled if the whole chaing of certificates presented by the server - * are going to be checked. - */ - public void setVerifyChainEnabled(boolean verifyChainEnabled) { - this.verifyChainEnabled = verifyChainEnabled; - } - - /** - * Returns true if root CA checking is going to be done. By default checking is disabled. - * - * @return true if root CA checking is going to be done. - */ - public boolean isVerifyRootCAEnabled() { - return verifyRootCAEnabled; - } - - /** - * Sets if root CA checking is going to be done. By default checking is disabled. - * - * @param verifyRootCAEnabled if root CA checking is going to be done. - */ - public void setVerifyRootCAEnabled(boolean verifyRootCAEnabled) { - this.verifyRootCAEnabled = verifyRootCAEnabled; - } - - /** - * Returns true if self-signed certificates are going to be accepted. By default - * this option is disabled. - * - * @return true if self-signed certificates are going to be accepted. - */ - public boolean isSelfSignedCertificateEnabled() { - return selfSignedCertificateEnabled; - } - - /** - * Sets if self-signed certificates are going to be accepted. By default - * this option is disabled. - * - * @param selfSignedCertificateEnabled if self-signed certificates are going to be accepted. - */ - public void setSelfSignedCertificateEnabled(boolean selfSignedCertificateEnabled) { - this.selfSignedCertificateEnabled = selfSignedCertificateEnabled; - } - - /** - * Returns true if certificates presented by the server are going to be checked for their - * validity. By default certificates are not verified. - * - * @return true if certificates presented by the server are going to be checked for their - * validity. - */ - public boolean isExpiredCertificatesCheckEnabled() { - return expiredCertificatesCheckEnabled; - } - - /** - * Sets if certificates presented by the server are going to be checked for their - * validity. By default certificates are not verified. - * - * @param expiredCertificatesCheckEnabled if certificates presented by the server are going - * to be checked for their validity. - */ - public void setExpiredCertificatesCheckEnabled(boolean expiredCertificatesCheckEnabled) { - this.expiredCertificatesCheckEnabled = expiredCertificatesCheckEnabled; - } - - /** - * Returns true if certificates presented by the server are going to be checked for their - * domain. By default certificates are not verified. - * - * @return true if certificates presented by the server are going to be checked for their - * domain. - */ - public boolean isNotMatchingDomainCheckEnabled() { - return notMatchingDomainCheckEnabled; - } - - /** - * Sets if certificates presented by the server are going to be checked for their - * domain. By default certificates are not verified. - * - * @param notMatchingDomainCheckEnabled if certificates presented by the server are going - * to be checked for their domain. - */ - public void setNotMatchingDomainCheckEnabled(boolean notMatchingDomainCheckEnabled) { - this.notMatchingDomainCheckEnabled = notMatchingDomainCheckEnabled; - } - /** * Gets the custom SSLContext for SSL sockets. This is null by default. * diff --git a/source/org/jivesoftware/smack/ServerTrustManager.java b/source/org/jivesoftware/smack/ServerTrustManager.java deleted file mode 100644 index a9e45a308..000000000 --- a/source/org/jivesoftware/smack/ServerTrustManager.java +++ /dev/null @@ -1,342 +0,0 @@ -/** - * $RCSfile$ - * $Revision$ - * $Date$ - * - * Copyright 2003-2005 Jive Software. - * - * All rights reserved. Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.jivesoftware.smack; - -import javax.net.ssl.X509TrustManager; -import java.io.FileInputStream; -import java.io.InputStream; -import java.io.IOException; -import java.security.*; -import java.security.cert.CertificateException; -import java.security.cert.CertificateParsingException; -import java.security.cert.X509Certificate; -import java.util.*; -import java.util.logging.Logger; -import java.util.regex.Matcher; -import java.util.regex.Pattern; - -/** - * Trust manager that checks all certificates presented by the server. This class - * is used during TLS negotiation. It is possible to disable/enable some or all checkings - * by configuring the {@link ConnectionConfiguration}. The truststore file that contains - * knows and trusted CA root certificates can also be configure in {@link ConnectionConfiguration}. - * - * @author Gaston Dombiak - */ -class ServerTrustManager implements X509TrustManager { - private static Logger log = Logger.getLogger(ServerTrustManager.class.getName()); - - private static Pattern cnPattern = Pattern.compile("(?i)(cn=)([^,]*)"); - - private ConnectionConfiguration configuration; - - /** - * Holds the domain of the remote server we are trying to connect - */ - private String server; - private KeyStore trustStore; - - private static Map stores = new HashMap(); - - public ServerTrustManager(String server, ConnectionConfiguration configuration) { - this.configuration = configuration; - this.server = server; - - InputStream in = null; - char[] password = null; - synchronized (stores) { - KeyStoreOptions options = new KeyStoreOptions(configuration.getTruststoreType(), - configuration.getTruststorePath(), configuration.getTruststorePassword()); - if (stores.containsKey(options)) { - trustStore = stores.get(options); - } else { - try { - trustStore = KeyStore.getInstance(options.getType()); - if (options.getPassword() != null) { - password = options.getPassword().toCharArray(); - } - - if (options.getPath() == null) { - trustStore.load(null, password); - } - else { - in = new FileInputStream(options.getPath()); - trustStore.load(in, password); - } - - } catch (Exception e) { - trustStore = null; - e.printStackTrace(); - } finally { - if (in != null) { - try { - in.close(); - } catch (IOException ioe) { - // Ignore. - } - } - } - stores.put(options, trustStore); - } - if (trustStore == null) - // Disable root CA checking - configuration.setVerifyRootCAEnabled(false); - } - } - - public X509Certificate[] getAcceptedIssuers() { - return new X509Certificate[0]; - } - - public void checkClientTrusted(X509Certificate[] arg0, String arg1) - throws CertificateException { - } - - public void checkServerTrusted(X509Certificate[] x509Certificates, String arg1) - throws CertificateException { - - int nSize = x509Certificates.length; - - List peerIdentities = getPeerIdentity(x509Certificates[0]); - - if (configuration.isVerifyChainEnabled()) { - // Working down the chain, for every certificate in the chain, - // verify that the subject of the certificate is the issuer of the - // next certificate in the chain. - Principal principalLast = null; - for (int i = nSize -1; i >= 0 ; i--) { - X509Certificate x509certificate = x509Certificates[i]; - Principal principalIssuer = x509certificate.getIssuerDN(); - Principal principalSubject = x509certificate.getSubjectDN(); - if (principalLast != null) { - if (principalIssuer.equals(principalLast)) { - try { - PublicKey publickey = - x509Certificates[i + 1].getPublicKey(); - x509Certificates[i].verify(publickey); - } - catch (GeneralSecurityException generalsecurityexception) { - throw new CertificateException( - "signature verification failed of " + peerIdentities); - } - } - else { - throw new CertificateException( - "subject/issuer verification failed of " + peerIdentities); - } - } - principalLast = principalSubject; - } - } - - if (configuration.isVerifyRootCAEnabled()) { - // Verify that the the last certificate in the chain was issued - // by a third-party that the client trusts. - boolean trusted = false; - try { - trusted = trustStore.getCertificateAlias(x509Certificates[nSize - 1]) != null; - if (!trusted && nSize == 1 && configuration.isSelfSignedCertificateEnabled()) - { - log.info("Accepting self-signed certificate of remote server: " + peerIdentities); - trusted = true; - } - } - catch (KeyStoreException e) { - e.printStackTrace(); - } - if (!trusted) { - throw new CertificateException("root certificate not trusted of " + peerIdentities); - } - } - - if (configuration.isNotMatchingDomainCheckEnabled()) { - // Verify that the first certificate in the chain corresponds to - // the server we desire to authenticate. - // Check if the certificate uses a wildcard indicating that subdomains are valid - if (peerIdentities.size() == 1 && peerIdentities.get(0).startsWith("*.")) { - // Remove the wildcard - String peerIdentity = peerIdentities.get(0).replace("*.", ""); - // Check if the requested subdomain matches the certified domain - if (!server.endsWith(peerIdentity)) { - throw new CertificateException("target verification failed of " + peerIdentities); - } - } - else if (!peerIdentities.contains(server)) { - throw new CertificateException("target verification failed of " + peerIdentities); - } - } - - if (configuration.isExpiredCertificatesCheckEnabled()) { - // For every certificate in the chain, verify that the certificate - // is valid at the current time. - Date date = new Date(); - for (int i = 0; i < nSize; i++) { - try { - x509Certificates[i].checkValidity(date); - } - catch (GeneralSecurityException generalsecurityexception) { - throw new CertificateException("invalid date of " + server); - } - } - } - - } - - /** - * Returns the identity of the remote server as defined in the specified certificate. The - * identity is defined in the subjectDN of the certificate and it can also be defined in - * the subjectAltName extension of type "xmpp". When the extension is being used then the - * identity defined in the extension in going to be returned. Otherwise, the value stored in - * the subjectDN is returned. - * - * @param x509Certificate the certificate the holds the identity of the remote server. - * @return the identity of the remote server as defined in the specified certificate. - */ - public static List getPeerIdentity(X509Certificate x509Certificate) { - // Look the identity in the subjectAltName extension if available - List names = getSubjectAlternativeNames(x509Certificate); - if (names.isEmpty()) { - String name = x509Certificate.getSubjectDN().getName(); - Matcher matcher = cnPattern.matcher(name); - if (matcher.find()) { - name = matcher.group(2); - } - // Create an array with the unique identity - names = new ArrayList(); - names.add(name); - } - return names; - } - - /** - * Returns the JID representation of an XMPP entity contained as a SubjectAltName extension - * in the certificate. If none was found then return null. - * - * @param certificate the certificate presented by the remote entity. - * @return the JID representation of an XMPP entity contained as a SubjectAltName extension - * in the certificate. If none was found then return null. - */ - private static List getSubjectAlternativeNames(X509Certificate certificate) { - List identities = new ArrayList(); - try { - Collection> altNames = certificate.getSubjectAlternativeNames(); - // Check that the certificate includes the SubjectAltName extension - if (altNames == null) { - return Collections.emptyList(); - } - // Use the type OtherName to search for the certified server name - /*for (List item : altNames) { - Integer type = (Integer) item.get(0); - if (type == 0) { - // Type OtherName found so return the associated value - try { - // Value is encoded using ASN.1 so decode it to get the server's identity - ASN1InputStream decoder = new ASN1InputStream((byte[]) item.toArray()[1]); - DEREncodable encoded = decoder.readObject(); - encoded = ((DERSequence) encoded).getObjectAt(1); - encoded = ((DERTaggedObject) encoded).getObject(); - encoded = ((DERTaggedObject) encoded).getObject(); - String identity = ((DERUTF8String) encoded).getString(); - // Add the decoded server name to the list of identities - identities.add(identity); - } - catch (UnsupportedEncodingException e) { - // Ignore - } - catch (IOException e) { - // Ignore - } - catch (Exception e) { - e.printStackTrace(); - } - } - // Other types are not good for XMPP so ignore them - log.info("SubjectAltName of invalid type found: " + certificate); - }*/ - } - catch (CertificateParsingException e) { - e.printStackTrace(); - } - return identities; - } - - private static class KeyStoreOptions { - private final String type; - private final String path; - private final String password; - - public KeyStoreOptions(String type, String path, String password) { - super(); - this.type = type; - this.path = path; - this.password = password; - } - - public String getType() { - return type; - } - - public String getPath() { - return path; - } - - public String getPassword() { - return password; - } - - @Override - public int hashCode() { - final int prime = 31; - int result = 1; - result = prime * result + ((password == null) ? 0 : password.hashCode()); - result = prime * result + ((path == null) ? 0 : path.hashCode()); - result = prime * result + ((type == null) ? 0 : type.hashCode()); - return result; - } - - @Override - public boolean equals(Object obj) { - if (this == obj) - return true; - if (obj == null) - return false; - if (getClass() != obj.getClass()) - return false; - KeyStoreOptions other = (KeyStoreOptions) obj; - if (password == null) { - if (other.password != null) - return false; - } else if (!password.equals(other.password)) - return false; - if (path == null) { - if (other.path != null) - return false; - } else if (!path.equals(other.path)) - return false; - if (type == null) { - if (other.type != null) - return false; - } else if (!type.equals(other.type)) - return false; - return true; - } - } -} diff --git a/source/org/jivesoftware/smack/XMPPConnection.java b/source/org/jivesoftware/smack/XMPPConnection.java index 13c16e1cd..a73910b15 100644 --- a/source/org/jivesoftware/smack/XMPPConnection.java +++ b/source/org/jivesoftware/smack/XMPPConnection.java @@ -844,8 +844,7 @@ public class XMPPConnection extends Connection { // Verify certificate presented by the server if (context == null) { context = SSLContext.getInstance("TLS"); - context.init(kms, new javax.net.ssl.TrustManager[] { new ServerTrustManager(getServiceName(), config) }, - new java.security.SecureRandom()); + context.init(kms, null, new java.security.SecureRandom()); } Socket plain = socket; // Secure the plain connection