Cookie file permissions are narrowed when saved as a different user

[hwti]

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]

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]

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]

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]

... 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]

See #12395 for my take on a fix.

[hwti]

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).