From 1f2860a851af2a22929d2c6da28e5429e1e36467 Mon Sep 17 00:00:00 2001 From: blaipr Date: Wed, 26 Aug 2026 21:30:55 +0200 Subject: [PATCH] fix: the config cache is restricted to its owner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `var/cache/config.cache` is a serialized ConfigData. That carries the database password, the mail password, the LDAP bind password and the password salt — the same material as `config.xml`, whose directory `ConfigUtil::checkConfigDir()` deliberately pins to 0750 for exactly that reason. The cache was left at whatever the umask gave it, measured at 0644 in the running container, so on a shared host every local account could read the installation's credentials out of it. It is written 0600 now, which is what the backup archives already do and for the same stated reason. I also had the cache *directory* held at 0750, matching the config directory, and backed that out. It works — verified going 755 to 750 on a live request — but `var/cache/.blank` is a tracked file and the directory is owned by the web user, so host-side git immediately failed with `lstat("var/cache/.blank"): Permission denied` and refused to operate. Fine in production, not fine for anyone working in the repository, and plausibly not fine in CI. That leaves the compiled DI containers, which php-di writes 0666 in its own library code (`@chmod($tmpFile, 0666)` in Compiler.php) and which are PHP executed on every request. Restricting the directory was what would have covered those, since we do not control how php-di writes them. It wants a deployment answer — ownership and umask on `var/cache` — rather than an application chmod, and is recorded here rather than half-fixed. Checked by dropping the chmod: the save test fails. --- .../Config/Services/ConfigFile.php | 22 +++++++++++++++++-- src/Domain/Storage/Ports/FileCacheService.php | 10 +++++++++ src/Infrastructure/File/FileCacheBase.php | 12 ++++++++++ .../Config/Services/ConfigFileTest.php | 10 +++++++++ 4 files changed, 52 insertions(+), 2 deletions(-) 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,