From 52764cb4b9219d699b66e96ccf54db1c37c638bb Mon Sep 17 00:00:00 2001 From: Tobias Thierer Date: Fri, 5 Jan 2018 21:06:44 +0000 Subject: [PATCH] OkHostnameVerifier: Don't fall back to CN verification. The use of Common Name was deprecated in RFC 2818 (May 2000), section 3.1: Although the use of the Common Name is existing practice, it is deprecated and Certification Authorities are encouraged to use the dNSName instead. In 2017, Chrome 58, Firefox 48, and Opera 45 web browsers removed this fallback, with Chrome leaving it configurable for enterprise deployments (see https://www.chromestatus.com/feature/4981025180483584). Android is removing it in http://r.android.com/581382 . --- .../okhttp3/internal/tls/ClientAuthTest.java | 40 ++++++++++++++++--- .../internal/tls/HostnameVerifierTest.java | 16 ++++---- .../internal/tls/OkHostnameVerifier.java | 17 +------- 3 files changed, 45 insertions(+), 28 deletions(-) diff --git a/okhttp-tests/src/test/java/okhttp3/internal/tls/ClientAuthTest.java b/okhttp-tests/src/test/java/okhttp3/internal/tls/ClientAuthTest.java index 6642f009b57c..55e82e9cf925 100644 --- a/okhttp-tests/src/test/java/okhttp3/internal/tls/ClientAuthTest.java +++ b/okhttp-tests/src/test/java/okhttp3/internal/tls/ClientAuthTest.java @@ -19,6 +19,7 @@ import java.net.SocketException; import java.security.GeneralSecurityException; import javax.net.ssl.SSLHandshakeException; +import javax.net.ssl.SSLPeerUnverifiedException; import javax.net.ssl.SSLSocket; import javax.net.ssl.SSLSocketFactory; import javax.security.auth.x500.X500Principal; @@ -57,36 +58,42 @@ public void setUp() throws GeneralSecurityException { .serialNumber("1") .ca(3) .commonName("root") + .subjectAlternativeName("root_ca.com") .build(); serverIntermediateCa = new HeldCertificate.Builder() .issuedBy(serverRootCa) .ca(2) .serialNumber("2") .commonName("intermediate_ca") + .subjectAlternativeName("intermediate_ca.com") .build(); serverCert = new HeldCertificate.Builder() .issuedBy(serverIntermediateCa) .serialNumber("3") - .commonName(server.getHostName()) + .commonName("Local Host") + .subjectAlternativeName(server.getHostName()) .build(); clientRootCa = new HeldCertificate.Builder() .serialNumber("1") .ca(13) .commonName("root") + .subjectAlternativeName("root_ca.com") .build(); clientIntermediateCa = new HeldCertificate.Builder() .issuedBy(serverRootCa) .ca(12) .serialNumber("2") .commonName("intermediate_ca") + .subjectAlternativeName("intermediate_ca.com") .build(); clientCert = new HeldCertificate.Builder() .issuedBy(clientIntermediateCa) .serialNumber("4") .commonName("Jethro Willis") + .subjectAlternativeName("jethrowillis.com") .build(); } @@ -100,7 +107,7 @@ public void setUp() throws GeneralSecurityException { Call call = client.newCall(new Request.Builder().url(server.url("/")).build()); Response response = call.execute(); - assertEquals(new X500Principal("CN=" + server.getHostName()), response.handshake().peerPrincipal()); + assertEquals(new X500Principal("CN=Local Host"), response.handshake().peerPrincipal()); assertEquals(new X500Principal("CN=Jethro Willis"), response.handshake().localPrincipal()); assertEquals("abc", response.body().string()); } @@ -115,7 +122,7 @@ public void setUp() throws GeneralSecurityException { Call call = client.newCall(new Request.Builder().url(server.url("/")).build()); Response response = call.execute(); - assertEquals(new X500Principal("CN=" + server.getHostName()), response.handshake().peerPrincipal()); + assertEquals(new X500Principal("CN=Local Host"), response.handshake().peerPrincipal()); assertEquals(new X500Principal("CN=Jethro Willis"), response.handshake().localPrincipal()); assertEquals("abc", response.body().string()); } @@ -130,7 +137,7 @@ public void setUp() throws GeneralSecurityException { Call call = client.newCall(new Request.Builder().url(server.url("/")).build()); Response response = call.execute(); - assertEquals(new X500Principal("CN=" + server.getHostName()), response.handshake().peerPrincipal()); + assertEquals(new X500Principal("CN=Local Host"), response.handshake().peerPrincipal()); assertEquals(null, response.handshake().localPrincipal()); assertEquals("abc", response.body().string()); } @@ -145,7 +152,7 @@ public void setUp() throws GeneralSecurityException { Call call = client.newCall(new Request.Builder().url(server.url("/")).build()); Response response = call.execute(); - assertEquals(new X500Principal("CN=" + server.getHostName()), response.handshake().peerPrincipal()); + assertEquals(new X500Principal("CN=Local Host"), response.handshake().peerPrincipal()); assertEquals(null, response.handshake().localPrincipal()); assertEquals("abc", response.body().string()); } @@ -168,6 +175,29 @@ public void setUp() throws GeneralSecurityException { } } + @Test public void commonNameIsNotTrusted() throws Exception { + serverCert = new HeldCertificate.Builder() + .issuedBy(serverIntermediateCa) + .serialNumber("3") + .commonName(server.getHostName()) + .subjectAlternativeName("different-host.com") + .build(); + + OkHttpClient client = buildClient(clientCert, clientIntermediateCa); + + SSLSocketFactory socketFactory = buildServerSslSocketFactory(ClientAuth.NEEDS); + + server.useHttps(socketFactory, false); + + Call call = client.newCall(new Request.Builder().url(server.url("/")).build()); + + try { + call.execute(); + fail(); + } catch (SSLPeerUnverifiedException expected) { + } + } + @Test public void invalidClientAuthFails() throws Throwable { HeldCertificate clientCert2 = new HeldCertificate.Builder() .serialNumber("4") diff --git a/okhttp-tests/src/test/java/okhttp3/internal/tls/HostnameVerifierTest.java b/okhttp-tests/src/test/java/okhttp3/internal/tls/HostnameVerifierTest.java index f03f197d9460..6dd5bbbd7c2f 100644 --- a/okhttp-tests/src/test/java/okhttp3/internal/tls/HostnameVerifierTest.java +++ b/okhttp-tests/src/test/java/okhttp3/internal/tls/HostnameVerifierTest.java @@ -72,7 +72,7 @@ public final class HostnameVerifierTest { + "HwlNrAu8jlZ2UqSgskSWlhYdMTAP9CPHiUv9N7FcT58Itv/I4fKREINQYjDpvQcx\n" + "SaTYb9dr5sB4WLNglk7zxDtM80H518VvihTcP7FHL+Gn6g4j5fkI98+S\n" + "-----END CERTIFICATE-----\n"); - 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)); } @@ -105,7 +105,7 @@ public final class HostnameVerifierTest { + "9BsO7qe46hidgn39hKh1WjKK2VcL/3YRsC4wUi0PBtFW6ScMCuMhgIRXSPU55Rae\n" + "UIlOdPjjr1SUNWGId1rD7W16Scpwnknn310FNxFMHVI0GTGFkNdkilNCFJcIoRA=\n" + "-----END CERTIFICATE-----\n"); - 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)); } @@ -258,7 +258,7 @@ public final class HostnameVerifierTest { assertFalse(verifier.verify("a.foo.com", session)); assertFalse(verifier.verify("bar.com", session)); assertFalse(verifier.verify("a.bar.com", session)); - 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)); } @@ -291,8 +291,8 @@ public final class HostnameVerifierTest { + "l3Q/RK95bnA6cuRClGusLad0e6bjkBzx/VQ3VarDEpAkTLUGVAa0CLXtnyc=\n" + "-----END CERTIFICATE-----\n"); assertFalse(verifier.verify("foo.com", session)); - assertTrue(verifier.verify("www.foo.com", session)); - assertTrue(verifier.verify("\u82b1\u5b50.foo.com", session)); + assertFalse(verifier.verify("www.foo.com", session)); + assertFalse(verifier.verify("\u82b1\u5b50.foo.com", session)); assertFalse(verifier.verify("a.b.foo.com", session)); } @@ -325,8 +325,8 @@ public final class HostnameVerifierTest { + "UGPLEUDzRHMPHLnSqT1n5UU5UDRytbjJPXzF+l/+WZIsanefWLsxnkgAuZe/oMMF\n" + "EJMryEzOjg4Tfuc5qM0EXoPcQ/JlheaxZ40p2IyHqbsWV4MRYuFH4bkM\n" + "-----END CERTIFICATE-----\n"); - assertTrue(verifier.verify("foo.co.jp", session)); - assertTrue(verifier.verify("\u82b1\u5b50.co.jp", session)); + assertFalse(verifier.verify("foo.co.jp", session)); + assertFalse(verifier.verify("\u82b1\u5b50.co.jp", session)); } /** @@ -451,7 +451,7 @@ public final class HostnameVerifierTest { + "U6LFxmZr31lFyis2/T68PpjAppc0DpNQuA2m/Y7oTHBDi55Fw6HVHCw3lucuWZ5d\n" + "qUYo4ES548JdpQtcLrW2sA==\n" + "-----END CERTIFICATE-----"); - assertTrue(verifier.verify("google.com", session)); + assertFalse(verifier.verify("google.com", session)); } @Test public void subjectAltName() throws Exception { diff --git a/okhttp/src/main/java/okhttp3/internal/tls/OkHostnameVerifier.java b/okhttp/src/main/java/okhttp3/internal/tls/OkHostnameVerifier.java index 7441abadade5..36ac5b3813d3 100644 --- a/okhttp/src/main/java/okhttp3/internal/tls/OkHostnameVerifier.java +++ b/okhttp/src/main/java/okhttp3/internal/tls/OkHostnameVerifier.java @@ -27,7 +27,6 @@ import javax.net.ssl.HostnameVerifier; import javax.net.ssl.SSLException; import javax.net.ssl.SSLSession; -import javax.security.auth.x500.X500Principal; import static okhttp3.internal.Util.verifyAsIpAddress; @@ -73,24 +72,12 @@ private boolean verifyIpAddress(String ipAddress, X509Certificate certificate) { /** Returns true if {@code certificate} matches {@code hostname}. */ private boolean verifyHostname(String hostname, X509Certificate certificate) { hostname = hostname.toLowerCase(Locale.US); - boolean hasDns = false; List altNames = getSubjectAltNames(certificate, ALT_DNS_NAME); - for (int i = 0, size = altNames.size(); i < size; i++) { - hasDns = true; - if (verifyHostname(hostname, altNames.get(i))) { + for (String altName : altNames) { + if (verifyHostname(hostname, altName)) { return true; } } - - if (!hasDns) { - X500Principal principal = certificate.getSubjectX500Principal(); - // RFC 2818 advises using the most specific name for matching. - String cn = new DistinguishedNameParser(principal).findMostSpecific("cn"); - if (cn != null) { - return verifyHostname(hostname, cn); - } - } - return false; }