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.

diff --git a/README.md b/README.md
index f1803b4611a43c3b721b2b57bfcd7ee9e0e68197..eff9ecf53cf7525c3d064c612dfc379a31516a83 100644 (file)
--- 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 b8c8dcbb03e57923e04c298bf692bb580afe64c5..0da8964fb5262a0034e4bea47830ae82022d1b04 100644 (file)
--- 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 7ac17274b8759598984287c6e3c19de82fa14a0f..9b193fddfb91c84323b28cbe83499db57ef919ab 100755 (executable)
--- 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 70076b72980e7f3bf36af49b9f9e8db4ead0442c..27bfa2415656c62a4ecafdfc10cfa8a08d48d2ea 100644 (file)
--- 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 f74078d6a21c4771789a975af249c2e41fc41b7a..0e15256e4f11c50b9983b3a9b4dad40edcf1e717 100644 (file)
--- 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 {