diff options
| -rw-r--r-- | filesystem/filesystem.go | 65 | ||||
| -rw-r--r-- | 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") + } +} |