From 9ec9d0c49cfbdc9ceba10d7534b77e527c0a3cdc Mon Sep 17 00:00:00 2001 From: Jakob Unterwurzacher Date: Sat, 8 Sep 2018 17:41:17 +0200 Subject: [PATCH] syscallcompat: untangle OpenNofollow and rename to OpenDirNofollow The function used to do two things: 1) Walk the directory tree in a manner safe from symlink attacks 2) Open the final component in the mode requested by the caller This change drops (2), which was only used once, and lets the caller handle it. This simplifies the function and makes it fit for reuse in forward mode in openBackingPath(), and for using O_PATH on Linux. --- .../fusefrontend_reverse/reverse_longnames.go | 2 +- internal/fusefrontend_reverse/rfile.go | 9 +++++++- internal/fusefrontend_reverse/rfs.go | 2 +- internal/fusefrontend_reverse/rpath.go | 2 +- internal/fusefrontend_reverse/virtualfile.go | 2 +- internal/syscallcompat/open_nofollow.go | 23 ++++++++----------- internal/syscallcompat/open_nofollow_test.go | 10 +++++--- 7 files changed, 28 insertions(+), 22 deletions(-) diff --git a/internal/fusefrontend_reverse/reverse_longnames.go b/internal/fusefrontend_reverse/reverse_longnames.go index 46f7399..f826d1b 100644 --- a/internal/fusefrontend_reverse/reverse_longnames.go +++ b/internal/fusefrontend_reverse/reverse_longnames.go @@ -63,7 +63,7 @@ func (rfs *ReverseFS) findLongnameParent(dir string, dirIV []byte, longname stri if hit != "" { return hit, nil } - fd, err := syscallcompat.OpenNofollow(rfs.args.Cipherdir, dir, syscall.O_RDONLY|syscall.O_DIRECTORY, 0) + fd, err := syscallcompat.OpenDirNofollow(rfs.args.Cipherdir, dir) if err != nil { tlog.Warn.Printf("findLongnameParent: opendir failed: %v\n", err) return "", err diff --git a/internal/fusefrontend_reverse/rfile.go b/internal/fusefrontend_reverse/rfile.go index 7df0906..75932cb 100644 --- a/internal/fusefrontend_reverse/rfile.go +++ b/internal/fusefrontend_reverse/rfile.go @@ -4,6 +4,7 @@ import ( "bytes" "io" "os" + "path/filepath" "syscall" // In newer Go versions, this has moved to just "sync/syncmap". @@ -46,7 +47,13 @@ func (rfs *ReverseFS) newFile(relPath string) (*reverseFile, fuse.Status) { if err != nil { return nil, fuse.ToStatus(err) } - fd, err := syscallcompat.OpenNofollow(rfs.args.Cipherdir, pRelPath, syscall.O_RDONLY, 0) + dir := filepath.Dir(pRelPath) + dirfd, err := syscallcompat.OpenDirNofollow(rfs.args.Cipherdir, dir) + if err != nil { + return nil, fuse.ToStatus(err) + } + fd, err := syscallcompat.Openat(dirfd, filepath.Base(pRelPath), syscall.O_RDONLY|syscall.O_NOFOLLOW, 0) + syscall.Close(dirfd) if err != nil { return nil, fuse.ToStatus(err) } diff --git a/internal/fusefrontend_reverse/rfs.go b/internal/fusefrontend_reverse/rfs.go index 1ca0b28..4f94f3c 100644 --- a/internal/fusefrontend_reverse/rfs.go +++ b/internal/fusefrontend_reverse/rfs.go @@ -305,7 +305,7 @@ func (rfs *ReverseFS) OpenDir(cipherPath string, context *fuse.Context) ([]fuse. return nil, fuse.ToStatus(err) } // Read plaintext dir - fd, err := syscallcompat.OpenNofollow(rfs.args.Cipherdir, relPath, syscall.O_RDONLY|syscall.O_DIRECTORY, 0) + fd, err := syscallcompat.OpenDirNofollow(rfs.args.Cipherdir, relPath) if err != nil { return nil, fuse.ToStatus(err) } diff --git a/internal/fusefrontend_reverse/rpath.go b/internal/fusefrontend_reverse/rpath.go index b783686..7115426 100644 --- a/internal/fusefrontend_reverse/rpath.go +++ b/internal/fusefrontend_reverse/rpath.go @@ -108,7 +108,7 @@ func (rfs *ReverseFS) openBackingDir(cRelPath string) (dirfd int, pName string, } // Open directory, safe against symlink races pDir := filepath.Dir(pRelPath) - dirfd, err = syscallcompat.OpenNofollow(rfs.args.Cipherdir, pDir, syscall.O_RDONLY|syscall.O_DIRECTORY, 0) + dirfd, err = syscallcompat.OpenDirNofollow(rfs.args.Cipherdir, pDir) if err != nil { return -1, "", err } diff --git a/internal/fusefrontend_reverse/virtualfile.go b/internal/fusefrontend_reverse/virtualfile.go index 8509b87..963c801 100644 --- a/internal/fusefrontend_reverse/virtualfile.go +++ b/internal/fusefrontend_reverse/virtualfile.go @@ -91,7 +91,7 @@ func (f *virtualFile) Read(buf []byte, off int64) (resultData fuse.ReadResult, s // GetAttr - FUSE call func (f *virtualFile) GetAttr(a *fuse.Attr) fuse.Status { dir := filepath.Dir(f.parentFile) - dirfd, err := syscallcompat.OpenNofollow(f.cipherdir, dir, syscall.O_RDONLY|syscall.O_DIRECTORY, 0) + dirfd, err := syscallcompat.OpenDirNofollow(f.cipherdir, dir) if err != nil { return fuse.ToStatus(err) } diff --git a/internal/syscallcompat/open_nofollow.go b/internal/syscallcompat/open_nofollow.go index 4f75bd6..d440fc3 100644 --- a/internal/syscallcompat/open_nofollow.go +++ b/internal/syscallcompat/open_nofollow.go @@ -8,18 +8,18 @@ import ( "github.com/rfjakob/gocryptfs/internal/tlog" ) -// OpenNofollow opens the file/dir at "relPath" in a way that is secure against +// OpenDirNofollow opens the dir at "relPath" in a way that is secure against // symlink attacks. Symlinks that are part of "relPath" are never followed. // This function is implemented by walking the directory tree, starting at // "baseDir", using the Openat syscall with the O_NOFOLLOW flag. // Symlinks that are part of the "baseDir" path are followed. -func OpenNofollow(baseDir string, relPath string, flags int, mode uint32) (fd int, err error) { +func OpenDirNofollow(baseDir string, relPath string) (fd int, err error) { if !filepath.IsAbs(baseDir) { - tlog.Warn.Printf("BUG: OpenNofollow called with relative baseDir=%q", baseDir) + tlog.Warn.Printf("BUG: OpenDirNofollow called with relative baseDir=%q", baseDir) return -1, syscall.EINVAL } if filepath.IsAbs(relPath) { - tlog.Warn.Printf("BUG: OpenNofollow called with absolute relPath=%q", relPath) + tlog.Warn.Printf("BUG: OpenDirNofollow called with absolute relPath=%q", relPath) return -1, syscall.EINVAL } // Open the base dir (following symlinks) @@ -31,14 +31,11 @@ func OpenNofollow(baseDir string, relPath string, flags int, mode uint32) (fd in if relPath == "" { return dirfd, nil } - // Split the path into components and separate intermediate directories - // and the final basename + // Split the path into components parts := strings.Split(relPath, "/") - dirs := parts[:len(parts)-1] - final := parts[len(parts)-1] - // Walk intermediate directories + // Walk the directory tree var dirfd2 int - for _, name := range dirs { + for _, name := range parts { dirfd2, err = Openat(dirfd, name, syscall.O_RDONLY|syscall.O_NOFOLLOW|syscall.O_DIRECTORY, 0) syscall.Close(dirfd) if err != nil { @@ -46,8 +43,6 @@ func OpenNofollow(baseDir string, relPath string, flags int, mode uint32) (fd in } dirfd = dirfd2 } - defer syscall.Close(dirfd) - // Open the final component with the flags and permissions requested by - // the user plus forced NOFOLLOW. - return Openat(dirfd, final, flags|syscall.O_NOFOLLOW, mode) + // Return fd to final directory + return dirfd, nil } diff --git a/internal/syscallcompat/open_nofollow_test.go b/internal/syscallcompat/open_nofollow_test.go index 1f21557..1eeac3a 100644 --- a/internal/syscallcompat/open_nofollow_test.go +++ b/internal/syscallcompat/open_nofollow_test.go @@ -12,7 +12,11 @@ func TestOpenNofollow(t *testing.T) { t.Fatal(err) } // Create a file - fd, err := OpenNofollow(tmpDir, "d1/d2/d3/f1", syscall.O_RDWR|syscall.O_CREAT|syscall.O_EXCL, 0600) + dirfd, err := OpenDirNofollow(tmpDir, "d1/d2/d3") + if err != nil { + t.Fatal(err) + } + fd, err := Openat(dirfd, "f1", syscall.O_RDWR|syscall.O_CREAT|syscall.O_EXCL, 0600) if err != nil { t.Fatal(err) } @@ -27,7 +31,7 @@ func TestOpenNofollow(t *testing.T) { t.Fatal(err) } os.Symlink(tmpDir+"/d1.renamed", tmpDir+"/d1") - fd, err = OpenNofollow(tmpDir, "d1/d2/d3/f1", syscall.O_RDWR|syscall.O_CREAT, 0600) + fd, err = OpenDirNofollow(tmpDir, "d1/d2/d3") if err == nil { t.Fatalf("should have failed") } @@ -35,7 +39,7 @@ func TestOpenNofollow(t *testing.T) { t.Errorf("expected ELOOP or ENOTDIR, got %v", err) } // Check to see that the base dir can be opened as well - fd, err = OpenNofollow(tmpDir, "", syscall.O_RDONLY, 0) + fd, err = OpenDirNofollow(tmpDir, "") if err != nil { t.Errorf("cannot open base dir: %v", err) } else {