Skip to content

restrict cache storage directory permissions to owner#2048

Open
sahvx655-wq wants to merge 1 commit into
fsspec:masterfrom
sahvx655-wq:cache-dir-permissions
Open

restrict cache storage directory permissions to owner#2048
sahvx655-wq wants to merge 1 commit into
fsspec:masterfrom
sahvx655-wq:cache-dir-permissions

Conversation

@sahvx655-wq

Copy link
Copy Markdown

While looking at how filecache/blockcache/simplecache persist downloaded data, I noticed an inconsistency in the permissions they leave behind. The cache metadata file ends up 0o600 because it goes through atomic_write, which creates its temporary via mkstemp, but the cache directory and the cached data files inside it do not. CachingFileSystem.__init__, _mkcache and SimpleCacheFileSystem.__init__ each create the storage directory with a bare os.makedirs(..., exist_ok=True), so under a normal 0o022 umask the directory comes out 0o755 and the cached files 0o644. When cache_storage points at a persistent location (the documented way to keep a cache between runs), any other local user can traverse the directory and read another user's cached content, which for the http/s3/gcs backends is data that was fetched with that user's credentials.

The change sets mode=0o700 on the three os.makedirs calls that create the cache storage directory, so the directory fsspec owns becomes owner-only and the data files within inherit that protection whichever backend wrote them. Keeping it at the directory-creation point rather than chmod-ing each cached file individually puts the guard in one place and covers all three cache implementations and every write path through them; directories fsspec did not create are left as they are. I have added a parametrised regression test over filecache/simplecache/blockcache that fails on the current tree (the directory is 0o755) and passes once the mode is applied.

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.

1 participant