From 1a47718420317f893831b0223153d56005d5b02b Mon Sep 17 00:00:00 2001
From: Eric Biggers <ebiggers@google.com>
Date: Wed, 23 Feb 2022 12:35:04 -0800
Subject: 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_test.go | 63 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

(limited to 'filesystem/filesystem_test.go')

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")
+	}
+}
-- 
cgit v1.3