Merge pull request #31499 from nextcloud/bugfix/empty-secret

Add fallback routines for empty secret cases
nextcloud · Oct 17, 2022 · 9919116 · 9919116
2 parents 44d2eb8 + ef31396
commit 9919116
Show file tree

Hide file tree

Showing 5 changed files with 70 additions and 12 deletions.
diff --git a/lib/private/Authentication/Token/PublicKeyTokenProvider.php b/lib/private/Authentication/Token/PublicKeyTokenProvider.php
@@ -111,8 +111,14 @@ public function getToken(string $tokenId): IToken {
 				$token = $this->mapper->getToken($this->hashToken($tokenId));
 				$this->cache[$token->getToken()] = $token;
 			} catch (DoesNotExistException $ex) {
-				$this->cache[$tokenHash] = $ex;
-				throw new InvalidTokenException("Token does not exist: " . $ex->getMessage(), 0, $ex);
+				try {
+					$token = $this->mapper->getToken($this->hashTokenWithEmptySecret($tokenId));
+					$this->cache[$token->getToken()] = $token;
+					$this->rotate($token, $tokenId, $tokenId);
+				} catch (DoesNotExistException $ex2) {
+					$this->cache[$tokenHash] = $ex2;
+					throw new InvalidTokenException("Token does not exist: " . $ex->getMessage(), 0, $ex);
+				}
 			}
 		}
 
@@ -189,6 +195,7 @@ public function invalidateToken(string $token) {
 		$this->cache->clear();
 
 		$this->mapper->invalidate($this->hashToken($token));
+		$this->mapper->invalidate($this->hashTokenWithEmptySecret($token));
 	}
 
 	public function invalidateTokenById(string $uid, int $id) {
@@ -305,9 +312,14 @@ private function decrypt(string $cipherText, string $token): string {
 		try {
 			return $this->crypto->decrypt($cipherText, $token . $secret);
 		} catch (\Exception $ex) {
-			// Delete the invalid token
-			$this->invalidateToken($token);
-			throw new InvalidTokenException("Could not decrypt token password: " . $ex->getMessage(), 0, $ex);
+			// Retry with empty secret as a fallback for instances where the secret might not have been set by accident
+			try {
+				return $this->crypto->decrypt($cipherText, $token);
+			} catch (\Exception $ex2) {
+				// Delete the invalid token
+				$this->invalidateToken($token);
+				throw new InvalidTokenException("Could not decrypt token password: " . $ex->getMessage(), 0, $ex2);
+			}
 		}
 	}
 
@@ -330,6 +342,13 @@ private function hashToken(string $token): string {
 		return hash('sha512', $token . $secret);
 	}
 
+	/**
+	 * @deprecated Fallback for instances where the secret might not have been set by accident
+	 */
+	private function hashTokenWithEmptySecret(string $token): string {
+		return hash('sha512', $token);
+	}
+
 	/**
 	 * @throws \RuntimeException when OpenSSL reports a problem
 	 */

diff --git a/lib/private/Security/Crypto.php b/lib/private/Security/Crypto.php
@@ -122,9 +122,22 @@ public function encrypt(string $plaintext, string $password = ''): string {
 	 * @throws Exception If the decryption failed
 	 */
 	public function decrypt(string $authenticatedCiphertext, string $password = ''): string {
-		if ($password === '') {
-			$password = $this->config->getSystemValue('secret');
+		$secret = $this->config->getSystemValue('secret');
+		try {
+			if ($password === '') {
+				return $this->decryptWithoutSecret($authenticatedCiphertext, $secret);
+			}
+			return $this->decryptWithoutSecret($authenticatedCiphertext, $password);
+		} catch (Exception $e) {
+			if ($password === '') {
+				// Retry with empty secret as a fallback for instances where the secret might not have been set by accident
+				return $this->decryptWithoutSecret($authenticatedCiphertext, '');
+			}
+			throw $e;
 		}
+	}
+
+	private function decryptWithoutSecret(string $authenticatedCiphertext, string $password = ''): string {
 		$hmacKey = $encryptionKey = $password;
 
 		$parts = explode('|', $authenticatedCiphertext);

diff --git a/lib/private/Security/Hasher.php b/lib/private/Security/Hasher.php
@@ -137,6 +137,15 @@ protected function legacyHashVerify($message, $hash, &$newHash = null): bool {
 			return true;
 		}
 
+		// Verify whether it matches a legacy PHPass or SHA1 string
+		// Retry with empty passwordsalt for cases where it was not set
+		$hashLength = \strlen($hash);
+		if (($hashLength === 60 && password_verify($message, $hash)) ||
+			($hashLength === 40 && hash_equals($hash, sha1($message)))) {
+			$newHash = $this->hash($message);
+			return true;
+		}
+
 		return false;
 	}
 

diff --git a/lib/private/Security/VerificationToken/VerificationToken.php b/lib/private/Security/VerificationToken/VerificationToken.php
@@ -84,10 +84,15 @@ public function check(string $token, ?IUser $user, string $subject, string $pass
 		try {
 			$decryptedToken = $this->crypto->decrypt($encryptedToken, $passwordPrefix.$this->config->getSystemValue('secret'));
 		} catch (\Exception $e) {
-			$this->throwInvalidTokenException(InvalidTokenException::TOKEN_DECRYPTION_ERROR);
+			// Retry with empty secret as a fallback for instances where the secret might not have been set by accident
+			try {
+				$decryptedToken = $this->crypto->decrypt($encryptedToken, $passwordPrefix);
+			} catch (\Exception $e2) {
+				$this->throwInvalidTokenException(InvalidTokenException::TOKEN_DECRYPTION_ERROR);
+			}
 		}
 
-		$splitToken = explode(':', $decryptedToken ?? '');
+		$splitToken = explode(':', $decryptedToken);
 		if (count($splitToken) !== 2) {
 			$this->throwInvalidTokenException(InvalidTokenException::TOKEN_INVALID_FORMAT);
 		}

diff --git a/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php b/tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php
@@ -296,9 +296,12 @@ public function testSetPasswordInvalidToken() {
 	}
 
 	public function testInvalidateToken() {
-		$this->mapper->expects($this->once())
+		$this->mapper->expects($this->at(0))
 			->method('invalidate')
 			->with(hash('sha512', 'token7'.'1f4h9s'));
+		$this->mapper->expects($this->at(1))
+			->method('invalidate')
+			->with(hash('sha512', 'token7'));
 
 		$this->tokenProvider->invalidateToken('token7');
 	}
@@ -429,13 +432,22 @@ public function testGetToken(): void {
 	public function testGetInvalidToken() {
 		$this->expectException(InvalidTokenException::class);
 
-		$this->mapper->method('getToken')
+		$this->mapper->expects($this->at(0))
+			->method('getToken')
 			->with(
-				$this->callback(function (string $token) {
+				$this->callback(function (string $token): bool {
 					return hash('sha512', 'unhashedToken'.'1f4h9s') === $token;
 				})
 			)->willThrowException(new DoesNotExistException('nope'));
 
+		$this->mapper->expects($this->at(1))
+			->method('getToken')
+			->with(
+				$this->callback(function (string $token): bool {
+					return hash('sha512', 'unhashedToken') === $token;
+				})
+			)->willThrowException(new DoesNotExistException('nope'));
+
 		$this->tokenProvider->getToken('unhashedToken');
 	}