bagit-python icon indicating copy to clipboard operation
bagit-python copied to clipboard

replace tempfile.mkdtemp with tempfile.mktemp

Open fkloft opened this issue 6 years ago • 2 comments

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.

fkloft avatar Mar 15 '19 07:03 fkloft

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.

acdha avatar Mar 15 '19 14:03 acdha

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.

fkloft avatar Mar 15 '19 16:03 fkloft