From cbd3309c2f3d31b7f61fd733dedeaa1ff9507131 Mon Sep 17 00:00:00 2001 From: jwilson Date: Mon, 28 Mar 2016 14:56:42 -0400 Subject: [PATCH] Use X509TrustManagerExtensions on Android 17+. Closes: https://github.com/square/okhttp/issues/2407 --- .../okhttp3/CertificateChainCleanerTest.java | 33 ++++++------ .../main/java/okhttp3/CertificatePinner.java | 2 +- .../internal/tls/CertificateChainCleaner.java | 52 +++++++++++++++++-- .../okhttp3/internal/tls/TrustRootIndex.java | 4 ++ 4 files changed, 71 insertions(+), 20 deletions(-) diff --git a/okhttp-tests/src/test/java/okhttp3/CertificateChainCleanerTest.java b/okhttp-tests/src/test/java/okhttp3/CertificateChainCleanerTest.java index a7a1747f6dcf..8bfcb8c2aed2 100644 --- a/okhttp-tests/src/test/java/okhttp3/CertificateChainCleanerTest.java +++ b/okhttp-tests/src/test/java/okhttp3/CertificateChainCleanerTest.java @@ -34,7 +34,7 @@ public final class CertificateChainCleanerTest { .serialNumber("1") .build(); CertificateChainCleaner cleaner = CertificateChainCleaner.get(root.certificate); - assertEquals(list(root), cleaner.clean(list(root))); + assertEquals(list(root), cleaner.clean(list(root), "hostname")); } @Test public void normalizeUnknownSelfSignedCertificate() throws Exception { @@ -44,7 +44,7 @@ public final class CertificateChainCleanerTest { CertificateChainCleaner cleaner = CertificateChainCleaner.get(); try { - cleaner.clean(list(root)); + cleaner.clean(list(root), "hostname"); fail(); } catch (SSLPeerUnverifiedException expected) { } @@ -64,7 +64,7 @@ public final class CertificateChainCleanerTest { .build(); CertificateChainCleaner cleaner = CertificateChainCleaner.get(root.certificate); - assertEquals(list(certB, certA, root), cleaner.clean(list(certB, certA, root))); + assertEquals(list(certB, certA, root), cleaner.clean(list(certB, certA, root), "hostname")); } @Test public void orderedChainOfCertificatesWithoutRoot() throws Exception { @@ -81,7 +81,8 @@ public final class CertificateChainCleanerTest { .build(); CertificateChainCleaner cleaner = CertificateChainCleaner.get(root.certificate); - assertEquals(list(certB, certA, root), cleaner.clean(list(certB, certA))); // Root is added! + assertEquals(list(certB, certA, root), + cleaner.clean(list(certB, certA), "hostname")); // Root is added! } @Test public void unorderedChainOfCertificatesWithRoot() throws Exception { @@ -102,7 +103,8 @@ public final class CertificateChainCleanerTest { .build(); CertificateChainCleaner cleaner = CertificateChainCleaner.get(root.certificate); - assertEquals(list(certC, certB, certA, root), cleaner.clean(list(certC, certA, root, certB))); + assertEquals(list(certC, certB, certA, root), + cleaner.clean(list(certC, certA, root, certB), "hostname")); } @Test public void unorderedChainOfCertificatesWithoutRoot() throws Exception { @@ -123,7 +125,8 @@ public final class CertificateChainCleanerTest { .build(); CertificateChainCleaner cleaner = CertificateChainCleaner.get(root.certificate); - assertEquals(list(certC, certB, certA, root), cleaner.clean(list(certC, certA, certB))); + assertEquals(list(certC, certB, certA, root), + cleaner.clean(list(certC, certA, certB), "hostname")); } @Test public void unrelatedCertificatesAreOmitted() throws Exception { @@ -144,7 +147,7 @@ public final class CertificateChainCleanerTest { CertificateChainCleaner cleaner = CertificateChainCleaner.get(root.certificate); assertEquals(list(certB, certA, root), - cleaner.clean(list(certB, certUnnecessary, certA, root))); + cleaner.clean(list(certB, certUnnecessary, certA, root), "hostname")); } @Test public void chainGoesAllTheWayToSelfSignedRoot() throws Exception { @@ -167,11 +170,11 @@ public final class CertificateChainCleanerTest { CertificateChainCleaner cleaner = CertificateChainCleaner.get( selfSigned.certificate, trusted.certificate); assertEquals(list(certB, certA, trusted, selfSigned), - cleaner.clean(list(certB, certA))); + cleaner.clean(list(certB, certA), "hostname")); assertEquals(list(certB, certA, trusted, selfSigned), - cleaner.clean(list(certB, certA, trusted))); + cleaner.clean(list(certB, certA, trusted), "hostname")); assertEquals(list(certB, certA, trusted, selfSigned), - cleaner.clean(list(certB, certA, trusted, selfSigned))); + cleaner.clean(list(certB, certA, trusted, selfSigned), "hostname")); } @Test public void trustedRootNotSelfSigned() throws Exception { @@ -193,9 +196,9 @@ public final class CertificateChainCleanerTest { CertificateChainCleaner cleaner = CertificateChainCleaner.get(trusted.certificate); assertEquals(list(certificate, intermediateCa, trusted), - cleaner.clean(list(certificate, intermediateCa))); + cleaner.clean(list(certificate, intermediateCa), "hostname")); assertEquals(list(certificate, intermediateCa, trusted), - cleaner.clean(list(certificate, intermediateCa, trusted))); + cleaner.clean(list(certificate, intermediateCa, trusted), "hostname")); } @Test public void chainMaxLength() throws Exception { @@ -207,8 +210,8 @@ public final class CertificateChainCleanerTest { X509Certificate root = heldCertificates.get(heldCertificates.size() - 1).certificate; CertificateChainCleaner cleaner = CertificateChainCleaner.get(root); - assertEquals(certificates, cleaner.clean(certificates)); - assertEquals(certificates, cleaner.clean(certificates.subList(0, 9))); + assertEquals(certificates, cleaner.clean(certificates, "hostname")); + assertEquals(certificates, cleaner.clean(certificates.subList(0, 9), "hostname")); } @Test public void chainTooLong() throws Exception { @@ -221,7 +224,7 @@ public final class CertificateChainCleanerTest { X509Certificate root = heldCertificates.get(heldCertificates.size() - 1).certificate; CertificateChainCleaner cleaner = CertificateChainCleaner.get(root); try { - cleaner.clean(certificates); + cleaner.clean(certificates, "hostname"); fail(); } catch (SSLPeerUnverifiedException expected) { } diff --git a/okhttp/src/main/java/okhttp3/CertificatePinner.java b/okhttp/src/main/java/okhttp3/CertificatePinner.java index f3344d00dd4d..964959470678 100644 --- a/okhttp/src/main/java/okhttp3/CertificatePinner.java +++ b/okhttp/src/main/java/okhttp3/CertificatePinner.java @@ -145,7 +145,7 @@ public void check(String hostname, List peerCertificates) if (pins.isEmpty()) return; if (certificateChainCleaner != null) { - peerCertificates = certificateChainCleaner.clean(peerCertificates); + peerCertificates = certificateChainCleaner.clean(peerCertificates, hostname); } for (int c = 0, certsSize = peerCertificates.size(); c < certsSize; c++) { diff --git a/okhttp/src/main/java/okhttp3/internal/tls/CertificateChainCleaner.java b/okhttp/src/main/java/okhttp3/internal/tls/CertificateChainCleaner.java index 0e056cb5dc96..672e48651b0d 100644 --- a/okhttp/src/main/java/okhttp3/internal/tls/CertificateChainCleaner.java +++ b/okhttp/src/main/java/okhttp3/internal/tls/CertificateChainCleaner.java @@ -16,6 +16,9 @@ */ package okhttp3.internal.tls; +import java.lang.reflect.Constructor; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; import java.security.GeneralSecurityException; import java.security.cert.Certificate; import java.security.cert.X509Certificate; @@ -38,11 +41,20 @@ * pinning. */ public abstract class CertificateChainCleaner { - public abstract List clean(List chain) + public abstract List clean(List chain, String hostname) throws SSLPeerUnverifiedException; public static CertificateChainCleaner get(X509TrustManager trustManager) { - return new BasicCertificateChainCleaner(TrustRootIndex.get(trustManager)); + try { + Class extensionsClass = Class.forName("android.net.http.X509TrustManagerExtensions"); + Constructor constructor = extensionsClass.getConstructor(X509TrustManager.class); + Object extensions = constructor.newInstance(trustManager); + Method checkServerTrusted = extensionsClass.getMethod( + "checkServerTrusted", X509Certificate[].class, String.class, String.class); + return new AndroidCertificateChainCleaner(extensions, checkServerTrusted); + } catch (Exception e) { + return new BasicCertificateChainCleaner(TrustRootIndex.get(trustManager)); + } } public static CertificateChainCleaner get(X509Certificate... caCerts) { @@ -51,7 +63,8 @@ public static CertificateChainCleaner get(X509Certificate... caCerts) { /** * A certificate chain cleaner that uses a set of trusted root certificates to build the trusted - * chain. + * chain. This class duplicates the clean chain building performed during the TLS handshake. We + * prefer other mechanisms where they exist, such as with {@link AndroidCertificateChainCleaner}. * *

This class includes code from Conscrypt's {@code * TrustManagerImpl} and {@code TrustedCertificateIndex}. @@ -73,7 +86,7 @@ public BasicCertificateChainCleaner(TrustRootIndex trustRootIndex) { * constructed. This is unexpected unless the trust root index in this class has a different * trust manager than what was used to establish {@code chain}. */ - @Override public List clean(List chain) + @Override public List clean(List chain, String hostname) throws SSLPeerUnverifiedException { Deque queue = new ArrayDeque<>(chain); List result = new ArrayList<>(); @@ -134,4 +147,35 @@ private boolean verifySignature(X509Certificate toVerify, X509Certificate signin } } } + + /** + * X509TrustManagerExtensions was added to Android in API 17 (Android 4.2, released in late 2012). + * This is the best way to get a clean chain on Android because it uses the same code as the TLS + * handshake. + */ + static final class AndroidCertificateChainCleaner extends CertificateChainCleaner { + private final Object x509TrustManagerExtensions; + private final Method checkServerTrusted; + + AndroidCertificateChainCleaner(Object x509TrustManagerExtensions, Method checkServerTrusted) { + this.x509TrustManagerExtensions = x509TrustManagerExtensions; + this.checkServerTrusted = checkServerTrusted; + } + + @SuppressWarnings({"unchecked", "SuspiciousToArrayCall"}) // Reflection on List. + @Override public List clean(List chain, String hostname) + throws SSLPeerUnverifiedException { + try { + X509Certificate[] certificates = chain.toArray(new X509Certificate[chain.size()]); + return (List) checkServerTrusted.invoke( + x509TrustManagerExtensions, certificates, "RSA", hostname); + } catch (InvocationTargetException e) { + SSLPeerUnverifiedException exception = new SSLPeerUnverifiedException(e.getMessage()); + exception.initCause(e); + throw exception; + } catch (IllegalAccessException e) { + throw new AssertionError(e); + } + } + } } diff --git a/okhttp/src/main/java/okhttp3/internal/tls/TrustRootIndex.java b/okhttp/src/main/java/okhttp3/internal/tls/TrustRootIndex.java index 3c314d70742c..fcc7468e0e01 100644 --- a/okhttp/src/main/java/okhttp3/internal/tls/TrustRootIndex.java +++ b/okhttp/src/main/java/okhttp3/internal/tls/TrustRootIndex.java @@ -52,6 +52,10 @@ public static TrustRootIndex get(X509Certificate... caCerts) { * An index of trusted root certificates that exploits knowledge of Android implementation * details. This class is potentially much faster to initialize than {@link BasicTrustRootIndex} * because it doesn't need to load and index trusted CA certificates. + * + *

This class uses APIs added to Android in API 14 (Android 4.0, released October 2011). This + * class shouldn't be used in Android API 17 or better because those releases are better served by + * {@link CertificateChainCleaner.AndroidCertificateChainCleaner}. */ static final class AndroidTrustRootIndex extends TrustRootIndex { private final X509TrustManager trustManager;