From 932efbd4593fe6be6c86f0dafeaea32910b7c246 Mon Sep 17 00:00:00 2001 From: Jakob Unterwurzacher Date: Sun, 14 Oct 2018 20:11:49 +0200 Subject: [PATCH] fusefrontend: make openBackingDir() symlink-safe openBackingDir() used encryptPath(), which is not symlink-safe itself. Drop encryptPath() and implement our own directory walk. Adds three seconds to untar and two seconds to rm: $ ./benchmark.bash Testing gocryptfs at /tmp/benchmark.bash.MzG: gocryptfs v1.6-36-g8fb3c2f-dirty; go-fuse v20170619-66-g6df8ddc; 2018-10-14 go1.11 WRITE: 262144000 bytes (262 MB, 250 MiB) copied, 1.25078 s, 210 MB/s READ: 262144000 bytes (262 MB, 250 MiB) copied, 1.0318 s, 254 MB/s UNTAR: 20.941 MD5: 11.568 LS: 1.638 RM: 5.337 --- internal/fusefrontend/names.go | 51 ++++++-- internal/fusefrontend/names_test.go | 152 +++++++++++++++++++++++ internal/fusefrontend/xattr_unit_test.go | 5 +- internal/nametransform/diriv.go | 9 +- 4 files changed, 204 insertions(+), 13 deletions(-) create mode 100644 internal/fusefrontend/names_test.go diff --git a/internal/fusefrontend/names.go b/internal/fusefrontend/names.go index 3bf64d5..5ec252b 100644 --- a/internal/fusefrontend/names.go +++ b/internal/fusefrontend/names.go @@ -4,8 +4,11 @@ package fusefrontend import ( "path/filepath" + "strings" + "syscall" "github.com/rfjakob/gocryptfs/internal/configfile" + "github.com/rfjakob/gocryptfs/internal/nametransform" "github.com/rfjakob/gocryptfs/internal/syscallcompat" "github.com/rfjakob/gocryptfs/internal/tlog" ) @@ -42,19 +45,53 @@ func (fs *FS) getBackingPath(relPath string) (string, error) { // 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. +// For convenience, if relPath is "", cName is going to be ".". +// +// openBackingDir is secure against symlink races by using Openat and +// ReadDirIVAt. func (fs *FS) openBackingDir(relPath string) (dirfd int, cName string, err error) { - cRelPath, err := fs.encryptPath(relPath) + // With PlaintextNames, we don't need to read DirIVs. Easy. + if fs.args.PlaintextNames { + dir := nametransform.Dir(relPath) + dirfd, err = syscallcompat.OpenDirNofollow(fs.args.Cipherdir, dir) + if err != nil { + return -1, "", err + } + // If relPath is empty, cName is ".". + cName = filepath.Base(relPath) + return dirfd, cName, nil + } + // Open cipherdir (following symlinks) + dirfd, err = syscall.Open(fs.args.Cipherdir, syscall.O_RDONLY|syscall.O_DIRECTORY|syscallcompat.O_PATH, 0) if err != nil { return -1, "", err } - // Open parent dir - dirfd, err = syscallcompat.OpenDirNofollow(fs.args.Cipherdir, filepath.Dir(cRelPath)) - if err != nil { - return -1, "", err + // If relPath is empty, cName is ".". + if relPath == "" { + return dirfd, ".", nil + } + // Walk the directory tree + parts := strings.Split(relPath, "/") + for i, name := range parts { + iv, err := nametransform.ReadDirIVAt(dirfd) + if err != nil { + return -1, "", err + } + cName = fs.nameTransform.EncryptAndHashName(name, iv) + // Last part? We are done. + if i == len(parts)-1 { + break + } + // Not the last part? Descend into next directory. + dirfd2, err := syscallcompat.Openat(dirfd, cName, syscall.O_RDONLY|syscall.O_NOFOLLOW|syscall.O_DIRECTORY|syscallcompat.O_PATH, 0) + syscall.Close(dirfd) + if err != nil { + return -1, "", err + } + dirfd = dirfd2 } - cName = filepath.Base(cRelPath) return dirfd, cName, nil } diff --git a/internal/fusefrontend/names_test.go b/internal/fusefrontend/names_test.go new file mode 100644 index 0000000..8453e52 --- /dev/null +++ b/internal/fusefrontend/names_test.go @@ -0,0 +1,152 @@ +package fusefrontend + +import ( + "strings" + "syscall" + "testing" + + "golang.org/x/sys/unix" + + "github.com/rfjakob/gocryptfs/internal/syscallcompat" + "github.com/rfjakob/gocryptfs/tests/test_helpers" +) + +func TestOpenBackingDir(t *testing.T) { + cipherdir := test_helpers.InitFS(t) + args := Args{ + Cipherdir: cipherdir, + } + fs := newTestFS(args) + + code := fs.Mkdir("dir1", 0700, nil) + if !code.Ok() { + t.Fatal(code) + } + code = fs.Mkdir("dir1/dir2", 0700, nil) + if !code.Ok() { + t.Fatal(code) + } + + dirfd, cName, err := fs.openBackingDir("") + if err != nil { + t.Fatal(err) + } + if cName != "." { + t.Fatal("cName should be .") + } + err = syscallcompat.Faccessat(dirfd, cName, unix.R_OK) + if err != nil { + t.Error(err) + } + err = syscallcompat.Faccessat(dirfd, ".", unix.R_OK) + if err != nil { + t.Error(err) + } + syscall.Close(dirfd) + + dirfd, cName, err = fs.openBackingDir("dir1") + if err != nil { + t.Fatal(err) + } + if cName == "" { + t.Fatal("cName should not be empty") + } + err = syscallcompat.Faccessat(dirfd, cName, unix.R_OK) + if err != nil { + t.Error(err) + } + syscall.Close(dirfd) + + dirfd, cName, err = fs.openBackingDir("dir1/dir2") + if err != nil { + t.Fatal(err) + } + if cName == "" { + t.Fatal("cName should not be empty") + } + err = syscallcompat.Faccessat(dirfd, cName, unix.R_OK) + if err != nil { + t.Error(err) + } + syscall.Close(dirfd) + + n255 := strings.Repeat("n", 255) + path := "dir1/" + n255 + fs.Mkdir(path, 0700, nil) + dirfd, cName, err = fs.openBackingDir(path) + if err != nil { + t.Fatal(err) + } + if cName == "" { + t.Fatal("cName should not be empty") + } + if len(cName) >= 255 { + t.Fatalf("cName is too long: %q", cName) + } + err = syscallcompat.Faccessat(dirfd, cName, unix.R_OK) + if err != nil { + t.Error(err) + } + syscall.Close(dirfd) +} + +func TestOpenBackingDirPlaintextNames(t *testing.T) { + cipherdir := test_helpers.InitFS(t, "-plaintextnames") + args := Args{ + Cipherdir: cipherdir, + PlaintextNames: true, + } + fs := newTestFS(args) + + code := fs.Mkdir("dir1", 0700, nil) + if !code.Ok() { + t.Fatal(code) + } + code = fs.Mkdir("dir1/dir2", 0700, nil) + if !code.Ok() { + t.Fatal(code) + } + + dirfd, cName, err := fs.openBackingDir("") + if err != nil { + t.Fatal(err) + } + if cName != "." { + t.Fatal("cName should be .") + } + err = syscallcompat.Faccessat(dirfd, cName, unix.R_OK) + if err != nil { + t.Error(err) + } + err = syscallcompat.Faccessat(dirfd, ".", unix.R_OK) + if err != nil { + t.Error(err) + } + syscall.Close(dirfd) + + dirfd, cName, err = fs.openBackingDir("dir1") + if err != nil { + t.Fatal(err) + } + if cName != "dir1" { + t.Fatalf("wrong cName: %q", cName) + } + err = syscallcompat.Faccessat(dirfd, cName, unix.R_OK) + if err != nil { + t.Error(err) + } + syscall.Close(dirfd) + + dirfd, cName, err = fs.openBackingDir("dir1/dir2") + if err != nil { + t.Fatal(err) + } + if cName != "dir2" { + t.Fatalf("wrong cName: %q", cName) + } + err = syscallcompat.Faccessat(dirfd, cName, unix.R_OK) + if err != nil { + t.Error(err) + } + syscall.Close(dirfd) +} diff --git a/internal/fusefrontend/xattr_unit_test.go b/internal/fusefrontend/xattr_unit_test.go index c5c9360..b43aeb7 100644 --- a/internal/fusefrontend/xattr_unit_test.go +++ b/internal/fusefrontend/xattr_unit_test.go @@ -11,18 +11,17 @@ import ( "github.com/rfjakob/gocryptfs/internal/nametransform" ) -func newTestFS() *FS { +func newTestFS(args Args) *FS { // Init crypto backend key := make([]byte, cryptocore.KeyLen) cCore := cryptocore.New(key, cryptocore.BackendGoGCM, contentenc.DefaultIVBits, true, false) cEnc := contentenc.New(cCore, contentenc.DefaultBS, false) nameTransform := nametransform.New(cCore.EMECipher, true, true) - args := Args{} return NewFS(args, cEnc, nameTransform) } func TestEncryptDecryptXattrName(t *testing.T) { - fs := newTestFS() + fs := newTestFS(Args{}) attr1 := "user.foo123456789" cAttr := fs.encryptXattrName(attr1) t.Logf("cAttr=%v", cAttr) diff --git a/internal/nametransform/diriv.go b/internal/nametransform/diriv.go index b45ae52..c2b9bb1 100644 --- a/internal/nametransform/diriv.go +++ b/internal/nametransform/diriv.go @@ -121,7 +121,7 @@ func WriteDirIV(dirfd int, dir string) error { // encryptAndHashName encrypts "name" and hashes it to a longname if it is // too long. -func (be *NameTransform) encryptAndHashName(name string, iv []byte) string { +func (be *NameTransform) EncryptAndHashName(name string, iv []byte) string { cName := be.EncryptName(name, iv) if be.longNames && len(cName) > unix.NAME_MAX { return be.HashLongName(cName) @@ -132,6 +132,9 @@ func (be *NameTransform) encryptAndHashName(name string, iv []byte) string { // EncryptPathDirIV - encrypt relative plaintext path "plainPath" using EME with // DirIV. "rootDir" is the backing storage root directory. // Components that are longer than 255 bytes are hashed if be.longnames == true. +// +// TODO: EncryptPathDirIV is NOT SAFE against symlink races. This function +// should eventually be deleted. func (be *NameTransform) EncryptPathDirIV(plainPath string, rootDir string) (string, error) { var err error // Empty string means root directory @@ -148,7 +151,7 @@ func (be *NameTransform) EncryptPathDirIV(plainPath string, rootDir string) (str // in the tar extract benchmark. parentDir := Dir(plainPath) if iv, cParentDir := be.DirIVCache.Lookup(parentDir); iv != nil { - cBaseName := be.encryptAndHashName(baseName, iv) + cBaseName := be.EncryptAndHashName(baseName, iv) return filepath.Join(cParentDir, cBaseName), nil } // We have to walk the directory tree, starting at the root directory. @@ -166,7 +169,7 @@ func (be *NameTransform) EncryptPathDirIV(plainPath string, rootDir string) (str } be.DirIVCache.Store(plainWD, iv, cipherWD) } - cipherName := be.encryptAndHashName(plainName, iv) + cipherName := be.EncryptAndHashName(plainName, iv) cipherWD = filepath.Join(cipherWD, cipherName) plainWD = filepath.Join(plainWD, plainName) }