Race-conditions related to PreserveOwner allow to set owner/group of arbitrary files when using plaintextnames

[slackner]

Steps to reproduce:

1. Create a forward mount point with plaintextnames enabled
2. Mount as root user with allow_other
3. For testing purposes create a file /tmp/file_owned_by_root which is owned by the root user
4. As a regular user run inside of the GoCryptFS mount:

mkdir tempdir
mknod tempdir/file_owned_by_root p &
mv tempdir tempdir2
ln -s /tmp tempdir

When the steps are done fast enough and in the right order (run in a loop!), the device file will be created in tempdir, but the lchown will be executed by following the symlink. As a result, the ownership of the file located at /tmp/file_owned_by_root will be changed.

[slackner]

Note that this issue could be fixed by always opening a dirfd, and then using fchownat.

[rfjakob]

Problem on MacOS:

+ GOOS=darwin
+ GOARCH=amd64
+ go build -tags without_openssl
# github.com/rfjakob/gocryptfs/internal/fusefrontend
internal/fusefrontend/fs.go:311:3: undefined: syscall.Fchownat

Needs a compat wrapper like https://github.com/rfjakob/gocryptfs/blob/master/internal/syscallcompat/sys_darwin.go#L92

[slackner]

Regarding the compat wrappers in general: wouldn't it be better to use open(".") and store a FD instead of the path (at least where it is possible)? This also reduces the risk of race conditions.

[rfjakob]

On macos, you mean?

[slackner]

On macos, you mean?

Yes. Instead of using Getwd() to store the previous working directory, you could just open the current directory with open(".") and later use Fchdir. This would also avoid race-conditions when directories are renamed in the meantime.

[rfjakob]

Yes, good idea. Looks like the whole file can be simplified by moving the locking and chdir-back-logic into dirfdAbs, at a small performance cost for Renameat (one additional chdir).

[slackner]

Not sure if that is a good idea. Besides rename, all other functions only expect one path argument. Instead of the conversion to an absolute path, you could just Chdir and then run the syscall with relative path. This is already done for Openat and probably faster than calling Getwd. The common logic could still be moved to a helper function of course.

[rfjakob]

Downside is that you hold chdirMutex across the syscall, but it does look safer.

[slackner]

For a first version it would also be fine to use the existing mechanism. As soon as this first patch is in, I will try to fix Symlink (this bug and also issue #176) - but apparently the syscall Symlinkat is not exported by golang. How should we deal with that? Would you also be fine to have syscall numbers hardcoded in the gocryptfs source?

[rfjakob]

Usually the syscall numbers are there, in this case it's syscall.SYS_SYMLINKAT, but a wrapper function must be written. Like for example https://github.com/hanwen/go-fuse/blob/66de2f17d31b22d3ea58fd1587619ab2eda87b79/fuse/pathfs/syscall_linux.go#L130 . Yes I'm good with having our own symlinkat in gocryptfs.

[rfjakob]

Mknod is fixed by 72b9758. Other entrypoints:

• Create is clean due to Fchown usage
• Symlink looks vulnerable
• Mkdir looks vulnerable