AW: disallow secure cookies with HTTP on R

This adds a targetSdk quirk for WebView's CookieManager API. Starting
for apps which target Android R, WebView will no longer accept cookies
for insecure ('http' scheme) URLs which also specify the 'Secure' cookie
directive.

While this changed for Chrome in M58
(https://www.chromestatus.com/feature/4506322921848832), WebView has
been working around this by fixing up the URL's scheme to 'https'.
WebView will continue to do so for apps targeting < R.

This adds a histogram to track the new case.

Bug: 933981
Test: run_webview_instrumentation_test_apk -f CookieManagerTest#testSetSecureCookieForHttpUrl*
Change-Id: Ie31f02911d38984983fdf4afeb4d6e1eb4f7d094
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2040685Reviewed-by: Richard Coles <torne@chromium.org>
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#739044}

android_webview/browser/cookie_manager.cc

@@ -13,6 +13,7 @@
 #include "android_webview/browser/aw_browser_context.h"
 #include "android_webview/browser/aw_cookie_access_policy.h"
 #include "android_webview/browser_jni_headers/AwCookieManager_jni.h"
+#include "base/android/build_info.h"
 #include "base/android/callback_android.h"
 #include "base/android/jni_string.h"
 #include "base/android/path_utils.h"
@@ -88,7 +89,8 @@ enum class SecureCookieAction {
   kInvalidCookie = 2,
   kNotASecureCookie = 3,
   kFixedUp = 4,
-  kMaxValue = kFixedUp,
+  kDisallowedAndroidR = 5,
+  kMaxValue = kDisallowedAndroidR,
 };
 
 // Since this function parses the set-cookie line into a ParsedCookie, it is
@@ -97,13 +99,17 @@ enum class SecureCookieAction {
 GURL MaybeFixUpSchemeForSecureCookieAndGetSameSite(
     const GURL& host,
     const std::string& value,
-    net::CookieSameSiteString* samesite_out) {
+    bool workaround_http_secure_cookies,
+    net::CookieSameSiteString* samesite_out,
+    bool* should_allow_cookie) {
   net::ParsedCookie parsed_cookie(value);
 
   // Grab the SameSite value for histogramming.
   DCHECK(samesite_out);
   parsed_cookie.SameSite(samesite_out);
 
+  *should_allow_cookie = true;
+
   // Log message for catching strict secure cookies related bugs.
   // TODO(ntfschr): try to remove this, based on UMA stats
   // (https://crbug.com/933981)
@@ -128,9 +134,20 @@ GURL MaybeFixUpSchemeForSecureCookieAndGetSameSite(
     return host;
   }
 
-  LOG(WARNING) << "Strict Secure Cookie policy does not allow setting a "
-                  "secure cookie for "
-               << host.spec();
+  LOG(ERROR) << "Strict Secure Cookie policy does not allow setting a "
+                "secure cookie for "
+             << host.spec()
+             << " for apps targeting >= R. Please either use the 'https:' "
+                "scheme for this URL or omit the 'Secure' directive in the "
+                "cookie value.";
+  if (!workaround_http_secure_cookies) {
+    // Don't allow setting this cookie if we target >= R.
+    *should_allow_cookie = false;
+    base::UmaHistogramEnumeration(kSecureCookieHistogramName,
+                                  SecureCookieAction::kDisallowedAndroidR);
+    return host;
+  }
+
   base::UmaHistogramEnumeration(kSecureCookieHistogramName,
                                 SecureCookieAction::kFixedUp);
   GURL::Replacements replace_host;
@@ -185,6 +202,8 @@ base::FilePath GetPathInAppDirectory(std::string path) {
 CookieManager::CookieManager()
     : allow_file_scheme_cookies_(kDefaultFileSchemeAllowed),
       cookie_store_created_(false),
+      workaround_http_secure_cookies_(
+          base::android::BuildInfo::GetInstance()->targets_at_least_r()),
       cookie_store_client_thread_("CookieMonsterClient"),
       cookie_store_backend_thread_("CookieMonsterBackend"),
       setting_new_mojo_cookie_manager_(false) {
@@ -385,6 +404,23 @@ void CookieManager::SwapMojoCookieManagerAsync(
   RunPendingCookieTasks();
 }
 
+void CookieManager::SetWorkaroundHttpSecureCookiesForTesting(
+    JNIEnv* env,
+    const JavaParamRef<jobject>& obj,
+    jboolean allow) {
+  ExecCookieTaskSync(
+      base::BindOnce(&CookieManager::SetWorkaroundHttpSecureCookiesAsyncHelper,
+                     base::Unretained(this), allow));
+}
+
+void CookieManager::SetWorkaroundHttpSecureCookiesAsyncHelper(
+    bool allow,
+    base::OnceClosure complete) {
+  DCHECK(cookie_store_task_runner_->RunsTasksInCurrentSequence());
+  workaround_http_secure_cookies_ = allow;
+  std::move(complete).Run();
+}
+
 void CookieManager::SetShouldAcceptCookies(JNIEnv* env,
                                            const JavaParamRef<jobject>& obj,
                                            jboolean accept) {
@@ -429,9 +465,13 @@ void CookieManager::SetCookieSync(JNIEnv* env,
 void CookieManager::SetCookieHelper(const GURL& host,
                                     const std::string& value,
                                     base::OnceCallback<void(bool)> callback) {
+  DCHECK(cookie_store_task_runner_->RunsTasksInCurrentSequence());
+
   net::CookieSameSiteString samesite = net::CookieSameSiteString::kUnspecified;
-  const GURL& new_host =
-      MaybeFixUpSchemeForSecureCookieAndGetSameSite(host, value, &samesite);
+  bool should_allow_cookie = true;
+  const GURL& new_host = MaybeFixUpSchemeForSecureCookieAndGetSameSite(
+      host, value, workaround_http_secure_cookies_, &samesite,
+      &should_allow_cookie);
 
   UMA_HISTOGRAM_ENUMERATION(
       "Android.WebView.CookieManager.SameSiteAttributeValue", samesite);
@@ -441,7 +481,7 @@ void CookieManager::SetCookieHelper(const GURL& host,
       net::CanonicalCookie::Create(new_host, value, base::Time::Now(),
                                    base::nullopt /* server_time */, &status));
 
-  if (!cc) {
+  if (!cc || !should_allow_cookie) {
     MaybeRunCookieCallback(std::move(callback), false);
     return;
   }

android_webview/browser/cookie_manager.h

@@ -92,6 +92,14 @@ class CookieManager {
   void SetMojoCookieManager(
       mojo::PendingRemote<network::mojom::CookieManager> cookie_manager_remote);
 
+  // Configure whether or not this CookieManager should workaround cookies
+  // specified for insecure URLs with the 'Secure' directive. See
+  // |workaround_http_secure_cookies_| for the default behavior. This should not
+  // be needed in production, as the default is the desirable behavior.
+  void SetWorkaroundHttpSecureCookiesForTesting(
+      JNIEnv* env,
+      const base::android::JavaParamRef<jobject>& obj,
+      jboolean allow);
   void SetShouldAcceptCookies(JNIEnv* env,
                               const base::android::JavaParamRef<jobject>& obj,
                               jboolean accept);
@@ -183,6 +191,9 @@ class CookieManager {
                        const std::string& value,
                        base::OnceCallback<void(bool)> callback);
 
+  void SetWorkaroundHttpSecureCookiesAsyncHelper(bool allow,
+                                                 base::OnceClosure complete);
+
   void GotCookies(const std::vector<net::CanonicalCookie>& cookies);
   void GetCookieListAsyncHelper(const GURL& host,
                                 net::CookieList* result,
@@ -232,6 +243,12 @@ class CookieManager {
   // |cookie_store_task_runner_|.
   bool cookie_store_created_;
 
+  // Whether or not to workaround 'Secure' cookies set on insecure URLs. See
+  // MaybeFixUpSchemeForSecureCookieAndGetSameSite. Only accessed on
+  // |cookie_store_task_runner_|. Defaults to false starting for apps targeting
+  // >= R.
+  bool workaround_http_secure_cookies_;
+
   base::Thread cookie_store_client_thread_;
   base::Thread cookie_store_backend_thread_;
 

android_webview/java/src/org/chromium/android_webview/AwCookieManager.java

@@ -8,6 +8,7 @@ import android.os.Handler;
 import android.os.Looper;
 
 import androidx.annotation.Nullable;
+import androidx.annotation.VisibleForTesting;
 
 import org.chromium.base.Callback;
 import org.chromium.base.annotations.JNINamespace;
@@ -176,6 +177,16 @@ public final class AwCookieManager {
                 mNativeCookieManager, AwCookieManager.this, accept);
     }
 
+    /**
+     * Sets whether cookies for insecure schemes (http:) are permitted to include the "Secure"
+     * directive.
+     */
+    @VisibleForTesting
+    public void setWorkaroundHttpSecureCookiesForTesting(boolean allow) {
+        AwCookieManagerJni.get().setWorkaroundHttpSecureCookiesForTesting(
+                mNativeCookieManager, AwCookieManager.this, allow);
+    }
+
     /**
      * CookieCallback is a bridge that knows how to call a Callback on its original thread.
      * We need to arrange for the users Callback#onResult to be called on the original
@@ -269,5 +280,7 @@ public final class AwCookieManager {
         boolean getAllowFileSchemeCookies(long nativeCookieManager, AwCookieManager caller);
         void setAllowFileSchemeCookies(
                 long nativeCookieManager, AwCookieManager caller, boolean allow);
+        void setWorkaroundHttpSecureCookiesForTesting(
+                long nativeCookieManager, AwCookieManager caller, boolean allow);
     }
 }

android_webview/javatests/src/org/chromium/android_webview/test/AwActivityTestRule.java

@@ -41,6 +41,8 @@ import java.lang.annotation.Annotation;
 import java.util.Map;
 import java.util.concurrent.BlockingQueue;
 import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
 import java.util.regex.Matcher;
@@ -487,6 +489,44 @@ public class AwActivityTestRule extends ActivityTestRule<AwTestRunnerActivity> {
         pollInstrumentationThread(() -> TestThreadUtils.runOnUiThreadBlocking(callable));
     }
 
+    /**
+     * Waits for {@code future} and returns its value (or times out). If {@code future} has an
+     * associated Exception, this will re-throw that Exception on the instrumentation thread
+     * (wrapping with an unchecked Exception if necessary, to avoid requiring callers to declare
+     * checked Exceptions).
+     *
+     * @param future the {@link Future} representing a value of interest.
+     * @return the value {@code future} represents.
+     */
+    public static <T> T waitForFuture(Future<T> future) {
+        try {
+            return future.get(WAIT_TIMEOUT_MS, TimeUnit.MILLISECONDS);
+        } catch (ExecutionException e) {
+            // ExecutionException means this Future has an associated Exception that we should
+            // re-throw on the current thread. We throw the cause instead of ExecutionException,
+            // since ExecutionException itself isn't interesting, and might mislead those debugging
+            // test failures to suspect this method is the culprit (whereas the root cause is from
+            // another thread).
+            Throwable cause = e.getCause();
+            // If the cause is an unchecked Throwable type, re-throw as-is.
+            if (cause instanceof Error) throw(Error) cause;
+            if (cause instanceof RuntimeException) throw(RuntimeException) cause;
+            // Otherwise, wrap this in an unchecked Exception so callers don't need to declare
+            // checked Exceptions.
+            throw new RuntimeException(cause);
+        } catch (InterruptedException | TimeoutException e) {
+            // Don't call e.getCause() for either of these. Unlike ExecutionException, these don't
+            // wrap the root cause, but rather are themselves interesting. Again, we wrap these
+            // checked Exceptions with an unchecked Exception for the caller's convenience.
+            //
+            // Although we might be tempted to handle InterruptedException by calling
+            // Thread.currentThread().interrupt(), this is not correct in this case. The interrupted
+            // thread was likely a different thread than the current thread, so there's nothing
+            // special we need to do.
+            throw new RuntimeException(e);
+        }
+    }
+
     /**
      * Takes an element out of the {@link BlockingQueue} (or times out).
      */

android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerTest.java

@@ -11,6 +11,8 @@ import android.util.Pair;
 
 import androidx.annotation.IntDef;
 
+import com.google.common.util.concurrent.SettableFuture;
+
 import org.junit.After;
 import org.junit.Assert;
 import org.junit.Before;
@@ -375,13 +377,16 @@ public class CookieManagerTest {
     @Test
     @MediumTest
     @Feature({"AndroidWebView", "Privacy"})
-    public void testSetSecureCookieForHttpUrl() {
+    public void testSetSecureCookieForHttpUrlNotTargetingAndroidR() {
+        mCookieManager.setWorkaroundHttpSecureCookiesForTesting(true);
         Assert.assertEquals(
                 0, RecordHistogram.getHistogramTotalCountForTesting(SECURE_COOKIE_HISTOGRAM_NAME));
         String url = "http://www.example.com";
         String secureUrl = "https://www.example.com";
         String cookie = "name=test";
-        mCookieManager.setCookie(url, cookie + ";secure");
+        boolean success = setCookieOnUiThreadSync(url, cookie + ";secure");
+
+        Assert.assertTrue("Setting the cookie should succeed", success);
         assertCookieEquals(cookie, secureUrl);
         Assert.assertEquals(
                 1, RecordHistogram.getHistogramTotalCountForTesting(SECURE_COOKIE_HISTOGRAM_NAME));
@@ -390,6 +395,29 @@ public class CookieManagerTest {
                         SECURE_COOKIE_HISTOGRAM_NAME, 4 /* kFixedUp */));
     }
 
+    @Test
+    @MediumTest
+    @Feature({"AndroidWebView", "Privacy"})
+    public void testSetSecureCookieForHttpUrlTargetingAndroidR() {
+        mCookieManager.setWorkaroundHttpSecureCookiesForTesting(false);
+        Assert.assertEquals(
+                0, RecordHistogram.getHistogramTotalCountForTesting(SECURE_COOKIE_HISTOGRAM_NAME));
+        String url = "http://www.example.com";
+        String secureUrl = "https://www.example.com";
+        String cookie = "name=test";
+        boolean success = setCookieOnUiThreadSync(url, cookie + ";secure");
+
+        Assert.assertFalse("Setting the cookie should fail", success);
+        assertNoCookies(url);
+        assertNoCookies(secureUrl);
+
+        Assert.assertEquals(
+                1, RecordHistogram.getHistogramTotalCountForTesting(SECURE_COOKIE_HISTOGRAM_NAME));
+        Assert.assertEquals(1,
+                RecordHistogram.getHistogramValueCountForTesting(
+                        SECURE_COOKIE_HISTOGRAM_NAME, 5 /* kDisallowedAndroidR */));
+    }
+
     @Test
     @MediumTest
     @Feature({"AndroidWebView", "Privacy"})
@@ -1183,6 +1211,17 @@ public class CookieManagerTest {
                 () -> mCookieManager.setCookie(url, cookie, callback));
     }
 
+    private boolean setCookieOnUiThreadSync(final String url, final String cookie) {
+        final SettableFuture<Boolean> cookieResultFuture = SettableFuture.create();
+        InstrumentationRegistry.getInstrumentation().runOnMainSync(
+                () -> mCookieManager.setCookie(url, cookie, cookieResultFuture::set));
+        Boolean success = AwActivityTestRule.waitForFuture(cookieResultFuture);
+        if (success == null) {
+            throw new RuntimeException("setCookie() should never return null in its callback");
+        }
+        return success;
+    }
+
     private void removeSessionCookiesOnUiThread(final Callback<Boolean> callback) {
         InstrumentationRegistry.getInstrumentation().runOnMainSync(
                 () -> mCookieManager.removeSessionCookies(callback));

base/android/build_info.cc

@@ -77,7 +77,8 @@ BuildInfo::BuildInfo(const std::vector<std::string>& params)
       resources_version_(StrDupParam(params, 20)),
       extracted_file_suffix_(params[21]),
       is_at_least_q_(GetIntParam(params, 22)),
-      is_debug_android_(GetIntParam(params, 23)) {}
+      targets_at_least_r_(GetIntParam(params, 23)),
+      is_debug_android_(GetIntParam(params, 24)) {}
 
 // static
 BuildInfo* BuildInfo::GetInstance() {

base/android/build_info.h

@@ -124,6 +124,8 @@ class BASE_EXPORT BuildInfo {
 
   bool is_at_least_q() const { return is_at_least_q_; }
 
+  bool targets_at_least_r() const { return targets_at_least_r_; }
+
   bool is_debug_android() const { return is_debug_android_; }
 
  private:
@@ -159,6 +161,7 @@ class BASE_EXPORT BuildInfo {
   // Not needed by breakpad.
   const std::string extracted_file_suffix_;
   const bool is_at_least_q_;
+  const bool targets_at_least_r_;
   const bool is_debug_android_;
 
   DISALLOW_COPY_AND_ASSIGN(BuildInfo);

base/android/java/src/org/chromium/base/BuildInfo.java

@@ -84,6 +84,7 @@ public class BuildInfo {
                 buildInfo.resourcesVersion,
                 buildInfo.extractedFileSuffix,
                 isAtLeastQ() ? "1" : "0",
+                targetsAtLeastR() ? "1" : "0",
                 isDebugAndroid() ? "1" : "0",
         };
     }

tools/metrics/histograms/enums.xml

@@ -57713,8 +57713,11 @@ Called by update_net_trust_anchors.py.-->
       label="Cookie was not marked as 'Secure', so no URL-modification was
              performed"/>
   <int value="4"
-      label="The URL's scheme was fixed up 'HTTPS' for backwards
-             compatibility"/>
+      label="The URL's scheme was fixed up to 'HTTPS' for backwards
+             compatibility (app targets &lt; Android R)"/>
+  <int value="5"
+      label="The cookie was rejected because it uses both 'HTTP' scheme and
+             'Secure' directive (app targets &gt;= Android R)"/>
 </enum>
 
 <enum name="SecurityInterstitialDecision">