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

@@ -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();

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;
   }

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'),
+        $envelope = new PhutilOpaqueEnvelope($request->getStr('password'));
-          $request->getStr('password'));
+        $this->provider->auth($request->getStr('username'), $envelope);
-
       } catch (Exception $e) {
         $errors[] = $e->getMessage();
       }

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.';
           }
         }

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) {
+  public function auth($username, PhutilOpaqueEnvelope $password) {
-    if (strlen(trim($username)) == 0 || strlen(trim($password)) == 0) {
+    if (strlen(trim($username)) == 0) {
-      throw new Exception('Username and/or password can not be empty');
+      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;

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(

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);
 

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) {
+  public function comparePassword(PhutilOpaqueEnvelope $envelope) {
-    if (!strlen($password)) {
+    if (!strlen($envelope->openEnvelope())) {
       return false;
     }
     if (!strlen($this->getPasswordHash())) {
       return false;
     }
-    $password = $this->hashPassword($password);
+    $password_hash = $this->hashPassword($envelope);
-    return ($password === $this->getPasswordHash());
+    return ($password_hash === $this->getPasswordHash());
   }
 
-  private function hashPassword($password) {
+  private function hashPassword(PhutilOpaqueEnvelope $envelope) {
-    $password = $this->getUsername().
+    $hash = $this->getUsername().
-                $password.
+            $envelope->openEnvelope().
-                $this->getPHID().
+            $this->getPHID().
-                $this->getPasswordSalt();
+            $this->getPasswordSalt();
     for ($ii = 0; $ii < 1000; $ii++) {
-      $password = md5($password);
+      $hash = md5($hash);
     }
-    return $password;
+    return $hash;
   }
 
   const CSRF_CYCLE_FREQUENCY  = 3600;