[Win] Add option to reauthenticate the OS user before revealing passwords.

chrome/browser/password_manager/password_manager_util_win.cc

+// Copyright (c) 2013 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 <Ntsecapi.h>

[cpu_(ooo_6.6-7.5), 2013/10/22 21:10:06]

it seems we don't capitalize the files that we #include, so to remain
consistent, don't.

[Will Harris, 2013/10/23 14:57:18]

Done.

+#include <WinCred.h>
+#include <Windows.h>
+
+// SECURITY_WIN32 must be defined in order to get
+// EXTENDED_NAME_FORMAT enumeration.
+#define SECURITY_WIN32 1
+
+#define PASSWORD_MANAGER_MAX_PASSWORD_TRIES 3
+
+#include <Security.h>
+

[cpu_(ooo_6.6-7.5), 2013/10/22 21:10:06]

possibly undefine SECURITY_WIN32 here?

[Will Harris, 2013/10/23 14:57:18]

Done.

+#include "base/strings/utf_string_conversions.h"
+#include "grit/chromium_strings.h"
+#include "ui/base/l10n/l10n_util.h"
+
+namespace password_manager_util {
+
+bool AuthenticateUser() {

[cpu_(ooo_6.6-7.5), 2013/10/22 21:10:06]

can you point me to the caller of this function?

[Will Harris, 2013/10/23 14:57:18]

in CL 28713002 filename password_manager_handler.cc:

https://codereview.chromium.org/28713002/diff/590001/chrome/browser/ui/webui/...

+  bool retval = false;
+  CREDUI_INFO cui = {};
+  WCHAR pszUserName[CREDUI_MAX_USERNAME_LENGTH+1] = {};
+  WCHAR pszDisplayName[CREDUI_MAX_USERNAME_LENGTH+1] = {};
+  WCHAR pszPwd[CREDUI_MAX_PASSWORD_LENGTH+1] = {};
+  DWORD usernameLen = CREDUI_MAX_USERNAME_LENGTH;
+  std::wstring product_name =
+    UTF16ToWide(l10n_util::GetStringUTF16(IDS_PRODUCT_NAME));

[cpu_(ooo_6.6-7.5), 2013/10/22 21:10:06]

indenting is off.

[Will Harris, 2013/10/23 14:57:18]

Done.

+  std::wstring password_prompt =
+    UTF16ToWide(l10n_util::GetStringUTF16(
+                  IDS_PASSWORDS_PAGE_AUTHENTICATION_PROMPT));
+  HANDLE hToken = INVALID_HANDLE_VALUE;

[cpu_(ooo_6.6-7.5), 2013/10/22 21:10:06]

here and elsewhere, do not name variables Microsoft style. Check our style
guide.

[Will Harris, 2013/10/23 14:57:18]

Done.

+  int tries = 0;
+  bool useDisplayName = false;
+  bool useUpnName = false;
+  DWORD logonErr = 0;
+
+  // On a domain, we obtain the User Principle Name

[cpu_(ooo_6.6-7.5), 2013/10/22 21:10:06]

/s/Principal

[Will Harris, 2013/10/23 14:57:18]

Done.

+  // for domain authentication.
+  if (GetUserNameEx(NameUserPrincipal, pszUserName, &usernameLen)) {
+    useUpnName = true;
+  } else {
+    usernameLen = CREDUI_MAX_USERNAME_LENGTH;
+    // Otherwise, we're a workstation, use the plain local username.
+    if (!GetUserName(pszUserName, &usernameLen)) {
+      DLOG(ERROR) << "Unable to obtain username " << GetLastError();
+      return false;
+    } else {
+      // as we are on a workstation, it's possible the user
+      // has no password, so check here
+      logonErr = LogonUser(pszUserName,
+                           L".",
+                           L"",
+                           LOGON32_LOGON_INTERACTIVE,

[cpu_(ooo_6.6-7.5), 2013/10/22 21:10:06]

see my comment of line 114

[Will Harris, 2013/10/23 14:57:18]

Done.

+                           LOGON32_PROVIDER_DEFAULT,
+                           &hToken);
+      // ERROR_ACCOUNT_RESTRICTION means the password is blank
+      // and Windows XP and above return ERROR_ACCOUNT_RESTRICTION
+      // see http://support.microsoft.com/kb/303846
+      if (logonErr || GetLastError() == ERROR_ACCOUNT_RESTRICTION) {
+        SecureZeroMemory(pszUserName, sizeof(pszUserName));
+        CloseHandle(hToken);

[cpu_(ooo_6.6-7.5), 2013/10/22 21:10:06]

My reading indicates that you are closing INVALID_HANDLE_VALUE here.

[Will Harris, 2013/10/23 14:57:18]

Done.

+        return true;
+      }
+    }
+  }
+
+  // Try and obtain a friendly display name.
+  usernameLen = CREDUI_MAX_USERNAME_LENGTH;
+  if (GetUserNameEx(NameDisplay, pszDisplayName, &usernameLen)) {
+    useDisplayName = true;
+  }
+
+  cui.cbSize = sizeof(CREDUI_INFO);
+  cui.hwndParent = NULL;
+  cui.pszMessageText = password_prompt.c_str();
+  cui.pszCaptionText = product_name.c_str();
+
+  cui.hbmBanner = NULL;
+  BOOL fSave = FALSE;
+  DWORD credErr = NO_ERROR;
+
+  do {
+    SecureZeroMemory(pszPwd, sizeof(pszPwd));
+    tries++;
+
+    // TODO(wfh) make sure we support smart cards here

[cpu_(ooo_6.6-7.5), 2013/10/22 21:10:06]

here and elsewhere, make sure comments end with a period.

[Will Harris, 2013/10/23 14:57:18]

Done.

+    credErr = CredUIPromptForCredentials(
+                &cui,
+                product_name.c_str(),
+                NULL,
+                0,
+                useDisplayName ? pszDisplayName : pszUserName,
+                CREDUI_MAX_USERNAME_LENGTH+1,
+                pszPwd,
+                CREDUI_MAX_PASSWORD_LENGTH+1,
+                &fSave,
+                CREDUI_FLAGS_GENERIC_CREDENTIALS |
+                  CREDUI_FLAGS_EXCLUDE_CERTIFICATES |
+                  CREDUI_FLAGS_KEEP_USERNAME |
+                  CREDUI_FLAGS_ALWAYS_SHOW_UI |
+                  CREDUI_FLAGS_DO_NOT_PERSIST |
+                  (tries > 1 ? CREDUI_FLAGS_INCORRECT_PASSWORD : 0));
+
+    if (credErr == NO_ERROR) {
+      usernameLen = CREDUI_MAX_USERNAME_LENGTH;
+
+      logonErr = LogonUser(pszUserName,
+                           useUpnName ? NULL : L".",
+                           pszPwd,
+                           LOGON32_LOGON_INTERACTIVE,

[cpu_(ooo_6.6-7.5), 2013/10/22 21:10:06]

this is the expensive one, try using the cheap ones which I don't remember which
one is, maybe LOGON32_LOGON_BATCH or or LOGON32_LOGON_NETWORK

[Will Harris, 2013/10/23 14:57:18]

changed to LOGON32_LOGON_NETWORK

+                           LOGON32_PROVIDER_DEFAULT,
+                           &hToken);
+      if (logonErr) {
+          retval = true;
+        CloseHandle(hToken);
+      } else {
+        if ( GetLastError() == ERROR_ACCOUNT_RESTRICTION &&

[cpu_(ooo_6.6-7.5), 2013/10/22 21:10:06]

No spaces between ( and the following expression.

[Will Harris, 2013/10/23 14:57:18]

Done.

+             wcslen(pszPwd) == 0 ) {
+          // Password is blank, so permit.
+          retval = true;
+        } else {
+          DLOG(WARNING) << "Unable to authenticate " << GetLastError();
+        }
+      }
+    }
+  } while (credErr == NO_ERROR &&
+           (retval == false && tries < PASSWORD_MANAGER_MAX_PASSWORD_TRIES));

[cpu_(ooo_6.6-7.5), 2013/10/22 21:10:06]

I don't agree with this loop of tries, I think it is very cheap to press the
'show my passwords' button again. Unless is several clicks/gestures away.

It also makes it expensive for people that click the button by accident.

[Will Harris, 2013/10/23 14:57:18]

as discussed, this is okay because the user can break out of the loop at any
time
by hitting the cancel button on the authentication dialog

+
+  SecureZeroMemory(pszDisplayName, sizeof(pszDisplayName));
+  SecureZeroMemory(pszUserName, sizeof(pszUserName));
+  SecureZeroMemory(pszPwd, sizeof(pszPwd));
+
+  return retval;
+}
+
+}  // namespace password_manager_util