Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

site_cache_dir: use /var/tmp instead of /var/cache on unix #148

Merged
merged 1 commit into from
Mar 10, 2023

Conversation

efiop
Copy link
Contributor

@efiop efiop commented Mar 10, 2023

Turns out /var/cache might be non-writable by regular users (e.g. on ubuntu), so we are better off using /var/tmp which is and it is what was suggested in original appdirs discussion in ActiveState/appdirs#77

site_cache_dir was introduced very recently and it seems fine to make this breaking change right now.

Related #145

Turns out `/var/cache` might be non-writable by regular users (e.g. on ubuntu),
so we are better off using `/var/tmp` which is and it is what was suggested in
original appdirs discussion in ActiveState/appdirs#77
@gaborbernat gaborbernat merged commit c9202f7 into tox-dev:main Mar 10, 2023
@ThomasWaldmann
Copy link
Contributor

ThomasWaldmann commented Mar 16, 2023

Hmm, this change somehow seems counter-intuitive.

Also, check what the FHS says about this (5.5 and 5.15):

https://refspecs.linuxfoundation.org/FHS_3.0/fhs/ch05.html

Especially that stuff in /var/tmp is deleted "less frequently", but in a site-specific (not: app-specific) manner seems to make it unsuitable for application cache files. Cache files are not temporary files, they are just files that can be rebuilt (but that might be expensive, so you don't want some external "site cleanup cron job" delete that stuff).

Maybe the only thing that caused the initial issue (see top post) with /var/cache is that the package installation script (running as root) would need to create a subdirectory (like /var/cache/APPNAME) there with suitable permissions?

@efiop
Copy link
Contributor Author

efiop commented Mar 16, 2023

@ThomasWaldmann The application was not installed by root, but user-installed applications want to share expensive cache between users as well. In practical terms it seems systemd will delete files that weren't even read by anyone in /var/tmp once a month https://systemd.io/TEMPORARY_DIRECTORIES , which could be considered a free cache expiration policy 😄

I agree that /var/cache seems more suitable on paper, hence why I implemented it like that initially. But app-specific and site-specific don't seem mutually exclusive to me, as it is normal that the site has its own cache policy (e.g. when running low on space). I totally get that this is stretching /var/tmp a bit. Maybe what we need here is site_tmp_dir with persistent(or some other name) option to switch from /tmp to /var/tmp? Wdyt?

@andersk
Copy link
Contributor

andersk commented Nov 9, 2023

/var/tmp is an insecure location to store anything with a predictable filename, because any other user could have written it first. This leads to vulnerabilities categorized under CWE-377 and CAPEC-149.

This breaking change in platformdirs’ behavior is especially dangerous because it will have introduced these vulnerabilities into applications that were previously secure!

@ThomasWaldmann is correct that applications should put their own cache data in a subdirectory of /var/cache (e.g. /var/cache/cups), and the application’s package is responsible for ensuring the subdirectory exists and giving it the correct permissions.

Please consider reverting this.

@gaborbernat
Copy link
Member

is correct that applications should put their own cache data in a subdirectory of /var/cache (e.g. /var/cache/cups), and the application’s package is responsible for ensuring the subdirectory exists and giving it the correct permissions.

Open a PR with this change. We do not plan to revert.

andersk added a commit to andersk/platformdirs that referenced this pull request Nov 9, 2023
This directory was changed from /var/cache to /var/tmp in tox-dev#148 due to
permissions issues. However, /var/tmp is an insecure location to store
anything with a predictable filename, because any other user could
have written it first. This leads to vulnerabilities categorized under
CWE-377 and CAPEC-149.

To deal with the permissions issues, applications should put their own
cache data in a subdirectory of /var/cache (e.g. /var/cache/cups), and
the application’s package is responsible for ensuring the subdirectory
exists and giving it the correct permissions.

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
@andersk
Copy link
Contributor

andersk commented Nov 9, 2023

Opened #239.

@efiop
Copy link
Contributor Author

efiop commented Nov 10, 2023

This breaking change in platformdirs’ behavior is especially dangerous because it will have introduced these vulnerabilities into applications that were previously secure!

Just to be clear, site_cache_dir was introduced about a week before this PR, realistically I doubt anyone was using it during that period.

Opened #239.

Thanks, let's continue there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants