From 04858ddd222bbf7156f33f99cfb293a9b1e15ec8 Mon Sep 17 00:00:00 2001 From: Jakob Unterwurzacher Date: Wed, 2 Jun 2021 14:21:30 +0200 Subject: [PATCH] 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 | 16 +++++++--- internal/fusefrontend/root_node.go | 9 ++++-- internal/fusefrontend/xattr_unit_test.go | 5 +++- .../fusefrontend_reverse/ctlsock_interface.go | 5 +++- internal/fusefrontend_reverse/node_dir_ops.go | 6 +++- internal/fusefrontend_reverse/root_node.go | 7 +++-- internal/nametransform/diriv.go | 5 +++- internal/nametransform/longnames.go | 5 +++- internal/nametransform/names.go | 29 ++++++++----------- internal/nametransform/names_test.go | 24 +++++++++++++++ internal/nametransform/valid.go | 27 +++++++++++++++++ .../example_filesystems_test.go | 3 +- 12 files changed, 109 insertions(+), 32 deletions(-) create mode 100644 internal/nametransform/valid.go 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. - 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 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)