From 1f58b14bd18565b3dc4aaf15c072033e68b69660 Mon Sep 17 00:00:00 2001 From: jwilson Date: Sat, 2 Jul 2016 10:43:02 -0400 Subject: [PATCH] Always pass a host to NetworkSecurityPolicy.isCleartextTrafficPermitted(). Previously we were misinterpretting which hosts this method applied to. Suppose an Android app was configured to require TLS for bank.com and not for any other address. The NetworkSecurityPolicy.isCleartextTrafficPermitted() method would return false because cleartext traffic wasn't universally permitted. And OkHttp would incorrectly forbid cleartext communication to other hosts like puppies.com. Closes: https://github.com/square/okhttp/issues/2640 --- okhttp-tests/src/test/java/okhttp3/CallTest.java | 2 +- okhttp/src/main/java/okhttp3/OkHttpClient.java | 10 ++-------- .../internal/connection/RealConnection.java | 14 ++++++++++---- .../internal/platform/AndroidPlatform.java | 15 ++++++--------- .../java/okhttp3/internal/platform/Platform.java | 2 +- 5 files changed, 20 insertions(+), 23 deletions(-) diff --git a/okhttp-tests/src/test/java/okhttp3/CallTest.java b/okhttp-tests/src/test/java/okhttp3/CallTest.java index f6569fe79867..a87ecc7b5efa 100644 --- a/okhttp-tests/src/test/java/okhttp3/CallTest.java +++ b/okhttp-tests/src/test/java/okhttp3/CallTest.java @@ -1063,7 +1063,7 @@ private void postBodyRetransmittedAfterAuthorizationFail(String body) throws Exc client.newCall(request).execute(); fail(); } catch (UnknownServiceException expected) { - assertTrue(expected.getMessage().contains("CLEARTEXT communication not supported")); + assertEquals("CLEARTEXT communication not enabled for client", expected.getMessage()); } } diff --git a/okhttp/src/main/java/okhttp3/OkHttpClient.java b/okhttp/src/main/java/okhttp3/OkHttpClient.java index 6402b02022a2..c14ee105f510 100644 --- a/okhttp/src/main/java/okhttp3/OkHttpClient.java +++ b/okhttp/src/main/java/okhttp3/OkHttpClient.java @@ -65,16 +65,10 @@ public class OkHttpClient implements Cloneable, Call.Factory { private static final List DEFAULT_PROTOCOLS = Util.immutableList( Protocol.HTTP_2, Protocol.SPDY_3, Protocol.HTTP_1_1); - private static final List DEFAULT_CONNECTION_SPECS; + private static final List DEFAULT_CONNECTION_SPECS = Util.immutableList( + ConnectionSpec.MODERN_TLS, ConnectionSpec.COMPATIBLE_TLS, ConnectionSpec.CLEARTEXT); static { - List connSpecs = new ArrayList<>(Arrays.asList(ConnectionSpec.MODERN_TLS, - ConnectionSpec.COMPATIBLE_TLS)); - if (Platform.get().isCleartextTrafficPermitted()) { - connSpecs.add(ConnectionSpec.CLEARTEXT); - } - DEFAULT_CONNECTION_SPECS = Util.immutableList(connSpecs); - Internal.instance = new Internal() { @Override public void addLenient(Headers.Builder builder, String line) { builder.addLenient(line); diff --git a/okhttp/src/main/java/okhttp3/internal/connection/RealConnection.java b/okhttp/src/main/java/okhttp3/internal/connection/RealConnection.java index c21931b6027b..5dddb549e845 100644 --- a/okhttp/src/main/java/okhttp3/internal/connection/RealConnection.java +++ b/okhttp/src/main/java/okhttp3/internal/connection/RealConnection.java @@ -93,10 +93,16 @@ public void connect(int connectTimeout, int readTimeout, int writeTimeout, RouteException routeException = null; ConnectionSpecSelector connectionSpecSelector = new ConnectionSpecSelector(connectionSpecs); - if (route.address().sslSocketFactory() == null - && !connectionSpecs.contains(ConnectionSpec.CLEARTEXT)) { - throw new RouteException(new UnknownServiceException( - "CLEARTEXT communication not supported: " + connectionSpecs)); + if (route.address().sslSocketFactory() == null) { + if (!connectionSpecs.contains(ConnectionSpec.CLEARTEXT)) { + throw new RouteException(new UnknownServiceException( + "CLEARTEXT communication not enabled for client")); + } + String host = route.address().url().host(); + if (!Platform.get().isCleartextTrafficPermitted(host)) { + throw new RouteException(new UnknownServiceException( + "CLEARTEXT communication to " + host + " not permitted by network security policy")); + } } while (protocol == null) { diff --git a/okhttp/src/main/java/okhttp3/internal/platform/AndroidPlatform.java b/okhttp/src/main/java/okhttp3/internal/platform/AndroidPlatform.java index a903f3b6c3cb..bccdc0b5898d 100644 --- a/okhttp/src/main/java/okhttp3/internal/platform/AndroidPlatform.java +++ b/okhttp/src/main/java/okhttp3/internal/platform/AndroidPlatform.java @@ -132,20 +132,17 @@ public AndroidPlatform(Class sslParametersClass, OptionalMethod setUs } } - @Override public boolean isCleartextTrafficPermitted() { + @Override public boolean isCleartextTrafficPermitted(String hostname) { try { Class networkPolicyClass = Class.forName("android.security.NetworkSecurityPolicy"); Method getInstanceMethod = networkPolicyClass.getMethod("getInstance"); Object networkSecurityPolicy = getInstanceMethod.invoke(null); Method isCleartextTrafficPermittedMethod = networkPolicyClass - .getMethod("isCleartextTrafficPermitted"); - boolean cleartextPermitted = (boolean) isCleartextTrafficPermittedMethod - .invoke(networkSecurityPolicy); - return cleartextPermitted; - } catch (ClassNotFoundException e) { - return super.isCleartextTrafficPermitted(); - } catch (NoSuchMethodException | IllegalAccessException | IllegalArgumentException - | InvocationTargetException e) { + .getMethod("isCleartextTrafficPermitted", String.class); + return (boolean) isCleartextTrafficPermittedMethod.invoke(networkSecurityPolicy, hostname); + } catch (ClassNotFoundException | NoSuchMethodException e) { + return super.isCleartextTrafficPermitted(hostname); + } catch (IllegalAccessException | IllegalArgumentException | InvocationTargetException e) { throw new AssertionError(); } } diff --git a/okhttp/src/main/java/okhttp3/internal/platform/Platform.java b/okhttp/src/main/java/okhttp3/internal/platform/Platform.java index 1d94c2e69ecd..477ff23e9af7 100644 --- a/okhttp/src/main/java/okhttp3/internal/platform/Platform.java +++ b/okhttp/src/main/java/okhttp3/internal/platform/Platform.java @@ -129,7 +129,7 @@ public void log(int level, String message, Throwable t) { logger.log(logLevel, message, t); } - public boolean isCleartextTrafficPermitted() { + public boolean isCleartextTrafficPermitted(String hostname) { return true; }