feat(security): simplify decrypt error handling

Before this patch, when decrypting a value without using a password,
it would call `decryptWithoutSecret` with the system `secret` as
`password`. When this fails, it would retry with an empty string as
`password`.

This has the practical disadvantage that it can lead to confusing
error messages. For example, when using the TOTP app, when the system
`secret` is misconfigured, the first invocation will throw a sensible
`HMAC does not match.` error, but then it is retried and the retry
throws a `Hash_hkdf(): Argument #2 ($key) cannot be empty` error
causing confusion (e.g.
https://help.nextcloud.com/t/hash-hkdf-argument-2-key-cannot-be-empty/192556).

Of course this fallback to using an empty string is likely part of
some sort of graceful migration from the days when the secret could
be empty (e.g. #34012,
#31499).

However, taking a wider perspective, such 'fallback logic' in
security-critical areas makes things more complex, which is a risk.
It's not quite the same scenario, but Heartbleed does come to mind.

For this reason, rather than a 'surgical' improvement for the particular
case encountered above (increasing complexity further), I think it'd be
worth to start considering removing this fallback entirely
(perhaps in v32.0.0?) - hence this conversation-starter PR.

Signed-off-by: Arnout Engelen <[email protected]>

lib/private/Security/Crypto.php

@@ -91,18 +91,10 @@ public function encrypt(string $plaintext, string $password = ''): string {
 	 */
 	public function decrypt(string $authenticatedCiphertext, string $password = ''): string {
 		$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;
+		if ($password === '') {
+			return $this->decryptWithoutSecret($authenticatedCiphertext, $secret);
 		}
+		return $this->decryptWithoutSecret($authenticatedCiphertext, $password);
 	}
 
 	private function decryptWithoutSecret(string $authenticatedCiphertext, string $password = ''): string {