nametransform: check name validity on encryption

xfstests generic/523 discovered that we allowed to set
xattrs with "/" in the name, but did not allow to read
them later.

With this change we do not allow to set them in the first
place.

12 files changed, 108 insertions, 31 deletions

diff --git a/internal/fusefrontend/node_xattr.go b/internal/fusefrontend/node_xattr.go
index 3855b55..925dcbf 100644
--- a/internal/fusefrontend/node_xattr.go
+++ b/internal/fusefrontend/node_xattr.go
@@ -56,12 +56,14 @@ func (n *Node) Getxattr(ctx context.Context, attr string, dest []byte) (uint32,
 		}
 	} else {
 		// encrypted user xattr
-		cAttr := rn.encryptXattrName(attr)
+		cAttr, err := rn.encryptXattrName(attr)
+		if err != nil {
+			return minus1, syscall.EIO
+		}
 		cData, errno := n.getXAttr(cAttr)
 		if errno != 0 {
 			return 0, errno
 		}
-		var err error
 		data, err = rn.decryptXattrValue(cData)
 		if err != nil {
 			tlog.Warn.Printf("GetXAttr: %v", err)
@@ -91,7 +93,10 @@ func (n *Node) Setxattr(ctx context.Context, attr string, data []byte, flags uin
 		return n.setXAttr(attr, data, flags)
 	}
 
-	cAttr := rn.encryptXattrName(attr)
+	cAttr, err := rn.encryptXattrName(attr)
+	if err != nil {
+		return syscall.EINVAL
+	}
 	cData := rn.encryptXattrValue(data)
 	return n.setXAttr(cAttr, cData, flags)
 }
@@ -107,7 +112,10 @@ func (n *Node) Removexattr(ctx context.Context, attr string) syscall.Errno {
 		return n.removeXAttr(attr)
 	}
 
-	cAttr := rn.encryptXattrName(attr)
+	cAttr, err := rn.encryptXattrName(attr)
+	if err != nil {
+		return syscall.EINVAL
+	}
 	return n.removeXAttr(cAttr)
 }
 
diff --git a/internal/fusefrontend/root_node.go b/internal/fusefrontend/root_node.go
index e998e9d..a830cc4 100644
--- a/internal/fusefrontend/root_node.go
+++ b/internal/fusefrontend/root_node.go
@@ -311,10 +311,13 @@ func (rn *RootNode) decryptXattrValue(cData []byte) (data []byte, err error) {
 }
 
 // encryptXattrName transforms "user.foo" to "user.gocryptfs.a5sAd4XAa47f5as6dAf"
-func (rn *RootNode) encryptXattrName(attr string) (cAttr string) {
+func (rn *RootNode) encryptXattrName(attr string) (string, error) {
 	// xattr names are encrypted like file names, but with a fixed IV.
-	cAttr = xattrStorePrefix + rn.nameTransform.EncryptName(attr, xattrNameIV)
-	return cAttr
+	cAttr, err := rn.nameTransform.EncryptName(attr, xattrNameIV)
+	if err != nil {
+		return "", err
+	}
+	return xattrStorePrefix + cAttr, nil
 }
 
 func (rn *RootNode) decryptXattrName(cAttr string) (attr string, err error) {
diff --git a/internal/fusefrontend/xattr_unit_test.go b/internal/fusefrontend/xattr_unit_test.go
index f6c0469..a0cf4c8 100644
--- a/internal/fusefrontend/xattr_unit_test.go
+++ b/internal/fusefrontend/xattr_unit_test.go
@@ -33,7 +33,10 @@ func newTestFS(args Args) *RootNode {
 func TestEncryptDecryptXattrName(t *testing.T) {
 	fs := newTestFS(Args{})
 	attr1 := "user.foo123456789"
-	cAttr := fs.encryptXattrName(attr1)
+	cAttr, err := fs.encryptXattrName(attr1)
+	if err != nil {
+		t.Fatal(err)
+	}
 	t.Logf("cAttr=%v", cAttr)
 	attr2, err := fs.decryptXattrName(cAttr)
 	if attr1 != attr2 || err != nil {
diff --git a/internal/fusefrontend_reverse/ctlsock_interface.go b/internal/fusefrontend_reverse/ctlsock_interface.go
index 6bf2e6a..2157044 100644
--- a/internal/fusefrontend_reverse/ctlsock_interface.go
+++ b/internal/fusefrontend_reverse/ctlsock_interface.go
@@ -23,7 +23,10 @@ func (rn *RootNode) EncryptPath(plainPath string) (string, error) {
 	parts := strings.Split(plainPath, "/")
 	for _, part := range parts {
 		dirIV := pathiv.Derive(cipherPath, pathiv.PurposeDirIV)
-		encryptedPart := rn.nameTransform.EncryptName(part, dirIV)
+		encryptedPart, err := rn.nameTransform.EncryptName(part, dirIV)
+		if err != nil {
+			return "", err
+		}
 		if rn.args.LongNames && len(encryptedPart) > unix.NAME_MAX {
 			encryptedPart = rn.nameTransform.HashLongName(encryptedPart)
 		}
diff --git a/internal/fusefrontend_reverse/node_dir_ops.go b/internal/fusefrontend_reverse/node_dir_ops.go
index 22f8122..c287284 100644
--- a/internal/fusefrontend_reverse/node_dir_ops.go
+++ b/internal/fusefrontend_reverse/node_dir_ops.go
@@ -64,7 +64,11 @@ func (n *Node) Readdir(ctx context.Context) (stream fs.DirStream, errno syscall.
 			!rn.args.ConfigCustom {
 			cName = configfile.ConfDefaultName
 		} else {
-			cName = rn.nameTransform.EncryptName(entries[i].Name, dirIV)
+			cName, err = rn.nameTransform.EncryptName(entries[i].Name, dirIV)
+			if err != nil {
+				entries[i].Name = "___GOCRYPTFS_INVALID_NAME___"
+				continue
+			}
 			if len(cName) > unix.NAME_MAX {
 				cName = rn.nameTransform.HashLongName(cName)
 				dotNameFile := fuse.DirEntry{
diff --git a/internal/fusefrontend_reverse/root_node.go b/internal/fusefrontend_reverse/root_node.go
index b7a259a..10b0d69 100644
--- a/internal/fusefrontend_reverse/root_node.go
+++ b/internal/fusefrontend_reverse/root_node.go
@@ -71,9 +71,12 @@ func (rn *RootNode) findLongnameParent(fd int, diriv []byte, longname string) (p
 		if len(entry.Name) <= shortNameMax {
 			continue
 		}
-		cFullName = rn.nameTransform.EncryptName(entry.Name, diriv)
+		cFullName, err = rn.nameTransform.EncryptName(entry.Name, diriv)
+		if err != nil {
+			continue
+		}
 		if len(cFullName) <= unix.NAME_MAX {
-			// Entry should have been skipped by the "continue" above
+			// Entry should have been skipped by the shortNameMax check above
 			log.Panic("logic error or wrong shortNameMax constant?")
 		}
 		hName := rn.nameTransform.HashLongName(cFullName)
diff --git a/internal/nametransform/diriv.go b/internal/nametransform/diriv.go
index b2f165b..1d27aa5 100644
--- a/internal/nametransform/diriv.go
+++ b/internal/nametransform/diriv.go
@@ -102,7 +102,10 @@ func (be *NameTransform) EncryptAndHashName(name string, iv []byte) (string, err
 	if len(name) > NameMax {
 		return "", syscall.ENAMETOOLONG
 	}
-	cName := be.EncryptName(name, iv)
+	cName, err := be.EncryptName(name, iv)
+	if err != nil {
+		return "", err
+	}
 	if be.longNames && len(cName) > NameMax {
 		return be.HashLongName(cName), nil
 	}
diff --git a/internal/nametransform/longnames.go b/internal/nametransform/longnames.go
index aa463a1..74ddb07 100644
--- a/internal/nametransform/longnames.go
+++ b/internal/nametransform/longnames.go
@@ -132,7 +132,10 @@ func (n *NameTransform) WriteLongNameAt(dirfd int, hashName string, plainName st
 	if err != nil {
 		return err
 	}
-	cName := n.EncryptName(plainName, dirIV)
+	cName, err := n.EncryptName(plainName, dirIV)
+	if err != nil {
+		return err
+	}
 
 	// Write the encrypted name into hashName.name
 	fdRaw, err := syscallcompat.Openat(dirfd, hashName+LongNameSuffix,
diff --git a/internal/nametransform/names.go b/internal/nametransform/names.go
index 119d592..ca28230 100644
--- a/internal/nametransform/names.go
+++ b/internal/nametransform/names.go
@@ -2,7 +2,6 @@
 package nametransform
 
 import (
-	"bytes"
 	"crypto/aes"
 	"encoding/base64"
 	"path/filepath"
@@ -21,7 +20,7 @@ const (
 // NameTransformer is an interface used to transform filenames.
 type NameTransformer interface {
 	DecryptName(cipherName string, iv []byte) (string, error)
-	EncryptName(plainName string, iv []byte) string
+	EncryptName(plainName string, iv []byte) (string, error)
 	EncryptAndHashName(name string, iv []byte) (string, error)
 	// HashLongName - take the hash of a long string "name" and return
 	// "gocryptfs.longname.[sha256]"
@@ -99,22 +98,14 @@ func (n *NameTransform) decryptName(cipherName string, iv []byte) (string, error
 	bin = n.emeCipher.Decrypt(iv, bin)
 	bin, err = unPad16(bin)
 	if err != nil {
-		tlog.Debug.Printf("DecryptName: unPad16 error detail: %v", err)
-		// unPad16 returns detailed errors including the position of the
-		// incorrect bytes. Kill the padding oracle by lumping everything into
-		// a generic error.
+		tlog.Warn.Printf("DecryptName %q: unPad16 error: %v", cipherName, err)
 		return "", syscall.EBADMSG
 	}
-	// A name can never contain a null byte or "/". Make sure we never return those
-	// to the kernel, even when we read a corrupted (or fuzzed) filesystem.
-	if bytes.Contains(bin, []byte{0}) || bytes.Contains(bin, []byte("/")) {
-		return "", syscall.EBADMSG
-	}
-	// The name should never be "." or "..".
-	if bytes.Equal(bin, []byte(".")) || bytes.Equal(bin, []byte("..")) {
+	plain := string(bin)
+	if err := IsValidName(plain); err != nil {
+		tlog.Warn.Printf("DecryptName %q: invalid name after decryption: %v", cipherName, err)
 		return "", syscall.EBADMSG
 	}
-	plain := string(bin)
 	return plain, err
 }
 
@@ -123,12 +114,16 @@ func (n *NameTransform) decryptName(cipherName string, iv []byte) (string, error
 //
 // This function is exported because in some cases, fusefrontend needs access
 // to the full (not hashed) name if longname is used.
-func (n *NameTransform) EncryptName(plainName string, iv []byte) (cipherName64 string) {
+func (n *NameTransform) EncryptName(plainName string, iv []byte) (cipherName64 string, err error) {
+	if err := IsValidName(plainName); err != nil {
+		tlog.Warn.Printf("EncryptName %q: invalid plainName: %v", plainName, err)
+		return "", syscall.EBADMSG
+	}
 	bin := []byte(plainName)
 	bin = pad16(bin)
 	bin = n.emeCipher.Encrypt(iv, bin)
 	cipherName64 = n.B64.EncodeToString(bin)
-	return cipherName64
+	return cipherName64, nil
 }
 
 // B64EncodeToString returns a Base64-encoded string
diff --git a/internal/nametransform/names_test.go b/internal/nametransform/names_test.go
index 9d77c2d..b4e98d4 100644
--- a/internal/nametransform/names_test.go
+++ b/internal/nametransform/names_test.go
@@ -2,6 +2,7 @@ package nametransform
 
 import (
 	"bytes"
+	"strings"
 	"testing"
 )
 
@@ -51,3 +52,26 @@ func TestUnpad16Garbage(t *testing.T) {
 		}
 	}
 }
+
+func TestIsValidName(t *testing.T) {
+	cases := []struct {
+		in   string
+		want bool
+	}{
+		{"", false},
+		{".", false},
+		{"..", false},
+		{"...", true},
+		{"asdasd/asdasd", false},
+		{"asdasd\000asdasd", false},
+		{"hello", true},
+		{strings.Repeat("x", 255), true},
+		{strings.Repeat("x", 256), false},
+	}
+	for _, c := range cases {
+		have := IsValidName(c.in)
+		if (have == nil) != c.want {
+			t.Errorf("IsValidName(%q): want %v have %v", c.in, c.want, have)
+		}
+	}
+}
diff --git a/internal/nametransform/valid.go b/internal/nametransform/valid.go
new file mode 100644
index 0000000..b991074
--- /dev/null
+++ b/internal/nametransform/valid.go
@@ -0,0 +1,27 @@
+package nametransform
+
+import (
+	"fmt"
+	"strings"
+)
+
+// IsValidName checks if `name` is a valid name for a normal file
+// (does not contain null bytes or "/" etc...).
+func IsValidName(name string) error {
+	if name == "" {
+		return fmt.Errorf("empty input")
+	}
+	if len(name) > NameMax {
+		return fmt.Errorf("too long")
+	}
+	// A name can never contain a null byte or "/". Make sure we never return those
+	// to the kernel, even when we read a corrupted (or fuzzed) filesystem.
+	if strings.Contains(name, "\000") || strings.Contains(name, "/") {
+		return fmt.Errorf("contains forbidden bytes")
+	}
+	// The name should never be "." or "..".
+	if name == "." || name == ".." {
+		return fmt.Errorf(". and .. are forbidden names")
+	}
+	return nil
+}
diff --git a/tests/example_filesystems/example_filesystems_test.go b/tests/example_filesystems/example_filesystems_test.go
index 88132c0..36fb554 100644
--- a/tests/example_filesystems/example_filesystems_test.go
+++ b/tests/example_filesystems/example_filesystems_test.go
@@ -333,7 +333,8 @@ func TestExampleFSv13reverse(t *testing.T) {
 	}
 	dirA = tmpFsPath + dirA
 	// Mount using password
-	test_helpers.MountOrFatal(t, dirA, dirB, "-reverse", "-extpass", "echo test", opensslOpt)
+	// We pass "-wpanic=false" because the '..' and '.' tests deliverately trigger warnings
+	test_helpers.MountOrFatal(t, dirA, dirB, "-reverse", "-extpass", "echo test", "-wpanic=false", opensslOpt)
 	c := dirB + "/gocryptfs.conf"
 	if !test_helpers.VerifyExistence(t, c) {
 		t.Errorf("%s missing", c)