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); + } +}