From 6e355131670ad014e45f879475ddf800f0080d41 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Wed, 23 Feb 2022 12:35:04 -0800 Subject: Make 'fscrypt setup' offer a choice of directory modes World-writable directories are not appropriate for some systems, so offer a choice of single-user-writable and world-writable modes, with single-user-writable being the default. Add a new documentation section to help users decide which one to use. --- cli-tests/run.sh | 4 +-- cli-tests/t_encrypt.out | 18 +++++++++----- cli-tests/t_encrypt_custom.out | 9 ++++--- cli-tests/t_encrypt_login.out | 42 ++++++++++++++++++++----------- cli-tests/t_encrypt_raw_key.out | 15 +++++++---- cli-tests/t_metadata.out | 6 +++-- cli-tests/t_not_supported.sh | 2 +- cli-tests/t_setup.out | 6 +++-- cli-tests/t_setup.sh | 4 +-- cli-tests/t_single_user.out | 30 ++++++++++++++++++++++ cli-tests/t_single_user.sh | 55 +++++++++++++++++++++++++++++++++++++++++ cli-tests/t_status.out | 6 +++-- cli-tests/t_v1_policy.out | 3 ++- 13 files changed, 160 insertions(+), 40 deletions(-) create mode 100644 cli-tests/t_single_user.out create mode 100755 cli-tests/t_single_user.sh (limited to 'cli-tests') diff --git a/cli-tests/run.sh b/cli-tests/run.sh index dc17b5b..f6a4868 100755 --- a/cli-tests/run.sh +++ b/cli-tests/run.sh @@ -159,7 +159,7 @@ setup_for_test() # Give the tests their own fscrypt.conf. export FSCRYPT_CONF="$TMPDIR/fscrypt.conf" - fscrypt setup --time=1ms > /dev/null + fscrypt setup --time=1ms --quiet --all-users > /dev/null # The tests assume kernel support for v2 policies. if ! grep -q '"policy_version": "2"' "$FSCRYPT_CONF"; then @@ -171,7 +171,7 @@ EOF fi # Set up the test filesystems that aren't already set up. - fscrypt setup "$MNT" > /dev/null + fscrypt setup --quiet --all-users "$MNT" > /dev/null } run_test() diff --git a/cli-tests/t_encrypt.out b/cli-tests/t_encrypt.out index f067fc0..b92c9d9 100644 --- a/cli-tests/t_encrypt.out +++ b/cli-tests/t_encrypt.out @@ -1,7 +1,8 @@ # Try to encrypt a nonexistent directory [ERROR] fscrypt encrypt: no such file or directory -ext4 filesystem "MNT" has 0 protectors and 0 policies +ext4 filesystem "MNT" has 0 protectors and 0 policies. +All users can create fscrypt metadata on this filesystem. [ERROR] fscrypt status: file or directory "MNT/dir" is not encrypted @@ -23,7 +24,8 @@ files into it, and securely delete the original directory. For example: Caution: due to the nature of modern storage devices and filesystems, the original data may still be recoverable from disk. It's much better to encrypt your files from the start. -ext4 filesystem "MNT" has 0 protectors and 0 policies +ext4 filesystem "MNT" has 0 protectors and 0 policies. +All users can create fscrypt metadata on this filesystem. [ERROR] fscrypt status: file or directory "MNT/dir" is not encrypted @@ -45,13 +47,15 @@ files into it, and securely delete the original directory. For example: Caution: due to the nature of modern storage devices and filesystems, the original data may still be recoverable from disk. It's much better to encrypt your files from the start. -ext4 filesystem "MNT" has 0 protectors and 0 policies +ext4 filesystem "MNT" has 0 protectors and 0 policies. +All users can create fscrypt metadata on this filesystem. [ERROR] fscrypt status: file or directory "MNT/dir" is not encrypted # Encrypt a directory as non-root user -ext4 filesystem "MNT" has 1 protector and 1 policy +ext4 filesystem "MNT" has 1 protector and 1 policy. +All users can create fscrypt metadata on this filesystem. PROTECTOR LINKED DESCRIPTION desc1 No custom protector "prot" @@ -67,7 +71,8 @@ Unlocked: Yes Protected with 1 protector: PROTECTOR LINKED DESCRIPTION desc1 No custom protector "prot" -ext4 filesystem "MNT" has 1 protector and 1 policy +ext4 filesystem "MNT" has 1 protector and 1 policy. +All users can create fscrypt metadata on this filesystem. PROTECTOR LINKED DESCRIPTION desc1 No custom protector "prot" @@ -94,7 +99,8 @@ desc1 No custom protector "prot" Encryption can only be enabled on a directory you own, even if you have write access to the directory. -ext4 filesystem "MNT" has 0 protectors and 0 policies +ext4 filesystem "MNT" has 0 protectors and 0 policies. +All users can create fscrypt metadata on this filesystem. [ERROR] fscrypt status: file or directory "MNT/dir" is not encrypted diff --git a/cli-tests/t_encrypt_custom.out b/cli-tests/t_encrypt_custom.out index 8dd15e3..ac53d6f 100644 --- a/cli-tests/t_encrypt_custom.out +++ b/cli-tests/t_encrypt_custom.out @@ -1,6 +1,7 @@ # Encrypt with custom passphrase protector -ext4 filesystem "MNT" has 1 protector and 1 policy +ext4 filesystem "MNT" has 1 protector and 1 policy. +All users can create fscrypt metadata on this filesystem. PROTECTOR LINKED DESCRIPTION desc1 No custom protector "prot" @@ -28,7 +29,8 @@ Enter a name for the new protector: prot Enter custom passphrase for protector "prot": Confirm passphrase: "MNT/dir" is now encrypted, unlocked, and ready for use. -ext4 filesystem "MNT" has 1 protector and 1 policy +ext4 filesystem "MNT" has 1 protector and 1 policy. +All users can create fscrypt metadata on this filesystem. PROTECTOR LINKED DESCRIPTION desc6 No custom protector "prot" @@ -49,7 +51,8 @@ desc6 No custom protector "prot" [ERROR] fscrypt encrypt: custom_passphrase protectors must be named Use --name=PROTECTOR_NAME to specify a protector name. -ext4 filesystem "MNT" has 0 protectors and 0 policies +ext4 filesystem "MNT" has 0 protectors and 0 policies. +All users can create fscrypt metadata on this filesystem. [ERROR] fscrypt status: file or directory "MNT/dir" is not encrypted diff --git a/cli-tests/t_encrypt_login.out b/cli-tests/t_encrypt_login.out index 269f597..b84216a 100644 --- a/cli-tests/t_encrypt_login.out +++ b/cli-tests/t_encrypt_login.out @@ -7,7 +7,8 @@ IMPORTANT: See "MNT/dir/fscrypt_recovery_readme.txt" for 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 +ext4 filesystem "MNT" has 2 protectors and 1 policy. +All users can create fscrypt metadata on this filesystem. PROTECTOR LINKED DESCRIPTION desc1 Yes (MNT_ROOT) login protector for fscrypt-test-user @@ -15,7 +16,8 @@ desc2 No custom protector "Recovery passphrase POLICY UNLOCKED PROTECTORS desc3 Yes desc1, desc2 -ext4 filesystem "MNT_ROOT" has 1 protector and 0 policies +ext4 filesystem "MNT_ROOT" has 1 protector and 0 policies. +All users can create fscrypt metadata on this filesystem. PROTECTOR LINKED DESCRIPTION desc1 No login protector for fscrypt-test-user @@ -57,7 +59,8 @@ IMPORTANT: See "MNT/dir/fscrypt_recovery_readme.txt" for system or move this filesystem to another system. "MNT/dir" is now encrypted, unlocked, and ready for use. -ext4 filesystem "MNT" has 2 protectors and 1 policy +ext4 filesystem "MNT" has 2 protectors and 1 policy. +All users can create fscrypt metadata on this filesystem. PROTECTOR LINKED DESCRIPTION desc10 Yes (MNT_ROOT) login protector for fscrypt-test-user @@ -65,7 +68,8 @@ desc11 No custom protector "Recovery passphras POLICY UNLOCKED PROTECTORS desc12 Yes desc10, desc11 -ext4 filesystem "MNT_ROOT" has 1 protector and 0 policies +ext4 filesystem "MNT_ROOT" has 1 protector and 0 policies. +All users can create fscrypt metadata on this filesystem. PROTECTOR LINKED DESCRIPTION desc10 No login protector for fscrypt-test-user @@ -88,7 +92,8 @@ IMPORTANT: See "MNT/dir/fscrypt_recovery_readme.txt" for 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 +ext4 filesystem "MNT" has 2 protectors and 1 policy. +All users can create fscrypt metadata on this filesystem. PROTECTOR LINKED DESCRIPTION desc19 Yes (MNT_ROOT) login protector for fscrypt-test-user @@ -96,7 +101,8 @@ desc20 No custom protector "Recovery passphras POLICY UNLOCKED PROTECTORS desc21 Yes desc19, desc20 -ext4 filesystem "MNT_ROOT" has 1 protector and 0 policies +ext4 filesystem "MNT_ROOT" has 1 protector and 0 policies. +All users can create fscrypt metadata on this filesystem. PROTECTOR LINKED DESCRIPTION desc19 No login protector for fscrypt-test-user @@ -114,14 +120,16 @@ desc20 No custom protector "Recovery passphras Protector is owned by fscrypt-test-user:fscrypt-test-user # Encrypt with login protector with --no-recovery -ext4 filesystem "MNT" has 1 protector and 1 policy +ext4 filesystem "MNT" has 1 protector and 1 policy. +All users can create fscrypt metadata on this filesystem. PROTECTOR LINKED DESCRIPTION desc28 Yes (MNT_ROOT) login protector for fscrypt-test-user POLICY UNLOCKED PROTECTORS desc29 Yes desc28 -ext4 filesystem "MNT_ROOT" has 1 protector and 0 policies +ext4 filesystem "MNT_ROOT" has 1 protector and 0 policies. +All users can create fscrypt metadata on this filesystem. PROTECTOR LINKED DESCRIPTION desc28 No login protector for fscrypt-test-user @@ -145,7 +153,8 @@ Unlocked: Yes Protected with 1 protector: PROTECTOR LINKED DESCRIPTION desc35 No login protector for fscrypt-test-user -ext4 filesystem "MNT_ROOT" has 1 protector and 1 policy +ext4 filesystem "MNT_ROOT" has 1 protector and 1 policy. +All users can create fscrypt metadata on this filesystem. PROTECTOR LINKED DESCRIPTION desc35 No login protector for fscrypt-test-user @@ -159,18 +168,22 @@ desc34 Yes desc35 identified by user, not by name. To fix this, don't specify the --name=PROTECTOR_NAME option. -ext4 filesystem "MNT" has 0 protectors and 0 policies +ext4 filesystem "MNT" has 0 protectors and 0 policies. +All users can create fscrypt metadata on this filesystem. -ext4 filesystem "MNT_ROOT" has 0 protectors and 0 policies +ext4 filesystem "MNT_ROOT" has 0 protectors and 0 policies. +All users can create fscrypt metadata on this filesystem. [ERROR] fscrypt status: file or directory "MNT/dir" is not encrypted # Try to use the wrong login passphrase [ERROR] fscrypt encrypt: incorrect login passphrase -ext4 filesystem "MNT" has 0 protectors and 0 policies +ext4 filesystem "MNT" has 0 protectors and 0 policies. +All users can create fscrypt metadata on this filesystem. -ext4 filesystem "MNT_ROOT" has 0 protectors and 0 policies +ext4 filesystem "MNT_ROOT" has 0 protectors and 0 policies. +All users can create fscrypt metadata on this filesystem. [ERROR] fscrypt status: file or directory "MNT/dir" is not encrypted @@ -183,7 +196,8 @@ IMPORTANT: See "MNT/dir/fscrypt_recovery_readme.txt" for 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 +ext4 filesystem "MNT" has 2 protectors and 1 policy. +All users can create fscrypt metadata on this filesystem. PROTECTOR LINKED DESCRIPTION desc39 No custom protector "Recovery passphrase for dir" diff --git a/cli-tests/t_encrypt_raw_key.out b/cli-tests/t_encrypt_raw_key.out index 1f51dc0..4cfc050 100644 --- a/cli-tests/t_encrypt_raw_key.out +++ b/cli-tests/t_encrypt_raw_key.out @@ -1,6 +1,7 @@ # Encrypt with raw_key protector from file -ext4 filesystem "MNT" has 1 protector and 1 policy +ext4 filesystem "MNT" has 1 protector and 1 policy. +All users can create fscrypt metadata on this filesystem. PROTECTOR LINKED DESCRIPTION desc1 No raw key protector "prot" @@ -18,7 +19,8 @@ PROTECTOR LINKED DESCRIPTION desc1 No raw key protector "prot" # Encrypt with raw_key protector from stdin -ext4 filesystem "MNT" has 1 protector and 1 policy +ext4 filesystem "MNT" has 1 protector and 1 policy. +All users can create fscrypt metadata on this filesystem. PROTECTOR LINKED DESCRIPTION desc6 No raw key protector "prot" @@ -37,21 +39,24 @@ desc6 No raw key protector "prot" # Try to encrypt with raw_key protector from file, using wrong key length [ERROR] fscrypt encrypt: TMPDIR/raw_key: key file must be 32 bytes -ext4 filesystem "MNT" has 0 protectors and 0 policies +ext4 filesystem "MNT" has 0 protectors and 0 policies. +All users can create fscrypt metadata on this filesystem. [ERROR] fscrypt status: file or directory "MNT/dir" is not encrypted # Try to encrypt with raw_key protector from stdin, using wrong key length [ERROR] fscrypt encrypt: unexpected EOF -ext4 filesystem "MNT" has 0 protectors and 0 policies +ext4 filesystem "MNT" has 0 protectors and 0 policies. +All users can create fscrypt metadata on this filesystem. [ERROR] fscrypt status: file or directory "MNT/dir" is not encrypted # Encrypt with raw_key protector from file, unlock from stdin "MNT/dir" is now locked. -ext4 filesystem "MNT" has 1 protector and 1 policy +ext4 filesystem "MNT" has 1 protector and 1 policy. +All users can create fscrypt metadata on this filesystem. PROTECTOR LINKED DESCRIPTION desc11 No raw key protector "prot" diff --git a/cli-tests/t_metadata.out b/cli-tests/t_metadata.out index fba816a..bbcc0f2 100644 --- a/cli-tests/t_metadata.out +++ b/cli-tests/t_metadata.out @@ -1,4 +1,5 @@ -ext4 filesystem "MNT" has 3 protectors and 1 policy +ext4 filesystem "MNT" has 3 protectors and 1 policy. +All users can create fscrypt metadata on this filesystem. PROTECTOR LINKED DESCRIPTION desc1 No custom protector "foo" @@ -7,7 +8,8 @@ desc3 No custom protector "baz" POLICY UNLOCKED PROTECTORS desc4 No desc1, desc2, desc3 -ext4 filesystem "MNT" has 2 protectors and 1 policy +ext4 filesystem "MNT" has 2 protectors and 1 policy. +All users can create fscrypt metadata on this filesystem. PROTECTOR LINKED DESCRIPTION desc1 No custom protector "foo" diff --git a/cli-tests/t_not_supported.sh b/cli-tests/t_not_supported.sh index 9ff90e1..8b52392 100755 --- a/cli-tests/t_not_supported.sh +++ b/cli-tests/t_not_supported.sh @@ -10,7 +10,7 @@ umount "$MNT" mount tmpfs -t tmpfs -o size=128m "$MNT" _print_header "Try to create fscrypt metadata on tmpfs" -_expect_failure "fscrypt setup '$MNT'" +_expect_failure "fscrypt setup --quiet '$MNT'" _print_header "Try to encrypt a directory on tmpfs" mkdir "$MNT/dir" diff --git a/cli-tests/t_setup.out b/cli-tests/t_setup.out index 943a781..6ea03e3 100644 --- a/cli-tests/t_setup.out +++ b/cli-tests/t_setup.out @@ -9,7 +9,8 @@ Skipping creating MNT_ROOT/.fscrypt because it already exists. Defaulting to policy_version 2 because kernel supports it. Customizing passphrase hashing difficulty for this system... Created global config file at "FSCRYPT_CONF". -Metadata directories created at "MNT_ROOT/.fscrypt". +Allow users other than root to create fscrypt metadata on this filesystem? (See +https://github.com/google/fscrypt#setting-up-fscrypt-on-a-filesystem) [y/N] Metadata directories created at "MNT_ROOT/.fscrypt", writable by everyone. # fscrypt setup when fscrypt.conf already exists (cancel) Replace "FSCRYPT_CONF"? [y/N] [ERROR] fscrypt setup: operation canceled @@ -31,7 +32,8 @@ If desired, use --force to automatically run destructive operations. # fscrypt setup --quiet --force when fscrypt.conf already exists # fscrypt setup filesystem -Metadata directories created at "MNT/.fscrypt". +Allow users other than root to create fscrypt metadata on this filesystem? (See +https://github.com/google/fscrypt#setting-up-fscrypt-on-a-filesystem) [y/N] Metadata directories created at "MNT/.fscrypt", writable by everyone. # fscrypt setup filesystem (already set up) [ERROR] fscrypt setup: filesystem MNT is already setup for diff --git a/cli-tests/t_setup.sh b/cli-tests/t_setup.sh index a8a62a3..f7e302d 100755 --- a/cli-tests/t_setup.sh +++ b/cli-tests/t_setup.sh @@ -14,7 +14,7 @@ fscrypt setup --time=1ms _print_header "fscrypt setup creates fscrypt.conf and /.fscrypt" _rm_metadata "$MNT_ROOT" rm -f "$FSCRYPT_CONF" -fscrypt setup --time=1ms +echo y | fscrypt setup --time=1ms [ -e "$MNT_ROOT/.fscrypt" ] _print_header "fscrypt setup when fscrypt.conf already exists (cancel)" @@ -37,7 +37,7 @@ fscrypt setup --quiet --force --time=1ms _print_header "fscrypt setup filesystem" _rm_metadata "$MNT" -fscrypt setup "$MNT" +echo y | fscrypt setup "$MNT" [ -e "$MNT/.fscrypt" ] _print_header "fscrypt setup filesystem (already set up)" diff --git a/cli-tests/t_single_user.out b/cli-tests/t_single_user.out new file mode 100644 index 0000000..e788b3e --- /dev/null +++ b/cli-tests/t_single_user.out @@ -0,0 +1,30 @@ +ext4 filesystem "MNT" has 0 protectors and 0 policies. +Only root can create fscrypt metadata on this filesystem. + +ext4 filesystem "MNT" has 0 protectors and 0 policies. +Only root can create fscrypt metadata on this filesystem. + + +# Encrypt, lock, and unlock as root +"MNT/dir" is now locked. + +# Encrypt as root with user's login protector + +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. + +Protector desc1 no longer protecting policy desc2. +"MNT/dir" is now locked. +Enter login passphrase for fscrypt-test-user: "MNT/dir" is now unlocked and ready for use. + +# Encrypt as user (should fail) +[ERROR] fscrypt encrypt: user lacks permission to create fscrypt metadata on + MNT + +For how to allow users to create fscrypt metadata on a filesystem, refer to +https://github.com/google/fscrypt#setting-up-fscrypt-on-a-filesystem + +# Encrypt as user if they set up filesystem (should succeed) diff --git a/cli-tests/t_single_user.sh b/cli-tests/t_single_user.sh new file mode 100755 index 0000000..c569f20 --- /dev/null +++ b/cli-tests/t_single_user.sh @@ -0,0 +1,55 @@ +#!/bin/bash + +# Test 'fscrypt setup' without --all-users. + +cd "$(dirname "$0")" +. common.sh + +_rm_metadata "$MNT_ROOT" +_rm_metadata "$MNT" +rm "$FSCRYPT_CONF" +fscrypt setup --time=1ms --quiet +fscrypt setup --time=1ms --quiet "$MNT" +fscrypt status "$MNT" +_user_do "fscrypt status \"$MNT\"" + +dir=$MNT/dir + +begin() +{ + _reset_filesystems + mkdir "$dir" + _print_header "$1" +} + +begin "Encrypt, lock, and unlock as root" +echo hunter2 | fscrypt encrypt --quiet --name=dir --skip-unlock "$dir" +echo hunter2 | fscrypt unlock --quiet "$dir" +fscrypt lock "$dir" + +begin "Encrypt as root with user's login protector" +echo TEST_USER_PASS | fscrypt encrypt --quiet --source=pam_passphrase --user="$TEST_USER" "$dir" +# The user should be able to update the policy and protectors created by the +# above command themselves. The easiest way to test this is by updating the +# policy to remove the auto-generated recovery protector. This verifies that +# (a) the policy was made owned by the user, and that (b) policy updates fall +# back to overwrites when the process cannot write to the containing directory. +# (It would be better to test updating the protectors too, but this is the +# easiest test to do here.) +policy=$(fscrypt status "$dir" | awk '/Policy/{print $2}') +recovery_protector=$(_get_protector_descriptor "$MNT" custom 'Recovery passphrase for dir') +_user_do "fscrypt metadata remove-protector-from-policy --force --protector=$MNT:$recovery_protector --policy=$MNT:$policy" +chown "$TEST_USER" "$dir" +_user_do "fscrypt lock $dir" +_user_do "echo TEST_USER_PASS | fscrypt unlock $dir" + +begin "Encrypt as user (should fail)" +chown "$TEST_USER" "$dir" +_user_do_and_expect_failure "echo hunter2 | fscrypt encrypt --quiet --name=dir --skip-unlock \"$dir\"" + +begin "Encrypt as user if they set up filesystem (should succeed)" +_rm_metadata "$MNT" +chown "$TEST_USER" "$MNT" +chown "$TEST_USER" "$dir" +_user_do "fscrypt setup --time=1ms --quiet $MNT" +_user_do "echo hunter2 | fscrypt encrypt --quiet --name=dir3 --skip-unlock \"$dir\"" diff --git a/cli-tests/t_status.out b/cli-tests/t_status.out index 0d478b5..eb425d0 100644 --- a/cli-tests/t_status.out +++ b/cli-tests/t_status.out @@ -4,9 +4,11 @@ ext4 supported Yes ext4 supported Yes # Get status of setup mountpoint -ext4 filesystem "MNT" has 0 protectors and 0 policies +ext4 filesystem "MNT" has 0 protectors and 0 policies. +All users can create fscrypt metadata on this filesystem. -ext4 filesystem "MNT" has 0 protectors and 0 policies +ext4 filesystem "MNT" has 0 protectors and 0 policies. +All users can create fscrypt metadata on this filesystem. # Get status of unencrypted directory on setup mountpoint diff --git a/cli-tests/t_v1_policy.out b/cli-tests/t_v1_policy.out index 9adb00a..1f4f9d7 100644 --- a/cli-tests/t_v1_policy.out +++ b/cli-tests/t_v1_policy.out @@ -120,7 +120,8 @@ Unlocked: Partially (incompletely locked, or unlocked by another user) Protected with 1 protector: PROTECTOR LINKED DESCRIPTION desc2 No custom protector "prot" -ext4 filesystem "MNT" has 1 protector and 1 policy +ext4 filesystem "MNT" has 1 protector and 1 policy. +All users can create fscrypt metadata on this filesystem. PROTECTOR LINKED DESCRIPTION desc2 No custom protector "prot" -- cgit v1.3 From 74e870b7bd1585b4b509da47e0e75db66336e576 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Wed, 23 Feb 2022 12:35:04 -0800 Subject: Strictly validate metadata file ownership by default The metadata validation checks introduced by the previous commits are good, but to reduce the attack surface it would be much better to avoid reading and parsing files owned by other users in the first place. There are some possible use cases for users sharing fscrypt metadata files, but I think that for the vast majority of users it is unneeded and just opens up attack surface. Thus, make fscrypt (and pam_fscrypt) not process policies or protectors owned by other users by default. Specifically, * If fscrypt or pam_fscrypt is running as a non-root user, only policies and protectors owned by the user or by root can be used. * If fscrypt is running as root, any policy or protector can be used. (This is to match user expectations -- starting a sudo session should gain rights, not remove rights.) * If pam_fscrypt is running as root, only policies and protectors owned by root can be used. Note that this only applies when the root user themselves has an fscrypt login protector, which is rare. Add an option 'allow_cross_user_metadata' to /etc/fscrypt.conf which allows restoring the old behavior for anyone who really needs it. --- README.md | 11 +++- actions/context.go | 20 ++++++- actions/policy.go | 8 +-- actions/protector.go | 4 +- cli-tests/t_encrypt.out | 2 +- cli-tests/t_single_user.out | 2 +- cli-tests/t_status.out | 2 +- cli-tests/t_v1_policy.out | 2 +- cmd/fscrypt/status.go | 10 +++- filesystem/filesystem.go | 107 +++++++++++++++++++++++++---------- filesystem/filesystem_test.go | 54 +++++++++++++----- metadata/config_test.go | 3 +- metadata/metadata.pb.go | 126 +++++++++++++++++++++++------------------- metadata/metadata.proto | 1 + pam_fscrypt/run_fscrypt.go | 12 +++- 15 files changed, 245 insertions(+), 119 deletions(-) (limited to 'cli-tests') diff --git a/README.md b/README.md index 26fd084..f1803b4 100644 --- a/README.md +++ b/README.md @@ -320,7 +320,8 @@ that looks like the following: "filenames": "AES_256_CTS", "policy_version": "2" }, - "use_fs_keyring_for_v1_policies": false + "use_fs_keyring_for_v1_policies": false, + "allow_cross_user_metadata": false } ``` @@ -378,6 +379,14 @@ The fields are: kernels, it's better to not use this setting and instead (re-)create your encrypted directories with `"policy_version": "2"`. +* "allow\_cross\_user\_metadata" specifies whether `fscrypt` will allow + protectors and policies from other non-root users to be read, e.g. to be + offered as options by `fscrypt encrypt`. The default value is `false`, since + other users might be untrusted and could create malicious files. This can be + set to `true` to restore the old behavior on systems where `fscrypt` metadata + needs to be shared between multiple users. Note that this option is + independent from the permissions on the metadata files themselves. + ## Setting up `fscrypt` on a filesystem `fscrypt` needs some directories to exist on the filesystem on which encryption diff --git a/actions/context.go b/actions/context.go index 26295ec..1ee0d60 100644 --- a/actions/context.go +++ b/actions/context.go @@ -58,6 +58,12 @@ type Context struct { // the user for whom the keys are claimed in the filesystem keyring when // v2 policies are provisioned. TargetUser *user.User + // TrustedUser is the user for whom policies and protectors are allowed + // to be read. Specifically, if TrustedUser is set, then only + // policies and protectors owned by TrustedUser or by root will be + // allowed to be read. If it's nil, then all policies and protectors + // the process has filesystem-level read access to will be allowed. + TrustedUser *user.User } // NewContextFromPath makes a context for the filesystem containing the @@ -112,6 +118,16 @@ func newContextFromUser(targetUser *user.User) (*Context, error) { return nil, err } + // By default, when running as a non-root user we only read policies and + // protectors owned by the user or root. When running as root, we allow + // reading all policies and protectors. + if !ctx.Config.GetAllowCrossUserMetadata() && !util.IsUserRoot() { + ctx.TrustedUser, err = util.EffectiveUser() + if err != nil { + return nil, err + } + } + log.Printf("creating context for user %q", targetUser.Username) return ctx, nil } @@ -136,7 +152,7 @@ func (ctx *Context) getKeyringOptions() *keyring.Options { // getProtectorOption returns the ProtectorOption for the protector on the // context's mountpoint with the specified descriptor. func (ctx *Context) getProtectorOption(protectorDescriptor string) *ProtectorOption { - mnt, data, err := ctx.Mount.GetProtector(protectorDescriptor) + mnt, data, err := ctx.Mount.GetProtector(protectorDescriptor, ctx.TrustedUser) if err != nil { return &ProtectorOption{ProtectorInfo{}, nil, err} } @@ -155,7 +171,7 @@ func (ctx *Context) ProtectorOptions() ([]*ProtectorOption, error) { if err := ctx.checkContext(); err != nil { return nil, err } - descriptors, err := ctx.Mount.ListProtectors() + descriptors, err := ctx.Mount.ListProtectors(ctx.TrustedUser) if err != nil { return nil, err } diff --git a/actions/policy.go b/actions/policy.go index 7204380..3b8eb30 100644 --- a/actions/policy.go +++ b/actions/policy.go @@ -145,7 +145,7 @@ func PurgeAllPolicies(ctx *Context) error { if err := ctx.checkContext(); err != nil { return err } - policies, err := ctx.Mount.ListPolicies() + policies, err := ctx.Mount.ListPolicies(nil) if err != nil { return err } @@ -225,7 +225,7 @@ func GetPolicy(ctx *Context, descriptor string) (*Policy, error) { if err := ctx.checkContext(); err != nil { return nil, err } - data, err := ctx.Mount.GetPolicy(descriptor) + data, err := ctx.Mount.GetPolicy(descriptor, ctx.TrustedUser) if err != nil { return nil, err } @@ -262,7 +262,7 @@ func GetPolicyFromPath(ctx *Context, path string) (*Policy, error) { descriptor := pathData.KeyDescriptor log.Printf("found policy %s for %q", descriptor, path) - mountData, err := ctx.Mount.GetPolicy(descriptor) + mountData, err := ctx.Mount.GetPolicy(descriptor, ctx.TrustedUser) if err != nil { log.Printf("getting policy metadata: %v", err) if _, ok := err.(*filesystem.ErrPolicyNotFound); ok { @@ -428,7 +428,7 @@ func (policy *Policy) AddProtector(protector *Protector) error { if policy.Context.Mount != protector.Context.Mount { log.Printf("policy on %s\n protector on %s\n", policy.Context.Mount, protector.Context.Mount) isNewLink, err := policy.Context.Mount.AddLinkedProtector( - protector.Descriptor(), protector.Context.Mount) + protector.Descriptor(), protector.Context.Mount, protector.Context.TrustedUser) if err != nil { return err } diff --git a/actions/protector.go b/actions/protector.go index 3278e63..1171c83 100644 --- a/actions/protector.go +++ b/actions/protector.go @@ -199,7 +199,7 @@ func GetProtector(ctx *Context, descriptor string) (*Protector, error) { } protector := &Protector{Context: ctx} - protector.data, err = ctx.Mount.GetRegularProtector(descriptor) + protector.data, err = ctx.Mount.GetRegularProtector(descriptor, ctx.TrustedUser) return protector, err } @@ -218,7 +218,7 @@ func GetProtectorFromOption(ctx *Context, option *ProtectorOption) (*Protector, // Replace the context if this is a linked protector if option.LinkedMount != nil { - ctx = &Context{ctx.Config, option.LinkedMount, ctx.TargetUser} + ctx = &Context{ctx.Config, option.LinkedMount, ctx.TargetUser, ctx.TrustedUser} } return &Protector{Context: ctx, data: option.data}, nil } diff --git a/cli-tests/t_encrypt.out b/cli-tests/t_encrypt.out index b92c9d9..ecdc46b 100644 --- a/cli-tests/t_encrypt.out +++ b/cli-tests/t_encrypt.out @@ -71,7 +71,7 @@ Unlocked: Yes Protected with 1 protector: PROTECTOR LINKED DESCRIPTION desc1 No custom protector "prot" -ext4 filesystem "MNT" has 1 protector and 1 policy. +ext4 filesystem "MNT" has 1 protector and 1 policy (only including ones owned by fscrypt-test-user or root). All users can create fscrypt metadata on this filesystem. PROTECTOR LINKED DESCRIPTION diff --git a/cli-tests/t_single_user.out b/cli-tests/t_single_user.out index e788b3e..d038d52 100644 --- a/cli-tests/t_single_user.out +++ b/cli-tests/t_single_user.out @@ -1,7 +1,7 @@ ext4 filesystem "MNT" has 0 protectors and 0 policies. Only root can create fscrypt metadata on this filesystem. -ext4 filesystem "MNT" has 0 protectors and 0 policies. +ext4 filesystem "MNT" has 0 protectors and 0 policies (only including ones owned by fscrypt-test-user or root). Only root can create fscrypt metadata on this filesystem. diff --git a/cli-tests/t_status.out b/cli-tests/t_status.out index eb425d0..058c62c 100644 --- a/cli-tests/t_status.out +++ b/cli-tests/t_status.out @@ -7,7 +7,7 @@ ext4 supported Yes ext4 filesystem "MNT" has 0 protectors and 0 policies. All users can create fscrypt metadata on this filesystem. -ext4 filesystem "MNT" has 0 protectors and 0 policies. +ext4 filesystem "MNT" has 0 protectors and 0 policies (only including ones owned by fscrypt-test-user or root). All users can create fscrypt metadata on this filesystem. diff --git a/cli-tests/t_v1_policy.out b/cli-tests/t_v1_policy.out index 1f4f9d7..2353527 100644 --- a/cli-tests/t_v1_policy.out +++ b/cli-tests/t_v1_policy.out @@ -120,7 +120,7 @@ Unlocked: Partially (incompletely locked, or unlocked by another user) Protected with 1 protector: PROTECTOR LINKED DESCRIPTION desc2 No custom protector "prot" -ext4 filesystem "MNT" has 1 protector and 1 policy. +ext4 filesystem "MNT" has 1 protector and 1 policy (only including ones owned by fscrypt-test-user or root). All users can create fscrypt metadata on this filesystem. PROTECTOR LINKED DESCRIPTION diff --git a/cmd/fscrypt/status.go b/cmd/fscrypt/status.go index 54c6f1f..ed5a764 100644 --- a/cmd/fscrypt/status.go +++ b/cmd/fscrypt/status.go @@ -160,14 +160,18 @@ func writeFilesystemStatus(w io.Writer, ctx *actions.Context) error { return err } - policyDescriptors, err := ctx.Mount.ListPolicies() + policyDescriptors, err := ctx.Mount.ListPolicies(ctx.TrustedUser) if err != nil { return err } - fmt.Fprintf(w, "%s filesystem %q has %s and %s.\n", ctx.Mount.FilesystemType, + filterDescription := "" + if ctx.TrustedUser != nil { + filterDescription = fmt.Sprintf(" (only including ones owned by %s or root)", ctx.TrustedUser.Username) + } + fmt.Fprintf(w, "%s filesystem %q has %s and %s%s.\n", ctx.Mount.FilesystemType, ctx.Mount.Path, pluralize(len(options), "protector"), - pluralize(len(policyDescriptors), "policy")) + pluralize(len(policyDescriptors), "policy"), filterDescription) if setupMode, user, err := ctx.Mount.GetSetupMode(); err == nil { switch setupMode { case filesystem.WorldWritable: diff --git a/filesystem/filesystem.go b/filesystem/filesystem.go index 1450f0f..6567dd6 100644 --- a/filesystem/filesystem.go +++ b/filesystem/filesystem.go @@ -370,6 +370,20 @@ func (m *Mount) CheckSupport() error { return m.EncryptionSupportError(metadata.CheckSupport(m.Path)) } +func checkOwnership(path string, info os.FileInfo, trustedUser *user.User) bool { + if trustedUser == nil { + return true + } + trustedUID := uint32(util.AtoiOrPanic(trustedUser.Uid)) + actualUID := info.Sys().(*syscall.Stat_t).Uid + if actualUID != 0 && actualUID != trustedUID { + log.Printf("WARNING: %q is owned by uid %d, but expected %d or 0", + path, actualUID, trustedUID) + return false + } + return true +} + // CheckSetup returns an error if all the fscrypt metadata directories do not // exist. Will log any unexpected errors or incorrect permissions. func (m *Mount) CheckSetup() error { @@ -600,6 +614,8 @@ func (m *Mount) addMetadata(path string, md metadata.Metadata, owner *user.User) // 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). +// - If trustedUser is non-nil, then the file must be owned by the given user +// or by root. // // 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 @@ -611,7 +627,7 @@ func (m *Mount) addMetadata(path string, md metadata.Metadata, owner *user.User) // 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) { +func readMetadataFileSafe(path string, trustedUser *user.User) ([]byte, int64, error) { file, err := os.OpenFile(path, os.O_RDONLY|unix.O_NOFOLLOW|unix.O_NONBLOCK, 0) if err != nil { return nil, -1, err @@ -625,6 +641,9 @@ func readMetadataFileSafe(path string) ([]byte, int64, error) { if !info.Mode().IsRegular() { return nil, -1, &ErrCorruptMetadata{path, errors.New("not a regular file")} } + if !checkOwnership(path, info, trustedUser) { + return nil, -1, &ErrCorruptMetadata{path, errors.New("metadata file belongs to another user")} + } // 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. @@ -645,8 +664,8 @@ func readMetadataFileSafe(path string) ([]byte, int64, error) { // 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) (int64, error) { - data, owner, err := readMetadataFileSafe(path) +func (m *Mount) getMetadata(path string, trustedUser *user.User, md metadata.Metadata) (int64, error) { + data, owner, err := readMetadataFileSafe(path, trustedUser) if err != nil { log.Printf("could not read metadata from %q: %v", path, err) return -1, err @@ -704,19 +723,19 @@ func (m *Mount) AddProtector(data *metadata.ProtectorData) error { // AddLinkedProtector adds a link in this filesystem to the protector metadata // in the dest filesystem, if one doesn't already exist. On success, the return // value is a nil error and a bool that is true iff the link is newly created. -func (m *Mount) AddLinkedProtector(descriptor string, dest *Mount) (bool, error) { +func (m *Mount) AddLinkedProtector(descriptor string, dest *Mount, trustedUser *user.User) (bool, error) { if err := m.CheckSetup(); err != nil { return false, err } // Check that the link is good (descriptor exists, filesystem has UUID). - if _, err := dest.GetRegularProtector(descriptor); err != nil { + if _, err := dest.GetRegularProtector(descriptor, trustedUser); err != nil { return false, err } linkPath := m.linkedProtectorPath(descriptor) // Check whether the link already exists. - existingLink, _, err := readMetadataFileSafe(linkPath) + existingLink, _, err := readMetadataFileSafe(linkPath, trustedUser) if err == nil { existingLinkedMnt, err := getMountFromLink(string(existingLink)) if err != nil { @@ -742,13 +761,13 @@ func (m *Mount) AddLinkedProtector(descriptor string, dest *Mount) (bool, error) // GetRegularProtector looks up the protector metadata by descriptor. This will // fail with ErrProtectorNotFound if the descriptor is a linked protector. -func (m *Mount) GetRegularProtector(descriptor string) (*metadata.ProtectorData, error) { +func (m *Mount) GetRegularProtector(descriptor string, trustedUser *user.User) (*metadata.ProtectorData, error) { if err := m.CheckSetup(); err != nil { return nil, err } data := new(metadata.ProtectorData) path := m.protectorPath(descriptor) - owner, err := m.getMetadata(path, data) + owner, err := m.getMetadata(path, trustedUser, data) if os.IsNotExist(err) { err = &ErrProtectorNotFound{descriptor, m} } @@ -772,17 +791,17 @@ func (m *Mount) GetRegularProtector(descriptor string) (*metadata.ProtectorData, // GetProtector returns the Mount of the filesystem containing the information // and that protector's data. If the descriptor is a regular (not linked) // protector, the mount will return itself. -func (m *Mount) GetProtector(descriptor string) (*Mount, *metadata.ProtectorData, error) { +func (m *Mount) GetProtector(descriptor string, trustedUser *user.User) (*Mount, *metadata.ProtectorData, error) { if err := m.CheckSetup(); err != nil { return nil, nil, err } // Get the link data from the link file path := m.linkedProtectorPath(descriptor) - link, _, err := readMetadataFileSafe(path) + link, _, err := readMetadataFileSafe(path, trustedUser) if err != nil { // If the link doesn't exist, try for a regular protector. if os.IsNotExist(err) { - data, err := m.GetRegularProtector(descriptor) + data, err := m.GetRegularProtector(descriptor, trustedUser) return m, data, err } return nil, nil, err @@ -792,7 +811,7 @@ func (m *Mount) GetProtector(descriptor string) (*Mount, *metadata.ProtectorData if err != nil { return nil, nil, errors.Wrap(err, path) } - data, err := linkedMnt.GetRegularProtector(descriptor) + data, err := linkedMnt.GetRegularProtector(descriptor, trustedUser) if err != nil { return nil, nil, &ErrFollowLink{string(link), err} } @@ -818,12 +837,10 @@ func (m *Mount) RemoveProtector(descriptor string) error { } // ListProtectors lists the descriptors of all protectors on this filesystem. -// This does not include linked protectors. -func (m *Mount) ListProtectors() ([]string, error) { - if err := m.CheckSetup(); err != nil { - return nil, err - } - return m.listDirectory(m.ProtectorDir()) +// This does not include linked protectors. If trustedUser is non-nil, then +// the protectors are restricted to those owned by the given user or by root. +func (m *Mount) ListProtectors(trustedUser *user.User) ([]string, error) { + return m.listMetadata(m.ProtectorDir(), "protectors", trustedUser) } // AddPolicy adds the policy metadata to the filesystem storage. @@ -836,12 +853,12 @@ func (m *Mount) AddPolicy(data *metadata.PolicyData) error { } // GetPolicy looks up the policy metadata by descriptor. -func (m *Mount) GetPolicy(descriptor string) (*metadata.PolicyData, error) { +func (m *Mount) GetPolicy(descriptor string, trustedUser *user.User) (*metadata.PolicyData, error) { if err := m.CheckSetup(); err != nil { return nil, err } data := new(metadata.PolicyData) - _, err := m.getMetadata(m.PolicyPath(descriptor), data) + _, err := m.getMetadata(m.PolicyPath(descriptor), trustedUser, data) if os.IsNotExist(err) { err = &ErrPolicyNotFound{descriptor, m} } @@ -860,12 +877,11 @@ func (m *Mount) RemovePolicy(descriptor string) error { return err } -// ListPolicies lists the descriptors of all policies on this filesystem. -func (m *Mount) ListPolicies() ([]string, error) { - if err := m.CheckSetup(); err != nil { - return nil, err - } - return m.listDirectory(m.PolicyDir()) +// ListPolicies lists the descriptors of all policies on this filesystem. If +// trustedUser is non-nil, then the policies are restricted to those owned by +// the given user or by root. +func (m *Mount) ListPolicies(trustedUser *user.User) ([]string, error) { + return m.listMetadata(m.PolicyDir(), "policies", trustedUser) } type namesAndTimes struct { @@ -902,7 +918,6 @@ func sortFileListByLastMtime(directoryPath string, names []string) error { // listDirectory returns a list of descriptors for a metadata directory, // including files which are links to other filesystem's metadata. func (m *Mount) listDirectory(directoryPath string) ([]string, error) { - log.Printf("listing descriptors in %q", directoryPath) dir, err := os.Open(directoryPath) if err != nil { return nil, err @@ -925,7 +940,41 @@ func (m *Mount) listDirectory(directoryPath string) ([]string, error) { // Be sure to include links as well descriptors = append(descriptors, strings.TrimSuffix(name, linkFileExtension)) } - - log.Printf("found %d descriptor(s)", len(descriptors)) return descriptors, nil } + +func (m *Mount) listMetadata(dirPath string, metadataType string, owner *user.User) ([]string, error) { + log.Printf("listing %s in %q", metadataType, dirPath) + if err := m.CheckSetup(); err != nil { + return nil, err + } + names, err := m.listDirectory(dirPath) + if err != nil { + return nil, err + } + filesIgnoredDescription := "" + if owner != nil { + filteredNames := make([]string, 0, len(names)) + uid := uint32(util.AtoiOrPanic(owner.Uid)) + for _, name := range names { + info, err := os.Lstat(filepath.Join(dirPath, name)) + if err != nil { + continue + } + fileUID := info.Sys().(*syscall.Stat_t).Uid + if fileUID != uid && fileUID != 0 { + continue + } + filteredNames = append(filteredNames, name) + } + numIgnored := len(names) - len(filteredNames) + if numIgnored != 0 { + filesIgnoredDescription = + fmt.Sprintf(" (ignored %d %s not owned by %s or root)", + numIgnored, metadataType, owner.Username) + } + names = filteredNames + } + log.Printf("found %d %s%s", len(names), metadataType, filesIgnoredDescription) + return names, nil +} diff --git a/filesystem/filesystem_test.go b/filesystem/filesystem_test.go index 365c5cb..d4ef826 100644 --- a/filesystem/filesystem_test.go +++ b/filesystem/filesystem_test.go @@ -23,6 +23,7 @@ import ( "io/ioutil" "log" "os" + "os/user" "path/filepath" "syscall" "testing" @@ -323,7 +324,7 @@ func TestSetPolicy(t *testing.T) { t.Fatal(err) } - realPolicy, err := mnt.GetPolicy(policy.KeyDescriptor) + realPolicy, err := mnt.GetPolicy(policy.KeyDescriptor, nil) if err != nil { t.Fatal(err) } @@ -347,7 +348,7 @@ func TestSetProtector(t *testing.T) { t.Fatal(err) } - realProtector, err := mnt.GetRegularProtector(protector.ProtectorDescriptor) + realProtector, err := mnt.GetRegularProtector(protector.ProtectorDescriptor, nil) if err != nil { t.Fatal(err) } @@ -374,7 +375,7 @@ func TestSpoofedLoginProtector(t *testing.T) { if err = mnt.AddProtector(protector); err != nil { t.Fatal(err) } - _, err = mnt.GetRegularProtector(protector.ProtectorDescriptor) + _, err = mnt.GetRegularProtector(protector.ProtectorDescriptor, nil) if err != nil { t.Fatal(err) } @@ -389,7 +390,7 @@ func TestSpoofedLoginProtector(t *testing.T) { if err = mnt.AddProtector(protector); err != nil { t.Fatal(err) } - _, err = mnt.GetRegularProtector(protector.ProtectorDescriptor) + _, err = mnt.GetRegularProtector(protector.ProtectorDescriptor, nil) if myUID == 0 { if err != nil { t.Fatal(err) @@ -439,13 +440,13 @@ func TestLinkedProtector(t *testing.T) { // Add the link to the second filesystem var isNewLink bool - if isNewLink, err = fakeMnt.AddLinkedProtector(protector.ProtectorDescriptor, realMnt); err != nil { + if isNewLink, err = fakeMnt.AddLinkedProtector(protector.ProtectorDescriptor, realMnt, nil); err != nil { t.Fatal(err) } if !isNewLink { t.Fatal("Link was not new") } - if isNewLink, err = fakeMnt.AddLinkedProtector(protector.ProtectorDescriptor, realMnt); err != nil { + if isNewLink, err = fakeMnt.AddLinkedProtector(protector.ProtectorDescriptor, realMnt, nil); err != nil { t.Fatal(err) } if isNewLink { @@ -453,12 +454,12 @@ func TestLinkedProtector(t *testing.T) { } // Get the protector though the second system - _, err = fakeMnt.GetRegularProtector(protector.ProtectorDescriptor) + _, err = fakeMnt.GetRegularProtector(protector.ProtectorDescriptor, nil) if _, ok := err.(*ErrProtectorNotFound); !ok { t.Fatal(err) } - retMnt, retProtector, err := fakeMnt.GetProtector(protector.ProtectorDescriptor) + retMnt, retProtector, err := fakeMnt.GetProtector(protector.ProtectorDescriptor, nil) if err != nil { t.Fatal(err) } @@ -480,6 +481,11 @@ func createFile(path string, size int64) error { // Tests the readMetadataFileSafe() function. func TestReadMetadataFileSafe(t *testing.T) { + currentUser, err := util.EffectiveUser() + otherUser := &user.User{Uid: "-1"} + if err != nil { + t.Fatal(err) + } tempDir, err := ioutil.TempDir("", "fscrypt") if err != nil { t.Fatal(err) @@ -491,17 +497,39 @@ func TestReadMetadataFileSafe(t *testing.T) { if err = createFile(filePath, 1000); err != nil { t.Fatal(err) } - _, owner, err := readMetadataFileSafe(filePath) + _, owner, err := readMetadataFileSafe(filePath, nil) if err != nil { t.Fatal("failed to read file") } if owner != int64(os.Geteuid()) { t.Fatal("got wrong owner") } + // Also try it with the trustedUser argument set to the current user. + if _, _, err = readMetadataFileSafe(filePath, currentUser); err != nil { + t.Fatal("failed to read file") + } + os.Remove(filePath) + + // File owned by another user. We might not have permission to actually + // change the file's ownership, so we simulate this by passing in a bad + // value for the trustedUser argument. + if err = createFile(filePath, 1000); err != nil { + t.Fatal(err) + } + _, _, err = readMetadataFileSafe(filePath, otherUser) + if util.IsUserRoot() { + if err != nil { + t.Fatal("root-owned file didn't pass owner validation") + } + } else { + if err == nil { + t.Fatal("unexpectedly could read file owned by another user") + } + } os.Remove(filePath) // Nonexistent file - _, _, err = readMetadataFileSafe(filePath) + _, _, err = readMetadataFileSafe(filePath, nil) if !os.IsNotExist(err) { t.Fatal("trying to read nonexistent file didn't fail with expected error") } @@ -510,7 +538,7 @@ func TestReadMetadataFileSafe(t *testing.T) { if err = os.Symlink("target", filePath); err != nil { t.Fatal(err) } - _, _, err = readMetadataFileSafe(filePath) + _, _, err = readMetadataFileSafe(filePath, nil) if err.(*os.PathError).Err != syscall.ELOOP { t.Fatal("trying to read symlink didn't fail with ELOOP") } @@ -520,7 +548,7 @@ func TestReadMetadataFileSafe(t *testing.T) { if err = unix.Mkfifo(filePath, 0600); err != nil { t.Fatal(err) } - _, _, err = readMetadataFileSafe(filePath) + _, _, err = readMetadataFileSafe(filePath, nil) if _, ok := err.(*ErrCorruptMetadata); !ok { t.Fatal("trying to read FIFO didn't fail with expected error") } @@ -530,7 +558,7 @@ func TestReadMetadataFileSafe(t *testing.T) { if err = createFile(filePath, 1000000); err != nil { t.Fatal(err) } - _, _, err = readMetadataFileSafe(filePath) + _, _, err = readMetadataFileSafe(filePath, nil) if _, ok := err.(*ErrCorruptMetadata); !ok { t.Fatal("trying to read very large file didn't fail with expected error") } diff --git a/metadata/config_test.go b/metadata/config_test.go index 52f83f2..2874bb8 100644 --- a/metadata/config_test.go +++ b/metadata/config_test.go @@ -49,7 +49,8 @@ var testConfigString = `{ "filenames": "AES_256_CTS", "policy_version": "1" }, - "use_fs_keyring_for_v1_policies": false + "use_fs_keyring_for_v1_policies": false, + "allow_cross_user_metadata": false } ` diff --git a/metadata/metadata.pb.go b/metadata/metadata.pb.go index a2148ce..6709804 100644 --- a/metadata/metadata.pb.go +++ b/metadata/metadata.pb.go @@ -45,7 +45,7 @@ func (x SourceType) String() string { return proto.EnumName(SourceType_name, int32(x)) } func (SourceType) EnumDescriptor() ([]byte, []int) { - return fileDescriptor_metadata_20fa0d9b7a38c428, []int{0} + return fileDescriptor_metadata_31965d2849cb292a, []int{0} } // Type of encryption; should match declarations of unix.FSCRYPT_MODE @@ -87,7 +87,7 @@ func (x EncryptionOptions_Mode) String() string { return proto.EnumName(EncryptionOptions_Mode_name, int32(x)) } func (EncryptionOptions_Mode) EnumDescriptor() ([]byte, []int) { - return fileDescriptor_metadata_20fa0d9b7a38c428, []int{3, 0} + return fileDescriptor_metadata_31965d2849cb292a, []int{3, 0} } // Cost parameters to be used in our hashing functions. @@ -104,7 +104,7 @@ func (m *HashingCosts) Reset() { *m = HashingCosts{} } func (m *HashingCosts) String() string { return proto.CompactTextString(m) } func (*HashingCosts) ProtoMessage() {} func (*HashingCosts) Descriptor() ([]byte, []int) { - return fileDescriptor_metadata_20fa0d9b7a38c428, []int{0} + return fileDescriptor_metadata_31965d2849cb292a, []int{0} } func (m *HashingCosts) XXX_Unmarshal(b []byte) error { return xxx_messageInfo_HashingCosts.Unmarshal(m, b) @@ -159,7 +159,7 @@ func (m *WrappedKeyData) Reset() { *m = WrappedKeyData{} } func (m *WrappedKeyData) String() string { return proto.CompactTextString(m) } func (*WrappedKeyData) ProtoMessage() {} func (*WrappedKeyData) Descriptor() ([]byte, []int) { - return fileDescriptor_metadata_20fa0d9b7a38c428, []int{1} + return fileDescriptor_metadata_31965d2849cb292a, []int{1} } func (m *WrappedKeyData) XXX_Unmarshal(b []byte) error { return xxx_messageInfo_WrappedKeyData.Unmarshal(m, b) @@ -219,7 +219,7 @@ func (m *ProtectorData) Reset() { *m = ProtectorData{} } func (m *ProtectorData) String() string { return proto.CompactTextString(m) } func (*ProtectorData) ProtoMessage() {} func (*ProtectorData) Descriptor() ([]byte, []int) { - return fileDescriptor_metadata_20fa0d9b7a38c428, []int{2} + return fileDescriptor_metadata_31965d2849cb292a, []int{2} } func (m *ProtectorData) XXX_Unmarshal(b []byte) error { return xxx_messageInfo_ProtectorData.Unmarshal(m, b) @@ -303,7 +303,7 @@ func (m *EncryptionOptions) Reset() { *m = EncryptionOptions{} } func (m *EncryptionOptions) String() string { return proto.CompactTextString(m) } func (*EncryptionOptions) ProtoMessage() {} func (*EncryptionOptions) Descriptor() ([]byte, []int) { - return fileDescriptor_metadata_20fa0d9b7a38c428, []int{3} + return fileDescriptor_metadata_31965d2849cb292a, []int{3} } func (m *EncryptionOptions) XXX_Unmarshal(b []byte) error { return xxx_messageInfo_EncryptionOptions.Unmarshal(m, b) @@ -363,7 +363,7 @@ func (m *WrappedPolicyKey) Reset() { *m = WrappedPolicyKey{} } func (m *WrappedPolicyKey) String() string { return proto.CompactTextString(m) } func (*WrappedPolicyKey) ProtoMessage() {} func (*WrappedPolicyKey) Descriptor() ([]byte, []int) { - return fileDescriptor_metadata_20fa0d9b7a38c428, []int{4} + return fileDescriptor_metadata_31965d2849cb292a, []int{4} } func (m *WrappedPolicyKey) XXX_Unmarshal(b []byte) error { return xxx_messageInfo_WrappedPolicyKey.Unmarshal(m, b) @@ -411,7 +411,7 @@ func (m *PolicyData) Reset() { *m = PolicyData{} } func (m *PolicyData) String() string { return proto.CompactTextString(m) } func (*PolicyData) ProtoMessage() {} func (*PolicyData) Descriptor() ([]byte, []int) { - return fileDescriptor_metadata_20fa0d9b7a38c428, []int{5} + return fileDescriptor_metadata_31965d2849cb292a, []int{5} } func (m *PolicyData) XXX_Unmarshal(b []byte) error { return xxx_messageInfo_PolicyData.Unmarshal(m, b) @@ -458,6 +458,7 @@ type Config struct { HashCosts *HashingCosts `protobuf:"bytes,2,opt,name=hash_costs,json=hashCosts,proto3" json:"hash_costs,omitempty"` Options *EncryptionOptions `protobuf:"bytes,4,opt,name=options,proto3" json:"options,omitempty"` UseFsKeyringForV1Policies bool `protobuf:"varint,5,opt,name=use_fs_keyring_for_v1_policies,json=useFsKeyringForV1Policies,proto3" json:"use_fs_keyring_for_v1_policies,omitempty"` + AllowCrossUserMetadata bool `protobuf:"varint,6,opt,name=allow_cross_user_metadata,json=allowCrossUserMetadata,proto3" json:"allow_cross_user_metadata,omitempty"` XXX_NoUnkeyedLiteral struct{} `json:"-"` XXX_unrecognized []byte `json:"-"` XXX_sizecache int32 `json:"-"` @@ -467,7 +468,7 @@ func (m *Config) Reset() { *m = Config{} } func (m *Config) String() string { return proto.CompactTextString(m) } func (*Config) ProtoMessage() {} func (*Config) Descriptor() ([]byte, []int) { - return fileDescriptor_metadata_20fa0d9b7a38c428, []int{6} + return fileDescriptor_metadata_31965d2849cb292a, []int{6} } func (m *Config) XXX_Unmarshal(b []byte) error { return xxx_messageInfo_Config.Unmarshal(m, b) @@ -515,6 +516,13 @@ func (m *Config) GetUseFsKeyringForV1Policies() bool { return false } +func (m *Config) GetAllowCrossUserMetadata() bool { + if m != nil { + return m.AllowCrossUserMetadata + } + return false +} + func init() { proto.RegisterType((*HashingCosts)(nil), "metadata.HashingCosts") proto.RegisterType((*WrappedKeyData)(nil), "metadata.WrappedKeyData") @@ -527,53 +535,55 @@ func init() { proto.RegisterEnum("metadata.EncryptionOptions_Mode", EncryptionOptions_Mode_name, EncryptionOptions_Mode_value) } -func init() { proto.RegisterFile("metadata/metadata.proto", fileDescriptor_metadata_20fa0d9b7a38c428) } - -var fileDescriptor_metadata_20fa0d9b7a38c428 = []byte{ - // 716 bytes of a gzipped FileDescriptorProto - 0x1f, 0x8b, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0xff, 0x9c, 0x54, 0xdd, 0x6a, 0xdb, 0x48, - 0x14, 0x5e, 0x49, 0x8e, 0x7f, 0x8e, 0x7f, 0x56, 0x99, 0x64, 0xb3, 0xda, 0x5d, 0x58, 0x8c, 0x97, - 0x40, 0x58, 0x42, 0x16, 0x7b, 0x49, 0x69, 0xa1, 0x14, 0x52, 0x27, 0x69, 0x93, 0x10, 0x9a, 0x8e, - 0x8d, 0xdb, 0x42, 0x41, 0x4c, 0xa4, 0xb1, 0x3d, 0x58, 0xd2, 0x88, 0x99, 0x71, 0x8c, 0xee, 0x7a, - 0xd7, 0x07, 0xe8, 0xbb, 0xf4, 0x69, 0xfa, 0x28, 0xbd, 0x28, 0x1a, 0xc9, 0x7f, 0x09, 0x84, 0xa4, - 0x37, 0xe2, 0x9c, 0x6f, 0xce, 0xef, 0x77, 0xce, 0x11, 0xfc, 0x1e, 0x52, 0x45, 0x7c, 0xa2, 0xc8, - 0x7f, 0x73, 0xe1, 0x20, 0x16, 0x5c, 0x71, 0x54, 0x9e, 0xeb, 0xad, 0x8f, 0x50, 0x7b, 0x4d, 0xe4, - 0x98, 0x45, 0xa3, 0x2e, 0x97, 0x4a, 0x22, 0x04, 0x05, 0xc5, 0x42, 0xea, 0x98, 0x4d, 0x63, 0xcf, - 0xc2, 0x5a, 0x46, 0x3b, 0x50, 0x0c, 0x69, 0xc8, 0x45, 0xe2, 0x58, 0x1a, 0xcd, 0x35, 0xd4, 0x84, - 0x6a, 0x4c, 0x04, 0x09, 0x02, 0x1a, 0x30, 0x19, 0x3a, 0x05, 0xfd, 0xb8, 0x0a, 0xb5, 0x3e, 0x40, - 0xe3, 0x9d, 0x20, 0x71, 0x4c, 0xfd, 0x0b, 0x9a, 0x1c, 0x13, 0x45, 0x50, 0x03, 0xcc, 0xb3, 0x81, - 0x63, 0x34, 0x8d, 0xbd, 0x1a, 0x36, 0xcf, 0x06, 0xe8, 0x1f, 0xa8, 0xd3, 0xc8, 0x13, 0x49, 0xac, - 0xa8, 0xef, 0x4e, 0x68, 0xa2, 0x13, 0xd7, 0x70, 0x6d, 0x01, 0x5e, 0xd0, 0x24, 0x2d, 0x6a, 0x1c, - 0x12, 0x4f, 0xa7, 0xaf, 0x61, 0x2d, 0xb7, 0xbe, 0x98, 0x50, 0xbf, 0x12, 0x5c, 0x51, 0x4f, 0x71, - 0xa1, 0x43, 0xb7, 0x61, 0x3b, 0x9e, 0x03, 0xae, 0x4f, 0xa5, 0x27, 0x58, 0xac, 0xb8, 0xd0, 0xc9, - 0x2a, 0x78, 0x6b, 0xf1, 0x76, 0xbc, 0x78, 0x42, 0xfb, 0x50, 0x94, 0x7c, 0x2a, 0xbc, 0xac, 0xdf, - 0x46, 0x67, 0xfb, 0x60, 0x41, 0x54, 0x4f, 0xe3, 0xfd, 0x24, 0xa6, 0x38, 0xb7, 0x49, 0xcb, 0x88, - 0x48, 0x48, 0x75, 0x19, 0x15, 0xac, 0x65, 0xb4, 0x0f, 0x1b, 0x5e, 0x4a, 0x9c, 0xee, 0xbe, 0xda, - 0xd9, 0x59, 0x06, 0x58, 0xa5, 0x15, 0x67, 0x46, 0x69, 0x04, 0x49, 0x02, 0xe5, 0x6c, 0x64, 0x8d, - 0xa4, 0x32, 0xb2, 0xc1, 0x9a, 0x32, 0xdf, 0x29, 0x6a, 0xf6, 0x52, 0x11, 0x3d, 0x83, 0xea, 0x2c, - 0x63, 0x4d, 0x33, 0x52, 0xd2, 0x91, 0x9d, 0x65, 0xe4, 0x75, 0x4a, 0x31, 0xcc, 0x16, 0x7a, 0xeb, - 0x9b, 0x09, 0x9b, 0x27, 0x19, 0x75, 0x8c, 0x47, 0x6f, 0xf4, 0x57, 0x22, 0x07, 0x4a, 0x31, 0xf1, - 0x7d, 0x16, 0x8d, 0x34, 0x19, 0x16, 0x9e, 0xab, 0xe8, 0x39, 0x94, 0x3d, 0x1e, 0x29, 0x1a, 0x29, - 0x99, 0x53, 0xd0, 0x5c, 0xe6, 0xb9, 0x13, 0xe8, 0xe0, 0x92, 0xfb, 0x14, 0x2f, 0x3c, 0xd0, 0x0b, - 0xa8, 0x0c, 0x59, 0x40, 0x53, 0x22, 0xa4, 0x66, 0xe5, 0x21, 0xee, 0x4b, 0x17, 0xb4, 0x0b, 0x8d, - 0x98, 0x07, 0xcc, 0x4b, 0xdc, 0x1b, 0x2a, 0x24, 0xe3, 0x51, 0xbe, 0x43, 0xf5, 0x0c, 0x1d, 0x64, - 0x60, 0xeb, 0xb3, 0x01, 0x85, 0xd4, 0x15, 0x55, 0xa1, 0xe4, 0xd3, 0x21, 0x99, 0x06, 0xca, 0xfe, - 0x05, 0xfd, 0x0a, 0xd5, 0xa3, 0x93, 0x9e, 0xdb, 0x39, 0x7c, 0xe2, 0xbe, 0xef, 0xf7, 0x6c, 0x63, - 0x15, 0x78, 0xd5, 0xbd, 0xb4, 0xcd, 0x55, 0xa0, 0xfb, 0xb2, 0x6b, 0x5b, 0x6b, 0x40, 0xbf, 0x67, - 0x17, 0xe6, 0x40, 0xbb, 0xf3, 0x54, 0x5b, 0x6c, 0xac, 0x01, 0xfd, 0x9e, 0x5d, 0x44, 0x35, 0x28, - 0x1f, 0xf9, 0x8c, 0x44, 0x6a, 0x1a, 0xda, 0x95, 0xd6, 0x27, 0x03, 0xec, 0x9c, 0xfd, 0x2b, 0x5d, - 0x62, 0xba, 0x9d, 0x3f, 0xb1, 0x77, 0xb7, 0x26, 0x6c, 0x3e, 0x62, 0xc2, 0x5f, 0x0d, 0x80, 0x2c, - 0xb7, 0x5e, 0xfa, 0x5d, 0x68, 0x4c, 0x68, 0x72, 0x37, 0x6d, 0x7d, 0x42, 0x93, 0x95, 0x84, 0x87, - 0x50, 0xe2, 0xd9, 0x10, 0xf2, 0x64, 0x7f, 0xdd, 0x33, 0x27, 0x3c, 0xb7, 0x45, 0xe7, 0xb0, 0x35, - 0xaf, 0x33, 0x1f, 0xd4, 0x84, 0x26, 0xe9, 0xa8, 0xad, 0xbd, 0x6a, 0xe7, 0xcf, 0x3b, 0xf5, 0x2e, - 0x38, 0xc1, 0x9b, 0xb3, 0x5b, 0x88, 0x6c, 0x7d, 0x37, 0xa0, 0xd8, 0xe5, 0xd1, 0x90, 0x8d, 0x56, - 0xce, 0xce, 0x78, 0xc0, 0xd9, 0x1d, 0x02, 0x8c, 0x89, 0x1c, 0xbb, 0xd9, 0x9d, 0x99, 0xf7, 0xde, - 0x59, 0x25, 0xb5, 0xcc, 0xfe, 0x64, 0x2b, 0x2d, 0x17, 0x1e, 0xd1, 0xf2, 0x11, 0xfc, 0x3d, 0x95, - 0xd4, 0x1d, 0xca, 0xb4, 0x55, 0xc1, 0xa2, 0x91, 0x3b, 0xe4, 0xc2, 0xbd, 0x69, 0x67, 0x04, 0x30, - 0x2a, 0xf5, 0xf1, 0x96, 0xf1, 0x1f, 0x53, 0x49, 0x4f, 0xe5, 0x45, 0x66, 0x73, 0xca, 0xc5, 0xa0, - 0x7d, 0x95, 0x1b, 0x9c, 0x17, 0xca, 0x96, 0x5d, 0xc0, 0x75, 0x8f, 0x87, 0x31, 0x51, 0xec, 0x9a, - 0x05, 0x4c, 0x25, 0xff, 0xbe, 0x05, 0x58, 0xf6, 0xb6, 0xbe, 0xc9, 0x08, 0x1a, 0x31, 0x09, 0xdd, - 0x98, 0x48, 0x19, 0x8f, 0x05, 0x91, 0xd4, 0x36, 0xd0, 0x6f, 0xb0, 0xe9, 0x4d, 0xa5, 0xe2, 0x6b, - 0xb0, 0x99, 0xfa, 0x09, 0x32, 0x4b, 0x4b, 0xb3, 0xad, 0xeb, 0xa2, 0xfe, 0x99, 0xff, 0xff, 0x23, - 0x00, 0x00, 0xff, 0xff, 0x3d, 0x33, 0x9f, 0x0d, 0xe7, 0x05, 0x00, 0x00, +func init() { proto.RegisterFile("metadata/metadata.proto", fileDescriptor_metadata_31965d2849cb292a) } + +var fileDescriptor_metadata_31965d2849cb292a = []byte{ + // 748 bytes of a gzipped FileDescriptorProto + 0x1f, 0x8b, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, 0x02, 0xff, 0x9c, 0x55, 0xdb, 0x6a, 0xf3, 0x46, + 0x10, 0xae, 0x24, 0xc7, 0x87, 0xf1, 0xa1, 0xca, 0xfe, 0x69, 0xaa, 0xb4, 0x50, 0x8c, 0x4b, 0x20, + 0x94, 0x90, 0x62, 0x97, 0x94, 0x06, 0x4a, 0x21, 0x75, 0x92, 0x36, 0x09, 0xa1, 0xe9, 0xda, 0x75, + 0x5b, 0x28, 0x88, 0x8d, 0xb4, 0xb6, 0x17, 0x4b, 0x5a, 0xb1, 0xbb, 0x8a, 0xd1, 0x5d, 0xef, 0xfa, + 0x00, 0x7d, 0x97, 0xf6, 0x65, 0xfa, 0x30, 0x45, 0x2b, 0xc9, 0x87, 0x04, 0x42, 0xf2, 0xdf, 0x98, + 0xd9, 0x6f, 0x67, 0xe6, 0x9b, 0xf9, 0x66, 0xc7, 0x82, 0x8f, 0x43, 0xaa, 0x88, 0x4f, 0x14, 0xf9, + 0xb2, 0x34, 0x4e, 0x62, 0xc1, 0x15, 0x47, 0xf5, 0xf2, 0xdc, 0xfb, 0x03, 0x5a, 0x3f, 0x12, 0x39, + 0x67, 0xd1, 0x6c, 0xc8, 0xa5, 0x92, 0x08, 0x41, 0x45, 0xb1, 0x90, 0x3a, 0x66, 0xd7, 0x38, 0xb2, + 0xb0, 0xb6, 0xd1, 0x3e, 0x54, 0x43, 0x1a, 0x72, 0x91, 0x3a, 0x96, 0x46, 0x8b, 0x13, 0xea, 0x42, + 0x33, 0x26, 0x82, 0x04, 0x01, 0x0d, 0x98, 0x0c, 0x9d, 0x8a, 0xbe, 0xdc, 0x84, 0x7a, 0xbf, 0x43, + 0xe7, 0x57, 0x41, 0xe2, 0x98, 0xfa, 0xb7, 0x34, 0xbd, 0x20, 0x8a, 0xa0, 0x0e, 0x98, 0xd7, 0x13, + 0xc7, 0xe8, 0x1a, 0x47, 0x2d, 0x6c, 0x5e, 0x4f, 0xd0, 0xe7, 0xd0, 0xa6, 0x91, 0x27, 0xd2, 0x58, + 0x51, 0xdf, 0x5d, 0xd0, 0x54, 0x13, 0xb7, 0x70, 0x6b, 0x05, 0xde, 0xd2, 0x34, 0x2b, 0x6a, 0x1e, + 0x12, 0x4f, 0xd3, 0xb7, 0xb0, 0xb6, 0x7b, 0x7f, 0x9b, 0xd0, 0xbe, 0x17, 0x5c, 0x51, 0x4f, 0x71, + 0xa1, 0x53, 0xf7, 0x61, 0x2f, 0x2e, 0x01, 0xd7, 0xa7, 0xd2, 0x13, 0x2c, 0x56, 0x5c, 0x68, 0xb2, + 0x06, 0x7e, 0xb7, 0xba, 0xbb, 0x58, 0x5d, 0xa1, 0x63, 0xa8, 0x4a, 0x9e, 0x08, 0x2f, 0xef, 0xb7, + 0x33, 0xd8, 0x3b, 0x59, 0x09, 0x35, 0xd2, 0xf8, 0x38, 0x8d, 0x29, 0x2e, 0x7c, 0xb2, 0x32, 0x22, + 0x12, 0x52, 0x5d, 0x46, 0x03, 0x6b, 0x1b, 0x1d, 0xc3, 0x8e, 0x97, 0x09, 0xa7, 0xbb, 0x6f, 0x0e, + 0xf6, 0xd7, 0x09, 0x36, 0x65, 0xc5, 0xb9, 0x53, 0x96, 0x41, 0x92, 0x40, 0x39, 0x3b, 0x79, 0x23, + 0x99, 0x8d, 0x6c, 0xb0, 0x12, 0xe6, 0x3b, 0x55, 0xad, 0x5e, 0x66, 0xa2, 0x33, 0x68, 0x2e, 0x73, + 0xd5, 0xb4, 0x22, 0x35, 0x9d, 0xd9, 0x59, 0x67, 0xde, 0x96, 0x14, 0xc3, 0x72, 0x75, 0xee, 0xfd, + 0x67, 0xc2, 0xee, 0x65, 0x2e, 0x1d, 0xe3, 0xd1, 0x4f, 0xfa, 0x57, 0x22, 0x07, 0x6a, 0x31, 0xf1, + 0x7d, 0x16, 0xcd, 0xb4, 0x18, 0x16, 0x2e, 0x8f, 0xe8, 0x5b, 0xa8, 0x7b, 0x3c, 0x52, 0x34, 0x52, + 0xb2, 0x90, 0xa0, 0xbb, 0xe6, 0x79, 0x96, 0xe8, 0xe4, 0x8e, 0xfb, 0x14, 0xaf, 0x22, 0xd0, 0x77, + 0xd0, 0x98, 0xb2, 0x80, 0x66, 0x42, 0x48, 0xad, 0xca, 0x6b, 0xc2, 0xd7, 0x21, 0xe8, 0x10, 0x3a, + 0x31, 0x0f, 0x98, 0x97, 0xba, 0x8f, 0x54, 0x48, 0xc6, 0xa3, 0xe2, 0x0d, 0xb5, 0x73, 0x74, 0x92, + 0x83, 0xbd, 0xbf, 0x0c, 0xa8, 0x64, 0xa1, 0xa8, 0x09, 0x35, 0x9f, 0x4e, 0x49, 0x12, 0x28, 0xfb, + 0x03, 0xf4, 0x21, 0x34, 0xcf, 0x2f, 0x47, 0xee, 0xe0, 0xf4, 0x6b, 0xf7, 0xb7, 0xf1, 0xc8, 0x36, + 0x36, 0x81, 0x1f, 0x86, 0x77, 0xb6, 0xb9, 0x09, 0x0c, 0xbf, 0x1f, 0xda, 0xd6, 0x16, 0x30, 0x1e, + 0xd9, 0x95, 0x12, 0xe8, 0x0f, 0xbe, 0xd1, 0x1e, 0x3b, 0x5b, 0xc0, 0x78, 0x64, 0x57, 0x51, 0x0b, + 0xea, 0xe7, 0x3e, 0x23, 0x91, 0x4a, 0x42, 0xbb, 0xd1, 0xfb, 0xd3, 0x00, 0xbb, 0x50, 0xff, 0x5e, + 0x97, 0x98, 0xbd, 0xce, 0xf7, 0x78, 0x77, 0x4f, 0x26, 0x6c, 0xbe, 0x61, 0xc2, 0xff, 0x18, 0x00, + 0x39, 0xb7, 0x7e, 0xf4, 0x87, 0xd0, 0x59, 0xd0, 0xf4, 0x39, 0x6d, 0x7b, 0x41, 0xd3, 0x0d, 0xc2, + 0x53, 0xa8, 0xf1, 0x7c, 0x08, 0x05, 0xd9, 0xa7, 0x2f, 0xcc, 0x09, 0x97, 0xbe, 0xe8, 0x06, 0xde, + 0x95, 0x75, 0x16, 0x83, 0x5a, 0xd0, 0x34, 0x1b, 0xb5, 0x75, 0xd4, 0x1c, 0x7c, 0xf2, 0xac, 0xde, + 0x95, 0x26, 0x78, 0x77, 0xf9, 0x04, 0x91, 0xbd, 0x7f, 0x4d, 0xa8, 0x0e, 0x79, 0x34, 0x65, 0xb3, + 0x8d, 0xb5, 0x33, 0x5e, 0xb1, 0x76, 0xa7, 0x00, 0x73, 0x22, 0xe7, 0x6e, 0xbe, 0x67, 0xe6, 0x8b, + 0x7b, 0xd6, 0xc8, 0x3c, 0xf3, 0x7f, 0xb2, 0x8d, 0x96, 0x2b, 0x6f, 0x68, 0xf9, 0x1c, 0x3e, 0x4b, + 0x24, 0x75, 0xa7, 0x32, 0x6b, 0x55, 0xb0, 0x68, 0xe6, 0x4e, 0xb9, 0x70, 0x1f, 0xfb, 0xb9, 0x00, + 0x8c, 0x4a, 0xbd, 0xbc, 0x75, 0x7c, 0x90, 0x48, 0x7a, 0x25, 0x6f, 0x73, 0x9f, 0x2b, 0x2e, 0x26, + 0xfd, 0xfb, 0xc2, 0x01, 0x9d, 0xc1, 0x01, 0x09, 0x02, 0xbe, 0x74, 0x3d, 0xc1, 0xa5, 0x74, 0x13, + 0x49, 0x85, 0x5b, 0x52, 0xeb, 0x3d, 0xaf, 0xe3, 0x7d, 0xed, 0x30, 0xcc, 0xee, 0x7f, 0x91, 0x54, + 0xdc, 0x15, 0xb7, 0x37, 0x95, 0xba, 0x65, 0x57, 0x70, 0xdb, 0xe3, 0x61, 0x4c, 0x14, 0x7b, 0x60, + 0x01, 0x53, 0xe9, 0x17, 0x3f, 0x03, 0xac, 0x65, 0xd9, 0x5e, 0x02, 0x04, 0x9d, 0x98, 0x84, 0x6e, + 0x4c, 0xa4, 0x8c, 0xe7, 0x82, 0x48, 0x6a, 0x1b, 0xe8, 0x23, 0xd8, 0xf5, 0x12, 0xa9, 0xf8, 0x16, + 0x6c, 0x66, 0x71, 0x82, 0x2c, 0xb3, 0xae, 0x6c, 0xeb, 0xa1, 0xaa, 0xbf, 0x03, 0x5f, 0xfd, 0x1f, + 0x00, 0x00, 0xff, 0xff, 0xe2, 0x78, 0x9e, 0x2e, 0x22, 0x06, 0x00, 0x00, } diff --git a/metadata/metadata.proto b/metadata/metadata.proto index 8ffb4f6..84245e0 100644 --- a/metadata/metadata.proto +++ b/metadata/metadata.proto @@ -99,6 +99,7 @@ message Config { HashingCosts hash_costs = 2; EncryptionOptions options = 4; bool use_fs_keyring_for_v1_policies = 5; + bool allow_cross_user_metadata = 6; // reserve the removed field 'string compatibility = 3;' reserved 3; diff --git a/pam_fscrypt/run_fscrypt.go b/pam_fscrypt/run_fscrypt.go index ef7ff92..8c640ce 100644 --- a/pam_fscrypt/run_fscrypt.go +++ b/pam_fscrypt/run_fscrypt.go @@ -137,6 +137,13 @@ func loginProtector(handle *pam.Handle) (*actions.Protector, error) { if err != nil { return nil, err } + // Ensure that pam_fscrypt only processes metadata files owned by the + // user or root, even if the user is root themselves. (Normally, when + // fscrypt is run as root it is allowed to process all metadata files. + // This implements stricter behavior for pam_fscrypt.) + if !ctx.Config.GetAllowCrossUserMetadata() { + ctx.TrustedUser = handle.PamUser + } // Find the user's PAM protector. options, err := ctx.ProtectorOptions() @@ -164,10 +171,11 @@ func policiesUsingProtector(protector *actions.Protector) []*actions.Policy { var policies []*actions.Policy for _, mount := range mounts { // Skip mountpoints that do not use the protector. - if _, _, err := mount.GetProtector(protector.Descriptor()); err != nil { + if _, _, err := mount.GetProtector(protector.Descriptor(), + protector.Context.TrustedUser); err != nil { continue } - policyDescriptors, err := mount.ListPolicies() + policyDescriptors, err := mount.ListPolicies(protector.Context.TrustedUser) if err != nil { log.Printf("listing policies: %s", err) continue -- cgit v1.3 From d4ce0b892cbe68db9f90f4015342e6a9069b079c Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Wed, 23 Feb 2022 12:35:04 -0800 Subject: Make all new metadata files owned by user when needed Since commit 4c7c6631cc5a ("Set owner of login protectors to correct user"), login protectors are made owned by the user when root creates one on a user's behalf. That's good, but the same isn't true of other files that get created at the same time: - The policy protecting the directory - The protector link file, if the policy is on a different filesystem - The recovery protector, if the policy is on a different filesystem - The recovery instructions file In preparation for setting all metadata files to mode 0600, start making all these files owned by the user in this scenario as well. --- actions/policy.go | 36 ++++++++++++++++++++++++++++++++++-- actions/policy_test.go | 8 ++++---- actions/protector.go | 16 +++++++++------- actions/protector_test.go | 4 ++-- actions/recovery.go | 8 +++++++- cli-tests/t_encrypt_login.out | 2 ++ cli-tests/t_encrypt_login.sh | 8 ++++++++ cmd/fscrypt/protector.go | 8 ++++++-- filesystem/filesystem.go | 21 ++++++++------------- filesystem/filesystem_test.go | 34 +++++++++++++++++----------------- 10 files changed, 97 insertions(+), 48 deletions(-) (limited to 'cli-tests') diff --git a/actions/policy.go b/actions/policy.go index 3b8eb30..3b20176 100644 --- a/actions/policy.go +++ b/actions/policy.go @@ -23,6 +23,7 @@ import ( "fmt" "log" "os" + "os/user" "github.com/golang/protobuf/proto" "github.com/pkg/errors" @@ -178,6 +179,7 @@ type Policy struct { data *metadata.PolicyData key *crypto.Key created bool + ownerIfCreating *user.User newLinkedProtectors []string } @@ -210,6 +212,12 @@ func CreatePolicy(ctx *Context, protector *Protector) (*Policy, error) { created: true, } + policy.ownerIfCreating, err = getOwnerOfMetadataForProtector(protector) + if err != nil { + policy.Lock() + return nil, err + } + if err = policy.AddProtector(protector); err != nil { policy.Lock() return nil, err @@ -410,6 +418,25 @@ func (policy *Policy) UsesProtector(protector *Protector) bool { return ok } +// getOwnerOfMetadataForProtector returns the User to whom the owner of any new +// policies or protector links for the given protector should be set. +// +// This will return a non-nil value only when the protector is a login protector +// and the process is running as root. In this scenario, root is setting up +// encryption on the user's behalf, so we need to make new policies and +// protector links owned by the user (rather than root) to allow them to be read +// by the user, just like the login protector itself which is handled elsewhere. +func getOwnerOfMetadataForProtector(protector *Protector) (*user.User, error) { + if protector.data.Source == metadata.SourceType_pam_passphrase && util.IsUserRoot() { + owner, err := util.UserFromUID(protector.data.Uid) + if err != nil { + return nil, err + } + return owner, nil + } + return nil, nil +} + // AddProtector updates the data that is wrapping the Policy Key so that the // provided Protector is now protecting the specified Policy. If an error is // returned, no data has been changed. If the policy and protector are on @@ -427,8 +454,13 @@ func (policy *Policy) AddProtector(protector *Protector) error { // to it on the policy's filesystem. if policy.Context.Mount != protector.Context.Mount { log.Printf("policy on %s\n protector on %s\n", policy.Context.Mount, protector.Context.Mount) + ownerIfCreating, err := getOwnerOfMetadataForProtector(protector) + if err != nil { + return err + } isNewLink, err := policy.Context.Mount.AddLinkedProtector( - protector.Descriptor(), protector.Context.Mount, protector.Context.TrustedUser) + protector.Descriptor(), protector.Context.Mount, + protector.Context.TrustedUser, ownerIfCreating) if err != nil { return err } @@ -554,7 +586,7 @@ func (policy *Policy) CanBeAppliedWithoutProvisioning() bool { // commitData writes the Policy's current data to the filesystem. func (policy *Policy) commitData() error { - return policy.Context.Mount.AddPolicy(policy.data) + return policy.Context.Mount.AddPolicy(policy.data, policy.ownerIfCreating) } // findWrappedPolicyKey returns the index of the wrapped policy key diff --git a/actions/policy_test.go b/actions/policy_test.go index 07943b8..8248862 100644 --- a/actions/policy_test.go +++ b/actions/policy_test.go @@ -27,7 +27,7 @@ import ( // Makes a protector and policy func makeBoth() (*Protector, *Policy, error) { - protector, err := CreateProtector(testContext, testProtectorName, goodCallback) + protector, err := CreateProtector(testContext, testProtectorName, goodCallback, nil) if err != nil { return nil, nil, err } @@ -68,7 +68,7 @@ func TestPolicyGoodAddProtector(t *testing.T) { t.Fatal(err) } - pro2, err := CreateProtector(testContext, testProtectorName2, goodCallback) + pro2, err := CreateProtector(testContext, testProtectorName2, goodCallback, nil) if err != nil { t.Fatal(err) } @@ -103,7 +103,7 @@ func TestPolicyGoodRemoveProtector(t *testing.T) { t.Fatal(err) } - pro2, err := CreateProtector(testContext, testProtectorName2, goodCallback) + pro2, err := CreateProtector(testContext, testProtectorName2, goodCallback, nil) if err != nil { t.Fatal(err) } @@ -129,7 +129,7 @@ func TestPolicyBadRemoveProtector(t *testing.T) { t.Fatal(err) } - pro2, err := CreateProtector(testContext, testProtectorName2, goodCallback) + pro2, err := CreateProtector(testContext, testProtectorName2, goodCallback, nil) if err != nil { t.Fatal(err) } diff --git a/actions/protector.go b/actions/protector.go index 1171c83..b986eb0 100644 --- a/actions/protector.go +++ b/actions/protector.go @@ -109,17 +109,18 @@ func checkIfUserHasLoginProtector(ctx *Context, uid int64) error { // to unlock policies and create new polices. As with the key struct, a // Protector should be wiped after use. type Protector struct { - Context *Context - data *metadata.ProtectorData - key *crypto.Key - created bool + Context *Context + data *metadata.ProtectorData + key *crypto.Key + created bool + ownerIfCreating *user.User } // CreateProtector creates an unlocked protector with a given name (name only // needed for custom and raw protector types). The keyFn provided to create the // Protector key will only be called once. If an error is returned, no data has // been changed on the filesystem. -func CreateProtector(ctx *Context, name string, keyFn KeyFunc) (*Protector, error) { +func CreateProtector(ctx *Context, name string, keyFn KeyFunc, owner *user.User) (*Protector, error) { if err := ctx.checkContext(); err != nil { return nil, err } @@ -147,7 +148,8 @@ func CreateProtector(ctx *Context, name string, keyFn KeyFunc) (*Protector, erro Name: name, Source: ctx.Config.Source, }, - created: true, + created: true, + ownerIfCreating: owner, } // Extra data is needed for some SourceTypes @@ -294,5 +296,5 @@ func (protector *Protector) Rewrap(keyFn KeyFunc) error { return err } - return protector.Context.Mount.AddProtector(protector.data) + return protector.Context.Mount.AddProtector(protector.data, protector.ownerIfCreating) } diff --git a/actions/protector_test.go b/actions/protector_test.go index 6c81d49..f20dbcf 100644 --- a/actions/protector_test.go +++ b/actions/protector_test.go @@ -43,7 +43,7 @@ func badCallback(info ProtectorInfo, retry bool) (*crypto.Key, error) { // Tests that we can create a valid protector. func TestCreateProtector(t *testing.T) { - p, err := CreateProtector(testContext, testProtectorName, goodCallback) + p, err := CreateProtector(testContext, testProtectorName, goodCallback, nil) if err != nil { t.Error(err) } else { @@ -54,7 +54,7 @@ func TestCreateProtector(t *testing.T) { // Tests that a failure in the callback is relayed back to the caller. func TestBadCallback(t *testing.T) { - p, err := CreateProtector(testContext, testProtectorName, badCallback) + p, err := CreateProtector(testContext, testProtectorName, badCallback, nil) if err == nil { p.Lock() p.Destroy() diff --git a/actions/recovery.go b/actions/recovery.go index f533906..8a769cc 100644 --- a/actions/recovery.go +++ b/actions/recovery.go @@ -25,6 +25,7 @@ import ( "github.com/google/fscrypt/crypto" "github.com/google/fscrypt/metadata" + "github.com/google/fscrypt/util" ) // modifiedContextWithSource returns a copy of ctx with the protector source @@ -66,7 +67,7 @@ func AddRecoveryPassphrase(policy *Policy, dirname string) (*crypto.Key, *Protec if seq != 1 { name += " (" + strconv.Itoa(seq) + ")" } - recoveryProtector, err = CreateProtector(customCtx, name, getPassphraseFn) + recoveryProtector, err = CreateProtector(customCtx, name, getPassphraseFn, policy.ownerIfCreating) if err == nil { break } @@ -121,5 +122,10 @@ It is safe to keep it around though, as the recovery passphrase is high-entropy. if _, err = file.WriteString(str); err != nil { return err } + if recoveryProtector.ownerIfCreating != nil { + if err = util.Chown(file, recoveryProtector.ownerIfCreating); err != nil { + return err + } + } return file.Sync() } diff --git a/cli-tests/t_encrypt_login.out b/cli-tests/t_encrypt_login.out index b84216a..bb91a46 100644 --- a/cli-tests/t_encrypt_login.out +++ b/cli-tests/t_encrypt_login.out @@ -118,6 +118,8 @@ desc19 Yes (MNT_ROOT) login protector for fscrypt-test-user desc20 No custom protector "Recovery passphrase for dir" Protector is owned by fscrypt-test-user:fscrypt-test-user +"MNT/dir" is now locked. +"MNT/dir" is now locked. # Encrypt with login protector with --no-recovery ext4 filesystem "MNT" has 1 protector and 1 policy. diff --git a/cli-tests/t_encrypt_login.sh b/cli-tests/t_encrypt_login.sh index 225a47d..b6ae2d8 100755 --- a/cli-tests/t_encrypt_login.sh +++ b/cli-tests/t_encrypt_login.sh @@ -58,9 +58,17 @@ begin "Encrypt with login protector as root" echo TEST_USER_PASS | fscrypt encrypt --quiet --source=pam_passphrase --user="$TEST_USER" "$dir" show_status true # The newly-created login protector should be owned by the user, not root. +# This is partly redundant with the below check, but we might as well test both. login_protector=$(_get_login_descriptor) owner=$(stat -c "%U:%G" "$MNT_ROOT/.fscrypt/protectors/$login_protector") echo -e "\nProtector is owned by $owner" +# The user should be able to lock and unlock the directory themselves. This +# tests that the fscrypt metadata file permissions got set appropriately when +# root set up the encryption on the user's behalf. +chown "$TEST_USER" "$dir" +_user_do "fscrypt lock $dir" +_user_do "echo TEST_USER_PASS | fscrypt unlock $dir --quiet --unlock-with=$MNT_ROOT:$login_protector" +_user_do "fscrypt lock $dir" begin "Encrypt with login protector with --no-recovery" chown "$TEST_USER" "$dir" diff --git a/cmd/fscrypt/protector.go b/cmd/fscrypt/protector.go index ac864dd..186ca7a 100644 --- a/cmd/fscrypt/protector.go +++ b/cmd/fscrypt/protector.go @@ -23,6 +23,7 @@ package main import ( "fmt" "log" + "os/user" "github.com/google/fscrypt/actions" "github.com/google/fscrypt/filesystem" @@ -38,7 +39,6 @@ func createProtectorFromContext(ctx *actions.Context) (*actions.Protector, error return nil, err } log.Printf("using source: %s", ctx.Config.Source.String()) - if ctx.Config.Source == metadata.SourceType_pam_passphrase { if userFlag.Value == "" && util.IsUserRoot() { return nil, ErrSpecifyUser @@ -70,7 +70,11 @@ IMPORTANT: Before continuing, ensure you have properly set up your system for } } - return actions.CreateProtector(ctx, name, createKeyFn) + var owner *user.User + if ctx.Config.Source == metadata.SourceType_pam_passphrase && util.IsUserRoot() { + owner = ctx.TargetUser + } + return actions.CreateProtector(ctx, name, createKeyFn, owner) } // selectExistingProtector returns a locked Protector which corresponds to an diff --git a/filesystem/filesystem.go b/filesystem/filesystem.go index 6e4f2c6..1877b1b 100644 --- a/filesystem/filesystem.go +++ b/filesystem/filesystem.go @@ -649,6 +649,8 @@ func (m *Mount) writeData(path string, data []byte, owner *user.User) error { tempFile.Close() return err } + // Override the file owner if one was specified. This happens when root + // needs to create files owned by a particular user. if owner != nil { if err = util.Chown(tempFile, owner); err != nil { log.Printf("could not set owner of %q to %v: %v", @@ -786,7 +788,7 @@ func (m *Mount) removeMetadata(path string) error { // will overwrite the value of an existing protector with this descriptor. This // will fail with ErrLinkedProtector if a linked protector with this descriptor // already exists on the filesystem. -func (m *Mount) AddProtector(data *metadata.ProtectorData) error { +func (m *Mount) AddProtector(data *metadata.ProtectorData, owner *user.User) error { var err error if err = m.CheckSetup(nil); err != nil { return err @@ -796,21 +798,14 @@ func (m *Mount) AddProtector(data *metadata.ProtectorData) error { data.ProtectorDescriptor, m.Path) } path := m.protectorPath(data.ProtectorDescriptor) - - var owner *user.User - if data.Source == metadata.SourceType_pam_passphrase && util.IsUserRoot() { - owner, err = util.UserFromUID(data.Uid) - if err != nil { - return err - } - } return m.addMetadata(path, data, owner) } // AddLinkedProtector adds a link in this filesystem to the protector metadata // in the dest filesystem, if one doesn't already exist. On success, the return // value is a nil error and a bool that is true iff the link is newly created. -func (m *Mount) AddLinkedProtector(descriptor string, dest *Mount, trustedUser *user.User) (bool, error) { +func (m *Mount) AddLinkedProtector(descriptor string, dest *Mount, trustedUser *user.User, + ownerIfCreating *user.User) (bool, error) { if err := m.CheckSetup(trustedUser); err != nil { return false, err } @@ -843,7 +838,7 @@ func (m *Mount) AddLinkedProtector(descriptor string, dest *Mount, trustedUser * if err != nil { return false, err } - return true, m.writeData(linkPath, []byte(newLink), nil) + return true, m.writeData(linkPath, []byte(newLink), ownerIfCreating) } // GetRegularProtector looks up the protector metadata by descriptor. This will @@ -931,12 +926,12 @@ func (m *Mount) ListProtectors(trustedUser *user.User) ([]string, error) { } // AddPolicy adds the policy metadata to the filesystem storage. -func (m *Mount) AddPolicy(data *metadata.PolicyData) error { +func (m *Mount) AddPolicy(data *metadata.PolicyData, owner *user.User) error { if err := m.CheckSetup(nil); err != nil { return err } - return m.addMetadata(m.PolicyPath(data.KeyDescriptor), data, nil) + return m.addMetadata(m.PolicyPath(data.KeyDescriptor), data, owner) } // GetPolicy looks up the policy metadata by descriptor. diff --git a/filesystem/filesystem_test.go b/filesystem/filesystem_test.go index 92e113b..f74078d 100644 --- a/filesystem/filesystem_test.go +++ b/filesystem/filesystem_test.go @@ -253,31 +253,31 @@ func TestAddProtector(t *testing.T) { defer mnt.RemoveAllMetadata() protector := getFakeProtector() - if err = mnt.AddProtector(protector); err != nil { + if err = mnt.AddProtector(protector, nil); err != nil { t.Error(err) } // Change the source to bad one, or one that requires hashing costs protector.Source = metadata.SourceType_default - if mnt.AddProtector(protector) == nil { + if mnt.AddProtector(protector, nil) == nil { t.Error("bad source for a descriptor should make metadata invalid") } protector.Source = metadata.SourceType_custom_passphrase - if mnt.AddProtector(protector) == nil { + if mnt.AddProtector(protector, nil) == nil { t.Error("protectors using passphrases should require hashing costs") } protector.Source = metadata.SourceType_raw_key // Use a bad wrapped key protector.WrappedKey = wrappedPolicyKey - if mnt.AddProtector(protector) == nil { + if mnt.AddProtector(protector, nil) == nil { t.Error("bad length for protector keys should make metadata invalid") } protector.WrappedKey = wrappedProtectorKey // Change the descriptor (to a bad length) protector.ProtectorDescriptor = "abcde" - if mnt.AddProtector(protector) == nil { + if mnt.AddProtector(protector, nil) == nil { t.Error("bad descriptor length should make metadata invalid") } @@ -292,32 +292,32 @@ func TestAddPolicy(t *testing.T) { defer mnt.RemoveAllMetadata() policy := getFakePolicy() - if err = mnt.AddPolicy(policy); err != nil { + if err = mnt.AddPolicy(policy, nil); err != nil { t.Error(err) } // Bad encryption options should make policy invalid policy.Options.Padding = 7 - if mnt.AddPolicy(policy) == nil { + if mnt.AddPolicy(policy, nil) == nil { t.Error("padding not a power of 2 should make metadata invalid") } policy.Options.Padding = 16 policy.Options.Filenames = metadata.EncryptionOptions_default - if mnt.AddPolicy(policy) == nil { + if mnt.AddPolicy(policy, nil) == nil { t.Error("encryption mode not set should make metadata invalid") } policy.Options.Filenames = metadata.EncryptionOptions_AES_256_CTS // Use a bad wrapped key policy.WrappedPolicyKeys[0].WrappedKey = wrappedProtectorKey - if mnt.AddPolicy(policy) == nil { + if mnt.AddPolicy(policy, nil) == nil { t.Error("bad length for policy keys should make metadata invalid") } policy.WrappedPolicyKeys[0].WrappedKey = wrappedPolicyKey // Change the descriptor (to a bad length) policy.KeyDescriptor = "abcde" - if mnt.AddPolicy(policy) == nil { + if mnt.AddPolicy(policy, nil) == nil { t.Error("bad descriptor length should make metadata invalid") } } @@ -331,7 +331,7 @@ func TestSetPolicy(t *testing.T) { defer mnt.RemoveAllMetadata() policy := getFakePolicy() - if err = mnt.AddPolicy(policy); err != nil { + if err = mnt.AddPolicy(policy, nil); err != nil { t.Fatal(err) } @@ -355,7 +355,7 @@ func TestSetProtector(t *testing.T) { defer mnt.RemoveAllMetadata() protector := getFakeProtector() - if err = mnt.AddProtector(protector); err != nil { + if err = mnt.AddProtector(protector, nil); err != nil { t.Fatal(err) } @@ -383,7 +383,7 @@ func TestSpoofedLoginProtector(t *testing.T) { // Control case: protector with matching UID should be accepted. protector := getFakeLoginProtector(myUID) - if err = mnt.AddProtector(protector); err != nil { + if err = mnt.AddProtector(protector, nil); err != nil { t.Fatal(err) } _, err = mnt.GetRegularProtector(protector.ProtectorDescriptor, nil) @@ -398,7 +398,7 @@ func TestSpoofedLoginProtector(t *testing.T) { // *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 { + if err = mnt.AddProtector(protector, nil); err != nil { t.Fatal(err) } _, err = mnt.GetRegularProtector(protector.ProtectorDescriptor, nil) @@ -445,19 +445,19 @@ func TestLinkedProtector(t *testing.T) { // Add the protector to the first filesystem protector := getFakeProtector() - if err = realMnt.AddProtector(protector); err != nil { + if err = realMnt.AddProtector(protector, nil); err != nil { t.Fatal(err) } // Add the link to the second filesystem var isNewLink bool - if isNewLink, err = fakeMnt.AddLinkedProtector(protector.ProtectorDescriptor, realMnt, nil); err != nil { + if isNewLink, err = fakeMnt.AddLinkedProtector(protector.ProtectorDescriptor, realMnt, nil, nil); err != nil { t.Fatal(err) } if !isNewLink { t.Fatal("Link was not new") } - if isNewLink, err = fakeMnt.AddLinkedProtector(protector.ProtectorDescriptor, realMnt, nil); err != nil { + if isNewLink, err = fakeMnt.AddLinkedProtector(protector.ProtectorDescriptor, realMnt, nil, nil); err != nil { t.Fatal(err) } if isNewLink { -- cgit v1.3 From 06c989df4e31dd9f172f94fbd6243f49d4dd0b92 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Wed, 23 Feb 2022 12:35:04 -0800 Subject: filesystem: create metadata files with mode 0600 Currently, fscrypt policies and protectors are world readable, as they are created with mode 0644. While this can be nice for use cases where users share these files, those use cases seem to be quite rare, and it's not a great default security-wise since it exposes password hashes to all users. While fscrypt uses a very strong password hash algorithm, it would still be best to follow the lead of /etc/shadow and keep this information non-world-readable. Therefore, start creating these files with mode 0600. Of course, if users do actually want to share these files, they have the option of simply chmod'ing them to a less restrictive mode. An option could also be added to make fscrypt use the old mode 0644; however, the need for that is currently unclear. --- README.md | 4 +++- cli-tests/t_lock.out | 1 - cli-tests/t_lock.sh | 5 ++++- filesystem/filesystem.go | 11 ++++++++--- filesystem/filesystem_test.go | 35 +++++++++++++++++++++++++++++++++++ 5 files changed, 50 insertions(+), 6 deletions(-) (limited to 'cli-tests') diff --git a/README.md b/README.md index f1803b4..eff9ecf 100644 --- a/README.md +++ b/README.md @@ -385,7 +385,9 @@ The fields are: other users might be untrusted and could create malicious files. This can be set to `true` to restore the old behavior on systems where `fscrypt` metadata needs to be shared between multiple users. Note that this option is - independent from the permissions on the metadata files themselves. + independent from the permissions on the metadata files themselves, which are + set to 0600 by default; users who wish to share their metadata files with + other users would also need to explicitly change their mode to 0644. ## Setting up `fscrypt` on a filesystem diff --git a/cli-tests/t_lock.out b/cli-tests/t_lock.out index b8c8dcb..0da8964 100644 --- a/cli-tests/t_lock.out +++ b/cli-tests/t_lock.out @@ -76,7 +76,6 @@ cat: MNT/dir/file: No such file or directory mkdir: cannot create directory 'MNT/dir/subdir': Required key not available # Try to lock directory while other user has unlocked -Enter custom passphrase for protector "prot": "MNT/dir" is now unlocked and ready for use. [ERROR] fscrypt lock: Directory "MNT/dir" couldn't be fully locked because other user(s) have unlocked it. diff --git a/cli-tests/t_lock.sh b/cli-tests/t_lock.sh index 7ac1727..9b193fd 100755 --- a/cli-tests/t_lock.sh +++ b/cli-tests/t_lock.sh @@ -43,8 +43,11 @@ _expect_failure "cat '$dir/file'" _expect_failure "mkdir '$dir/subdir'" _print_header "Try to lock directory while other user has unlocked" +rm -rf "$dir" +mkdir "$dir" chown "$TEST_USER" "$dir" -_user_do "echo hunter2 | fscrypt unlock '$dir'" +_user_do "echo hunter2 | fscrypt encrypt --quiet --name=prot '$dir'" +_user_do "echo contents > $dir/file" _expect_failure "fscrypt lock '$dir'" cat "$dir/file" fscrypt lock --all-users "$dir" diff --git a/filesystem/filesystem.go b/filesystem/filesystem.go index 70076b7..27bfa24 100644 --- a/filesystem/filesystem.go +++ b/filesystem/filesystem.go @@ -252,9 +252,14 @@ const ( // The base directory should be read-only (except for the creator) basePermissions = 0755 - // The metadata files are globally visible, but can only be deleted by - // the user that created them - filePermissions = os.FileMode(0644) + + // The metadata files shouldn't be readable or writable by other users. + // Having them be world-readable wouldn't necessarily be a huge issue, + // but given that some of these files contain (strong) password hashes, + // we error on the side of caution -- similar to /etc/shadow. + // Note: existing files on-disk might have mode 0644, as that was the + // mode used by fscrypt v0.3.2 and earlier. + filePermissions = os.FileMode(0600) // 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 diff --git a/filesystem/filesystem_test.go b/filesystem/filesystem_test.go index f74078d..0e15256 100644 --- a/filesystem/filesystem_test.go +++ b/filesystem/filesystem_test.go @@ -413,6 +413,41 @@ func TestSpoofedLoginProtector(t *testing.T) { } } +// Tests that the fscrypt metadata files are given mode 0600. +func TestMetadataFileMode(t *testing.T) { + mnt, err := getSetupMount(t) + if err != nil { + t.Fatal(err) + } + defer mnt.RemoveAllMetadata() + + // Policy + policy := getFakePolicy() + if err = mnt.AddPolicy(policy, nil); err != nil { + t.Fatal(err) + } + fi, err := os.Stat(filepath.Join(mnt.Path, ".fscrypt/policies/", policy.KeyDescriptor)) + if err != nil { + t.Fatal(err) + } + if fi.Mode()&0777 != 0600 { + t.Error("Policy file has wrong mode") + } + + // Protector + protector := getFakeProtector() + if err = mnt.AddProtector(protector, nil); err != nil { + t.Fatal(err) + } + fi, err = os.Stat(filepath.Join(mnt.Path, ".fscrypt/protectors", protector.ProtectorDescriptor)) + if err != nil { + t.Fatal(err) + } + if fi.Mode()&0777 != 0600 { + t.Error("Protector file has wrong mode") + } +} + // 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 { -- cgit v1.3