Possible race-conditions between file creation and Fchownat

[slackner]

The method would work as follows:

• Assume a system contains a gocryptfs mount as root user with -allow_other
• As a regular user create a new file with mode containing the SUID flag and write access for other users
• Before gocryptfs executes the Fchownat call, try to open the file again, write some exploit code to it, and try to run it.

For a short time, the file is owned by root and has the SUID flag, so this is pretty dangerous. To mitigate this issue, we could temporarily change the effective UID/GID and skip the Fchown step.

Some more thoughts:

• In Open, we might want to explicitly filter O_CREAT from the flags - if it is ever used for file creation, it skips the PreserveOwner and LongName creation code.

• The same issue might also be relevant for Mknod. Should a regular user really be able to create block devices with arbitrary modes? This probably needs further investigations.

[rfjakob]

Good find! The problem with seteuid is that affects all threads. We could create the file with mode 000, chown it, and then chmod to the requested mode.

Filtering O_creat is a good idea.

About mknod, the kernel checks if the user can do that before sending us the request. User should get permission denied from the kernel

[rfjakob]

Actually, i'm not sure if we can get o_create in open, but blindly filtering it out does no harm.

[slackner]

I have been thinking about this issue a bit more. Apparently, it is possible to change the effective permissions for a single thread on Linux, this is just a limitation of the POSIX api.

See http://man7.org/linux/man-pages/man2/setresuid.2.html:

  At the kernel level, user IDs and group IDs are a per-thread
  attribute.  However, POSIX requires that all threads in a process
  share the same credentials.  The NPTL threading implementation
  handles the POSIX requirements by providing wrapper functions for the
  various system calls that change process UIDs and GIDs.  These
  wrapper functions (including those for setresuid() and setresgid())
  employ a signal-based technique to ensure that when one thread
  changes credentials, all of the other threads in the process also
  change their credentials.  For details, see nptl(7).

Unfortunately, with Go things are a bit more complicated. Since Go routines do not necessarily correspond 1:1 to kernel threads, it seems dangerous to change user/group in the program. It would be necessary to lock for basically all file system operations, so this is not really feasible. I was wondering what os.Setuid and os.Setgid does - if the Go runtime would have support for saving/restoring effective user/group ids on thread change the problem would be easy to solve - and apparently they are bascially just broken: See golang/go#1435.

Getting the Fchown method right, however, is not trivial. There are many corner cases that are difficult to handle correctly. Something like this would be a minimal fix: http://ix.io/1vsl (completely untested). To list a few difficulties:

• The problem is not only relevant for SUID/SGID files. When files are temporarily owned by the wrong user/group with a permissive mode, other processes could open a fd. They would then have a way to bypass any new protections we set. We have to check if this is also relevant for other file objects (directories maybe?).

• If Fchown fails, we don't want to give SUID/SGID permissions to the file, and ideally should roll back to a clean previous state. Unfortunately, Linux does not provide any method to delete a file by fd. Using Unlinkat introduces the risk of race-conditions. I decided to skip file deletion in the draft patch above.

• There is still a small race-condition that programs see the file with the wrong owner, or with missing SUID/SGID mode. This is similar to bug Avoid possible race-conditions related to openWriteOnlyFile #329, which is about other processes being able to see to read permissions for a very short time. At some point, we should really use locking so that noone is able to see those inconstent intermediate states.

[rfjakob]

Quick question about the patch, why 0600 permissions instead of 0000?

[slackner]

For regular files, it probably doesn't really matter if we use 0600 or 0000. Initially, I tried to only filter SUID/SGID mode and pass through all other mode flags (basically mode & 0777), but as mentioned above, other processes could then theoretically open the file before they are chowned to the correct user. In the latest patch, I used 0600 to make sure the files can also easily be deleted by the user in case they are leaked.

[slackner]

To clarify: For read-only files, commands like rm would ask the user for confirmation. Of course, the user still has to run this as root, and it only matters in the error case. Otherwise it doesn't make a difference.

[slackner]

What about using runtime.LockOSThread, then seteuid/setegid syscalls (which only change the current thread), and then runtime.UnlockOSThread? Would something like this also work on macOS? And what about the performance overhead?

It would have the advantage that we could use it for all file related syscalls where the owner/group is important. Note that there is also a (probably difficult to exploit, but still) security issue related to Open. Since gocryptfs -allow_other is running as root, only the kernel checks permissions. However, the information from the last GetAttr call could be outdated... If a user somehow manages to get his own uid:gid into the kernel cache, and then modifies the file system in such a way that the path points to a root-owned file, this also could be used as an exploit. (Now with proper symlink handling this is much more difficult though.)

[slackner]

On macOS there is apparently settid, but we would have to write our own syscall wrappers. Some resources that might help (if we decide to go into this direction):

• https://wiki.freebsd.org/Per-Thread%20Credentials
• http://fxr.watson.org/fxr/source/bsd/kern/kern_prot.c?v=xnu-1228#L1354

[rfjakob]

The lockosthread thing sounds too brittle for my taste. If at all possible i'd like to avoid it.

[slackner]

It certainly would be less portable. Then (at least for now) let's stick to a Fchmod solution similar to the one in my draft patch. If you have some time to work on it this evening, feel free to adjust my patch.

[rfjakob]

Commited as b22cc03 (slightly adjusted), thanks!

[rfjakob]

Sorry, I did not do it this time, but do you mind if I add

Signed-off-by: Sebastian Lackner [email protected]

in cases like this in the future? When you propose a patch but don't make a PR.

Faking the git author would be another option, but that feels kinda wrong

[slackner]

Thanks! Now we carefully have to think if we need similar logic for other file objects aswell: What about directories, symlinks, or device files? Could it be exploited in any way if they are created with the wrong owner? The only risk I see would be that a user opens the corresponding file objects before we have set the correct owner. Is that actually possible (and could it be exploited)?

Regarding adding Signed-off-by: I don't mind if you want to use that to mark patches based on work by other authors. I only would like to point out, that Signed-off-by is also often used in a different context (e.g., agreeing to developer certificates in the kernel field). Maybe some other tags like Co-Developed-by: ... would fit better? In the end the decision is up to you though ;) See https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html.

[rfjakob]

Works for me as well, just want to give credit!

[rfjakob]

Fixed by 03b9d65 , Fchownat is gone