From e827763f2e6226d9f5778d56c28270264950c0f5 Mon Sep 17 00:00:00 2001 From: Jakob Unterwurzacher Date: Tue, 23 May 2017 20:46:24 +0200 Subject: [PATCH] nametransform: harden name decryption against invalid input This fixes a few issues I have found reviewing the code: 1) Limit the amount of data ReadLongName() will read. Previously, you could send gocryptfs into out-of-memory by symlinking gocryptfs.diriv to /dev/zero. 2) Handle the empty input case in unPad16() by returning an error. Previously, it would panic with an out-of-bounds array read. It is unclear to me if this could actually be triggered. 3) Reject empty names after base64-decoding in DecryptName(). An empty name crashes emeCipher.Decrypt(). It is unclear to me if B64.DecodeString() can actually return a non-error empty result, but let's guard against it anyway. --- internal/fusefrontend/fs_dir.go | 11 ++++------- internal/nametransform/longnames.go | 27 ++++++++++++++++++++++----- internal/nametransform/names.go | 8 ++++++-- internal/nametransform/names_test.go | 17 +++++++++++++++++ internal/nametransform/pad16.go | 17 ++++++++++------- 5 files changed, 59 insertions(+), 21 deletions(-) diff --git a/internal/fusefrontend/fs_dir.go b/internal/fusefrontend/fs_dir.go index 7d1e3ef..30e715a 100644 --- a/internal/fusefrontend/fs_dir.go +++ b/internal/fusefrontend/fs_dir.go @@ -273,16 +273,14 @@ func (fs *FS) OpenDir(dirName string, context *fuse.Context) ([]fuse.DirEntry, f // silently ignore "gocryptfs.conf" in the top level dir continue } - if !fs.args.PlaintextNames && cName == nametransform.DirIVFilename { - // silently ignore "gocryptfs.diriv" everywhere if dirIV is enabled - continue - } - if fs.args.PlaintextNames { plain = append(plain, cipherEntries[i]) continue } - + if cName == nametransform.DirIVFilename { + // silently ignore "gocryptfs.diriv" everywhere if dirIV is enabled + continue + } // Handle long file name isLong := nametransform.LongNameNone if fs.args.LongNames { @@ -301,7 +299,6 @@ func (fs *FS) OpenDir(dirName string, context *fuse.Context) ([]fuse.DirEntry, f // ignore "gocryptfs.longname.*.name" continue } - name, err := fs.nameTransform.DecryptName(cName, cachedIV) if err != nil { tlog.Warn.Printf("OpenDir %q: invalid entry %q: %v", diff --git a/internal/nametransform/longnames.go b/internal/nametransform/longnames.go index 54095a4..8af191d 100644 --- a/internal/nametransform/longnames.go +++ b/internal/nametransform/longnames.go @@ -2,7 +2,8 @@ package nametransform import ( "crypto/sha256" - "io/ioutil" + "fmt" + "io" "os" "path/filepath" "strings" @@ -62,13 +63,29 @@ func IsLongContent(cName string) bool { return NameType(cName) == LongNameContent } -// ReadLongName - read path.name +// ReadLongName - read "$path.name" func ReadLongName(path string) (string, error) { - content, err := ioutil.ReadFile(path + LongNameSuffix) + path += LongNameSuffix + fd, err := os.Open(path) if err != nil { - tlog.Warn.Printf("ReadLongName: %v", err) + return "", err } - return string(content), err + defer fd.Close() + // 256 (=255 padded to 16) bytes base64-encoded take 344 bytes: "AAAAAAA...AAA==" + lim := 344 + // Allocate a bigger buffer so we see whether the file is too big + buf := make([]byte, lim+1) + n, err := fd.ReadAt(buf, 0) + if err != nil && err != io.EOF { + return "", err + } + if n == 0 { + return "", fmt.Errorf("ReadLongName: empty file") + } + if n > lim { + return "", fmt.Errorf("ReadLongName: size=%d > limit=%d", n, lim) + } + return string(buf[0:n]), nil } // DeleteLongName deletes "hashName.name". diff --git a/internal/nametransform/names.go b/internal/nametransform/names.go index c96f7ce..3447583 100644 --- a/internal/nametransform/names.go +++ b/internal/nametransform/names.go @@ -42,6 +42,10 @@ func (n *NameTransform) DecryptName(cipherName string, iv []byte) (string, error if err != nil { return "", err } + if len(bin) == 0 { + tlog.Warn.Printf("DecryptName: empty input") + return "", syscall.EBADMSG + } if len(bin)%aes.BlockSize != 0 { tlog.Debug.Printf("DecryptName %q: decoded length %d is not a multiple of 16", cipherName, len(bin)) return "", syscall.EBADMSG @@ -49,14 +53,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: %v", err) + 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 user, even when we read a corrupted (or fuzzed) filesystem. + // 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 } diff --git a/internal/nametransform/names_test.go b/internal/nametransform/names_test.go index d772af2..0254777 100644 --- a/internal/nametransform/names_test.go +++ b/internal/nametransform/names_test.go @@ -32,3 +32,20 @@ func TestPad16(t *testing.T) { } } } + +// TestUnpad16Garbage - unPad16 should never crash on corrupt or malicious inputs +func TestUnpad16Garbage(t *testing.T) { + var testCases [][]byte + testCases = append(testCases, make([]byte, 0)) + testCases = append(testCases, make([]byte, 16)) + testCases = append(testCases, make([]byte, 1)) + testCases = append(testCases, make([]byte, 17)) + testCases = append(testCases, bytes.Repeat([]byte{16}, 16)) + testCases = append(testCases, bytes.Repeat([]byte{17}, 16)) + for _, v := range testCases { + _, err := unPad16([]byte(v)) + if err == nil { + t.Fail() + } + } +} diff --git a/internal/nametransform/pad16.go b/internal/nametransform/pad16.go index 2b30bcb..833be0e 100644 --- a/internal/nametransform/pad16.go +++ b/internal/nametransform/pad16.go @@ -31,6 +31,9 @@ func pad16(orig []byte) (padded []byte) { // unPad16 - remove padding func unPad16(padded []byte) ([]byte, error) { oldLen := len(padded) + if oldLen == 0 { + return nil, errors.New("Empty input") + } if oldLen%aes.BlockSize != 0 { return nil, errors.New("Unaligned size") } @@ -39,12 +42,16 @@ func unPad16(padded []byte) ([]byte, error) { // The padding byte's value is the padding length padLen := int(padByte) // Padding must be at least 1 byte - if padLen <= 0 { + if padLen == 0 { return nil, errors.New("Padding cannot be zero-length") } - // Larger paddings make no sense + // Padding more than 16 bytes make no sense if padLen > aes.BlockSize { - return nil, fmt.Errorf("Padding too long, padLen = %d > 16", padLen) + return nil, fmt.Errorf("Padding too long, padLen=%d > 16", padLen) + } + // Padding cannot be as long as (or longer than) the whole string, + if padLen >= oldLen { + return nil, fmt.Errorf("Padding too long, oldLen=%d >= padLen=%d", oldLen, padLen) } // All padding bytes must be identical for i := oldLen - padLen; i < oldLen; i++ { @@ -53,9 +60,5 @@ func unPad16(padded []byte) ([]byte, error) { } } newLen := oldLen - padLen - // Padding an empty string makes no sense - if newLen == 0 { - return nil, errors.New("Unpadded length is zero") - } return padded[0:newLen], nil }