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

Fix CLI Handling of Permissions and Ownership (Again) #3432

Merged
merged 7 commits into from Jan 21, 2023

Conversation

felixhandte
Copy link
Contributor

This PR switches the zstd CLI's handling of output files' ownership and permissions back (again). This PR brings zstd's behavior back into alignment with gzip's, without introducing any gaps where output files have incorrectly loose permissions.

This PR addresses #2946. Previous issues and PRs on this topic include #2739, #2525, #2491 and #2495, and #1630 and #1644.

When the input is not a regular file, we create the output file with default 0666 permissions (modulated by the umask), and then don't change anything at the end of the operation.

When the input is a regular file, we create it with 0600 (u+rw) permissions, and then copy the input's ownership, permissions, and timestamps to the output file at the end of the operation. E.g.:

$ ls -lAh ~/prog/silesia/silesia.tar; \
  ./zstd -q -5 ~/prog/silesia/silesia.tar -o silesia.tar.zst & \
  sleep 1; \
  ls -lAh silesia.tar.zst; \
  wait; \
  ls -lAh silesia.tar.zst
-rw-r----- 1 felixh users 203M Aug 18  2021 /home/felixh/prog/silesia/silesia.tar
[1] 4035425
-rw------- 1 felixh users 25M Jan 18 14:05 silesia.tar.zst
[1]+  Done                    ./zstd -q -5 ~/prog/silesia/silesia.tar -o silesia.tar.zst
-rw-r----- 1 felixh users 61M Aug 18  2021 silesia.tar.zst

We also leverage a trick found in gzip to avoid a different permissions/ownership race by setting the correct group, then permissions, then user. Otherwise there would be a window of time in which incorrectly lax permissions might be exposed to the wrong user or group. (It's not meaningful for there to be incorrectly lax user permissions exposed to the user that zstd is running as / the file is created as, since an attacker running as that user would already be able to chmod() the file to make it user r/w/x-able. But without this 3-step process, it would be possible for incorrectly lax permissions to be applied to the group the file is created as, or the user the file ends up being owned by.)

Finally, this PR refactors how we stat files slightly, to reduce the count of stat() calls we make. A further improvement might be to switch to doing fstat() calls.

Adding testing for this PR is a challenge. We have reasonably good coverage of checking that timestamps and mode bits are copied over. But it's difficult to extend that to testing that the user and group are copied over correctly. (1) We can't rely on particular other users/groups existing across all the platforms on which we want to test. (2) Zstd will mostly fail to chown() the output file anyways:

Only a privileged process (Linux: one with the CAP_CHOWN capability) may change the owner of a file. The owner of a file may change the group of the file to any group of which that owner is a member.
- chown(2) man page

I have locally verified that it does correctly propagate ownership when invoked with sudo.

@@ -185,7 +185,7 @@ int UTIL_isRegularFileStat(const stat_t* statbuf)
int UTIL_chmod(char const* filename, const stat_t* statbuf, mode_t permissions)
{
stat_t localStatBuf;
UTIL_TRACE_CALL("UTIL_chmod(%s, %u)", filename, (unsigned)permissions);
UTIL_TRACE_CALL("UTIL_chmod(%s, %#4o)", filename, (unsigned)permissions);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First time I see this formating instruction...

@felixhandte felixhandte merged commit 3d25502 into facebook:dev Jan 21, 2023
@Cyan4973 Cyan4973 mentioned this pull request Feb 9, 2023
dongsupark added a commit to flatcar/scripts that referenced this pull request Aug 11, 2023
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.
dongsupark added a commit to flatcar/scripts that referenced this pull request Aug 11, 2023
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

compressed file ownership inconsistent with that of uncompressed file
3 participants