fix: Critical and high audit findings (#81) #83
@@ -121,11 +121,27 @@ class BackupsController extends ApiController
|
||||
|
||||
$data = [];
|
||||
|
||||
// Strip sensitive credentials before serialization
|
||||
$sensitiveFields = [
|
||||
'ftp_password', 'ftp_username',
|
||||
's3_access_key', 's3_secret_key',
|
||||
'gdrive_client_secret', 'gdrive_refresh_token',
|
||||
'encryption_password', 'ntfy_token',
|
||||
];
|
||||
|
||||
foreach ($items as $item) {
|
||||
$safe = clone $item;
|
||||
|
||||
foreach ($sensitiveFields as $field) {
|
||||
if (isset($safe->$field) && $safe->$field !== '') {
|
||||
$safe->$field = '***';
|
||||
}
|
||||
}
|
||||
|
||||
$data[] = [
|
||||
'type' => 'profiles',
|
||||
'id' => $item->id,
|
||||
'attributes' => $item,
|
||||
'id' => $safe->id,
|
||||
'attributes' => $safe,
|
||||
];
|
||||
}
|
||||
|
||||
|
||||
@@ -206,6 +206,11 @@ class JpaUnarchiver
|
||||
}
|
||||
}
|
||||
|
||||
// Path traversal protection: reject absolute paths and directory traversal
|
||||
if (str_starts_with($path, '/') || str_starts_with($path, '\\') || str_contains($path, '..')) {
|
||||
return; // skip malicious entry
|
||||
}
|
||||
|
||||
// Is this a directory?
|
||||
if (substr($path, -1) === '/' || $uncompSize === 0 && $compSize === 0) {
|
||||
$dirPath = $this->outputDir . '/' . $path;
|
||||
@@ -228,6 +233,24 @@ class JpaUnarchiver
|
||||
|
||||
// Write file
|
||||
$fullPath = $this->outputDir . '/' . $path;
|
||||
|
||||
// Verify resolved path stays within output directory
|
||||
$realOutput = realpath($this->outputDir);
|
||||
|
||||
if ($realOutput !== false) {
|
||||
$parentDir = dirname($fullPath);
|
||||
|
||||
if (!is_dir($parentDir)) {
|
||||
mkdir($parentDir, 0755, true);
|
||||
}
|
||||
|
||||
$realDest = realpath($parentDir);
|
||||
|
||||
if ($realDest === false || !str_starts_with($realDest, $realOutput)) {
|
||||
return; // path escapes staging directory
|
||||
}
|
||||
}
|
||||
|
||||
$parentDir = dirname($fullPath);
|
||||
|
||||
if (!is_dir($parentDir)) {
|
||||
|
||||
@@ -38,15 +38,27 @@ class PreflightCheck
|
||||
*/
|
||||
public function run(int $profileId): array
|
||||
{
|
||||
$db = Factory::getDbo();
|
||||
try {
|
||||
$db = Factory::getDbo();
|
||||
} catch (\Exception $e) {
|
||||
$this->errors[] = 'Cannot connect to database: ' . $e->getMessage();
|
||||
|
||||
return $this->result();
|
||||
}
|
||||
|
||||
// Load profile
|
||||
$query = $db->getQuery(true)
|
||||
->select('*')
|
||||
->from($db->quoteName('#__mokosuitebackup_profiles'))
|
||||
->where($db->quoteName('id') . ' = ' . $profileId);
|
||||
$db->setQuery($query);
|
||||
$profile = $db->loadObject();
|
||||
try {
|
||||
$query = $db->getQuery(true)
|
||||
->select('*')
|
||||
->from($db->quoteName('#__mokosuitebackup_profiles'))
|
||||
->where($db->quoteName('id') . ' = ' . (int) $profileId);
|
||||
$db->setQuery($query);
|
||||
$profile = $db->loadObject();
|
||||
} catch (\Exception $e) {
|
||||
$this->errors[] = 'Cannot load profile: ' . $e->getMessage();
|
||||
|
||||
return $this->result();
|
||||
}
|
||||
|
||||
if (!$profile) {
|
||||
$this->errors[] = 'Profile not found: #' . $profileId;
|
||||
@@ -111,14 +123,19 @@ class PreflightCheck
|
||||
$resolvedDir = BackupDirectory::resolve($resolver->resolve($configuredDir));
|
||||
|
||||
if (BackupDirectory::hasPlaceholders($resolvedDir)) {
|
||||
// Can't fully validate paths with unresolved placeholders
|
||||
$this->warnings[] = 'Backup directory contains unresolved placeholders: ' . $resolvedDir
|
||||
. ' — directory cannot be validated until backup runs';
|
||||
|
||||
return;
|
||||
}
|
||||
|
||||
if (!is_dir($resolvedDir)) {
|
||||
// Try to create it
|
||||
if (!@mkdir($resolvedDir, 0755, true)) {
|
||||
$this->errors[] = 'Backup directory does not exist and cannot be created: ' . $resolvedDir;
|
||||
$lastError = error_get_last();
|
||||
$reason = $lastError['message'] ?? 'unknown reason';
|
||||
$this->errors[] = 'Backup directory does not exist and cannot be created: ' . $resolvedDir
|
||||
. ' (' . $reason . ')';
|
||||
|
||||
return;
|
||||
}
|
||||
|
||||
@@ -76,8 +76,9 @@ class RestoreEngine
|
||||
return ['success' => false, 'message' => 'Backup archive not found: ' . $archivePath];
|
||||
}
|
||||
|
||||
// Create staging directory
|
||||
$this->stagingDir = JPATH_ROOT . '/tmp/mokosuitebackup-restore-' . $record->tag;
|
||||
// Create staging directory (sanitize tag to prevent path traversal)
|
||||
$safeTag = preg_replace('/[^a-zA-Z0-9_-]/', '', $record->tag ?: 'restore');
|
||||
$this->stagingDir = JPATH_ROOT . '/tmp/mokosuitebackup-restore-' . $safeTag;
|
||||
|
||||
if (is_dir($this->stagingDir)) {
|
||||
$this->recursiveDelete($this->stagingDir);
|
||||
|
||||
@@ -36,7 +36,7 @@ class BackupModel extends AdminModel
|
||||
$data = $this->getItem();
|
||||
}
|
||||
|
||||
return $data;
|
||||
return is_array($data) ? (object) $data : $data;
|
||||
}
|
||||
|
||||
public function getTable($name = 'Backup', $prefix = 'Administrator', $options = [])
|
||||
|
||||
@@ -36,7 +36,7 @@ class ProfileModel extends AdminModel
|
||||
$data = $this->getItem();
|
||||
}
|
||||
|
||||
return $data;
|
||||
return is_array($data) ? (object) $data : $data;
|
||||
}
|
||||
|
||||
public function getTable($name = 'Profile', $prefix = 'Administrator', $options = [])
|
||||
|
||||
@@ -39,11 +39,22 @@ class BackupTable extends Table
|
||||
|
||||
public function delete($pk = null): bool
|
||||
{
|
||||
// Delete the archive file if it exists
|
||||
if (!empty($this->absolute_path) && is_file($this->absolute_path)) {
|
||||
@unlink($this->absolute_path);
|
||||
$archivePath = $this->absolute_path;
|
||||
|
||||
// Delete DB record first — if this fails, the file is preserved
|
||||
$result = parent::delete($pk);
|
||||
|
||||
if ($result && !empty($archivePath) && is_file($archivePath)) {
|
||||
@unlink($archivePath);
|
||||
|
||||
// Also remove the log file if it exists alongside the archive
|
||||
$logPath = preg_replace('/\.(zip|tar\.gz)$/i', '.log', $archivePath);
|
||||
|
||||
if (is_file($logPath)) {
|
||||
@unlink($logPath);
|
||||
}
|
||||
}
|
||||
|
||||
return parent::delete($pk);
|
||||
return $result;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -86,7 +86,7 @@ class RestoreCommand extends AbstractCommand
|
||||
}
|
||||
|
||||
$engine = new RestoreEngine();
|
||||
$result = $engine->restore($record->absolute_path, $record->backup_type);
|
||||
$result = $engine->restore($recordId);
|
||||
|
||||
if ($result['success']) {
|
||||
$io->success($result['message']);
|
||||
|
||||
@@ -59,11 +59,15 @@ final class MokoSuiteBackup extends CMSPlugin implements SubscriberInterface
|
||||
// Reject if disabled or no secret configured
|
||||
if (!$enabled || $configSecret === '') {
|
||||
$this->sendJsonResponse(false, 'Web cron is not enabled', 403);
|
||||
|
||||
return;
|
||||
}
|
||||
|
||||
// Validate secret (timing-safe comparison)
|
||||
if (!hash_equals($configSecret, $secret)) {
|
||||
$this->sendJsonResponse(false, 'Invalid secret', 403);
|
||||
|
||||
return;
|
||||
}
|
||||
|
||||
// IP whitelist check (if configured)
|
||||
@@ -73,6 +77,8 @@ final class MokoSuiteBackup extends CMSPlugin implements SubscriberInterface
|
||||
|
||||
if (!in_array($clientIp, $allowedIps, true)) {
|
||||
$this->sendJsonResponse(false, 'IP not allowed', 403);
|
||||
|
||||
return;
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user