AeonDumpManager/docs/research/dump-binary-config.md

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:

$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

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:

$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

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()

$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()
• ExecutableFinder.php — symfony/process, GitHub (7.1 branch)
• 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).