Pre-beta review: safer destructive commands and hardening

- backup:restore:full refuses on an installation with more than one user
  and asks before it starts (--force skips both, also to rerun after a
  failure halfway), and warns when the snapshot's database dump was not
  one consistent transaction.
- backup:user:restore --mode=replace asks before deleting an existing
  account; non-interactive use needs --force.
- The recovery kit file is created owner-only and never over an existing
  file or symlink; backup:key:import shows the deletion delay it read from
  the kit.
- Adding a location through the web API needs password confirmation, like
  removing one.
- Key derivation parameters from a kit or repository config are bounded,
  and a stalled network stream no longer spins at full CPU.
- Floating-point columns are dumped exactly instead of with 14 digits.

docs/HOWTO.md

@@ -102,7 +102,9 @@ occ backup:user:restore offsite users/<uid>/<exportId> --mode=as-backup
   use, add `--install-user-migration` to approve that explicitly. The command stops after the
   installation; run it once more.
 - `replace` first makes a safety export of the current account, then recreates the user under the
-  same id. `as-backup` restores into `<uid>-bak` and leaves the original untouched.
+  same id. When the account exists, the command asks first; in scripts, add `--force`. Shares to
+  others and data of apps without a user_migration migrator are not restored, so prefer
+  `as-backup` when in doubt: it restores into `<uid>-bak` and leaves the original untouched.
 - A restore onto a server where the user does not exist works. Calendars and address books get an
   internal name with the prefix `migrated-` (for example `migrated-personal`). That prefix comes from
   user_migration and cannot be turned off. The user can change the display name of these calendars and
@@ -124,6 +126,9 @@ occ backup:list offsite                              # the snapshots must be lis
 occ backup:restore:full offsite <snapshot>
 ```
 
+The command refuses on an installation that already has more than one user, and asks before it
+starts; `--force` skips both (also needed to rerun it after it stopped halfway).
+
 Afterwards, check that users, files and the database are complete, and run a backup of the restored
 server. The restored installation keeps its own app configuration and secret.
 

lib/Command/KeyImport.php

@@ -76,6 +76,9 @@ HELP);
 			return 1;
 		}
 		$output->writeln('Backup key imported, fingerprint <info>' . $this->keys->fingerprint() . '</info>.');
+		// Read from the kit file, which is not authenticated beyond the wrapped key: show it so a
+		// wrong or altered value stands out.
+		$output->writeln('Deletion delay for forgotten snapshots: <info>' . $this->keys->deleteDelayDays() . '</info> days (from the kit).');
 		$output->writeln('Next: connect the backup location with the same details as before (<info>occ backup:target:add</info>), then <info>occ backup:list</info> and <info>occ backup:restore:full</info>.');
 		return 0;
 	}

lib/Command/KeyKit.php

@@ -39,11 +39,18 @@ final class KeyKit extends Command {
 			$output->write($json, false, OutputInterface::OUTPUT_RAW);
 			return 0;
 		}
-		if (file_put_contents($file, $json) === false) {
-			$output->writeln("<error>Cannot write $file</error>");
+		// Created owner-only from the start (no window where it is world-readable), never over an
+		// existing file or symlink someone else may have placed at a predictable path.
+		$old = umask(0077);
+		try {
+			$fh = @fopen($file, 'xb');
+		} finally {
+			umask($old);
+		}
+		if ($fh === false || fwrite($fh, $json) !== strlen($json) || !fclose($fh)) {
+			$output->writeln("<error>Cannot write $file (it must not exist yet)</error>");
 			return 1;
 		}
-		@chmod($file, 0600);
 		$output->writeln("Recovery kit written to <info>$file</info>. Copy it off this server, then run <info>occ backup:key:confirm</info>.");
 		return 0;
 	}

lib/Command/RestoreFull.php

@@ -9,7 +9,11 @@ namespace OCA\NgBackup\Command;
 
 use OCA\NgBackup\Service\RestoreFullService;
 use OCA\NgBackup\Service\TargetService;
+use OCP\IUserManager;
 use Symfony\Component\Console\Command\Command;
+use Symfony\Component\Console\Helper\QuestionHelper;
+use Symfony\Component\Console\Input\InputOption;
+use Symfony\Component\Console\Question\ConfirmationQuestion;
 use Symfony\Component\Console\Input\InputArgument;
 use Symfony\Component\Console\Input\InputInterface;
 use Symfony\Component\Console\Output\OutputInterface;
@@ -18,6 +22,7 @@ final class RestoreFull extends Command {
 	public function __construct(
 		private TargetService $targets,
 		private RestoreFullService $restoreFull,
+		private IUserManager $userManager,
 	) {
 		parent::__construct();
 	}
@@ -27,6 +32,7 @@ final class RestoreFull extends Command {
 			->setDescription('Disaster recovery: restore the database, data directory and config on a fresh installation')
 			->addArgument('target', InputArgument::REQUIRED, 'Location name or id')
 			->addArgument('snapshot', InputArgument::REQUIRED, 'Snapshot id (see backup:list)')
+			->addOption('force', null, InputOption::VALUE_NONE, 'Also on an installation that already has more than one user, and without asking')
 			->setHelp(<<<'HELP'
 For a FRESH Nextcloud installation only, same major version as the backup, right after
 occ maintenance:install, occ backup:key:import and occ backup:target:add -- before anyone else
@@ -50,12 +56,29 @@ HELP);
 	}
 
 	protected function execute(InputInterface $input, OutputInterface $output): int {
+		$force = (bool)$input->getOption('force');
+		// A whole-server restore replaces the database and data directory of THIS instance. On
+		// anything but a fresh install (only the admin from occ maintenance:install) that is
+		// almost certainly a mistake, so it needs --force.
+		$users = $this->userManager->countUsersTotal(2);
+		if (!$force && ($users === false || $users > 1)) {
+			$output->writeln('<error>This installation already has more than one user: backup:restore:full would replace its whole database and data directory. It is meant for a fresh installation. Use --force if this really is what you want.</error>');
+			return 1;
+		}
+		if (!$force && $input->isInteractive()) {
+			$q = new ConfirmationQuestion('This replaces the database, the data directory and config.php of this installation. Continue? [y/N] ', false);
+			if (!(new QuestionHelper())->ask($input, $output, $q)) {
+				$output->writeln('Cancelled.');
+				return 1;
+			}
+		}
 		try {
 			$target = $this->targets->get($input->getArgument('target'));
 			$snapshot = (string)$input->getArgument('snapshot');
 			$result = $this->restoreFull->restoreFull($target, $snapshot);
 		} catch (\Throwable $e) {
 			$output->writeln('<error>' . $e->getMessage() . '</error>');
+			$output->writeln('If this happened after the restore had started, the instance may be partly restored: fix the cause and run the same command again with --force.');
 			return 1;
 		}
 		$output->writeln(sprintf(
@@ -65,6 +88,9 @@ HELP);
 			$result['configKeys'],
 			$result['dataFiles'],
 		));
+		if (!$result['dbConsistent']) {
+			$output->writeln('<comment>Warning: this snapshot\'s database dump was made over several steps (web cron), so it is not one consistent point in time. Check the instance carefully; prefer a snapshot made with system cron.</comment>');
+		}
 		$output->writeln('Next: check trusted_domains and overwritehost/overwriteprotocol match this server, then occ maintenance:repair.');
 		return 0;
 	}

lib/Command/RestoreUser.php

@@ -10,7 +10,10 @@ namespace OCA\NgBackup\Command;
 use OCA\NgBackup\Service\TargetService;
 use OCA\NgBackup\Service\UserRestoreService;
 use OCA\NgBackup\UserMigration\UserMigrationInstaller;
+use OCP\IUserManager;
 use Symfony\Component\Console\Command\Command;
+use Symfony\Component\Console\Helper\QuestionHelper;
+use Symfony\Component\Console\Question\ConfirmationQuestion;
 use Symfony\Component\Console\Input\InputArgument;
 use Symfony\Component\Console\Input\InputInterface;
 use Symfony\Component\Console\Input\InputOption;
@@ -21,6 +24,7 @@ final class RestoreUser extends Command {
 		private TargetService $targets,
 		private UserRestoreService $userRestore,
 		private UserMigrationInstaller $installer,
+		private IUserManager $userManager,
 	) {
 		parent::__construct();
 	}
@@ -31,6 +35,7 @@ final class RestoreUser extends Command {
 			->addArgument('target', InputArgument::REQUIRED, 'Location name or id')
 			->addArgument('manifest', InputArgument::REQUIRED, 'Manifest path printed by backup:user:backup (users/<uid>/<exportId>)')
 			->addOption('mode', 'm', InputOption::VALUE_REQUIRED, 'replace or as-backup', UserRestoreService::MODE_REPLACE)
+			->addOption('force', null, InputOption::VALUE_NONE, 'Replace an existing account without asking (required when not interactive)')
 			->addOption('install-user-migration', null, InputOption::VALUE_NONE, 'Install and enable user_migration if it is missing (non-interactive consent)')
 			->setHelp(<<<'HELP'
 Never imports over a live account. Modes:
@@ -47,11 +52,26 @@ HELP);
 	}
 
 	protected function execute(InputInterface $input, OutputInterface $output): int {
+		$mode = (string)$input->getOption('mode');
+		$uid = explode('/', (string)$input->getArgument('manifest'))[1] ?? '';
+		if ($mode === UserRestoreService::MODE_REPLACE && $uid !== '' && $this->userManager->userExists($uid) && !$input->getOption('force')) {
+			// Replace deletes the live account (after a safety export). Shares to others and data of
+			// apps without a user_migration migrator do not come back, so never do it unasked.
+			if (!$input->isInteractive()) {
+				$output->writeln("<error>$uid exists: --mode=replace deletes and recreates it. Add --force, or use --mode=as-backup.</error>");
+				return 1;
+			}
+			$q = new ConfirmationQuestion("$uid exists and will be deleted and recreated from the backup (a safety export is made first; its shares to others and data of apps without a migrator are not restored). Continue? [y/N] ", false);
+			if (!(new QuestionHelper())->ask($input, $output, $q)) {
+				$output->writeln('Cancelled.');
+				return 1;
+			}
+		}
 		try {
 			$output->writeln('<comment>Note: on restore, user_migration only imports the app settings on its own allowlist (e.g. calendar view and reminder settings); other app settings of this user are skipped. Reason: NG Backup uses user_migration\'s importer; widening the list would mean building a settings importer of our own.</comment>');
 			$target = $this->targets->get($input->getArgument('target'));
 			$this->installer->ensureEnabled($input, $output);
-			$user = $this->userRestore->restoreUser($target, (string)$input->getArgument('manifest'), (string)$input->getOption('mode'));
+			$user = $this->userRestore->restoreUser($target, (string)$input->getArgument('manifest'), $mode);
 		} catch (\Throwable $e) {
 			$output->writeln('<error>' . $e->getMessage() . '</error>');
 			return 1;

lib/Controller/AdminApiController.php

@@ -168,6 +168,7 @@ final class AdminApiController extends Controller {
 		return new JSONResponse($this->targets->availableBackends());
 	}
 
+	#[PasswordConfirmationRequired]
 	public function addTarget(string $name, string $backend, string $auth, array $options = [], string $path = 'ng_backup', ?float $maxGb = null): JSONResponse {
 		try {
 			$r = $this->targets->add($name, $backend, $auth, $options, $path);

lib/Crypto/KeyRing.php

@@ -96,6 +96,12 @@ final class KeyRing {
 	/** @param array{v:int, salt:string, ops:int, mem:int, nonce:string, key:string} $wrapped */
 	public static function unwrap(array $wrapped, #[\SensitiveParameter] string $passphrase): self {
 		$b64 = static fn (string $s): string => sodium_base642bin($s, SODIUM_BASE64_VARIANT_ORIGINAL);
+		// ops/mem come from unauthenticated JSON (kit file, repository config): bound them, so a
+		// damaged or crafted file cannot make the KDF use unbounded memory or time.
+		if ($wrapped['ops'] < SODIUM_CRYPTO_PWHASH_OPSLIMIT_INTERACTIVE || $wrapped['ops'] > SODIUM_CRYPTO_PWHASH_OPSLIMIT_SENSITIVE
+			|| $wrapped['mem'] < SODIUM_CRYPTO_PWHASH_MEMLIMIT_INTERACTIVE || $wrapped['mem'] > SODIUM_CRYPTO_PWHASH_MEMLIMIT_SENSITIVE) {
+			throw new CryptoException('Unsupported key derivation parameters (damaged or foreign key file)');
+		}
 		$kek = sodium_crypto_pwhash(SODIUM_CRYPTO_SECRETBOX_KEYBYTES, $passphrase, $b64($wrapped['salt']),
 			$wrapped['ops'], $wrapped['mem'], SODIUM_CRYPTO_PWHASH_ALG_ARGON2ID13);
 		$master = sodium_crypto_secretbox_open($b64($wrapped['key']), $b64($wrapped['nonce']), $kek);

lib/Crypto/StreamCipher.php

@@ -122,6 +122,7 @@ final class StreamCipher {
 	/** fread() may return short reads on network streams; keep reading until $length or EOF. */
 	private static function readFull($in, int $length): string {
 		$buffer = '';
+		$empty = 0;
 		while (strlen($buffer) < $length && !feof($in)) {
 			$chunk = fread($in, $length - strlen($buffer));
 			if ($chunk === false) {
@@ -132,8 +133,15 @@ final class StreamCipher {
 				if (feof($in)) {
 					break;
 				}
+				// A stalled network stream returns '' without EOF forever: back off, then give up
+				// (about a minute) instead of spinning at full CPU until the lease expires.
+				if (++$empty > 600) {
+					throw new CryptoException('Read stalled');
+				}
+				usleep(100000);
 				continue;
 			}
+			$empty = 0;
 			$buffer .= $chunk;
 		}
 		return $buffer;

lib/Db/DbDumper.php

@@ -256,7 +256,11 @@ final class DbDumper {
 					} elseif (is_string($row[$i]) && !mb_check_encoding($row[$i], 'UTF-8')) {
 						// Keep invalid UTF-8 byte-exact instead of substituting characters.
 						$row[$i] = ['$b' => base64_encode($row[$i])];
-					} elseif (is_int($row[$i]) || is_float($row[$i]) || is_bool($row[$i])) {
+					} elseif (is_float($row[$i])) {
+						// var_export uses serialize_precision (shortest exact form), not the lossy
+						// 14-digit 'precision' of a (string) cast.
+						$row[$i] = var_export($row[$i], true);
+					} elseif (is_int($row[$i]) || is_bool($row[$i])) {
 						// Normalise: drivers differ in returning numbers as int or string.
 						$row[$i] = (string)(is_bool($row[$i]) ? (int)$row[$i] : $row[$i]);
 					}

lib/Service/RestoreFullService.php

@@ -64,7 +64,7 @@ final class RestoreFullService {
 		];
 	}
 
-	/** @return array{dbReport:array, dataFiles:int, configKeys:int} */
+	/** @return array{dbReport:array, dataFiles:int, configKeys:int, dbConsistent:bool} */
 	public function restoreFull(Target $target, string $snapshotId): array {
 		$check = $this->checkCompatibility($target, $snapshotId);
 		if (!$check['ok']) {
@@ -76,6 +76,7 @@ final class RestoreFullService {
 			$meta = $repo->snapshotMeta($snapshotId);
 
 			$dbReport = null;
+			$manifest = [];
 			if (isset($meta['db'])) {
 				$manifest = json_decode($repo->getObject($meta['db']), true, 512, JSON_THROW_ON_ERROR);
 				$prefix = $this->config->getSystemValueString('dbtableprefix', 'oc_');
@@ -124,7 +125,8 @@ final class RestoreFullService {
 				);
 			}
 
-			return ['dbReport' => $dbReport ?? ['skipped' => [], 'restored' => []], 'dataFiles' => $dataFiles, 'configKeys' => count($backupConfig)];
+			return ['dbReport' => $dbReport ?? ['skipped' => [], 'restored' => []], 'dataFiles' => $dataFiles, 'configKeys' => count($backupConfig),
+				'dbConsistent' => (bool)($manifest['consistent'] ?? true)];
 		});
 	}
 

tests/integration/user-roundtrip.sh

@@ -48,7 +48,8 @@ ok "as-backup restores into <uid>-bak, byte-identical, original account untouche
 # replace: damage the account, then restore it in place under the same uid.
 $X sh -c "rm '$DATA/$U/files/Docs/f1.bin' && echo damaged > '$DATA/$U/files/Docs/f1.bin'"
 occ files:scan "$U" >/dev/null
-occ backup:user:restore ci "$MANIFEST" --mode=replace >/dev/null || fail "restore replace"
+if occ backup:user:restore ci "$MANIFEST" --mode=replace --no-interaction >/dev/null 2>&1; then fail "replace of an existing account ran without --force"; fi
+occ backup:user:restore ci "$MANIFEST" --mode=replace --force >/dev/null || fail "restore replace"
 occ files:scan "$U" >/dev/null
 SUM_AFTER=$($X sh -c "cd $DATA/$U/files && find . -type f -print0 | sort -z | xargs -0 md5sum | md5sum")
 [ "$SUM_BEFORE" = "$SUM_AFTER" ] || fail "files differ after a replace restore"