richardcocks opened a new issue, #4058:
URL: https://github.com/apache/iggy/issues/4058
### Bug description
Note: There is no path traversal due to a filename prefix, so as per
`SECURITY.md`, "missing hardening or defence-in-depth suggestions with no
accompanying exploit" is out of scope for private disclosure. I have been
unable to find any exploit angle despite initially looking like it could be a
path traversal exploit.
---
## Bug description
The connectors runtime control API takes the connector `key` from the
request path and uses it,
unvalidated, to build a filesystem path for the connector's config file. A
`key` containing path
separators or `..` is written straight into the target path.
**What I was doing:** reviewing the local config provider's write path for
the connector control API
(`POST /sources/{key}/configs` and the `/sinks/{key}/configs` equivalent).
**What I expected:** the `key` is validated (or the resulting path is
confined to `config_dir`) before
any filesystem operation.
**What actually happens:** there is no validation anywhere. The path
parameter flows verbatim into the
config filename:
- `create_source_config(&key, …)` —
`core/connectors/runtime/src/api/source.rs:158`
- → `cmd.to_source_config(key, next_version)` sets `config.key = key`
- → `ConnectorId::to_filename_key()` = `format!("{}_{}", self.key,
self.version)` —
`core/connectors/runtime/src/configs/connectors/local_provider.rs:41`
- → `format!("{}/source_{}.toml", self.config_dir,
connector_id.to_filename_key())` —
`local_provider.rs:445`
- → `std::fs::write(&path, …)` — `local_provider.rs:450` (sink equivalent at
`:411`/`:415`)
There appears to be no `..`/separator check, no path canonicalization, and
no validation
on this path. The `key` is fully controlled by anyone who can post to the
connectors API.
( It is also unclear to me if that is only trusted users, which further
reduces and removes any security impact. )
### Impact
Attempting the classic traversal does not escape `config_dir`:
1. **Filename-prefix fusion.** The template welds a literal `source_` (or
`sink_`) onto the first
token of the key: `source_` + `../../evil` → `source_../../evil`, whose
first path component is
`source_..` — an ordinary (nonexistent) directory name, not `..`. The
leading traversal step is
dead on arrival.
2. **No intermediate-dir creation.** `std::fs::write` does not create parent
directories, so any
multi-level `..` climb needs a real, traversable directory under
`config_dir` to stand on. There
is none by default (config_dir holds only `*.toml` files), and I
confirmed by sweeping every
`create_dir*` call in the tree that no runtime code path creates a
`key`-named directory under
`config_dir`. The only directories the config/state planes create are
`config_dir` and
`state_path` themselves, both from operator config. `delete_*_config`
removes a stored path from
its in-memory map, not a key-derived path, so it is not a vector either.
So the traversal has no working escape. This safety could be unintentionally
removed in a refactor:
It breaks if either (a) an operator places any subdirectory under
`config_dir`, or (b) a refactor reorders the template (key-first, or a per-key
subdir like `config_dir/{key}/config.toml`) or adds
`create_dir_all(path.parent())` before the write.
### Threat model / scope
- The control API is **unauthenticated by default** (`api_key = ""`,
`core/connectors/runtime/config.toml:21`;
the auth middleware early-returns when the key is empty,
`core/connectors/runtime/src/api/auth.rs:41`).
- Connector plugins are **fully trusted** by design: they are `dlopen`'d
native code
(`unsafe { Container::<SinkApi>::load(&path) }`,
`core/connectors/runtime/src/sink.rs:98`) called
in-process via FFI, with no sandbox (no seccomp/landlock/chroot/rlimit
anywhere in the runtime). A
malicious plugin already has full process privileges, so this issue is
irrelevant to a
plugin-deploying attacker.
- The only adversary for whom this matters is one who can reach the control
API but **cannot** deploy
a plugin (e.g. control API exposed on the network, plugin directory locked
down / plugins vetted).
This is an input-validation gap with no demonstrated escape.
### Suggested fix
Validate `key` at the API boundary (and defensively in `create_*_config`)
before it touches the
filesystem: reject any key that is not `^[A-Za-z0-9._-]+$` (equivalently:
contains `/`, `\`, `..`, a
path separator, or a NUL), returning `400 Bad Request`. This mirrors the
input validation the local
provider already performs elsewhere and removes the reliance on accidental
filename-prefix fusion.
### Affected area / component
Connectors
### Deployment
Compiled from source
### Versions
Server `master` @ `be2265c9e` (0.11 edge line). Component: connectors
runtime, local config provider.
### Hardware / environment
_No response_
### Sample code
With a connectors runtime running its control API locally (default config,
empty `api_key`):
```bash
API=http://127.0.0.1:8081
H='content-type: application/json'
BODY='{"enabled":false,"name":"x","path":"/tmp/evil.so","streams":[]}'
# key contains encoded "../" — captured as a single path segment, then
percent-decoded into the key
curl -s --path-as-is -o /dev/null -w '%{http_code}\n' \
-X POST -H "$H" -d "$BODY" "$API/sources/..%2F..%2Fpwned/configs"
# descend-then-climb variant
curl -s --path-as-is -o /dev/null -w '%{http_code}\n' \
-X POST -H "$H" -d "$BODY"
"$API/sources/x%2F..%2F..%2F..%2F..%2Ftmp%2Fpwn/configs"
```
The write target resolves to a nonexistent intermediate directory and fails
(ENOENT → `500`).
### Logs
_No response_
### Iggy server config
_No response_
### Reproduction
_No response_
### Contribution
- [ ] I'm willing to submit a pull request to fix this bug
### Good first issue
- [ ] I think this could be a good first issue for a new contributor
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
