Skip to content

Commit

Permalink
OkHostnameVerifier: Don't fall back to CN verification. (#3764)
Browse files Browse the repository at this point in the history 
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 .
  • Loading branch information
@15characterlimi @swankjesse
15characterlimi authored and swankjesse committed Jan 5, 2018
1 parent 901a514 commit cd84d79
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 28 deletions.
40 changes: 35 additions & 5 deletions okhttp-tests/src/test/java/okhttp3/internal/tls/ClientAuthTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
}

Expand All @@ -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());
}
Expand All @@ -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());
}
Expand All @@ -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());
}
Expand All @@ -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());
}
Expand All @@ -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")
Expand Down
16 changes: 8 additions & 8 deletions okhttp-tests/src/test/java/okhttp3/internal/tls/HostnameVerifierTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand Down Expand Up @@ -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));
}

Expand Down Expand Up @@ -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));
}

Expand Down Expand Up @@ -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));
}

Expand Down Expand Up @@ -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));
}

/**
Expand Down Expand Up @@ -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 {
Expand Down
17 changes: 2 additions & 15 deletions okhttp/src/main/java/okhttp3/internal/tls/OkHostnameVerifier.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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<String> 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;
}

Expand Down

0 comments on commit cd84d79

Please sign in to comment.