haylan/AeonDumpManager
1
0
Fork 0
Code Issues Pull Requests Actions Packages Releases Wiki Activity

docs: add dump manager research notes

Browse Source
This commit is contained in:
haylan
2026-09-14 22:00:26 +02:00
parent 0dc19897a4
commit b4e84d0570
2 changed files with 315 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/research/dump-binary-config.md
+178
View File
@@ -0,0 +1,178 @@
# Research: Admin-configurable dump binary path — is it an attack surface?
Target: `src/Service/DumpBinaryLocator.php`'s `dumpBinaryPath` config field and how it feeds
`src/Service/DumpService.php::buildProcess()`.
## Summary / Recommendation
- **Not a shell-injection vector.** `Process` built from an array (`new Process([$binary, ...])`)
never goes through a shell — PHP's `proc_open()` execs the array directly, so whatever string sits
in `$binary` is passed straight to `exec()` as the program path. There's no shell metacharacter
parsing to exploit via that string's *content* (§1).
- **The real concern is authorization scope, not injection**: `dumpBinaryPath` is whatever
`SystemConfigService::getString()` returns, gated only by Shopware's `system_config:read`/`:update`
ACL privilege — a privilege that can be granted independently of shell/SSH access to the box. A
role with system-config write access but *no* shell access can currently point this field at any
executable already reachable on disk and have the plugin run it as `www-data`. That's a real,
if narrow, privilege-escalation surface for this one specific plugin — not a "trusts itself" non-issue,
because system-config-write and filesystem/shell access are not the same trust tier in Shopware's ACL
model (§1).
- **Recommendation for this plugin**: drop `dumpBinaryPath` (arbitrary absolute path) in favor of a
`dumpBinaryName` override, resolved through `ExecutableFinder::find($name)` exactly like the
auto-detected default — never returned as a raw path straight from config. This closes the
arbitrary-path surface (an admin can only ever get a `PATH`-resolved binary, never point at
`/etc/shadow`-adjacent binaries or an arbitrarily uploaded file) while still covering the one
legitimate reason to override at all: a distro/environment that names the client something
nonstandard. Given dockware ships `mysqldump` on `PATH` already (confirmed in the prior research doc,
`.scratch/admin-dump-manager/issues/01-detect-dump-binary.md` §3), the override will sit unused in
the common case — which is fine, it's a two-line fallback, not new infrastructure (§2, §3).
- An env-var override was considered and rejected as the *primary* mechanism for this plugin — it's
the right shape for ops-owned infra config, but this plugin already has a config-field UI and
`SystemConfigService` wired in for `maxDumps`/`retantionDays`; adding a second override channel
(env var) alongside the DB-stored one is more moving parts than the problem needs (§2, §3).
## 1. Is there a real security concern?
`DumpService::buildProcess()` builds the command as an array:
```php
$command = [$binary, '--single-transaction', '--quick', '-h', ..., '-u', ..., $dbname];
$process = new Process($command, null, ['MYSQL_PWD' => $password]);
```
Symfony's own docs are explicit that array form is the safe form and *why*:
> "Using an array of arguments is the recommended way to define commands. This saves you from any
> escaping and allows sending signals seamlessly." — and contrasts it with `Process::fromShellCommandline()`,
> where "it becomes your responsibility to deal with escaping and portability."
> [Symfony Process component docs](https://symfony.com/doc/current/components/process.html)
Mechanically: `Process` (array constructor) hands the array straight to PHP's `proc_open()`, which —
given an array `command` argument — execs the binary directly (no `/bin/sh -c` wrapping), per PHP's
own `proc_open()` behavior for array-form commands (bypasses the shell entirely on Linux since PHP
7.4). So `$binary`'s *string content* can't be used to inject additional shell commands (no `;`, `` ` ``,
`$()`, pipes, etc. get interpreted) — the entire "shell injection via a crafted path string" concern
the user was worried about does not apply to this code as written. This holds **regardless of whether
`$binary` comes from `ExecutableFinder::find()` or directly from `SystemConfigService::getString()`** —
both are just strings landing in the same array slot.
**So what is the actual concern?** Not injection — authorization scope. `dumpBinaryPath` is read via:
```php
$configuredPath = $this->systemConfigService->getString('AeonDumpManager.config.dumpBinaryPath');
if ($configuredPath !== '') {
return $configuredPath;
}
```
This value is *whatever an admin with write access to this plugin's store-config typed*, used
**verbatim** as the program to exec — no `is_executable()` check, no restriction to a known directory,
no validation at all beyond non-empty. Setting it, in Shopware's ACL model, requires the
`system_config:read`/`system_config:update` privilege (or a role with `administration` access broadly).
That privilege is not the same tier as "has a shell on the container" — Shopware routinely grants
narrower admin roles (e.g. a merchandising-ops role scoped to a handful of modules including this
plugin's settings card) without granting SSH/exec access to the underlying host. For *this specific
field*, anyone who can write plugin config can make the plugin exec an arbitrary path on disk as
`www-data` — e.g. pointing it at any already-present script/binary, or (in a shared-hosting-style setup)
a path they can write to via some unrelated upload feature. That's a real, if narrow and
low-severity-in-practice (dev-tooling plugin, dockware-only target per `CLAUDE.md`), escalation: it
turns "can edit this plugin's config" into "can execute an arbitrary path as the PHP process user,"
which is a strictly larger capability than the plugin should need to grant. It is not an
*unauthenticated-attacker* vector — Shopware's admin panel is authenticated and this requires a
specific privilege — but "only a privileged admin can trigger it" doesn't make it a non-issue when the
privilege in question (system-config write) is deliberately a narrower grant than shell access in
Shopware's own permission model.
## 2. Alternatives, evaluated
### a. Drop the field entirely, `ExecutableFinder::find()` only, fail loudly
Simplest option. `ExecutableFinder::find($name)` (confirmed against `symfony/process` source,
`ExecutableFinder.php`, current branch) searches, in order:
1. Directories from the `PATH` env var (`getenv('PATH')`, or `Path` on Windows), merged with any
`$extraDirs` argument.
2. For each directory, `is_file($dir/$name$suffix) && is_executable(...)` (suffix loop is a no-op on
Linux; Windows-only `PATHEXT` extensions).
3. As a last resort on non-Windows, shells out to `command -v -- $name` (via `escapeshellarg()`) —
this is the *only* place `ExecutableFinder` itself touches a shell, and it shell-escapes the binary
*name* being searched for, not a path being executed.
[`ExecutableFinder.php` — symfony/process, GitHub](https://github.com/symfony/process/blob/7.1/ExecutableFinder.php)
Pro: zero config-surface, nothing to misconfigure. Con: on the (rare, per the linked prior research)
case where `mysqldump`/`mariadb-dump` genuinely isn't on `PATH` for `www-data`, there's no way to
recover without an env var or code change — for a plugin whose whole job is producing dumps, a hard
dependency on `PATH` composition an admin can't influence from the plugin's own UI is a mildly awkward
dead end.
### b. Binary *name* override (`dumpBinaryName`), still resolved via `ExecutableFinder::find()`
```php
$configuredName = $this->systemConfigService->getString('AeonDumpManager.config.dumpBinaryName');
$binaryNames = $configuredName !== ''
? [$configuredName]
: ($this->isMariaDb() ? ['mariadb-dump', 'mysqldump'] : ['mysqldump']);
foreach ($binaryNames as $binaryName) {
$path = $this->executableFinder->find($binaryName);
if ($path !== null) {
return $path;
}
}
```
This is a ~3-line diff from the current code — same shape, same loop, just the value coming out of
config is *always* passed through `find()` rather than ever being returned directly. Every path
`buildProcess()` can end up executing is a `PATH`-resolved, `is_executable()`-checked file — an admin
can redirect *which name* gets looked up (covers a renamed/nonstandard binary on an unusual
distro/environment) but can never point the plugin at an arbitrary filesystem path outside `PATH`.
This directly closes the gap in §1 while preserving the one legitimate override use case the original
field existed for.
### c. Environment variable override (`getenv('AEON_DUMP_BINARY_PATH')` or similar)
Moves the override from "whoever has Shopware admin-config write access" to "whoever controls the
container/deployment env" (ops/devops, via `docker-compose.yml`, `.env`, or the container's process
environment) — a meaningfully different, generally higher, trust tier, and consistent with how
`docker-compose.yml` already configures this repo's dev container (`SSH_USER`/`SSH_PWD` env vars per
`CLAUDE.md`). This is a legitimate pattern in general. For *this* plugin specifically it's more
machinery than the problem justifies: it adds a second config source (env var) next to the existing
`SystemConfigService`-backed admin UI field, for a value that (per §3) essentially never needs
overriding in the actual target environment (dockware). It's the right tool if this were, say, a
plugin deployed across heterogeneous customer infrastructure where ops teams routinely differ from
Shopware-admin teams — not proportionate for a project-local dev-tooling plugin with one target
container image.
## 3. Recommendation for this plugin
Use **(b)** — replace `dumpBinaryPath` (text, absolute path) with `dumpBinaryName` (text, binary name,
default empty → falls back to the existing MariaDB/MySQL name-detection list) in
`src/Resources/config/config.xml`, and change `DumpBinaryLocator::locate()` to prepend the configured
name (if any) to the existing `$binaryNames` loop instead of short-circuiting with a raw path. This is
sized right for a dev-tooling plugin that per the prior research (`.scratch/.../01-detect-dump-binary.md`
§3) targets dockware, where `mysqldump` is already confirmed present on `PATH` — the override becomes
a rarely-touched escape hatch rather than the primary mechanism, and it removes the one place in this
plugin where admin-supplied config was executed as a program path with no validation. Skip (c)/env-var:
it solves a deployment-heterogeneity problem this single-container plugin doesn't have, and would be a
second override mechanism doing the same job as the config field, for no concrete gain here. Skip
(a)/no-override-at-all: it's the smallest diff but removes a genuine, if narrow, recovery path for a
`PATH`-less edge case at effectively zero cost to keep (b) instead — (b) is one loop-entry away from (a)
and already sits inside the existing `foreach ($binaryNames as $binaryName)` structure in
`DumpBinaryLocator::locate()`.
No code changes were made as part of this research task — see
`src/Service/DumpBinaryLocator.php`, `src/Service/DumpService.php::buildProcess()`, and
`src/Resources/config/config.xml` for the current state to build (b) against.
## Sources
- [Symfony Process component docs — command arrays vs `fromShellCommandline()`](https://symfony.com/doc/current/components/process.html)
- [`ExecutableFinder.php` — symfony/process, GitHub (7.1 branch)](https://github.com/symfony/process/blob/7.1/ExecutableFinder.php)
- PHP manual, `proc_open()` — array-form `command` execs directly without a shell wrapper (documented
behavior since PHP 7.4 for the array-command form on non-Windows).
- `src/Service/DumpBinaryLocator.php`, `src/Service/DumpService.php` (this repo, current state).
- `.scratch/admin-dump-manager/issues/01-detect-dump-binary.md` (prior locked research: dockware ships
`mysqldump` on `PATH`, MariaDB binary-naming history, `ExecutableFinder` recommendation).
- `.scratch/dump-manager-implementation-spec/spec.md` §0, §2.2 (locked design context for the
`dumpBinaryPath` field and `DumpBinaryLocator` shape being reconsidered here).