CrOS: Add local password complexity check function

Bug: b:290917006, b:291808449
Change-Id: I5f4fb75f93f2da2cb77170ce8f14658577263559
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4798392
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Commit-Queue: Martin Bidlingmaier <mbid@google.com>
Reviewed-by: Elie Maamari <emaamari@google.com>
Cr-Commit-Position: refs/heads/main@{#1192963}

chrome/browser/ash/login/auth_factor_config_browsertest.cc

@@ -0,0 +1,139 @@
+// Copyright 2023 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/memory/raw_ptr.h"
+#include "base/test/test_future.h"
+#include "base/values.h"
+#include "chrome/browser/ash/login/quick_unlock/quick_unlock_factory.h"
+#include "chrome/browser/ash/login/quick_unlock/quick_unlock_storage.h"
+#include "chrome/browser/ash/login/test/cryptohome_mixin.h"
+#include "chrome/browser/ash/login/test/logged_in_user_mixin.h"
+#include "chrome/browser/extensions/api/quick_unlock_private/quick_unlock_private_ash_utils.h"
+#include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/common/extensions/api/quick_unlock_private.h"
+#include "chrome/test/base/mixin_based_in_process_browser_test.h"
+#include "chromeos/ash/components/login/auth/public/authentication_error.h"
+#include "chromeos/ash/services/auth_factor_config/in_process_instances.h"
+#include "content/public/test/browser_test.h"
+#include "third_party/abseil-cpp/absl/types/optional.h"
+
+namespace ash::auth {
+
+namespace {
+
+enum class PasswordType {
+  kGaia,
+  kLocal,
+};
+
+// The initial password set up by the test fixture.
+const char kPassword[] = "the-password";
+
+}  // namespace
+
+using extensions::api::quick_unlock_private::TokenInfo;
+
+class AuthFactorConfigTestBase : public MixinBasedInProcessBrowserTest {
+ public:
+  explicit AuthFactorConfigTestBase(PasswordType password_type) {
+    cryptohome_.set_enable_auth_check(true);
+    cryptohome_.MarkUserAsExisting(logged_in_user_mixin_.GetAccountId());
+
+    switch (password_type) {
+      case PasswordType::kGaia:
+        cryptohome_.AddGaiaPassword(logged_in_user_mixin_.GetAccountId(),
+                                    kPassword);
+        break;
+      case PasswordType::kLocal:
+        cryptohome_.AddLocalPassword(logged_in_user_mixin_.GetAccountId(),
+                                     kPassword);
+        break;
+    }
+  }
+
+  void SetUpOnMainThread() override {
+    MixinBasedInProcessBrowserTest::SetUpOnMainThread();
+
+    logged_in_user_mixin_.LogInUser();
+  }
+
+  // Create a new auth token. Returns nullopt if something went wrong, probably
+  // because the provided password is incorrect.
+  absl::optional<std::string> MakeAuthToken(const std::string password) {
+    Profile* profile = ProfileManager::GetPrimaryUserProfile();
+    CHECK(profile);
+    extensions::QuickUnlockPrivateGetAuthTokenHelper token_helper(profile,
+                                                                  password);
+    base::test::TestFuture<absl::optional<TokenInfo>,
+                           absl::optional<AuthenticationError>>
+        result;
+    token_helper.Run(result.GetCallback());
+    if (result.Get<0>().has_value()) {
+      return result.Get<0>()->token;
+    }
+
+    CHECK(result.Get<1>().has_value());
+    return absl::nullopt;
+  }
+
+ protected:
+  CryptohomeMixin cryptohome_{&mixin_host_};
+  LoggedInUserMixin logged_in_user_mixin_{
+      &mixin_host_, LoggedInUserMixin::LogInType::kRegular,
+      embedded_test_server(), this};
+};
+
+class AuthFactorConfigTestWithLocalPassword : public AuthFactorConfigTestBase {
+ public:
+  AuthFactorConfigTestWithLocalPassword()
+      : AuthFactorConfigTestBase(PasswordType::kLocal) {}
+};
+
+// Checks that PasswordFactorEditor::SetLocalPassword can be used to set a new
+// password. This test is mostly here to make sure that the test fixture works
+// as intended.
+IN_PROC_BROWSER_TEST_F(AuthFactorConfigTestWithLocalPassword,
+                       SetLocalPasswordSuccess) {
+  static const std::string kGoodPassword = "asdfas∆f";
+
+  absl::optional<std::string> auth_token = MakeAuthToken(kPassword);
+  ASSERT_TRUE(auth_token.has_value());
+  mojom::PasswordFactorEditor& password_editor =
+      GetPasswordFactorEditor(quick_unlock::QuickUnlockFactory::GetDelegate());
+
+  base::test::TestFuture<mojom::ConfigureResult> result;
+  password_editor.SetLocalPassword(*auth_token, kGoodPassword,
+                                   result.GetCallback());
+
+  ASSERT_EQ(result.Get(), mojom::ConfigureResult::kSuccess);
+  // Since MakeAuthToken authenticates using the provided password, this will
+  // check that the new password works:
+  auth_token = MakeAuthToken(kGoodPassword);
+  ASSERT_TRUE(auth_token.has_value());
+}
+
+// Checks that PasswordFactorEditor::SetLocalPassword rejects insufficiently
+// complex passwords.
+IN_PROC_BROWSER_TEST_F(AuthFactorConfigTestWithLocalPassword,
+                       SetLocalPasswordComplexityFailure) {
+  static const std::string kBadPassword = "asdfas∆";
+
+  absl::optional<std::string> auth_token = MakeAuthToken(kPassword);
+  ASSERT_TRUE(auth_token.has_value());
+  mojom::PasswordFactorEditor& password_editor =
+      GetPasswordFactorEditor(quick_unlock::QuickUnlockFactory::GetDelegate());
+
+  base::test::TestFuture<mojom::ConfigureResult> result;
+  password_editor.SetLocalPassword(*auth_token, kBadPassword,
+                                   result.GetCallback());
+
+  ASSERT_EQ(result.Get(), mojom::ConfigureResult::kFatalError);
+  // Since MakeAuthToken authenticates using the provided password, this will
+  // check that the bad password really hasn't been set:
+  auth_token = MakeAuthToken(kBadPassword);
+  ASSERT_TRUE(!auth_token.has_value());
+}
+
+}  // namespace ash::auth

chrome/test/BUILD.gn

@@ -4152,6 +4152,7 @@ if (!is_android) {
         "../browser/ash/login/app_mode/test/web_kiosk_lacros_base_test.h",
         "../browser/ash/login/app_mode/test/web_kiosk_lacros_browsertest.cc",
         "../browser/ash/login/ash_hud_login_browsertest.cc",
+        "../browser/ash/login/auth_factor_config_browsertest.cc",
         "../browser/ash/login/challenge_response_auth_keys_loader_browsertest.cc",
         "../browser/ash/login/configuration_based_oobe_browsertest.cc",
         "../browser/ash/login/crash_restore_browsertest.cc",

chromeos/ash/services/auth_factor_config/in_process_instances.cc

@@ -82,4 +82,9 @@ void BindToPasswordFactorEditor(
   GetPasswordFactorEditorImpl(storage).BindReceiver(std::move(receiver));
 }
 
+mojom::PasswordFactorEditor& GetPasswordFactorEditor(
+    QuickUnlockStorageDelegate& delegate) {
+  return GetPasswordFactorEditorImpl(delegate);
+}
+
 }  // namespace ash::auth

chromeos/ash/services/auth_factor_config/in_process_instances.h

@@ -40,6 +40,8 @@ void BindToPinFactorEditor(
 void BindToPasswordFactorEditor(
     mojo::PendingReceiver<mojom::PasswordFactorEditor> receiver,
     QuickUnlockStorageDelegate&);
+mojom::PasswordFactorEditor& GetPasswordFactorEditor(
+    QuickUnlockStorageDelegate& delegate);
 
 }  // namespace ash::auth
 

chromeos/ash/services/auth_factor_config/password_factor_editor.cc

@@ -8,6 +8,7 @@
 
 #include "ash/constants/ash_features.h"
 #include "base/functional/callback.h"
+#include "base/strings/utf_string_conversion_utils.h"
 #include "chromeos/ash/components/cryptohome/auth_factor.h"
 #include "chromeos/ash/components/login/auth/public/cryptohome_key_constants.h"
 #include "chromeos/ash/components/login/auth/public/user_context.h"
@@ -20,6 +21,30 @@
 
 namespace ash::auth {
 
+namespace {
+
+const std::size_t kLocalPasswordMinimumLength = 8;
+
+// The synchronous implementation of `CheckLocalPasswordComplexity`. The
+// provided `password` string must be valid UTF-8.
+mojom::PasswordComplexity CheckLocalPasswordComplexityImpl(
+    const std::string& password) {
+  // We're counting unicode points here because we already have a function for
+  // that, but graphemes might be closer to the user's understanding of what
+  // the length of a string is.
+  absl::optional<size_t> unicode_size =
+      base::CountUnicodeCharacters(password.data(), password.size());
+  CHECK(unicode_size.has_value());
+
+  const mojom::PasswordComplexity complexity =
+      *unicode_size < kLocalPasswordMinimumLength
+          ? mojom::PasswordComplexity::kTooShort
+          : mojom::PasswordComplexity::kOk;
+  return complexity;
+}
+
+}  // namespace
+
 PasswordFactorEditor::PasswordFactorEditor(AuthFactorConfig* auth_factor_config,
                                            QuickUnlockStorageDelegate* storage)
     : auth_factor_config_(auth_factor_config),
@@ -35,6 +60,14 @@ void PasswordFactorEditor::SetLocalPassword(
     const std::string& auth_token,
     const std::string& new_password,
     base::OnceCallback<void(mojom::ConfigureResult)> callback) {
+  // Mojo strings are valid UTF-8, so the `CheckLocalPasswordComplexityImpl`
+  // call is OK.
+  if (CheckLocalPasswordComplexityImpl(new_password) !=
+      mojom::PasswordComplexity::kOk) {
+    std::move(callback).Run(mojom::ConfigureResult::kFatalError);
+    return;
+  }
+
   std::unique_ptr<UserContext> user_context;
 
   if (ash::features::ShouldUseAuthSessionStorage()) {
@@ -92,6 +125,14 @@ void PasswordFactorEditor::SetLocalPassword(
                      auth_token));
 }
 
+void PasswordFactorEditor::CheckLocalPasswordComplexity(
+    const std::string& password,
+    base::OnceCallback<void(mojom::PasswordComplexity)> callback) {
+  // Mojo strings are valid UTF-8, so the `CheckLocalPasswordComplexityImpl`
+  // call is OK.
+  std::move(callback).Run(CheckLocalPasswordComplexityImpl(password));
+}
+
 void PasswordFactorEditor::BindReceiver(
     mojo::PendingReceiver<mojom::PasswordFactorEditor> receiver) {
   receivers_.Add(this, std::move(receiver));

chromeos/ash/services/auth_factor_config/password_factor_editor.h

@@ -32,6 +32,10 @@ class PasswordFactorEditor : public mojom::PasswordFactorEditor {
       const std::string& new_password,
       base::OnceCallback<void(mojom::ConfigureResult)> callback) override;
 
+  void CheckLocalPasswordComplexity(
+      const std::string& password,
+      base::OnceCallback<void(mojom::PasswordComplexity)> callback) override;
+
   void BindReceiver(
       mojo::PendingReceiver<mojom::PasswordFactorEditor> receiver);
 

chromeos/ash/services/auth_factor_config/public/mojom/auth_factor_config.mojom

@@ -106,6 +106,12 @@ interface PinFactorEditor {
   RemovePin(string auth_token) => (ConfigureResult result);
 };
 
+// A value describing the complexity of a password.
+enum PasswordComplexity {
+  kOk,
+  kTooShort,
+};
+
 // Interface for methods specific to password authentication. Served from chrome
 // ash, intended to be consumed by the following trusted webuis:
 //
@@ -117,12 +123,19 @@ interface PinFactorEditor {
 // authentication factors.
 interface PasswordFactorEditor {
   // Sets the user's local password to the desired value. This will overwrite
-  // the user's current password, if any.
+  // the user's current password, if any. Callers must provide a sufficiently
+  // complex password. A password is considered sufficiently complex if the
+  // `CheckLocalPasswordComplexity` call returns `kOk` for the password.
+  //
   // TODO(b/290916811): For now, callers of this method must ensure that a local
   // password is already configured for that user, i.e. `GetPasswordFactorType`
   // must return `kLocal`. Eventually, calling this method should also be
   // possible when the user has a Gaia password (which will then be removed and
   // replaced by the provided local password) or no password at all.
   SetLocalPassword(string auth_token, string new_password) =>
       (ConfigureResult result);
+
+  // Returns a value describing the complexity of the provided password value.
+  CheckLocalPasswordComplexity(string password) =>
+      (PasswordComplexity complexity);
 };