Obfuscate account ID key for drivefs.

This key is PII, so to avoid exposing it through mount point name we
hash it with salt.

BUG=chromium:829694

Change-Id: I5c66ff64715026b7acc984c387b41760a2fb99b1
Reviewed-on: https://chromium-review.googlesource.com/1143119Reviewed-by: Sasha Morrissey <sashab@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Commit-Queue: Sergei Datsenko <dats@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577762}

chrome/browser/chromeos/drive/drive_integration_service.cc

@@ -11,9 +11,11 @@
 #include "base/files/file_util.h"
 #include "base/logging.h"
 #include "base/macros.h"
+#include "base/md5.h"
 #include "base/strings/stringprintf.h"
 #include "base/sys_info.h"
 #include "base/task_scheduler/post_task.h"
+#include "base/unguessable_token.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/chromeos/drive/debug_info_collector.h"
@@ -306,6 +308,11 @@ class DriveIntegrationService::DriveFsHolder
         ->GetAccountId();
   }
 
+  std::string GetObfuscatedAccountId() override {
+    return base::MD5String(GetProfileSalt() + "-" +
+                           GetAccountId().GetAccountIdKey());
+  }
+
   void OnMountFailed(base::Optional<base::TimeDelta> remount_delay) override {
     on_drivefs_unmounted_.Run(std::move(remount_delay));
   }
@@ -318,6 +325,19 @@ class DriveIntegrationService::DriveFsHolder
     on_drivefs_unmounted_.Run(std::move(remount_delay));
   }
 
+  const std::string& GetProfileSalt() {
+    if (!profile_salt_.empty()) {
+      return profile_salt_;
+    }
+    PrefService* prefs = profile_->GetPrefs();
+    profile_salt_ = prefs->GetString(prefs::kDriveFsProfileSalt);
+    if (profile_salt_.empty()) {
+      profile_salt_ = base::UnguessableToken::Create().ToString();
+      prefs->SetString(prefs::kDriveFsProfileSalt, profile_salt_);
+    }
+    return profile_salt_;
+  }
+
   std::unique_ptr<drivefs::DriveFsHost::MojoConnectionDelegate>
   CreateMojoConnectionDelegate() override {
     if (test_drivefs_mojo_connection_delegate_factory_)
@@ -337,6 +357,8 @@ class DriveIntegrationService::DriveFsHolder
 
   drivefs::DriveFsHost drivefs_host_;
 
+  std::string profile_salt_;
+
   DISALLOW_COPY_AND_ASSIGN(DriveFsHolder);
 };
 

chrome/browser/chromeos/file_manager/file_manager_browsertest_base.cc

@@ -42,6 +42,7 @@
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/fake_cros_disks_client.h"
 #include "components/drive/chromeos/file_system_interface.h"
+#include "components/drive/drive_pref_names.h"
 #include "components/drive/service/fake_drive_service.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_service.h"
@@ -376,6 +377,8 @@ class TestVolume {
   DISALLOW_COPY_AND_ASSIGN(TestVolume);
 };
 
+constexpr char kPredefinedProfileSalt[] = "salt";
+
 }  // anonymous namespace
 
 // LocalTestVolume: test volume for a local drive.
@@ -811,9 +814,11 @@ class DriveFsTestVolume : public DriveTestVolume {
           auto* user =
               chromeos::ProfileHelper::Get()->GetUserByProfile(profile);
           if (!user)
-            return AccountId();
+            return std::string();
 
-          return user->GetAccountId();
+          return base::MD5String(
+              kPredefinedProfileSalt +
+              ("-" + user->GetAccountId().GetAccountIdKey()));
         },
         profile_));
   }
@@ -1262,6 +1267,8 @@ void FileManagerBrowserTestBase::OnCommand(const std::string& name,
 drive::DriveIntegrationService*
 FileManagerBrowserTestBase::CreateDriveIntegrationService(Profile* profile) {
   if (base::FeatureList::IsEnabled(chromeos::features::kDriveFs)) {
+    profile->GetPrefs()->SetString(drive::prefs::kDriveFsProfileSalt,
+                                   kPredefinedProfileSalt);
     drive_volumes_[profile->GetOriginalProfile()] =
         std::make_unique<DriveFsTestVolume>(profile->GetOriginalProfile());
   } else {

chrome/browser/chromeos/preferences.cc

@@ -308,6 +308,7 @@ void Preferences::RegisterProfilePrefs(
   registry->RegisterBooleanPref(
       drive::prefs::kDisableDriveHostedFiles, false,
       user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
+  registry->RegisterStringPref(drive::prefs::kDriveFsProfileSalt, "");
   // We don't sync prefs::kLanguageCurrentInputMethod and PreviousInputMethod
   // because they're just used to track the logout state of the device.
   registry->RegisterStringPref(prefs::kLanguageCurrentInputMethod, "");

chromeos/components/drivefs/drivefs_host.cc

@@ -78,9 +78,8 @@ class DriveFsHost::MountState : public mojom::DriveFsDelegate,
         binding_(this) {
     source_path_ = base::StrCat({kMountScheme, pending_token_.ToString()});
     std::string datadir_option = base::StrCat(
-        {"datadir=",
-         host_->profile_path_.Append(kDataPath)
-             .Append(host_->delegate_->GetAccountId().GetAccountIdKey())
+        {"datadir=", host_->profile_path_.Append(kDataPath)
+                         .Append(host_->delegate_->GetObfuscatedAccountId())
                          .value()});
     auto bootstrap =
         mojo::MakeProxy(mojo_connection_delegate_->InitializeMojoConnection());
@@ -98,8 +97,7 @@ class DriveFsHost::MountState : public mojom::DriveFsDelegate,
 
     chromeos::disks::DiskMountManager::GetInstance()->MountPath(
         source_path_, "",
-        base::StrCat(
-            {"drivefs-", host_->delegate_->GetAccountId().GetAccountIdKey()}),
+        base::StrCat({"drivefs-", host_->delegate_->GetObfuscatedAccountId()}),
         {datadir_option}, chromeos::MOUNT_TYPE_NETWORK_STORAGE,
         chromeos::MOUNT_ACCESS_MODE_READ_WRITE);
   }

chromeos/components/drivefs/drivefs_host.h

@@ -59,6 +59,7 @@ class COMPONENT_EXPORT(DRIVEFS) DriveFsHost
     virtual net::URLRequestContextGetter* GetRequestContext() = 0;
     virtual service_manager::Connector* GetConnector() = 0;
     virtual const AccountId& GetAccountId() = 0;
+    virtual std::string GetObfuscatedAccountId() = 0;
     virtual std::unique_ptr<OAuth2MintTokenFlow> CreateMintTokenFlow(
         OAuth2MintTokenFlow::Delegate* delegate,
         const std::string& client_id,

chromeos/components/drivefs/drivefs_host_unittest.cc

@@ -129,6 +129,9 @@ class TestingDriveFsHostDelegate : public DriveFsHost::Delegate {
     return connector_.get();
   }
   const AccountId& GetAccountId() override { return account_id_; }
+  std::string GetObfuscatedAccountId() override {
+    return "salt-" + account_id_.GetAccountIdKey();
+  }
 
   std::unique_ptr<OAuth2MintTokenFlow> CreateMintTokenFlow(
       OAuth2MintTokenFlow::Delegate* delegate,
@@ -270,8 +273,9 @@ class DriveFsHostTest : public ::testing::Test, public mojom::DriveFsBootstrap {
     std::string source;
     EXPECT_CALL(
         *disk_manager_,
-        MountPath(testing::StartsWith("drivefs://"), "", "drivefs-g-ID",
-                  testing::Contains("datadir=/path/to/profile/GCache/v2/g-ID"),
+        MountPath(
+            testing::StartsWith("drivefs://"), "", "drivefs-salt-g-ID",
+            testing::Contains("datadir=/path/to/profile/GCache/v2/salt-g-ID"),
             _, chromeos::MOUNT_ACCESS_MODE_READ_WRITE))
         .WillOnce(testing::SaveArg<0>(&source));
 
@@ -290,7 +294,7 @@ class DriveFsHostTest : public ::testing::Test, public mojom::DriveFsBootstrap {
     DispatchMountEvent(chromeos::disks::DiskMountManager::MOUNTING,
                        chromeos::MOUNT_ERROR_NONE,
                        {base::StrCat({"drivefs://", token}),
-                        "/media/drivefsroot/g-ID",
+                        "/media/drivefsroot/salt-g-ID",
                         chromeos::MOUNT_TYPE_NETWORK_STORAGE,
                         {}});
   }
@@ -322,7 +326,7 @@ class DriveFsHostTest : public ::testing::Test, public mojom::DriveFsBootstrap {
     base::RunLoop run_loop;
     base::OnceClosure quit_closure = run_loop.QuitClosure();
     EXPECT_CALL(*host_delegate_,
-                OnMounted(base::FilePath("/media/drivefsroot/g-ID")))
+                OnMounted(base::FilePath("/media/drivefsroot/salt-g-ID")))
         .WillOnce(RunQuitClosure(&quit_closure));
     SendOnMounted();
     run_loop.Run();
@@ -361,7 +365,8 @@ TEST_F(DriveFsHostTest, Basic) {
 
   ASSERT_NO_FATAL_FAILURE(DoMount());
 
-  EXPECT_EQ(base::FilePath("/media/drivefsroot/g-ID"), host_->GetMountPath());
+  EXPECT_EQ(base::FilePath("/media/drivefsroot/salt-g-ID"),
+            host_->GetMountPath());
 }
 
 TEST_F(DriveFsHostTest, OnMountedBeforeMountEvent) {
@@ -376,12 +381,13 @@ TEST_F(DriveFsHostTest, OnMountedBeforeMountEvent) {
   EXPECT_FALSE(host_->IsMounted());
 
   EXPECT_CALL(*host_delegate_,
-              OnMounted(base::FilePath("/media/drivefsroot/g-ID")));
+              OnMounted(base::FilePath("/media/drivefsroot/salt-g-ID")));
 
   DispatchMountSuccessEvent(token);
 
   ASSERT_TRUE(host_->IsMounted());
-  EXPECT_EQ(base::FilePath("/media/drivefsroot/g-ID"), host_->GetMountPath());
+  EXPECT_EQ(base::FilePath("/media/drivefsroot/salt-g-ID"),
+            host_->GetMountPath());
 }
 
 TEST_F(DriveFsHostTest, OnMountFailedFromMojo) {
@@ -409,7 +415,7 @@ TEST_F(DriveFsHostTest, OnMountFailedFromDbus) {
   DispatchMountEvent(chromeos::disks::DiskMountManager::MOUNTING,
                      chromeos::MOUNT_ERROR_INVALID_MOUNT_OPTIONS,
                      {base::StrCat({"drivefs://", token}),
-                      "/media/drivefsroot/g-ID",
+                      "/media/drivefsroot/salt-g-ID",
                       chromeos::MOUNT_TYPE_NETWORK_STORAGE,
                       {}});
   run_loop.Run();
@@ -424,7 +430,7 @@ TEST_F(DriveFsHostTest, UnmountAfterMountComplete) {
 
   ASSERT_NO_FATAL_FAILURE(DoMount());
 
-  EXPECT_CALL(*disk_manager_, UnmountPath("/media/drivefsroot/g-ID",
+  EXPECT_CALL(*disk_manager_, UnmountPath("/media/drivefsroot/salt-g-ID",
                                           chromeos::UNMOUNT_OPTIONS_NONE, _));
   EXPECT_CALL(observer, OnUnmounted());
   base::RunLoop run_loop;
@@ -455,7 +461,7 @@ TEST_F(DriveFsHostTest, UnmountBeforeMojoConnection) {
   DispatchMountSuccessEvent(token);
 
   EXPECT_FALSE(host_->IsMounted());
-  EXPECT_CALL(*disk_manager_, UnmountPath("/media/drivefsroot/g-ID",
+  EXPECT_CALL(*disk_manager_, UnmountPath("/media/drivefsroot/salt-g-ID",
                                           chromeos::UNMOUNT_OPTIONS_NONE, _));
 
   host_->Unmount();
@@ -473,7 +479,7 @@ TEST_F(DriveFsHostTest, DestroyBeforeMountEvent) {
 TEST_F(DriveFsHostTest, DestroyBeforeMojoConnection) {
   auto token = StartMount();
   DispatchMountSuccessEvent(token);
-  EXPECT_CALL(*disk_manager_, UnmountPath("/media/drivefsroot/g-ID",
+  EXPECT_CALL(*disk_manager_, UnmountPath("/media/drivefsroot/salt-g-ID",
                                           chromeos::UNMOUNT_OPTIONS_NONE, _));
 
   host_.reset();
@@ -493,7 +499,7 @@ TEST_F(DriveFsHostTest, ObserveOtherMount) {
   DispatchMountEvent(chromeos::disks::DiskMountManager::UNMOUNTING,
                      chromeos::MOUNT_ERROR_NONE,
                      {base::StrCat({"drivefs://", token}),
-                      "/media/drivefsroot/g-ID",
+                      "/media/drivefsroot/salt-g-ID",
                       chromeos::MOUNT_TYPE_NETWORK_STORAGE,
                       {}});
   EXPECT_FALSE(host_->IsMounted());

chromeos/components/drivefs/fake_drivefs.cc

@@ -36,11 +36,11 @@ class FakeDriveFsMojoConnectionDelegate
   DISALLOW_COPY_AND_ASSIGN(FakeDriveFsMojoConnectionDelegate);
 };
 
-std::vector<std::pair<base::RepeatingCallback<AccountId()>,
+std::vector<std::pair<base::RepeatingCallback<std::string()>,
                       base::WeakPtr<FakeDriveFs>>>&
 GetRegisteredFakeDriveFsIntances() {
   static base::NoDestructor<std::vector<std::pair<
-      base::RepeatingCallback<AccountId()>, base::WeakPtr<FakeDriveFs>>>>
+      base::RepeatingCallback<std::string()>, base::WeakPtr<FakeDriveFs>>>>
       registered_fake_drivefs_instances;
   return *registered_fake_drivefs_instances;
 }
@@ -66,9 +66,9 @@ base::FilePath MaybeMountDriveFs(
   }
   CHECK(!datadir_suffix.empty());
   for (auto& registration : GetRegisteredFakeDriveFsIntances()) {
-    AccountId account_id = registration.first.Run();
-    if (registration.second && account_id.HasAccountIdKey() &&
-        account_id.GetAccountIdKey() == datadir_suffix) {
+    std::string account_id = registration.first.Run();
+    if (registration.second && !account_id.empty() &&
+        account_id == datadir_suffix) {
       return registration.second->mount_path();
     }
   }
@@ -97,7 +97,7 @@ FakeDriveFs::FakeDriveFs(const base::FilePath& mount_path)
 FakeDriveFs::~FakeDriveFs() = default;
 
 void FakeDriveFs::RegisterMountingForAccountId(
-    base::RepeatingCallback<AccountId()> account_id_getter) {
+    base::RepeatingCallback<std::string()> account_id_getter) {
   chromeos::DBusThreadManager* dbus_thread_manager =
       chromeos::DBusThreadManager::Get();
   static_cast<chromeos::FakeCrosDisksClient*>(

chromeos/components/drivefs/fake_drivefs.h

@@ -16,8 +16,6 @@
 #include "chromeos/components/drivefs/mojom/drivefs.mojom.h"
 #include "mojo/public/cpp/bindings/binding.h"
 
-class AccountId;
-
 namespace drivefs {
 
 class FakeDriveFs : public drivefs::mojom::DriveFs,
@@ -27,7 +25,7 @@ class FakeDriveFs : public drivefs::mojom::DriveFs,
   ~FakeDriveFs() override;
 
   void RegisterMountingForAccountId(
-      base::RepeatingCallback<AccountId()> account_id_getter);
+      base::RepeatingCallback<std::string()> account_id_getter);
 
   std::unique_ptr<drivefs::DriveFsHost::MojoConnectionDelegate>
   CreateConnectionDelegate();

components/drive/drive_pref_names.cc

@@ -19,5 +19,9 @@ const char kDisableDriveOverCellular[] = "gdata.cellular.disabled";
 // The pref prefix should remain as "gdata" for backward compatibility.
 const char kDisableDriveHostedFiles[] = "gdata.hosted_files.disabled";
 
+// A string pref containing a random salt used to obfuscate account IDs
+// when passed to drivefs.
+const char kDriveFsProfileSalt[] = "drivefs.profile_salt";
+
 }  // namespace prefs
 }  // namespace drive

components/drive/drive_pref_names.h

@@ -13,6 +13,7 @@ namespace prefs {
 extern const char kDisableDrive[];
 extern const char kDisableDriveOverCellular[];
 extern const char kDisableDriveHostedFiles[];
+extern const char kDriveFsProfileSalt[];
 
 }  // namespace prefs
 }  // namespace drive