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/cache again instead of /var/tmp on UNIX #239

Merged
merged 1 commit into from
Nov 10, 2023

Conversation

andersk
Copy link
Contributor

@andersk andersk commented Nov 9, 2023

This directory was changed from /var/cache to /var/tmp in #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.

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>
@efiop
Copy link
Contributor

efiop commented Nov 10, 2023

site_cache_dir on both windows and mac are writable by regular users, while /var/cache is not, so this change will break applications that are not using elevated privileges. That seems unexpected and inconsistent in the practical sense.

Maybe this should be a new opt-in option?

@efiop
Copy link
Contributor

efiop commented Nov 10, 2023

This directory was changed from /var/cache to /var/tmp in #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.

Please correct me if I'm wrong, but seems like permissions of the base dir (/var/cache vs /var/tmp) don't really prevent users from creating those vulnerabilities, as they still need to handle file permissions correctly when creating files/subdirs. But using /var/cache makes it so that no regular user can create anything in /var/cache without additional privileges, thus forcing the application to be installed with sudo, which is inconsistent with site_cache_dir on other platforms.

@andersk
Copy link
Contributor Author

andersk commented Nov 10, 2023

@efiop On Windows, C:\ProgramData is only writable by administrators. I’m not familiar with the expectations regarding /Library/Caches on macOS. (I note there’s a /System/Library/Caches only writable by root.)

On Unix, a subdirectory of /var/cache will not be writable by other users unless you explicitly set it up that way. The goal here is not to make it impossible to explicitly write vulnerabilities; the goal is to avoid introducing them implicitly.

@efiop
Copy link
Contributor

efiop commented Nov 10, 2023

Good point about ProgramData.

Indeed, looks like I'm in the wrong here.

Not sure if platformdirs has a procedure for breaking changes like this. We'll need some help from maintainers here.

@gaborbernat
Copy link
Member

Because this is a fairly recent feature, I am okay too. Make this change now. I might make it a new major release just in case. But please feel free to go ahead.

@efiop
Copy link
Contributor

efiop commented Nov 10, 2023

A major release would indeed be great and would allow us to migrate gracefuly.

@gaborbernat gaborbernat merged commit 3935ea2 into tox-dev:main Nov 10, 2023
30 checks passed
hugovk referenced this pull request in hugovk/pypistats Dec 1, 2023
[![Mend Renovate logo
banner](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [platformdirs](https://github.com/platformdirs/platformdirs) |
`==3.11.0` -> `==4.0.0` |
[![age](https://developer.mend.io/api/mc/badges/age/pypi/platformdirs/4.0.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/pypi/platformdirs/4.0.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/pypi/platformdirs/3.11.0/4.0.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/pypi/platformdirs/3.11.0/4.0.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>platformdirs/platformdirs (platformdirs)</summary>

###
[`v4.0.0`](https://github.com/platformdirs/platformdirs/releases/tag/4.0.0)

[Compare
Source](https://github.com/platformdirs/platformdirs/compare/3.11.0...4.0.0)

<!-- Release notes generated using configuration in .github/release.yml
at main -->

#### What's Changed

- site_cache_dir: Use `/var/cache` again instead of `/var/tmp` on UNIX
by [@&#8203;andersk](https://github.com/andersk) in
[https://github.com/platformdirs/platformdirs/pull/239](https://github.com/platformdirs/platformdirs/pull/239)

#### New Contributors

- [@&#8203;andersk](https://github.com/andersk) made their first
contribution in
[https://github.com/platformdirs/platformdirs/pull/239](https://github.com/platformdirs/platformdirs/pull/239)

**Full Changelog**:
tox-dev/platformdirs@3.11.0...4.0.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "on the first day of the month" (UTC),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/hugovk/pypistats).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy41OS44IiwidXBkYXRlZEluVmVyIjoiMzcuNTkuOCIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->
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.

3 participants