Skip to content

Commit d122d9e

Browse files
author
epriestley
committed
Allow users to recover from a missing password hasher
Summary: Fixes T5934. If you hash a password with, e.g., bcrypt, and then lose the bcrypt hasher for some reason, we currently fatal when trying to figure out if we can upgrade. Instead, detect that the current hasher implementation has vanished and let the user reset their password (for account passwords) or choose a new one (for VCS passwords)> Test Plan: Account password: - Artifically disabled bcrypt hasher. - Viewed password panel, saw warnings about missing hasher. - Used password reset workflow to change password, saw iterated MD5 hashed password get set. - Enabled bcrypt hasher again. - Saw upgrade warning. - Upgraded password to bcrypt. VCS password: - Artificially disabled bcrypt hasher. - Viewed password panel, saw warnings about missing hasher. - Reset password. - Saw iterated md5 password. - Reenabled bcrypt. - Upgraded to bcrypt. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T5934 Differential Revision: https://secure.phabricator.com/D10325
1 parent 241cfc2 commit d122d9e

File tree

3 files changed

+54
-9
lines changed

3 files changed

+54
-9
lines changed

src/applications/diffusion/panel/DiffusionSetPasswordPanel.php

Lines changed: 29 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -74,16 +74,23 @@ public function processRequest(AphrontRequest $request) {
7474
if (!$errors) {
7575
$envelope = new PhutilOpaqueEnvelope($new_password);
7676

77+
try {
78+
// NOTE: This test is against $viewer (not $user), so that the error
79+
// message below makes sense in the case that the two are different,
80+
// and because an admin reusing their own password is bad, while
81+
// system agents generally do not have passwords anyway.
82+
83+
$same_password = $viewer->comparePassword($envelope);
84+
} catch (PhabricatorPasswordHasherUnavailableException $ex) {
85+
// If we're missing the hasher, just let the user continue.
86+
$same_password = false;
87+
}
88+
7789
if ($new_password !== $confirm) {
7890
$e_password = pht('Does Not Match');
7991
$e_confirm = pht('Does Not Match');
8092
$errors[] = pht('Password and confirmation do not match.');
81-
} else if ($viewer->comparePassword($envelope)) {
82-
// NOTE: The above test is against $viewer (not $user), so that the
83-
// error message below makes sense in the case that the two are
84-
// different, and because an admin reusing their own password is bad,
85-
// while system agents generally do not have passwords anyway.
86-
93+
} else if ($same_password) {
8794
$e_password = pht('Not Unique');
8895
$e_confirm = pht('Not Unique');
8996
$errors[] = pht(
@@ -197,7 +204,22 @@ public function processRequest(AphrontRequest $request) {
197204
->setValue(PhabricatorPasswordHasher::getBestAlgorithmName()));
198205

199206
if (strlen($hash_envelope->openEnvelope())) {
200-
if (PhabricatorPasswordHasher::canUpgradeHash($hash_envelope)) {
207+
try {
208+
$can_upgrade = PhabricatorPasswordHasher::canUpgradeHash(
209+
$hash_envelope);
210+
} catch (PhabricatorPasswordHasherUnavailableException $ex) {
211+
$can_upgrade = false;
212+
$errors[] = pht(
213+
'Your VCS password is currently hashed using an algorithm which is '.
214+
'no longer available on this install.');
215+
$errors[] = pht(
216+
'Because the algorithm implementation is missing, your password '.
217+
'can not be used.');
218+
$errors[] = pht(
219+
'You can set a new password to replace the old password.');
220+
}
221+
222+
if ($can_upgrade) {
201223
$errors[] = pht(
202224
'The strength of your stored VCS password hash can be upgraded. '.
203225
'To upgrade, either: use the password to authenticate with a '.

src/applications/settings/panel/PhabricatorSettingsPanelPassword.php

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,28 @@ public function processRequest(AphrontRequest $request) {
126126

127127
$hash_envelope = new PhutilOpaqueEnvelope($user->getPasswordHash());
128128
if (strlen($hash_envelope->openEnvelope())) {
129-
if (PhabricatorPasswordHasher::canUpgradeHash($hash_envelope)) {
129+
try {
130+
$can_upgrade = PhabricatorPasswordHasher::canUpgradeHash(
131+
$hash_envelope);
132+
} catch (PhabricatorPasswordHasherUnavailableException $ex) {
133+
$can_upgrade = false;
134+
135+
// Only show this stuff if we aren't on the reset workflow. We can
136+
// do resets regardless of the old hasher's availability.
137+
if (!$token) {
138+
$errors[] = pht(
139+
'Your password is currently hashed using an algorithm which is '.
140+
'no longer available on this install.');
141+
$errors[] = pht(
142+
'Because the algorithm implementation is missing, your password '.
143+
'can not be used or updated.');
144+
$errors[] = pht(
145+
'To set a new password, request a password reset link from the '.
146+
'login screen and then follow the instructions.');
147+
}
148+
}
149+
150+
if ($can_upgrade) {
130151
$errors[] = pht(
131152
'The strength of your stored password hash can be upgraded. '.
132153
'To upgrade, either: log out and log in using your password; or '.

src/infrastructure/util/password/PhabricatorPasswordHasher.php

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -407,7 +407,9 @@ public static function getCurrentAlgorithmName(PhutilOpaqueEnvelope $hash) {
407407
$current_hasher = PhabricatorPasswordHasher::getHasherForHash($hash);
408408
return $current_hasher->getHumanReadableName();
409409
} catch (Exception $ex) {
410-
return pht('Unknown');
410+
$info = self::parseHashFromStorage($hash);
411+
$name = $info['name'];
412+
return pht('Unknown ("%s")', $name);
411413
}
412414
}
413415

0 commit comments

Comments
 (0)