VM-1452: replace pickle cache serialization with JSON

integration/keeper_secrets_manager_ansible/README.md

@@ -19,6 +19,13 @@ For more information see our official documentation page https://docs.keeper.io/
 
 # Changes
 
+## 1.5.0
+* **Security**: VM-1452 / CWE-502 — Replaced pickle with JSON for encrypted record cache serialization
+  - Cache encrypt/decrypt no longer uses `pickle.loads`, removing insecure deserialization risk
+  - Legacy or invalid registered caches are ignored; records are fetched from the vault until
+    `keeper_cache_records` rebuilds a JSON cache
+  - Existing playbook-registered caches are ephemeral; regenerate with `keeper_cache_records` after upgrade
+
 ## 1.4.0
 * KSM-827: Fixed Tower Execution Environment Docker image missing system packages required by AAP
   - Added `openssh-clients`, `sshpass`, `rsync`, and `git` to `additional_build_packages` in `execution-environment.yml`

integration/keeper_secrets_manager_ansible/ansible_galaxy/keepersecurity/keeper_secrets_manager/README.md

@@ -119,6 +119,13 @@ configuration file or even a playbook.
 
 # Changes
 
+## 1.5.0
+* **Security**: VM-1452 / CWE-502 — Replaced pickle with JSON for encrypted record cache serialization
+  - Cache encrypt/decrypt no longer uses `pickle.loads`, removing insecure deserialization risk
+  - Legacy or invalid registered caches are ignored; records are fetched from the vault until
+    `keeper_cache_records` rebuilds a JSON cache
+  - Existing playbook-registered caches are ephemeral; regenerate with `keeper_cache_records` after upgrade
+
 ## 1.4.0
 * KSM-827: Fixed Tower Execution Environment Docker image missing system packages required by AAP
   - Added `openssh-clients`, `sshpass`, `rsync`, and `git` to the EE image

integration/keeper_secrets_manager_ansible/keeper_secrets_manager_ansible/__init__.py

@@ -46,6 +46,10 @@
 display = Display()
 
 
+class CacheUnusableError(ValueError):
+    """Encrypted record cache cannot be decrypted or deserialized; treat as a cache miss."""
+
+
 class KeeperFieldType(Enum):
     FIELD = "field"
     CUSTOM_FIELD = "custom_field"
@@ -391,9 +395,39 @@ def encrypt(self, data):
 
     def decrypt(self, ciphertext):
         secret_key = self.get_encryption_key()
-        json_bytes = Fernet(secret_key).decrypt(ciphertext)
-        record_dicts = json.loads(json_bytes)
-        return [KeeperAnsible._record_from_dict(d) for d in record_dicts]
+        try:
+            plaintext = Fernet(secret_key).decrypt(ciphertext)
+        except Exception as err:
+            raise CacheUnusableError(
+                "Unable to decrypt the record cache. Check keeper_record_cache_secret "
+                "or regenerate the cache with keeper_cache_records."
+            ) from err
+
+        # Pickle protocol markers (e.g. 0x80) -- never call pickle.loads (CWE-502 / VM-1452).
+        if plaintext.startswith(b"\x80"):
+            raise CacheUnusableError(
+                "Unable to deserialize the record cache. The cache may be from an older "
+                "plugin version or is invalid. Regenerate the cache with keeper_cache_records."
+            )
+
+        try:
+            payload = json.loads(plaintext.decode("utf-8"))
+        except (UnicodeDecodeError, json.JSONDecodeError, TypeError, ValueError) as err:
+            raise CacheUnusableError(
+                "Unable to deserialize the record cache. The cache may be from an older "
+                "plugin version or is invalid. Regenerate the cache with keeper_cache_records."
+            ) from err
+
+        if not isinstance(payload, list):
+            raise CacheUnusableError(
+                "Unable to deserialize the record cache. Expected a list of records. "
+                "Regenerate the cache with keeper_cache_records."
+            )
+
+        try:
+            return [KeeperAnsible._record_from_dict(d) for d in payload]
+        except (KeyError, TypeError, ValueError) as err:
+            raise CacheUnusableError(str(err)) from err
 
     @staticmethod
     def convert_records_into_dict(records):
@@ -538,7 +572,15 @@ def get_records_from_cache(self, cache, uids=None, titles=None):
     def get_records(self, uids=None, titles=None, cache=None, encrypt=False):
 
         if cache is not None:
-            records = self.get_records_from_cache(cache, uids=uids, titles=titles)
+            try:
+                records = self.get_records_from_cache(cache, uids=uids, titles=titles)
+            except CacheUnusableError:
+                # Invalidate legacy/invalid cache and start from scratch via the vault.
+                display.warning(
+                    "Keeper record cache is unusable (legacy or invalid format) and was ignored. "
+                    "Fetching records from the vault. Regenerate the cache with keeper_cache_records."
+                )
+                records = self.get_records_from_vault(uids=uids, titles=titles, encrypt=encrypt)
         else:
             records = self.get_records_from_vault(uids=uids, titles=titles, encrypt=encrypt)
 

integration/keeper_secrets_manager_ansible/tests/keeper_cache_encrypt_test.py

@@ -0,0 +1,198 @@
+# -*- coding: utf-8 -*-
+"""Unit tests for JSON-based record cache encrypt/decrypt (VM-1452 / CWE-502)."""
+
+import base64
+import io
+import os
+import pickle
+import socket
+import sys
+import tempfile
+import unittest
+from unittest.mock import MagicMock, patch
+
+# ansible-core imports fcntl (unavailable on Windows). Stub ansible for unit tests
+# that only exercise encrypt/decrypt crypto, not the full plugin runtime.
+try:
+    import fcntl  # noqa: F401
+except ImportError:
+    class _FakeAnsibleError(Exception):
+        pass
+
+    _ansible_stub = MagicMock()
+    _ansible_stub.AnsibleError = _FakeAnsibleError
+    _ansible_stub.errors.AnsibleError = _FakeAnsibleError
+    sys.modules.setdefault("ansible", _ansible_stub)
+    sys.modules.setdefault("ansible.utils", _ansible_stub)
+    sys.modules.setdefault("ansible.utils.display", _ansible_stub)
+    sys.modules.setdefault("ansible.errors", _ansible_stub)
+    sys.modules.setdefault("ansible.module_utils", _ansible_stub)
+    sys.modules.setdefault("ansible.module_utils.basic", _ansible_stub)
+    sys.modules.setdefault("ansible.module_utils.common", _ansible_stub)
+    sys.modules.setdefault("ansible.module_utils.common.text", _ansible_stub)
+    sys.modules.setdefault("ansible.module_utils.common.text.converters", _ansible_stub)
+    _ansible_stub.module_utils.basic.missing_required_lib = lambda name: name
+    _ansible_stub.module_utils.common.text.converters.jsonify = lambda x: str(x)
+    _ansible_stub.utils.display.Display = MagicMock
+
+from cryptography.fernet import Fernet
+from cryptography.hazmat.primitives import hashes
+from cryptography.hazmat.primitives.kdf.pbkdf2 import PBKDF2HMAC
+from keeper_secrets_manager_core.dto.dtos import Record
+
+from keeper_secrets_manager_ansible import CacheUnusableError, KeeperAnsible
+
+
+def _make_record(uid="uid123", title="Test Record", password="secret-pass"):
+    """Build a minimal Record suitable for cache serialize/deserialize."""
+    record = Record.__new__(Record)
+    record.uid = uid
+    record.title = title
+    record.type = "login"
+    record.dict = {
+        "title": title,
+        "type": "login",
+        "fields": [
+            {"type": "login", "value": ["user1"]},
+            {"type": "password", "value": [password]},
+        ],
+        "custom": [],
+    }
+    record.raw_json = None
+    record.record_key_bytes = os.urandom(32)
+    record.folder_uid = ""
+    record.inner_folder_uid = ""
+    record.revision = 1
+    record.is_editable = True
+    record.password = password
+    record.links = []
+    record.files = []
+    return record
+
+
+def _stub_keeper(cache_secret="unit-test-cache-secret"):
+    """Return a KeeperAnsible instance with __init__ bypassed."""
+    with patch.object(KeeperAnsible, "__init__", lambda self, *a, **k: None):
+        keeper = KeeperAnsible.__new__(KeeperAnsible)
+    keeper.task_vars = {"keeper_record_cache_secret": cache_secret}
+    keeper.client = MagicMock()
+    keeper.action_module = None
+    return keeper
+
+
+class KeeperCacheEncryptTest(unittest.TestCase):
+
+    def test_encrypt_decrypt_round_trip(self):
+        keeper = _stub_keeper()
+        original = _make_record()
+
+        ciphertext = keeper.encrypt([original])
+        restored = keeper.decrypt(ciphertext)
+
+        self.assertEqual(len(restored), 1)
+        self.assertEqual(restored[0].uid, original.uid)
+        self.assertEqual(restored[0].title, original.title)
+        self.assertEqual(restored[0].type, original.type)
+        self.assertEqual(restored[0].dict, original.dict)
+        self.assertEqual(restored[0].record_key_bytes, original.record_key_bytes)
+        self.assertEqual(restored[0].field("password"), ["secret-pass"])
+        self.assertEqual(restored[0].field("login"), ["user1"])
+
+    def test_decrypt_rejects_pickle_payload(self):
+        """Encrypted pickle must not execute; decrypt must fail safely (VM-1452)."""
+        cache_secret = "attacker-controlled-secret-12345"
+        proof_path = None
+
+        with tempfile.NamedTemporaryFile(delete=False) as tmp:
+            proof_path = tmp.name
+        try:
+            if os.path.exists(proof_path):
+                os.remove(proof_path)
+
+            class Exploit:
+                def __reduce__(self):
+                    return (open, (proof_path, "w"))
+
+            hostname = socket.gethostname()
+            salt = hostname.zfill(32)[0:32]
+            kdf = PBKDF2HMAC(
+                algorithm=hashes.SHA256(),
+                length=32,
+                salt=salt.encode(),
+                iterations=390000,
+            )
+            key = base64.urlsafe_b64encode(kdf.derive(cache_secret.encode()))
+            buf = io.BytesIO()
+            pickle.dump(Exploit(), buf)
+            malicious = Fernet(key).encrypt(buf.getvalue())
+
+            keeper = _stub_keeper(cache_secret=cache_secret)
+            with self.assertRaises(CacheUnusableError):
+                keeper.decrypt(malicious)
+
+            self.assertFalse(
+                os.path.exists(proof_path),
+                "pickle payload must not execute during decrypt",
+            )
+        finally:
+            if proof_path and os.path.exists(proof_path):
+                os.remove(proof_path)
+
+    def test_decrypt_rejects_invalid_json_shape(self):
+        keeper = _stub_keeper()
+        secret_key = keeper.get_encryption_key()
+        bad = Fernet(secret_key).encrypt(b'{"not": "a list"}')
+        with self.assertRaises(CacheUnusableError) as ctx:
+            keeper.decrypt(bad)
+        self.assertIn("keeper_cache_records", str(ctx.exception))
+
+    def test_get_records_falls_back_to_vault_on_legacy_cache(self):
+        """Unusable cache is ignored; records are fetched from the vault."""
+        keeper = _stub_keeper()
+        vault_record = _make_record(uid="from-vault", title="Vault Record")
+        keeper.client.get_secrets.return_value = [vault_record]
+
+        secret_key = keeper.get_encryption_key()
+        legacy_cache = Fernet(secret_key).encrypt(b"\x80\x04legacy-pickle-bytes")
+
+        with patch("keeper_secrets_manager_ansible.display.warning") as mock_warn:
+            records = keeper.get_records(uids=["from-vault"], cache=legacy_cache)
+
+        self.assertEqual(len(records), 1)
+        self.assertEqual(records[0].uid, "from-vault")
+        keeper.client.get_secrets.assert_called_once_with(["from-vault"])
+        mock_warn.assert_called_once()
+        self.assertIn("ignored", mock_warn.call_args[0][0])
+
+    def test_get_records_falls_back_to_vault_on_invalid_json_cache(self):
+        keeper = _stub_keeper()
+        vault_record = _make_record(uid="from-vault")
+        keeper.client.get_secrets.return_value = [vault_record]
+
+        secret_key = keeper.get_encryption_key()
+        bad_cache = Fernet(secret_key).encrypt(b'{"not": "a list"}')
+
+        records = keeper.get_records(uids=["from-vault"], cache=bad_cache)
+        self.assertEqual(records[0].uid, "from-vault")
+        keeper.client.get_secrets.assert_called_once_with(["from-vault"])
+
+    def test_get_records_legacy_cache_vault_miss_still_fails(self):
+        """After cache invalidate, missing vault records still raise."""
+        keeper = _stub_keeper()
+        keeper.client.get_secrets.return_value = []
+
+        secret_key = keeper.get_encryption_key()
+        legacy_cache = Fernet(secret_key).encrypt(b"\x80\x04legacy")
+
+        with self.assertRaises(Exception) as ctx:
+            keeper.get_records(uids=["missing-uid"], cache=legacy_cache)
+
+        # Prefer args over str()/ .message — newer ansible-core AnsibleError.__str__
+        # can recurse when formatting the exception message.
+        detail = ctx.exception.args[0] if ctx.exception.args else ""
+        self.assertIn("missing-uid", detail)
+        keeper.client.get_secrets.assert_called_once_with(["missing-uid"])
+
+
+if __name__ == "__main__":
+    unittest.main()