From b8009c1a146dece285f51f8b9f7685d674c2e9e1 Mon Sep 17 00:00:00 2001 From: Danny Reidenbach Date: Fri, 31 Jul 2020 22:14:59 +0000 Subject: [PATCH] Updated verify method to allow certificates to be passed in directly --- .../main/java/org/conscrypt/Conscrypt.java | 3 +- .../conscrypt/ConscryptHostnameVerifier.java | 3 +- .../org/conscrypt/OkHostnameVerifier.java | 16 ++- .../java/org/conscrypt/TrustManagerImpl.java | 20 +-- .../org/conscrypt/HostnameVerifierTest.java | 130 ++++++++++-------- .../org/conscrypt/TrustManagerImplTest.java | 2 +- 6 files changed, 92 insertions(+), 82 deletions(-) diff --git a/common/src/main/java/org/conscrypt/Conscrypt.java b/common/src/main/java/org/conscrypt/Conscrypt.java index b87336d04..3eb3c736f 100644 --- a/common/src/main/java/org/conscrypt/Conscrypt.java +++ b/common/src/main/java/org/conscrypt/Conscrypt.java @@ -21,6 +21,7 @@ import java.security.KeyManagementException; import java.security.PrivateKey; import java.security.Provider; +import java.security.cert.X509Certificate; import java.util.Properties; import javax.net.ssl.HostnameVerifier; import javax.net.ssl.HttpsURLConnection; @@ -791,7 +792,7 @@ public static ConscryptHostnameVerifier wrapHostnameVerifier(final HostnameVerif // Cannot find HttpsURLConnection.HostnameVerifier return new ConscryptHostnameVerifier() { @Override - public boolean verify(String hostname, SSLSession session) { + public boolean verify(X509Certificate[] certs, String hostname, SSLSession session) { return verifier.verify(hostname, session); } }; diff --git a/common/src/main/java/org/conscrypt/ConscryptHostnameVerifier.java b/common/src/main/java/org/conscrypt/ConscryptHostnameVerifier.java index e09ac1ebc..ffdd762f7 100644 --- a/common/src/main/java/org/conscrypt/ConscryptHostnameVerifier.java +++ b/common/src/main/java/org/conscrypt/ConscryptHostnameVerifier.java @@ -17,6 +17,7 @@ package org.conscrypt; import javax.net.ssl.SSLSession; +import java.security.cert.X509Certificate; /** * This interface is used to implement hostname verification in Conscrypt. Unlike with @@ -29,6 +30,6 @@ public interface ConscryptHostnameVerifier { * Returns whether the given hostname is allowable given the peer's authentication information * from the given session. */ - boolean verify(String hostname, SSLSession session); + boolean verify(X509Certificate[] certs, String hostname, SSLSession session); } diff --git a/common/src/main/java/org/conscrypt/OkHostnameVerifier.java b/common/src/main/java/org/conscrypt/OkHostnameVerifier.java index f5b9f771e..57f12f483 100644 --- a/common/src/main/java/org/conscrypt/OkHostnameVerifier.java +++ b/common/src/main/java/org/conscrypt/OkHostnameVerifier.java @@ -70,12 +70,16 @@ public static OkHostnameVerifier strictInstance() { // END Android-changed: Add a mode which disallows top-level domain wildcards. b/144694112 @Override - public boolean verify(String host, SSLSession session) { - try { - Certificate[] certificates = session.getPeerCertificates(); - return verify(host, (X509Certificate) certificates[0]); - } catch (SSLException e) { - return false; + public boolean verify(X509Certificate[] certs, String host, SSLSession session) { + if (certs.length > 0) { + return verify(host, certs[0]); + } else { + try { + Certificate[] certificates = session.getPeerCertificates(); + return verify(host, (X509Certificate) certificates[0]); + } catch (SSLException e) { + return false; + } } } diff --git a/common/src/main/java/org/conscrypt/TrustManagerImpl.java b/common/src/main/java/org/conscrypt/TrustManagerImpl.java index e561556de..2a17068a4 100644 --- a/common/src/main/java/org/conscrypt/TrustManagerImpl.java +++ b/common/src/main/java/org/conscrypt/TrustManagerImpl.java @@ -404,7 +404,7 @@ private List checkTrusted(X509Certificate[] certs, String authT String identificationAlgorithm = parameters.getEndpointIdentificationAlgorithm(); if ("HTTPS".equalsIgnoreCase(identificationAlgorithm)) { ConscryptHostnameVerifier verifier = getHttpsVerifier(); - if (!verifier.verify(hostname, session)) { + if (!verifier.verify(certs, hostname, session)) { throw new CertificateException("No subjectAltNames on the certificate match"); } } @@ -998,28 +998,14 @@ void setHostnameVerifier(ConscryptHostnameVerifier verifier) { * * @see #setHostnameVerifier(ConscryptHostnameVerifier) */ - ConscryptHostnameVerifier getHostnameVerifier() { // doggo + ConscryptHostnameVerifier getHostnameVerifier() { return hostnameVerifier; } -// private enum GlobalHostnameVerifierAdapter implements ConscryptHostnameVerifier { -// INSTANCE; -// -// @Override -// public boolean verify(String hostname, SSLSession session) { -// return HttpsURLConnection.getDefaultHostnameVerifier().verify(hostname, session); -// } -// } - - private ConscryptHostnameVerifier getHttpsVerifier() { // doggo + private ConscryptHostnameVerifier getHttpsVerifier() { if (hostnameVerifier != null) { return hostnameVerifier; } -// // remove -// ConscryptHostnameVerifier defaultVerifier = getDefaultHostnameVerifier(); // this is still needed for current tests. Will I have to remove this and update the tests to work with default or are teh predefined tests still valid? -// if (defaultVerifier != null) { -// return defaultVerifier; -// } return Platform.getDefaultHostnameVerifier(); } diff --git a/common/src/test/java/org/conscrypt/HostnameVerifierTest.java b/common/src/test/java/org/conscrypt/HostnameVerifierTest.java index f552292f5..5ed130283 100644 --- a/common/src/test/java/org/conscrypt/HostnameVerifierTest.java +++ b/common/src/test/java/org/conscrypt/HostnameVerifierTest.java @@ -28,6 +28,7 @@ import java.security.cert.X509Certificate; import java.util.Arrays; import java.util.Collection; +import javax.net.ssl.SSLException; import javax.net.ssl.SSLPeerUnverifiedException; import javax.net.ssl.SSLSession; import javax.security.auth.x500.X500Principal; @@ -57,9 +58,8 @@ public FakeSSLSession(Certificate... certificates) throws Exception { public Certificate[] getPeerCertificates() throws SSLPeerUnverifiedException { if (certificates.length == 0) { throw new SSLPeerUnverifiedException("peer not authenticated"); - } else { - return certificates; } + return certificates; } } @@ -82,7 +82,8 @@ public static Collection data() { @Test public void verify() throws Exception { FakeSSLSession session = new FakeSSLSession(); - assertFalse(verifier.verify("localhost", session)); + X509Certificate[] certs = {}; + assertFalse(verifier.verify(certs,"localhost", session)); } @Test public void verifyCn() throws Exception { @@ -115,9 +116,10 @@ public static Collection data() { + "-----END CERTIFICATE-----\n"); // Android-changed: Ignore common name in hostname verification. http://b/70278814 // assertTrue(verifier.verify("foo.com", session)); - assertFalse(verifier.verify("foo.com", session)); - assertFalse(verifier.verify("a.foo.com", session)); - assertFalse(verifier.verify("bar.com", session)); + X509Certificate[] certs = {}; + assertFalse(verifier.verify(certs, "foo.com", session)); + assertFalse(verifier.verify(certs, "a.foo.com", session)); + assertFalse(verifier.verify(certs, "bar.com", session)); } @Test public void verifyNonAsciiCn() throws Exception { @@ -150,8 +152,9 @@ public static Collection data() { + "-----END CERTIFICATE-----\n"); // Android-changed: Ignore common name in hostname verification. http://b/70278814 // assertTrue(verifier.verify("\u82b1\u5b50.co.jp", session)); - assertFalse(verifier.verify("\u82b1\u5b50.co.jp", session)); - assertFalse(verifier.verify("a.\u82b1\u5b50.co.jp", session)); + X509Certificate[] certs = {}; + assertFalse(verifier.verify(certs, "\u82b1\u5b50.co.jp", session)); + assertFalse(verifier.verify(certs, "a.\u82b1\u5b50.co.jp", session)); } @Test public void verifySubjectAlt() throws Exception { @@ -183,10 +186,11 @@ public static Collection data() { + "2xq+8bc6HojdtbCyug/fvBZvZqQXSmU8m8IVcMmWMz0ZQO8ee3QkBHMZfCy7P/kr\n" + "VbWx/uETImUu+NZg22ewEw==\n" + "-----END CERTIFICATE-----\n"); - assertFalse(verifier.verify("foo.com", session)); - assertFalse(verifier.verify("a.foo.com", session)); - assertTrue(verifier.verify("bar.com", session)); - assertFalse(verifier.verify("a.bar.com", session)); + X509Certificate[] certs = {}; + assertFalse(verifier.verify(certs, "foo.com", session)); + assertFalse(verifier.verify(certs, "a.foo.com", session)); + assertTrue(verifier.verify(certs, "bar.com", session)); + assertFalse(verifier.verify(certs, "a.bar.com", session)); } /** @@ -224,8 +228,9 @@ public static Collection data() { + "sWIKHYrmhCIRshUNohGXv50m2o+1w9oWmQ6Dkq7lCjfXfUB4wIbggJjpyEtbNqBt\n" + "j4MC2x5rfsLKKqToKmNE7pFEgqwe8//Aar1b+Qj+\n" + "-----END CERTIFICATE-----\n"); - assertTrue(verifier.verify("foo.com", session)); - assertFalse(verifier.verify("a.foo.com", session)); + X509Certificate[] certs = {}; + assertTrue(verifier.verify(certs, "foo.com", session)); + assertFalse(verifier.verify(certs, "a.foo.com", session)); // these checks test alternative subjects. The test data contains an // alternative subject starting with a japanese kanji character. This is // not supported by Android because the underlying implementation from @@ -264,10 +269,11 @@ public static Collection data() { + "qAVqixM+J0qJmQStgAc53i2aTMvAQu3A3snvH/PHTBo+5UL72n9S1kZyNCsVf1Qo\n" + "n8jKTiRriEM+fMFlcgQP284EBFzYHyCXFb9O/hMjK2+6mY9euMB1U1aFFzM/Bg==\n" + "-----END CERTIFICATE-----\n"); - assertTrue(verifier.verify("foo.com", session)); - assertFalse(verifier.verify("a.foo.com", session)); - assertTrue(verifier.verify("foo.com", session)); - assertFalse(verifier.verify("a.foo.com", session)); + X509Certificate[] certs = {}; + assertTrue(verifier.verify(certs, "foo.com", session)); + assertFalse(verifier.verify(certs, "a.foo.com", session)); + assertTrue(verifier.verify(certs, "foo.com", session)); + assertFalse(verifier.verify(certs, "a.foo.com", session)); } @Test public void verifyMultipleCn() throws Exception { @@ -299,14 +305,15 @@ public static Collection data() { + "SpQFCfo02NO0uNRDPUdJx2huycdNb+AXHaO7eXevDLJ+QnqImIzxWiY6zLOdzjjI\n" + "VBMkLHmnP7SjGSQ3XA4ByrQOxfOUTyLyE7NuemhHppuQPxE=\n" + "-----END CERTIFICATE-----\n"); - assertFalse(verifier.verify("foo.com", session)); - assertFalse(verifier.verify("a.foo.com", session)); - assertFalse(verifier.verify("bar.com", session)); - assertFalse(verifier.verify("a.bar.com", session)); + X509Certificate[] certs = {}; + assertFalse(verifier.verify(certs, "foo.com", session)); + assertFalse(verifier.verify(certs, "a.foo.com", session)); + assertFalse(verifier.verify(certs, "bar.com", session)); + assertFalse(verifier.verify(certs, "a.bar.com", session)); // Android-changed: Ignore common name in hostname verification. http://b/70278814 // assertTrue(verifier.verify("\u82b1\u5b50.co.jp", session)); - assertFalse(verifier.verify("\u82b1\u5b50.co.jp", session)); - assertFalse(verifier.verify("a.\u82b1\u5b50.co.jp", session)); + assertFalse(verifier.verify(certs, "\u82b1\u5b50.co.jp", session)); + assertFalse(verifier.verify(certs, "a.\u82b1\u5b50.co.jp", session)); } @Test public void verifyWilcardCn() throws Exception { @@ -337,14 +344,15 @@ public static Collection data() { + "G9Z6tyMbmfRY+dLSh3a9JwoEcBUso6EWYBakLbq4nG/nvYdYvG9ehrnLVwZFL82e\n" + "l3Q/RK95bnA6cuRClGusLad0e6bjkBzx/VQ3VarDEpAkTLUGVAa0CLXtnyc=\n" + "-----END CERTIFICATE-----\n"); - assertFalse(verifier.verify("foo.com", session)); + X509Certificate[] certs = {}; + assertFalse(verifier.verify(certs, "foo.com", session)); // Android-changed: Ignore common name in hostname verification. http://b/70278814 // assertTrue(verifier.verify("www.foo.com", session)); - assertFalse(verifier.verify("www.foo.com", session)); + assertFalse(verifier.verify(certs, "www.foo.com", session)); // Android-changed: Ignore common name in hostname verification. http://b/70278814 // assertTrue(verifier.verify("\u82b1\u5b50.foo.com", session)); - assertFalse(verifier.verify("\u82b1\u5b50.foo.com", session)); - assertFalse(verifier.verify("a.b.foo.com", session)); + assertFalse(verifier.verify(certs, "\u82b1\u5b50.foo.com", session)); + assertFalse(verifier.verify(certs, "a.b.foo.com", session)); } @Test public void verifyWilcardCnOnTld() throws Exception { @@ -376,12 +384,13 @@ public static Collection data() { + "UGPLEUDzRHMPHLnSqT1n5UU5UDRytbjJPXzF+l/+WZIsanefWLsxnkgAuZe/oMMF\n" + "EJMryEzOjg4Tfuc5qM0EXoPcQ/JlheaxZ40p2IyHqbsWV4MRYuFH4bkM\n" + "-----END CERTIFICATE-----\n"); + X509Certificate[] certs = {}; // Android-changed: Ignore common name in hostname verification. http://b/70278814 // assertTrue(verifier.verify("foo.co.jp", session)); - assertFalse(verifier.verify("foo.co.jp", session)); + assertFalse(verifier.verify(certs, "foo.co.jp", session)); // Android-changed: Ignore common name in hostname verification. http://b/70278814 // assertTrue(verifier.verify("\u82b1\u5b50.co.jp", session)); - assertFalse(verifier.verify("\u82b1\u5b50.co.jp", session)); + assertFalse(verifier.verify(certs, "\u82b1\u5b50.co.jp", session)); } /** @@ -419,11 +428,12 @@ public static Collection data() { + "qFr0AIZKBlg6NZZFf/0dP9zcKhzSriW27bY0XfzA6GSiRDXrDjgXq6baRT6YwgIg\n" + "pgJsDbJtZfHnV1nd3M6zOtQPm1TIQpNmMMMd/DPrGcUQerD3\n" + "-----END CERTIFICATE-----\n"); + X509Certificate[] certs = {}; // try the foo.com variations - assertTrue(verifier.verify("foo.com", session)); - assertTrue(verifier.verify("www.foo.com", session)); - assertTrue(verifier.verify("\u82b1\u5b50.foo.com", session)); - assertFalse(verifier.verify("a.b.foo.com", session)); + assertTrue(verifier.verify(certs, "foo.com", session)); + assertTrue(verifier.verify(certs, "www.foo.com", session)); + assertTrue(verifier.verify(certs, "\u82b1\u5b50.foo.com", session)); + assertFalse(verifier.verify(certs, "a.b.foo.com", session)); // these checks test alternative subjects. The test data contains an // alternative subject starting with a japanese kanji character. This is // not supported by Android because the underlying implementation from @@ -459,15 +469,18 @@ public static Collection data() { + "DCzOCv9Ma6Lv5o5jcYWVxvBSTsnt22hsJpWD1K7iY9lbkLwl0ivn73pG2evsAn9G\n" + "X8YKH52fnHsCrhSD\n" + "-----END CERTIFICATE-----"); - assertEquals(new X500Principal("CN=localhost"), certificate.getSubjectX500Principal()); + assertEquals(new X500Principal("CN=localhost"), certificate.getSubjectX500Principal()); FakeSSLSession session = new FakeSSLSession(certificate); - assertTrue(verifier.verify("localhost", session)); - assertTrue(verifier.verify("localhost.localdomain", session)); - assertFalse(verifier.verify("local.host", session)); - assertTrue(verifier.verify("127.0.0.1", session)); - assertFalse(verifier.verify("127.0.0.2", session)); + X509Certificate[] certs = {}; + + assertTrue(verifier.verify(certs, "localhost", session)); + assertTrue(verifier.verify(certs, "localhost.localdomain", session)); + assertFalse(verifier.verify(certs, "local.host", session)); + + assertTrue(verifier.verify(certs, "127.0.0.1", session)); + assertFalse(verifier.verify(certs, "127.0.0.2", session)); } @Test public void wildcardsCannotMatchIpAddresses() throws Exception { @@ -484,7 +497,8 @@ public static Collection data() { + "BQUAA0EAk9i88xdjWoewqvE+iMC9tD2obMchgFDaHH0ogxxiRaIKeEly3g0uGxIt\n" + "fl2WRY8hb4x+zRrwsFaLEpdEvqcjOQ==\n" + "-----END CERTIFICATE-----"); - assertFalse(verifier.verify("127.0.0.1", session)); + X509Certificate[] certs = {}; + assertFalse(verifier.verify(certs, "127.0.0.1", session)); } /** @@ -509,7 +523,8 @@ public static Collection data() { + "-----END CERTIFICATE-----"); // Android-changed: Ignore common name in hostname verification. http://b/70278814 // assertTrue(verifier.verify("google.com", session)); - assertFalse(verifier.verify("google.com", session)); + X509Certificate[] certs = {}; + assertFalse(verifier.verify(certs, "google.com", session)); } @Test public void subjectAltName() throws Exception { @@ -535,11 +550,12 @@ public static Collection data() { + "DQYJKoZIhvcNAQEFBQADQQBXpZZPOY2Dy1lGG81JTr8L4or9jpKacD7n51eS8iqI\n" + "oTznPNuXHU5bFN0AAGX2ij47f/EahqTpo5RdS95P4sVm\n" + "-----END CERTIFICATE-----"); - assertFalse(verifier.verify("foo.com", session)); - assertTrue(verifier.verify("bar.com", session)); - assertTrue(verifier.verify("baz.com", session)); - assertFalse(verifier.verify("a.foo.com", session)); - assertFalse(verifier.verify("quux.com", session)); + X509Certificate[] certs = {}; + assertFalse(verifier.verify(certs, "foo.com", session)); + assertTrue(verifier.verify(certs, "bar.com", session)); + assertTrue(verifier.verify(certs, "baz.com", session)); + assertFalse(verifier.verify(certs, "a.foo.com", session)); + assertFalse(verifier.verify(certs, "quux.com", session)); } @Test public void subjectAltNameWithWildcard() throws Exception { @@ -565,13 +581,14 @@ public static Collection data() { + "bTANBgkqhkiG9w0BAQUFAANBAB8yrSl8zqy07i0SNYx2B/FnvQY734pxioaqFWfO\n" + "Bqo1ZZl/9aPHEWIwBrxYNVB0SGu/kkbt/vxqOjzzrkXukmI=\n" + "-----END CERTIFICATE-----"); - assertFalse(verifier.verify("foo.com", session)); - assertTrue(verifier.verify("bar.com", session)); - assertTrue(verifier.verify("a.baz.com", session)); - assertFalse(verifier.verify("baz.com", session)); - assertFalse(verifier.verify("a.foo.com", session)); - assertFalse(verifier.verify("a.bar.com", session)); - assertFalse(verifier.verify("quux.com", session)); + X509Certificate[] certs = {}; + assertFalse(verifier.verify(certs, "foo.com", session)); + assertTrue(verifier.verify(certs, "bar.com", session)); + assertTrue(verifier.verify(certs, "a.baz.com", session)); + assertFalse(verifier.verify(certs, "baz.com", session)); + assertFalse(verifier.verify(certs, "a.foo.com", session)); + assertFalse(verifier.verify(certs, "a.bar.com", session)); + assertFalse(verifier.verify(certs, "quux.com", session)); } // BEGIN Android-added: Verify behaviour with top level wildcard SAN. http://b/144694112 @@ -595,8 +612,9 @@ public void subjectAltNameWithToplevelWildcard() throws Exception { + "DQEBCwUAA0EAK710g2hQpXSmpbOQH4dHG61fkVDtM/kR/4/R61vDDqVkgOuyHqXl\n" + "GUZFKHMeOZ8peQLT8b+5ik6pIO7Vu2pF6w==\n" + "-----END CERTIFICATE-----\n"); - assertTrue(OkHostnameVerifier.INSTANCE.verify("google.com", session)); - assertFalse(OkHostnameVerifier.strictInstance().verify("google.com", session)); + X509Certificate[] certs = {}; + assertTrue(OkHostnameVerifier.INSTANCE.verify(certs, "google.com", session)); + assertFalse(OkHostnameVerifier.strictInstance().verify(certs, "google.com", session)); } // END Android-added: Verify behaviour with top level wildcard SAN. http://b/144694112 diff --git a/common/src/test/java/org/conscrypt/TrustManagerImplTest.java b/common/src/test/java/org/conscrypt/TrustManagerImplTest.java index 7a7183c69..a20e362e6 100644 --- a/common/src/test/java/org/conscrypt/TrustManagerImplTest.java +++ b/common/src/test/java/org/conscrypt/TrustManagerImplTest.java @@ -243,7 +243,7 @@ public void testHttpsEndpointIdentification() throws Exception { } catch (CertificateException expected) { } certs = tmi.getTrustedChainForServer(chain, "RSA", - new FakeSSLSocket(new FakeSSLSession(goodHostname, chain), params)); + new FakeSSLSocket(new FakeSSLSession(goodHostname, chain), params)); assertEquals(Arrays.asList(chain), certs); } catch (Exception e) { }