LibraryOfCongress / bagit-python

Work with BagIt packages from Python.
http://libraryofcongress.github.io/bagit-python
216 stars 85 forks source link

replace tempfile.mkdtemp with tempfile.mktemp #132

Closed fkloft closed 1 year ago

fkloft commented 5 years ago

tempfile.mkdtemp has permission bits hardcoded into it, which works for /tmp-like filesystems where multiple users are expected to be active at the same time, but might break regular folders in case more complex permissions are involved (e.g. ACLs). tempfile.mktemp followed by os.mkdir uses the OS's default permission. mktemp is deprecated because there is a (negligible) chance that someone else could create the file in between the current process generating the name and creating the file. This should not be relevant to bagit, as concurrent write access to a single bag is not supported.

acdha commented 5 years ago

Do you have an example of this creating a problem? This would be a change in security exposure for anyone creating bags in a shared directory, which I'd like to avoid, and it seems like it'd be a substantial amount of non-portable code to actually copy ACLs, which os.mkdir will also have problems with.

fkloft commented 5 years ago

The thing about ACLs is that you usually shouldn't have to care about them - and unless you're writing software dedicated to handling permissions, you shouldn't even be using chmod. ACLs can be set to inherit to newly created files so applications don't have to do anything. In fact, there are different types of ACLs, some of which don't even have any API besides OS-provided CLI tools. That's why I propose to remove any IO calls related to permissions. Just using mkdir creates a directory using the most basic system call available, leaving all permission work to the OS. We are using an ACL to provide write access for a specific group, independent of the file's owner. Unfortunately, when using chmod on affected folders, the ACL is modified and doesn't work any more for newly created files inside that folder. I don't see how this should affect security - if the bag directory is readable to other users, so should the payload directory. If the bag folder isn't, everything inside won't be readable in the first place.