arc53 / arc53/DocsGPT

πŸ› Bug Report: Stored credential encryption lacks integrity protection

Open
#2,503 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.