Commit 78e4eff7 authored by Dave Cridland's avatar Dave Cridland

OF-405 : Perform proper path validation on certificate chains

What this patch actually does is place existing certificates into a CertStore,
including those from its (untrusted) keystore, the trust store, and any from
the chain supplied by the peer, and then rebuild a chain back to a known trust
anchor (from the trust store).

This strategy will cope with unknown ICAs in chains, abbreviated chains, and so
on, and replaces attempts to specifically handle self-signed certificates.

That last said, there is an explicit shortcut to handle self-signed certificates
which are supplied as end-entity certificates. These are simply checked against
the trust store without any attempt to build a path.
parent 73b59106
...@@ -187,24 +187,18 @@ public class SASLAuthentication { ...@@ -187,24 +187,18 @@ public class SASLAuthentication {
if (session instanceof IncomingServerSession) { if (session instanceof IncomingServerSession) {
// Server connections don't follow the same rules as clients // Server connections don't follow the same rules as clients
if (session.isSecure()) { if (session.isSecure()) {
boolean usingSelfSigned; boolean usingSelfSigned = true;
final Certificate[] chain = session.getConnection().getLocalCertificates();
if (chain == null || chain.length == 0) { try {
usingSelfSigned = true; X509Certificate trusted = CertificateManager.getEndEntityCertificate(session.getConnection().getPeerCertificates(), SSLConfig.getKeyStore(), SSLConfig.gets2sTrustStore());
} else { usingSelfSigned = trusted == null;
try { } catch (IOException ex) {
usingSelfSigned = CertificateManager.isSelfSignedCertificate(SSLConfig.getKeyStore(), (X509Certificate) chain[0]); Log.warn("Exception occurred while trying to determine whether remote certificate is trusted. Proceeding as if it is.", ex);
} catch (KeyStoreException ex) { usingSelfSigned = true;
Log.warn("Exception occurred while trying to determine whether local certificate is self-signed. Proceeding as if it is.", ex);
usingSelfSigned = true;
} catch (IOException ex) {
Log.warn("Exception occurred while trying to determine whether local certificate is self-signed. Proceeding as if it is.", ex);
usingSelfSigned = true;
}
} }
if (!usingSelfSigned) { if (!usingSelfSigned) {
// Offer SASL EXTERNAL only if TLS has already been negotiated and we are not // Offer SASL EXTERNAL only if TLS has already been negotiated and the peer is not
// using a self-signed certificate // using a self-signed certificate
sb.append("<mechanism>EXTERNAL</mechanism>"); sb.append("<mechanism>EXTERNAL</mechanism>");
} }
...@@ -229,11 +223,21 @@ public class SASLAuthentication { ...@@ -229,11 +223,21 @@ public class SASLAuthentication {
Element mechs = DocumentHelper.createElement(new QName("mechanisms", Element mechs = DocumentHelper.createElement(new QName("mechanisms",
new Namespace("", "urn:ietf:params:xml:ns:xmpp-sasl"))); new Namespace("", "urn:ietf:params:xml:ns:xmpp-sasl")));
if (session instanceof IncomingServerSession) { if (session instanceof IncomingServerSession) {
// Server connections dont follow the same rules as clients // Server connections don't follow the same rules as clients
if (session.isSecure()) { if (session.isSecure()) {
// Offer SASL EXTERNAL only if TLS has already been negotiated boolean usingSelfSigned = true;
Element mechanism = mechs.addElement("mechanism"); try {
mechanism.setText("EXTERNAL"); X509Certificate trusted = CertificateManager.getEndEntityCertificate(((LocalSession)session).getConnection().getPeerCertificates(), SSLConfig.getKeyStore(), SSLConfig.gets2sTrustStore());
usingSelfSigned = trusted == null;
} catch (IOException ex) {
Log.warn("Exception occurred while trying to determine whether remote certificate is trusted. Proceeding as if it is.", ex);
usingSelfSigned = true;
}
if (!usingSelfSigned) {
// Offer SASL EXTERNAL only if TLS has already been negotiated and the peer has a trusted cert.
Element mechanism = mechs.addElement("mechanism");
mechanism.setText("EXTERNAL");
}
} }
} }
else { else {
...@@ -546,7 +550,7 @@ public class SASLAuthentication { ...@@ -546,7 +550,7 @@ public class SASLAuthentication {
} }
hostname = new String(StringUtils.decodeBase64(hostname), CHARSET); hostname = new String(StringUtils.decodeBase64(hostname), CHARSET);
// Check if cerificate validation is disabled for s2s // Check if certificate validation is disabled for s2s
// Flag that indicates if certificates of the remote server should be validated. // Flag that indicates if certificates of the remote server should be validated.
// Disabling certificate validation is not recommended for production environments. // Disabling certificate validation is not recommended for production environments.
boolean verify = boolean verify =
...@@ -557,19 +561,24 @@ public class SASLAuthentication { ...@@ -557,19 +561,24 @@ public class SASLAuthentication {
} }
// Check that hostname matches the one provided in a certificate // Check that hostname matches the one provided in a certificate
Connection connection = session.getConnection(); Connection connection = session.getConnection();
try {
for (Certificate certificate : connection.getPeerCertificates()) { X509Certificate trusted = CertificateManager.getEndEntityCertificate(connection.getPeerCertificates(), SSLConfig.getKeyStore(), SSLConfig.gets2sTrustStore());
for (String identity : CertificateManager.getPeerIdentities((X509Certificate) certificate)) {
// Verify that either the identity is the same as the hostname, or for wildcarded if (trusted != null) {
// identities that the hostname ends with .domainspecified or -is- domainspecified. for (String identity : CertificateManager.getPeerIdentities(trusted)) {
if ((identity.startsWith("*.") // Verify that either the identity is the same as the hostname, or for wildcarded
&& (hostname.endsWith(identity.replace("*.", ".")) // identities that the hostname ends with .domainspecified or -is- domainspecified.
|| hostname.equals(identity.replace("*.", "")))) if ((identity.startsWith("*.")
|| hostname.equals(identity)) { && (hostname.endsWith(identity.replace("*.", "."))
authenticationSuccessful(session, hostname, null); || hostname.equals(identity.replace("*.", ""))))
return Status.authenticated; || hostname.equals(identity)) {
authenticationSuccessful(session, hostname, null);
return Status.authenticated;
}
} }
} }
} catch(IOException e) {
/// Keystore problem.
} }
} }
......
...@@ -37,9 +37,19 @@ import java.security.Provider; ...@@ -37,9 +37,19 @@ import java.security.Provider;
import java.security.PublicKey; import java.security.PublicKey;
import java.security.SecureRandom; import java.security.SecureRandom;
import java.security.Security; import java.security.Security;
import java.security.cert.CertPath;
import java.security.cert.CertPathBuilder;
import java.security.cert.CertPathBuilderException;
import java.security.cert.CertPathValidator;
import java.security.cert.CertPathValidatorException;
import java.security.cert.CertStore;
import java.security.cert.Certificate; import java.security.cert.Certificate;
import java.security.cert.CertificateFactory; import java.security.cert.CertificateFactory;
import java.security.cert.CertificateParsingException; import java.security.cert.CertificateParsingException;
import java.security.cert.CollectionCertStoreParameters;
import java.security.cert.PKIXBuilderParameters;
import java.security.cert.TrustAnchor;
import java.security.cert.X509CertSelector;
import java.security.cert.X509Certificate; import java.security.cert.X509Certificate;
import java.util.ArrayList; import java.util.ArrayList;
import java.util.Collection; import java.util.Collection;
...@@ -47,6 +57,7 @@ import java.util.Collections; ...@@ -47,6 +57,7 @@ import java.util.Collections;
import java.util.Date; import java.util.Date;
import java.util.Enumeration; import java.util.Enumeration;
import java.util.HashMap; import java.util.HashMap;
import java.util.HashSet;
import java.util.Hashtable; import java.util.Hashtable;
import java.util.LinkedList; import java.util.LinkedList;
import java.util.List; import java.util.List;
...@@ -208,6 +219,110 @@ public class CertificateManager { ...@@ -208,6 +219,110 @@ public class CertificateManager {
} }
} }
/**
* Decide whether or not to trust the given supplied certificate chain, returning the
* End Entity Certificate in this case where it can, and null otherwise.
* A self-signed certificate will, for example, return null.
* For certain failures, we SHOULD generate an exception - revocations and the like.
*
* @param certChain an array of X509Certificate where the first one is the endEntityCertificate.
* @return a boolean indicating whether the endEntityCertificate should be trusted.
*/
public static X509Certificate getEndEntityCertificate(Certificate chain[],
KeyStore certStore, KeyStore trustStore) {
if (chain.length == 0) {
return null;
}
X509Certificate first = (X509Certificate) chain[0];
if (chain.length == 1
&& first.getSubjectDN().equals(first.getIssuerDN())) {
// Chain is single cert, and self-signed.
try {
if (trustStore.getCertificateAlias(first) != null) {
// Interesting case: trusted self-signed cert.
return first;
}
} catch (KeyStoreException e) {
// Ignore.
}
return null;
}
ArrayList<Object> all_certs = new ArrayList<Object>();
try {
// First, load up certStore contents into a CertStore.
// It's a mystery why these objects are different.
for (Enumeration<String> aliases = certStore.aliases(); aliases
.hasMoreElements();) {
String alias = aliases.nextElement();
if (certStore.isCertificateEntry(alias)) {
X509Certificate cert = (X509Certificate) certStore
.getCertificate(alias);
all_certs.add(cert);
}
}
// Now add the trusted certs.
for (Enumeration<String> aliases = trustStore.aliases(); aliases
.hasMoreElements();) {
String alias = aliases.nextElement();
if (trustStore.isCertificateEntry(alias)) {
X509Certificate cert = (X509Certificate) trustStore
.getCertificate(alias);
all_certs.add(cert);
}
}
// Finally, add all the certs in the chain except the first:
for (int i = 0; i < chain.length; ++i) {
all_certs.add(chain[i]);
}
CertStore cs = CertStore.getInstance("Collection",
new CollectionCertStoreParameters(all_certs));
X509CertSelector selector = new X509CertSelector();
selector.setCertificate(first);
// / selector.setSubject(first.getSubjectX500Principal());
PKIXBuilderParameters params = new PKIXBuilderParameters(
trustStore, selector);
params.addCertStore(cs);
params.setDate(new Date());
params.setRevocationEnabled(false);
/* Code here is the right way to do things. */
CertPathBuilder pathBuilder = CertPathBuilder
.getInstance(CertPathBuilder.getDefaultType());
CertPath cp = pathBuilder.build(params).getCertPath();
/**
* This section is an alternative to using CertPathBuilder which is
* not as complete (or safe), but will emit much better errors. If
* things break, swap around the code.
*
**** COMMENTED OUT. ****
ArrayList<X509Certificate> ls = new ArrayList<X509Certificate>();
for (int i = 0; i < chain.length; ++i) {
ls.add((X509Certificate) chain[i]);
}
for (X509Certificate last = ls.get(ls.size() - 1); !last
.getIssuerDN().equals(last.getSubjectDN()); last = ls
.get(ls.size() - 1)) {
X509CertSelector sel = new X509CertSelector();
sel.setSubject(last.getIssuerX500Principal());
ls.add((X509Certificate) cs.getCertificates(sel).toArray()[0]);
}
CertPath cp = CertificateFactory.getInstance("X.509").generateCertPath(ls);
****** END ALTERNATIVE. ****
*/
// Not entirely sure if I need to do this with CertPathBuilder.
// Can't hurt.
CertPathValidator pathValidator = CertPathValidator
.getInstance("PKIX");
pathValidator.validate(cp, params);
return (X509Certificate) cp.getCertificates().get(0);
} catch (CertPathBuilderException e) {
Log.warn("Path builder: " + e.getMessage());
} catch (CertPathValidatorException e) {
Log.warn("Path validator: " + e.getMessage());
} catch (Exception e) {
}
return null;
}
/** /**
* Returns the identities of the remote server as defined in the specified certificate. The * Returns the identities of the remote server as defined in the specified certificate. The
* identities are defined in the subjectDN of the certificate and it can also be defined in * identities are defined in the subjectDN of the certificate and it can also be defined in
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment