π Bug Report: Stored credential encryption lacks integrity protection
- Dominant language
- Python
- Stars
- 18.3k
- Forks
- 2.1k
- Avg merge
- 1d 2h
- Merged PRs (30d)
- 29
Description
### π Description
The pre-PR implementation of `application/security/encryption.py` encrypted stored credentials with AES-CBC and custom padding, but did not authenticate the ciphertext, IV, salt, or user context with an integrity tag.
As a result, a party that can modify a stored encrypted credential blob can make controlled changes to decrypted plaintext without the application detecting tampering. This can affect API tokens and connector credentials stored through this helper.
PR #2331 migrates new credential encryption to AES-GCM and binds `user_id` as AAD. The PR branch also keeps a legacy CBC fallback for backward compatibility, so existing CBC ciphertexts should be rotated or re-encrypted after deployment.
Affected component:
- `application/security/encryption.py`
- `encrypt_credentials(credentials: dict, user_id: str)`
- `decrypt_credentials(encrypted_data: str, user_id: str)`
### π Reproduction steps
1. Check out the vulnerable baseline before PR #2331, for example local `main` at commit `06e827573cac6ad3b445ffea0fb08169071c1b50`.
2. Set only the test environment variable name `ENCRYPTION_SECRET_KEY` to a controlled test value.
3. Encrypt a synthetic credential object such as `{"a": "AAAA", "b": 1}` for `user-123`.
4. Base64-decode the stored blob.
5. Flip byte 7 of the IV with `ord("A") ^ ord("Z")`.
6. Base64-encode the modified blob and call `decrypt_credentials(..., "user-123")`.
7. Observe that the decrypted object changes to `{"a": "ZAAA", "b": 1}` instead of failing authentication.
8. Check out PR #2331 head `5ca9a64e9eae6a7423b667bdeae9386200355478`.
9. Repeat tampering against a newly encrypted GCM blob.
10. Observe that the decrypt path returns `{}` for tampered GCM ciphertext and `{}` for cross-user replay.
Local validation command used:
```shell
PYTHONPATH=/home/ubuntu/summer/agentscan-repos/repos/222 /tmp/docsgpt-pr2331-venv/bin/python -m pytest tests/security/test_encryption.py -q -o addopts=''
```
Result:
```shell
11 passed in 0.56s
```
Controlled PoC result:
```shell
legacy_pre_pr_targeted_tamper_result= {'a': 'ZAAA', 'b': 1}
fixed_gcm_tamper_result= {}
fixed_gcm_cross_user_replay_result= {}
fixed_format_version_byte= 1
### π Expected behavior
Credential ciphertext should be authenticated. Any modification to the salt, nonce/IV, ciphertext, tag, or user binding should fail decryption and return no credential data.
Newly encrypted credentials should use an AEAD mode such as AES-GCM. Legacy CBC credentials should be migrated or rotated so the stored data is no longer accepted without an integrity check.
### π Actual Behavior with Screenshots
In the vulnerable baseline, AES-CBC decryption accepts modified ciphertext when the resulting plaintext is syntactically valid JSON. There is no authentication tag, so targeted IV manipulation can alter plaintext bytes in the first decrypted block without detection.
No screenshots are applicable because this is a backend cryptographic integrity issue.
### π» Operating system
Linux
### What browsers are you seeing the problem on?
Something else
### π€ What development environment are you experiencing this bug on?
Local dev server
### π Did you set the correct environment variables in the right path? List the environment variable names (not values please!)
Only environment variable names used during validation:
- `ENCRYPTION_SECRET_KEY`
No secret values are included in this report.
### π Provide any additional context for the Bug.
PR #2331:
- `https://github.com/arc53/DocsGPT/pull/2331`
PR metadata observed during validation:
- Title: `security: migrate AES-CBC to AES-GCM authenticated encryption`
- State: `open`
- Head branch: `fix/aes-gcm-authenticated-encryption`
- Head commit: `5ca9a64e9eae6a7423b667bdeae9386200355478`
- Modified files:
- `application/security/encryption.py`
- `tests/security/test_encryption.py`
Security policy note: `SECURITY.md` asks vulnerability reporters to email `security@arc53.com`. This file is prepared in the repository's bug report format, but the project security policy should be followed before public disclosure.
### π Relevant log output
```shell
legacy_pre_pr_targeted_tamper_result= {'a': 'ZAAA', 'b': 1}
fixed_gcm_tamper_result= {}
fixed_gcm_cross_user_replay_result= {}
fixed_format_version_byte= 1
```
### π Have you spent some time to check if this bug has been raised before?
- [x] I checked and didn't find similar issue
### π Are you willing to submit PR?
Yes, I am willing to submit a PR!
### π§ββοΈ Code of Conduct
- [x] I agree to follow this project's Code of Conduct
Contributor guide
Research direction
Start with application/security/encryption.py and the related tests in tests/security/test_encryption.py; run the stated pytest command to establish the current behavior. Compare the vulnerable baseline with PR #2331, and consider the work complete when tampering with credential data or user context is rejected while legacy handling and the documented test coverage remain correct.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- backend, security
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Clearly specified
- Newbie friendliness
- 20/100