From b44fbe71e1e93c47050322af51725bac997641e0 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Wed, 23 Feb 2022 12:35:04 -0800 Subject: [PATCH] filesystem: reject spoofed login protectors If a login protector contains a UID that differs from the file owner (and the file owner is not root), it might be a spoofed file that was created maliciously, so make sure to consider such files to be invalid. --- filesystem/filesystem.go | 57 ++++++++++++++++++---------- filesystem/filesystem_test.go | 71 ++++++++++++++++++++++++++++++++--- 2 files changed, 103 insertions(+), 25 deletions(-) diff --git a/filesystem/filesystem.go b/filesystem/filesystem.go index 3c44160..8ce8bde 100644 --- a/filesystem/filesystem.go +++ b/filesystem/filesystem.go @@ -42,6 +42,7 @@ import ( "path/filepath" "sort" "strings" + "syscall" "time" "github.com/golang/protobuf/proto" @@ -520,58 +521,60 @@ func (m *Mount) addMetadata(path string, md metadata.Metadata, owner *user.User) // 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) { +// This function returns the data read as well as the UID of the user who owns +// the file. The returned UID is needed for login protectors, where the UID +// needs to be cross-checked with the UID stored in the file itself. +func readMetadataFileSafe(path string) ([]byte, int64, error) { file, err := os.OpenFile(path, os.O_RDONLY|unix.O_NOFOLLOW|unix.O_NONBLOCK, 0) if err != nil { - return nil, err + return nil, -1, err } defer file.Close() info, err := file.Stat() if err != nil { - return nil, err + return nil, -1, err } if !info.Mode().IsRegular() { - return nil, &ErrCorruptMetadata{path, errors.New("not a regular file")} + return nil, -1, &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} + return nil, -1, &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 + return nil, -1, err } if reader.N == 0 { - return nil, &ErrCorruptMetadata{path, errors.New("metadata file size limit exceeded")} + return nil, -1, &ErrCorruptMetadata{path, errors.New("metadata file size limit exceeded")} } - return data, nil + return data, int64(info.Sys().(*syscall.Stat_t).Uid), 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 := readMetadataFileSafe(path) +func (m *Mount) getMetadata(path string, md metadata.Metadata) (int64, error) { + data, owner, err := readMetadataFileSafe(path) if err != nil { log.Printf("could not read metadata from %q: %v", path, err) - return err + return -1, err } if err := proto.Unmarshal(data, md); err != nil { - return &ErrCorruptMetadata{path, err} + return -1, &ErrCorruptMetadata{path, err} } if err := md.CheckValidity(); err != nil { - return &ErrCorruptMetadata{path, err} + return -1, &ErrCorruptMetadata{path, err} } log.Printf("successfully read metadata from %q", path) - return nil + return owner, nil } // removeMetadata deletes the metadata struct from the file with the specified @@ -626,7 +629,7 @@ func (m *Mount) AddLinkedProtector(descriptor string, dest *Mount) (bool, error) linkPath := m.linkedProtectorPath(descriptor) // Check whether the link already exists. - existingLink, err := readMetadataFileSafe(linkPath) + existingLink, _, err := readMetadataFileSafe(linkPath) if err == nil { existingLinkedMnt, err := getMountFromLink(string(existingLink)) if err != nil { @@ -658,11 +661,25 @@ func (m *Mount) GetRegularProtector(descriptor string) (*metadata.ProtectorData, } data := new(metadata.ProtectorData) path := m.protectorPath(descriptor) - err := m.getMetadata(path, data) + owner, err := m.getMetadata(path, data) if os.IsNotExist(err) { err = &ErrProtectorNotFound{descriptor, m} } - return data, err + if err != nil { + return nil, err + } + // Login protectors have their UID stored in the file. Since normally + // any user can create files in the fscrypt metadata directories, for a + // login protector to be considered valid it *must* be owned by the + // claimed user or by root. Note: fscrypt v0.3.2 and later always makes + // login protectors owned by the user, but previous versions could + // create them owned by root -- that is the main reason we allow root. + if data.Source == metadata.SourceType_pam_passphrase && owner != 0 && owner != data.Uid { + log.Printf("WARNING: %q claims to be the login protector for uid %d, but it is owned by uid %d. Needs to be %d or 0.", + path, data.Uid, owner, data.Uid) + return nil, &ErrCorruptMetadata{path, errors.New("login protector belongs to wrong user")} + } + return data, nil } // GetProtector returns the Mount of the filesystem containing the information @@ -674,7 +691,7 @@ func (m *Mount) GetProtector(descriptor string) (*Mount, *metadata.ProtectorData } // Get the link data from the link file path := m.linkedProtectorPath(descriptor) - link, err := readMetadataFileSafe(path) + link, _, err := readMetadataFileSafe(path) if err != nil { // If the link doesn't exist, try for a regular protector. if os.IsNotExist(err) { @@ -737,7 +754,7 @@ func (m *Mount) GetPolicy(descriptor string) (*metadata.PolicyData, error) { return nil, err } data := new(metadata.PolicyData) - err := m.getMetadata(m.PolicyPath(descriptor), data) + _, err := m.getMetadata(m.PolicyPath(descriptor), data) if os.IsNotExist(err) { err = &ErrPolicyNotFound{descriptor, m} } diff --git a/filesystem/filesystem_test.go b/filesystem/filesystem_test.go index 71724ae..7aa97cb 100644 --- a/filesystem/filesystem_test.go +++ b/filesystem/filesystem_test.go @@ -60,6 +60,19 @@ func getFakeProtector() *metadata.ProtectorData { } } +func getFakeLoginProtector(uid int64) *metadata.ProtectorData { + protector := getFakeProtector() + protector.Source = metadata.SourceType_pam_passphrase + protector.Uid = uid + protector.Costs = &metadata.HashingCosts{ + Time: 1, + Memory: 1 << 8, + Parallelism: 1, + } + protector.Salt = make([]byte, 16) + return protector +} + func getFakePolicy() *metadata.PolicyData { return &metadata.PolicyData{ KeyDescriptor: "0123456789abcdef", @@ -315,6 +328,50 @@ func TestSetProtector(t *testing.T) { } } +// Tests that a login protector whose embedded UID doesn't match the file owner +// is considered invalid. (Such a file could be created by a malicious user to +// try to confuse fscrypt into processing the wrong file.) +func TestSpoofedLoginProtector(t *testing.T) { + myUID := int64(os.Geteuid()) + badUID := myUID + 1 // anything different from myUID + mnt, err := getSetupMount(t) + if err != nil { + t.Fatal(err) + } + defer mnt.RemoveAllMetadata() + + // Control case: protector with matching UID should be accepted. + protector := getFakeLoginProtector(myUID) + if err = mnt.AddProtector(protector); err != nil { + t.Fatal(err) + } + _, err = mnt.GetRegularProtector(protector.ProtectorDescriptor) + if err != nil { + t.Fatal(err) + } + if err = mnt.RemoveProtector(protector.ProtectorDescriptor); err != nil { + t.Fatal(err) + } + + // The real test: protector with mismatching UID should rejected, + // *unless* the process running the tests (and hence the file owner) is + // root in which case it should be accepted. + protector = getFakeLoginProtector(badUID) + if err = mnt.AddProtector(protector); err != nil { + t.Fatal(err) + } + _, err = mnt.GetRegularProtector(protector.ProtectorDescriptor) + if myUID == 0 { + if err != nil { + t.Fatal(err) + } + } else { + if err == nil { + t.Fatal("reading protector with bad UID unexpectedly succeeded") + } + } +} + // Gets a setup mount and a fake second mount func getTwoSetupMounts(t *testing.T) (realMnt, fakeMnt *Mount, err error) { if realMnt, err = getSetupMount(t); err != nil { @@ -405,13 +462,17 @@ func TestReadMetadataFileSafe(t *testing.T) { if err = createFile(filePath, 1000); err != nil { t.Fatal(err) } - if _, err = readMetadataFileSafe(filePath); err != nil { + _, owner, err := readMetadataFileSafe(filePath) + if err != nil { t.Fatal("failed to read file") } + if owner != int64(os.Geteuid()) { + t.Fatal("got wrong owner") + } os.Remove(filePath) // Nonexistent file - _, err = readMetadataFileSafe(filePath) + _, _, err = readMetadataFileSafe(filePath) if !os.IsNotExist(err) { t.Fatal("trying to read nonexistent file didn't fail with expected error") } @@ -420,7 +481,7 @@ func TestReadMetadataFileSafe(t *testing.T) { if err = os.Symlink("target", filePath); err != nil { t.Fatal(err) } - _, err = readMetadataFileSafe(filePath) + _, _, err = readMetadataFileSafe(filePath) if err.(*os.PathError).Err != syscall.ELOOP { t.Fatal("trying to read symlink didn't fail with ELOOP") } @@ -430,7 +491,7 @@ func TestReadMetadataFileSafe(t *testing.T) { if err = unix.Mkfifo(filePath, 0600); err != nil { t.Fatal(err) } - _, err = readMetadataFileSafe(filePath) + _, _, err = readMetadataFileSafe(filePath) if _, ok := err.(*ErrCorruptMetadata); !ok { t.Fatal("trying to read FIFO didn't fail with expected error") } @@ -440,7 +501,7 @@ func TestReadMetadataFileSafe(t *testing.T) { if err = createFile(filePath, 1000000); err != nil { t.Fatal(err) } - _, err = readMetadataFileSafe(filePath) + _, _, 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