-
Notifications
You must be signed in to change notification settings - Fork 8.1k
fix(security): add HMAC integrity verification to PickleHandler and module allowlist for agent repository imports #6871
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
0c3f622
0cb8d91
c1443aa
3f4968a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,11 @@ | ||
| from datetime import datetime | ||
| import hashlib | ||
| import hmac | ||
| import json | ||
| import os | ||
| import pickle | ||
| import secrets | ||
| import stat | ||
| from typing import Any, TypedDict | ||
|
|
||
| from crewai_core.lock_store import lock as store_lock | ||
|
|
@@ -117,7 +121,11 @@ def log(self, **kwargs: Unpack[LogEntry]) -> None: | |
|
|
||
|
|
||
| class PickleHandler: | ||
| """Handler for saving and loading data using pickle. | ||
| """Handler for saving and loading data using pickle with integrity verification. | ||
|
|
||
| A keyed HMAC-SHA256 signature is written alongside the pickle file on save. | ||
| On load, the signature is verified before deserialization to detect tampering. | ||
| Files without a signature are rejected to prevent loading untrusted data. | ||
|
|
||
| Attributes: | ||
| file_path: The path to the pickle file. | ||
|
|
@@ -135,34 +143,203 @@ def __init__(self, file_name: str) -> None: | |
| file_name += ".pkl" | ||
|
|
||
| self.file_path = os.path.join(os.getcwd(), file_name) | ||
| self._key = self._load_or_create_key() | ||
|
|
||
| @property | ||
| def _sig_path(self) -> str: | ||
| """Path to the HMAC signature file.""" | ||
| return self.file_path + ".sig" | ||
|
|
||
| def _load_or_create_key(self) -> bytes: | ||
| """Load the HMAC key from the user home directory, or create a new one. | ||
|
|
||
| The key is stored in ``~/.crewai/.hmac_key`` with mode 0600 to keep it | ||
| separate from the working directory where pickle files reside. The | ||
| directory and key file are validated for ownership and restrictive | ||
| permissions before use. Key creation uses an exclusive-create flag so | ||
| concurrent processes cannot overwrite each other's key. | ||
|
|
||
| Returns: | ||
| The 32-byte HMAC key. | ||
|
|
||
| Raises: | ||
| OSError: If the key file cannot be created or permissioned. | ||
| PermissionError: If existing key storage has insecure ownership or mode. | ||
| """ | ||
| key_dir = os.path.join(os.path.expanduser("~"), ".crewai") | ||
| key_path = os.path.join(key_dir, ".hmac_key") | ||
|
|
||
| if os.path.exists(key_path): | ||
| if self._validate_key_storage(key_dir, key_path): | ||
| try: | ||
| with open(key_path, "rb") as f: | ||
| key = f.read() | ||
| if len(key) == 32: | ||
| return key | ||
| raise ValueError( | ||
| f"HMAC key file {key_path} exists but has invalid length " | ||
| f"({len(key)} bytes, expected 32). Remove the file to " | ||
| "regenerate, or restore from a valid backup." | ||
| ) | ||
| except OSError: | ||
| pass | ||
| # If validation passed but read failed, fall through to create. | ||
|
|
||
| # Validate directory before first-time key creation: os.makedirs with | ||
| # exist_ok=True does not tighten an existing insecure directory. | ||
| if os.path.exists(key_dir): | ||
| self._validate_key_storage(key_dir, key_path) | ||
| else: | ||
| os.makedirs(key_dir, mode=0o700, exist_ok=False) | ||
|
|
||
| key = secrets.token_bytes(32) | ||
|
|
||
| # Atomic no-clobber creation: O_CREAT|O_EXCL prevents two processes | ||
| # from writing different keys simultaneously. If another process won, | ||
| # load its key instead of using our in-memory copy. | ||
| try: | ||
| fd = os.open(key_path, os.O_CREAT | os.O_EXCL | os.O_WRONLY, 0o600) | ||
| except FileExistsError: | ||
| # Another process created the key between our check and create. | ||
| # Validate and load the installed key. | ||
| if self._validate_key_storage(key_dir, key_path): | ||
| with open(key_path, "rb") as f: | ||
| installed = f.read() | ||
| if len(installed) == 32: | ||
| return installed | ||
| raise ValueError( | ||
| f"HMAC key file {key_path} was created concurrently but has " | ||
| "invalid length. Remove the file to regenerate." | ||
| ) from None | ||
| raise | ||
|
|
||
| try: | ||
| # Write all bytes, handling short writes from the OS. | ||
| offset = 0 | ||
| while offset < len(key): | ||
| offset += os.write(fd, key[offset:]) | ||
| os.fsync(fd) | ||
| finally: | ||
| os.close(fd) | ||
|
|
||
| return key | ||
|
coderabbitai[bot] marked this conversation as resolved.
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| @staticmethod | ||
| def _validate_key_storage(key_dir: str, key_path: str) -> bool: | ||
| """Validate that key storage is owned by the current user and has restrictive permissions. | ||
|
|
||
| Args: | ||
| key_dir: Directory containing the key file. | ||
| key_path: Path to the key file. May not exist yet during first creation. | ||
|
|
||
| Returns: | ||
| True if storage is safe to use. | ||
|
|
||
| Raises: | ||
| PermissionError: If ownership or permissions are insecure. | ||
| """ | ||
| dir_stat = os.stat(key_dir) | ||
|
|
||
| current_uid = os.getuid() | ||
|
|
||
| if dir_stat.st_uid != current_uid: | ||
| raise PermissionError( | ||
| f"HMAC key directory {key_dir} is not owned by the current user" | ||
| ) | ||
|
|
||
| if os.path.islink(key_dir): | ||
| raise PermissionError("HMAC key directory must not be a symlink") | ||
|
|
||
| dir_mode = stat.S_IMODE(dir_stat.st_mode) | ||
|
|
||
| if dir_mode & 0o077: | ||
| raise PermissionError( | ||
| f"HMAC key directory {key_dir} has insecure mode {oct(dir_mode)}; expected 0700" | ||
| ) | ||
|
|
||
| # Validate key file only if it exists (it may not during first creation). | ||
| if os.path.exists(key_path): | ||
| file_stat = os.stat(key_path) | ||
|
|
||
| if file_stat.st_uid != current_uid: | ||
| raise PermissionError( | ||
| f"HMAC key file {key_path} is not owned by the current user" | ||
| ) | ||
|
|
||
| if os.path.islink(key_path): | ||
| raise PermissionError("HMAC key file must not be a symlink") | ||
|
|
||
| file_mode = stat.S_IMODE(file_stat.st_mode) | ||
|
|
||
| if file_mode & 0o077: | ||
| raise PermissionError( | ||
| f"HMAC key file {key_path} has insecure mode {oct(file_mode)}; expected 0600" | ||
| ) | ||
|
|
||
| return True | ||
|
|
||
| def initialize_file(self) -> None: | ||
| """Initialize the file with an empty dictionary and overwrite any existing data.""" | ||
| self.save({}) | ||
|
|
||
| def save(self, data: Any) -> None: | ||
| """ | ||
| Save the data to the specified file using pickle. | ||
| """Save the data to the specified file using pickle with HMAC signature. | ||
|
|
||
| Args: | ||
| data: The data to be saved to the file. | ||
| data: The data to be saved to the file. | ||
| """ | ||
| with store_lock(f"file:{os.path.realpath(self.file_path)}"): | ||
| with open(self.file_path, "wb") as f: | ||
| pickle.dump(obj=data, file=f) | ||
|
|
||
| with open(self.file_path, "rb") as f: | ||
| payload = f.read() | ||
| signature = hmac.new(self._key, payload, hashlib.sha256).digest() | ||
| with open(self._sig_path, "wb") as f: | ||
| f.write(signature) | ||
|
|
||
| def load(self) -> Any: | ||
| """Load the data from the specified file using pickle. | ||
| """Load the data from the specified file with HMAC integrity verification. | ||
|
|
||
| The signature file must exist and match the pickle file's contents. | ||
| Files without a signature are rejected to prevent loading untrusted data. | ||
|
|
||
| Returns: | ||
| The data loaded from the file. | ||
|
|
||
| Raises: | ||
| ValueError: If the signature file is missing or verification fails. | ||
| """ | ||
| if not os.path.exists(self.file_path): | ||
| return {} | ||
|
|
||
| with store_lock(f"file:{os.path.realpath(self.file_path)}"): | ||
| with open(self.file_path, "rb") as file: | ||
| payload = file.read() | ||
|
|
||
| if not os.path.exists(self._sig_path): | ||
| raise ValueError( | ||
| f"Integrity check failed for {self.file_path}: " | ||
| "no signature file found. Re-save the data to generate one." | ||
| ) | ||
|
|
||
| try: | ||
| with open(self.file_path, "rb") as file: | ||
| return pickle.load(file) # noqa: S301 | ||
| except (FileNotFoundError, EOFError): | ||
| return {} | ||
| with open(self._sig_path, "rb") as f: | ||
| stored_sig = f.read() | ||
| except FileNotFoundError: | ||
| raise ValueError( | ||
| f"Integrity check failed for {self.file_path}: " | ||
| "signature file disappeared during loading." | ||
| ) from None | ||
|
Comment on lines
326
to
+333
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Exercise the disappearing-signature handler.
Return 🧰 Tools🪛 ast-grep (0.45.0)[warning] 326-326: File path is request-/variable-derived; validate and normalize to prevent path traversal. (open-filename-from-request) 🤖 Prompt for AI Agents |
||
|
|
||
| expected_sig = hmac.new(self._key, payload, hashlib.sha256).digest() | ||
|
|
||
| if not hmac.compare_digest(stored_sig, expected_sig): | ||
| raise ValueError( | ||
| f"Integrity check failed for {self.file_path}: " | ||
| "signature mismatch - file may have been tampered with" | ||
| ) | ||
|
|
||
| import io | ||
|
|
||
| return pickle.load(io.BytesIO(payload)) # noqa: S301 | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle the first-use directory race.
If two processes initialize key storage at the same time, both can observe that
key_diris absent. One process creates it. The other process raisesFileExistsErrorat Line 193 before the key-file race handling at Line 202 can run.Catch this
FileExistsError, validate the directory, and continue to the existing no-clobber key creation path. Add a multi-process test that starts without~/.crewai.Proposed fix
if os.path.exists(key_dir): self._validate_key_storage(key_dir, key_path) else: - os.makedirs(key_dir, mode=0o700, exist_ok=False) + try: + os.makedirs(key_dir, mode=0o700, exist_ok=False) + except FileExistsError: + self._validate_key_storage(key_dir, key_path)📝 Committable suggestion
🤖 Prompt for AI Agents