diff --git a/src/Application/Config/Services/ConfigFile.php b/src/Application/Config/Services/ConfigFile.php index a6c5742e9..931133b80 100644 --- a/src/Application/Config/Services/ConfigFile.php +++ b/src/Application/Config/Services/ConfigFile.php @@ -116,7 +116,7 @@ private function loadFromFile(): ?ConfigDataInterface { try { $configData = $this->configMapper($this->fileStorage->load('config')); - $this->fileCache->save($configData); + $this->saveCache($configData); return $configData; } catch (ReflectionException|FileException $e) { @@ -188,7 +188,7 @@ public function save( if ($commit) { // Save only attributes to avoid a parent attributes node within the XML $this->fileStorage->save($configData->getAttributes(), 'config'); - $this->fileCache->save($configData); + $this->saveCache($configData); } $this->configData = $configData; @@ -251,4 +251,22 @@ public function generateUpgradeKey(): ConfigFileService return $this; } + + /** + * Write the config cache, restricted to the user the application runs as. + * + * It is a serialized ConfigData, so it carries the database password, the mail password, the + * LDAP bind password and the password salt — the same material as config.xml, which sits in a + * directory deliberately held at 0750. This file was left at whatever the umask gave it, + * measured at 0644. The directory is restricted too (ConfigUtil::checkCacheDir()); this is the + * artefact itself, so a file created before that guard existed is brought into line the next + * time it is written. + * + * @throws FileException + */ + private function saveCache(ConfigDataInterface $configData): void + { + $this->fileCache->save($configData); + $this->fileCache->chmod(0600); + } } diff --git a/src/Domain/Storage/Ports/FileCacheService.php b/src/Domain/Storage/Ports/FileCacheService.php index bf7566511..9a3301fba 100644 --- a/src/Domain/Storage/Ports/FileCacheService.php +++ b/src/Domain/Storage/Ports/FileCacheService.php @@ -56,6 +56,16 @@ public function save(mixed $data, ?string $path = null): FileCacheService; public function delete(): FileCacheService; + /** + * Restrict the cached file's permissions + * + * @param int $permissions + * + * @return FileCacheService + * @throws FileException + */ + public function chmod(int $permissions): FileCacheService; + /** * Returns whether the file is expired */ diff --git a/src/Infrastructure/File/FileCacheBase.php b/src/Infrastructure/File/FileCacheBase.php index 8486e9dd3..54c13f26e 100644 --- a/src/Infrastructure/File/FileCacheBase.php +++ b/src/Infrastructure/File/FileCacheBase.php @@ -104,6 +104,18 @@ public function exists(): bool return file_exists($this->path->getFile()); } + /** + * @inheritDoc + */ + public function chmod(int $permissions): FileCacheService + { + $this->checkOrInitializePath(); + + $this->path?->chmod($permissions); + + return $this; + } + /** * @throws FileException */ diff --git a/tests/Unit/Application/Config/Services/ConfigFileTest.php b/tests/Unit/Application/Config/Services/ConfigFileTest.php index 83730c4ed..36756dfaf 100644 --- a/tests/Unit/Application/Config/Services/ConfigFileTest.php +++ b/tests/Unit/Application/Config/Services/ConfigFileTest.php @@ -404,6 +404,16 @@ public function testSave() ->method('save') ->with(self::anything()); + // The cache is a serialized ConfigData, so it holds the database password, the mail + // password, the LDAP bind password and the password salt — the same material as + // config.xml, whose directory is deliberately held at 0750. This file was left at whatever + // the umask gave it, measured at 0644, so on a shared host every local account could read + // the installation's credentials out of it. + $this->fileCacheService + ->expects(self::exactly(2)) + ->method('chmod') + ->with(0600); + $configFile = new ConfigFile( $this->fileStorageService, $this->fileCacheService,