Android: Restrict data directory for both Chrome and WebView

Context.getDir() creates Chrome's data directory, app_chrome/, with
rwxrwx--x. ChromeMainDelegateAndroid::RunProcess() then limits this to
rwx------. RunProcess() goes out of its way to avoid granting any user
permissions that weren't already present, but this seems like a mistake;
it shouldn't be possible for app_chrome/ to have fewer permissions than
rwx------. So RunProcess is simplified to set the permissions to exactly
rwx------. Also don't print data_path in the error message because if
PathService::Get() failed, data_path is empty.

Also restrict WebView's directory, app_webview/, using Os.chown(). Doing
this in PathUtils covers both Chrome and WebView. However, Os.chown()
requires API >= 21, which is the case for WebView but not Chrome, so
Chrome's RunProcess() code must stay for now.

Rehabilitate //chrome/test:chrome_app_unittests to run on Android (crbug
609855 says it was broken but it seems to work now) and add a unit test
for the simplified native code.

BUG=832388,609855
internal bug b/19993402

Change-Id: I1bcfe72940ddc1fb23f2b0bef50775853843ea76
Reviewed-on: https://chromium-review.googlesource.com/984773
Commit-Queue: Paul Miller <paulmiller@chromium.org>
Reviewed-by: Tao Bai <michaelbai@chromium.org>
Reviewed-by: Richard Coles <torne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551795}

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

@@ -4,11 +4,14 @@
 
 package org.chromium.base;
 
+import android.annotation.SuppressLint;
 import android.content.Context;
 import android.content.pm.ApplicationInfo;
 import android.os.AsyncTask;
+import android.os.Build;
 import android.os.Environment;
 import android.os.SystemClock;
+import android.system.Os;
 
 import org.chromium.base.annotations.CalledByNative;
 import org.chromium.base.annotations.MainDex;
@@ -24,6 +27,7 @@ import java.util.concurrent.atomic.AtomicBoolean;
  */
 @MainDex
 public abstract class PathUtils {
+    private static final String TAG = "PathUtils";
     private static final String THUMBNAIL_DIRECTORY_NAME = "textures";
 
     private static final int DATA_DIRECTORY = 0;
@@ -83,6 +87,18 @@ public abstract class PathUtils {
         return null;
     }
 
+    @SuppressLint("NewApi")
+    private static void chmod(String path, int mode) {
+        // Both Os.chmod and ErrnoException require SDK >= 21. But while Dalvik on < 21 tolerates
+        // Os.chmod, it throws VerifyError for ErrnoException, so catch Exception instead.
+        if (Build.VERSION.SDK_INT < Build.VERSION_CODES.LOLLIPOP) return;
+        try {
+            Os.chmod(path, mode);
+        } catch (Exception e) {
+            Log.e(TAG, "Failed to set permissions for path \"" + path + "\"");
+        }
+    }
+
     /**
      * Fetch the path of the directory where private data is to be stored by the application. This
      * is meant to be called in an AsyncTask in setPrivateDataDirectorySuffix(), but if we need the
@@ -96,6 +112,8 @@ public abstract class PathUtils {
         Context appContext = ContextUtils.getApplicationContext();
         paths[DATA_DIRECTORY] = appContext.getDir(
                 sDataDirectorySuffix, Context.MODE_PRIVATE).getPath();
+        // MODE_PRIVATE results in rwxrwx--x, but we want rwx------, as a defence-in-depth measure.
+        chmod(paths[DATA_DIRECTORY], 0700);
         paths[THUMBNAIL_DIRECTORY] = appContext.getDir(
                 THUMBNAIL_DIRECTORY_NAME, Context.MODE_PRIVATE).getPath();
         if (appContext.getCacheDir() != null) {

chrome/app/android/chrome_main_delegate_android.cc

@@ -59,31 +59,29 @@ void ChromeMainDelegateAndroid::SandboxInitialized(
   ChromeMainDelegate::SandboxInitialized(process_type);
 }
 
+void ChromeMainDelegateAndroid::SecureDataDirectory() {
+  // By default, Android creates the directory accessible by others.
+  // We'd like to tighten security and make it accessible only by
+  // the browser process.
+  // TODO(crbug.com/832388): Remove this once minsdk >= 21,
+  // at which point this will be handled by PathUtils.java.
+  base::FilePath data_path;
+  bool ok = PathService::Get(base::DIR_ANDROID_APP_DATA, &data_path);
+  if (ok) {
+    ok = base::SetPosixFilePermissions(data_path,
+                                       base::FILE_PERMISSION_USER_MASK);
+  }
+  if (!ok) {
+    LOG(ERROR) << "Failed to set data directory permissions";
+  }
+}
+
 int ChromeMainDelegateAndroid::RunProcess(
     const std::string& process_type,
     const content::MainFunctionParams& main_function_params) {
   TRACE_EVENT0("startup", "ChromeMainDelegateAndroid::RunProcess")
   if (process_type.empty()) {
-    // By default, Android creates the directory accessible by others.
-    // We'd like to tighten security and make it accessible only by
-    // the browser process.
-    base::FilePath data_path;
-    bool ok = PathService::Get(base::DIR_ANDROID_APP_DATA, &data_path);
-    if (ok) {
-      int permissions;
-      ok = base::GetPosixFilePermissions(data_path, &permissions);
-      if (ok) {
-        permissions &= base::FILE_PERMISSION_USER_MASK;
-      } else {
-        permissions = base::FILE_PERMISSION_READ_BY_USER |
-                      base::FILE_PERMISSION_WRITE_BY_USER |
-                      base::FILE_PERMISSION_EXECUTE_BY_USER;
-      }
-
-      ok = base::SetPosixFilePermissions(data_path, permissions);
-    }
-    if (!ok)
-      LOG(ERROR) << "Failed to set permission of " << data_path.value();
+    SecureDataDirectory();
 
     // Because the browser process can be started asynchronously as a series of
     // UI thread tasks a second request to start it can come in while the

chrome/app/android/chrome_main_delegate_android.h

@@ -19,6 +19,7 @@ class SafeBrowsingApiHandler;
 class ChromeMainDelegateAndroid : public ChromeMainDelegate {
  public:
   static ChromeMainDelegateAndroid* Create();
+  static void SecureDataDirectory();  // visible for testing
 
  protected:
   ChromeMainDelegateAndroid();

chrome/app/android/chrome_main_delegate_android_unittest.cc

+// 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.
+
+#include "chrome/app/android/chrome_main_delegate_android.h"
+
+#include "base/base_paths_android.h"
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "base/files/scoped_temp_dir.h"
+#include "base/test/scoped_path_override.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+class ChromeMainDelegateAndroidTest : public testing::Test {
+ protected:
+  ChromeMainDelegateAndroidTest() {}
+  ~ChromeMainDelegateAndroidTest() override {}
+
+  void SetUp() override {
+    ASSERT_TRUE(mock_data_dir_.CreateUniqueTempDir());
+    path_override_.reset(new base::ScopedPathOverride(
+        base::DIR_ANDROID_APP_DATA, mock_data_dir_.GetPath()));
+  }
+
+  base::FilePath dataDir() const { return mock_data_dir_.GetPath(); }
+
+ private:
+  base::ScopedTempDir mock_data_dir_;
+  std::unique_ptr<base::ScopedPathOverride> path_override_;
+};
+
+TEST_F(ChromeMainDelegateAndroidTest, VerifyDataDirPermissions) {
+  EXPECT_TRUE(base::SetPosixFilePermissions(dataDir(), 0777));
+  ChromeMainDelegateAndroid::SecureDataDirectory();
+  int new_mode;
+  EXPECT_TRUE(base::GetPosixFilePermissions(dataDir(), &new_mode));
+  EXPECT_EQ(new_mode, 0700);
+}

chrome/test/BUILD.gn

@@ -5004,29 +5004,31 @@ if (!is_android) {
       ]
     }
   }
+}
 
-  # TODO(609855): Make this compile on Android and run on the bots.
-  test("chrome_app_unittests") {
-    sources = [
-      "../app/chrome_watcher_client_unittest_win.cc",
-      "../app/chrome_watcher_client_win.cc",
-      "../app/chrome_watcher_command_line_win.cc",
-      "../app/chrome_watcher_command_line_win_unittest.cc",
-      "../app/resources/resources_unittest.cc",
-    ]
-    deps = [
-      ":test_support",
-      "//base/test:run_all_unittests",
-      "//base/test:test_support",
-      "//chrome/browser",
-      "//chrome/child",
-      "//components/crash/core/common",
-      "//components/flags_ui:switches",
-    ]
-    if (!is_fuchsia) {
-      # TODO(crbug.com/753619): Enable crash reporting on Fuchsia.
-      deps += [ "//third_party/breakpad:client" ]
-    }
+test("chrome_app_unittests") {
+  sources = [
+    "../app/android/chrome_main_delegate_android.cc",
+    "../app/android/chrome_main_delegate_android.h",
+    "../app/android/chrome_main_delegate_android_unittest.cc",
+    "../app/chrome_watcher_client_unittest_win.cc",
+    "../app/chrome_watcher_client_win.cc",
+    "../app/chrome_watcher_command_line_win.cc",
+    "../app/chrome_watcher_command_line_win_unittest.cc",
+    "../app/resources/resources_unittest.cc",
+  ]
+  deps = [
+    ":test_support",
+    "//base/test:run_all_unittests",
+    "//base/test:test_support",
+    "//chrome/browser",
+    "//chrome/child",
+    "//components/crash/core/common",
+    "//components/flags_ui:switches",
+  ]
+  if (!is_fuchsia) {
+    # TODO(crbug.com/753619): Enable crash reporting on Fuchsia.
+    deps += [ "//third_party/breakpad:client" ]
   }
 }