From 3ab660b8fea2ed4d209f077cbbf6c4288b437ae0 Mon Sep 17 00:00:00 2001 From: Shreya Malviya Date: Mon, 4 Oct 2021 13:43:51 +0530 Subject: [PATCH 1/8] tests: Add unit tests for key based encryptor --- .../encryption/test_key_based_encryptor.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_key_based_encryptor.py diff --git a/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_key_based_encryptor.py b/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_key_based_encryptor.py new file mode 100644 index 000000000..391002417 --- /dev/null +++ b/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_key_based_encryptor.py @@ -0,0 +1,19 @@ +from monkey_island.cc.server_utils.encryption import KeyBasedEncryptor + +PLAINTEXT = "password" +PLAINTEXT_UTF8 = "slaptažodis" # "password" in Lithuanian +KEY = b"\x84\xd4qA\xb5\xd4Y\x9bH.\x14\xab\xd8\xc7+g\x12\xfa\x80'%\xfd#\xf8c\x94\xb9\x96_\xf4\xc51" + +kb_encryptor = KeyBasedEncryptor(KEY) + + +def test_encrypt_decrypt_string_with_key(): + encrypted = kb_encryptor.encrypt(PLAINTEXT) + decrypted = kb_encryptor.decrypt(encrypted) + assert decrypted == PLAINTEXT + + +def test_encrypt_decrypt_string_utf8_with_key(): + encrypted = kb_encryptor.encrypt(PLAINTEXT_UTF8) + decrypted = kb_encryptor.decrypt(encrypted) + assert decrypted == PLAINTEXT_UTF8 From fc1affc0e7400d1dae120bd6fd4b3fba260ef3b2 Mon Sep 17 00:00:00 2001 From: Shreya Malviya Date: Mon, 4 Oct 2021 14:02:51 +0530 Subject: [PATCH 2/8] island: Change KeyBasedEncryptor's padding functions to use Crypto.Util.Padding --- .../encryption/encryptors/key_based_encryptor.py | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/monkey/monkey_island/cc/server_utils/encryption/encryptors/key_based_encryptor.py b/monkey/monkey_island/cc/server_utils/encryption/encryptors/key_based_encryptor.py index b5fe92d96..78aaacf9c 100644 --- a/monkey/monkey_island/cc/server_utils/encryption/encryptors/key_based_encryptor.py +++ b/monkey/monkey_island/cc/server_utils/encryption/encryptors/key_based_encryptor.py @@ -5,6 +5,7 @@ import logging # is maintained. from Crypto import Random # noqa: DUO133 # nosec: B413 from Crypto.Cipher import AES # noqa: DUO133 # nosec: B413 +from Crypto.Util import Padding # noqa: DUO133 from monkey_island.cc.server_utils.encryption import IEncryptor @@ -37,11 +38,8 @@ class KeyBasedEncryptor(IEncryptor): cipher = AES.new(self._key, AES.MODE_CBC, cipher_iv) return self._unpad(cipher.decrypt(enc_message[AES.block_size :]).decode()) - # TODO: Review and evaluate the security of the padding function - def _pad(self, message): - return message + (self._BLOCK_SIZE - (len(message) % self._BLOCK_SIZE)) * chr( - self._BLOCK_SIZE - (len(message) % self._BLOCK_SIZE) - ) + def _pad(self, message: str) -> str: + return Padding.pad(message.encode(), self._BLOCK_SIZE).decode() - def _unpad(self, message: str): - return message[0 : -ord(message[len(message) - 1])] + def _unpad(self, message: str) -> str: + return Padding.unpad(message.encode(), self._BLOCK_SIZE).decode() From 404228b04c7a874518922be37c97d04c8ca86fb2 Mon Sep 17 00:00:00 2001 From: Shreya Malviya Date: Mon, 4 Oct 2021 14:06:28 +0530 Subject: [PATCH 3/8] island: Modify KeyBasedEncryptor to get rid of redundant encoding and decoding --- .../encryption/encryptors/key_based_encryptor.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/monkey/monkey_island/cc/server_utils/encryption/encryptors/key_based_encryptor.py b/monkey/monkey_island/cc/server_utils/encryption/encryptors/key_based_encryptor.py index 78aaacf9c..cb37a8c08 100644 --- a/monkey/monkey_island/cc/server_utils/encryption/encryptors/key_based_encryptor.py +++ b/monkey/monkey_island/cc/server_utils/encryption/encryptors/key_based_encryptor.py @@ -30,16 +30,16 @@ class KeyBasedEncryptor(IEncryptor): def encrypt(self, plaintext: str) -> str: cipher_iv = Random.new().read(AES.block_size) cipher = AES.new(self._key, AES.MODE_CBC, cipher_iv) - return base64.b64encode(cipher_iv + cipher.encrypt(self._pad(plaintext).encode())).decode() + return base64.b64encode(cipher_iv + cipher.encrypt(self._pad(plaintext))).decode() def decrypt(self, ciphertext: str): enc_message = base64.b64decode(ciphertext) cipher_iv = enc_message[0 : AES.block_size] cipher = AES.new(self._key, AES.MODE_CBC, cipher_iv) - return self._unpad(cipher.decrypt(enc_message[AES.block_size :]).decode()) + return self._unpad(cipher.decrypt(enc_message[AES.block_size :])) - def _pad(self, message: str) -> str: - return Padding.pad(message.encode(), self._BLOCK_SIZE).decode() + def _pad(self, message: str) -> bytes: + return Padding.pad(message.encode(), self._BLOCK_SIZE) - def _unpad(self, message: str) -> str: - return Padding.unpad(message.encode(), self._BLOCK_SIZE).decode() + def _unpad(self, message: bytes) -> str: + return Padding.unpad(message, self._BLOCK_SIZE).decode() From f6b13309827533559f8ec00b3b4bd0a622fa7f8a Mon Sep 17 00:00:00 2001 From: Shreya Malviya Date: Tue, 5 Oct 2021 12:05:49 +0530 Subject: [PATCH 4/8] tests: Add test cases for KeyBasedEncryptor's tests --- .../encryption/test_key_based_encryptor.py | 22 +++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_key_based_encryptor.py b/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_key_based_encryptor.py index 391002417..10803cff9 100644 --- a/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_key_based_encryptor.py +++ b/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_key_based_encryptor.py @@ -1,7 +1,9 @@ from monkey_island.cc.server_utils.encryption import KeyBasedEncryptor PLAINTEXT = "password" -PLAINTEXT_UTF8 = "slaptažodis" # "password" in Lithuanian +PLAINTEXT_UTF8_1 = "slaptažodis" # "password" in Lithuanian +PLAINTEXT_UTF8_2 = "弟" # Japanese +PLAINTEXT_UTF8_3 = "ж" # Ukranian KEY = b"\x84\xd4qA\xb5\xd4Y\x9bH.\x14\xab\xd8\xc7+g\x12\xfa\x80'%\xfd#\xf8c\x94\xb9\x96_\xf4\xc51" kb_encryptor = KeyBasedEncryptor(KEY) @@ -13,7 +15,19 @@ def test_encrypt_decrypt_string_with_key(): assert decrypted == PLAINTEXT -def test_encrypt_decrypt_string_utf8_with_key(): - encrypted = kb_encryptor.encrypt(PLAINTEXT_UTF8) +def test_encrypt_decrypt_string_utf8_with_key_1(): + encrypted = kb_encryptor.encrypt(PLAINTEXT_UTF8_1) decrypted = kb_encryptor.decrypt(encrypted) - assert decrypted == PLAINTEXT_UTF8 + assert decrypted == PLAINTEXT_UTF8_1 + + +def test_encrypt_decrypt_string_utf8_with_key_2(): + encrypted = kb_encryptor.encrypt(PLAINTEXT_UTF8_2) + decrypted = kb_encryptor.decrypt(encrypted) + assert decrypted == PLAINTEXT_UTF8_2 + + +def test_encrypt_decrypt_string_utf8_with_key_3(): + encrypted = kb_encryptor.encrypt(PLAINTEXT_UTF8_3) + decrypted = kb_encryptor.decrypt(encrypted) + assert decrypted == PLAINTEXT_UTF8_3 From f1b96836174555967f848be46110fb00b083011f Mon Sep 17 00:00:00 2001 From: Shreya Malviya Date: Tue, 5 Oct 2021 12:08:52 +0530 Subject: [PATCH 5/8] tests: Use pytest's parametrize for KeyBasedEncryptor's unit tests --- .../encryption/test_key_based_encryptor.py | 21 ++++++------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_key_based_encryptor.py b/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_key_based_encryptor.py index 10803cff9..56e6565a0 100644 --- a/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_key_based_encryptor.py +++ b/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_key_based_encryptor.py @@ -1,3 +1,5 @@ +import pytest + from monkey_island.cc.server_utils.encryption import KeyBasedEncryptor PLAINTEXT = "password" @@ -15,19 +17,8 @@ def test_encrypt_decrypt_string_with_key(): assert decrypted == PLAINTEXT -def test_encrypt_decrypt_string_utf8_with_key_1(): - encrypted = kb_encryptor.encrypt(PLAINTEXT_UTF8_1) +@pytest.mark.parametrize("plaintext", [PLAINTEXT_UTF8_1, PLAINTEXT_UTF8_2, PLAINTEXT_UTF8_3]) +def test_encrypt_decrypt_string_utf8_with_key(plaintext): + encrypted = kb_encryptor.encrypt(plaintext) decrypted = kb_encryptor.decrypt(encrypted) - assert decrypted == PLAINTEXT_UTF8_1 - - -def test_encrypt_decrypt_string_utf8_with_key_2(): - encrypted = kb_encryptor.encrypt(PLAINTEXT_UTF8_2) - decrypted = kb_encryptor.decrypt(encrypted) - assert decrypted == PLAINTEXT_UTF8_2 - - -def test_encrypt_decrypt_string_utf8_with_key_3(): - encrypted = kb_encryptor.encrypt(PLAINTEXT_UTF8_3) - decrypted = kb_encryptor.decrypt(encrypted) - assert decrypted == PLAINTEXT_UTF8_3 + assert decrypted == plaintext From 06778b75258290f07ec6585e3a91decf77022a7a Mon Sep 17 00:00:00 2001 From: Shreya Malviya Date: Tue, 5 Oct 2021 12:14:37 +0530 Subject: [PATCH 6/8] island: Remove thin wrappers for padding in KeyBasedEncryptor, call inline --- .../encryption/encryptors/key_based_encryptor.py | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/monkey/monkey_island/cc/server_utils/encryption/encryptors/key_based_encryptor.py b/monkey/monkey_island/cc/server_utils/encryption/encryptors/key_based_encryptor.py index cb37a8c08..551014be1 100644 --- a/monkey/monkey_island/cc/server_utils/encryption/encryptors/key_based_encryptor.py +++ b/monkey/monkey_island/cc/server_utils/encryption/encryptors/key_based_encryptor.py @@ -30,16 +30,12 @@ class KeyBasedEncryptor(IEncryptor): def encrypt(self, plaintext: str) -> str: cipher_iv = Random.new().read(AES.block_size) cipher = AES.new(self._key, AES.MODE_CBC, cipher_iv) - return base64.b64encode(cipher_iv + cipher.encrypt(self._pad(plaintext))).decode() + padded_plaintext = Padding.pad(plaintext.encode(), self._BLOCK_SIZE) + return base64.b64encode(cipher_iv + cipher.encrypt(padded_plaintext)).decode() def decrypt(self, ciphertext: str): enc_message = base64.b64decode(ciphertext) cipher_iv = enc_message[0 : AES.block_size] cipher = AES.new(self._key, AES.MODE_CBC, cipher_iv) - return self._unpad(cipher.decrypt(enc_message[AES.block_size :])) - - def _pad(self, message: str) -> bytes: - return Padding.pad(message.encode(), self._BLOCK_SIZE) - - def _unpad(self, message: bytes) -> str: - return Padding.unpad(message, self._BLOCK_SIZE).decode() + padded_plaintext = cipher.decrypt(enc_message[AES.block_size :]) + return Padding.unpad(padded_plaintext, self._BLOCK_SIZE).decode() From f2b632e46afd81abe2b2e08f8f38c62a2cd4a9c9 Mon Sep 17 00:00:00 2001 From: Shreya Malviya Date: Tue, 5 Oct 2021 12:26:02 +0530 Subject: [PATCH 7/8] tests: Add KeyBasedEcnryptor unit test for plaintext which is a multiple of block size in length --- .../server_utils/encryption/test_key_based_encryptor.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_key_based_encryptor.py b/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_key_based_encryptor.py index 56e6565a0..d41f866a7 100644 --- a/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_key_based_encryptor.py +++ b/monkey/tests/unit_tests/monkey_island/cc/server_utils/encryption/test_key_based_encryptor.py @@ -3,9 +3,11 @@ import pytest from monkey_island.cc.server_utils.encryption import KeyBasedEncryptor PLAINTEXT = "password" +PLAINTEXT_MULTIPLE_BLOCK_SIZE = "banana" * KeyBasedEncryptor._BLOCK_SIZE PLAINTEXT_UTF8_1 = "slaptažodis" # "password" in Lithuanian PLAINTEXT_UTF8_2 = "弟" # Japanese PLAINTEXT_UTF8_3 = "ж" # Ukranian + KEY = b"\x84\xd4qA\xb5\xd4Y\x9bH.\x14\xab\xd8\xc7+g\x12\xfa\x80'%\xfd#\xf8c\x94\xb9\x96_\xf4\xc51" kb_encryptor = KeyBasedEncryptor(KEY) @@ -22,3 +24,9 @@ def test_encrypt_decrypt_string_utf8_with_key(plaintext): encrypted = kb_encryptor.encrypt(plaintext) decrypted = kb_encryptor.decrypt(encrypted) assert decrypted == plaintext + + +def test_encrypt_decrypt_string_multiple_block_size_with_key(): + encrypted = kb_encryptor.encrypt(PLAINTEXT_MULTIPLE_BLOCK_SIZE) + decrypted = kb_encryptor.decrypt(encrypted) + assert decrypted == PLAINTEXT_MULTIPLE_BLOCK_SIZE From 19dad894685e71fe4627d6672df7245da8eaa4b6 Mon Sep 17 00:00:00 2001 From: Shreya Malviya Date: Tue, 5 Oct 2021 12:29:49 +0530 Subject: [PATCH 8/8] CHANGELOG: Add entry for encryptor not working with utf-8 characters bugfix --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0aec3d277..d56de4aa8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,9 @@ Changelog](https://keepachangelog.com/en/1.0.0/). - PBA table collapse in security report on data change. #1423 - Unsigned Windows agent binaries in Linux packages are now signed. #1444 - Some of the gathered credentials no longer appear in database plaintext. #1454 +- Encryptor breaking with UTF-8 characters. (Passwords in different languages can be submitted in + the config successfully now.) #1490 + ### Security - Generate a random password when creating a new user for CommunicateAsNewUser