Vault - Site backup and migration
tool_vault
- #1Integer values interpolated into SQL strings instead of bound as parameters
- #2Reference to a non-existent plugin setting `restorepreservetables`
- #3Custom curl wrapper unconditionally disables Moodle's URL block list
- #4Reflection used to access protected Moodle core internals
- #5Stored log output is rendered without escaping
Vault - Site backup and migration (tool_vault) is an admin tool that performs a complete site backup (database, dataroot, and content-addressed file storage) and uploads the encrypted archives to a remote storage service (lmsvault.io). It also restores a backup downloaded from the service onto the same or a different Moodle instance, optionally running an internal upgrade chain to bring the restored site up to the current Moodle version.
The plugin exposes a web UI under Site administration, plus three CLI scripts (cli/site_backup.php, cli/site_restore.php, cli/list_remote_backups.php). Backups can be encrypted with a passphrase (used as a server-side AES key for S3 SSE-C). All web entry points require the moodle/site:config capability.
The plugin is well-engineered for its very privileged purpose. All web entry points are gated behind admin_externalpage_setup (which enforces moodle/site:config), state-changing actions consistently call require_sesskey(), dynamic forms enforce require_capability in check_access_for_dynamic_submission, user input is read through required_param/optional_param with appropriate PARAM_* types, and database access goes through the $DB API with parameterised placeholders.
No SQL injection, XSS, CSRF, RFI, command-injection, or auth-bypass vulnerabilities were identified. The unauthenticated progress.php page is gated by a 32-character random accesskey and is read-only.
Findings are limited to low/info-level items: a couple of integer values are interpolated into SQL strings rather than passed as bound parameters in cleanup_existing_files.php (still safe due to PHP int typing), a setting name (restorepreservetables) that is read but never defined, the deliberate use of ignoresecurity = true on the bundled curl wrapper for talking to S3 (with mitigating URL allow-listing for encrypted uploads), use of reflection to access protected Moodle core internals, and unescaped log output that is currently safe but would become risky if untrusted content ever entered the log stream. None of these justify a downgrade below A; an admin-targeted backup/restore tool inherently performs many of the operations (DDL, dataroot writes, cross-engine SQL, schema introspection) that would normally be flagged on a regular plugin.
Overview
tool_vault is a mature, admin-only site backup and migration plugin. It is unusually broad in what it touches — full database export/import, schema reconstruction, dataroot file movement, content-addressed file store synchronisation, plugin uninstallation, and an internal Moodle upgrade chain that ships canned copies of the 3.11 → 4.4 core upgrade scripts. Despite that footprint, the security posture is clean.
What was checked
- All entry points (
index.php,progress.php, three CLI scripts). - All UI action handlers under
classes/local/uiactions/. - The API client (
classes/api.php), curl wrapper, dbops helper, file backup/restore helpers, tempfile handling. - The backup and restore engines (
site_backup.php,site_restore.php, dryrun variants). - All check classes (backup and restore prechecks) and operation/model classes.
- The dynamic forms (
apikey_form,start_backup_form,start_restore_form). - The privacy provider, db install/upgrade/tasks/messages/hooks.
- All mustache templates and AMD JavaScript modules.
- A representative sample of the bundled upgrade scripts (
upgrade_311,upgrade_401,upgrade_402,upgrade_404).
Security posture
- Capability enforcement is consistent:
admin_externalpage_setup('tool_vault_index', '', null, $section->url(), …)inindex.phpandrequire_capability('moodle/site:config', …)in every dynamic form'scheck_access_for_dynamic_submission(). - CSRF protection: every state-changing UI action (
main_forgetapikey,backup_newcheck,restore_dryrun,restore_restore,restore_resume,restore_updateremote,vaulttools,backup_checkreportadd-table action) callsrequire_sesskey()before mutating state. - SQL safety: the codebase uses
$DBexclusively, queries are parameterised, dynamic SQL portions are constructed from$DB->get_in_or_equal()/get_manager()->generator->getEncQuoted(). Two integer values are interpolated into SQL incleanup_existing_files.php, but they are PHP-int-typed and not user controlled. - Input validation: parameters are pulled with
required_param/optional_paramand constrained types (PARAM_ALPHANUMEXTforbackupkey/section/action,PARAM_INTfor ids,PARAM_LOCALURLfor return URLs, etc.). Class names are looked up only viaclass_exists()against namespaced prefixes plus aclean_param(..., PARAM_ALPHANUMEXT)round-trip — no dynamic class-name injection is possible. - HTTP egress: the bundled
curlsubclass setsignoresecurity = true, but the destination URLs come from the authenticated vault API; for encrypted uploads/downloads the plugin additionally enforces^https://[^/]+\.s3\.amazonaws\.com/before sending the SSE-C key. - Filesystem access: dataroot writes target
$CFG->dataroot . '/__vault_restore__'; filedir is handled exclusively viaget_file_storage()(add_file_to_pool,copy_content_to); temp files usemake_backup_temp_directory()and the plugin'stempfileshelper. - Privacy:
\core_privacy\local\metadata\null_provideris implemented and the external location (lmsvault.io) is declared viaadd_external_location_link. - Maintenance gate: while a backup or restore is in progress, the
after_confighook (and its legacy callback) places the site in maintenance mode for everyone except theprogress.phpaccess-key URL.
What is not in scope for normal review rules
A significant fraction of classes/local/restoreactions/upgrade_*/ is verbatim copies of Moodle core upgrade code, re-namespaced under tool_vault_*_xmldb_*_upgrade. Per the plugin's documented intent (running upgrades from old releases to the current major version during a restore), these files are intentionally divergent from the surrounding code style and are flagged with // phpcs:ignoreFile and // Mdlcode-disable … markers. Findings against that mirrored core code would not be actionable, so they were not reported.
Recommendation
The plugin is in good shape. The few findings below are quality-of-implementation items rather than security defects. The largest meaningful improvement opportunity is to replace the few integer-interpolated SQL strings with bound parameters and to mustache-escape stored log output (or pre-escape it at the model layer) — neither is currently exploitable.
Findings
In classes/local/restoreactions/cleanup_existing_files.php, the $restoreid value (a PHP int parameter) is concatenated directly into the SQL strings used by $DB->execute() instead of being passed via a named or positional placeholder.
Because the parameter is typed as int in PHP and the only callers in the codebase pass $logger->get_model()->id (an integer DB row id), the actual values cannot escape the integer domain — so this is not an SQL injection vulnerability today. It is, however, a deviation from Moodle's standard pattern ($DB->execute($sql, $params) with placeholders) and reduces defence in depth.
Low risk. The PHP int type-hint guarantees the value is numeric; PHP will throw a TypeError for any non-integer input. There is no user-controlled path into $restoreid. The finding is a coding-standard / defence-in-depth concern, not an exploitable injection.
These two queries are part of the orphaned-files cleanup that runs before restore (snapshot the existing contenthashes) and after restore (delete file content for hashes that no longer appear in {files}). The $restoreid value is the primary key of the in-progress tool_vault_operation row, generated server-side by $DB->insert_record — there is no path from external input to this variable.
$sql = "insert into {tool_vault_table_files_data} (restoreid, contenthash)
select distinct $restoreid AS restoreid, contenthash
from {files} where contenthash not in (
select contenthash
from {files}
where component $sqlcomponent
) and referencefileid is null";
$DB->delete_records_select('tool_vault_table_files_data', 'restoreid = ?', [$restoreid]);
$DB->execute($sql, $params);
Pass $restoreid as a bound parameter:
$sql = "insert into {tool_vault_table_files_data} (restoreid, contenthash)
select distinct :restoreid AS restoreid, contenthash
from {files} where contenthash not in (
select contenthash
from {files}
where component $sqlcomponent
) and referencefileid is null";
$params['restoreid'] = $restoreid;
$DB->execute($sql, $params);
$sql = "DELETE FROM {tool_vault_table_files_data}
WHERE restoreid=$restoreid
AND contenthash IN (SELECT contenthash FROM {files})";
$DB->execute($sql);
$sql = "DELETE FROM {tool_vault_table_files_data}
WHERE restoreid = ?
AND contenthash IN (SELECT contenthash FROM {files})";
$DB->execute($sql, [$restoreid]);
siteinfo::is_table_preserved_in_restore() reads the setting restorepreservetables via api::get_setting_array('restorepreservetables') and matches table names against it, but no such setting is registered anywhere — settings.php defines restorepreservedataroot, restorepreserveplugins, restorepreservepasswords, but not restorepreservetables. The author left an inline // TODO this setting does not exist! next to the call.
As a result, the wildcard branch of this method is dead code: it always evaluates against an empty array and never preserves any tables that are not already covered by the plugin-level exclusion list. A site that customised an excluded plugin's table for preservation would silently see those rows truncated during restore.
Low risk. Functional bug with no security impact. The worst-case effect is that an admin who discovers the setting in the source code and tries to set it via set_config('restorepreservetables', '…', 'tool_vault') would observe their preference being honoured even though it is undocumented and never UI-managed.
The function is symmetrical with is_table_excluded_from_backup() which uses backupexcludetables (a real setting). The mismatched name appears to be an oversight rather than a deliberate disable.
if (!$deftable) {
// This is a table that is not present in the install.xml files of core or any plugins.
// Exclude this table if it's name is in the 'backupexcludetables' setting.
$tables = api::get_setting_array('restorepreservetables');
// TODO this setting does not exist!
if (self::matches_wildcard_pattern($CFG->prefix . $tablename, $tables)) {
return true;
}
}
Either:
- Remove the dead branch if the symmetry with
is_table_excluded_from_backup()is not intended for restore, or - Register the setting in
settings.php(mirroringbackupexcludetables) so administrators can list extra (non-install.xml) tables to preserve during restore, and update the language file accordingly.
Whichever path is chosen, drop the // TODO this setting does not exist! comment.
tool_vault\local\helpers\curl extends Moodle's core \curl and forces $settings['ignoresecurity'] = true in its constructor. This is the documented core flag that bypasses the host/IP allow/deny lists ($CFG->curlsecurityblockedhosts, $CFG->curlsecurityallowedport) used to defend against SSRF.
During backup uploads and restore downloads, the plugin first calls its API server to obtain a presigned URL and then issues the actual transfer through this wrapper. The plugin does validate that the URL matches ^https://[^/]+\.s3\.amazonaws\.com/ before sending the SSE-C encryption key (so an attacker controlling the API server cannot exfiltrate the key to a non-AWS host), but for non-encrypted backups and for the head validation request the URL is followed without that allow-list check.
In the threat model where the lmsvault.io API server is fully trusted, this is fine. If that trust is reduced — for example, if a self-hosted vault server is misconfigured ($CFG->forced_plugin_settings['tool_vault']['apiurl']) — then a hostile API response could redirect uploads to attacker-controlled internal services.
Low risk in the standard deployment (lmsvault.io is the API server, the API key authenticates the admin). The risk surfaces only if $CFG->forced_plugin_settings['tool_vault']['apiurl'] is pointed at a hostile or compromised endpoint, and an attacker uses a non-encrypted backup so the AWS hostname check is skipped. In that scenario an admin-initiated backup could be steered into making HEAD/PUT requests against internal services. Exploitation requires admin to configure the API endpoint, which is itself a privileged action.
The custom curl class is used by api.php for S3 multipart uploads, single uploads, downloads, and the validate_backup HEAD request. The standard curl wrapper would otherwise refuse private addresses on most hosting setups.
public function __construct($settings = []) {
$settings['ignoresecurity'] = true;
parent::__construct($settings);
}
Two reasonable hardenings, in order of strength:
- Tighten the allow-list check so it applies to every outbound request, not just those that carry an encryption key. The S3 hostname pattern is already present in
api.php(upload_backup_file,validate_backup,download_backup_file); extract it into a singleassert_is_aws_s3_url($url)helper and call it before every storage transfer regardless of encryption state. - Only set
ignoresecurityfor the specific request methods that target storage providers (put_file_part,download_one,head), not in the constructor — so any future use of the wrapper for arbitrary URLs goes through Moodle's normal SSRF guards.
The plugin reaches into private/protected core APIs via reflection in two places:
classes/local/helpers/dbops.phpinvokesmysqli_native_moodle_database::emulate_bound_params()(aprotectedmethod) to compute the exact byte length of bulk-insert SQL chunks against MySQL'smax_allowed_packet.classes/local/restoreactions/upgrade.phpconstructs\cache_factory_disabledwithout its constructor and then re-injects it into theprotected static $instanceof\cache_factoryto disable caches during the in-restore upgrade.
Neither use is exploitable — both are defensive workarounds for limitations of the public API. The risk is fragility: a future Moodle release that renames/removes either symbol will break backup/restore at runtime, possibly silently (the upgrade case is wrapped in normal flow with no fallback).
Low risk. No exploitation potential — the concern is maintainability and forward compatibility against future Moodle versions.
Both reflection sites are inside admin-only restore code paths and run in CLI / cron context.
// Function mysqli_native_moodle_database::emulate_bound_params() is protected but we need to call it
// to get the exact length of the query.
$reflector = new \ReflectionObject($DB);
$method = $reflector->getMethod('emulate_bound_params');
$method->setAccessible(true);
Approximate the packet length without reflection (e.g. strlen($valuerowsql) + array_sum(array_map('strlen', $row)) plus a small overhead constant), trading exactness for forward compatibility. The current chunking already includes other safety margins, so an approximation should not regress correctness.
$class = new \ReflectionClass(\cache_factory_disabled::class);
$constructor = $class->getConstructor();
$constructor->setAccessible(true);
$object = $class->newInstanceWithoutConstructor();
$constructor->invoke($object, 1);
$reflection = new \ReflectionProperty(\cache_factory::class, 'instance');
$reflection->setAccessible(true);
$reflection->setValue(null, $object);
Use \cache_factory::disable_caching() if available (it has been the public entry point since Moodle 3.x) and gate this reflection block behind a class_exists / method_exists fallback so a future API change is detected loudly rather than silently failing.
operation_model::format_log_line() wraps the raw $log->message in a <span> via \html_writer::span($message, $class). html_writer::tag() does not escape its $contents argument — it only escapes attribute values. The resulting HTML is then rendered into the page through {{{logs}}} (triple-mustache, unescaped) in templates/backup_details.mustache, restore_details.mustache, and vaulttools_details.mustache, and is also surfaced unauthenticated on progress.php (gated only by the accesskey).
Log content originates from internal add_to_log() calls and from log_capture (which captures arbitrary stdout produced during backup/restore — including PHP warnings / debugging messages from any plugin or core code path). In normal operation everything written there is internal text, but the stored-XSS surface exists if any future code path were to log a user-controlled value verbatim.
Low risk. No present-day injection vector was identified — log content in shipped code is internal status text and exception messages whose content is bounded by core/plugin error-handling. The finding is preventative: the unescaped sink + the existence of log_capture (which forwards anything that PHP echoes during backup/restore into the same DB column) means a future code change could turn this into stored XSS without any obvious local signal. The blast radius would be admin-on-admin (or anyone who knows the access key).
progress.php is reached via a 32-character random_string(32) access key and bypasses normal auth (NO_MOODLE_COOKIES). It renders the same backup_details / restore_details mustache templates as the admin-side pages.
return $usehtml ? \html_writer::span($message, $class) : $message;
Escape the message content before wrapping:
return $usehtml ? \html_writer::span(s($message), $class) : $message;
Or switch the templates to escape: change {{{logs}}} to {{logs}} in backup_details.mustache, restore_details.mustache, and vaulttools_details.mustache. Note that the latter approach changes the rendering pipeline (line breaks, etc.) and may need an nl2br step.
api::prepare_encryption_key() derives the S3 SSE-C encryption key from the user's passphrase as base64_encode(hash('sha256', $passphrase, true)) — one round of SHA-256, no salt, no key-stretching (PBKDF2/Argon2/scrypt).
This is the AES-256 customer-provided key that AWS S3 uses to encrypt the backup at rest. Because backups must be deterministically decryptable on a different site (so the same passphrase always yields the same key), the design has no place for a per-backup salt. However, the absence of any iteration count means that an attacker who exfiltrates an encrypted backup file can mount an offline brute-force attack against the passphrase at near-hardware-native speed (billions of guesses per second on a GPU), so the encryption strength reduces to the entropy of the passphrase.
Informational. Not exploitable in isolation. The recommendation is a defence-in-depth hardening for high-value backups stored in environments where the cloud bucket itself might leak (publicly-misconfigured bucket, employee insider, etc.).
The derived key is sent in the x-amz-server-side-encryption-customer-key header to AWS during multipart upload and download. AWS itself never persists the key — it is recomputed from the passphrase on each transfer. Brute force requires the attacker to first obtain the encrypted ciphertext directly from S3, which itself requires either an S3 leak or compromise of the API.
public static function prepare_encryption_key(?string $passphrase): string {
return strlen($passphrase) ? base64_encode(hash('sha256', $passphrase, true)) : '';
}
If a deterministic, salt-less derivation must be retained, document the requirement that passphrases be high-entropy (e.g. ≥ 20 random characters) and enforce a minimum length / character-class policy in start_backup_form::validation(). A more thorough fix would derive the key with PBKDF2-SHA256 using a fixed, public, plugin-versioned salt and a high iteration count (hash_pbkdf2('sha256', $passphrase, 'tool_vault/v1', 600000, 32, true)), accepting the one-time migration cost for existing encrypted backups.
site_restore::restore_db() issues raw ALTER/CREATE/DROP statements through $DB->change_database_structure($altersql) and $DB->get_manager()->drop_table() outside db/upgrade.php. This is unavoidable for a backup/restore tool — the whole point is to bring the local schema into agreement with the schema captured in the backup — but it is worth flagging because the rule of thumb is otherwise that DDL belongs only in db/upgrade.php.
The DDL is generated entirely from the XMLDB structure parsed out of the backup's __structure__.xml and the corresponding dbtable::get_alter_sql() derivation, so the input is a parsed XMLDB tree rather than a free-form SQL string from the backup; injection into $altersql would require both crafting a malicious backup file and bypassing AWS S3 encryption / passphrase validation.
Informational. This is intrinsic to the plugin's purpose. Documented here only so that downstream reviewers do not flag it again; the layered controls (admin capability, restores-allowed setting, valid passphrase, integrity check on download) make it safe in practice.
Restore is admin-only, gated by api::are_restores_allowed() (off by default), and additionally requires a valid backup key + passphrase that pass api::validate_backup() (which performs an authenticated HEAD against the AWS S3 presigned URL with the SSE-C key derived from the passphrase).
if ($altersql = $table->get_alter_sql($originaltable)) {
try {
$DB->change_database_structure($altersql);
foreach ($extratables as $table) {
$DB->get_manager()->drop_table($table->get_xmldb_table());
}
log_capture::force_debug() overrides $CFG->debug, $CFG->debugdisplay, and $CFG->debugdeveloper to maximum values whenever the forcedebug plugin setting is enabled (which it is by default). The original values are restored by reset_debug() at the end of the operation and inside recalc_version_hash::reinitialise_cfg().
In normal CLI/cron execution this is harmless — there are no concurrent HTTP responses to leak debug output to. The tool_vault_after_config callback also forces every other request into maintenance mode while a backup/restore is active, so the elevated debug values cannot reach end users via a normal page load.
However, the override is process-wide and any unrelated work picked up by the same PHP request after force_debug() runs (none in the current code, but possible in future) would inherit the elevated values until reset_debug() ran. This is an architectural note rather than an issue.
Informational. No exploitation path identified.
Combined with the maintenance-mode gate, this is safe in the shipped invocation patterns (cron + CLI). The plugin documents the behaviour in the forcedebug setting description (settings_forcedebug_desc).
if (version_compare(phpversion(), '8.4', '<')) {
$CFG->debug = (E_ALL | E_STRICT);
} else {
$CFG->debug = (E_ALL);
}
$CFG->debugdisplay = 1;
$CFG->debugdeveloper = true;
Bundled core upgrade scripts. classes/local/restoreactions/upgrade_311/, upgrade_401/, upgrade_402/, and upgrade_404/ contain near-verbatim copies of Moodle core upgrade files re-namespaced under tool_vault_*_xmldb_*_upgrade. The plugin uses these to allow restore from older Moodle versions directly to the current site version (e.g. 3.9 → 4.4). They carry phpcs:ignoreFile and Mdlcode-disable markers and are intentionally divergent from normal coding-style rules. Issues inside these files were not separately reported, since they reflect the historical core code by design.
Maintenance-mode coverage. While a backup or restore is running, both the modern hook callback (tool_vault\hook_callbacks::after_config) and the legacy global function (tool_vault_after_config in lib.php) intercept every request and either abort the CLI or redirect the page to the standard Moodle maintenance message — except for progress.php, which is intentionally allowed through so admins can monitor progress. The redirect path uses print_maintenance_message() and adds X-Tool-Vault: true to make the special state easy to detect from monitoring.
Access-key endpoint. progress.php deliberately bypasses login (NO_MOODLE_COOKIES) and authenticates the viewer via a 32-character random accesskey stored on the operation row. The page is read-only (no state changes) and has a unique DB index on accesskey, eliminating the risk of cross-operation collisions. The accesskey is generated with random_string(32), which on Moodle 4.5+ uses random_int — sufficiently strong against brute force.
Privacy provider. tool_vault\privacy\provider declares the plugin as a null_provider and registers an external location link (lmsvault.io) covering all data. This is the right pattern given that the plugin itself does not read user data per request, but ships the entire site (including all user data) to the cloud.