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.

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)
 }
 

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) {

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 {

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)
 		}

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{

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)

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
 	}

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,

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.
-		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("..")) {
+		tlog.Warn.Printf("DecryptName %q: unPad16 error: %v", cipherName, err)
 		return "", syscall.EBADMSG
 	}
 	plain := string(bin)
+	if err := IsValidName(plain); err != nil {
+		tlog.Warn.Printf("DecryptName %q: invalid name after decryption: %v", cipherName, err)
+		return "", syscall.EBADMSG
+	}
 	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

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)
+		}
+	}
+}

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
+}

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)