Review fixes: the derived key is wiped also when a key is damaged, each tidy-up step after a removal on its own

- The key derived from the passphrase is wiped on every path, also when the key or nonce turns out damaged
- After a removal the pending flag and the cached index are tidied up independently; a failing log cannot stop the removal event
- Tests: a short salt or nonce gives the damaged-key error

lib/Crypto/KeyRing.php

@@ -108,14 +108,21 @@ final class KeyRing {
 			|| $wrapped['mem'] < SODIUM_CRYPTO_PWHASH_MEMLIMIT_INTERACTIVE || $wrapped['mem'] > SODIUM_CRYPTO_PWHASH_MEMLIMIT_SENSITIVE) {
 			throw new CryptoException('Unsupported key derivation parameters (damaged or foreign key file)');
 		}
+		$kek = null;
 		try {
-			$kek = sodium_crypto_pwhash(SODIUM_CRYPTO_SECRETBOX_KEYBYTES, $passphrase, $b64($wrapped['salt']),
+			$salt = $b64($wrapped['salt']);
+			$sealed = $b64($wrapped['key']);
+			$nonce = $b64($wrapped['nonce']);
+			$kek = sodium_crypto_pwhash(SODIUM_CRYPTO_SECRETBOX_KEYBYTES, $passphrase, $salt,
 				$wrapped['ops'], $wrapped['mem'], SODIUM_CRYPTO_PWHASH_ALG_ARGON2ID13);
-			$master = sodium_crypto_secretbox_open($b64($wrapped['key']), $b64($wrapped['nonce']), $kek);
+			$master = sodium_crypto_secretbox_open($sealed, $nonce, $kek);
-			sodium_memzero($kek);
 		} catch (\SodiumException|\ValueError $e) {
 			// invalid base64 or a salt/nonce of the wrong length: damaged, so the caller shows its recovery options
 			throw new CryptoException('Damaged key file: ' . $e->getMessage(), 0, $e);
+		} finally {
+			if ($kek !== null) {
+				sodium_memzero($kek);
+			}
 		}
 		if ($master === false) {
 			throw new CryptoException('Wrong passphrase or damaged key');

lib/Service/TargetService.php

@@ -452,16 +452,27 @@ final class TargetService {
 		$this->mapper->delete($target);
 		// The location is gone now: tidying up must not fail the removal, or the caller would skip
 		// the removal event and a retry would no longer find the location
-		try {
+		$this->tidyUp($target, fn () => $this->keys->setWrappedKeyPending($target->getId(), false));
-			$this->keys->setWrappedKeyPending($target->getId(), false);
+		// The cached index of its repository is only of use while a location points at it
-			// The cached index of its repository is only of use while a location points at it
+		$this->tidyUp($target, function () use ($target): void {
 			$repositoryId = $target->getRepositoryId();
 			if ($repositoryId !== null && !array_filter($this->list(), fn (Target $t) => $t->getRepositoryId() === $repositoryId)) {
 				$this->indexCache->forget($repositoryId);
 			}
+		});
+	}
+
+	/** One tidy-up step after a removal: a failure is logged (if even that works) and never thrown. */
+	private function tidyUp(Target $target, callable $step): void {
+		try {
+			$step();
 		} catch (\Throwable $e) {
-			Server::get(\Psr\Log\LoggerInterface::class)->warning('NG Backup: location ' . $target->getName()
+			try {
-				. ' removed, tidying up after it failed: ' . $e->getMessage(), ['exception' => $e]);
+				Server::get(\Psr\Log\LoggerInterface::class)->warning('NG Backup: location ' . $target->getName()
+					. ' removed, tidying up after it failed: ' . $e->getMessage(), ['exception' => $e]);
+			} catch (\Throwable) {
+				// nothing left to tell it to
+			}
 		}
 	}
 

tests/Unit/Crypto/KeyRingTest.php

@@ -30,6 +30,19 @@ class KeyRingTest extends TestCase {
 		KeyRing::unwrapAny($wrapped, 'right');
 	}
 
+	public function testAShortSaltOrNonceIsADamagedKey(): void {
+		foreach (['salt', 'nonce'] as $part) {
+			$wrapped = KeyRing::generate()->wrap('right');
+			$wrapped[$part] = base64_encode('short');
+			try {
+				KeyRing::unwrapAny($wrapped, 'right');
+				$this->fail("a short $part was accepted");
+			} catch (CryptoException $e) {
+				$this->assertStringContainsString('Damaged key file', $e->getMessage(), $part);
+			}
+		}
+	}
+
 	public function testBlobIdsAreKeyed(): void {
 		$this->assertNotSame(KeyRing::generate()->blobId('same'), KeyRing::generate()->blobId('same'));
 		$k = KeyRing::generate();