CatchTheTornado / CatchTheTornado/text-extract-api
[SECURITY] Path traversal via storage_profile lets an unauthenticated caller read arbitrary .yaml files and repoint the storage root
- Dominant language
- Python
- Stars
- 3.2k
- Forks
- 279
- PR merge metrics
- No merged PRs in 30d
Description
Hey — I was looking at the storage profile handling while reviewing #119 (the traversal fix in `LocalFilesystemStorageStrategy`) and noticed the same class of bug is still reachable through a different parameter: `storage_profile`. The July fix covers `file_name` really well, but `StorageManager.__init__` still joins the profile name straight into a filesystem path.
**Where it is**
`text_extract_api/files/storage_manager.py`, line 20:
```python
profile_path = os.path.join(os.getenv('STORAGE_PROFILE_PATH', '/storage_profiles'), f'{profile_name}.yaml')
with open(profile_path, 'r') as file:
self.profile = yaml.safe_load(file)
```
There's no basename/allowlist on `profile_name`, so `../` sequences walk out of the profiles directory.
The three storage endpoints in `main.py` (around lines 197–224) pass the query param straight in, and none of them have any auth:
- `GET /storage/list?storage_profile=...`
- `GET /storage/load?file_name=...&storage_profile=...`
- `DELETE /storage/delete?file_name=...&storage_profile=...`
**What you get**
Two steps, both unauthenticated:
1. First hit — file existence oracle. A `storage_profile` with `../` in it makes the server try to open YAML files outside the profiles dir. In my test run the traceback shows it literally attempting to open `storage_profiles_win\..\..\..\..\outside_secrets\leaked.yaml`, i.e. the path escaped the base directory. A 500 vs a different error tells you whether the `.yaml` exists.
2. The bigger problem — if the YAML you land on parses (any yaml the attacker can influence, or a config that already exists on the box), its `strategy:` and `settings.root_path:` values are attacker-controlled. `LocalFilesystemStorageStrategy` is happy to set its `base_directory` from that `root_path`, and then `/storage/load?file_name=...` reads files under whatever directory the attacker picked.
Verified end to end on current master (a78eebf) with a small profile dropped outside the profiles dir:
```
GET /storage/load?file_name=kube_config.yaml&storage_profile=../outside_secrets/evil_profile
```
response:
```json
{"content":"DB_PASSWORD: sup3rs3cret-POC\nAWS_KEY: AKIA-POC-1234\n"}
```
That's an arbitrary file read via a single unauthenticated GET, plus `DELETE /storage/delete` gives file deletion under the same attacker-chosen root. On a k8s deployment the obvious targets are manifests and anything ending in .yaml that holds credentials.
**One more thing — the validator on the ocr side**
`storage_profile_exists()` in `main.py` (lines 22–29) doesn't actually validate anything:
```python
profile_path = os.path.abspath(os.path.join(...f'{profile_name}.yaml'))
if not os.path.isfile(profile_path) and profile_path.startswith('..'):
...
return os.path.isfile(sub_profile_path)
return True
```
Since `abspath()` is applied first, `profile_path` is always absolute and never starts with `..` — that branch is dead code. The function returns `True` for everything, including profiles that don't exist, so the `/ocr` endpoints take traversal payloads too. Confirmed by calling it directly:
```
storage_profile_exists('../outside_secrets/evil_profile') -> True
storage_profile_exists('does_not_exist') -> True
```
**Suggested fix**
Same treatment `file_name` got in 9a6413f, applied to the profile name:
```python
name = os.path.basename(profile_name)
if not re.fullmatch(r'[A-Za-z0-9_.-]+', name):
raise ValueError('Invalid storage profile name')
```
and for `storage_profile_exists()`, dropping the dead fallback and just `return os.path.isfile(profile_path)` would make it do what it looks like it's meant to do.
Happy to send a PR with tests if that's useful — and since there's no SECURITY.md on the repo I'm posting here, but happy to move this to a private channel (GitHub advisory form works too) if you'd rather keep details out of public view until a patch lands.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start in text_extract_api/files/storage_manager.py and inspect the storage endpoints around lines 197–224 and storage_profile_exists() around lines 22–29 in main.py. Reproduce the traversal and nonexistent-profile cases, then add regression tests covering profile-name validation and file existence. Done means traversal cannot escape the profile directory, invalid profiles are rejected, and the storage endpoints no longer enable arbitrary reads or deletion.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- api, backend, security
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 78/100