diff options
| author | Eric Biggers <ebiggers@google.com> | 2022-02-23 12:35:04 -0800 |
|---|---|---|
| committer | Eric Biggers <ebiggers@google.com> | 2022-02-23 12:35:04 -0800 |
| commit | 85a747493ff368a72f511619ecd391016ecb933c (patch) | |
| tree | 1f8144d14b25b508edfa4a882b8bd3eb4da0d238 /filesystem/filesystem_test.go | |
| parent | 74e870b7bd1585b4b509da47e0e75db66336e576 (diff) | |
Extend ownership validation to entire directory structure
A previous commit extended file ownership validation to policy and
protector files (by default -- there's an opt-out in /etc/fscrypt.conf).
However, that didn't apply to the parent directories:
MOUNTPOINT
MOUNTPOINT/.fscrypt
MOUNTPOINT/.fscrypt/policies
MOUNTPOINT/.fscrypt/protectors
The problem is that if the parent directories aren't trusted (owned by
another non-root user), then untrusted changes to their contents can be
made at any time, including the introduction of symlinks and so on.
While it's debatable how much of a problem this really is, given the
other validations that are done, it seems to be appropriate to validate
the parent directories too.
Therefore, this commit applies the same ownership validations to the
above four directories as are done on the metadata files themselves.
In addition, it is validated that none of these directories are symlinks
except for ".fscrypt" where this is explicitly supported.
Diffstat (limited to 'filesystem/filesystem_test.go')
| -rw-r--r-- | filesystem/filesystem_test.go | 37 |
1 files changed, 24 insertions, 13 deletions
diff --git a/filesystem/filesystem_test.go b/filesystem/filesystem_test.go index d4ef826..92e113b 100644 --- a/filesystem/filesystem_test.go +++ b/filesystem/filesystem_test.go @@ -21,7 +21,6 @@ package filesystem import ( "io/ioutil" - "log" "os" "os/user" "path/filepath" @@ -103,7 +102,7 @@ func TestSetup(t *testing.T) { t.Fatal(err) } - if err := mnt.CheckSetup(); err != nil { + if err := mnt.CheckSetup(nil); err != nil { t.Error(err) } @@ -126,16 +125,6 @@ func TestRemoveAllMetadata(t *testing.T) { } } -// loggedLstat runs os.Lstat (doesn't dereference trailing symlink), but it logs -// the error if lstat returns any error other than nil or IsNotExist. -func loggedLstat(name string) (os.FileInfo, error) { - info, err := os.Lstat(name) - if err != nil && !os.IsNotExist(err) { - log.Print(err) - } - return info, err -} - // isSymlink returns true if the path exists and is that of a symlink. func isSymlink(path string) bool { info, err := loggedLstat(path) @@ -158,7 +147,7 @@ func testSetupWithSymlink(t *testing.T, mnt *Mount, symlinkTarget string, realDi t.Fatal(err) } defer mnt.RemoveAllMetadata() - if err := mnt.CheckSetup(); err != nil { + if err := mnt.CheckSetup(nil); err != nil { t.Fatal(err) } if !isSymlink(rawBaseDir) { @@ -233,6 +222,28 @@ func TestSetupModes(t *testing.T) { testSetupMode(t, mnt, SingleUserWritable, 0755) } +// Tests that fscrypt refuses to use metadata directories that are +// world-writable but don't have the sticky bit set. +func TestInsecurePermissions(t *testing.T) { + mnt, err := getTestMount(t) + if err != nil { + t.Fatal(err) + } + defer mnt.RemoveAllMetadata() + + if err = mnt.Setup(WorldWritable); err != nil { + t.Fatal(err) + } + if err = os.Chmod(mnt.PolicyDir(), 0777); err != nil { + t.Fatal(err) + } + defer os.Chmod(mnt.PolicyDir(), os.ModeSticky|0777) + err = mnt.CheckSetup(nil) + if _, ok := err.(*ErrInsecurePermissions); !ok { + t.Fatal("expected ErrInsecurePermissions") + } +} + // Adding a good Protector should succeed, adding a bad one should fail func TestAddProtector(t *testing.T) { mnt, err := getSetupMount(t) |