From 63505d2aab9bdc9b06328eba98dbbf363863c04f Mon Sep 17 00:00:00 2001 From: blaipr Date: Thu, 3 Sep 2026 02:10:06 +0200 Subject: [PATCH] fix: an archive and an export are private from the moment they exist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ArchiveHandler and XmlExport both chmod 0600 after the write completes, and buildFromDirectory() walks the whole application tree first — so on an installation of any size the finished file sits at the process umask, measured 0644, for as long as building it took, and a run that dies in between leaves it that way for good. Those files hold the database dump with every account's encrypted secret and the master-password hash, config.xml with the credentials and crypto keys, and for the export with no password the name, login, URL and notes of every account in the clear. FileBackupHandlersFactory already shows the right shape, restricting database.sql before any write. PharData offers no equivalent: it does not create the archive when constructed, so there is nothing to chmod beforehand, and compress() refuses when its target already exists. Narrowing the umask for the duration covers the tar, the gz and the export alike, with no window at all, and it is restored in a finally — leaking 0177 into the rest of the request would quietly make every later file owner-only. The chmods stay as the guarantee. The mutation result separates the two: with the chmod removed the archive is still private, and only removing the umask as well fails it. --- src/Application/Export/Services/XmlExport.php | 20 +- src/Infrastructure/File/ArchiveHandler.php | 61 +++-- .../File/ArchiveHandlerTest.php | 208 ++++++++++++++++++ 3 files changed, 266 insertions(+), 23 deletions(-) create mode 100644 tests/Unit/Infrastructure/File/ArchiveHandlerTest.php diff --git a/src/Application/Export/Services/XmlExport.php b/src/Application/Export/Services/XmlExport.php index fe0d1f563..9692528d4 100644 --- a/src/Application/Export/Services/XmlExport.php +++ b/src/Application/Export/Services/XmlExport.php @@ -179,15 +179,27 @@ private function buildAndSaveXml(string $file, ?string $password = null): void $this->appendNode($this->xmlAccountExportService->export(), $password); $this->appendHash($password); - if (!$this->document->save($file)) { - throw ServiceException::error(__u('Error while creating the XML file')); - } - // The backup archives are restricted to their owner; this was not, and it holds the // same installation. Every account's encrypted secret and its key are in here, and // when no export password was given so is everything around them in the clear — the // name, the login, the URL and the notes of every account. At the default umask that // is a world-readable file, which on a shared host is every local user. + // + // The umask is what makes it private *from the moment it exists*. A chmod after the + // write leaves the whole file readable for as long as writing it takes, which for an + // installation of any size is not an instant — and leaves it readable indefinitely if + // the process dies in between. The chmod stays as the guarantee: the umask decides + // what a newly created file gets, and nothing here should depend on that alone. + $umask = umask(0177); + + try { + if (!$this->document->save($file)) { + throw ServiceException::error(__u('Error while creating the XML file')); + } + } finally { + umask($umask); + } + @chmod($file, 0600); } catch (ServiceException $e) { throw $e; diff --git a/src/Infrastructure/File/ArchiveHandler.php b/src/Infrastructure/File/ArchiveHandler.php index 5c41ab0c7..b7388c0a9 100644 --- a/src/Infrastructure/File/ArchiveHandler.php +++ b/src/Infrastructure/File/ArchiveHandler.php @@ -37,6 +37,17 @@ */ class ArchiveHandler implements ArchiveHandlerInterface { + /** + * The umask held while the archives are written, so they are private from the moment they + * exist rather than from the moment the chmod runs. + * + * `PharData` gives no way to do this per file: it does not create the archive when it is + * constructed, so there is nothing to restrict beforehand, and `compress()` refuses outright + * when its target already exists — "phar ... exists and must be unlinked prior to conversion". + * The umask is what is left, and it covers both the tar and the gz. + */ + private const OWNER_ONLY = 0177; + private readonly PharData $archive; public function __construct(string $archive, PhpExtensionCheckerService $phpExtensionCheckerService) @@ -59,20 +70,26 @@ private static function makeArchiveName(string $archive): string */ public function compressDirectory(string $directory, ?string $regex = null): string { - $this->archive->buildFromDirectory($directory, $regex); + $umask = umask(self::OWNER_ONLY); + + try { + $this->archive->buildFromDirectory($directory, $regex); - // Before compressing, not only after: the uncompressed archive holds the same thing the - // compressed one does and exists for as long as compressing takes, which on a large - // installation is not an instant. `database.sql` is restricted the moment it is opened for - // exactly this reason; this is the same window, left open. - $this->restrictToOwner($this->archive->getPath()); + // Before compressing, not only after: the uncompressed archive holds the same thing + // the compressed one does and exists for as long as compressing takes, which on a + // large installation is not an instant. `database.sql` is restricted the moment it is + // opened for exactly this reason; this is the same window, left open. + $this->restrictToOwner($this->archive->getPath()); - $packed = $this->archive->compress(Phar::GZ); + $packed = $this->archive->compress(Phar::GZ); - // Delete the non-compressed archive - (new FileHandler($this->archive->getPath()))->delete(); + // Delete the non-compressed archive + (new FileHandler($this->archive->getPath()))->delete(); - $this->restrictToOwner($this->archive->getPath() . '.gz'); + $this->restrictToOwner($this->archive->getPath() . '.gz'); + } finally { + umask($umask); + } return $packed->getFileInfo()->getPathname(); } @@ -84,19 +101,25 @@ public function compressDirectory(string $directory, ?string $regex = null): str */ public function compressFile(string $file): string { - $this->archive->addFile($file, basename($file)); + $umask = umask(self::OWNER_ONLY); + + try { + $this->archive->addFile($file, basename($file)); - // See compressDirectory(): the uncompressed archive is as sensitive as the compressed one - // and lives for as long as compressing takes. - $this->restrictToOwner($this->archive->getPath()); + // See compressDirectory(): the uncompressed archive is as sensitive as the compressed + // one and lives for as long as compressing takes. + $this->restrictToOwner($this->archive->getPath()); - $packed = $this->archive->compress(Phar::GZ); + $packed = $this->archive->compress(Phar::GZ); - // Delete the non-compressed files - (new FileHandler($file))->delete(); - (new FileHandler($this->archive->getPath()))->delete(); + // Delete the non-compressed files + (new FileHandler($file))->delete(); + (new FileHandler($this->archive->getPath()))->delete(); - $this->restrictToOwner($this->archive->getPath() . '.gz'); + $this->restrictToOwner($this->archive->getPath() . '.gz'); + } finally { + umask($umask); + } return $packed->getFileInfo()->getPathname(); } diff --git a/tests/Unit/Infrastructure/File/ArchiveHandlerTest.php b/tests/Unit/Infrastructure/File/ArchiveHandlerTest.php new file mode 100644 index 000000000..6b161749f --- /dev/null +++ b/tests/Unit/Infrastructure/File/ArchiveHandlerTest.php @@ -0,0 +1,208 @@ +. + */ + +namespace SP\Tests\Unit\Infrastructure\File; + +use Phar; +use PharData; +use PHPUnit\Framework\Attributes\Group; +use PHPUnit\Framework\Attributes\Test; +use PHPUnit\Framework\TestCase; +use SP\Infrastructure\File\ArchiveHandler; +use SP\Infrastructure\PhpExtensionChecker; + +/** + * The backup archives hold the database dump — every account's encrypted secret and the + * master-password hash — and the application's own `config.xml`, with the database credentials and + * the crypto keys. They are meant to be readable only by their owner. + * + * They were, but only once written: the chmod ran after `buildFromDirectory()` had walked the whole + * application tree, so on an installation of any size the finished archive sat at the process umask + * — measured 0644, which on a shared host is every local user — for as long as building it took. + * A run that died in between left it that way for good. + * + * Uses real temporary files: `PharData` writes real archives. + */ +#[Group('unitary')] +class ArchiveHandlerTest extends TestCase +{ + /** + * The one production caller always passes a regex (`BackupFile::BACKUP_INCLUDE_REGEX`), and so + * does this — `compressDirectory()`'s `?string $regex = null` default reaches + * `PharData::buildFromDirectory()`, which requires a string, so the null is a TypeError. It is + * unreachable today and is not this change's to fix. + */ + private const EVERYTHING = '/.*/'; + + private string $dir; + private string $source; + + protected function setUp(): void + { + parent::setUp(); + + $this->dir = sys_get_temp_dir() . DIRECTORY_SEPARATOR . uniqid('sp_archive_', true); + $this->source = $this->dir . DIRECTORY_SEPARATOR . 'source'; + + mkdir($this->source, 0777, true); + file_put_contents($this->source . DIRECTORY_SEPARATOR . 'secret.txt', str_repeat('x', 4096)); + } + + protected function tearDown(): void + { + self::removeRecursively($this->dir); + + parent::tearDown(); + } + + /** + * The mechanism the fix rests on, pinned on its own. + * + * `PharData` gives no way to restrict an archive before its contents land: it does not create + * the file when it is constructed, so there is nothing to chmod beforehand, and `compress()` + * refuses outright when its target already exists ("phar ... exists and must be unlinked prior + * to conversion"). The umask is what is left. If a future PHP stopped honouring it here the + * archives would go back to being briefly world-readable with nothing else to notice, so this + * asserts the platform behaviour directly rather than assuming it. + */ + #[Test] + public function pharHonoursTheUmaskWhenItCreatesAnArchive(): void + { + $tar = $this->dir . DIRECTORY_SEPARATOR . 'probe.tar'; + + $umask = umask(0177); + + try { + $archive = new PharData($tar); + $archive->buildFromDirectory($this->source); + $archive->compress(Phar::GZ); + } finally { + umask($umask); + } + + clearstatcache(); + + self::assertSame(0600, fileperms($tar) & 0777, 'the tar must be created owner-only'); + self::assertSame(0600, fileperms($tar . '.gz') & 0777, 'and so must the gz'); + } + + /** + * The archive the handler produces is owner-only, with the ambient umask at its most + * permissive. + */ + #[Test] + public function theCompressedArchiveIsOwnerOnly(): void + { + $umask = umask(0); + + try { + $this->handler()->compressDirectory($this->source, self::EVERYTHING); + } finally { + umask($umask); + } + + clearstatcache(); + + // Not the method's return value: it answers `phar:///…/archive.tar.gz/`, a + // URL for a file *inside* the archive rather than the archive's own path. Every caller + // discards it, so that is a wart rather than a defect, but it is not what to measure. + self::assertSame(0600, fileperms($this->archivePath()) & 0777); + } + + /** + * And nothing is left beside it — the uncompressed tar, which holds exactly the same thing, + * is removed once the gz exists. + */ + #[Test] + public function theUncompressedArchiveDoesNotSurvive(): void + { + $this->handler()->compressDirectory($this->source, self::EVERYTHING); + + self::assertFileDoesNotExist($this->dir . DIRECTORY_SEPARATOR . 'archive.tar'); + self::assertFileExists($this->archivePath()); + } + + private function archivePath(): string + { + return $this->dir . DIRECTORY_SEPARATOR . 'archive.tar.gz'; + } + + /** + * And the process is left with the umask it had. + * + * The handler narrows it for the duration, so every file the rest of the request creates would + * be owner-only too if it were not put back — which for the cache and the compiled container + * would be a change nobody asked for, and one that only shows up later. + */ + #[Test] + public function theUmaskIsRestoredAfterwards(): void + { + $before = umask(); + + $this->handler()->compressDirectory($this->source, self::EVERYTHING); + + self::assertSame($before, umask()); + } + + /** + * Including when the work throws part-way, which is what the `finally` is for. + */ + #[Test] + public function theUmaskIsRestoredWhenTheArchiveCannotBeBuilt(): void + { + $before = umask(); + + try { + $this->handler()->compressDirectory($this->dir . DIRECTORY_SEPARATOR . 'no-such-directory', self::EVERYTHING); + } catch (\Throwable) { + // asserted below; what matters is the umask the caller is left with + } + + self::assertSame($before, umask()); + } + + private function handler(): ArchiveHandler + { + // The real checker: checkPhar() is a magic method behind an @method docblock, so a stub + // of the interface does not have it. The extension is present wherever this suite runs. + return new ArchiveHandler($this->dir . DIRECTORY_SEPARATOR . 'archive', new PhpExtensionChecker()); + } + + private static function removeRecursively(string $path): void + { + if (!is_dir($path)) { + @unlink($path); + + return; + } + + foreach (array_diff(scandir($path) ?: [], ['.', '..']) as $entry) { + self::removeRecursively($path . DIRECTORY_SEPARATOR . $entry); + } + + @rmdir($path); + } +}