encrypt user-uploaded payloads and decrypt on read#3361
Conversation
|
Azure Pipelines: 1 pipeline(s) were filtered out due to trigger conditions. |
|
|
1 similar comment
|
|
❌ The last analysis has failed. |
1 similar comment
|
❌ The last analysis has failed. |
deacon-mp
left a comment
deacon-mp
left a comment
There was a problem hiding this comment.
Two structural problems with the encryption design that I'd want resolved before this lands:
1. The AES key is written inside the same file as the ciphertext.
The file layout is flag || key || iv || ciphertext. Anyone who can read the file recovers the key in the same read and decrypts trivially. Against the threat model that motivates encrypting payloads at rest (an attacker who reads the filesystem), this provides zero confidentiality — it's obfuscation, not encryption.
The right primitive is already in the codebase: Caldera has encryption_key in config + a Fernet encryptor (app/utility/file_decryptor.py, app/service/file_svc.py:_get_encryptor). Use that — the server already protects encryption_key and rotating it rotates all payload encryption. Per-file random keys only help if those keys live somewhere the attacker can't reach (KMS, separate keystore, etc.); kept inline they're noise.
2. Circular import.
app/service/file_svc.py now imports from app/api/v2/handlers/payload_api.py. That inverts the layering — services don't import handlers — and will break in practice depending on module load order. Move the encryption flag / constants to app/utility/ so both sides can import them without cycle.
3. (Minor) Response semantics change is out-of-scope.
delete_payloads was changed from return web.HTTPNoContent() to raise web.HTTPNoContent(). That's a different code path — raise triggers the response middleware, which #3304 (if it merges) will then treat as an exception. Worth splitting into its own PR to keep the review surface small.
Requesting changes; happy to revisit after the keying model is reworked.
Regarding number 1, the purpose of this PR isn't to secure the payload contents in the sense that no one else can access them. The purpose is to prevent unintended execution of files on disk (e.g. python scripts) by modifying the contents on disk, but making sure the original contents are still delivered on file reads and payload downloads. So there's no issue in storing the key along with the file. Regardless, an attacker who has access to the file system already wins no matter what since they can just modify or execute the caldera python functions to get the original contents anyway. Plus the caldera encryption key in the config isn't currently protected and is readable to anyone who has access to the config, which is no different than the case here. We do not use that key here because if that key were to change (e.g. users run caldera with a different config file or update the key manually), the user-provided payloads would no longer be accessible. Number 2 is valid and will be addressed, either by putting the flag directly in the file svc python file or in a separate utility as suggested Number 3: these changes were done because of Python warnings from unit tests, and the warnings themselves actually recommended the change. However, I can revert them for now and save them for a separate PR to maintain scope |
|
|
❌ The last analysis has failed. |
2 participants
Description
Encrypt user-provided payloads on disk in
data/payloadsand have the file_svc decrypt them in memory when reading from disk. This avoids attack vectors that attempt to import user-provided python modules from disk.The encryption methodology generates a random 32-byte AES key and 16-byte IV for each payload and prepends the ciphertext with an encryption flag (marking the file as an encrypted user-provided payload and distinguishing it from typical CALDERA-encrypted files), the key, and IV. The key is embedded in the ciphertext blob on disk because the purpose is not to secure the payload contents but rather to prevent the files on disk from being directly executed. Keys are generated separately from the caldera configuration to allow user-provided payloads to still be accessible if the crypt/salt values in the caldera config change for any reason.
Existing user payloads that were uploaded prior to this change will not be affected
Also adds additional payload API unit test coverage and addresses some warnings
Type of change
How Has This Been Tested?
Unit tests, tested with local caldera installation (uploaded payloads via curl and UI)
Checklist: