From 11b85e52479b5870cfeaf0bf6f770e5b52149145 Mon Sep 17 00:00:00 2001 From: Marc Heckmann Date: Fri, 19 Oct 2018 10:03:22 -0400 Subject: [PATCH] Fix encoding of plaintext length indicator for secrets (fixes #2473) The encoding of the secret length indicator has likely been broken since commit b21833f79c86bcdbc5a080703fd15f0e12cfa7a0 which introduced Py3 support for secrets: the indicator was switched to using UTF-8 byte strings and this caused the length indicator to consume 3 bytes instead of just 2 under certain circumstances. For example when the plaintext secret length is between 128 and 256 bytes long. This is because contrary to latin-1 encoding, UTF-8 byte sting encoding consumes 2 bytes for code points > 80. See the table at https://en.wikipedia.org/wiki/UTF-8#Description The fix is to explicitely use 'latin-1' encoding for the length indicator. This makes the code behave exactly as it did with the original Python2 implemenation while remaining compatible w/ Py3. A test case which uses a 171 byte long plaintext string has also been added. This test triggers the bug when the fix is not present. This fixes #2473 --- netbox/secrets/models.py | 4 ++-- netbox/secrets/tests/test_models.py | 23 +++++++++++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/netbox/secrets/models.py b/netbox/secrets/models.py index 8bbf3d14d..3544fad33 100644 --- a/netbox/secrets/models.py +++ b/netbox/secrets/models.py @@ -400,8 +400,8 @@ class Secret(ChangeLoggedModel, CustomFieldModel): else: pad_length = 0 return ( - chr(len(s) >> 8).encode() + - chr(len(s) % 256).encode() + + chr(len(s) >> 8).encode('latin-1') + + chr(len(s) % 256).encode('latin-1') + s + os.urandom(pad_length) ) diff --git a/netbox/secrets/tests/test_models.py b/netbox/secrets/tests/test_models.py index 887c048bf..13a25de45 100644 --- a/netbox/secrets/tests/test_models.py +++ b/netbox/secrets/tests/test_models.py @@ -131,3 +131,26 @@ class SecretTestCase(TestCase): self.assertEqual(duplicate_ivs, [], "One or more duplicate IVs found!") duplicate_ciphertexts = [i for i, x in enumerate(ciphertexts) if ciphertexts.count(x) > 1] self.assertEqual(duplicate_ciphertexts, [], "One or more duplicate ciphertexts (first blocks) found!") + + def test_03_encrypt_longer_plaintext(self): + """ + Test basic encryption and decryption using a plaintext string with + more than 128 and less than 256 characters to trigger any potential + padding bugs which would prevent decryption + """ + plaintext = "ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBFDpfiHbXIB/J9Gpds6Ad3qkd7+nRWFjIzT/njvE3LZsz+ZIMtMS1bZeD16grrDPvEVPdj3TpIAHaBMUsdCq4g8= root@test" + secret_key = generate_random_key() + s = Secret(plaintext=plaintext) + s.encrypt(secret_key) + + # Enforce minimum ciphertext length + self.assertGreaterEqual(len(s.ciphertext), 80, "Ciphertext must be at least 80 bytes (16B IV + 64B+ ciphertext") + + # Test hash validation + self.assertTrue(s.validate(plaintext), "Plaintext does not validate against the generated hash") + self.assertFalse(s.validate(""), "Empty plaintext validated against hash") + self.assertFalse(s.validate("Invalid plaintext"), "Invalid plaintext validated against hash") + + # Test decryption + s.decrypt(secret_key) + self.assertEqual(plaintext, s.plaintext, "Decrypting Secret returned incorrect plaintext")