From d3af9620817220d737fdb532c1ae1032bdd65e11 Mon Sep 17 00:00:00 2001 From: Chad Brubaker Date: Mon, 16 Nov 2015 10:48:20 -0800 Subject: [PATCH] Expose findTrustAnchorBySubjectAndPublicKey This allows for faster lookups of TrustAnchors when checking pin overrides without needing to iterate over all certificates. Currently only the system and user trusted certificate store are optimized to avoid reading the entire source before doing the trust anchor lookup, improvements to the resource source will come in a later commit. This also refactors System/UserCertificateSource to avoid code duplication. Change-Id: Ice00c5e047140f3d102306937556b761faaf0d0e --- .../security/net/config/CertificateSource.java | 1 + .../security/net/config/CertificatesEntryRef.java | 13 ++ .../net/config/DirectoryCertificateSource.java | 138 +++++++++++++++++++++ .../net/config/KeyStoreCertificateSource.java | 25 +++- .../security/net/config/NetworkSecurityConfig.java | 34 ++++- .../net/config/NetworkSecurityTrustManager.java | 10 +- .../net/config/ResourceCertificateSource.java | 35 ++++-- .../net/config/SystemCertificateSource.java | 70 ++--------- .../security/net/config/UserCertificateSource.java | 59 +-------- .../res/xml/override_dedup.xml | 20 +++ .../security/net/config/TestCertificateSource.java | 14 +++ .../security/net/config/XmlConfigTests.java | 18 +++ 12 files changed, 299 insertions(+), 138 deletions(-) create mode 100644 core/java/android/security/net/config/DirectoryCertificateSource.java create mode 100644 tests/NetworkSecurityConfigTest/res/xml/override_dedup.xml diff --git a/core/java/android/security/net/config/CertificateSource.java b/core/java/android/security/net/config/CertificateSource.java index 386354dc4d57..2b7829eb6a31 100644 --- a/core/java/android/security/net/config/CertificateSource.java +++ b/core/java/android/security/net/config/CertificateSource.java @@ -22,4 +22,5 @@ import java.security.cert.X509Certificate; /** @hide */ public interface CertificateSource { Set getCertificates(); + X509Certificate findBySubjectAndPublicKey(X509Certificate cert); } diff --git a/core/java/android/security/net/config/CertificatesEntryRef.java b/core/java/android/security/net/config/CertificatesEntryRef.java index 2ba38c21c330..1d15e19a99f2 100644 --- a/core/java/android/security/net/config/CertificatesEntryRef.java +++ b/core/java/android/security/net/config/CertificatesEntryRef.java @@ -30,6 +30,10 @@ public final class CertificatesEntryRef { mOverridesPins = overridesPins; } + boolean overridesPins() { + return mOverridesPins; + } + public Set getTrustAnchors() { // TODO: cache this [but handle mutable sources] Set anchors = new ArraySet(); @@ -38,4 +42,13 @@ public final class CertificatesEntryRef { } return anchors; } + + public TrustAnchor findBySubjectAndPublicKey(X509Certificate cert) { + X509Certificate foundCert = mSource.findBySubjectAndPublicKey(cert); + if (foundCert == null) { + return null; + } + + return new TrustAnchor(foundCert, mOverridesPins); + } } diff --git a/core/java/android/security/net/config/DirectoryCertificateSource.java b/core/java/android/security/net/config/DirectoryCertificateSource.java new file mode 100644 index 000000000000..92c70920e040 --- /dev/null +++ b/core/java/android/security/net/config/DirectoryCertificateSource.java @@ -0,0 +1,138 @@ +/* + * Copyright (C) 2015 The Android Open Source Project + * + * 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 android.security.net.config; + +import android.os.Environment; +import android.os.UserHandle; +import android.util.ArraySet; +import android.util.Pair; +import java.io.BufferedInputStream; +import java.io.File; +import java.io.FileInputStream; +import java.io.InputStream; +import java.io.IOException; +import java.security.cert.Certificate; +import java.security.cert.CertificateException; +import java.security.cert.CertificateFactory; +import java.security.cert.X509Certificate; +import java.util.Set; +import libcore.io.IoUtils; + +import com.android.org.conscrypt.NativeCrypto; + +import javax.security.auth.x500.X500Principal; + +/** + * {@link CertificateSource} based on a directory where certificates are stored as individual files + * named after a hash of their SubjectName for more efficient lookups. + * @hide + */ +abstract class DirectoryCertificateSource implements CertificateSource { + private final File mDir; + private final Object mLock = new Object(); + private final CertificateFactory mCertFactory; + + private Set mCertificates; + + protected DirectoryCertificateSource(File caDir) { + mDir = caDir; + try { + mCertFactory = CertificateFactory.getInstance("X.509"); + } catch (CertificateException e) { + throw new RuntimeException("Failed to obtain X.509 CertificateFactory", e); + } + } + + protected abstract boolean isCertMarkedAsRemoved(String caFile); + + @Override + public Set getCertificates() { + // TODO: loading all of these is wasteful, we should instead use a keystore style API. + synchronized (mLock) { + if (mCertificates != null) { + return mCertificates; + } + + Set certs = new ArraySet(); + if (mDir.isDirectory()) { + for (String caFile : mDir.list()) { + if (isCertMarkedAsRemoved(caFile)) { + continue; + } + X509Certificate cert = readCertificate(caFile); + if (cert != null) { + certs.add(cert); + } + } + } + mCertificates = certs; + return mCertificates; + } + } + + @Override + public X509Certificate findBySubjectAndPublicKey(final X509Certificate cert) { + return findCert(cert.getSubjectX500Principal(), new CertSelector() { + @Override + public boolean match(X509Certificate ca) { + return ca.getPublicKey().equals(cert.getPublicKey()); + } + }); + } + + private static interface CertSelector { + boolean match(X509Certificate cert); + } + + private X509Certificate findCert(X500Principal subj, CertSelector selector) { + String hash = getHash(subj); + for (int index = 0; index >= 0; index++) { + String fileName = hash + "." + index; + if (!new File(mDir, fileName).exists()) { + break; + } + if (isCertMarkedAsRemoved(fileName)) { + continue; + } + X509Certificate cert = readCertificate(fileName); + if (!subj.equals(cert.getSubjectX500Principal())) { + continue; + } + if (selector.match(cert)) { + return cert; + } + } + return null; + } + + private String getHash(X500Principal name) { + int hash = NativeCrypto.X509_NAME_hash_old(name); + return IntegralToString.intToHexString(hash, false, 8); + } + + private X509Certificate readCertificate(String file) { + InputStream is = null; + try { + is = new BufferedInputStream(new FileInputStream(new File(mDir, file))); + return (X509Certificate) mCertFactory.generateCertificate(is); + } catch (CertificateException | IOException e) { + return null; + } finally { + IoUtils.closeQuietly(is); + } + } +} diff --git a/core/java/android/security/net/config/KeyStoreCertificateSource.java b/core/java/android/security/net/config/KeyStoreCertificateSource.java index 9167a9007325..7a01a6488a04 100644 --- a/core/java/android/security/net/config/KeyStoreCertificateSource.java +++ b/core/java/android/security/net/config/KeyStoreCertificateSource.java @@ -24,6 +24,8 @@ import java.security.cert.X509Certificate; import java.util.Enumeration; import java.util.Set; +import com.android.org.conscrypt.TrustedCertificateIndex; + /** * {@link CertificateSource} which provides certificates from trusted certificate entries of a * {@link KeyStore}. @@ -31,6 +33,7 @@ import java.util.Set; class KeyStoreCertificateSource implements CertificateSource { private final Object mLock = new Object(); private final KeyStore mKeyStore; + private TrustedCertificateIndex mIndex; private Set mCertificates; public KeyStoreCertificateSource(KeyStore ks) { @@ -39,24 +42,42 @@ class KeyStoreCertificateSource implements CertificateSource { @Override public Set getCertificates() { + ensureInitialized(); + return mCertificates; + } + + private void ensureInitialized() { synchronized (mLock) { if (mCertificates != null) { - return mCertificates; + return; } + try { + TrustedCertificateIndex localIndex = new TrustedCertificateIndex(); Set certificates = new ArraySet<>(mKeyStore.size()); for (Enumeration en = mKeyStore.aliases(); en.hasMoreElements();) { String alias = en.nextElement(); X509Certificate cert = (X509Certificate) mKeyStore.getCertificate(alias); if (cert != null) { certificates.add(cert); + localIndex.index(cert); } } + mIndex = localIndex; mCertificates = certificates; - return mCertificates; } catch (KeyStoreException e) { throw new RuntimeException("Failed to load certificates from KeyStore", e); } } } + + @Override + public X509Certificate findBySubjectAndPublicKey(X509Certificate cert) { + ensureInitialized(); + java.security.cert.TrustAnchor anchor = mIndex.findBySubjectAndPublicKey(cert); + if (anchor == null) { + return null; + } + return anchor.getTrustedCert(); + } } diff --git a/core/java/android/security/net/config/NetworkSecurityConfig.java b/core/java/android/security/net/config/NetworkSecurityConfig.java index 9eab80ca0771..2ab07b5abf18 100644 --- a/core/java/android/security/net/config/NetworkSecurityConfig.java +++ b/core/java/android/security/net/config/NetworkSecurityConfig.java @@ -22,6 +22,7 @@ import java.security.cert.X509Certificate; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.Comparator; import java.util.List; import java.util.Map; import java.util.Set; @@ -53,6 +54,19 @@ public final class NetworkSecurityConfig { mHstsEnforced = hstsEnforced; mPins = pins; mCertificatesEntryRefs = certificatesEntryRefs; + // Sort the certificates entry refs so that all entries that override pins come before + // non-override pin entries. This allows us to handle the case where a certificate is in + // multiple entry refs by returning the certificate from the first entry ref. + Collections.sort(mCertificatesEntryRefs, new Comparator() { + @Override + public int compare(CertificatesEntryRef lhs, CertificatesEntryRef rhs) { + if (lhs.overridesPins()) { + return rhs.overridesPins() ? 0 : -1; + } else { + return rhs.overridesPins() ? 1 : 0; + } + } + }); } public Set getTrustAnchors() { @@ -63,14 +77,15 @@ public final class NetworkSecurityConfig { // Merge trust anchors based on the X509Certificate. // If we see the same certificate in two TrustAnchors, one with overridesPins and one // without, the one with overridesPins wins. + // Because mCertificatesEntryRefs is sorted with all overridesPins anchors coming first + // this can be simplified to just using the first occurrence of a certificate. Map anchorMap = new ArrayMap<>(); for (CertificatesEntryRef ref : mCertificatesEntryRefs) { Set anchors = ref.getTrustAnchors(); for (TrustAnchor anchor : anchors) { - if (anchor.overridesPins) { - anchorMap.put(anchor.certificate, anchor); - } else if (!anchorMap.containsKey(anchor.certificate)) { - anchorMap.put(anchor.certificate, anchor); + X509Certificate cert = anchor.certificate; + if (!anchorMap.containsKey(cert)) { + anchorMap.put(cert, anchor); } } } @@ -108,6 +123,17 @@ public final class NetworkSecurityConfig { } } + /** @hide */ + public TrustAnchor findTrustAnchorBySubjectAndPublicKey(X509Certificate cert) { + for (CertificatesEntryRef ref : mCertificatesEntryRefs) { + TrustAnchor anchor = ref.findBySubjectAndPublicKey(cert); + if (anchor != null) { + return anchor; + } + } + return null; + } + /** * Return a {@link Builder} for the default {@code NetworkSecurityConfig}. * diff --git a/core/java/android/security/net/config/NetworkSecurityTrustManager.java b/core/java/android/security/net/config/NetworkSecurityTrustManager.java index 2b860fac45c1..6013c1e4023e 100644 --- a/core/java/android/security/net/config/NetworkSecurityTrustManager.java +++ b/core/java/android/security/net/config/NetworkSecurityTrustManager.java @@ -133,14 +133,8 @@ public class NetworkSecurityTrustManager implements X509TrustManager { return false; } X509Certificate anchorCert = chain.get(chain.size() - 1); - TrustAnchor chainAnchor = null; - // TODO: faster lookup - for (TrustAnchor anchor : mNetworkSecurityConfig.getTrustAnchors()) { - if (anchor.certificate.equals(anchorCert)) { - chainAnchor = anchor; - break; - } - } + TrustAnchor chainAnchor = + mNetworkSecurityConfig.findTrustAnchorBySubjectAndPublicKey(anchorCert); if (chainAnchor == null) { throw new CertificateException("Trusted chain does not end in a TrustAnchor"); } diff --git a/core/java/android/security/net/config/ResourceCertificateSource.java b/core/java/android/security/net/config/ResourceCertificateSource.java index 06dd9d42e364..b007f8f00a55 100644 --- a/core/java/android/security/net/config/ResourceCertificateSource.java +++ b/core/java/android/security/net/config/ResourceCertificateSource.java @@ -27,26 +27,29 @@ import java.security.cert.X509Certificate; import java.util.Collection; import java.util.Set; +import com.android.org.conscrypt.TrustedCertificateIndex; + /** * {@link CertificateSource} based on certificates contained in an application resource file. * @hide */ public class ResourceCertificateSource implements CertificateSource { - private Set mCertificates; + private final Object mLock = new Object(); private final int mResourceId; + + private Set mCertificates; private Context mContext; - private final Object mLock = new Object(); + private TrustedCertificateIndex mIndex; public ResourceCertificateSource(int resourceId, Context context) { mResourceId = resourceId; mContext = context.getApplicationContext(); } - @Override - public Set getCertificates() { + private void ensureInitialized() { synchronized (mLock) { if (mCertificates != null) { - return mCertificates; + return; } Set certificates = new ArraySet(); Collection certs; @@ -61,12 +64,30 @@ public class ResourceCertificateSource implements CertificateSource { } finally { IoUtils.closeQuietly(in); } + TrustedCertificateIndex indexLocal = new TrustedCertificateIndex(); for (Certificate cert : certs) { - certificates.add((X509Certificate) cert); + certificates.add((X509Certificate) cert); + indexLocal.index((X509Certificate) cert); } mCertificates = certificates; + mIndex = indexLocal; mContext = null; - return mCertificates; } } + + @Override + public Set getCertificates() { + ensureInitialized(); + return mCertificates; + } + + @Override + public X509Certificate findBySubjectAndPublicKey(X509Certificate cert) { + ensureInitialized(); + java.security.cert.TrustAnchor anchor = mIndex.findBySubjectAndPublicKey(cert); + if (anchor == null) { + return null; + } + return anchor.getTrustedCert(); + } } diff --git a/core/java/android/security/net/config/SystemCertificateSource.java b/core/java/android/security/net/config/SystemCertificateSource.java index 7649a977ff5c..abef7b453c79 100644 --- a/core/java/android/security/net/config/SystemCertificateSource.java +++ b/core/java/android/security/net/config/SystemCertificateSource.java @@ -18,29 +18,20 @@ package android.security.net.config; import android.os.Environment; import android.os.UserHandle; -import android.util.ArraySet; -import java.io.BufferedInputStream; import java.io.File; -import java.io.FileInputStream; -import java.io.InputStream; -import java.io.IOException; -import java.security.cert.Certificate; -import java.security.cert.CertificateException; -import java.security.cert.CertificateFactory; -import java.security.cert.X509Certificate; -import java.util.Set; -import libcore.io.IoUtils; /** * {@link CertificateSource} based on the system trusted CA store. * @hide */ -public class SystemCertificateSource implements CertificateSource { +public final class SystemCertificateSource extends DirectoryCertificateSource { private static final SystemCertificateSource INSTANCE = new SystemCertificateSource(); - private Set mSystemCerts = null; - private final Object mLock = new Object(); + private final File mUserRemovedCaDir; private SystemCertificateSource() { + super(new File(System.getenv("ANDROID_ROOT") + "/etc/security/cacerts")); + File configDir = Environment.getUserConfigDirectory(UserHandle.myUserId()); + mUserRemovedCaDir = new File(configDir, "cacerts-removed"); } public static SystemCertificateSource getInstance() { @@ -48,54 +39,7 @@ public class SystemCertificateSource implements CertificateSource { } @Override - public Set getCertificates() { - // TODO: loading all of these is wasteful, we should instead use a keystore style API. - synchronized (mLock) { - if (mSystemCerts != null) { - return mSystemCerts; - } - CertificateFactory certFactory; - try { - certFactory = CertificateFactory.getInstance("X.509"); - } catch (CertificateException e) { - throw new RuntimeException("Failed to obtain X.509 CertificateFactory", e); - } - - final String ANDROID_ROOT = System.getenv("ANDROID_ROOT"); - final File systemCaDir = new File(ANDROID_ROOT + "/etc/security/cacerts"); - final File configDir = Environment.getUserConfigDirectory(UserHandle.myUserId()); - final File userRemovedCaDir = new File(configDir, "cacerts-removed"); - // Sanity check - if (!systemCaDir.isDirectory()) { - throw new AssertionError(systemCaDir + " is not a directory"); - } - - Set systemCerts = new ArraySet(); - for (String caFile : systemCaDir.list()) { - // Skip any CAs in the user's deleted directory. - if (new File(userRemovedCaDir, caFile).exists()) { - continue; - } - InputStream is = null; - try { - is = new BufferedInputStream( - new FileInputStream(new File(systemCaDir, caFile))); - systemCerts.add((X509Certificate) certFactory.generateCertificate(is)); - } catch (CertificateException | IOException e) { - // Don't rethrow to be consistent with conscrypt's cert loading code. - continue; - } finally { - IoUtils.closeQuietly(is); - } - } - mSystemCerts = systemCerts; - return mSystemCerts; - } - } - - public void onCertificateStorageChange() { - synchronized (mLock) { - mSystemCerts = null; - } + protected boolean isCertMarkedAsRemoved(String caFile) { + return new File(mUserRemovedCaDir, caFile).exists(); } } diff --git a/core/java/android/security/net/config/UserCertificateSource.java b/core/java/android/security/net/config/UserCertificateSource.java index e9d5aa1e2f34..1a7d92456a3d 100644 --- a/core/java/android/security/net/config/UserCertificateSource.java +++ b/core/java/android/security/net/config/UserCertificateSource.java @@ -18,29 +18,18 @@ package android.security.net.config; import android.os.Environment; import android.os.UserHandle; -import android.util.ArraySet; -import java.io.BufferedInputStream; import java.io.File; -import java.io.FileInputStream; -import java.io.InputStream; -import java.io.IOException; -import java.security.cert.Certificate; -import java.security.cert.CertificateException; -import java.security.cert.CertificateFactory; -import java.security.cert.X509Certificate; -import java.util.Set; -import libcore.io.IoUtils; /** * {@link CertificateSource} based on the user-installed trusted CA store. * @hide */ -public class UserCertificateSource implements CertificateSource { +public final class UserCertificateSource extends DirectoryCertificateSource { private static final UserCertificateSource INSTANCE = new UserCertificateSource(); - private Set mUserCerts = null; - private final Object mLock = new Object(); private UserCertificateSource() { + super(new File( + Environment.getUserConfigDirectory(UserHandle.myUserId()), "cacerts-added")); } public static UserCertificateSource getInstance() { @@ -48,45 +37,7 @@ public class UserCertificateSource implements CertificateSource { } @Override - public Set getCertificates() { - // TODO: loading all of these is wasteful, we should instead use a keystore style API. - synchronized (mLock) { - if (mUserCerts != null) { - return mUserCerts; - } - CertificateFactory certFactory; - try { - certFactory = CertificateFactory.getInstance("X.509"); - } catch (CertificateException e) { - throw new RuntimeException("Failed to obtain X.509 CertificateFactory", e); - } - final File configDir = Environment.getUserConfigDirectory(UserHandle.myUserId()); - final File userCaDir = new File(configDir, "cacerts-added"); - Set userCerts = new ArraySet(); - // If the user hasn't added any certificates the directory may not exist. - if (userCaDir.isDirectory()) { - for (String caFile : userCaDir.list()) { - InputStream is = null; - try { - is = new BufferedInputStream( - new FileInputStream(new File(userCaDir, caFile))); - userCerts.add((X509Certificate) certFactory.generateCertificate(is)); - } catch (CertificateException | IOException e) { - // Don't rethrow to be consistent with conscrypt's cert loading code. - continue; - } finally { - IoUtils.closeQuietly(is); - } - } - } - mUserCerts = userCerts; - return mUserCerts; - } - } - - public void onCertificateStorageChange() { - synchronized (mLock) { - mUserCerts = null; - } + protected boolean isCertMarkedAsRemoved(String caFile) { + return false; } } diff --git a/tests/NetworkSecurityConfigTest/res/xml/override_dedup.xml b/tests/NetworkSecurityConfigTest/res/xml/override_dedup.xml new file mode 100644 index 000000000000..5ba56754e768 --- /dev/null +++ b/tests/NetworkSecurityConfigTest/res/xml/override_dedup.xml @@ -0,0 +1,20 @@ + + + + + android.com + + aaaaaaaaIAq2Y49orFOOQKurWxmmSFZhBCoQYcRhJ3Y= + + + + + + + + + + + + diff --git a/tests/NetworkSecurityConfigTest/src/android/security/net/config/TestCertificateSource.java b/tests/NetworkSecurityConfigTest/src/android/security/net/config/TestCertificateSource.java index 92eadc06cd49..69b2a9d55642 100644 --- a/tests/NetworkSecurityConfigTest/src/android/security/net/config/TestCertificateSource.java +++ b/tests/NetworkSecurityConfigTest/src/android/security/net/config/TestCertificateSource.java @@ -19,15 +19,29 @@ package android.security.net.config; import java.util.Set; import java.security.cert.X509Certificate; +import com.android.org.conscrypt.TrustedCertificateIndex; + /** @hide */ public class TestCertificateSource implements CertificateSource { private final Set mCertificates; + private final TrustedCertificateIndex mIndex = new TrustedCertificateIndex(); public TestCertificateSource(Set certificates) { mCertificates = certificates; + for (X509Certificate cert : certificates) { + mIndex.index(cert); + } } public Set getCertificates() { return mCertificates; } + + public X509Certificate findBySubjectAndPublicKey(X509Certificate cert) { + java.security.cert.TrustAnchor anchor = mIndex.findBySubjectAndPublicKey(cert); + if (anchor == null) { + return null; + } + return anchor.getTrustedCert(); + } } diff --git a/tests/NetworkSecurityConfigTest/src/android/security/net/config/XmlConfigTests.java b/tests/NetworkSecurityConfigTest/src/android/security/net/config/XmlConfigTests.java index c6f3680f455c..998bb681dd24 100644 --- a/tests/NetworkSecurityConfigTest/src/android/security/net/config/XmlConfigTests.java +++ b/tests/NetworkSecurityConfigTest/src/android/security/net/config/XmlConfigTests.java @@ -402,4 +402,22 @@ public class XmlConfigTests extends AndroidTestCase { context.init(null, tms, null); TestUtils.assertConnectionSucceeds(context, "android.com" , 443); } + + public void testDebugDedup() throws Exception { + XmlConfigSource source = new XmlConfigSource(getContext(), R.xml.override_dedup, true); + ApplicationConfig appConfig = new ApplicationConfig(source); + assertTrue(appConfig.hasPerDomainConfigs()); + // Check android.com. + NetworkSecurityConfig config = appConfig.getConfigForHostname("android.com"); + PinSet pinSet = config.getPins(); + assertFalse(pinSet.pins.isEmpty()); + // Check that all TrustAnchors come from the override pins debug source. + for (TrustAnchor anchor : config.getTrustAnchors()) { + assertTrue(anchor.overridesPins); + } + // Try connections. + SSLContext context = TestUtils.getSSLContext(source); + TestUtils.assertConnectionSucceeds(context, "android.com", 443); + TestUtils.assertUrlConnectionSucceeds(context, "android.com", 443); + } } -- 2.11.0