From 1a47718420317f893831b0223153d56005d5b02b Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Wed, 23 Feb 2022 12:35:04 -0800 Subject: [PATCH] filesystem: validate size and type of metadata files Don't allow reading metadata files that are very large, as they can crash the program due to the memory required. Similarly, don't allow reading metadata files that aren't regular files, such as FIFOs, or symlinks (which could point to a device node like /dev/zero), as that can hang the program. Both issues were particularly problematic for pam_fscrypt, as they could prevent users from being able to log in. Note: these checks are arguably unneeded if we strictly check the file ownership too, which a later commit will do. But there's no reason not to do these basic checks too. --- filesystem/filesystem.go | 65 ++++++++++++++++++++++++++++++++--- filesystem/filesystem_test.go | 63 +++++++++++++++++++++++++++++++++ 2 files changed, 124 insertions(+), 4 deletions(-) diff --git a/filesystem/filesystem.go b/filesystem/filesystem.go index da6b9cc..3c44160 100644 --- a/filesystem/filesystem.go +++ b/filesystem/filesystem.go @@ -34,6 +34,7 @@ package filesystem import ( "fmt" + "io" "io/ioutil" "log" "os" @@ -213,6 +214,12 @@ const ( // The metadata files are globally visible, but can only be deleted by // the user that created them filePermissions = 0644 + + // Maximum size of a metadata file. This value is arbitrary, and it can + // be changed. We just set a reasonable limit that shouldn't be reached + // in practice, except by users trying to cause havoc by creating + // extremely large files in the metadata directories. + maxMetadataFileSize = 16384 ) func (m *Mount) String() string { @@ -496,10 +503,60 @@ func (m *Mount) addMetadata(path string, md metadata.Metadata, owner *user.User) return m.writeDataAtomic(path, data, owner) } +// readMetadataFileSafe gets the contents of a metadata file extra-carefully, +// considering that it could be a malicious file created to cause a +// denial-of-service. Specifically, the following checks are done: +// +// - It must be a regular file, not another type of file like a symlink or FIFO. +// (Symlinks aren't bad by themselves, but given that a malicious user could +// point one to absolutely anywhere, and there is no known use case for the +// metadata files themselves being symlinks, it seems best to disallow them.) +// - It must have a reasonable size (<= maxMetadataFileSize). +// +// Take care to avoid TOCTOU (time-of-check-time-of-use) bugs when doing these +// tests. Notably, we must open the file before checking the file type, as the +// file type could change between any previous checks and the open. When doing +// this, O_NOFOLLOW is needed to avoid following a symlink (this applies to the +// last path component only), and O_NONBLOCK is needed to avoid blocking if the +// file is a FIFO. +// +// This function returns the data read. +func readMetadataFileSafe(path string) ([]byte, error) { + file, err := os.OpenFile(path, os.O_RDONLY|unix.O_NOFOLLOW|unix.O_NONBLOCK, 0) + if err != nil { + return nil, err + } + defer file.Close() + + info, err := file.Stat() + if err != nil { + return nil, err + } + if !info.Mode().IsRegular() { + return nil, &ErrCorruptMetadata{path, errors.New("not a regular file")} + } + // Clear O_NONBLOCK, since it has served its purpose when opening the + // file, and the behavior of reading from a regular file with O_NONBLOCK + // is technically unspecified. + if _, err = unix.FcntlInt(file.Fd(), unix.F_SETFL, 0); err != nil { + return nil, &os.PathError{Op: "clearing O_NONBLOCK", Path: path, Err: err} + } + // Read the file contents, allowing at most maxMetadataFileSize bytes. + reader := &io.LimitedReader{R: file, N: maxMetadataFileSize + 1} + data, err := ioutil.ReadAll(reader) + if err != nil { + return nil, err + } + if reader.N == 0 { + return nil, &ErrCorruptMetadata{path, errors.New("metadata file size limit exceeded")} + } + return data, nil +} + // getMetadata reads the metadata structure from the file with the specified // path. Only reads normal metadata files, not linked metadata. func (m *Mount) getMetadata(path string, md metadata.Metadata) error { - data, err := ioutil.ReadFile(path) + data, err := readMetadataFileSafe(path) if err != nil { log.Printf("could not read metadata from %q: %v", path, err) return err @@ -569,7 +626,7 @@ func (m *Mount) AddLinkedProtector(descriptor string, dest *Mount) (bool, error) linkPath := m.linkedProtectorPath(descriptor) // Check whether the link already exists. - existingLink, err := ioutil.ReadFile(linkPath) + existingLink, err := readMetadataFileSafe(linkPath) if err == nil { existingLinkedMnt, err := getMountFromLink(string(existingLink)) if err != nil { @@ -594,7 +651,7 @@ func (m *Mount) AddLinkedProtector(descriptor string, dest *Mount) (bool, error) } // GetRegularProtector looks up the protector metadata by descriptor. This will -// fail with ErrNoMetadata if the descriptor is a linked protector. +// fail with ErrProtectorNotFound if the descriptor is a linked protector. func (m *Mount) GetRegularProtector(descriptor string) (*metadata.ProtectorData, error) { if err := m.CheckSetup(); err != nil { return nil, err @@ -617,7 +674,7 @@ func (m *Mount) GetProtector(descriptor string) (*Mount, *metadata.ProtectorData } // Get the link data from the link file path := m.linkedProtectorPath(descriptor) - link, err := ioutil.ReadFile(path) + link, err := readMetadataFileSafe(path) if err != nil { // If the link doesn't exist, try for a regular protector. if os.IsNotExist(err) { diff --git a/filesystem/filesystem_test.go b/filesystem/filesystem_test.go index 92726b2..71724ae 100644 --- a/filesystem/filesystem_test.go +++ b/filesystem/filesystem_test.go @@ -24,9 +24,11 @@ import ( "log" "os" "path/filepath" + "syscall" "testing" "github.com/golang/protobuf/proto" + "golang.org/x/sys/unix" "github.com/google/fscrypt/crypto" "github.com/google/fscrypt/metadata" @@ -382,3 +384,64 @@ func TestLinkedProtector(t *testing.T) { t.Errorf("protector %+v does not equal expected protector %+v", retProtector, protector) } } + +func createFile(path string, size int64) error { + if err := ioutil.WriteFile(path, []byte{}, 0600); err != nil { + return err + } + return os.Truncate(path, size) +} + +// Tests the readMetadataFileSafe() function. +func TestReadMetadataFileSafe(t *testing.T) { + tempDir, err := ioutil.TempDir("", "fscrypt") + if err != nil { + t.Fatal(err) + } + filePath := filepath.Join(tempDir, "file") + defer os.RemoveAll(tempDir) + + // Good file (control case) + if err = createFile(filePath, 1000); err != nil { + t.Fatal(err) + } + if _, err = readMetadataFileSafe(filePath); err != nil { + t.Fatal("failed to read file") + } + os.Remove(filePath) + + // Nonexistent file + _, err = readMetadataFileSafe(filePath) + if !os.IsNotExist(err) { + t.Fatal("trying to read nonexistent file didn't fail with expected error") + } + + // Symlink + if err = os.Symlink("target", filePath); err != nil { + t.Fatal(err) + } + _, err = readMetadataFileSafe(filePath) + if err.(*os.PathError).Err != syscall.ELOOP { + t.Fatal("trying to read symlink didn't fail with ELOOP") + } + os.Remove(filePath) + + // FIFO + if err = unix.Mkfifo(filePath, 0600); err != nil { + t.Fatal(err) + } + _, err = readMetadataFileSafe(filePath) + if _, ok := err.(*ErrCorruptMetadata); !ok { + t.Fatal("trying to read FIFO didn't fail with expected error") + } + os.Remove(filePath) + + // Very large file + if err = createFile(filePath, 1000000); err != nil { + t.Fatal(err) + } + _, err = readMetadataFileSafe(filePath) + if _, ok := err.(*ErrCorruptMetadata); !ok { + t.Fatal("trying to read very large file didn't fail with expected error") + } +} -- 2.39.5