From: Tim Starling <tstarling@wikimedia.org>
Date: Mon, 14 Nov 2016 10:04:30 +0000 (+1100)
Subject: Fix multiple bugs in EncryptedPassword
X-Git-Tag: 1.31.0-rc.0~4718^2~1
X-Git-Url: http://git.cyclocoop.org/data/Fool?a=commitdiff_plain;h=6dbb8f2d787266078a803065c7483a8e7ca15d29;p=lhc%2Fweb%2Fwiklou.git

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 7b3d9f9cf7..0ea3c63198 100644
--- 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;