From 92c4f16d5e4d949e2551d5a95558072d09f4e7a0 Mon Sep 17 00:00:00 2001 From: Yuri Schimke Date: Fri, 30 Oct 2020 08:15:55 +0000 Subject: [PATCH 1/2] Use generated certificates in unit tests (#6346) * Use generated certificates in unit tests * Strict to ascii lowercase implementation Co-authored-by: Jesse Wilson --- .../internal/tls/OkHostnameVerifier.kt | 21 +++- okhttp/src/test/java/okhttp3/HttpUrlTest.java | 11 ++ .../internal/tls/HostnameVerifierTest.java | 103 ++++++++++++++++++ 3 files changed, 131 insertions(+), 4 deletions(-) diff --git a/okhttp/src/main/kotlin/okhttp3/internal/tls/OkHostnameVerifier.kt b/okhttp/src/main/kotlin/okhttp3/internal/tls/OkHostnameVerifier.kt index 79a8359163a5..389aac118819 100644 --- a/okhttp/src/main/kotlin/okhttp3/internal/tls/OkHostnameVerifier.kt +++ b/okhttp/src/main/kotlin/okhttp3/internal/tls/OkHostnameVerifier.kt @@ -16,14 +16,15 @@ */ package okhttp3.internal.tls +import okhttp3.internal.canParseAsIpAddress +import okhttp3.internal.toCanonicalHost +import okio.utf8Size import java.security.cert.CertificateParsingException import java.security.cert.X509Certificate import java.util.Locale import javax.net.ssl.HostnameVerifier import javax.net.ssl.SSLException import javax.net.ssl.SSLSession -import okhttp3.internal.canParseAsIpAddress -import okhttp3.internal.toCanonicalHost /** * A HostnameVerifier consistent with [RFC 2818][rfc_2818]. @@ -61,12 +62,24 @@ object OkHostnameVerifier : HostnameVerifier { /** Returns true if [certificate] matches [hostname]. */ private fun verifyHostname(hostname: String, certificate: X509Certificate): Boolean { - val hostname = hostname.toLowerCase(Locale.US) + val hostname = hostname.asciiToLowercase() return getSubjectAltNames(certificate, ALT_DNS_NAME).any { verifyHostname(hostname, it) } } + /** + * This is like [toLowerCase] except that it does nothing if this contains any non-ASCII + * characters. We want to avoid lower casing special chars like U+212A (Kelvin symbol) because + * they can return ASCII characters that match real hostnames. + */ + private fun String.asciiToLowercase(): String { + return when { + length == utf8Size().toInt() -> toLowerCase(Locale.US) // This is an ASCII string. + else -> this + } + } + /** * Returns true if [hostname] matches the domain name [pattern]. * @@ -108,7 +121,7 @@ object OkHostnameVerifier : HostnameVerifier { } // Hostname and pattern are now absolute domain names. - pattern = pattern.toLowerCase(Locale.US) + pattern = pattern.asciiToLowercase() // Hostname and pattern are now in lower case -- domain names are case-insensitive. if ("*" !in pattern) { diff --git a/okhttp/src/test/java/okhttp3/HttpUrlTest.java b/okhttp/src/test/java/okhttp3/HttpUrlTest.java index ff10f31d975a..14984a973934 100644 --- a/okhttp/src/test/java/okhttp3/HttpUrlTest.java +++ b/okhttp/src/test/java/okhttp3/HttpUrlTest.java @@ -1773,6 +1773,17 @@ public void unparseableTopPrivateDomain() { assertInvalid("http://../", "Invalid URL host: \"..\""); } + @Test + public void hostnameTelephone() throws Exception { + // https://www.gosecure.net/blog/2020/10/27/weakness-in-java-tls-host-verification/ + + // Map the single character telephone symbol (℡) to the string "tel". + assertThat(parse("http://\u2121").host()).isEqualTo("tel"); + + // Map the Kelvin symbol (K) to the string "k". + assertThat(parse("http://\u212A").host()).isEqualTo("k"); + } + private void assertInvalid(String string, String exceptionMessage) { if (useGet) { try { diff --git a/okhttp/src/test/java/okhttp3/internal/tls/HostnameVerifierTest.java b/okhttp/src/test/java/okhttp3/internal/tls/HostnameVerifierTest.java index ff3ed62b291e..611b58318ba2 100644 --- a/okhttp/src/test/java/okhttp3/internal/tls/HostnameVerifierTest.java +++ b/okhttp/src/test/java/okhttp3/internal/tls/HostnameVerifierTest.java @@ -24,7 +24,10 @@ import javax.net.ssl.SSLSession; import javax.security.auth.x500.X500Principal; import okhttp3.FakeSSLSession; +import okhttp3.OkHttpClient; import okhttp3.internal.Util; +import okhttp3.tls.HeldCertificate; +import okhttp3.tls.internal.TlsUtil; import org.junit.Ignore; import org.junit.Test; @@ -554,6 +557,106 @@ public final class HostnameVerifierTest { assertThat(verifier.verify("0:0:0:0:0:FFFF:C0A8:0101", session)).isTrue(); } + @Test public void generatedCertificate() throws Exception { + HeldCertificate heldCertificate = new HeldCertificate.Builder() + .commonName("Foo Corp") + .addSubjectAlternativeName("foo.com") + .build(); + + SSLSession session = session(heldCertificate.certificatePem()); + assertThat(verifier.verify("foo.com", session)).isTrue(); + assertThat(verifier.verify("bar.com", session)).isFalse(); + } + + @Test public void specialKInHostname() throws Exception { + // https://github.com/apache/httpcomponents-client/commit/303e435d7949652ea77a6c50df1c548682476b6e + // https://www.gosecure.net/blog/2020/10/27/weakness-in-java-tls-host-verification/ + + HeldCertificate heldCertificate = new HeldCertificate.Builder() + .commonName("Foo Corp") + .addSubjectAlternativeName("k.com") + .addSubjectAlternativeName("tel.com") + .build(); + + SSLSession session = session(heldCertificate.certificatePem()); + assertThat(verifier.verify("foo.com", session)).isFalse(); + assertThat(verifier.verify("bar.com", session)).isFalse(); + assertThat(verifier.verify("k.com", session)).isTrue(); + assertThat(verifier.verify("K.com", session)).isTrue(); + + assertThat(verifier.verify("\u2121.com", session)).isFalse(); + assertThat(verifier.verify("℡.com", session)).isFalse(); + + // These should ideally be false, but we know that hostname is usually already checked by us + assertThat(verifier.verify("\u212A.com", session)).isFalse(); + // Kelvin character below + assertThat(verifier.verify("K.com", session)).isFalse(); + } + + @Test public void specialKInCert() throws Exception { + // https://github.com/apache/httpcomponents-client/commit/303e435d7949652ea77a6c50df1c548682476b6e + // https://www.gosecure.net/blog/2020/10/27/weakness-in-java-tls-host-verification/ + + HeldCertificate heldCertificate = new HeldCertificate.Builder() + .commonName("Foo Corp") + .addSubjectAlternativeName("\u2121.com") + .addSubjectAlternativeName("\u212A.com") + .build(); + + SSLSession session = session(heldCertificate.certificatePem()); + assertThat(verifier.verify("foo.com", session)).isFalse(); + assertThat(verifier.verify("bar.com", session)).isFalse(); + assertThat(verifier.verify("k.com", session)).isFalse(); + assertThat(verifier.verify("K.com", session)).isFalse(); + + assertThat(verifier.verify("tel.com", session)).isFalse(); + assertThat(verifier.verify("k.com", session)).isFalse(); + } + + @Test public void specialKInExternalCert() throws Exception { + // $ cat ./cert.cnf + // [req] + // distinguished_name=distinguished_name + // req_extensions=req_extensions + // x509_extensions=x509_extensions + // [distinguished_name] + // [req_extensions] + // [x509_extensions] + // subjectAltName=DNS:℡.com,DNS:K.com + // + // $ openssl req -x509 -nodes -days 36500 -subj '/CN=foo.com' -config ./cert.cnf \ + // -newkey rsa:512 -out cert.pem + SSLSession session = session("" + + "-----BEGIN CERTIFICATE-----\n" + + "MIIBSDCB86ADAgECAhRLR4TGgXBegg0np90FZ1KPeWpDtjANBgkqhkiG9w0BAQsF\n" + + "ADASMRAwDgYDVQQDDAdmb28uY29tMCAXDTIwMTAyOTA2NTkwNVoYDzIxMjAxMDA1\n" + + "MDY1OTA1WjASMRAwDgYDVQQDDAdmb28uY29tMFwwDQYJKoZIhvcNAQEBBQADSwAw\n" + + "SAJBALQcTVW9aW++ClIV9/9iSzijsPvQGEu/FQOjIycSrSIheZyZmR8bluSNBq0C\n" + + "9fpalRKZb0S2tlCTi5WoX8d3K30CAwEAAaMfMB0wGwYDVR0RBBQwEoIH4oShLmNv\n" + + "bYIH4oSqLmNvbTANBgkqhkiG9w0BAQsFAANBAA1+/eDvSUGv78iEjNW+1w3OPAwt\n" + + "Ij1qLQ/YI8OogZPMk7YY46/ydWWp7UpD47zy/vKmm4pOc8Glc8MoDD6UADs=\n" + + "-----END CERTIFICATE-----\n"); + + assertThat(verifier.verify("tel.com", session)).isFalse(); + assertThat(verifier.verify("k.com", session)).isFalse(); + + assertThat(verifier.verify("foo.com", session)).isFalse(); + assertThat(verifier.verify("bar.com", session)).isFalse(); + assertThat(verifier.verify("k.com", session)).isFalse(); + assertThat(verifier.verify("K.com", session)).isFalse(); + } + + @Test + public void thatCatchesErrorsWithBadSession() { + HostnameVerifier localVerifier = new OkHttpClient().hostnameVerifier(); + + // Since this is public API, okhttp3.internal.tls.OkHostnameVerifier.verify is also + assertThat(verifier).isInstanceOf(OkHostnameVerifier.class); + + SSLSession session = TlsUtil.localhost().sslContext().createSSLEngine().getSession(); + assertThat(localVerifier.verify("\uD83D\uDCA9.com", session)).isFalse(); + } + @Test public void verifyAsIpAddress() { // IPv4 assertThat(Util.canParseAsIpAddress("127.0.0.1")).isTrue(); From 20f690380de54da38c4d8bc4e2b7ab2e108b33ea Mon Sep 17 00:00:00 2001 From: Yuri Schimke Date: Sat, 31 Oct 2020 09:28:44 +0000 Subject: [PATCH 2/2] More restrictive behaviour of OkHostnameVerifier (#6353) * Test quirks of HostnameVerifier. * Restrict successful results to ascii input. --- .../internal/tls/OkHostnameVerifier.kt | 16 +++- .../internal/tls/HostnameVerifierTest.java | 84 ++++++++++++++----- 2 files changed, 77 insertions(+), 23 deletions(-) diff --git a/okhttp/src/main/kotlin/okhttp3/internal/tls/OkHostnameVerifier.kt b/okhttp/src/main/kotlin/okhttp3/internal/tls/OkHostnameVerifier.kt index 389aac118819..5b07112bda07 100644 --- a/okhttp/src/main/kotlin/okhttp3/internal/tls/OkHostnameVerifier.kt +++ b/okhttp/src/main/kotlin/okhttp3/internal/tls/OkHostnameVerifier.kt @@ -37,11 +37,16 @@ object OkHostnameVerifier : HostnameVerifier { private const val ALT_IPA_NAME = 7 override fun verify(host: String, session: SSLSession): Boolean { - return try { - verify(host, session.peerCertificates[0] as X509Certificate) - } catch (_: SSLException) { + return if (!host.isAscii()) { false + } else { + try { + verify(host, session.peerCertificates[0] as X509Certificate) + } catch (_: SSLException) { + false + } } + } fun verify(host: String, certificate: X509Certificate): Boolean { @@ -75,11 +80,14 @@ object OkHostnameVerifier : HostnameVerifier { */ private fun String.asciiToLowercase(): String { return when { - length == utf8Size().toInt() -> toLowerCase(Locale.US) // This is an ASCII string. + isAscii() -> toLowerCase(Locale.US) // This is an ASCII string. else -> this } } + /** Returns true if the [String] is ASCII encoded (0-127). */ + private fun String.isAscii() = length == utf8Size().toInt() + /** * Returns true if [hostname] matches the domain name [pattern]. * diff --git a/okhttp/src/test/java/okhttp3/internal/tls/HostnameVerifierTest.java b/okhttp/src/test/java/okhttp3/internal/tls/HostnameVerifierTest.java index 611b58318ba2..91b8e5cc585d 100644 --- a/okhttp/src/test/java/okhttp3/internal/tls/HostnameVerifierTest.java +++ b/okhttp/src/test/java/okhttp3/internal/tls/HostnameVerifierTest.java @@ -19,7 +19,9 @@ import java.io.ByteArrayInputStream; import java.security.cert.CertificateFactory; +import java.security.cert.CertificateParsingException; import java.security.cert.X509Certificate; +import java.util.stream.Stream; import javax.net.ssl.HostnameVerifier; import javax.net.ssl.SSLSession; import javax.security.auth.x500.X500Principal; @@ -28,7 +30,6 @@ import okhttp3.internal.Util; import okhttp3.tls.HeldCertificate; import okhttp3.tls.internal.TlsUtil; -import org.junit.Ignore; import org.junit.Test; import static java.nio.charset.StandardCharsets.UTF_8; @@ -39,9 +40,9 @@ * from the Apache HTTP Client test suite. */ public final class HostnameVerifierTest { - private HostnameVerifier verifier = OkHostnameVerifier.INSTANCE; + private OkHostnameVerifier verifier = OkHostnameVerifier.INSTANCE; - @Test public void verify() throws Exception { + @Test public void verify() { FakeSSLSession session = new FakeSSLSession(); assertThat(verifier.verify("localhost", session)).isFalse(); } @@ -151,7 +152,7 @@ public final class HostnameVerifierTest { * are parsed. Android fails to parse these, which means we fall back to the CN. The RI does parse * them, so the CN is unused. */ - @Test @Ignore public void verifyNonAsciiSubjectAlt() throws Exception { + @Test public void verifyNonAsciiSubjectAlt() throws Exception { // CN=foo.com, subjectAlt=bar.com, subjectAlt=花子.co.jp // (hanako.co.jp in kanji) SSLSession session = session("" @@ -181,16 +182,20 @@ public final class HostnameVerifierTest { + "sWIKHYrmhCIRshUNohGXv50m2o+1w9oWmQ6Dkq7lCjfXfUB4wIbggJjpyEtbNqBt\n" + "j4MC2x5rfsLKKqToKmNE7pFEgqwe8//Aar1b+Qj+\n" + "-----END CERTIFICATE-----\n"); - assertThat(verifier.verify("foo.com", session)).isTrue(); + + X509Certificate peerCertificate = ((X509Certificate) session.getPeerCertificates()[0]); + assertThat(certificateSANs(peerCertificate)).containsExactly("bar.com", "������.co.jp"); + + assertThat(verifier.verify("foo.com", session)).isFalse(); assertThat(verifier.verify("a.foo.com", session)).isFalse(); // 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 // harmony follows the definition from rfc 1034 page 10 for alternative // subject names. This causes the code to drop all alternative subjects. - // assertTrue(verifier.verify("bar.com", session)); - // assertFalse(verifier.verify("a.bar.com", session)); - // assertFalse(verifier.verify("a.\u82b1\u5b50.co.jp", session)); + assertThat(verifier.verify("bar.com", session)).isTrue(); + assertThat(verifier.verify("a.bar.com", session)).isFalse(); + assertThat(verifier.verify("a.\u82b1\u5b50.co.jp", session)).isFalse(); } @Test public void verifySubjectAltOnly() throws Exception { @@ -332,11 +337,11 @@ public final class HostnameVerifierTest { } /** - * Ignored due to incompatibilities between Android and Java on how non-ASCII subject alt names - * are parsed. Android fails to parse these, which means we fall back to the CN. The RI does parse - * them, so the CN is unused. + * Previously ignored due to incompatibilities between Android and Java on how non-ASCII subject + * alt names are parsed. Android fails to parse these, which means we fall back to the CN. + * The RI does parse them, so the CN is unused. */ - @Test @Ignore public void testWilcardNonAsciiSubjectAlt() throws Exception { + @Test public void testWilcardNonAsciiSubjectAlt() throws Exception { // CN=*.foo.com, subjectAlt=*.bar.com, subjectAlt=*.花子.co.jp // (*.hanako.co.jp in kanji) SSLSession session = session("" @@ -366,20 +371,24 @@ public final class HostnameVerifierTest { + "qFr0AIZKBlg6NZZFf/0dP9zcKhzSriW27bY0XfzA6GSiRDXrDjgXq6baRT6YwgIg\n" + "pgJsDbJtZfHnV1nd3M6zOtQPm1TIQpNmMMMd/DPrGcUQerD3\n" + "-----END CERTIFICATE-----\n"); + + X509Certificate peerCertificate = ((X509Certificate) session.getPeerCertificates()[0]); + assertThat(certificateSANs(peerCertificate)).containsExactly("*.bar.com", "*.������.co.jp"); + // try the foo.com variations - assertThat(verifier.verify("foo.com", session)).isTrue(); - assertThat(verifier.verify("www.foo.com", session)).isTrue(); - assertThat(verifier.verify("\u82b1\u5b50.foo.com", session)).isTrue(); + assertThat(verifier.verify("foo.com", session)).isFalse(); + assertThat(verifier.verify("www.foo.com", session)).isFalse(); + assertThat(verifier.verify("\u82b1\u5b50.foo.com", session)).isFalse(); assertThat(verifier.verify("a.b.foo.com", session)).isFalse(); // 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 // harmony follows the definition from rfc 1034 page 10 for alternative // subject names. This causes the code to drop all alternative subjects. - // assertFalse(verifier.verify("bar.com", session)); - // assertTrue(verifier.verify("www.bar.com", session)); - // assertTrue(verifier.verify("\u82b1\u5b50.bar.com", session)); - // assertTrue(verifier.verify("a.b.bar.com", session)); + assertThat(verifier.verify("bar.com", session)).isFalse(); + assertThat(verifier.verify("www.bar.com", session)).isTrue(); + assertThat(verifier.verify("\u82b1\u5b50.bar.com", session)).isFalse(); + assertThat(verifier.verify("a.b.bar.com", session)).isFalse(); } @Test public void subjectAltUsesLocalDomainAndIp() throws Exception { @@ -637,6 +646,9 @@ public final class HostnameVerifierTest { + "Ij1qLQ/YI8OogZPMk7YY46/ydWWp7UpD47zy/vKmm4pOc8Glc8MoDD6UADs=\n" + "-----END CERTIFICATE-----\n"); + X509Certificate peerCertificate = ((X509Certificate) session.getPeerCertificates()[0]); + assertThat(certificateSANs(peerCertificate)).containsExactly("���.com", "���.com"); + assertThat(verifier.verify("tel.com", session)).isFalse(); assertThat(verifier.verify("k.com", session)).isFalse(); @@ -646,6 +658,40 @@ public final class HostnameVerifierTest { assertThat(verifier.verify("K.com", session)).isFalse(); } + private Stream certificateSANs(X509Certificate peerCertificate) + throws CertificateParsingException { + return peerCertificate.getSubjectAlternativeNames().stream().map(c -> (String) c.get(1)); + } + + @Test public void replacementCharacter() throws Exception { + // $ cat ./cert.cnf + // [req] + // distinguished_name=distinguished_name + // req_extensions=req_extensions + // x509_extensions=x509_extensions + // [distinguished_name] + // [req_extensions] + // [x509_extensions] + // subjectAltName=DNS:℡.com,DNS:K.com + // + // $ openssl req -x509 -nodes -days 36500 -subj '/CN=foo.com' -config ./cert.cnf \ + // -newkey rsa:512 -out cert.pem + SSLSession session = session("" + + "-----BEGIN CERTIFICATE-----\n" + + "MIIBSDCB86ADAgECAhRLR4TGgXBegg0np90FZ1KPeWpDtjANBgkqhkiG9w0BAQsF\n" + + "ADASMRAwDgYDVQQDDAdmb28uY29tMCAXDTIwMTAyOTA2NTkwNVoYDzIxMjAxMDA1\n" + + "MDY1OTA1WjASMRAwDgYDVQQDDAdmb28uY29tMFwwDQYJKoZIhvcNAQEBBQADSwAw\n" + + "SAJBALQcTVW9aW++ClIV9/9iSzijsPvQGEu/FQOjIycSrSIheZyZmR8bluSNBq0C\n" + + "9fpalRKZb0S2tlCTi5WoX8d3K30CAwEAAaMfMB0wGwYDVR0RBBQwEoIH4oShLmNv\n" + + "bYIH4oSqLmNvbTANBgkqhkiG9w0BAQsFAANBAA1+/eDvSUGv78iEjNW+1w3OPAwt\n" + + "Ij1qLQ/YI8OogZPMk7YY46/ydWWp7UpD47zy/vKmm4pOc8Glc8MoDD6UADs=\n" + + "-----END CERTIFICATE-----\n"); + + // Replacement characters are deliberate, from certificate loading. + assertThat(verifier.verify("���.com", session)).isFalse(); + assertThat(verifier.verify("℡.com", session)).isFalse(); + } + @Test public void thatCatchesErrorsWithBadSession() { HostnameVerifier localVerifier = new OkHttpClient().hostnameVerifier();