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

Cookie file permissions are narrowed when saved as a different user #12299

Closed
hwti opened this issue Nov 8, 2023 · 6 comments
Closed

Cookie file permissions are narrowed when saved as a different user #12299

hwti opened this issue Nov 8, 2023 · 6 comments
Labels

Comments

@hwti
Copy link
Contributor

hwti commented Nov 8, 2023

I did this

As user1 :

  • curl -c cookies.txt ...
  • chmod 0666 cookies.txt

As user2

  • curl -c cookies.txt ...
    => cookies.txt mode is 0600, so user1 can no longer access it.

I expected the following

cookies.txt stays at 0666, or at least 0666 & ~umask.

curl/libcurl version

curl 8.4.0-DEV (Linux) libcurl/8.4.0-DEV OpenSSL/3.0.9 zlib/1.2.13 libidn2/2.3.4 libpsl/0.21.2 (+libidn2/2.3.4)

operating system

Fedora 38

@hwti
Copy link
Contributor Author

hwti commented Nov 8, 2023

20f9dd6 or the CVE do not explain why :

  • the temporary file is created with 0600
  • the fchmod is only done when the uid and gid are the same

Maybe the temporary file could be created with 0600 | sb.st_mode.
Then the fchmod would restore the full mode if the umask prevented it (nsb.st_mode != (0600 | sb.st_mode) could be checked).
Even if the uid or gid are different, I think we could still do the fchmod.
Then a fchown could try restoring the uid / gid, but without CAP_CHOWN only the gid could be restored (if the current user is a member of this group) but this would cover cases where the file is shared with a group.

In 0600 | sb.st_mode, the 0600 would prevent creating a file we can't access, if the original rights lack permissions for the user (which would be strange, but it's possible).
For example if the original file is 0066 and belongs to user2, user1 can open it.
But the temporary file would belong to user2, so it would need to be 0666, else user2 wouldn't be able to access it later.

@bagder bagder added the cookies label Nov 9, 2023
@bagder
Copy link
Member

bagder commented Nov 9, 2023

I've pondered a bit back and forth on this and yes, I think 0600 | sb.st_mode is what we want. You up for writing a PR for this @hwti ?

@hwti
Copy link
Contributor Author

hwti commented Nov 13, 2023

What about the fchmod, should we do it without the uid/gid condition ?
If yes, then the fstat would actually be useless (no point to do a stat to avoid a chmod).

@bagder
Copy link
Member

bagder commented Nov 14, 2023

... but if we consider skipping fstat() to do chmod unconditionally, can't we then instead change the open() call to the version below, and cut out the entire #ifdef block?

  fd = open(tempstore, O_WRONLY | O_CREAT | O_EXCL, (mode_t)sb.st_mode);

bagder added a commit that referenced this issue Nov 23, 2023
Due to the trick where the functions rename the temp file to the target
name as a last step, if the file was previously own by a different user,
not ORing the old mode could otherwise end up creating a file that after
xsave was not readable by the original owner.

Reported-by: Loïc Yhuel
Fixes #12299
@bagder
Copy link
Member

bagder commented Nov 23, 2023

See #12395 for my take on a fix.

bagder added a commit that referenced this issue Nov 23, 2023
Because the function renames the temp file to the target name as a last
step, if the file was previously owned by a different user, not ORing
the old mode could otherwise end up creating a file that was no longer
readable by the original owner after save.

Reported-by: Loïc Yhuel
Fixes #12299
bagder added a commit that referenced this issue Nov 23, 2023
Because the function renames the temp file to the target name as a last
step, if the file was previously owned by a different user, not ORing
the old mode could otherwise end up creating a file that was no longer
readable by the original owner after save.

Reported-by: Loïc Yhuel
Fixes #12299
Closes #12395
@bagder bagder closed this as completed in 03cb1ff Nov 23, 2023
@hwti
Copy link
Contributor Author

hwti commented Nov 24, 2023

Sorry for not responding earlier, I was quite busy with other work.

... but if we consider skipping fstat() to do chmod unconditionally, can't we then instead change the open() call to the version below, and cut out the entire #ifdef block?

  fd = open(tempstore, O_WRONLY | O_CREAT | O_EXCL, (mode_t)sb.st_mode);

Unlike fchmod, open is affected by the umask (typically 022, so removing group / other write).
So if the permissions were modified outside of curl (or by calling curl with a modified umask, which is less likely), these additional modifications would be lost.

So to share a cookie file between users, there are several options :

  • run the program with a large umask (not great)
  • a manual chmod 0660 (or 0666), for example when pre-creating the file : it worked before 20f9dd6, but now it would require a fchmod to set the full 0600 | sb.st_mode
  • reverting 0c66718 : fopen(filename, FOPEN_WRITETEXT) needs write permission on the file, but rename() doesn't (so it could work with 0644 files, nothing special would be needed)

In fact, 0c66718 made the non-regular file check atomic, but at the same time truncated the file with FOPEN_WRITETEXT, which is not atomic.
I don't see an easy way to make both operations atomic, so we probably have to choose (unless maybe reopening /proc/self/fd/%d, but this would be Linux-specific, and I don't know if it would really work).

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

Successfully merging a pull request may close this issue.

2 participants