From e8d8ae54d3b852d1d35b6db81d4a0beb57fbdce1 Mon Sep 17 00:00:00 2001 From: Jakob Unterwurzacher Date: Sat, 8 Sep 2018 19:27:33 +0200 Subject: [PATCH] fusefrontend: use OpenDirNofollow in openBackingDir Rename openBackingPath to openBackingDir and use OpenDirNofollow to be safe against symlink races. Note that openBackingDir is not used in several important code paths like Create(). But it is used in Unlink, and the performance impact in the RM benchmark to be acceptable: Before $ ./benchmark.bash Testing gocryptfs at /tmp/benchmark.bash.bYO: gocryptfs v1.6-12-g930c37e-dirty; go-fuse v20170619-49-gb11e293; 2018-09-08 go1.10.3 WRITE: 262144000 bytes (262 MB, 250 MiB) copied, 1.07979 s, 243 MB/s READ: 262144000 bytes (262 MB, 250 MiB) copied, 0.882413 s, 297 MB/s UNTAR: 16.703 MD5: 7.606 LS: 1.349 RM: 3.237 After $ ./benchmark.bash Testing gocryptfs at /tmp/benchmark.bash.jK3: gocryptfs v1.6-13-g84d6faf-dirty; go-fuse v20170619-49-gb11e293; 2018-09-08 go1.10.3 WRITE: 262144000 bytes (262 MB, 250 MiB) copied, 1.06261 s, 247 MB/s READ: 262144000 bytes (262 MB, 250 MiB) copied, 0.947228 s, 277 MB/s UNTAR: 17.197 MD5: 7.540 LS: 1.364 RM: 3.410 --- internal/fusefrontend/fs.go | 14 +++++++------- internal/fusefrontend/fs_dir.go | 2 +- internal/fusefrontend/names.go | 16 ++++++++++------ tests/reverse/correctness_test.go | 2 +- 4 files changed, 19 insertions(+), 15 deletions(-) diff --git a/internal/fusefrontend/fs.go b/internal/fusefrontend/fs.go index 45b5b40..d68939b 100644 --- a/internal/fusefrontend/fs.go +++ b/internal/fusefrontend/fs.go @@ -250,7 +250,7 @@ func (fs *FS) Chmod(path string, mode uint32, context *fuse.Context) (code fuse. if fs.isFiltered(path) { return fuse.EPERM } - dirfd, cName, err := fs.openBackingPath(path) + dirfd, cName, err := fs.openBackingDir(path) if err != nil { return fuse.ToStatus(err) } @@ -266,7 +266,7 @@ func (fs *FS) Chown(path string, uid uint32, gid uint32, context *fuse.Context) if fs.isFiltered(path) { return fuse.EPERM } - dirfd, cName, err := fs.openBackingPath(path) + dirfd, cName, err := fs.openBackingDir(path) if err != nil { return fuse.ToStatus(err) } @@ -291,7 +291,7 @@ func (fs *FS) Mknod(path string, mode uint32, dev uint32, context *fuse.Context) if fs.isFiltered(path) { return fuse.EPERM } - dirfd, cName, err := fs.openBackingPath(path) + dirfd, cName, err := fs.openBackingDir(path) if err != nil { return fuse.ToStatus(err) } @@ -409,7 +409,7 @@ func (fs *FS) Unlink(path string, context *fuse.Context) (code fuse.Status) { if fs.isFiltered(path) { return fuse.EPERM } - dirfd, cName, err := fs.openBackingPath(path) + dirfd, cName, err := fs.openBackingDir(path) if err != nil { return fuse.ToStatus(err) } @@ -447,7 +447,7 @@ func (fs *FS) Symlink(target string, linkName string, context *fuse.Context) (co if fs.isFiltered(linkName) { return fuse.EPERM } - dirfd, cName, err := fs.openBackingPath(linkName) + dirfd, cName, err := fs.openBackingDir(linkName) if err != nil { return fuse.ToStatus(err) } @@ -579,12 +579,12 @@ func (fs *FS) Link(oldPath string, newPath string, context *fuse.Context) (code if fs.isFiltered(newPath) { return fuse.EPERM } - oldDirFd, cOldName, err := fs.openBackingPath(oldPath) + oldDirFd, cOldName, err := fs.openBackingDir(oldPath) if err != nil { return fuse.ToStatus(err) } defer oldDirFd.Close() - newDirFd, cNewName, err := fs.openBackingPath(newPath) + newDirFd, cNewName, err := fs.openBackingDir(newPath) if err != nil { return fuse.ToStatus(err) } diff --git a/internal/fusefrontend/fs_dir.go b/internal/fusefrontend/fs_dir.go index b7bc790..b9266dd 100644 --- a/internal/fusefrontend/fs_dir.go +++ b/internal/fusefrontend/fs_dir.go @@ -54,7 +54,7 @@ func (fs *FS) Mkdir(newPath string, mode uint32, context *fuse.Context) (code fu if fs.isFiltered(newPath) { return fuse.EPERM } - dirfd, cName, err := fs.openBackingPath(newPath) + dirfd, cName, err := fs.openBackingDir(newPath) if err != nil { return fuse.ToStatus(err) } diff --git a/internal/fusefrontend/names.go b/internal/fusefrontend/names.go index 9be2623..d7fbdce 100644 --- a/internal/fusefrontend/names.go +++ b/internal/fusefrontend/names.go @@ -7,6 +7,7 @@ import ( "path/filepath" "github.com/rfjakob/gocryptfs/internal/configfile" + "github.com/rfjakob/gocryptfs/internal/syscallcompat" "github.com/rfjakob/gocryptfs/internal/tlog" ) @@ -40,18 +41,21 @@ func (fs *FS) getBackingPath(relPath string) (string, error) { return cAbsPath, nil } -// openBackingPath - get the absolute encrypted path of the backing file -// and open the corresponding directory -func (fs *FS) openBackingPath(relPath string) (*os.File, string, error) { - cPath, err := fs.getBackingPath(relPath) +// openBackingDir opens the parent ciphertext directory of plaintext path +// "relPath" and returns the dirfd and the encrypted basename. +// The caller should then use Openat(dirfd, cName, ...) and friends. +// openBackingDir is secure against symlink races. +func (fs *FS) openBackingDir(relPath string) (*os.File, string, error) { + cRelPath, err := fs.encryptPath(relPath) if err != nil { return nil, "", err } - dirfd, err := os.Open(filepath.Dir(cPath)) + // Open parent dir + dirfd, err := syscallcompat.OpenDirNofollow(fs.args.Cipherdir, filepath.Dir(cRelPath)) if err != nil { return nil, "", err } - return dirfd, filepath.Base(cPath), nil + return os.NewFile(uintptr(dirfd), cRelPath), filepath.Base(cRelPath), nil } // encryptPath - encrypt relative plaintext path diff --git a/tests/reverse/correctness_test.go b/tests/reverse/correctness_test.go index bb93316..62e3bbc 100644 --- a/tests/reverse/correctness_test.go +++ b/tests/reverse/correctness_test.go @@ -210,7 +210,7 @@ func TestTooLongSymlink(t *testing.T) { // Test that we can traverse a directory with 0100 permissions // (execute but no read). This used to be a problem as OpenDirNofollow opened -// all directory in the path with O_RDONLY. Now it uses O_PATH, which only needs +// all directories in the path with O_RDONLY. Now it uses O_PATH, which only needs // the executable bit. func Test0100Dir(t *testing.T) { // Note: t.Name() is not available before in Go 1.8