Block account password recovery on enterprise CRD session start

Bug: b:395838339
Change-Id: I6eba011f91d1416f1bbee7f2f05ae14788a12216
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6258262
Reviewed-by: Denis Kuznetsov <antrim@chromium.org>
Reviewed-by: Jeroen Dhollander <jeroendh@google.com>
Commit-Queue: Ashutosh Singhal <macinashutosh@google.com>
Cr-Commit-Position: refs/heads/main@{#1421407}

chrome/browser/ash/policy/remote_commands/crd/BUILD.gn

@@ -39,6 +39,7 @@ static_library("crd") {
     "//chrome/browser/ash/app_mode/isolated_web_app",
     "//chrome/browser/ash/policy/core",
     "//chrome/common:constants",
+    "//chromeos/ash/components/dbus/userdataauth:userdataauth",
     "//chromeos/ash/services/network_config:in_process_instance",
     "//components/policy/core/common",
     "//components/policy/proto:",

chrome/browser/ash/policy/remote_commands/crd/crd_admin_session_controller.cc

@@ -35,8 +35,13 @@
 #include "chrome/browser/ash/policy/remote_commands/crd/remote_activity_notification_controller.h"
 #include "chrome/browser/device_identity/device_oauth2_token_service_factory.h"
 #include "chrome/common/pref_names.h"
+#include "chromeos/ash/components/dbus/userdataauth/userdataauth_client.h"
+#include "chromeos/ash/components/login/auth/auth_factor_editor.h"
+#include "chromeos/ash/components/login/auth/public/authentication_error.h"
+#include "chromeos/ash/components/login/auth/public/user_context.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
+#include "components/user_manager/user_manager.h"
 #include "mojo/public/cpp/bindings/pending_receiver.h"
 #include "remoting/host/chromeos/features.h"
 #include "remoting/host/chromeos/remote_support_host_ash.h"
@@ -289,6 +294,49 @@ class IdleHostTtlChecker : public CrdSessionObserver {
   std::optional<base::OneShotTimer> terminate_timer_;
 };
 
+// Prevents the remote admin from triggering the Cryptohome account recovery.
+// Otherwise a malicious admin could use this to gain access to the
+// files of a local user's account, even without knowing the password.
+class CryptohomeAccountRecoveryDisabler : public CrdSessionObserver {
+ public:
+  explicit CryptohomeAccountRecoveryDisabler(
+      base::OnceClosure terminate_session_callback)
+      : terminate_session_callback_(std::move(terminate_session_callback)) {}
+
+  CryptohomeAccountRecoveryDisabler(const CryptohomeAccountRecoveryDisabler&) =
+      delete;
+  CryptohomeAccountRecoveryDisabler& operator=(
+      const CryptohomeAccountRecoveryDisabler&) = delete;
+  ~CryptohomeAccountRecoveryDisabler() override = default;
+
+  // `CrdSessionObserver` implementation:
+  void OnClientConnected() override { LockAccountRecoveryForRemoteAccess(); }
+
+ private:
+  void LockAccountRecoveryForRemoteAccess() {
+    auth_factor_editor_.LockCryptohomeRecoveryUntilReboot(base::BindOnce(
+        &CryptohomeAccountRecoveryDisabler::OnCryptohomeRecoveryLocked,
+        weak_factory_.GetWeakPtr()));
+  }
+
+  void OnCryptohomeRecoveryLocked(
+      std::optional<ash::AuthenticationError> error) {
+    // Normally, this error shouldn't occur, hence, the error handling is not
+    // done gracefully.
+    if (error.has_value()) {
+      LOG(ERROR) << "Terminating the CRD session because locking Cryptohome "
+                    "recovery failed due to error: "
+                 << error->get_resolved_failure();
+      std::move(terminate_session_callback_).Run();
+    }
+  }
+
+  ash::AuthFactorEditor auth_factor_editor_{ash::UserDataAuthClient::Get()};
+  base::OnceClosure terminate_session_callback_;
+
+  base::WeakPtrFactory<CryptohomeAccountRecoveryDisabler> weak_factory_{this};
+};
+
 remoting::mojom::SupportSessionParamsPtr GetSessionParameters(
     const SessionParameters& parameters,
     std::string_view oauth_token) {
@@ -705,6 +753,9 @@ CrdAdminSessionController::CreateCrdHostSession() {
 
   result->AddOwnedObserver(std::make_unique<IdleHostTtlChecker>(base::BindOnce(
       &CrdAdminSessionController::TerminateSession, base::Unretained(this))));
+  result->AddOwnedObserver(std::make_unique<CryptohomeAccountRecoveryDisabler>(
+      base::BindOnce(&CrdAdminSessionController::TerminateSession,
+                     base::Unretained(this))));
   result->AddObserver(this);
 
   if (base::FeatureList::IsEnabled(kEnableCrdAdminRemoteAccessV2)) {

chrome/browser/ash/policy/remote_commands/crd/crd_admin_session_controller_unittest.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/ash/policy/remote_commands/crd/crd_admin_session_controller.h"
 
+#include <memory>
 #include <optional>
 #include <string>
 #include <tuple>
@@ -28,6 +29,9 @@
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/scoped_testing_local_state.h"
 #include "chrome/test/base/testing_browser_process.h"
+#include "chromeos/ash/components/dbus/cryptohome/UserDataAuth.pb.h"
+#include "chromeos/ash/components/dbus/userdataauth/fake_userdataauth_client.h"
+#include "chromeos/ash/components/dbus/userdataauth/userdataauth_client.h"
 #include "chromeos/crosapi/mojom/remoting.mojom.h"
 #include "components/prefs/testing_pref_service.h"
 #include "components/session_manager/core/session_manager.h"
@@ -426,14 +430,38 @@ class CrdAdminSessionControllerTest : public ash::AshTestBase {
     session_finish_result_.Clear();
   }
 
+  void FinishLockAccountRecoveryRequestWithSuccess() {
+    ash::FakeUserDataAuthClient::TestApi::Get()->SetNextOperationError(
+        ash::FakeUserDataAuthClient::Operation::kLockFactorUntilReboot,
+        cryptohome::ErrorWrapper::CreateFromErrorCodeOnly(
+            ::user_data_auth::CRYPTOHOME_ERROR_NOT_SET));
+  }
+
+  void FinishLockAccountRecoveryRequestWithError() {
+    ash::FakeUserDataAuthClient::TestApi::Get()->SetNextOperationError(
+        ash::FakeUserDataAuthClient::Operation::kLockFactorUntilReboot,
+        cryptohome::ErrorWrapper::CreateFromErrorCodeOnly(
+            ::user_data_auth::CRYPTOHOME_ERROR_NOT_IMPLEMENTED));
+  }
+
+  bool AccountRecoveryOperationIsCalled() {
+    return ash::FakeUserDataAuthClient::Get()
+        ->WasCalled<
+            ash::FakeUserDataAuthClient::Operation::kLockFactorUntilReboot>();
+  }
+
  protected:
   void SetUp() override {
+    ash::UserDataAuthClient::InitializeFake();
+
     AshTestBase::SetUp();
     RecreateSessionController();
     session_controller().SetOAuthTokenForTesting("test-oauth-token");
   }
 
   void TearDown() override {
+    ash::UserDataAuthClient::Shutdown();
+
     session_controller_->Shutdown();
     AshTestBase::TearDown();
   }
@@ -939,6 +967,60 @@ TEST_F(CrdAdminSessionControllerTest, ShouldAcceptFastIncomingConnections) {
   ASSERT_TRUE(delegate().HasActiveSession());
 }
 
+TEST_F(CrdAdminSessionControllerTest,
+       ShouldNotBlockAccountRecoveryBeforeCrdHostSessionIsConnected) {
+  InitWithNoReconnectableSession(session_controller());
+  SupportHostObserver& observer = StartCrdHostAndBindObserver();
+
+  observer.OnHostStateReceivedAccessCode("access-code", base::Days(1));
+  observer.OnHostStateStarting();
+  observer.OnHostStateConnecting();
+  observer.OnHostStateDisconnected(std::nullopt);
+  observer.OnHostStateError(1);
+  observer.OnPolicyError();
+  observer.OnInvalidDomainError();
+  FlushForTesting(observer);
+
+  EXPECT_FALSE(AccountRecoveryOperationIsCalled());
+}
+
+TEST_F(CrdAdminSessionControllerTest,
+       ShouldBlockAccountRecoveryOnCrdHostSessionIsConnected) {
+  InitWithNoReconnectableSession(session_controller());
+  SupportHostObserver& observer = StartCrdHostAndBindObserver();
+
+  observer.OnHostStateConnected("remote-user");
+  FlushForTesting(observer);
+
+  EXPECT_TRUE(AccountRecoveryOperationIsCalled());
+}
+
+TEST_F(CrdAdminSessionControllerTest,
+       ShouldNotTerminateIfBlockingAccountRecoverySucceeds) {
+  FinishLockAccountRecoveryRequestWithSuccess();
+  InitWithNoReconnectableSession(session_controller());
+  SupportHostObserver& observer = StartCrdHostAndBindObserver();
+
+  observer.OnHostStateConnected("remote-user");
+  FlushForTesting(observer);
+
+  EXPECT_TRUE(AccountRecoveryOperationIsCalled());
+  ASSERT_TRUE(delegate().HasActiveSession());
+}
+
+TEST_F(CrdAdminSessionControllerTest,
+       ShouldTerminateSessionIfBlockingAccountRecoveryFails) {
+  FinishLockAccountRecoveryRequestWithError();
+  InitWithNoReconnectableSession(session_controller());
+  SupportHostObserver& observer = StartCrdHostAndBindObserver();
+
+  observer.OnHostStateConnected("remote-user");
+  FlushForTesting(observer);
+
+  EXPECT_TRUE(AccountRecoveryOperationIsCalled());
+  ASSERT_FALSE(delegate().HasActiveSession());
+}
+
 // Fixture for all tests related to reconnecting to an existing session.
 class CrdAdminSessionControllerReconnectTest
     : public CrdAdminSessionControllerTest {

chromeos/ash/components/dbus/userdataauth/fake_userdataauth_client.cc

@@ -1942,7 +1942,16 @@ void FakeUserDataAuthClient::SetUserDataStorageWriteEnabled(
 void FakeUserDataAuthClient::LockFactorUntilReboot(
     const ::user_data_auth::LockFactorUntilRebootRequest& request,
     LockFactorUntilRebootCallback callback) {
+  RememberRequest<Operation::kLockFactorUntilReboot>(request);
   ::user_data_auth::LockFactorUntilRebootReply reply;
+
+  if (auto error = TakeOperationError(Operation::kLockFactorUntilReboot);
+      cryptohome::HasError(error)) {
+    SetErrorWrapperToReply(reply, error);
+    std::move(callback).Run(reply);
+    return;
+  }
+
   std::move(callback).Run(std::move(reply));
 }