From 5ae7da4ee6582099de5d1b14733f8d58f4dc2816 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Wed, 22 Dec 2021 22:46:16 -0600 Subject: [PATCH] filesystem: store mountpoint in link files as a fallback Currently, linked protectors use filesystem link files of the form "UUID=". These links get broken if the filesystem's UUID changes, e.g. due to the filesystem being re-created even if the ".fscrypt" directory is backed up and restored. To prevent links from being broken (in most cases), start storing the mountpoint path in the link files too, in the form "UUID=\nPATH=\n". When following a link, try the UUID first, and if it doesn't work try the PATH. While it's possible that the path changed too, for login protectors (the usual use case of linked protectors) this won't be an issue as the path will always be "/". An alternative solution would be to fall back to scanning all filesystems for the needed protector descriptor. I decided not to do that, since relying on a global scan doesn't seem to be a good design. It wouldn't scale to large numbers of filesystems, it could cross security boundaries, and it would make it possible for adding a new filesystem to break fscrypt on existing filesystems. And if a global scan was an acceptable way to find protectors during normal use, then there would be no need for link files in the first place. Note: this change is backwards compatible (i.e., fscrypt will continue to recognize old link files) but not forwards-compatible (i.e., previous versions of fscrypt won't recognize new link files). Fixes https://github.com/google/fscrypt/issues/311 --- cli-tests/t_encrypt_login.out | 17 ++++ cli-tests/t_encrypt_login.sh | 8 ++ filesystem/filesystem.go | 5 +- filesystem/mountpoint.go | 151 +++++++++++++++++++++++----------- filesystem/mountpoint_test.go | 68 ++++++++++++++- 5 files changed, 195 insertions(+), 54 deletions(-) diff --git a/cli-tests/t_encrypt_login.out b/cli-tests/t_encrypt_login.out index 220d901..269f597 100644 --- a/cli-tests/t_encrypt_login.out +++ b/cli-tests/t_encrypt_login.out @@ -174,3 +174,20 @@ ext4 filesystem "MNT_ROOT" has 0 protectors and 0 policies [ERROR] fscrypt status: file or directory "MNT/dir" is not encrypted + +# Test that linked protector works even if UUID link is broken + +IMPORTANT: See "MNT/dir/fscrypt_recovery_readme.txt" for + important recovery instructions. It is *strongly recommended* to + record the recovery passphrase in a secure location; otherwise you + will lose access to this directory if you reinstall the operating + system or move this filesystem to another system. + +ext4 filesystem "MNT" has 2 protectors and 1 policy + +PROTECTOR LINKED DESCRIPTION +desc39 No custom protector "Recovery passphrase for dir" +desc40 Yes (MNT_ROOT) login protector for fscrypt-test-user + +POLICY UNLOCKED PROTECTORS +desc41 Yes desc40, desc39 diff --git a/cli-tests/t_encrypt_login.sh b/cli-tests/t_encrypt_login.sh index e03122d..a163811 100755 --- a/cli-tests/t_encrypt_login.sh +++ b/cli-tests/t_encrypt_login.sh @@ -91,3 +91,11 @@ chown "$TEST_USER" "$dir" _user_do_and_expect_failure \ "echo wrong_passphrase | fscrypt encrypt --quiet --source=pam_passphrase '$dir'" show_status false + +begin "Test that linked protector works even if UUID link is broken" +echo TEST_USER_PASS | fscrypt encrypt --quiet --source=pam_passphrase --user="$TEST_USER" "$dir" +protector=$(get_login_protector) +link_file=$MNT/.fscrypt/protectors/$protector.link +[ -e "$link_file" ] || _fail "$link_file does not exist" +sed -i 's/UUID=.*/UUID=00000000-0000-0000-0000-000000000000/' "$link_file" +fscrypt status "$MNT" diff --git a/filesystem/filesystem.go b/filesystem/filesystem.go index 0b30452..ab4badb 100644 --- a/filesystem/filesystem.go +++ b/filesystem/filesystem.go @@ -176,8 +176,7 @@ var SortDescriptorsByLastMtime = false // // There is also the ability to reference another filesystem's metadata. This is // used when a Policy on filesystem A is protected with Protector on filesystem -// B. In this scenario, we store a "link file" in the protectors directory whose -// contents look like "UUID=3a6d9a76-47f0-4f13-81bf-3332fbe984fb". +// B. In this scenario, we store a "link file" in the protectors directory. // // We also allow ".fscrypt" to be a symlink which was previously created. This // allows login protectors to be created when the root filesystem is read-only, @@ -588,7 +587,7 @@ func (m *Mount) AddLinkedProtector(descriptor string, dest *Mount) (bool, error) // Right now, we only make links using UUIDs. var newLink string - newLink, err = makeLink(dest, "UUID") + newLink, err = makeLink(dest) if err != nil { return false, err } diff --git a/filesystem/mountpoint.go b/filesystem/mountpoint.go index c830780..1f518ec 100644 --- a/filesystem/mountpoint.go +++ b/filesystem/mountpoint.go @@ -51,6 +51,7 @@ var ( mountsInitialized bool // Supported tokens for filesystem links uuidToken = "UUID" + pathToken = "PATH" // Location to perform UUID lookup uuidDirectory = "/dev/disk/by-uuid" ) @@ -399,78 +400,132 @@ func GetMount(mountpoint string) (*Mount, error) { return mnt, nil } -// getMountFromLink returns the Mount object which matches the provided link. -// This link is formatted as a tag (e.g. =) similar to how they -// appear in "/etc/fstab". Currently, only "UUID" tokens are supported. An error -// is returned if the link is invalid or we cannot load the required mount data. -// If a mount has been updated since the last call to one of the mount -// functions, run UpdateMountInfo to see the change. -func getMountFromLink(link string) (*Mount, error) { - // Parse the link - link = strings.TrimSpace(link) - linkComponents := strings.Split(link, "=") - if len(linkComponents) != 2 { - return nil, &ErrFollowLink{link, errors.New("invalid link format")} - } - token := linkComponents[0] - value := linkComponents[1] - if token != uuidToken { - return nil, &ErrFollowLink{link, errors.Errorf("token type %q not supported", token)} - } - - // See if UUID points to an existing device - searchPath := filepath.Join(uuidDirectory, value) - if filepath.Base(searchPath) != value { - return nil, &ErrFollowLink{link, errors.Errorf("invalid UUID format %q", value)} - } - deviceNumber, err := getDeviceNumber(searchPath) - if err != nil { - return nil, &ErrFollowLink{link, errors.Errorf("no device with UUID %s", value)} - } +func uuidToDeviceNumber(uuid string) (DeviceNumber, error) { + uuidSymlinkPath := filepath.Join(uuidDirectory, uuid) + return getDeviceNumber(uuidSymlinkPath) +} - // Lookup mountpoints for device in global store +func deviceNumberToMount(deviceNumber DeviceNumber) (*Mount, bool) { mountMutex.Lock() defer mountMutex.Unlock() if err := loadMountInfo(); err != nil { - return nil, err + log.Print(err) + return nil, false } mnt, ok := mountsByDevice[deviceNumber] - if !ok { - return nil, &ErrFollowLink{link, errors.Errorf("no mounts for device %q (%v)", - getDeviceName(deviceNumber), deviceNumber)} + return mnt, ok +} + +// getMountFromLink returns the main Mount, if any, for the filesystem which the +// given link points to. The link should contain a series of token-value pairs +// (=), one per line. The supported tokens are "UUID" and "PATH". +// If the UUID is present and it works, then it is used; otherwise, PATH is used +// if it is present. (The fallback from UUID to PATH will keep the link working +// if the UUID of the target filesystem changes but its mountpoint doesn't.) +// +// If a mount has been updated since the last call to one of the mount +// functions, make sure to run UpdateMountInfo first. +func getMountFromLink(link string) (*Mount, error) { + // Parse the link. + uuid := "" + path := "" + lines := strings.Split(link, "\n") + for _, line := range lines { + line := strings.TrimSpace(line) + if line == "" { + continue + } + pair := strings.Split(line, "=") + if len(pair) != 2 { + log.Printf("ignoring invalid line in filesystem link file: %q", line) + continue + } + token := pair[0] + value := pair[1] + switch token { + case uuidToken: + uuid = value + case pathToken: + path = value + default: + log.Printf("ignoring unknown link token %q", token) + } } - if mnt == nil { - return nil, &ErrFollowLink{link, filesystemLacksMainMountError(deviceNumber)} + // At least one of UUID and PATH must be present. + if uuid == "" && path == "" { + return nil, &ErrFollowLink{link, errors.Errorf("invalid filesystem link file")} } - return mnt, nil -} -// makeLink returns a link of the form = where value is the tag -// value for the Mount's device. Currently, only "UUID" tokens are supported. An -// error is returned if the mount has no device, or no UUID. -func makeLink(mnt *Mount, token string) (string, error) { - if token != uuidToken { - return "", &ErrMakeLink{mnt, errors.Errorf("token type %q not supported", token)} + // Try following the UUID. + errMsg := "" + if uuid != "" { + deviceNumber, err := uuidToDeviceNumber(uuid) + if err == nil { + mnt, ok := deviceNumberToMount(deviceNumber) + if mnt != nil { + log.Printf("resolved filesystem link using UUID %q", uuid) + return mnt, nil + } + if ok { + return nil, &ErrFollowLink{link, filesystemLacksMainMountError(deviceNumber)} + } + log.Printf("cannot find filesystem with UUID %q", uuid) + } else { + log.Printf("cannot find filesystem with UUID %q: %v", uuid, err) + } + errMsg += fmt.Sprintf("cannot find filesystem with UUID %q", uuid) + if path != "" { + log.Printf("falling back to using mountpoint path instead of UUID") + } } + // UUID didn't work. As a fallback, try the mountpoint path. + if path != "" { + mnt, err := GetMount(path) + if mnt != nil { + log.Printf("resolved filesystem link using mountpoint path %q", path) + return mnt, nil + } + log.Print(err) + if errMsg == "" { + errMsg = fmt.Sprintf("cannot find filesystem with main mountpoint %q", path) + } else { + errMsg += fmt.Sprintf(" or main mountpoint %q", path) + } + } + // No method worked; return an error. + return nil, &ErrFollowLink{link, errors.New(errMsg)} +} +func (mnt *Mount) getFilesystemUUID() (string, error) { dirContents, err := ioutil.ReadDir(uuidDirectory) if err != nil { - return "", &ErrMakeLink{mnt, err} + return "", err } for _, fileInfo := range dirContents { if fileInfo.Mode()&os.ModeSymlink == 0 { continue // Only interested in UUID symlinks } uuid := fileInfo.Name() - deviceNumber, err := getDeviceNumber(filepath.Join(uuidDirectory, uuid)) + deviceNumber, err := uuidToDeviceNumber(uuid) if err != nil { log.Print(err) continue } if mnt.DeviceNumber == deviceNumber { - return fmt.Sprintf("%s=%s", uuidToken, uuid), nil + return uuid, nil } } - return "", &ErrMakeLink{mnt, errors.Errorf("cannot determine UUID of device %q (%v)", - mnt.Device, mnt.DeviceNumber)} + return "", errors.Errorf("cannot determine UUID of device %q (%v)", + mnt.Device, mnt.DeviceNumber) +} + +// makeLink creates the contents of a link file which will point to the given +// filesystem. This will be a string of the form "UUID=\nPATH=\n". +// An error is returned if the filesystem's UUID cannot be determined. +func makeLink(mnt *Mount) (string, error) { + uuid, err := mnt.getFilesystemUUID() + if err != nil { + return "", &ErrMakeLink{mnt, err} + } + return fmt.Sprintf("%s=%s\n%s=%s\n", uuidToken, uuid, pathToken, mnt.Path), nil } diff --git a/filesystem/mountpoint_test.go b/filesystem/mountpoint_test.go index 633ff94..6600d87 100644 --- a/filesystem/mountpoint_test.go +++ b/filesystem/mountpoint_test.go @@ -373,14 +373,14 @@ func TestLoadAmbiguousMounts(t *testing.T) { } } -// Test making a filesystem link (i.e. "UUID=...") and following it, and test -// that leading and trailing whitespace in the link is ignored. +// Test making a filesystem link and following it, and test that leading and +// trailing whitespace in the link is ignored. func TestGetMountFromLink(t *testing.T) { mnt, err := getTestMount(t) if err != nil { t.Skip(err) } - link, err := makeLink(mnt, uuidToken) + link, err := makeLink(mnt) if err != nil { t.Fatal(err) } @@ -405,6 +405,68 @@ func TestGetMountFromLink(t *testing.T) { } } +// Test that old filesystem links that contain a UUID only still work. +func TestGetMountFromLegacyLink(t *testing.T) { + mnt, err := getTestMount(t) + if err != nil { + t.Skip(err) + } + uuid, err := mnt.getFilesystemUUID() + if uuid == "" || err != nil { + t.Fatal("Can't get UUID of test filesystem") + } + + link := fmt.Sprintf("UUID=%s", uuid) + linkedMnt, err := getMountFromLink(link) + if err != nil { + t.Fatal(err) + } + if linkedMnt != mnt { + t.Fatal("Link doesn't point to the same Mount") + } +} + +// Test that if the UUID in a filesystem link doesn't work, then the PATH is +// used instead, and vice versa. +func TestGetMountFromLinkFallback(t *testing.T) { + mnt, err := getTestMount(t) + if err != nil { + t.Skip(err) + } + badUUID := "00000000-0000-0000-0000-000000000000" + badPath := "/NONEXISTENT_MOUNT" + goodUUID, err := mnt.getFilesystemUUID() + if goodUUID == "" || err != nil { + t.Fatal("Can't get UUID of test filesystem") + } + + // only PATH valid (should succeed) + link := fmt.Sprintf("UUID=%s\nPATH=%s\n", badUUID, mnt.Path) + linkedMnt, err := getMountFromLink(link) + if err != nil { + t.Fatal(err) + } + if linkedMnt != mnt { + t.Fatal("Link doesn't point to the same Mount") + } + + // only UUID valid (should succeed) + link = fmt.Sprintf("UUID=%s\nPATH=%s\n", goodUUID, badPath) + if linkedMnt, err = getMountFromLink(link); err != nil { + t.Fatal(err) + } + if linkedMnt != mnt { + t.Fatal("Link doesn't point to the same Mount") + } + + // neither valid (should fail) + link = fmt.Sprintf("UUID=%s\nPATH=%s\n", badUUID, badPath) + linkedMnt, err = getMountFromLink(link) + if linkedMnt != nil || err == nil { + t.Fatal("Following a bad link succeeded") + } +} + // Benchmarks how long it takes to update the mountpoint data func BenchmarkLoadFirst(b *testing.B) { for n := 0; n < b.N; n++ { -- 2.39.5