Race-conditions related to PreserveOwner allow to set owner/group of arbitrary files when using plaintextnames #177
Comments
|
Note that this issue could be fixed by always opening a dirfd, and then using |
If the user manages to replace the directory with a symlink at just the right time, we could be tricked into chown'ing the wrong file. This change fixes the race by using fchownat. Scenario, as described by @slackner at #177 : 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.
|
Problem on MacOS: Needs a compat wrapper like https://github.com/rfjakob/gocryptfs/blob/master/internal/syscallcompat/sys_darwin.go#L92 |
If the user manages to replace the directory with a symlink at just the right time, we could be tricked into chown'ing the wrong file. This change fixes the race by using fchownat. Scenario, as described by @slackner at #177 : 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.
|
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. |
|
On macos, you mean? |
Yes. Instead of using |
|
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). |
|
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 |
|
Downside is that you hold chdirMutex across the syscall, but it does look safer. |
|
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? |
If the user manages to replace the directory with a symlink at just the right time, we could be tricked into chown'ing the wrong file. This change fixes the race by using fchownat, which unfortunately is not available on darwin, hence a compat wrapper is added. Scenario, as described by @slackner at #177 : 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.
|
Usually the syscall numbers are there, in this case it's |
|
Mknod is fixed by 72b9758. Other entrypoints:
|
A user may be able to exploit this race to chown an unrelated file by swapping the parent directory with a symlink. This changes closes the race by using fchownat. Fixes #177
A user may be able to exploit this race to chown an unrelated file by swapping the parent directory with a symlink. This changes closes the race by using fchownat. #177
…d chown A user may be able to exploit this race to chown an unrelated file by swapping the parent directory with a symlink. This changes closes the race by using symlinkat + fchownat. #177
|
I believe this issue can be closed as fixed, or are you aware of any remaining issue regarding chown? |
|
I think that's it. When I wrote #177 (comment) I reviewed all functions look at PreserveOwner. |
Steps to reproduce:
plaintextnamesenabledallow_other/tmp/file_owned_by_rootwhich is owned by the root userWhen the steps are done fast enough and in the right order (run in a loop!), the device file will be created in
tempdir, but thelchownwill be executed by following the symlink. As a result, the ownership of the file located at/tmp/file_owned_by_rootwill be changed.The text was updated successfully, but these errors were encountered: