Skip to content

Commit c86200f

Browse files
Alexey PortnovAlexey Portnov
Alexey Portnov
authored and
Alexey Portnov
committed
fix(security): close 6 remaining command injection vectors in PBXCoreREST
Follow-up to 50dab8e which patched DownloadNewFirmwareAction and ConvertAudioFileAction but missed these six shell-interpolation sinks. 1. Modules/StartDownloadAction: validate moduleUniqueID against /^[A-Za-z0-9_\-]{1,128}$/, validate URL (http/https + filter_var), validate md5 (32 hex), escapeshellarg on mwExecBg path argument. This is the 1-to-1 twin of DownloadNewFirmwareAction (CVE-2025-52207). 2. Modules/UninstallModuleAction: validate moduleUniqueId with the same whitelist; completely rewrite the kill-block that used a nested $() subshell ("$kill -9 $($lsof ... | $awk '{print $2}' | $uniq)") — now runs lsof via escapeshellarg, parses PIDs in PHP, kills each via posix_kill(SIGKILL); escapeshellarg on "rm -rf $currentModuleDir". 3. Modules/ModuleInstallationBase: replace fragile single-quoted '$settings_file' with escapeshellarg($settings_file) in mwExecBg. 4. System/UpgradeFromImageAction: new confineToUploadDir() helper with realpath() prefix match rejects any imageFileLocation outside the configured upload directory; whitelist /dev/[A-Za-z0-9/_-]+ on bootPartition; escapeshellarg on mount/umount/gunzip/parted commands. 5. SysLogs/PrepareLogAction: escapeshellarg on $settings_file in mwExecBg (defence in depth — $prefix is currently bool-derived). 6. Files/UploadFileAction: sanitize resumableIdentifier via whitelist with strict equality rejection; cast resumableChunkNumber to int; sanitize file extension from pathinfo() to [A-Za-z0-9]{0,16} — closes both shell injection via crafted extension and a FORBIDDEN_EXTENSIONS bypass where "hack.php;xyz" would parse as extension "php;xyz", slip the blacklist, then land shell metacharacters in downstream paths; apply the same extension normalization inside validateFileType() so the blacklist operates on the cleaned value; escapeshellarg on $chunks_dest_file and $settings_file in mwExec/mwExecBg. Verified on boffart.miko.ru (2026.1.233-dev) with PoCs against each endpoint under a real admin JWT: - Evil uniqid in modules:startDownload body → 400 "Invalid module id" - Bad URL / bad md5 → 400 with specific message - Clean module download → 200 DOWNLOAD_IN_PROGRESS - Path traversal in system:upgrade (/etc/passwd) → 422 - Outside-uploadDir path → 422 - Evil resumableIdentifier (;touch, ../../, %27, %00) → rejected - Evil extension (hack.php;xyz, hack.PhP) → 422 "Forbidden extension" - Clean .wav upload → 200 MERGING - No shell marker files created in any PoC
1 parent 3fae6ce commit c86200f

6 files changed

Lines changed: 182 additions & 23 deletions

File tree

‎src/PBXCoreREST/Lib/Files/UploadFileAction.php‎

Lines changed: 34 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,26 @@ public static function main(array $parameters): PBXApiResult
160160
$res->messages['error'] = $validationResult['error'];
161161
return $res;
162162
}
163+
// Security: resumableIdentifier is HTTP-attacker-controlled and will
164+
// become part of filesystem + shell paths (via $chunks_dest_file,
165+
// $settings_file). Restrict it to a safe charset before any
166+
// interpolation. Empty / malformed values get a deterministic fallback
167+
// so a crafted request cannot target arbitrary directories.
168+
$rawIdentifier = (string)($parameters['resumableIdentifier'] ?? '');
169+
$safeIdentifier = preg_replace('/[^A-Za-z0-9._\-]/', '', $rawIdentifier);
170+
if ($safeIdentifier === '' || $safeIdentifier !== $rawIdentifier) {
171+
// Rejecting is safer than silently rewriting because the client
172+
// uses the identifier to resume/track the upload.
173+
$res->success = false;
174+
$res->messages['error'] = 'Invalid resumableIdentifier';
175+
return $res;
176+
}
177+
$parameters['resumableIdentifier'] = $safeIdentifier;
178+
179+
// Also coerce the chunk number to an integer — it participates in
180+
// shell paths below and must not carry metacharacters.
181+
$parameters['resumableChunkNumber'] = (int)($parameters['resumableChunkNumber'] ?? 0);
182+
163183
$parameters['uploadDir'] = $di->getShared('config')->path('www.uploadDir');
164184
$parameters['tempDir'] = "{$parameters['uploadDir']}/{$parameters['resumableIdentifier']}";
165185
if (!Util::mwMkdir($parameters['tempDir'])) {
@@ -176,6 +196,10 @@ public static function main(array $parameters): PBXApiResult
176196
$fileName = '' . md5(microtime()) . '-' . $fileName;
177197
}
178198
$extension = (string)pathinfo($parameters['resumableFilename'], PATHINFO_EXTENSION);
199+
// Security: extension from pathinfo() is attacker-controlled too
200+
// (e.g. "rce;touch /tmp/pwn" parses as extension "rce;touch /tmp/pwn").
201+
// Limit to a conservative alnum set and cap the length.
202+
$extension = substr(preg_replace('/[^A-Za-z0-9]/', '', $extension), 0, 16);
179203
$fileName .= '.' . $extension;
180204
$parameters['resumableFilename'] = $fileName;
181205
$parameters['fullUploadedFileName'] = "{$parameters['tempDir']}/$fileName";
@@ -276,7 +300,7 @@ private static function moveUploadedPartToSeparateDir(array $parameters, array $
276300
. "/{$parameters['resumableFilename']}.part{$parameters['resumableChunkNumber']}";
277301
if (file_exists($chunks_dest_file)) {
278302
$rm = Util::which('rm');
279-
Processes::mwExec("$rm -f $chunks_dest_file");
303+
Processes::mwExec("$rm -f " . escapeshellarg($chunks_dest_file));
280304
}
281305
$file->moveTo($chunks_dest_file);
282306

@@ -316,7 +340,9 @@ private static function tryToMergeChunksIfAllPartsUploaded(array $parameters): b
316340
// We will start the background process to merge parts into one file
317341
$php = Util::which('php');
318342
$workerFilesMergerPath = Util::getFilePathByClassName(WorkerMergeUploadedFile::class);
319-
Processes::mwExecBg("$php -f $workerFilesMergerPath start '$settings_file'");
343+
Processes::mwExecBg(
344+
"$php -f $workerFilesMergerPath start " . escapeshellarg($settings_file)
345+
);
320346

321347
return true;
322348
}
@@ -357,7 +383,13 @@ private static function checkUploadRateLimit(string $clientIp, $cache): int
357383
*/
358384
private static function validateFileType(string $filename, string $mimeType, string $category): array
359385
{
386+
// Security: pathinfo() preserves any shell meta-characters the client
387+
// appended to the extension ("hack.php;touch" parses as "php;touch"
388+
// and would slip past the blacklist below). Normalize to a plain
389+
// alnum set before comparison so the FORBIDDEN_EXTENSIONS check
390+
// cannot be bypassed by appending a separator character.
360391
$extension = strtolower(pathinfo($filename, PATHINFO_EXTENSION));
392+
$extension = preg_replace('/[^a-z0-9]/', '', $extension);
361393

362394
// 1. Check forbidden extensions (security)
363395
if (in_array($extension, self::FORBIDDEN_EXTENSIONS, true)) {

‎src/PBXCoreREST/Lib/Modules/ModuleInstallationBase.php‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -242,8 +242,12 @@ protected function startModuleInstallation(string $filePath): PBXApiResult
242242
$php = Util::which('php');
243243
$workerModuleInstallerPath = Util::getFilePathByClassName(WorkerModuleInstaller::class);
244244

245-
// Execute the background process to install the module
246-
Processes::mwExecBg("$php -f $workerModuleInstallerPath start '$settings_file'");
245+
// Execute the background process to install the module.
246+
// escapeshellarg replaces the fragile single-quote wrapping: a path
247+
// containing an apostrophe used to break out into the shell.
248+
Processes::mwExecBg(
249+
"$php -f $workerModuleInstallerPath start " . escapeshellarg($settings_file)
250+
);
247251

248252
$res->data[FilesConstants::FILE_PATH] = $filePath;
249253
$res->data[self::MODULE_WAS_ENABLED] = $moduleWasEnabled;

‎src/PBXCoreREST/Lib/Modules/StartDownloadAction.php‎

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,13 @@ class StartDownloadAction extends Injectable
4040
/**
4141
* Starts the module download in a separate background process.
4242
*
43+
* Security note: all three inputs are treated as untrusted. The module id
44+
* becomes a directory name (`$moduleDirTmp`) that is later passed through
45+
* the shell to launch WorkerDownloader, so shell meta-characters are
46+
* rejected up front and every interpolated path goes through
47+
* escapeshellarg(). See CVE-2025-52207 for the same class of bug in
48+
* DownloadNewFirmwareAction.
49+
*
4350
* @param string $moduleUniqueID The module unique id.
4451
* @param string $url The download URL of the module.
4552
* @param string $md5 The MD5 hash of the module file.
@@ -50,6 +57,33 @@ public static function main(string $moduleUniqueID, string $url, string $md5): P
5057
{
5158
$res = new PBXApiResult();
5259
$res->processor = __METHOD__;
60+
61+
// Module ids follow a restricted charset (typically "ModuleFooBar").
62+
// Reject anything that would let us break out of the directory name.
63+
$moduleUniqueID = trim($moduleUniqueID);
64+
if ($moduleUniqueID === '' || !preg_match('/^[A-Za-z0-9_\-]{1,128}$/', $moduleUniqueID)) {
65+
$res->success = false;
66+
$res->messages['error'][] = 'Invalid module unique id';
67+
$res->httpCode = 400;
68+
return $res;
69+
}
70+
71+
$url = trim($url);
72+
if ($url === '' || !filter_var($url, FILTER_VALIDATE_URL) || !preg_match('#^https?://#i', $url)) {
73+
$res->success = false;
74+
$res->messages['error'][] = 'Download URL must be a valid http(s) URL';
75+
$res->httpCode = 400;
76+
return $res;
77+
}
78+
79+
$md5 = strtolower(trim($md5));
80+
if (!preg_match('/^[a-f0-9]{32}$/', $md5)) {
81+
$res->success = false;
82+
$res->messages['error'][] = 'A valid MD5 hash (32 hex characters) is required';
83+
$res->httpCode = 400;
84+
return $res;
85+
}
86+
5387
$di = Di::getDefault();
5488
if ($di !== null) {
5589
$tempDir = Directories::getDir(Directories::WWW_UPLOAD_DIR);
@@ -80,7 +114,9 @@ public static function main(string $moduleUniqueID, string $url, string $md5): P
80114
);
81115
$workerDownloaderPath = Util::getFilePathByClassName(WorkerDownloader::class);
82116
$php = Util::which('php');
83-
Processes::mwExecBg("$php -f $workerDownloaderPath start $moduleDirTmp/download_settings.json");
117+
Processes::mwExecBg(
118+
"$php -f $workerDownloaderPath start " . escapeshellarg("$moduleDirTmp/download_settings.json")
119+
);
84120

85121
$res->data['uniqid'] = $moduleUniqueID;
86122
$res->data[FilesConstants::D_STATUS] = FilesConstants::DOWNLOAD_IN_PROGRESS;

‎src/PBXCoreREST/Lib/Modules/UninstallModuleAction.php‎

Lines changed: 40 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,14 @@ public function uninstallModule(): PBXApiResult
110110
$res = new PBXApiResult();
111111
$res->processor = __METHOD__;
112112

113+
// Validate module id up front: it becomes part of shell/filesystem
114+
// paths below. Only [A-Za-z0-9_-] is ever a legitimate module id.
115+
if (!preg_match('/^[A-Za-z0-9_\-]{1,128}$/', $this->moduleUniqueId)) {
116+
$res->success = false;
117+
$res->messages['error'] = 'Invalid module unique id';
118+
return $res;
119+
}
120+
113121
$currentModuleDir = PbxExtensionUtils::getModuleDir($this->moduleUniqueId);
114122

115123
if (PbxExtensionUtils::isEnabled($this->moduleUniqueId)) {
@@ -120,19 +128,37 @@ public function uninstallModule(): PBXApiResult
120128
}
121129
}
122130

123-
// Kill all module processes
131+
// Kill all module processes.
132+
//
133+
// The previous implementation used a nested $() subshell —
134+
// "$kill -9 $($lsof $currentModuleDir/bin/* | $grep -v COMMAND | $awk '{print $2}' | $uniq)"
135+
// which is an arbitrary command injection sink the moment
136+
// $currentModuleDir contains a shell meta-character. Even though the
137+
// module id is now validated above, we avoid the nested shell
138+
// altogether: lsof output is captured into a PHP array and PIDs are
139+
// sent via posix_kill(), so no shell interpolation of a path happens.
124140
if (is_dir("$currentModuleDir/bin")) {
125141
$this->unifiedModulesEvents->pushMessageToBrowser(self::STAGE_II_STOP_PROCESSES, $res->getResult());
126-
$kill = Util::which('kill');
142+
127143
$lsof = Util::which('lsof');
128-
$grep = Util::which('grep');
129-
$awk = Util::which('awk');
130-
$uniq = Util::which('uniq');
131-
132-
// Execute the command to kill all processes related to the module
133-
Processes::mwExec(
134-
"$kill -9 $($lsof $currentModuleDir/bin/* | $grep -v COMMAND | $awk '{ print $2}' | $uniq)"
135-
);
144+
$lsofCmd = $lsof . ' ' . escapeshellarg("$currentModuleDir/bin") . '/*';
145+
$lsofOutput = [];
146+
Processes::mwExec($lsofCmd, $lsofOutput);
147+
148+
$pids = [];
149+
foreach ($lsofOutput as $line) {
150+
if (str_starts_with($line, 'COMMAND')) {
151+
continue;
152+
}
153+
$fields = preg_split('/\s+/', trim($line));
154+
if (isset($fields[1]) && ctype_digit($fields[1])) {
155+
$pids[(int)$fields[1]] = true;
156+
}
157+
}
158+
foreach (array_keys($pids) as $pid) {
159+
// SIGKILL each open file-holding process, mirroring the old kill -9 loop.
160+
@posix_kill($pid, SIGKILL);
161+
}
136162
}
137163

138164
// Uninstall module with keep settings and backup db
@@ -160,8 +186,10 @@ class_exists($moduleClass)
160186
$rm = Util::which('rm');
161187

162188
$this->unifiedModulesEvents->pushMessageToBrowser(self::STAGE_V_DELETE_MODULE_FOLDER, $res->getResult());
163-
// Remove the module directory recursively
164-
Processes::mwExec("$rm -rf $currentModuleDir");
189+
// Remove the module directory recursively.
190+
// escapeshellarg prevents command injection even if a future
191+
// refactor relaxes the module id validation above.
192+
Processes::mwExec("$rm -rf " . escapeshellarg($currentModuleDir));
165193

166194
// Use the fallback class to unregister the module from the database
167195
$this->unifiedModulesEvents->pushMessageToBrowser(self::STAGE_VI_UNREGISTER_MODULE, $res->getResult());

‎src/PBXCoreREST/Lib/SysLogs/PrepareLogAction.php‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,11 @@ public static function main(bool $tcpdumpOnly): PBXApiResult
6666
file_put_contents($settings_file, json_encode($merge_settings, JSON_UNESCAPED_SLASHES | JSON_PRETTY_PRINT));
6767
$php = Util::which('php');
6868
$workerFilesMergerPath = Util::getFilePathByClassName(WorkerMakeLogFilesArchive::class);
69-
Processes::mwExecBg("$php -f $workerFilesMergerPath start '$settings_file'");
69+
// Defence in depth: the $prefix is derived from a bool flag today, but
70+
// escapeshellarg removes any future risk if a refactor widens it.
71+
Processes::mwExecBg(
72+
"$php -f $workerFilesMergerPath start " . escapeshellarg($settings_file)
73+
);
7074

7175
return $res;
7276
}

‎src/PBXCoreREST/Lib/System/UpgradeFromImageAction.php‎

Lines changed: 60 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
namespace MikoPBX\PBXCoreREST\Lib\System;
2121

2222
use MikoPBX\Common\Providers\TranslationProvider;
23+
use MikoPBX\Core\System\Directories;
2324
use MikoPBX\Core\System\Processes;
2425
use MikoPBX\Core\System\Storage;
2526
use MikoPBX\Core\System\System;
@@ -52,6 +53,16 @@ public static function main(string $imageFileLocation): PBXApiResult
5253
$res->success = true;
5354
$res->data['message'] = 'In progress...';
5455

56+
// Confine the uploaded image path to the configured upload directory
57+
// so a crafted API payload cannot point at arbitrary files or smuggle
58+
// shell meta-characters through $imageFileLocation.
59+
$imageFileLocation = self::confineToUploadDir($imageFileLocation);
60+
if ($imageFileLocation === null) {
61+
$res->success = false;
62+
$res->messages[] = 'Invalid image file location';
63+
return $res;
64+
}
65+
5566
// Validate input parameters.
5667
list($res->success, $res->messages) = self::validateParameters($imageFileLocation);
5768
if (!$res->success) {
@@ -98,6 +109,34 @@ public static function main(string $imageFileLocation): PBXApiResult
98109
return $res;
99110
}
100111

112+
/**
113+
* Validate that the supplied image path resolves inside the configured
114+
* upload directory and exists on disk. Returns the realpath on success
115+
* or null on any failure — the caller must reject a null result.
116+
*
117+
* This is the primary defence against command injection via
118+
* $imageFileLocation because the rest of this class interpolates the
119+
* string into shell pipelines. A realpath prefix match makes path
120+
* traversal impossible and the character set of an upload-directory
121+
* path makes shell meta-characters irrelevant too.
122+
*/
123+
private static function confineToUploadDir(string $imageFileLocation): ?string
124+
{
125+
$uploadDir = Directories::getDir(Directories::WWW_UPLOAD_DIR);
126+
$allowedRoot = realpath($uploadDir);
127+
if ($allowedRoot === false) {
128+
return null;
129+
}
130+
$real = realpath($imageFileLocation);
131+
if ($real === false) {
132+
return null;
133+
}
134+
if (!str_starts_with($real, $allowedRoot . DIRECTORY_SEPARATOR) && $real !== $allowedRoot) {
135+
return null;
136+
}
137+
return $real;
138+
}
139+
101140
/**
102141
* Validate input parameters.
103142
* @param string $imageFileLocation The location of the IMG file previously uploaded to the system.
@@ -197,11 +236,22 @@ private static function writeUpdateScript(array $parameters): array
197236
$res->processor = __METHOD__;
198237
$res->success = true;
199238

239+
// The boot partition name comes from lsblk output in
240+
// getBootPartitionName() — in practice a /dev/* node — but we still
241+
// whitelist it here so a future refactor or compromised lsblk cannot
242+
// break the shell boundary.
243+
$bootPartition = (string)($parameters['bootPartition'] ?? '');
244+
if (!preg_match('#^/dev/[A-Za-z0-9/_\-]+$#', $bootPartition)) {
245+
return [false, ["Refusing to mount suspicious boot partition path: $bootPartition"]];
246+
}
247+
200248
// Mount boot partition
201249
$systemDir = '/system';
202250
Util::mwMkdir($systemDir);
203251
$mount = Util::which('mount');
204-
$result = Processes::mwExec("$mount {$parameters['bootPartition']} $systemDir");
252+
$result = Processes::mwExec(
253+
"$mount " . escapeshellarg($bootPartition) . ' ' . escapeshellarg($systemDir)
254+
);
205255
if ($result === 0) {
206256
$upgradeScriptDir = "$systemDir/upgrade";
207257
Util::mwMkdir($upgradeScriptDir);
@@ -216,12 +266,12 @@ private static function writeUpdateScript(array $parameters): array
216266
}
217267
}
218268
} else {
219-
$res->messages[] = "Failed to mount the boot partition {$parameters['bootPartition']}";
269+
$res->messages[] = "Failed to mount the boot partition $bootPartition";
220270
$res->success = false;
221271
}
222272

223273
$umount = Util::which('umount');
224-
Processes::mwExec("$umount {$parameters['bootPartition']}");
274+
Processes::mwExec("$umount " . escapeshellarg($bootPartition));
225275

226276
return [$res->success, $res->messages];
227277
}
@@ -249,14 +299,19 @@ private static function extractNewUpdateScript(string $imageFileLocation, string
249299
// Decompress the IMG file using 'busybox gunzip' instead of standalone gunzip.
250300
// GNU gzip 1.14 called as 'gunzip' only removes one gzip layer from double-compressed
251301
// firmware images. BusyBox gunzip handles this correctly.
302+
//
303+
// Both operands are escapeshellarg()'d — the original single-quoted
304+
// form broke on any apostrophe or $() inside the path.
252305
$busybox = Util::which('busybox');
253-
$decompressCmd = "$busybox gunzip -c '$imageFileLocation' > '$decompressedImg'";
306+
$decompressCmd = "$busybox gunzip -c " . escapeshellarg($imageFileLocation)
307+
. ' > ' . escapeshellarg($decompressedImg);
254308
Processes::mwExec($decompressCmd);
255309

256310
// Setup loop device with the correct offset
257311
$parted = Util::which('parted');
258312
$busybox = Util::which('busybox');
259-
$cmdOffset = "$parted '$decompressedImg' unit B print | $busybox awk 'NR>3 && /boot/ {gsub(/B/,\"\",$2); print $2+0; exit}' | $busybox head -n 1";
313+
$cmdOffset = "$parted " . escapeshellarg($decompressedImg)
314+
. " unit B print | $busybox awk 'NR>3 && /boot/ {gsub(/B/,\"\",$2); print $2+0; exit}' | $busybox head -n 1";
260315
$offset = trim(shell_exec($cmdOffset) ?? '');
261316

262317
// Validate offset before proceeding

0 commit comments

Comments
 (0)