From 271e5b14d27c2cdc9d404c657a2215a5efe6de97 Mon Sep 17 00:00:00 2001 From: miagilepner Date: Thu, 23 Feb 2023 18:05:00 +0100 Subject: [PATCH] VAULT-12299 Use file.Stat when checking file permissions (#19311) * use file.Stat for config files * cleanup and add path * include directory path * revert changes to LoadConfigDir * remove path, add additional test: * add changelog --- changelog/19311.txt | 3 ++ command/server/config.go | 18 +++++-- helper/osutil/fileinfo.go | 14 +++++ helper/osutil/fileinfo_test.go | 96 ++++++++++++++++++++++++++++++++++ 4 files changed, 127 insertions(+), 4 deletions(-) create mode 100644 changelog/19311.txt diff --git a/changelog/19311.txt b/changelog/19311.txt new file mode 100644 index 000000000..5ad6e2c01 --- /dev/null +++ b/changelog/19311.txt @@ -0,0 +1,3 @@ +```release-note:bug +server/config: Use file.Stat when checking file permissions when VAULT_ENABLE_FILE_PERMISSIONS_CHECK is enabled +``` diff --git a/command/server/config.go b/command/server/config.go index d2d30b40e..733892352 100644 --- a/command/server/config.go +++ b/command/server/config.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "io" - "io/ioutil" "math" "os" "path/filepath" @@ -465,9 +464,14 @@ func LoadConfig(path string) (*Config, error) { return nil, errors.New("Error parsing the environment variable VAULT_ENABLE_FILE_PERMISSIONS_CHECK") } } + f, err := os.Open(path) + if err != nil { + return nil, err + } + defer f.Close() if enableFilePermissionsCheck { - err = osutil.OwnerPermissionsMatch(path, 0, 0) + err = osutil.OwnerPermissionsMatchFile(f, 0, 0) if err != nil { return nil, err } @@ -496,8 +500,14 @@ func CheckConfig(c *Config, e error) (*Config, error) { // LoadConfigFile loads the configuration from the given file. func LoadConfigFile(path string) (*Config, error) { + // Open the file + f, err := os.Open(path) + if err != nil { + return nil, err + } + defer f.Close() // Read the file - d, err := ioutil.ReadFile(path) + d, err := io.ReadAll(f) if err != nil { return nil, err } @@ -518,7 +528,7 @@ func LoadConfigFile(path string) (*Config, error) { if enableFilePermissionsCheck { // check permissions of the config file - err = osutil.OwnerPermissionsMatch(path, 0, 0) + err = osutil.OwnerPermissionsMatchFile(f, 0, 0) if err != nil { return nil, err } diff --git a/helper/osutil/fileinfo.go b/helper/osutil/fileinfo.go index 4b6ba7910..59b99859e 100644 --- a/helper/osutil/fileinfo.go +++ b/helper/osutil/fileinfo.go @@ -64,3 +64,17 @@ func OwnerPermissionsMatch(path string, uid int, permissions int) error { return nil } + +// OwnerPermissionsMatchFile checks if vault user is the owner and permissions are secure for the input file +func OwnerPermissionsMatchFile(file *os.File, uid int, permissions int) error { + info, err := file.Stat() + if err != nil { + return fmt.Errorf("file stat error on path %q: %w", file.Name(), err) + } + err = checkPathInfo(info, file.Name(), uid, permissions) + if err != nil { + return err + } + + return nil +} diff --git a/helper/osutil/fileinfo_test.go b/helper/osutil/fileinfo_test.go index 0c77d4873..febd11966 100644 --- a/helper/osutil/fileinfo_test.go +++ b/helper/osutil/fileinfo_test.go @@ -4,6 +4,7 @@ import ( "io/fs" "os" "os/user" + "path/filepath" "runtime" "strconv" "testing" @@ -82,3 +83,98 @@ func TestCheckPathInfo(t *testing.T) { } } } + +// TestOwnerPermissionsMatchFile creates a file and verifies that the current user of the process is the owner of the +// file +func TestOwnerPermissionsMatchFile(t *testing.T) { + currentUser, err := user.Current() + if err != nil { + t.Fatal("failed to get current user", err) + } + uid, err := strconv.ParseInt(currentUser.Uid, 0, 64) + if err != nil { + t.Fatal("failed to convert uid", err) + } + dir := t.TempDir() + path := filepath.Join(dir, "foo") + f, err := os.Create(path) + if err != nil { + t.Fatal("failed to create test file", err) + } + defer f.Close() + + info, err := os.Stat(path) + if err != nil { + t.Fatal("failed to stat test file", err) + } + + if err := OwnerPermissionsMatchFile(f, int(uid), int(info.Mode())); err != nil { + t.Fatalf("expected no error but got %v", err) + } +} + +// TestOwnerPermissionsMatchFile_OtherUser creates a file using the user that started the current process and verifies +// that a different user is not the owner of the file +func TestOwnerPermissionsMatchFile_OtherUser(t *testing.T) { + currentUser, err := user.Current() + if err != nil { + t.Fatal("failed to get current user", err) + } + uid, err := strconv.ParseInt(currentUser.Uid, 0, 64) + if err != nil { + t.Fatal("failed to convert uid", err) + } + dir := t.TempDir() + path := filepath.Join(dir, "foo") + f, err := os.Create(path) + if err != nil { + t.Fatal("failed to create test file", err) + } + defer f.Close() + + info, err := os.Stat(path) + if err != nil { + t.Fatal("failed to stat test file", err) + } + + if err := OwnerPermissionsMatchFile(f, int(uid)+1, int(info.Mode())); err == nil { + t.Fatalf("expected error but none") + } +} + +// TestOwnerPermissionsMatchFile_Symlink creates a file and a symlink to that file. The test verifies that the current +// user of the process is the owner of the file +func TestOwnerPermissionsMatchFile_Symlink(t *testing.T) { + currentUser, err := user.Current() + if err != nil { + t.Fatal("failed to get current user", err) + } + uid, err := strconv.ParseInt(currentUser.Uid, 0, 64) + if err != nil { + t.Fatal("failed to convert uid", err) + } + dir := t.TempDir() + path := filepath.Join(dir, "foo") + f, err := os.Create(path) + if err != nil { + t.Fatal("failed to create test file", err) + } + defer f.Close() + + symlink := filepath.Join(dir, "symlink") + err = os.Symlink(path, symlink) + if err != nil { + t.Fatal("failed to symlink file", err) + } + symlinkedFile, err := os.Open(symlink) + if err != nil { + t.Fatal("failed to open file", err) + } + info, err := os.Stat(symlink) + if err != nil { + t.Fatal("failed to stat test file", err) + } + if err := OwnerPermissionsMatchFile(symlinkedFile, int(uid), int(info.Mode())); err != nil { + t.Fatalf("expected no error but got %v", err) + } +}