From 5aa2456a145883f6688c6ee325635ed0c6339076 Mon Sep 17 00:00:00 2001 From: Jesse Wilson Date: Thu, 20 Sep 2012 16:01:17 -0400 Subject: [PATCH] Fix TLS requiresTunnel which was being computed incorrectly. We were only returning 'true' once we were already in a tunnel. This was bogus. In theory a TLS tunnel sending extra data could be corrupted due to this bug. Also migrate one of the TLS tunnel tests to use SslContextBuilder instead of TestSSLContext. --- pom.xml | 2 +- .../squareup/okhttp/OkHttpsConnection.java | 4 - .../java/libcore/net/http/HttpConnection.java | 35 ++++---- .../java/libcore/net/http/HttpEngine.java | 6 +- .../net/http/HttpsURLConnectionImpl.java | 6 +- .../libcore/net/http/URLConnectionTest.java | 85 ++++++++++--------- 6 files changed, 62 insertions(+), 76 deletions(-) diff --git a/pom.xml b/pom.xml index 336c964ea..8c9affb16 100644 --- a/pom.xml +++ b/pom.xml @@ -38,7 +38,7 @@ 1.6 8.1.2.v20120308 - 20120731 + 20120905 1.47 diff --git a/src/main/java/com/squareup/okhttp/OkHttpsConnection.java b/src/main/java/com/squareup/okhttp/OkHttpsConnection.java index 669a26e93..d1c144fd3 100644 --- a/src/main/java/com/squareup/okhttp/OkHttpsConnection.java +++ b/src/main/java/com/squareup/okhttp/OkHttpsConnection.java @@ -266,8 +266,6 @@ public abstract class OkHttpsConnection extends OkHttpConnection { /** * Returns the hostname verifier used by this instance. - * - * @return the hostname verifier used by this instance. */ public HostnameVerifier getHostnameVerifier() { return hostnameVerifier; @@ -290,8 +288,6 @@ public abstract class OkHttpsConnection extends OkHttpConnection { /** * Returns the SSL socket factory used by this instance. - * - * @return the SSL socket factory used by this instance. */ public SSLSocketFactory getSSLSocketFactory() { return sslSocketFactory; diff --git a/src/main/java/libcore/net/http/HttpConnection.java b/src/main/java/libcore/net/http/HttpConnection.java index 110f9aa73..4e7800ae5 100644 --- a/src/main/java/libcore/net/http/HttpConnection.java +++ b/src/main/java/libcore/net/http/HttpConnection.java @@ -116,21 +116,21 @@ final class HttpConnection { * for the SSL socket. */ int bufferSize = 128; - inputStream = address.requiresTunnel + inputStream = address.requiresTunnel() ? socket.getInputStream() : new BufferedInputStream(socket.getInputStream(), bufferSize); outputStream = socket.getOutputStream(); } public static HttpConnection connect(URI uri, SSLSocketFactory sslSocketFactory, - Proxy proxy, boolean requiresTunnel, int connectTimeout) throws IOException { + Proxy proxy, int connectTimeout) throws IOException { /* * Try an explicitly-specified proxy. */ if (proxy != null) { Address address = (proxy.type() == Proxy.Type.DIRECT) ? new Address(uri, sslSocketFactory) - : new Address(uri, sslSocketFactory, proxy, requiresTunnel); + : new Address(uri, sslSocketFactory, proxy); return HttpConnectionPool.INSTANCE.get(address, connectTimeout); } @@ -148,8 +148,7 @@ final class HttpConnection { continue; } try { - Address address = new Address(uri, sslSocketFactory, - selectedProxy, requiresTunnel); + Address address = new Address(uri, sslSocketFactory, selectedProxy); return HttpConnectionPool.INSTANCE.get(address, connectTimeout); } catch (IOException e) { // failed to connect, tell it to the selector @@ -293,7 +292,6 @@ final class HttpConnection { */ public static final class Address { private final Proxy proxy; - private final boolean requiresTunnel; private final String uriHost; private final int uriPort; private final String socketHost; @@ -302,7 +300,6 @@ final class HttpConnection { public Address(URI uri, SSLSocketFactory sslSocketFactory) throws UnknownHostException { this.proxy = null; - this.requiresTunnel = false; this.uriHost = uri.getHost(); this.uriPort = Libcore.getEffectivePort(uri); this.sslSocketFactory = sslSocketFactory; @@ -313,16 +310,9 @@ final class HttpConnection { } } - /** - * @param requiresTunnel true if the HTTP connection needs to tunnel one - * protocol over another, such as when using HTTPS through an HTTP - * proxy. When doing so, we must avoid buffering bytes intended for - * the higher-level protocol. - */ - public Address(URI uri, SSLSocketFactory sslSocketFactory, - Proxy proxy, boolean requiresTunnel) throws UnknownHostException { + public Address(URI uri, SSLSocketFactory sslSocketFactory, Proxy proxy) + throws UnknownHostException { this.proxy = proxy; - this.requiresTunnel = requiresTunnel; this.uriHost = uri.getHost(); this.uriPort = Libcore.getEffectivePort(uri); this.sslSocketFactory = sslSocketFactory; @@ -350,8 +340,7 @@ final class HttpConnection { return Objects.equal(this.proxy, that.proxy) && this.uriHost.equals(that.uriHost) && this.uriPort == that.uriPort - && Objects.equal(this.sslSocketFactory, that.sslSocketFactory) - && this.requiresTunnel == that.requiresTunnel; + && Objects.equal(this.sslSocketFactory, that.sslSocketFactory); } return false; } @@ -362,12 +351,20 @@ final class HttpConnection { result = 31 * result + uriPort; result = 31 * result + (sslSocketFactory != null ? sslSocketFactory.hashCode() : 0); result = 31 * result + (proxy != null ? proxy.hashCode() : 0); - result = 31 * result + (requiresTunnel ? 1 : 0); return result; } public HttpConnection connect(int connectTimeout) throws IOException { return new HttpConnection(this, connectTimeout); } + + /** + * Returns true if the HTTP connection needs to tunnel one protocol over + * another, such as when using HTTPS through an HTTP proxy. When doing so, + * we must avoid buffering bytes intended for the higher-level protocol. + */ + public boolean requiresTunnel() { + return sslSocketFactory != null && proxy != null && proxy.type() == Proxy.Type.HTTP; + } } } diff --git a/src/main/java/libcore/net/http/HttpEngine.java b/src/main/java/libcore/net/http/HttpEngine.java index aac0e53cb..e24821ae1 100644 --- a/src/main/java/libcore/net/http/HttpEngine.java +++ b/src/main/java/libcore/net/http/HttpEngine.java @@ -279,7 +279,7 @@ public class HttpEngine { protected final HttpConnection openSocketConnection() throws IOException { HttpConnection result = HttpConnection.connect(uri, getSslSocketFactory(), - policy.getProxy(), requiresTunnel(), policy.getConnectTimeout()); + policy.getProxy(), policy.getConnectTimeout()); Proxy proxy = result.getAddress().getProxy(); if (proxy != null) { policy.setProxy(proxy); @@ -580,10 +580,6 @@ public class HttpEngine { return result; } - protected boolean requiresTunnel() { - return false; - } - /** * Flushes the remaining request header and body, parses the HTTP response * headers and starts reading the HTTP response body if it exists. diff --git a/src/main/java/libcore/net/http/HttpsURLConnectionImpl.java b/src/main/java/libcore/net/http/HttpsURLConnectionImpl.java index 0ea33f7c1..a69d27101 100644 --- a/src/main/java/libcore/net/http/HttpsURLConnectionImpl.java +++ b/src/main/java/libcore/net/http/HttpsURLConnectionImpl.java @@ -452,7 +452,7 @@ public final class HttpsURLConnectionImpl extends OkHttpsConnection { // make an SSL Tunnel on the first message pair of each SSL + proxy connection if (connection == null) { connection = openSocketConnection(); - if (connection.getAddress().getProxy() != null) { + if (connection.getAddress().requiresTunnel()) { makeTunnel(policy, connection, getRequestHeaders()); } } @@ -527,9 +527,5 @@ public final class HttpsURLConnectionImpl extends OkHttpsConnection { HttpConnection connection) throws IOException { super(policy, HttpEngine.CONNECT, requestHeaders, connection, null); } - - @Override protected boolean requiresTunnel() { - return true; - } } } diff --git a/src/test/java/libcore/net/http/URLConnectionTest.java b/src/test/java/libcore/net/http/URLConnectionTest.java index b000b49e4..ae4abd2b5 100644 --- a/src/test/java/libcore/net/http/URLConnectionTest.java +++ b/src/test/java/libcore/net/http/URLConnectionTest.java @@ -23,6 +23,7 @@ import com.google.mockwebserver.SocketPolicy; import com.squareup.okhttp.OkHttpConnection; import com.squareup.okhttp.OkHttpsConnection; import java.io.ByteArrayOutputStream; +import java.io.File; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; @@ -54,6 +55,7 @@ import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.UUID; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; import java.util.zip.GZIPInputStream; @@ -636,48 +638,47 @@ public final class URLConnectionTest extends TestCase { assertEquals(Arrays.asList("verify android.com"), hostnameVerifier.calls); } -// /** -// * Tolerate bad https proxy response when using HttpResponseCache. http://b/6754912 -// */ -// public void testConnectViaHttpProxyToHttpsUsingBadProxyAndHttpResponseCache() throws Exception { -// ProxyConfig proxyConfig = ProxyConfig.PROXY_SYSTEM_PROPERTY; -// -// TestSSLContext testSSLContext = TestSSLContext.create(); -// -// initResponseCache(); -// -// server.useHttps(testSSLContext.serverContext.getSocketFactory(), true); -// server.enqueue(new MockResponse() -// .setSocketPolicy(SocketPolicy.UPGRADE_TO_SSL_AT_END) -// .clearHeaders() -// .setBody("bogus proxy connect response content")); // Key to reproducing b/6754912 -// server.play(); -// -// URL url = new URL("https://android.com/foo"); -// HttpsURLConnection connection = (HttpsURLConnection) proxyConfig.connect(server, url); -// connection.setSSLSocketFactory(testSSLContext.clientContext.getSocketFactory()); -// -// try { -// connection.connect(); -// fail(); -// } catch (IOException expected) { -// // Thrown when the connect causes SSLSocket.startHandshake() to throw -// // when it sees the "bogus proxy connect response content" -// // instead of a ServerHello handshake message. -// } -// -// RecordedRequest connect = server.takeRequest(); -// assertEquals("Connect line failure on proxy", -// "CONNECT android.com:443 HTTP/1.1", connect.getRequestLine()); -// assertContains(connect.getHeaders(), "Host: android.com"); -// } -// -// private void initResponseCache() throws IOException { -// String tmp = System.getProperty("java.io.tmpdir"); -// File cacheDir = new File(tmp, "HttpCache-" + UUID.randomUUID()); -// cache = new HttpResponseCache(cacheDir, Integer.MAX_VALUE); -// ResponseCache.setDefault(cache); -// } + /** + * Tolerate bad https proxy response when using HttpResponseCache. http://b/6754912 + */ + public void testConnectViaHttpProxyToHttpsUsingBadProxyAndHttpResponseCache() throws Exception { + ProxyConfig proxyConfig = ProxyConfig.PROXY_SYSTEM_PROPERTY; + + initResponseCache(); + + server.useHttps(sslContext.getSocketFactory(), true); + MockResponse response = new MockResponse() // Key to reproducing b/6754912 + .setSocketPolicy(SocketPolicy.UPGRADE_TO_SSL_AT_END) + .setBody("bogus proxy connect response content"); + server.enqueue(response); // For the first TLS tolerant connection + server.enqueue(response); // For the backwards-compatible SSLv3 retry + server.play(); + + URL url = new URL("https://android.com/foo"); + OkHttpsConnection connection = (OkHttpsConnection) proxyConfig.connect(server, url); + connection.setSSLSocketFactory(sslContext.getSocketFactory()); + + try { + connection.connect(); + fail(); + } catch (IOException expected) { + // Thrown when the connect causes SSLSocket.startHandshake() to throw + // when it sees the "bogus proxy connect response content" + // instead of a ServerHello handshake message. + } + + RecordedRequest connect = server.takeRequest(); + assertEquals("Connect line failure on proxy", + "CONNECT android.com:443 HTTP/1.1", connect.getRequestLine()); + assertContains(connect.getHeaders(), "Host: android.com"); + } + + private void initResponseCache() throws IOException { + String tmp = System.getProperty("java.io.tmpdir"); + File cacheDir = new File(tmp, "HttpCache-" + UUID.randomUUID()); + cache = new HttpResponseCache(cacheDir, Integer.MAX_VALUE); + ResponseCache.setDefault(cache); + } /** * Test which headers are sent unencrypted to the HTTP proxy.