[programs] set chmod 600 after opening destination file #1644
Conversation
This resolves a race condition where zstd or unzstd may expose read permissions beyond the original file allowed. Mode 600 is used temporarily during the compression and decompression write stage and the new file inherits the original file’s mode at the end. Fixes #1630
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
Thanks @chungy .
Also, it would have been great if it was possible to ship a test case alongside the patch, as to ensure the property remains verified after future changes. However, I must admit I'm completely out of idea on how to test this specific property (using a reasonably simple test). cc @felixhandte for the review. |
programs/fileio.c
Outdated
| @@ -566,6 +566,7 @@ static FILE* FIO_openDstFile(FIO_prefs_t* const prefs, const char* srcFileName, | |||
| { FILE* const f = fopen( dstFileName, "wb" ); | |||
| if (f == NULL) | |||
| DISPLAYLEVEL(1, "zstd: %s: %s\n", dstFileName, strerror(errno)); | |||
| chmod(dstFileName, 00600); | |||
There was a problem hiding this comment.
I understand that chmod() can fail silently,
yet it would feel cleaner if chmod() wasn't attempted when f == NULL.
An if { } else { } should do the trick.
There was a problem hiding this comment.
Totally right, I did miss that. :)
Repairs an oversight in my last commit, thanks @Cyan4973
|
I wonder about the race condition that is potentially introduced here, where An alternative approach would be to temporarily set the process umask to What do you think? |
|
I believe this patch solves the more pressing issue of ensuring that the temporary file cannot be read during compression, which is a long time compared to the very tiny window of opportunity between file creation and rights setting. |
|
This PR causes an unexpected change in 1.4.2 and 1.4.1 (compared to 1.4.0 and before): if compressing from stdin to a file, the file always has final rights 0600 instead of umask. This breaks a lot of code I fear. |
|
Possible workaround: use shell redirection ( Though yes, it is a new bug and you should file a regular issue on it so it doesn't become forgotten in this merged PR :) |
Since #950 was merged, tarball files `flatcar-{packages,sdk}-*.tar.zst` have been created with mode 0600 instead of 0644. As a result, the files with mode 0600 were uploaded to bincache, but afterwards `copy-to-origin.sh` that in turn runs rsync from bincache to the origin server could not read the tarballs. To fix that, it is necessary to chmod from 0600 to 0644 to make it readable by rsync during the release process. All of that happens because zstd sets the mode of the output file to 0600 in case of temporary files to avoid race condition. See also facebook/zstd#1644.
Since #950 was merged, tarball files `flatcar-{packages,sdk}-*.tar.zst` have been created with mode 0600 instead of 0644. As a result, the files with mode 0600 were uploaded to bincache, but afterwards `copy-to-origin.sh` that in turn runs rsync from bincache to the origin server could not read the tarballs. To fix that, it is necessary to chmod from 0600 to 0644 to make it readable by rsync during the release process. All of that happens because zstd sets the mode of the output file to 0600 in case of temporary files to avoid race condition. See also facebook/zstd#1644.
Since #950 was merged, tarball files `flatcar-{packages,sdk}-*.tar.zst` have been created with mode 0600 instead of 0644. As a result, the files with mode 0600 were uploaded to bincache, but afterwards `copy-to-origin.sh` that in turn runs rsync from bincache to the origin server could not read the tarballs. To fix that, it is necessary to chmod from 0600 to 0644 to make it readable by rsync during the release process. All of that happens because zstd sets the mode of the output file to 0600 in case of temporary files to avoid race condition. See also facebook/zstd#1644, facebook/zstd#3432.
Since #950 was merged, tarball files `flatcar-{packages,sdk}-*.tar.zst` have been created with mode 0600 instead of 0644. As a result, the files with mode 0600 were uploaded to bincache, but afterwards `copy-to-origin.sh` that in turn runs rsync from bincache to the origin server could not read the tarballs. To fix that, it is necessary to chmod from 0600 to 0644 to make it readable by rsync during the release process. All of that happens because zstd sets the mode of the output file to 0600 in case of temporary files to avoid race condition. See also facebook/zstd#1644, facebook/zstd#3432.
This resolves a race condition where zstd or unzstd may expose read
permissions beyond the original file allowed. Mode 600 is used
temporarily during the compression and decompression write stage
and the new file inherits the original file’s mode at the end.
Fixes #1630
chmod() should silently fail in situations where it isn't possible to set mode 600, such as on FAT file systems. I'm also not really able to tell if this builds okay on Windows. mingw-w64 seems to build it fine (as long as I changed timefn.h to have
<windows.h>instead of<Windows.h>), but i don't know if chmod() has any effect on that OS -- it's well possible that someone should make a Windows-specific equivalent to this.