HttpsURLConnection retry should not invoke X509TrustManager and HostnameVerifier more than once
Summary:
In 2.3, HttpsURLConnection was change to retry TLS connections as SSL
connections w/o compression to deal with servers that are TLS
intolerant. However, if the handshake proceeded to the point of
invoking the X509TrustManager, we should not retry. Similarly, if we
should not invoke the HostnameVerifier repeatedly, and need to wait
until the SSL handshake has completed.
Tested with (includes two new tests for this issue):
libcore/luni/src/test/java/libcore/javax/net/ssl/
libcore/luni/src/test/java/libcore/java/net/URLConnectionTest.java
libcore/luni/src/test/java/org/apache/harmony/luni/tests/internal/net/www/protocol/https/HttpsURLConnectionTest.java
Details:
HttpConnection.setupSecureSocket has been broken into two
pieces. setupSecureSocket now just does the SSL
handshaking. verifySecureSocketHostname now does the
verification. The old HttpConnection code was careful never to
assign its sslSocket field until verification was complete. A new
unverifiedSocket field is added to store the sslSocket before
verification is completed by verifySecureSocketHostname.
luni/src/main/java/org/apache/harmony/luni/internal/net/www/protocol/http/HttpConnection.java
HttpsEngine.makeConnection now skips TLS intolerant retry if the
reason for the makeSslConnection failure was a
CertificateException, since that implies that we failed during
certification validation after initial handshaking. We also
prevent retrying hostname verification by moving it out of
makeSslConnection and only doing it on new SSL connections,
tracking the changes to HttpConnection.setupSecureSocket mentioned
above. We also now skip the redundant call to setUpTransportIO in
makeSslConnection on reused SSLSockets.
luni/src/main/java/org/apache/harmony/luni/internal/net/www/protocol/https/HttpsURLConnectionImpl.java
Instead of throwing away the underlying CertificateExceptions, set
them as the cause of the SSLExceptions. This is what the RI does
in the case of X509TrustManager failures and is now used by
HttpsEngine.makeConnection.
luni/src/main/java/org/apache/harmony/xnet/provider/jsse/OpenSSLSessionImpl.java
luni/src/main/java/org/apache/harmony/xnet/provider/jsse/OpenSSLSocketImpl.java
Added new testConnectViaHttpsToUntrustedServer which makes sure
that connections are not retried on certificate verification
failure.
luni/src/test/java/libcore/java/net/URLConnectionTest.java
Added new test_SSLSocket_untrustedServer that verifies that an
SSLHandshakeException is thown containing a CertificateException
is thrown on certificate verification problems.
luni/src/test/java/libcore/javax/net/ssl/SSLSocketTest.java
Added second test CA and a new TestKeyStore.getClientCA2 test key
store that does not trust the primary test key stores. This is
useful for negative testing and is used in the above two new
tests.
support/src/test/java/libcore/java/security/TestKeyStore.java
Issue: http://code.google.com/p/android/issues/detail?id=13178
Bug: 3292412
Change-Id: I37136bb65f04d2bceaf2f32f542d6432c8b76ad4
diff --git a/luni/src/main/java/org/apache/harmony/luni/internal/net/www/protocol/http/HttpConnection.java b/luni/src/main/java/org/apache/harmony/luni/internal/net/www/protocol/http/HttpConnection.java
index 990789c..32c801f 100644
--- a/luni/src/main/java/org/apache/harmony/luni/internal/net/www/protocol/http/HttpConnection.java
+++ b/luni/src/main/java/org/apache/harmony/luni/internal/net/www/protocol/http/HttpConnection.java
@@ -52,6 +52,7 @@
private InputStream inputStream;
private OutputStream outputStream;
+ private SSLSocket unverifiedSocket;
private SSLSocket sslSocket;
private InputStream sslInputStream;
private OutputStream sslOutputStream;
@@ -157,21 +158,18 @@
}
/**
- * Return an {@code SSLSocket} that is connected and passed hostname verification.
+ * Create an {@code SSLSocket} and perform the SSL handshake
+ * (performing certificate validation.
*
* @param sslSocketFactory Source of new {@code SSLSocket} instances.
- * @param hostnameVerifier Used to verify the hostname we
- * connected to is an acceptable match for the peer certificate
- * chain of the SSLSession.
* @param tlsTolerant If true, assume server can handle common
* TLS extensions and SSL deflate compression. If false, use
* an SSL3 only fallback mode without compression.
*/
- public SSLSocket setupSecureSocket(SSLSocketFactory sslSocketFactory,
- HostnameVerifier hostnameVerifier,
- boolean tlsTolerant) throws IOException {
+ public void setupSecureSocket(SSLSocketFactory sslSocketFactory, boolean tlsTolerant)
+ throws IOException {
// create the wrapper over connected socket
- SSLSocket unverifiedSocket = (SSLSocket) sslSocketFactory.createSocket(socket,
+ unverifiedSocket = (SSLSocket) sslSocketFactory.createSocket(socket,
address.uriHost, address.uriPort, true /* autoClose */);
// tlsTolerant mimics Chrome's behavior
if (tlsTolerant && unverifiedSocket instanceof OpenSSLSocketImpl) {
@@ -183,6 +181,20 @@
} else {
unverifiedSocket.setEnabledProtocols(new String [] { "SSLv3" });
}
+ // force handshake, which can throw
+ unverifiedSocket.startHandshake();
+ }
+
+ /**
+ * Return an {@code SSLSocket} that is not only connected but has
+ * also passed hostname verification.
+ *
+ * @param hostnameVerifier Used to verify the hostname we
+ * connected to is an acceptable match for the peer certificate
+ * chain of the SSLSession.
+ */
+ public SSLSocket verifySecureSocketHostname(HostnameVerifier hostnameVerifier)
+ throws IOException {
if (!hostnameVerifier.verify(address.uriHost, unverifiedSocket.getSession())) {
throw new IOException("Hostname '" + address.uriHost + "' was not verified");
}
diff --git a/luni/src/main/java/org/apache/harmony/luni/internal/net/www/protocol/https/HttpsURLConnectionImpl.java b/luni/src/main/java/org/apache/harmony/luni/internal/net/www/protocol/https/HttpsURLConnectionImpl.java
index 9db8f96..5897685 100644
--- a/luni/src/main/java/org/apache/harmony/luni/internal/net/www/protocol/https/HttpsURLConnectionImpl.java
+++ b/luni/src/main/java/org/apache/harmony/luni/internal/net/www/protocol/https/HttpsURLConnectionImpl.java
@@ -25,9 +25,11 @@
import java.security.Permission;
import java.security.Principal;
import java.security.cert.Certificate;
+import java.security.cert.CertificateException;
import java.util.List;
import java.util.Map;
import javax.net.ssl.HttpsURLConnection;
+import javax.net.ssl.SSLHandshakeException;
import javax.net.ssl.SSLPeerUnverifiedException;
import javax.net.ssl.SSLSocket;
import org.apache.harmony.luni.internal.net.www.protocol.http.HttpConnection;
@@ -367,27 +369,39 @@
return;
}
+ boolean connectionReused;
// first try an SSL connection with compression and
// various TLS extensions enabled, if it fails (and its
// not unheard of that it will) fallback to a more
// barebones connections
try {
- makeSslConnection(true);
+ connectionReused = makeSslConnection(true);
} catch (IOException e) {
+ // If the problem was a CertificateException from the X509TrustManager,
+ // do not retry, we didn't have an abrupt server initiated exception.
+ if (e instanceof SSLHandshakeException
+ && e.getCause() instanceof CertificateException) {
+ throw e;
+ }
releaseSocket(false);
- sslSocket = null;
- makeSslConnection(false);
+ connectionReused = makeSslConnection(false);
}
+
+ if (!connectionReused) {
+ sslSocket = connection.verifySecureSocketHostname(getHostnameVerifier());
+ }
+ setUpTransportIO(connection);
}
/**
- * Attempt to make an https connection.
+ * Attempt to make an https connection. Returns true if a
+ * connection was reused, false otherwise.
*
* @param tlsTolerant If true, assume server can handle common
* TLS extensions and SSL deflate compression. If false, use
* an SSL3 only fallback mode without compression.
*/
- private void makeSslConnection(boolean tlsTolerant) throws IOException {
+ private boolean makeSslConnection(boolean tlsTolerant) throws IOException {
super.makeConnection();
@@ -399,11 +413,7 @@
// we already have an SSL connection,
if (sslSocket != null) {
- // ensure requestOut etc are reinitialized. they will
- // not have been set by super.makeSslConnection's call
- // to setUpTransportIO because sslSocket was not yet set.
- setUpTransportIO(connection);
- return;
+ return true;
}
// make SSL Tunnel
@@ -420,10 +430,8 @@
}
}
- sslSocket = connection.setupSecureSocket(getSSLSocketFactory(),
- getHostnameVerifier(),
- tlsTolerant);
- setUpTransportIO(connection);
+ connection.setupSecureSocket(getSSLSocketFactory(), tlsTolerant);
+ return false;
}
@Override protected void setUpTransportIO(HttpConnection connection) throws IOException {
diff --git a/luni/src/main/java/org/apache/harmony/xnet/provider/jsse/OpenSSLSessionImpl.java b/luni/src/main/java/org/apache/harmony/xnet/provider/jsse/OpenSSLSessionImpl.java
index 70cccc0..75e4cdd 100644
--- a/luni/src/main/java/org/apache/harmony/xnet/provider/jsse/OpenSSLSessionImpl.java
+++ b/luni/src/main/java/org/apache/harmony/xnet/provider/jsse/OpenSSLSessionImpl.java
@@ -234,9 +234,13 @@
}
return chain;
} catch (CertificateEncodingException e) {
- throw new SSLPeerUnverifiedException(e.getMessage());
+ SSLPeerUnverifiedException exception = new SSLPeerUnverifiedException(e.getMessage());
+ exception.initCause(exception);
+ throw exception;
} catch (CertificateException e) {
- throw new SSLPeerUnverifiedException(e.getMessage());
+ SSLPeerUnverifiedException exception = new SSLPeerUnverifiedException(e.getMessage());
+ exception.initCause(exception);
+ throw exception;
}
}
diff --git a/luni/src/main/java/org/apache/harmony/xnet/provider/jsse/OpenSSLSocketImpl.java b/luni/src/main/java/org/apache/harmony/xnet/provider/jsse/OpenSSLSocketImpl.java
index 3bd59a9..4bd91ad 100644
--- a/luni/src/main/java/org/apache/harmony/xnet/provider/jsse/OpenSSLSocketImpl.java
+++ b/luni/src/main/java/org/apache/harmony/xnet/provider/jsse/OpenSSLSocketImpl.java
@@ -35,7 +35,7 @@
import javax.net.ssl.HandshakeCompletedEvent;
import javax.net.ssl.HandshakeCompletedListener;
import javax.net.ssl.SSLException;
-import javax.net.ssl.SSLPeerUnverifiedException;
+import javax.net.ssl.SSLHandshakeException;
import javax.net.ssl.SSLSession;
import javax.security.auth.x500.X500Principal;
import org.apache.harmony.security.provider.cert.X509CertImpl;
@@ -474,7 +474,9 @@
sslSessionNativePointer = NativeCrypto.SSL_do_handshake(sslNativePointer, fd, this,
getSoTimeout(), client);
} catch (CertificateException e) {
- throw new SSLPeerUnverifiedException(e.getMessage());
+ SSLHandshakeException exception = new SSLHandshakeException(e.getMessage());
+ exception.initCause(e);
+ throw exception;
}
byte[] sessionId = NativeCrypto.SSL_SESSION_session_id(sslSessionNativePointer);
sslSession = (OpenSSLSessionImpl) sessionContext.getSession(sessionId);
diff --git a/luni/src/test/java/libcore/java/net/URLConnectionTest.java b/luni/src/test/java/libcore/java/net/URLConnectionTest.java
index 99c486d..e3394a5 100644
--- a/luni/src/test/java/libcore/java/net/URLConnectionTest.java
+++ b/luni/src/test/java/libcore/java/net/URLConnectionTest.java
@@ -55,10 +55,12 @@
import javax.net.ssl.HttpsURLConnection;
import javax.net.ssl.SSLContext;
import javax.net.ssl.SSLException;
+import javax.net.ssl.SSLHandshakeException;
import javax.net.ssl.SSLSession;
import javax.net.ssl.SSLSocketFactory;
import javax.net.ssl.TrustManager;
import javax.net.ssl.X509TrustManager;
+import libcore.java.security.TestKeyStore;
import libcore.javax.net.ssl.TestSSLContext;
import tests.http.DefaultResponseCache;
import tests.http.MockResponse;
@@ -461,6 +463,30 @@
assertEquals("GET /foo HTTP/1.1", request.getRequestLine());
}
+ /**
+ * Verify that we don't retry connections on certificate verification errors.
+ *
+ * http://code.google.com/p/android/issues/detail?id=13178
+ */
+ public void testConnectViaHttpsToUntrustedServer() throws IOException, InterruptedException {
+ TestSSLContext testSSLContext = TestSSLContext.create(TestKeyStore.getClientCA2(),
+ TestKeyStore.getServer());
+
+ server.useHttps(testSSLContext.serverContext.getSocketFactory(), false);
+ server.enqueue(new MockResponse()); // unused
+ server.play();
+
+ HttpsURLConnection connection = (HttpsURLConnection) server.getUrl("/foo").openConnection();
+ connection.setSSLSocketFactory(testSSLContext.clientContext.getSocketFactory());
+ try {
+ connection.getInputStream();
+ fail();
+ } catch (SSLHandshakeException expected) {
+ assertTrue(expected.getCause() instanceof CertificateException);
+ }
+ assertEquals(0, server.getRequestCount());
+ }
+
public void testConnectViaProxyUsingProxyArg() throws Exception {
testConnectViaProxy(ProxyConfig.CREATE_ARG);
}
diff --git a/luni/src/test/java/libcore/javax/net/ssl/SSLSocketTest.java b/luni/src/test/java/libcore/javax/net/ssl/SSLSocketTest.java
index 5e9567a..92b152b 100644
--- a/luni/src/test/java/libcore/javax/net/ssl/SSLSocketTest.java
+++ b/luni/src/test/java/libcore/javax/net/ssl/SSLSocketTest.java
@@ -24,11 +24,13 @@
import java.net.SocketTimeoutException;
import java.security.Principal;
import java.security.cert.Certificate;
+import java.security.cert.CertificateException;
import java.util.Arrays;
import javax.net.ssl.HandshakeCompletedEvent;
import javax.net.ssl.HandshakeCompletedListener;
import javax.net.ssl.SSLContext;
import javax.net.ssl.SSLException;
+import javax.net.ssl.SSLHandshakeException;
import javax.net.ssl.SSLParameters;
import javax.net.ssl.SSLPeerUnverifiedException;
import javax.net.ssl.SSLProtocolException;
@@ -501,6 +503,33 @@
}
}
+ public void test_SSLSocket_untrustedServer() throws Exception {
+ TestSSLContext c = TestSSLContext.create(TestKeyStore.getClientCA2(),
+ TestKeyStore.getServer());
+ SSLSocket client = (SSLSocket) c.clientContext.getSocketFactory().createSocket(c.host,
+ c.port);
+ final SSLSocket server = (SSLSocket) c.serverSocket.accept();
+ Thread thread = new Thread(new Runnable () {
+ public void run() {
+ try {
+ server.startHandshake();
+ } catch (RuntimeException e) {
+ throw e;
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ }
+ });
+ thread.start();
+ try {
+ client.startHandshake();
+ fail();
+ } catch (SSLHandshakeException expected) {
+ assertTrue(expected.getCause() instanceof CertificateException);
+ }
+ thread.join();
+ }
+
public void test_SSLSocket_clientAuth() throws Exception {
TestSSLContext c = TestSSLContext.create(TestKeyStore.getClientCertificate(),
TestKeyStore.getServer());
diff --git a/support/src/test/java/libcore/java/security/TestKeyStore.java b/support/src/test/java/libcore/java/security/TestKeyStore.java
index 346149e..e2d8828 100644
--- a/support/src/test/java/libcore/java/security/TestKeyStore.java
+++ b/support/src/test/java/libcore/java/security/TestKeyStore.java
@@ -133,6 +133,17 @@
false,
INTERMEDIATE_CA);
+ private static final TestKeyStore ROOT_CA_2
+ = create(new String[] { "RSA" },
+ null,
+ null,
+ "RootCA2",
+ x509Principal("Test Root Certificate Authority 2"),
+ true,
+ null);
+ private static final TestKeyStore CLIENT_2
+ = new TestKeyStore(createClient(ROOT_CA_2.keyStore), null, null);
+
/**
* Return a server keystore with a matched RSA certificate and
* private key as well as a CA certificate.
@@ -157,6 +168,15 @@
}
/**
+ * Return a keystore with a second CA certificate that does not
+ * trust the server certificate returned by getServer for negative
+ * testing.
+ */
+ public static TestKeyStore getClientCA2() {
+ return CLIENT_2;
+ }
+
+ /**
* Create a new KeyStore containing the requested key types.
* Since key generation can be expensive, most tests should reuse
* the RSA-only singleton instance returned by TestKeyStore.get
