Fix multiple bugs in EncryptedPassword

* openssl_decrypt() expects the encrypted string you give it to be the
  exact one that came out of openssl_encrypt(), it doesn't expect you to
  pre-decode the base64 encoding. So don't do that.
* Use the same IV when re-encrypting the underlying hash for comparison.
* Check the return value of OpenSSL functions, and report meaningful
  error messages, for sysadmin convenience and to avoid e.g. giving all
  users the same hash if an invalid cipher method was chosen (which was
  the previous behaviour).
* Fix EncryptedPassword::update(). Tested it with eval.php since there
  doesn't seem to be any callers.

Change-Id: I3a39de152d0329f93d16aa4ed43faf08f665b8e2

diff --git a/includes/password/EncryptedPassword.php b/includes/password/EncryptedPassword.php
index 7b3d9f9..0ea3c63 100644 (file)
--- a/includes/password/EncryptedPassword.php
+++ b/includes/password/EncryptedPassword.php
@@ -41,20 +41,33 @@ class EncryptedPassword extends ParameterizedPassword {
        public function crypt( $password ) {
                $secret = $this->config['secrets'][$this->params['secret']];
 
+               // Clear error string
+               while ( openssl_error_string() !== false );
+
                if ( $this->hash ) {
-                       $underlyingPassword = $this->factory->newFromCiphertext( openssl_decrypt(
-                                       base64_decode( $this->hash ), $this->params['cipher'],
-                                       $secret, 0, base64_decode( $this->args[0] )
-                               ) );
+                       $decrypted = openssl_decrypt(
+                                       $this->hash, $this->params['cipher'],
+                                       $secret, 0, base64_decode( $this->args[0] ) );
+                       if ( $decrypted === false ) {
+                               throw new PasswordError( 'Error decrypting password: ' . openssl_error_string() );
+                       }
+                       $underlyingPassword = $this->factory->newFromCiphertext( $decrypted );
                } else {
                        $underlyingPassword = $this->factory->newFromType( $this->config['underlying'] );
                }
 
                $underlyingPassword->crypt( $password );
-               $iv = MWCryptRand::generate( openssl_cipher_iv_length( $this->params['cipher'] ), true );
+               if ( count( $this->args ) ) {
+                       $iv = base64_decode( $this->args[0] );
+               } else {
+                       $iv = MWCryptRand::generate( openssl_cipher_iv_length( $this->params['cipher'] ), true );
+               }
 
                $this->hash = openssl_encrypt(
                        $underlyingPassword->toString(), $this->params['cipher'], $secret, 0, $iv );
+               if ( $this->hash === false ) {
+                       throw new PasswordError( 'Error encrypting password: ' . openssl_error_string() );
+               }
                $this->args = [ base64_encode( $iv ) ];
        }
 
@@ -65,32 +78,42 @@ class EncryptedPassword extends ParameterizedPassword {
         * @return bool True if the password was updated
         */
        public function update() {
-               if ( count( $this->args ) != 2 || $this->params == $this->getDefaultParams() ) {
+               if ( count( $this->args ) != 1 || $this->params == $this->getDefaultParams() ) {
                        // Hash does not need updating
                        return false;
                }
 
+               // Clear error string
+               while ( openssl_error_string() !== false );
+
                // Decrypt the underlying hash
                $underlyingHash = openssl_decrypt(
-                       base64_decode( $this->args[1] ),
+                       $this->hash,
                        $this->params['cipher'],
                        $this->config['secrets'][$this->params['secret']],
                        0,
                        base64_decode( $this->args[0] )
                );
+               if ( $underlyingHash === false ) {
+                       throw new PasswordError( 'Error decrypting password: ' . openssl_error_string() );
+               }
 
                // Reset the params
                $this->params = $this->getDefaultParams();
 
                // Check the key size with the new params
                $iv = MWCryptRand::generate( openssl_cipher_iv_length( $this->params['cipher'] ), true );
-               $this->hash = base64_encode( openssl_encrypt(
+               $this->hash = openssl_encrypt(
                                $underlyingHash,
                                $this->params['cipher'],
                                $this->config['secrets'][$this->params['secret']],
                                0,
                                $iv
-                       ) );
+                       );
+               if ( $this->hash === false ) {
+                       throw new PasswordError( 'Error encrypting password: ' . openssl_error_string() );
+               }
+
                $this->args = [ base64_encode( $iv ) ];
 
                return true;