Make request line parsing more robust
- Break up a gnarly function for parsing requests and opening a
socket.
- Explicitly handle some error cases.
- Document where the request handling is not spec-compliant.
- Add unit tests.
Bug: 15324060
Change-Id: I7f7b2dbb3820745c217fcd6a4507d36bab1f1edb
diff --git a/Android.mk b/Android.mk
index fa85200..7a0bbfd 100644
--- a/Android.mk
+++ b/Android.mk
@@ -17,6 +17,8 @@
LOCAL_CERTIFICATE := platform
LOCAL_PRIVILEGED_MODULE := true
+LOCAL_PROGUARD_FLAG_FILES := proguard.flags
+
include frameworks/opt/setupwizard/library/common.mk
include $(BUILD_PACKAGE)
diff --git a/comm/Android.mk b/comm/Android.mk
index 70005b5..13f137a 100644
--- a/comm/Android.mk
+++ b/comm/Android.mk
@@ -18,6 +18,8 @@
LOCAL_SDK_VERSION := current
+LOCAL_STATIC_JAVA_LIBRARIES := guava
+
LOCAL_PROTOC_FLAGS := --proto_path=$(LOCAL_PATH)/protos/
LOCAL_PROTOC_OPTIMIZE_TYPE := nano
@@ -29,3 +31,4 @@
include $(BUILD_STATIC_JAVA_LIBRARY)
+include $(call all-makefiles-under,$(LOCAL_PATH))
\ No newline at end of file
diff --git a/comm/src/com/android/managedprovisioning/comm/ProxyConnection.java b/comm/src/com/android/managedprovisioning/comm/ProxyConnection.java
index 4da6924..ff31c26 100644
--- a/comm/src/com/android/managedprovisioning/comm/ProxyConnection.java
+++ b/comm/src/com/android/managedprovisioning/comm/ProxyConnection.java
@@ -18,28 +18,39 @@
import com.android.managedprovisioning.comm.Bluetooth.NetworkData;
+import com.google.common.annotations.VisibleForTesting;
+
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.io.PipedInputStream;
import java.io.PipedOutputStream;
import java.net.InetSocketAddress;
+import java.net.MalformedURLException;
import java.net.Proxy;
import java.net.ProxySelector;
import java.net.Socket;
import java.net.URI;
import java.net.URISyntaxException;
+import java.net.URL;
import java.util.ArrayList;
import java.util.List;
/**
- * The connection between a channel and a socket to a web server. It does a basic check on the
- * first line and then passes through all data like a dummy proxy.
- */
+ * The connection between a channel and a socket to a web server. This connection handles most
+ * compliant proxy clients. It expects an initial CONNECT request
+ * {@see http://tools.ietf.org/html/rfc2817#section-5.2}. Additionally, it can handle clients which
+ * omit the CONNECT request, as long as they specify an absoluteURI in their request line.
+ * {@see http://www.w3.org/Protocols/rfc2616/rfc2616-sec5.html}.
+ *
+ * If a client does not send a CONNECT request, and attempts to make a request using an
+ * abs_path {@see http://www.w3.org/Protocols/rfc2616/rfc2616-sec5.html} in the request line, the
+ * connection will be rejected.
+ */
public class ProxyConnection extends Thread {
private static final String CONNECT = "CONNECT";
-
+ private static final String HTTPS = "https";
private static final String RESPONSE_OK = "HTTP/1.1 200 OK\n\n";
private NetToBtThread mNetToBt;
@@ -196,97 +207,155 @@
return buffer.toString();
}
+ @VisibleForTesting
+ protected static class RequestLineInfo {
+ String method;
+ URI uri;
+
+ public RequestLineInfo(String method, URI uri) {
+ this.method = method;
+ this.uri = uri;
+ }
+ }
+
+ /**
+ * Parse a request line. Supports CONNECT requests, as well as other requests using absoluteURI
+ * {@see http://www.w3.org/Protocols/rfc2616/rfc2616-sec5.html}
+ * {@see http://tools.ietf.org/html/rfc2817#section-5.2}
+ * {@see https://www.ietf.org/rfc/rfc2396.txt}
+ * @param requestLine The requestLine from the HTTP request
+ * @return A struct containing the parsed request if parsing was successful, otherwise null.
+ */
+ @VisibleForTesting
+ protected static RequestLineInfo parseRequestLine(String requestLine) {
+ String[] split = requestLine.split(" ");
+
+ if (split.length < 2) {
+ ProvisionCommLogger.loge("Could not parse request line: " + requestLine);
+ return null;
+ }
+
+ String method = split[0];
+ String uriString = split[1];
+
+ if (CONNECT.equals(method)) {
+ // CONNECT request lines come through as an 'authority' element (see RFC 2396), which do
+ // not contain a scheme. We don't need the scheme to open the socket, but we do need it
+ // for the ProxySelector - so force it to HTTPS.
+ if (!uriString.contains("://")) {
+ uriString = HTTPS + "://" + uriString;
+ }
+ }
+
+ URI uri;
+ try {
+ // parse with URL first - this is more restrictive, and catches the case where a GET
+ // request comes through with an abs_path, but no host, in the request line. This
+ // situation is unsupported by this proxy (but should never happen with a compliant
+ // client - we should always see a CONNECT request first).
+ URL url = new URL(uriString);
+ uri = url.toURI();
+ } catch (MalformedURLException|URISyntaxException e) {
+ ProvisionCommLogger.loge(
+ "Invalid or unsupported URI in request line: " + requestLine, e);
+ return null;
+ }
+
+ if (uri.getPort() < 0) {
+ // If no port was specified, choose a default
+ int newPort = 80;
+ if (CONNECT.equals(method) || HTTPS.equals(uri.getScheme().toLowerCase())) {
+ newPort = 443;
+ }
+
+ try {
+ // sadly this is the only way to "mutate" a URI
+ uri = new URI(uri.getScheme(), uri.getUserInfo(), uri.getHost(), newPort,
+ uri.getPath(), uri.getQuery(), uri.getFragment());
+ } catch (URISyntaxException e) {
+ ProvisionCommLogger.loge(
+ "Invalid URI when trying to enforce https: " + requestLine, e);
+ return null;
+ }
+ }
+
+ return new RequestLineInfo(method, uri);
+ }
+
+ /**
+ * Create a socket to the requested host. Check for an applicable proxy, and use it if found.
+ * @param uri URI containing the host to connect to.
+ * @return
+ * @throws IOException
+ */
+ private boolean createSocket(URI uri) throws IOException {
+ boolean usingProxy = false;
+
+ String host = uri.getHost();
+ int port = uri.getPort();
+
+ List<Proxy> list = ProxySelector.getDefault().select(uri);
+ for (Proxy proxy : list) {
+ if (proxy.equals(Proxy.NO_PROXY)) {
+ break; // break out and create a normal socket
+ } else {
+ if (proxy.address() instanceof InetSocketAddress) {
+ // Only Inets created by PacProxySelector and ProxySelectorImpl.
+ InetSocketAddress inetSocketAddress =
+ (InetSocketAddress)proxy.address();
+ // A proxy specified with an IP addr should only ever use that IP. This
+ // will ensure that the proxy only ever connects to its specified
+ // address. If the proxy is resolved, use the associated IP address. If
+ // unresolved, use the specified host name.
+ host = inetSocketAddress.isUnresolved() ?
+ inetSocketAddress.getHostName() :
+ inetSocketAddress.getAddress().getHostAddress();
+ port = inetSocketAddress.getPort();
+ usingProxy = true;
+ break;
+ } else {
+ ProvisionCommLogger.logw("Unsupported Inet type from ProxySelector, skipping:" +
+ proxy.address().getClass().getSimpleName());
+ }
+ }
+ }
+
+ mNetSocket = new Socket(host, port);
+ return usingProxy;
+ }
+
private void processConnect() {
try {
String requestLine = getLine() + '\r' + '\n';
- String[] split = requestLine.split(" ");
+ RequestLineInfo info = parseRequestLine(requestLine);
- String method = split[0];
- String uri = split[1];
-
- ProvisionCommLogger.logi("Method: " + method);
- String host = "";
- int port = 80;
- String toSend = "";
-
- if (CONNECT.equals(method)) {
- String[] hostPortSplit = uri.split(":");
- host = hostPortSplit[0];
- try {
- port = Integer.parseInt(hostPortSplit[1]);
- } catch (NumberFormatException nfe) {
- port = 443;
- }
- uri = "Https://" + host + ":" + port;
- } else {
- try {
- URI url = new URI(uri);
- host = url.getHost();
- port = url.getPort();
- if (port < 0) {
- port = 80;
- }
- } catch (URISyntaxException e) {
- ProvisionCommLogger.logw("Trying to proxy invalid URL", e);
- mNetRunning = false;
- return;
- }
- toSend = requestLine;
+ if (info == null) {
+ mNetRunning = false;
+ return;
}
- List<Proxy> list = new ArrayList<Proxy>();
+ boolean usingProxy;
try {
- list = ProxySelector.getDefault().select(new URI(uri));
- } catch (URISyntaxException e) {
- ProvisionCommLogger.loge("Unable to parse URI from request", e);
+ usingProxy = createSocket(info.uri);
+ } catch (IOException e) {
+ ProvisionCommLogger.loge("Failed to create socket: " +
+ info.uri.getHost() + ":" + info.uri.getPort(), e);
+ mNetRunning = false;
+ mNetSocket = null;
+ return;
}
- for (Proxy proxy : list) {
- try {
- if (proxy.equals(Proxy.NO_PROXY)) {
- mNetSocket = new Socket(host, port);
- if (CONNECT.equals(method)) {
- handleConnect();
- } else {
- toSend = requestLine;
- }
- } else {
- if (proxy.address() instanceof InetSocketAddress) {
- // Only Inets created by PacProxySelector and ProxySelectorImpl.
- InetSocketAddress inetSocketAddress =
- (InetSocketAddress)proxy.address();
- // A proxy specified with an IP addr should only ever use that IP. This
- // will ensure that the proxy only ever connects to its specified
- // address. If the proxy is resolved, use the associated IP address. If
- // unresolved, use the specified host name.
- String hostName = inetSocketAddress.isUnresolved() ?
- inetSocketAddress.getHostName() :
- inetSocketAddress.getAddress().getHostAddress();
- mNetSocket = new Socket(hostName, inetSocketAddress.getPort());
- toSend = requestLine;
- } else {
- ProvisionCommLogger.logw("Unsupported Inet Type from ProxySelector");
- continue;
- }
- }
- } catch (IOException ioe) {
- }
- if (mNetSocket != null) {
- break;
- }
- }
+ String toSend = "";
if (mNetSocket == null) {
- mNetSocket = new Socket(host, port);
- if (CONNECT.equals(method)) {
+ if (CONNECT.equals(info.method) && !usingProxy) {
+ // If we're not talking to a proxy, and we're handling a CONNECT, we need to
+ // send a response
handleConnect();
} else {
- toSend = requestLine;
+ mNetSocket.getOutputStream().write(toSend.getBytes());
}
}
- // For HTTP or PROXY, send the request back out.
- mNetSocket.getOutputStream().write(toSend.getBytes());
-
mNetToBt = new NetToBtThread();
mNetToBt.start();
mBtToNet = new BtToNetThread();
diff --git a/comm/tests/Android.mk b/comm/tests/Android.mk
new file mode 100644
index 0000000..92b67f9
--- /dev/null
+++ b/comm/tests/Android.mk
@@ -0,0 +1,18 @@
+LOCAL_PATH:= $(call my-dir)
+include $(CLEAR_VARS)
+
+LOCAL_MODULE_TAGS := tests
+
+LOCAL_STATIC_JAVA_LIBRARIES := android-support-test
+
+LOCAL_SRC_FILES := $(call all-java-files-under, src)
+
+LOCAL_PACKAGE_NAME := ManagedProvisioningCommTests
+LOCAL_INSTRUMENTATION_FOR := ManagedProvisioning
+LOCAL_CERTIFICATE := platform
+
+LOCAL_PROGUARD_ENABLED := disabled
+
+include $(BUILD_PACKAGE)
+
+include $(call all-makefiles-under,$(LOCAL_PATH))
diff --git a/comm/tests/AndroidManifest.xml b/comm/tests/AndroidManifest.xml
new file mode 100644
index 0000000..d9ed5f0
--- /dev/null
+++ b/comm/tests/AndroidManifest.xml
@@ -0,0 +1,38 @@
+<?xml version="1.0" encoding="utf-8"?>
+<!-- Copyright (C) 2008 The Android Open Source Project
+
+ Licensed under the Apache License, Version 2.0 (the "License");
+ you may not use this file except in compliance with the License.
+ You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+
+<!-- package name must be unique so suffix with "tests" so package loader doesn't ignore us -->
+<manifest xmlns:android="http://schemas.android.com/apk/res/android"
+ package="com.android.managedprovisioning.comm.tests">
+
+ <!-- We add an application tag here just so that we can indicate that
+ this package needs to link against the android.test library,
+ which is needed when building test cases. -->
+ <application>
+ <uses-library android:name="android.test.runner" />
+ </application>
+
+ <!--
+ This declares that this app uses the instrumentation test runner targeting
+ the package of com.android.managedprovisioning.comm. To run the tests use the command:
+ "adb shell am instrument \
+ -w com.android.managedprovisioning.comm.tests/android.support.test.runner.AndroidJUnitRunner"
+ -->
+ <instrumentation android:name="android.support.test.runner.AndroidJUnitRunner"
+ android:targetPackage="com.android.managedprovisioning"
+ android:label="Tests for ManagedProvisioning"/>
+
+</manifest>
diff --git a/comm/tests/src/com/android/managedprovisioning/comm/ProxyConnectionTest.java b/comm/tests/src/com/android/managedprovisioning/comm/ProxyConnectionTest.java
new file mode 100644
index 0000000..deda8c5
--- /dev/null
+++ b/comm/tests/src/com/android/managedprovisioning/comm/ProxyConnectionTest.java
@@ -0,0 +1,161 @@
+/*
+ * Copyright (C) 2015 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.android.managedprovisioning.comm;
+
+import org.junit.Test;
+
+import java.net.URI;
+
+import static junit.framework.Assert.assertEquals;
+import static junit.framework.Assert.assertNull;
+
+public class ProxyConnectionTest {
+ private static final String CONNECT = "CONNECT";
+ private static final String GET = "GET";
+ private static final String VERSION = "HTTP/1.1";
+ private static final String HTTPS = "https://";
+ private static final String HTTP = "http://";
+ private static final String HTTPS_PORT = "443";
+ private static final String HTTP_PORT = "80";
+
+ private void checkInfo(String exMethod, String exHost, int exPort, URI exUri,
+ ProxyConnection.RequestLineInfo actual) {
+ assertEquals(exMethod, actual.method);
+
+ // compare host and port separately to ensure the URI was parsed as expected (e.g. URI will
+ // happily parse "www.google.com:80" into a scheme of "www.google.com" and a host of "80").
+ assertEquals(exHost, actual.uri.getHost());
+ assertEquals(exPort, actual.uri.getPort());
+
+ // still compare the full URI to ensure any extra bits match
+ assertEquals(exUri, actual.uri);
+ }
+
+ @Test
+ public void testTypicalConnect() throws Exception {
+ String host = "www.google.com";
+ int port = 443;
+ String hostPort = host + ":" + port;
+ URI uri = new URI(hostPort);
+ String line = CONNECT + " " + uri.toString() + " " + VERSION;
+
+ ProxyConnection.RequestLineInfo info = ProxyConnection.parseRequestLine(line);
+
+ checkInfo(CONNECT, host, port, new URI(HTTPS + hostPort), info);
+ }
+
+ @Test
+ public void testConnectWithScheme() throws Exception {
+ String host = "www.google.com";
+ int port = 443;
+ URI uri = new URI(HTTPS + host + ":" + port);
+ String line = CONNECT + " " + uri + " " + VERSION;
+
+ ProxyConnection.RequestLineInfo info = ProxyConnection.parseRequestLine(line);
+
+ checkInfo(CONNECT, host, port, uri, info);
+ }
+
+ @Test
+ public void testConnectWithoutPort() throws Exception {
+ String host = "www.google.com";
+ URI uri = new URI(host);
+ String line = CONNECT + " " + uri + " " + VERSION;
+
+ ProxyConnection.RequestLineInfo info = ProxyConnection.parseRequestLine(line);
+
+ checkInfo(CONNECT, host, Integer.parseInt(HTTPS_PORT),
+ new URI(HTTPS + host + ":" + HTTPS_PORT), info);
+ }
+
+ @Test
+ public void testConnectInvalidAddress() throws Exception {
+ String host = "LessThan(<)IsNotAllowedInURIs";
+ String line = CONNECT + " " + host + " " + VERSION;
+
+ ProxyConnection.RequestLineInfo info = ProxyConnection.parseRequestLine(line);
+ assertNull(info);
+ }
+
+ @Test
+ public void testGet() throws Exception {
+ String host = "www.google.com";
+ int port = 8001;
+ URI uri = new URI(HTTPS + host + ":" + port);
+ String line = GET + " " + uri + " " + VERSION;
+
+ ProxyConnection.RequestLineInfo info = ProxyConnection.parseRequestLine(line);
+
+ checkInfo(GET, host, port, uri, info);
+ }
+
+ @Test
+ public void testGetHttpsWithoutPort() throws Exception {
+ String host = "www.google.com";
+ URI uri = new URI(HTTPS + host);
+ String line = GET + " " + uri + " " + VERSION;
+
+ ProxyConnection.RequestLineInfo info = ProxyConnection.parseRequestLine(line);
+
+ checkInfo(GET, host, Integer.parseInt(HTTPS_PORT), new URI(HTTPS + host + ":" + HTTPS_PORT),
+ info);
+ }
+
+ @Test
+ public void testGetHttpWithoutPort() throws Exception {
+ String host = "www.google.com";
+ URI uri = new URI(HTTP + host);
+ String line = GET + " " + uri + " " + VERSION;
+
+ ProxyConnection.RequestLineInfo info = ProxyConnection.parseRequestLine(line);
+
+ checkInfo(GET, host, Integer.parseInt(HTTP_PORT), new URI(HTTP + host + ":" + HTTP_PORT),
+ info);
+ }
+
+ @Test
+ public void testGetAbsPath() throws Exception {
+ // GET requests using abs_path in the request line are unsupported
+ String path = "/foo/bar/baz.html";
+ String line = GET + " " + path + " " + VERSION;
+
+ ProxyConnection.RequestLineInfo info = ProxyConnection.parseRequestLine(line);
+
+ assertNull(info);
+ }
+
+ @Test
+ public void testGetNoScheme() throws Exception {
+ // GET requests with no scheme should also fail
+ String host = "www.google.com";
+ int port = 80;
+ URI uri = new URI(host + ":" + port);
+ String line = GET + " " + uri + " " + VERSION;
+
+ ProxyConnection.RequestLineInfo info = ProxyConnection.parseRequestLine(line);
+
+ assertNull(info);
+ }
+
+ @Test
+ public void testGarbage() {
+ String data = "\u1D4D\u1D44\u1D89\u1D66\u1D43\u1D4D\u1D49";
+ ProxyConnection.RequestLineInfo info = ProxyConnection.parseRequestLine(data);
+ assertNull(info);
+ }
+
+}
diff --git a/proguard.flags b/proguard.flags
new file mode 100644
index 0000000..398c17c
--- /dev/null
+++ b/proguard.flags
@@ -0,0 +1,3 @@
+-keep class com.android.managedprovisioning.comm.ProxyConnection
+-keep class com.android.managedprovisioning.comm.ProxyConnection$RequestLineInfo
+