From dd70c59465056d513072a35d0fc5d40764484100 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 17 Jul 2012 12:06:33 -0700 Subject: [PATCH] Use OpaqueEnvelopes for all passwords in Phabricator Summary: See D2991 / T1526. Two major changes here: - PHP just straight-up logs passwords on ldap_bind() failures. Suppress that with "@" and keep them out of DarkConsole by enabling discard mode. - Use PhutilOpaqueEnvelope whenever we send a password into a call stack. Test Plan: - Created a new account. - Reset password. - Changed password. - Logged in with valid password. - Tried to login with bad password. - Changed password via accountadmin. - Hit various LDAP errors and made sure nothing appears in the logs. Reviewers: vrana, btrahan Reviewed By: vrana CC: aran Differential Revision: https://secure.phabricator.com/D2993 --- scripts/user/account_admin.php | 3 +- .../errorlog/DarkConsoleErrorLogPluginAPI.php | 4 +++ .../PhabricatorLDAPLoginController.php | 5 ++-- .../controller/PhabricatorLoginController.php | 5 +++- .../auth/ldap/PhabricatorLDAPProvider.php | 23 +++++++++------ .../people/PhabricatorUserEditor.php | 7 +++-- ...torUserPasswordSettingsPanelController.php | 6 ++-- .../people/storage/PhabricatorUser.php | 28 +++++++++---------- 8 files changed, 50 insertions(+), 31 deletions(-) diff --git a/scripts/user/account_admin.php b/scripts/user/account_admin.php index ad403cf9e9..d0ae23287b 100755 --- a/scripts/user/account_admin.php +++ b/scripts/user/account_admin.php @@ -166,7 +166,8 @@ $user->openTransaction(); $editor->makeAdminUser($user, $set_admin); if ($changed_pass !== false) { - $editor->changePassword($user, $changed_pass); + $envelope = new PhutilOpaqueEnvelope($changed_pass); + $editor->changePassword($user, $envelope); } $user->saveTransaction(); diff --git a/src/aphront/console/plugin/errorlog/DarkConsoleErrorLogPluginAPI.php b/src/aphront/console/plugin/errorlog/DarkConsoleErrorLogPluginAPI.php index 98d1801596..142accdc84 100644 --- a/src/aphront/console/plugin/errorlog/DarkConsoleErrorLogPluginAPI.php +++ b/src/aphront/console/plugin/errorlog/DarkConsoleErrorLogPluginAPI.php @@ -29,6 +29,10 @@ final class DarkConsoleErrorLogPluginAPI { self::$discardMode = true; } + public static function disableDiscardMode() { + self::$discardMode = false; + } + public static function getErrors() { return self::$errors; } diff --git a/src/applications/auth/controller/PhabricatorLDAPLoginController.php b/src/applications/auth/controller/PhabricatorLDAPLoginController.php index 0525625373..524f52ef13 100644 --- a/src/applications/auth/controller/PhabricatorLDAPLoginController.php +++ b/src/applications/auth/controller/PhabricatorLDAPLoginController.php @@ -37,9 +37,8 @@ final class PhabricatorLDAPLoginController extends PhabricatorAuthController { if ($request->isFormPost()) { try { - $this->provider->auth($request->getStr('username'), - $request->getStr('password')); - + $envelope = new PhutilOpaqueEnvelope($request->getStr('password')); + $this->provider->auth($request->getStr('username'), $envelope); } catch (Exception $e) { $errors[] = $e->getMessage(); } diff --git a/src/applications/auth/controller/PhabricatorLoginController.php b/src/applications/auth/controller/PhabricatorLoginController.php index c48b287233..2af18468dc 100644 --- a/src/applications/auth/controller/PhabricatorLoginController.php +++ b/src/applications/auth/controller/PhabricatorLoginController.php @@ -119,7 +119,10 @@ final class PhabricatorLoginController if (!$errors) { // Perform username/password tests only if we didn't get rate limited // by the CAPTCHA. - if (!$user || !$user->comparePassword($request->getStr('password'))) { + + $envelope = new PhutilOpaqueEnvelope($request->getStr('password')); + + if (!$user || !$user->comparePassword($envelope)) { $errors[] = 'Bad username/password.'; } } diff --git a/src/applications/auth/ldap/PhabricatorLDAPProvider.php b/src/applications/auth/ldap/PhabricatorLDAPProvider.php index d73659c5fb..166b312a5d 100644 --- a/src/applications/auth/ldap/PhabricatorLDAPProvider.php +++ b/src/applications/auth/ldap/PhabricatorLDAPProvider.php @@ -53,7 +53,7 @@ final class PhabricatorLDAPProvider { public function retrieveUserEmail() { return $this->userData['mail'][0]; } - + public function retrieveUserRealName() { return $this->retrieveUserRealNameFromData($this->userData); } @@ -106,9 +106,9 @@ final class PhabricatorLDAPProvider { return $this->userData; } - public function auth($username, $password) { - if (strlen(trim($username)) == 0 || strlen(trim($password)) == 0) { - throw new Exception('Username and/or password can not be empty'); + public function auth($username, PhutilOpaqueEnvelope $password) { + if (strlen(trim($username)) == 0) { + throw new Exception('Username can not be empty'); } $activeDirectoryDomain = @@ -121,10 +121,17 @@ final class PhabricatorLDAPProvider { $this->getBaseDN(); } - $result = ldap_bind($this->getConnection(), $dn, $password); + $conn = $this->getConnection(); + + // NOTE: It is very important we suppress any messages that occur here, + // because it logs passwords if it reaches an error log of any sort. + DarkConsoleErrorLogPluginAPI::enableDiscardMode(); + $result = @ldap_bind($conn, $dn, $password->openEnvelope()); + DarkConsoleErrorLogPluginAPI::disableDiscardMode(); if (!$result) { - throw new Exception('Bad username/password.'); + throw new Exception( + "LDAP Error #".ldap_errno($conn).": ".ldap_error($conn)); } $this->userData = $this->getUser($username); @@ -184,14 +191,14 @@ final class PhabricatorLDAPProvider { $row = array(); $entry = $entries[$i]; - // Get username, email and realname + // Get username, email and realname $username = $entry[$this->getSearchAttribute()][0]; if(empty($username)) { continue; } $row[] = $username; $row[] = $entry['mail'][0]; - $row[] = $this->retrieveUserRealNameFromData($entry); + $row[] = $this->retrieveUserRealNameFromData($entry); $rows[] = $row; diff --git a/src/applications/people/PhabricatorUserEditor.php b/src/applications/people/PhabricatorUserEditor.php index 0752a4a937..1ad29c0919 100644 --- a/src/applications/people/PhabricatorUserEditor.php +++ b/src/applications/people/PhabricatorUserEditor.php @@ -127,7 +127,10 @@ final class PhabricatorUserEditor { /** * @task edit */ - public function changePassword(PhabricatorUser $user, $password) { + public function changePassword( + PhabricatorUser $user, + PhutilOpaqueEnvelope $envelope) { + if (!$user->getID()) { throw new Exception("User has not been created yet!"); } @@ -135,7 +138,7 @@ final class PhabricatorUserEditor { $user->openTransaction(); $user->reload(); - $user->setPassword($password); + $user->setPassword($envelope); $user->save(); $log = PhabricatorUserLog::newLog( diff --git a/src/applications/people/controller/settings/panels/PhabricatorUserPasswordSettingsPanelController.php b/src/applications/people/controller/settings/panels/PhabricatorUserPasswordSettingsPanelController.php index 3f2ca1021a..af2e8d9b93 100644 --- a/src/applications/people/controller/settings/panels/PhabricatorUserPasswordSettingsPanelController.php +++ b/src/applications/people/controller/settings/panels/PhabricatorUserPasswordSettingsPanelController.php @@ -59,7 +59,8 @@ final class PhabricatorUserPasswordSettingsPanelController $errors = array(); if ($request->isFormPost()) { if (!$valid_token) { - if (!$user->comparePassword($request->getStr('old_pw'))) { + $envelope = new PhutilOpaqueEnvelope($request->getStr('old_pw')); + if (!$user->comparePassword($envelope)) { $errors[] = 'The old password you entered is incorrect.'; $e_old = 'Invalid'; } @@ -85,9 +86,10 @@ final class PhabricatorUserPasswordSettingsPanelController // is changed here the CSRF token check will fail. $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + $envelope = new PhutilOpaqueEnvelope($pass); id(new PhabricatorUserEditor()) ->setActor($user) - ->changePassword($user, $pass); + ->changePassword($user, $envelope); unset($unguarded); diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php index 8786196b89..71383fc17f 100644 --- a/src/applications/people/storage/PhabricatorUser.php +++ b/src/applications/people/storage/PhabricatorUser.php @@ -74,18 +74,18 @@ final class PhabricatorUser extends PhabricatorUserDAO implements PhutilPerson { PhabricatorPHIDConstants::PHID_TYPE_USER); } - public function setPassword($password) { + public function setPassword(PhutilOpaqueEnvelope $envelope) { if (!$this->getPHID()) { throw new Exception( "You can not set a password for an unsaved user because their PHID ". "is a salt component in the password hash."); } - if (!strlen($password)) { + if (!strlen($envelope->openEnvelope())) { $this->setPasswordHash(''); } else { $this->setPasswordSalt(md5(mt_rand())); - $hash = $this->hashPassword($password); + $hash = $this->hashPassword($envelope); $this->setPasswordHash($hash); } return $this; @@ -129,26 +129,26 @@ final class PhabricatorUser extends PhabricatorUserDAO implements PhutilPerson { return Filesystem::readRandomCharacters(255); } - public function comparePassword($password) { - if (!strlen($password)) { + public function comparePassword(PhutilOpaqueEnvelope $envelope) { + if (!strlen($envelope->openEnvelope())) { return false; } if (!strlen($this->getPasswordHash())) { return false; } - $password = $this->hashPassword($password); - return ($password === $this->getPasswordHash()); + $password_hash = $this->hashPassword($envelope); + return ($password_hash === $this->getPasswordHash()); } - private function hashPassword($password) { - $password = $this->getUsername(). - $password. - $this->getPHID(). - $this->getPasswordSalt(); + private function hashPassword(PhutilOpaqueEnvelope $envelope) { + $hash = $this->getUsername(). + $envelope->openEnvelope(). + $this->getPHID(). + $this->getPasswordSalt(); for ($ii = 0; $ii < 1000; $ii++) { - $password = md5($password); + $hash = md5($hash); } - return $password; + return $hash; } const CSRF_CYCLE_FREQUENCY = 3600;