Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[stable25] Add fallback routines for empty secret cases #35605

Merged
merged 5 commits into from
Dec 5, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 24 additions & 5 deletions lib/private/Authentication/Token/PublicKeyTokenProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$this->cache[$tokenHash] = $ex2;
$this->cache[$tokenHash] = $ex;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non-critical, ignoring for now for the approaching release

can be addressed afterwards

throw new InvalidTokenException("Token does not exist: " . $ex->getMessage(), 0, $ex);
}
}
}

Expand Down Expand Up @@ -198,6 +204,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) {
Expand Down Expand Up @@ -314,9 +321,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);
}
}
}

Expand All @@ -339,6 +351,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
*/
Expand Down
17 changes: 15 additions & 2 deletions lib/private/Security/Crypto.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
9 changes: 9 additions & 0 deletions lib/private/Security/Hasher.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
9 changes: 7 additions & 2 deletions lib/private/Security/VerificationToken/VerificationToken.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
18 changes: 15 additions & 3 deletions tests/lib/Authentication/Token/PublicKeyTokenProviderTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -310,9 +310,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');
}
Expand Down Expand Up @@ -443,13 +446,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');
}

Expand Down