Backport OkHttp security fix from d948330

grpc · Jan 22, 2016 · 829e7e8 · 829e7e8
1 parent 3e0f44f
commit 829e7e8
Show file tree

Hide file tree

Showing 3 changed files with 46 additions and 7 deletions.
diff --git a/interop-testing/src/test/java/io/grpc/testing/integration/Http2OkHttpTest.java b/interop-testing/src/test/java/io/grpc/testing/integration/Http2OkHttpTest.java
@@ -31,6 +31,12 @@
 
 package io.grpc.testing.integration;
 
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+import com.google.common.base.Throwables;
+import com.google.protobuf.EmptyProtos.Empty;
+
 import com.squareup.okhttp.ConnectionSpec;
 import com.squareup.okhttp.TlsVersion;
 
@@ -52,6 +58,8 @@
 
 import java.io.IOException;
 
+import javax.net.ssl.SSLPeerUnverifiedException;
+
 /**
  * Integration tests for GRPC over Http2 using the OkHttp framework.
  */
@@ -118,4 +126,28 @@ public void receivedDataForFinishedStream() throws Exception {
     recorder.awaitCompletion();
     emptyUnary();
   }
+
+  @Test(timeout = 10000)
+  public void wrongHostNameFailHostnameVerification() throws Exception {
+    OkHttpChannelBuilder builder = OkHttpChannelBuilder.forAddress("127.0.0.1", serverPort)
+        .connectionSpec(new ConnectionSpec.Builder(OkHttpChannelBuilder.DEFAULT_CONNECTION_SPEC)
+            .cipherSuites(TestUtils.preferredTestCiphers().toArray(new String[0]))
+            .tlsVersions(ConnectionSpec.MODERN_TLS.tlsVersions().toArray(new TlsVersion[0]))
+            .build())
+        .overrideAuthority("I.am.a.bad.hostname:" + serverPort);
+    ManagedChannel channel = builder.sslSocketFactory(
+        TestUtils.newSslSocketFactoryForCa(TestUtils.loadCert("ca.pem"))).build();
+    TestServiceGrpc.TestServiceBlockingStub blockingStub =
+        TestServiceGrpc.newBlockingStub(channel);
+
+    try {
+      blockingStub.emptyCall(Empty.getDefaultInstance());
+      fail("The rpc should have been failed due to hostname verification");
+    } catch (Throwable t) {
+      Throwable cause = Throwables.getRootCause(t);
+      assertTrue("Failed by unexpected exception: " + cause,
+          cause instanceof SSLPeerUnverifiedException);
+    }
+    channel.shutdown();
+  }
 }
diff --git a/okhttp/src/main/java/com/squareup/okhttp/OkHttpTlsUpgrader.java b/okhttp/src/main/java/com/squareup/okhttp/OkHttpTlsUpgrader.java
@@ -34,13 +34,15 @@
 import com.google.common.base.Preconditions;
 
 import com.squareup.okhttp.internal.OkHttpProtocolNegotiator;
+import com.squareup.okhttp.internal.tls.OkHostnameVerifier;
 
 import java.io.IOException;
 import java.net.Socket;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 
+import javax.net.ssl.SSLPeerUnverifiedException;
 import javax.net.ssl.SSLSocket;
 import javax.net.ssl.SSLSocketFactory;
 
@@ -68,11 +70,13 @@ public static SSLSocket upgrade(SSLSocketFactory sslSocketFactory,
     SSLSocket sslSocket = (SSLSocket) sslSocketFactory.createSocket(
         socket, host, port, true /* auto close */);
     spec.apply(sslSocket, false);
-    if (spec.supportsTlsExtensions()) {
-      String negotiatedProtocol =
-          OkHttpProtocolNegotiator.get().negotiate(sslSocket, host, TLS_PROTOCOLS);
-      Preconditions.checkState(HTTP2_PROTOCOL_NAME.equals(negotiatedProtocol),
-          "Only \"h2\" is supported, but negotiated protocol is %s", negotiatedProtocol);
+    String negotiatedProtocol = OkHttpProtocolNegotiator.get().negotiate(
+        sslSocket, host, spec.supportsTlsExtensions() ? TLS_PROTOCOLS : null);
+    Preconditions.checkState(HTTP2_PROTOCOL_NAME.equals(negotiatedProtocol),
+        "Only \"h2\" is supported, but negotiated protocol is %s", negotiatedProtocol);
+
+    if (!OkHostnameVerifier.INSTANCE.verify(host, sslSocket.getSession())) {
+      throw new SSLPeerUnverifiedException("Cannot verify hostname: " + host);
     }
     return sslSocket;
   }

diff --git a/okhttp/src/main/java/com/squareup/okhttp/internal/OkHttpProtocolNegotiator.java b/okhttp/src/main/java/com/squareup/okhttp/internal/OkHttpProtocolNegotiator.java
@@ -39,6 +39,7 @@
 import java.security.Security;
 import java.util.List;
 
+import javax.annotation.Nullable;
 import javax.net.ssl.SSLSocket;
 
 /**
@@ -80,8 +81,10 @@ private static OkHttpProtocolNegotiator createNegotiator() {
    * @throws RuntimeException if the negotiation completed, but no protocol was selected.
    */
   public String negotiate(
-      SSLSocket sslSocket, String hostname, List<Protocol> protocols) throws IOException {
-    configureTlsExtensions(sslSocket, hostname, protocols);
+      SSLSocket sslSocket, String hostname, @Nullable List<Protocol> protocols) throws IOException {
+    if (protocols != null) {
+      configureTlsExtensions(sslSocket, hostname, protocols);
+    }
     try {
       // Force handshake.
       sslSocket.startHandshake();