From 1e90bb677e502482cf7b57aaf90effcdf20939d5 Mon Sep 17 00:00:00 2001
From: Clemens Schwaighofer <clemens.schwaighofer@egplusww.com>
Date: Thu, 12 Dec 2024 21:07:17 +0900
Subject: [PATCH] Fix Symmetric encryption with wrong key handling

- static call encrypt: do not check pre set key
- indirect call: set new if key is different
---
 ...oreLibsSecuritySymmetricEncryptionTest.php | 167 +++++++++++++++++-
 .../CoreLibs/Security/SymmetricEncryption.php |   8 +-
 2 files changed, 172 insertions(+), 3 deletions(-)

diff --git a/4dev/tests/Security/CoreLibsSecuritySymmetricEncryptionTest.php b/4dev/tests/Security/CoreLibsSecuritySymmetricEncryptionTest.php
index d3f502af..d7486501 100644
--- a/4dev/tests/Security/CoreLibsSecuritySymmetricEncryptionTest.php
+++ b/4dev/tests/Security/CoreLibsSecuritySymmetricEncryptionTest.php
@@ -56,7 +56,24 @@ final class CoreLibsSecuritySymmetricEncryptionTest extends TestCase
 			$decrypted,
 			'Class call',
 		);
+	}
 
+	/**
+	 * test encrypt/decrypt produce correct output
+	 *
+	 * @covers ::generateRandomKey
+	 * @covers ::encrypt
+	 * @covers ::decrypt
+	 * @dataProvider providerEncryptDecryptSuccess
+	 * @testdox encrypt/decrypt indirect $input must be $expected [$_dataName]
+	 *
+	 * @param  string $input
+	 * @param  string $expected
+	 * @return void
+	 */
+	public function testEncryptDecryptSuccessIndirect(string $input, string $expected): void
+	{
+		$key = CreateKey::generateRandomKey();
 		// test indirect
 		$encrypted = SymmetricEncryption::getInstance($key)->encrypt($input);
 		$decrypted = SymmetricEncryption::getInstance($key)->decrypt($encrypted);
@@ -65,7 +82,24 @@ final class CoreLibsSecuritySymmetricEncryptionTest extends TestCase
 			$decrypted,
 			'Class Instance call',
 		);
+	}
 
+	/**
+	 * test encrypt/decrypt produce correct output
+	 *
+	 * @covers ::generateRandomKey
+	 * @covers ::encrypt
+	 * @covers ::decrypt
+	 * @dataProvider providerEncryptDecryptSuccess
+	 * @testdox encrypt/decrypt static $input must be $expected [$_dataName]
+	 *
+	 * @param  string $input
+	 * @param  string $expected
+	 * @return void
+	 */
+	public function testEncryptDecryptSuccessStatic(string $input, string $expected): void
+	{
+		$key = CreateKey::generateRandomKey();
 		// test static
 		$encrypted = SymmetricEncryption::encryptKey($input, $key);
 		$decrypted = SymmetricEncryption::decryptKey($encrypted, $key);
@@ -114,13 +148,51 @@ final class CoreLibsSecuritySymmetricEncryptionTest extends TestCase
 		$crypt = new SymmetricEncryption($key);
 		$encrypted = $crypt->encrypt($input);
 		$this->expectExceptionMessage($exception_message);
-		$crypt->setKey($key);
+		$crypt->setKey($wrong_key);
 		$crypt->decrypt($encrypted);
+	}
+
+	/**
+	 * Test decryption with wrong key
+	 *
+	 * @covers ::generateRandomKey
+	 * @covers ::encrypt
+	 * @covers ::decrypt
+	 * @dataProvider providerEncryptFailed
+	 * @testdox decrypt indirect with wrong key $input throws $exception_message [$_dataName]
+	 *
+	 * @param  string $input
+	 * @param  string $exception_message
+	 * @return void
+	 */
+	public function testEncryptFailedIndirect(string $input, string $exception_message): void
+	{
+		$key = CreateKey::generateRandomKey();
+		$wrong_key = CreateKey::generateRandomKey();
 
 		// class instance
 		$encrypted = SymmetricEncryption::getInstance($key)->encrypt($input);
 		$this->expectExceptionMessage($exception_message);
 		SymmetricEncryption::getInstance($wrong_key)->decrypt($encrypted);
+	}
+
+	/**
+	 * Test decryption with wrong key
+	 *
+	 * @covers ::generateRandomKey
+	 * @covers ::encrypt
+	 * @covers ::decrypt
+	 * @dataProvider providerEncryptFailed
+	 * @testdox decrypt static with wrong key $input throws $exception_message [$_dataName]
+	 *
+	 * @param  string $input
+	 * @param  string $exception_message
+	 * @return void
+	 */
+	public function testEncryptFailedStatic(string $input, string $exception_message): void
+	{
+		$key = CreateKey::generateRandomKey();
+		$wrong_key = CreateKey::generateRandomKey();
 
 		// class static
 		$encrypted = SymmetricEncryption::encryptKey($input, $key);
@@ -190,6 +262,56 @@ final class CoreLibsSecuritySymmetricEncryptionTest extends TestCase
 		SymmetricEncryption::decryptKey($encrypted, $key);
 	}
 
+	/**
+	 * test invalid key provided to decrypt or encrypt
+	 *
+	 * @covers ::encrypt
+	 * @covers ::decrypt
+	 * @dataProvider providerWrongKey
+	 * @testdox wrong key indirect $key throws $exception_message [$_dataName]
+	 *
+	 * @param  string $key
+	 * @param  string $exception_message
+	 * @return void
+	 */
+	public function testWrongKeyIndirect(string $key, string $exception_message): void
+	{
+		$enc_key = CreateKey::generateRandomKey();
+
+		// class instance
+		$this->expectExceptionMessage($exception_message);
+		SymmetricEncryption::getInstance($key)->encrypt('test');
+		// we must encrypt valid thing first so we can fail with the wrong key
+		$encrypted = SymmetricEncryption::getInstance($enc_key)->encrypt('test');
+		$this->expectExceptionMessage($exception_message);
+		SymmetricEncryption::getInstance($key)->decrypt($encrypted);
+	}
+
+		/**
+	 * test invalid key provided to decrypt or encrypt
+	 *
+	 * @covers ::encrypt
+	 * @covers ::decrypt
+	 * @dataProvider providerWrongKey
+	 * @testdox wrong key static $key throws $exception_message [$_dataName]
+	 *
+	 * @param  string $key
+	 * @param  string $exception_message
+	 * @return void
+	 */
+	public function testWrongKeyStatic(string $key, string $exception_message): void
+	{
+		$enc_key = CreateKey::generateRandomKey();
+
+		// class static
+		$this->expectExceptionMessage($exception_message);
+		SymmetricEncryption::encryptKey('test', $key);
+		// we must encrypt valid thing first so we can fail with the wrong key
+		$encrypted = SymmetricEncryption::encryptKey('test', $enc_key);
+		$this->expectExceptionMessage($exception_message);
+		SymmetricEncryption::decryptKey($encrypted, $key);
+	}
+
 	/**
 	 * Undocumented function
 	 *
@@ -232,6 +354,49 @@ final class CoreLibsSecuritySymmetricEncryptionTest extends TestCase
 		$this->expectExceptionMessage($exception_message);
 		SymmetricEncryption::decryptKey($input, $key);
 	}
+
+	/**
+	 * Undocumented function
+	 *
+	 * @covers ::decrypt
+	 * @dataProvider providerWrongCiphertext
+	 * @testdox too short ciphertext indirect $input throws $exception_message [$_dataName]
+	 *
+	 * @param  string $input
+	 * @param  string $exception_message
+	 * @return void
+	 */
+	public function testWrongCiphertextIndirect(string $input, string $exception_message): void
+	{
+		$key = CreateKey::generateRandomKey();
+
+		// class instance
+		$this->expectExceptionMessage($exception_message);
+		SymmetricEncryption::getInstance($key)->decrypt($input);
+
+		// class static
+		$this->expectExceptionMessage($exception_message);
+		SymmetricEncryption::decryptKey($input, $key);
+	}
+
+		/**
+	 * Undocumented function
+	 *
+	 * @covers ::decrypt
+	 * @dataProvider providerWrongCiphertext
+	 * @testdox too short ciphertext static $input throws $exception_message [$_dataName]
+	 *
+	 * @param  string $input
+	 * @param  string $exception_message
+	 * @return void
+	 */
+	public function testWrongCiphertextStatic(string $input, string $exception_message): void
+	{
+		$key = CreateKey::generateRandomKey();
+		// class static
+		$this->expectExceptionMessage($exception_message);
+		SymmetricEncryption::decryptKey($input, $key);
+	}
 }
 
 // __END__
diff --git a/www/lib/CoreLibs/Security/SymmetricEncryption.php b/www/lib/CoreLibs/Security/SymmetricEncryption.php
index c12f4b3f..2f8fb75e 100644
--- a/www/lib/CoreLibs/Security/SymmetricEncryption.php
+++ b/www/lib/CoreLibs/Security/SymmetricEncryption.php
@@ -49,7 +49,11 @@ class SymmetricEncryption
 	 */
 	public static function getInstance(string|null $key = null): self
 	{
-		if (empty(self::$instance)) {
+		// new if no instsance or key is different
+		if (
+			empty(self::$instance) ||
+			self::$instance->key != $key
+		) {
 			self::$instance = new self($key);
 		}
 		return self::$instance;
@@ -130,7 +134,7 @@ class SymmetricEncryption
 	 */
 	private function encryptData(string $message, ?string $key): string
 	{
-		if (empty($this->key) || $key === null) {
+		if ($key === null) {
 			throw new \UnexpectedValueException('Key not set');
 		}
 		$key = $this->createKey($key);