fix: the config cache is restricted to its owner - #883
Merged
Conversation
`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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
var/cache/config.cacheis 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 directoryConfigUtil::checkConfigDir()deliberately pins to 0750 forexactly 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/.blankis a tracked file and the directory is owned by the web user, so host-side git
immediately failed with
lstat("var/cache/.blank"): Permission deniedand refused tooperate. 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.