From 1d880706d1705e78fb9a28afbb48dad0e2561a9e Mon Sep 17 00:00:00 2001 From: Yuri Schimke Date: Sun, 22 Mar 2020 10:48:01 +0000 Subject: [PATCH 1/5] Verify IPv6 hostnames --- .../internal/tls/OkHostnameVerifier.kt | 9 +++++- .../internal/tls/HostnameVerifierTest.java | 28 +++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/okhttp/src/main/java/okhttp3/internal/tls/OkHostnameVerifier.kt b/okhttp/src/main/java/okhttp3/internal/tls/OkHostnameVerifier.kt index 4b0954cca3cb..6b809be4e2dd 100644 --- a/okhttp/src/main/java/okhttp3/internal/tls/OkHostnameVerifier.kt +++ b/okhttp/src/main/java/okhttp3/internal/tls/OkHostnameVerifier.kt @@ -16,6 +16,7 @@ */ package okhttp3.internal.tls +import okhttp3.internal.assertionsEnabled import java.security.cert.CertificateParsingException import java.security.cert.X509Certificate import java.util.Locale @@ -23,6 +24,7 @@ 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]. @@ -51,8 +53,13 @@ object OkHostnameVerifier : HostnameVerifier { /** Returns true if [certificate] matches [ipAddress]. */ private fun verifyIpAddress(ipAddress: String, certificate: X509Certificate): Boolean { + if (assertionsEnabled) { + // HttpUrl has already canonicalized the ipAddress + check(ipAddress == ipAddress.toCanonicalHost()) { "Non canonical host passed in '$ipAddress'" } + } + return getSubjectAltNames(certificate, ALT_IPA_NAME).any { - ipAddress.equals(it, ignoreCase = true) + ipAddress.equals(it.toCanonicalHost(), ignoreCase = true) } } diff --git a/okhttp/src/test/java/okhttp3/internal/tls/HostnameVerifierTest.java b/okhttp/src/test/java/okhttp3/internal/tls/HostnameVerifierTest.java index 75764ee116b6..4b9b91f00c1e 100644 --- a/okhttp/src/test/java/okhttp3/internal/tls/HostnameVerifierTest.java +++ b/okhttp/src/test/java/okhttp3/internal/tls/HostnameVerifierTest.java @@ -516,6 +516,34 @@ public final class HostnameVerifierTest { assertThat(verifier.verify("quux.com", session)).isFalse(); } + @Test public void subjectAltNameWithIPv6Address() 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=IP:0:0:0:0:0:0:0:1 + // + // $ 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" + + "MIIBPTCB6KADAgECAgkAtb7i/qF94lcwDQYJKoZIhvcNAQELBQAwEjEQMA4GA1UE\n" + + "AwwHZm9vLmNvbTAgFw0yMDAzMjIxMDQ1MDFaGA8yMTIwMDIyNzEwNDUwMVowEjEQ\n" + + "MA4GA1UEAwwHZm9vLmNvbTBcMA0GCSqGSIb3DQEBAQUAA0sAMEgCQQDsKO3C4UiW\n" + + "/cihdtKEgYw5wZVHKddWoShi4eMpDvESqTf+ikGC1y/7VihMILlsaomgiUdWQ4zp\n" + + "q40LCcVN4KzZAgMBAAGjHzAdMBsGA1UdEQQUMBKHEAAAAAAAAAAAAAAAAAAAAAEw\n" + + "DQYJKoZIhvcNAQELBQADQQBkjb4M3cNsExUAH+zbWKvYvPcaFtnmnxT26ZKwlLd7\n" + + "YfBU6GYPfDiLGuDCOD9N+tS0tqgbMqNXsMWb05bDNcLB\n" + + "-----END CERTIFICATE-----"); + assertThat(verifier.verify("foo.com", session)).isFalse(); + assertThat(verifier.verify("::1", session)).isTrue(); + assertThat(verifier.verify("::2", session)).isFalse(); + } + @Test public void verifyAsIpAddress() { // IPv4 assertThat(Util.canParseAsIpAddress("127.0.0.1")).isTrue(); From 5bc03fe8547f8dccb4481e69ffcb556f11b06909 Mon Sep 17 00:00:00 2001 From: Yuri Schimke Date: Sun, 22 Mar 2020 10:52:03 +0000 Subject: [PATCH 2/5] Simpler check --- okhttp/src/main/java/okhttp3/internal/tls/OkHostnameVerifier.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/okhttp/src/main/java/okhttp3/internal/tls/OkHostnameVerifier.kt b/okhttp/src/main/java/okhttp3/internal/tls/OkHostnameVerifier.kt index 6b809be4e2dd..8b44df830ba3 100644 --- a/okhttp/src/main/java/okhttp3/internal/tls/OkHostnameVerifier.kt +++ b/okhttp/src/main/java/okhttp3/internal/tls/OkHostnameVerifier.kt @@ -59,7 +59,7 @@ object OkHostnameVerifier : HostnameVerifier { } return getSubjectAltNames(certificate, ALT_IPA_NAME).any { - ipAddress.equals(it.toCanonicalHost(), ignoreCase = true) + ipAddress == it.toCanonicalHost() } } From c216cfaeb01b78e04f40b7fca8eeec97b8fba5a4 Mon Sep 17 00:00:00 2001 From: Yuri Schimke Date: Sun, 22 Mar 2020 11:05:55 +0000 Subject: [PATCH 3/5] Tests --- .../internal/tls/HostnameVerifierTest.java | 32 +++++++++++++------ 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/okhttp/src/test/java/okhttp3/internal/tls/HostnameVerifierTest.java b/okhttp/src/test/java/okhttp3/internal/tls/HostnameVerifierTest.java index 4b9b91f00c1e..a31c48696bf9 100644 --- a/okhttp/src/test/java/okhttp3/internal/tls/HostnameVerifierTest.java +++ b/okhttp/src/test/java/okhttp3/internal/tls/HostnameVerifierTest.java @@ -30,6 +30,7 @@ import static java.nio.charset.StandardCharsets.UTF_8; import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.Assert.fail; /** * Tests for our hostname verifier. Most of these tests are from AOSP, which itself includes tests @@ -516,7 +517,7 @@ public final class HostnameVerifierTest { assertThat(verifier.verify("quux.com", session)).isFalse(); } - @Test public void subjectAltNameWithIPv6Address() throws Exception { + @Test public void subjectAltNameWithIPAddresses() throws Exception { // $ cat ./cert.cnf // [req] // distinguished_name=distinguished_name @@ -525,23 +526,36 @@ public final class HostnameVerifierTest { // [distinguished_name] // [req_extensions] // [x509_extensions] - // subjectAltName=IP:0:0:0:0:0:0:0:1 + // subjectAltName=IP:0:0:0:0:0:0:0:1,IP:2a03:2880:f003:c07:face:b00c::2,IP:0::5,IP:192.168.1.1 // // $ 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" - + "MIIBPTCB6KADAgECAgkAtb7i/qF94lcwDQYJKoZIhvcNAQELBQAwEjEQMA4GA1UE\n" - + "AwwHZm9vLmNvbTAgFw0yMDAzMjIxMDQ1MDFaGA8yMTIwMDIyNzEwNDUwMVowEjEQ\n" - + "MA4GA1UEAwwHZm9vLmNvbTBcMA0GCSqGSIb3DQEBAQUAA0sAMEgCQQDsKO3C4UiW\n" - + "/cihdtKEgYw5wZVHKddWoShi4eMpDvESqTf+ikGC1y/7VihMILlsaomgiUdWQ4zp\n" - + "q40LCcVN4KzZAgMBAAGjHzAdMBsGA1UdEQQUMBKHEAAAAAAAAAAAAAAAAAAAAAEw\n" - + "DQYJKoZIhvcNAQELBQADQQBkjb4M3cNsExUAH+zbWKvYvPcaFtnmnxT26ZKwlLd7\n" - + "YfBU6GYPfDiLGuDCOD9N+tS0tqgbMqNXsMWb05bDNcLB\n" + + "MIIBaDCCARKgAwIBAgIJALxN+AOBVGwQMA0GCSqGSIb3DQEBCwUAMBIxEDAOBgNV\n" + + "BAMMB2Zvby5jb20wIBcNMjAwMzIyMTEwNDI4WhgPMjEyMDAyMjcxMTA0MjhaMBIx\n" + + "EDAOBgNVBAMMB2Zvby5jb20wXDANBgkqhkiG9w0BAQEFAANLADBIAkEAlnVbVfQ9\n" + + "4aYjrPCcFuxOpjXuvyOc9Hcha4K7TfXyfsrjhAvCjCBIT/TiLOUVF3sx4yoCAtX8\n" + + "wmt404tTbKD6UwIDAQABo0kwRzBFBgNVHREEPjA8hxAAAAAAAAAAAAAAAAAAAAAB\n" + + "hxAqAyiA8AMMB/rOsAwAAAAChxAAAAAAAAAAAAAAAAAAAAAFhwTAqAEBMA0GCSqG\n" + + "SIb3DQEBCwUAA0EAPSOYHJh7hB4ElBqTCAFW+T5Y7mXsv9nQjBJ7w0YIw83V2PEI\n" + + "3KbBIyGTrqHD6lG8QGZy+yNkIcRlodG8OfQRUg==\n" + "-----END CERTIFICATE-----"); assertThat(verifier.verify("foo.com", session)).isFalse(); assertThat(verifier.verify("::1", session)).isTrue(); assertThat(verifier.verify("::2", session)).isFalse(); + assertThat(verifier.verify("::5", session)).isTrue(); + assertThat(verifier.verify("2a03:2880:f003:c07:face:b00c:0:2", session)).isTrue(); + assertThat(verifier.verify("2a03:2880:f003:c07:face:b00c:0:3", session)).isFalse(); + assertThat(verifier.verify("127.0.0.1", session)).isFalse(); + assertThat(verifier.verify("192.168.1.1", session)).isTrue(); + + try { + verifier.verify("2a03:2880:f003:c07:face:b00c::2", session); + fail(); + } catch (IllegalStateException ise) { + // expected + } } @Test public void verifyAsIpAddress() { From e83ce01f3c17fcfc7174f63e9ba357b05e0bd51d Mon Sep 17 00:00:00 2001 From: Yuri Schimke Date: Sun, 22 Mar 2020 11:11:11 +0000 Subject: [PATCH 4/5] Avoid exceptions --- .../main/java/okhttp3/internal/tls/OkHostnameVerifier.kt | 7 ++----- .../java/okhttp3/internal/tls/HostnameVerifierTest.java | 9 ++------- 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/okhttp/src/main/java/okhttp3/internal/tls/OkHostnameVerifier.kt b/okhttp/src/main/java/okhttp3/internal/tls/OkHostnameVerifier.kt index 8b44df830ba3..eb82654e3c7d 100644 --- a/okhttp/src/main/java/okhttp3/internal/tls/OkHostnameVerifier.kt +++ b/okhttp/src/main/java/okhttp3/internal/tls/OkHostnameVerifier.kt @@ -53,13 +53,10 @@ object OkHostnameVerifier : HostnameVerifier { /** Returns true if [certificate] matches [ipAddress]. */ private fun verifyIpAddress(ipAddress: String, certificate: X509Certificate): Boolean { - if (assertionsEnabled) { - // HttpUrl has already canonicalized the ipAddress - check(ipAddress == ipAddress.toCanonicalHost()) { "Non canonical host passed in '$ipAddress'" } - } + val canonicalIpAddress = ipAddress.toCanonicalHost() return getSubjectAltNames(certificate, ALT_IPA_NAME).any { - ipAddress == it.toCanonicalHost() + canonicalIpAddress == it.toCanonicalHost() } } diff --git a/okhttp/src/test/java/okhttp3/internal/tls/HostnameVerifierTest.java b/okhttp/src/test/java/okhttp3/internal/tls/HostnameVerifierTest.java index a31c48696bf9..03690f0f450a 100644 --- a/okhttp/src/test/java/okhttp3/internal/tls/HostnameVerifierTest.java +++ b/okhttp/src/test/java/okhttp3/internal/tls/HostnameVerifierTest.java @@ -545,17 +545,12 @@ public final class HostnameVerifierTest { assertThat(verifier.verify("::1", session)).isTrue(); assertThat(verifier.verify("::2", session)).isFalse(); assertThat(verifier.verify("::5", session)).isTrue(); + assertThat(verifier.verify("2a03:2880:f003:c07:face:b00c::2", session)).isTrue(); assertThat(verifier.verify("2a03:2880:f003:c07:face:b00c:0:2", session)).isTrue(); + assertThat(verifier.verify("2a03:2880:f003:c07:FACE:B00C:0:2", session)).isTrue(); assertThat(verifier.verify("2a03:2880:f003:c07:face:b00c:0:3", session)).isFalse(); assertThat(verifier.verify("127.0.0.1", session)).isFalse(); assertThat(verifier.verify("192.168.1.1", session)).isTrue(); - - try { - verifier.verify("2a03:2880:f003:c07:face:b00c::2", session); - fail(); - } catch (IllegalStateException ise) { - // expected - } } @Test public void verifyAsIpAddress() { From bca5b1fb202b3689b6beba2b2b36928c09a43a40 Mon Sep 17 00:00:00 2001 From: Yuri Schimke Date: Sun, 22 Mar 2020 11:15:14 +0000 Subject: [PATCH 5/5] Fixes --- okhttp/src/main/java/okhttp3/internal/tls/OkHostnameVerifier.kt | 1 - .../src/test/java/okhttp3/internal/tls/HostnameVerifierTest.java | 1 - 2 files changed, 2 deletions(-) diff --git a/okhttp/src/main/java/okhttp3/internal/tls/OkHostnameVerifier.kt b/okhttp/src/main/java/okhttp3/internal/tls/OkHostnameVerifier.kt index eb82654e3c7d..79a8359163a5 100644 --- a/okhttp/src/main/java/okhttp3/internal/tls/OkHostnameVerifier.kt +++ b/okhttp/src/main/java/okhttp3/internal/tls/OkHostnameVerifier.kt @@ -16,7 +16,6 @@ */ package okhttp3.internal.tls -import okhttp3.internal.assertionsEnabled import java.security.cert.CertificateParsingException import java.security.cert.X509Certificate import java.util.Locale diff --git a/okhttp/src/test/java/okhttp3/internal/tls/HostnameVerifierTest.java b/okhttp/src/test/java/okhttp3/internal/tls/HostnameVerifierTest.java index 03690f0f450a..c78239fa5bcc 100644 --- a/okhttp/src/test/java/okhttp3/internal/tls/HostnameVerifierTest.java +++ b/okhttp/src/test/java/okhttp3/internal/tls/HostnameVerifierTest.java @@ -30,7 +30,6 @@ import static java.nio.charset.StandardCharsets.UTF_8; import static org.assertj.core.api.Assertions.assertThat; -import static org.junit.Assert.fail; /** * Tests for our hostname verifier. Most of these tests are from AOSP, which itself includes tests