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

Race condition allows attacker to access world-readable destination file #2491

Closed
sdelafond opened this issue Feb 11, 2021 · 2 comments
Closed
Assignees
Labels

Comments

@sdelafond
Copy link

The patches for #1630 still create the file with the default umask, before chmod'ing down to 0600, so an attacker could still open it in the meantime. inotify can for instance help automating such an attack.

zstd should either set the mode directly through open(2), or use umask(2) before creating the file.

This is Debian bug https://bugs.debian.org/982519.

@carnil
Copy link

carnil commented Feb 11, 2021

It was suggested in https://bugs.debian.org/981404#28 to do something like:

fd = open(dstFileName, O_WRONLY|O_CREAT|O_EXCL, 0600);
if (fd != -1)
    f = fdopen( fd, "wb" );
if (fd == -1 || f == NULL)
    DISPLAYLEVEL(1, "zstd: %s: %s\n", dstFileName, strerror(errno));
return f;

@felixhandte felixhandte self-assigned this Feb 11, 2021
felixhandte added a commit to felixhandte/zstd that referenced this issue Feb 11, 2021
This commit addresses facebook#2491.

Note that a downside of this solution is that it is global: `umask()` affects
all file creation calls in the process. I believe this is safe since
`fileio.c` functions should only ever be used in the zstd binary, and these
are (almost) the only files ever created by zstd, and AIUI they're only
created in a single thread. So we can get away with messing with global state.

Note that this doesn't change the permissions of files created by `dibio.c`.
I'm not sure what those should be...
felixhandte added a commit to felixhandte/zstd that referenced this issue Feb 17, 2021
This commit addresses facebook#2491.

Note that a downside of this solution is that it is global: `umask()` affects
all file creation calls in the process. I believe this is safe since
`fileio.c` functions should only ever be used in the zstd binary, and these
are (almost) the only files ever created by zstd, and AIUI they're only
created in a single thread. So we can get away with messing with global state.

Note that this doesn't change the permissions of files created by `dibio.c`.
I'm not sure what those should be...
@felixhandte
Copy link
Contributor

Thanks for the report. The fix has been merged to dev and will go out in the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants