From be33028561c414ef56e557237bff8170a2922331 Mon Sep 17 00:00:00 2001 From: Ethan Lowman Date: Tue, 23 Aug 2022 17:39:00 -0400 Subject: [PATCH] Clarify permissions check naming and comment, add tests --- client/filejsonstore/filejsonstore.go | 4 +- internal/fsutil/fsutil.go | 3 -- internal/fsutil/perm.go | 26 ++++++---- internal/fsutil/perm_test.go | 71 +++++++++++++++++++++++++++ internal/fsutil/perm_windows.go | 4 +- 5 files changed, 92 insertions(+), 16 deletions(-) create mode 100644 internal/fsutil/perm_test.go diff --git a/client/filejsonstore/filejsonstore.go b/client/filejsonstore/filejsonstore.go index c80d8eded..b43754375 100644 --- a/client/filejsonstore/filejsonstore.go +++ b/client/filejsonstore/filejsonstore.go @@ -71,7 +71,7 @@ func NewFileJSONStore(baseDir string) (*FileJSONStore, error) { baseDir) } // Verify file mode is not too permissive. - if err = fsutil.EnsurePermission(fi, dirCreateMode); err != nil { + if err = fsutil.EnsureMaxPermissions(fi, dirCreateMode); err != nil { return nil, ErrTooPermissive } } @@ -104,7 +104,7 @@ func (f *FileJSONStore) GetMeta() (map[string]json.RawMessage, error) { if err != nil { return nil, err } - if err = fsutil.EnsurePermission(info, fileCreateMode); err != nil { + if err = fsutil.EnsureMaxPermissions(info, fileCreateMode); err != nil { return nil, ErrTooPermissive } b, err := os.ReadFile(p) diff --git a/internal/fsutil/fsutil.go b/internal/fsutil/fsutil.go index d07c72a75..774246f88 100644 --- a/internal/fsutil/fsutil.go +++ b/internal/fsutil/fsutil.go @@ -3,13 +3,10 @@ package fsutil import ( - "errors" "os" "path/filepath" ) -var ErrPermission = errors.New("unexpected permission") - // IsMetaFile tests wheter a DirEntry appears to be a metaddata file or not. func IsMetaFile(e os.DirEntry) (bool, error) { if e.IsDir() || filepath.Ext(e.Name()) != ".json" { diff --git a/internal/fsutil/perm.go b/internal/fsutil/perm.go index 9b0cb2eae..f94add608 100644 --- a/internal/fsutil/perm.go +++ b/internal/fsutil/perm.go @@ -4,18 +4,26 @@ package fsutil import ( - "io/fs" + "fmt" "os" ) -// EnsurePemissions tests the provided file info to make sure the -// permission bits matches the provided. -func EnsurePermission(fi os.FileInfo, perm os.FileMode) error { - // Clear all bits which are note related to the permission. - mode := fi.Mode() & fs.ModePerm - mask := ^perm - if (mode & mask) != 0 { - return ErrPermission +// EnsureMaxPermissions tests the provided file info, returning an error if the +// file's permission bits contain excess permissions not set in maxPerms. +// +// For example, a file with permissions -rw------- will successfully validate +// with maxPerms -rw-r--r-- or -rw-rw-r--, but will not validate with maxPerms +// -r-------- (due to excess --w------- permission) or --w------- (due to +// excess -r-------- permission). +// +// Only permission bits of the file modes are considered. +func EnsureMaxPermissions(fi os.FileInfo, maxPerms os.FileMode) error { + gotPerm := fi.Mode().Perm() + forbiddenPerms := (^maxPerms).Perm() + excessPerms := gotPerm & forbiddenPerms + + if excessPerms != 0 { + return fmt.Errorf("permission bits for file %v failed validation: want at most %v, got %v with excess perms %v", fi.Name(), maxPerms.Perm(), gotPerm, excessPerms) } return nil diff --git a/internal/fsutil/perm_test.go b/internal/fsutil/perm_test.go new file mode 100644 index 000000000..6061d2918 --- /dev/null +++ b/internal/fsutil/perm_test.go @@ -0,0 +1,71 @@ +//go:build !windows +// +build !windows + +package fsutil + +import ( + "fmt" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestEnsureMaxPermissions(t *testing.T) { + tmp := t.TempDir() + p := filepath.Join(tmp, "file.txt") + + // Start with 0644 and change using os.Chmod so umask doesn't interfere. + err := os.WriteFile(p, []byte(`AAA`), 0644) + assert.NoError(t, err) + + // Check matching (1) + err = os.Chmod(p, 0464) + assert.NoError(t, err) + fi, err := os.Stat(p) + assert.NoError(t, err) + err = EnsureMaxPermissions(fi, os.FileMode(0464)) + assert.NoError(t, err) + + // Check matching (2) + err = os.Chmod(p, 0642) + assert.NoError(t, err) + fi, err = os.Stat(p) + assert.NoError(t, err) + err = EnsureMaxPermissions(fi, os.FileMode(0642)) + assert.NoError(t, err) + + // Check matching with file mode bits + err = os.Chmod(p, 0444) + assert.NoError(t, err) + fi, err = os.Stat(p) + assert.NoError(t, err) + err = EnsureMaxPermissions(fi, os.ModeSymlink|os.ModeAppend|os.FileMode(0444)) + assert.NoError(t, err) + + // Check not matching (1) + err = os.Chmod(p, 0444) + assert.NoError(t, err) + fi, err = os.Stat(p) + assert.NoError(t, err) + err = EnsureMaxPermissions(fi, os.FileMode(0400)) + assert.Error(t, err) + + // Check not matching (2) + err = os.Chmod(p, 0444) + assert.NoError(t, err) + fi, err = os.Stat(p) + assert.NoError(t, err) + err = EnsureMaxPermissions(fi, os.FileMode(0222)) + assert.Error(t, err) + fmt.Println(err) + + // Check matching due to more restrictive perms on file + err = os.Chmod(p, 0444) + assert.NoError(t, err) + fi, err = os.Stat(p) + assert.NoError(t, err) + err = EnsureMaxPermissions(fi, os.FileMode(0666)) + assert.NoError(t, err) +} diff --git a/internal/fsutil/perm_windows.go b/internal/fsutil/perm_windows.go index 2f8ed61c3..11b0d0c73 100644 --- a/internal/fsutil/perm_windows.go +++ b/internal/fsutil/perm_windows.go @@ -4,11 +4,11 @@ import ( "os" ) -// EnsurePemissions tests the provided file info to make sure the +// EnsureMaxPermissions tests the provided file info to make sure the // permission bits matches the provided. // On Windows system the permission bits are not really compatible with // UNIX-like permission bits. // Currently this method will always return nil. -func EnsurePermission(fi os.FileInfo, perm os.FileMode) error { +func EnsureMaxPermissions(fi os.FileInfo, perm os.FileMode) error { return nil }