🛃 Filter headers in IntentHandler.

Bug: 873178
Change-Id: I700d6d710474b356e32f58b435d78e7e24cfb0ff
Reviewed-on: https://chromium-review.googlesource.com/1174537Reviewed-by: Paul Jensen <pauljensen@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Commit-Queue: Peter Conn <peconn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583273}

chrome/android/java/src/org/chromium/chrome/browser/IntentHandler.java

@@ -51,13 +51,13 @@ import org.chromium.content_public.browser.BrowserStartupController;
 import org.chromium.content_public.browser.LoadUrlParams;
 import org.chromium.content_public.common.ContentUrlConstants;
 import org.chromium.content_public.common.Referrer;
+import org.chromium.net.HttpUtil;
 import org.chromium.ui.base.PageTransition;
 import org.chromium.webapk.lib.common.WebApkConstants;
 
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 import java.util.ArrayList;
-import java.util.Iterator;
 import java.util.List;
 import java.util.Locale;
 
@@ -737,13 +737,14 @@ public class IntentHandler {
         Bundle bundleExtraHeaders = IntentUtils.safeGetBundleExtra(intent, Browser.EXTRA_HEADERS);
         if (bundleExtraHeaders == null) return null;
         StringBuilder extraHeaders = new StringBuilder();
-        Iterator<String> keys = bundleExtraHeaders.keySet().iterator();
-        while (keys.hasNext()) {
-            String key = keys.next();
+        for (String key : bundleExtraHeaders.keySet()) {
             String value = bundleExtraHeaders.getString(key);
-            if ("referer".equals(key.toLowerCase(Locale.US))) continue;
+
             // Strip the custom header that can only be added by ourselves.
             if ("x-chrome-intent-type".equals(key.toLowerCase(Locale.US))) continue;
+
+            if (!HttpUtil.isAllowedHeader(key, value)) continue;
+
             if (extraHeaders.length() != 0) extraHeaders.append("\n");
             extraHeaders.append(key);
             extraHeaders.append(": ");

net/BUILD.gn

@@ -406,6 +406,7 @@ component("net") {
       "android/gurl_utils.cc",
       "android/http_auth_negotiate_android.cc",
       "android/http_auth_negotiate_android.h",
+      "android/android_http_util.cc",
       "android/keystore.cc",
       "android/keystore.h",
       "android/legacy_openssl.h",
@@ -2996,6 +2997,7 @@ if (is_android) {
       "android/java/src/org/chromium/net/AndroidTrafficStats.java",
       "android/java/src/org/chromium/net/GURLUtils.java",
       "android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java",
+      "android/java/src/org/chromium/net/HttpUtil.java",
       "android/java/src/org/chromium/net/NetStringUtil.java",
       "android/java/src/org/chromium/net/NetworkChangeNotifier.java",
       "android/java/src/org/chromium/net/ProxyChangeListener.java",

net/android/BUILD.gn

@@ -15,6 +15,7 @@ android_library("net_java") {
     "java/src/org/chromium/net/GURLUtils.java",
     "java/src/org/chromium/net/HttpNegotiateAuthenticator.java",
     "java/src/org/chromium/net/HttpNegotiateConstants.java",
+    "java/src/org/chromium/net/HttpUtil.java",
     "java/src/org/chromium/net/NetStringUtil.java",
     "java/src/org/chromium/net/NetworkChangeNotifier.java",
     "java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java",
@@ -155,6 +156,7 @@ android_library("net_javatests") {
     "javatests/src/org/chromium/net/AndroidNetworkLibraryTestUtil.java",
     "javatests/src/org/chromium/net/AndroidProxyConfigServiceTestUtil.java",
     "javatests/src/org/chromium/net/AndroidProxySelectorTest.java",
+    "javatests/src/org/chromium/net/HttpUtilTest.java",
     "javatests/src/org/chromium/net/NetErrorsTest.java",
     "javatests/src/org/chromium/net/NetworkChangeNotifierNoNativeTest.java",
     "javatests/src/org/chromium/net/NetworkChangeNotifierTest.java",

net/android/android_http_util.cc

+// Copyright (c) 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/android/jni_string.h"
+#include "jni/HttpUtil_jni.h"
+#include "net/http/http_util.h"
+#include "url/gurl.h"
+
+using base::android::JavaParamRef;
+using base::android::ScopedJavaLocalRef;
+using base::android::ConvertJavaStringToUTF16;
+
+namespace net {
+
+jboolean JNI_HttpUtil_IsAllowedHeader(
+    JNIEnv* env,
+    const JavaParamRef<jclass>& clazz,
+    const JavaParamRef<jstring>& j_header_name,
+    const JavaParamRef<jstring>& j_header_value) {
+  std::string header_name(ConvertJavaStringToUTF8(env, j_header_name));
+  std::string header_value(ConvertJavaStringToUTF8(env, j_header_value));
+
+  return HttpUtil::IsValidHeaderName(header_name)
+    && HttpUtil::IsSafeHeader(header_name)
+    && HttpUtil::IsValidHeaderValue(header_value);
+}
+
+}  // namespace net
+

net/android/java/src/org/chromium/net/HttpUtil.java

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.net;
+
+import org.chromium.base.annotations.JNINamespace;
+
+/**
+ * Class to access HttpUtil library from Java.
+ *
+ * The corresponding native code is in net/android/android_http_util.cc.
+ */
+@JNINamespace("net")
+public final class HttpUtil {
+    /**
+     * Returns true iff:
+     * - |headerName| is validly formed (see HttpUtil::IsValidHeaderName).
+     * - |headerName| is not a forbidden header (see HttpUtil::IsSafeHeader).
+     * - |headerValue| is validly formed (see HttpUtil::IsValidHeaderValue).
+     */
+    public static boolean isAllowedHeader(String headerName, String headerValue) {
+        return nativeIsAllowedHeader(headerName, headerValue);
+    }
+    private static native boolean nativeIsAllowedHeader(String headerName, String headerValue);
+}

net/android/javatests/src/org/chromium/net/HttpUtilTest.java

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.net;
+
+import android.support.test.filters.MediumTest;
+
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.chromium.base.library_loader.LibraryLoader;
+import org.chromium.base.library_loader.LibraryProcessType;
+import org.chromium.base.library_loader.ProcessInitException;
+import org.chromium.base.test.BaseJUnit4ClassRunner;
+
+import java.util.Arrays;
+import java.util.List;
+
+/**
+ * Tests for {@link HttpUtil}. HttpUtil forwards to native code, and the lion's share of the logic
+ * is tested there. This test is primarily to make sure everything is plumbed through correctly.
+ */
+@RunWith(BaseJUnit4ClassRunner.class)
+public class HttpUtilTest {
+    private static final List<String> UNALLOWED_HEADER_NAMES = Arrays.asList(
+            "accept-encoding",  // Unsafe header.
+            "ACCEPT-ENCODING",  // Unsafe header.
+            "referer",  // Unsafe header.
+            "",  // Badly formed header.
+            "ref(erer",  // Badly formed header.
+            "ref\nerer"  // Badly formed header.
+    );
+
+    private static final List<String> ALLOWED_HEADER_NAMES = Arrays.asList(
+            "accept-language",
+            "Cache-Control"
+    );
+
+    private static final String UNALLOWED_HEADER_VALUE = "value\nAccept-Encoding: br";
+    private static final String ALLOWED_HEADER_VALUE = "value";
+
+    @Before
+    public void setUp() throws ProcessInitException {
+        LibraryLoader.getInstance().ensureInitialized(LibraryProcessType.PROCESS_BROWSER);
+    }
+
+    @Test
+    @MediumTest
+    public void testAllowedHeaders() {
+        for (String headerName: ALLOWED_HEADER_NAMES) {
+            Assert.assertTrue(headerName,
+                    HttpUtil.isAllowedHeader(headerName, ALLOWED_HEADER_VALUE));
+        }
+    }
+
+    @Test
+    @MediumTest
+    public void testUnallowedHeaders() {
+        for (String headerName: UNALLOWED_HEADER_NAMES) {
+            Assert.assertFalse(headerName,
+                    HttpUtil.isAllowedHeader(headerName, UNALLOWED_HEADER_VALUE));
+        }
+    }
+
+    @Test
+    @MediumTest
+    public void testUnallowedHeaderNames() {
+        for (String headerName: UNALLOWED_HEADER_NAMES) {
+            Assert.assertFalse(headerName,
+                    HttpUtil.isAllowedHeader(headerName, ALLOWED_HEADER_VALUE));
+        }
+    }
+
+
+    @Test
+    @MediumTest
+    public void testUnallowedHeaderValue() {
+        for (String headerName: ALLOWED_HEADER_NAMES) {
+            Assert.assertFalse(headerName,
+                    HttpUtil.isAllowedHeader(headerName, UNALLOWED_HEADER_VALUE));
+        }
+    }
+}