Ensembia/nextcloud-ng_backup
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases 17 Wiki Activity

Fixes from the phase-1 code review

Browse Source 
- one locking protocol per location: backup steps and restores hold a
  shared lock, prune and untrash an exclusive one and refuse while any
  run (backup, restore, export) exists; starting a backup checks and
  creates the run under the exclusive lock, closing the race with prune
- snapshot entries are validated when read (relative path without '.',
  '..', NUL, backslash or control characters); raw restore also checks
  that the resolved directory stays below the target; the restore API
  and service accept canonical paths only
- recovery kit confirmation requires a recent password confirmation and
  the confirmation code printed in the current kit file (new code per
  kit version), proving the kit was actually downloaded
- prune decides on the authenticated index files on the location and
  refreshes the local index cache from them
- restore job marks the run failed on any error; restore runs without
  progress for 6 hours are failed so they cannot block cleanup forever
- tests for safe paths and a malicious snapshot path
This commit is contained in:
Dillard Blom
2026-10-02 11:21:40 +02:00
parent 9b03f98d0d
commit 7bde1cfdf0
14 changed files with 226 additions and 40 deletions
Download Patch File Download Diff File Expand all files Collapse all files
lib/Service/BackupService.php
+24 -2
View File
@@ -52,9 +52,22 @@ class BackupService {
if ($this->config->getSystemValue('objectstore', null) !== null) {
throw new \RuntimeException('Object storage as primary storage is not supported yet');
}
if ($this->locking->isLocked(PruneService::lockKey($target), ILockingProvider::LOCK_EXCLUSIVE)) {
throw new \RuntimeException('A cleanup of ' . $target->getName() . ' is running; try again later');
// Check and create the run under the location's exclusive lock, so a cleanup cannot start
// in between (it takes the same lock and refuses while a run exists).
$lock = PruneService::lockKey($target);
try {
$this->locking->acquireLock($lock, ILockingProvider::LOCK_EXCLUSIVE);
} catch (LockedException) {
throw new \RuntimeException('A cleanup or another start for ' . $target->getName() . ' is running; try again later');
}
try {
return $this->createRun($target, $label);
} finally {
$this->locking->releaseLock($lock, ILockingProvider::LOCK_EXCLUSIVE);
}
}
private function createRun(Target $target, string $label): Run {
$running = $this->runs->findRunning($target->getId(), self::KIND_FULL);
if ($running !== []) {
return $running[0];
@@ -96,6 +109,14 @@ class BackupService {
} catch (LockedException) {
return $run;
}
// Shared lock on the location while this step writes: a cleanup (exclusive) cannot run now.
$targetLock = 'ng_backup/target/' . $run->getTargetId();
try {
$this->locking->acquireLock($targetLock, ILockingProvider::LOCK_SHARED);
} catch (LockedException) {
$this->locking->releaseLock($lockKey, ILockingProvider::LOCK_EXCLUSIVE);
return $run;
}
try {
$run = $this->runs->find($run->getId()); // fresh state after acquiring the lock
if ($run->getStatus() !== Run::RUNNING || $run->getKind() !== self::KIND_FULL) {
@@ -140,6 +161,7 @@ class BackupService {
}
return $run;
} finally {
$this->locking->releaseLock($targetLock, ILockingProvider::LOCK_SHARED);
$this->locking->releaseLock($lockKey, ILockingProvider::LOCK_EXCLUSIVE);
}
}
lib/Service/KeyService.php
+32
View File
@@ -29,6 +29,7 @@ class KeyService {
private const KEY_FINGERPRINT = 'master_key_fingerprint';
private const KEY_DELETE_DELAY = 'delete_delay_days';
private const KEY_KIT_VERSION = 'recovery_kit_version';
private const KEY_KIT_CODE = 'recovery_kit_code';
public const DEFAULT_DELETE_DELAY = 7;
private ?KeyRing $cached = null;
@@ -188,6 +189,7 @@ class KeyService {
'app' => 'NG Backup',
'fingerprint' => $this->fingerprint(),
'kit_version' => $this->kitVersion(),
'confirmation_code' => $this->kitCode(),
'created' => gmdate('c'),
'wrapped_key' => $this->wrappedKey(),
'slots' => $this->slots(),
@@ -197,6 +199,36 @@ class KeyService {
];
}
/**
* Short code printed in the current kit file. Typing it back when confirming proves the
* admin actually has the downloaded kit. A new code per kit version; the server only keeps
* its own copy encrypted with the instance secret.
*/
private function kitCode(): string {
$stored = json_decode($this->appConfig->getValueString(Application::APP_ID, self::KEY_KIT_CODE, '{}', true), true) ?: [];
if (($stored['version'] ?? 0) === $this->kitVersion() && isset($stored['code'])) {
return $this->crypto->decrypt($stored['code']);
}
$alphabet = 'ABCDEFGHJKLMNPQRSTUVWXYZ23456789'; // no 0/O, 1/I
$code = '';
for ($i = 0; $i < 8; $i++) {
$code .= $alphabet[random_int(0, strlen($alphabet) - 1)];
}
$code = substr($code, 0, 4) . '-' . substr($code, 4);
$this->appConfig->setValueString(Application::APP_ID, self::KEY_KIT_CODE,
json_encode(['version' => $this->kitVersion(), 'code' => $this->crypto->encrypt($code)], JSON_THROW_ON_ERROR), true, true);
return $code;
}
public function checkKitCode(string $code): bool {
$stored = json_decode($this->appConfig->getValueString(Application::APP_ID, self::KEY_KIT_CODE, '{}', true), true) ?: [];
if (($stored['version'] ?? 0) !== $this->kitVersion() || !isset($stored['code'])) {
return false; // no kit downloaded for this version yet
}
$normalise = static fn (string $c) => strtoupper(preg_replace('/[^A-Za-z0-9]/', '', $c) ?? '');
return hash_equals($normalise($this->crypto->decrypt($stored['code'])), $normalise($code));
}
public function confirmRecoveryKit(string $uid): void {
$this->appConfig->setValueString(Application::APP_ID, self::KEY_CONFIRMED,
json_encode(['uid' => $uid, 'time' => time(), 'fingerprint' => $this->fingerprint(), 'kit_version' => $this->kitVersion()], JSON_THROW_ON_ERROR), true);
lib/Service/PruneService.php
+26 -8
View File
@@ -33,6 +33,31 @@ class PruneService {
}
public function untrash(Target $target, string $snapshotId): void {
$this->lockExclusive($target);
try {
$this->doUntrash($target, $snapshotId);
} finally {
$this->locking->releaseLock(self::lockKey($target), ILockingProvider::LOCK_EXCLUSIVE);
}
}
/**
* Exclusive lock on the location, then make sure no run of any kind (backup, restore, export)
* is in progress: their data may not be referenced by a snapshot yet, or is being read.
*/
private function lockExclusive(Target $target): void {
try {
$this->locking->acquireLock(self::lockKey($target), ILockingProvider::LOCK_EXCLUSIVE);
} catch (LockedException) {
throw new \RuntimeException('A backup, restore or cleanup of ' . $target->getName() . ' is in progress; try again later');
}
if ($this->runs->findRunning($target->getId()) !== []) {
$this->locking->releaseLock(self::lockKey($target), ILockingProvider::LOCK_EXCLUSIVE);
throw new \RuntimeException('A backup or restore of ' . $target->getName() . ' is running; try again later');
}
}
private function doUntrash(Target $target, string $snapshotId): void {
$repo = $this->targets->repository($target);
if (!array_key_exists($snapshotId, $repo->trash())) {
throw new \InvalidArgumentException('That snapshot is not in the trash (or was already deleted)');
@@ -84,14 +109,7 @@ class PruneService {
if ($target->getAppendOnly()) {
throw new \RuntimeException('Location ' . $target->getName() . ' is append-only: NG Backup does not delete anything there');
}
if ($this->runs->findRunning($target->getId(), BackupService::KIND_FULL) !== []) {
throw new \RuntimeException('A backup to ' . $target->getName() . ' is running; try again later');
}
try {
$this->locking->acquireLock(self::lockKey($target), ILockingProvider::LOCK_EXCLUSIVE);
} catch (LockedException) {
throw new \RuntimeException('Another backup or cleanup of ' . $target->getName() . ' is running');
}
$this->lockExclusive($target);
try {
$repo = $this->targets->repository($target);
$times = [];
lib/Service/RestoreService.php
+22 -2
View File
@@ -36,9 +36,25 @@ class RestoreService {
public function __construct(
private TargetService $targets,
private IRootFolder $root,
private \OCP\Lock\ILockingProvider $locking,
) {
}
/** Shared lock on the location while restoring: a cleanup (exclusive) must not delete what is being read. */
private function withSharedLock(Target $target, callable $fn): mixed {
$key = PruneService::lockKey($target);
try {
$this->locking->acquireLock($key, \OCP\Lock\ILockingProvider::LOCK_SHARED);
} catch (\OCP\Lock\LockedException) {
throw new \RuntimeException('A cleanup of ' . $target->getName() . ' is running; try again later');
}
try {
return $fn();
} finally {
$this->locking->releaseLock($key, \OCP\Lock\ILockingProvider::LOCK_SHARED);
}
}
/**
* Direct children (files and folders) of a path in a snapshot.
*
@@ -72,7 +88,11 @@ class RestoreService {
* @return array{restored:int, trashed:int, target:string}
*/
public function restoreUserFiles(Target $target, string $snapshotId, string $logicalPath, string $mode = self::MODE_NEW_FOLDER, ?callable $progress = null): array {
if (!preg_match('#^data/([^/]+)/files(?:/(.*))?$#', trim($logicalPath, '/'), $m)) {
return $this->withSharedLock($target, fn () => $this->doRestoreUserFiles($target, $snapshotId, $logicalPath, $mode, $progress));
}
private function doRestoreUserFiles(Target $target, string $snapshotId, string $logicalPath, string $mode, ?callable $progress): array {
if (!Repository::isSafePath(trim($logicalPath, '/')) || !preg_match('#^data/([^/]+)/files(?:/(.*))?$#', trim($logicalPath, '/'), $m)) {
throw new \InvalidArgumentException('Expected a path like data/<user>/files/<folder or file>');
}
[$uid, $rel] = [$m[1], trim($m[2] ?? '', '/')];
@@ -127,7 +147,7 @@ class RestoreService {
if (!is_dir($directory) && !mkdir($directory, 0750, true) && !is_dir($directory)) {
throw new \RuntimeException("Cannot create $directory");
}
return $this->targets->repository($target)->restore($snapshotId, $directory, trim($prefix, '/') === '' ? '' : trim($prefix, '/') . '/');
return $this->withSharedLock($target, fn () => $this->targets->repository($target)->restore($snapshotId, $directory, trim($prefix, '/') === '' ? '' : trim($prefix, '/') . '/'));
}
private function writeFile(Repository $repo, Folder $dest, string $relPath, array $entry): void {