diff --git a/translator/cmdutil/userutil.go b/translator/cmdutil/userutil.go index baa7635d33..54005a0ed7 100644 --- a/translator/cmdutil/userutil.go +++ b/translator/cmdutil/userutil.go @@ -8,18 +8,23 @@ package cmdutil import ( "fmt" "log" - "os/exec" + "os" + "path/filepath" "github.com/aws/amazon-cloudwatch-agent/translator/config" "github.com/aws/amazon-cloudwatch-agent/translator/context" ) -const ( +var ( agentLogDir = "/opt/aws/amazon-cloudwatch-agent/logs" agentVarDir = "/opt/aws/amazon-cloudwatch-agent/var" agentEtcDir = "/opt/aws/amazon-cloudwatch-agent/etc" ) +type ChownFunc func(name string, uid, gid int) error + +var chown ChownFunc = os.Chown + // DetectRunAsUser get the user name from toml config. It runs on all platforms except windows. func DetectRunAsUser(mergedJsonConfigMap map[string]interface{}) (runAsUser string, err error) { fmt.Printf("I! Detecting run_as_user...\n") @@ -42,29 +47,66 @@ func DetectRunAsUser(mergedJsonConfigMap map[string]interface{}) (runAsUser stri } // changeFileOwner changes both user and group of a directory. -func changeFileOwner(runAsUser string, groupName string) error { - owner := runAsUser - if groupName != "" { - owner = owner + ":" + groupName - } else { - log.Print("W! Group name is empty, change user without group.") - } +func changeFileOwner(uid, gid int) error { dirs := []string{agentLogDir, agentEtcDir, agentVarDir} - log.Printf("I! Changing ownership of %v to %s", dirs, owner) + log.Printf("I! Changing ownership of %v to %v:%v", dirs, uid, gid) for _, d := range dirs { - if err := chownRecursive(owner, d); err != nil { + if err := chownRecursive(uid, gid, d); err != nil { return err } } return nil } -// chownRecursive shells out to chown -R -L -func chownRecursive(owner, dir string) error { - cmd := exec.Command("chown", "-R", "-L", owner, dir) - b, err := cmd.CombinedOutput() +// chownRecursive would recursively change the ownership of the directory +// similar to `chown -R `, except it will igore any files that are: +// - Executable +// - With SUID or SGID bit set +// - Allow anyone to write to +// - Symbolic links +// This would prevent any accidental ownership change to files that are executable +// or with special purpose to be changed to be owned by root when run_as_user option +// is removed from the configuration +func chownRecursive(uid, gid int, dir string) error { + + err := filepath.Walk(dir, func(path string, info os.FileInfo, err error) error { + if err != nil { + return err + } + + fmode := info.Mode() + if fmode.IsRegular() { + // Do not change ownership of files with SUID or SGID + if fmode&os.ModeSetuid != 0 || fmode&os.ModeSetgid != 0 { + return nil + } + + // Do not change ownership of executable files + // Perm() returns the lower 7 bit of permission of file, which represes rwxrwxrws + // 0111 maps to --x--x--x, so it would check any user have the execution right + if fmode.Perm()&0111 != 0 { + return nil + } + + // Do not change ownership of files that allow anyone to write to + // Prevents adding SUID and Executable bits after ownership change + if fmode.Perm()&0002 != 0 { + return nil + } + } + + if fmode&os.ModeSymlink != 0 { + return nil + } + + if err := chown(path, uid, gid); err != nil { + return err + } + return nil + }) + if err != nil { - return fmt.Errorf("error change owner of dir %s to %s: %w %s", dir, owner, err, b) + return fmt.Errorf("error change owner of dir %s to %v:%v due to error: %w", dir, uid, gid, err) } return nil } diff --git a/translator/cmdutil/userutil_darwin.go b/translator/cmdutil/userutil_darwin.go index 7bff74c030..94ec9fefa7 100644 --- a/translator/cmdutil/userutil_darwin.go +++ b/translator/cmdutil/userutil_darwin.go @@ -26,7 +26,7 @@ import ( const ( // dsbin is a cli for querying macOS directory service. - dsbin = "dscacheutil" + dsbin = "/usr/bin/dscacheutil" ) // dsLookup shells out to dscacheutil to get uid, gid from username. @@ -57,28 +57,6 @@ func dsLookup(username string) (*user.User, error) { return u, nil } -// dsLookupGroup shells out to dscacheutil to get group name from id. -// To get a gid for user, use dsLookup. -func dsLookupGroupId(gid string) (*user.Group, error) { - // dscacheutil -q group -a gid 2896053708 - // name: CloudWatch - // password: *** - // gid: 2896053708 - // - m, err := runDS("-q", "group", "-a", "gid", gid) - if err != nil { - return nil, err - } - g := &user.Group{ - Gid: m["gid"], - Name: m["name"], - } - if g.Gid == "" || g.Gid != gid { - return nil, user.UnknownGroupIdError(gid) - } - return g, nil -} - // runDS shells out query to dscacheutil and parse it to key value pair. func runDS(args ...string) (map[string]string, error) { b, err := exec.Command(dsbin, args...).CombinedOutput() @@ -147,11 +125,18 @@ func ChangeUser(mergedJsonConfigMap map[string]interface{}) (string, error) { return runAsUser, err } - g, err := dsLookupGroupId(execUser.Gid) + uid, err := strconv.Atoi(execUser.Uid) if err != nil { - return runAsUser, fmt.Errorf("error lookup group by id: %w", err) + return runAsUser, fmt.Errorf("UID %s cannot be converted to intger uid: %w", execUser.Uid, err) } - if err := changeFileOwner(runAsUser, g.Name); err != nil { + + gid, err := strconv.Atoi(execUser.Gid) + if err != nil { + log.Printf("W! GID %s cannot be converted to intger gid, not changing gid of files: %v", execUser.Gid, err) + gid = -1 // -1 means not changing the GUID, see: https://golang.org/pkg/os/#Chown + } + + if err := changeFileOwner(uid, gid); err != nil { return runAsUser, fmt.Errorf("error change ownership of dirs: %w", err) } diff --git a/translator/cmdutil/userutil_linux.go b/translator/cmdutil/userutil_linux.go index b90440eedc..bc4cd4ffbb 100644 --- a/translator/cmdutil/userutil_linux.go +++ b/translator/cmdutil/userutil_linux.go @@ -9,8 +9,6 @@ import ( "fmt" "log" "os" - gouser "os/user" - "strconv" "syscall" "golang.org/x/sys/unix" @@ -71,11 +69,7 @@ func ChangeUser(mergedJsonConfigMap map[string]interface{}) (user string, err er return runAsUser, err } - g, err := gouser.LookupGroupId(strconv.Itoa(execUser.Gid)) - if err != nil { - return runAsUser, fmt.Errorf("error lookup group by id: %w", err) - } - if err := changeFileOwner(runAsUser, g.Name); err != nil { + if err := changeFileOwner(execUser.Uid, execUser.Gid); err != nil { return runAsUser, fmt.Errorf("error change ownership of dirs: %w", err) } diff --git a/translator/cmdutil/userutil_test.go b/translator/cmdutil/userutil_test.go new file mode 100644 index 0000000000..d6d57d8c2c --- /dev/null +++ b/translator/cmdutil/userutil_test.go @@ -0,0 +1,116 @@ +// +build !windows + +package cmdutil + +import ( + "io/ioutil" + "os" + "path/filepath" + "reflect" + "testing" +) + +type MockChowner struct { + chowns []chownEvent +} + +type chownEvent struct { + path string + uid, gid int +} + +func (c *MockChowner) Chown(path string, uid, gid int) error { + c.chowns = append(c.chowns, chownEvent{path, uid, gid}) + return nil +} + +func TestChangeFileOwner(t *testing.T) { + base, err := ioutil.TempDir("", "testChown") + if err != nil { + t.Fatalf("failed to crate temp test folder: %v", err) + } + defer os.RemoveAll(base) // Post test clean up + + // Override the agent dirs + agentLogDir = filepath.Join(base, "logs") + agentVarDir = filepath.Join(base, "var") + agentEtcDir = filepath.Join(base, "etc") + + linkedTo := filepath.Join(base, "file-to-be-linked-to.txt") + createFile(t, linkedTo, 0644) + + // etc + createFile(t, filepath.Join(agentEtcDir, "amazon-cloudwatch-agent.json"), 0644) + createFile(t, filepath.Join(agentEtcDir, "amazon-cloudwatch-agent.toml"), 0644) + createFile(t, filepath.Join(agentEtcDir, "common-config.toml"), 0644) + createFile(t, filepath.Join(agentEtcDir, "amazon-cloudwatch-agent.d/default.json"), 0644) + + // var + createFile(t, filepath.Join(agentVarDir, "some-file-in-var"), 0644) + + // Log files + createFile(t, filepath.Join(agentLogDir, "amazon-cloudwatch-agent.log"), 0644) + createFile(t, filepath.Join(agentLogDir, "state/statefile"), 0644) + + // Files that should be excluded + createFile(t, filepath.Join(agentLogDir, "anyone-can-write.log"), 0666) + createFile(t, filepath.Join(agentLogDir, "suid-file.log"), 0644|os.ModeSetuid) + createFile(t, filepath.Join(agentLogDir, "sgid-file.log"), 0644|os.ModeSetgid) + createFile(t, filepath.Join(agentLogDir, "suid-and-sgid-file.log"), 0644|os.ModeSetuid|os.ModeSetgid) + createFile(t, filepath.Join(agentLogDir, "owner-executable.log"), 0744) + createFile(t, filepath.Join(agentLogDir, "group-executable.log"), 0654) + createFile(t, filepath.Join(agentLogDir, "other-executable.log"), 0645) + createFile(t, filepath.Join(agentLogDir, "all-executable.log"), 0755) + createSymlink(t, linkedTo, filepath.Join(agentLogDir, "link-to-other-file")) + + // Call changeFileOwner + var mc MockChowner + chown = mc.Chown + const someUid, someGid = 1111, 9999 + changeFileOwner(someUid, someGid) + + expected := []chownEvent{ + {filepath.Join(base, "logs"), someUid, someGid}, + {filepath.Join(base, "logs/amazon-cloudwatch-agent.log"), someUid, someGid}, + {filepath.Join(base, "logs/state"), someUid, someGid}, + {filepath.Join(base, "logs/state/statefile"), someUid, someGid}, + {filepath.Join(base, "etc"), someUid, someGid}, + {filepath.Join(base, "etc/amazon-cloudwatch-agent.d"), someUid, someGid}, + {filepath.Join(base, "etc/amazon-cloudwatch-agent.d/default.json"), someUid, someGid}, + {filepath.Join(base, "etc/amazon-cloudwatch-agent.json"), someUid, someGid}, + {filepath.Join(base, "etc/amazon-cloudwatch-agent.toml"), someUid, someGid}, + {filepath.Join(base, "etc/common-config.toml"), someUid, someGid}, + {filepath.Join(base, "var"), someUid, someGid}, + {filepath.Join(base, "var/some-file-in-var"), someUid, someGid}, + } + + if !reflect.DeepEqual(mc.chowns, expected) { + t.Errorf("wrong files has been changed ownership, expecting\n%v\n\n but got\n%v", expected, mc.chowns) + } +} + +func mkdir(t *testing.T, path string) { + if err := os.MkdirAll(path, 0755); err != nil { + t.Fatalf("failed to create dir %v: %v", path, err) + } +} + +func createFile(t *testing.T, path string, mode os.FileMode) { + dir, _ := filepath.Split(path) + mkdir(t, dir) + f, err := os.Create(path) + if err != nil { + t.Fatalf("failed to create file %v: %v", path, err) + } + f.Close() + + if err := os.Chmod(path, mode); err != nil { + t.Fatalf("failed to change file mode of %v to mode %o: %v", path, mode, err) + } +} + +func createSymlink(t *testing.T, from, to string) { + if err := os.Symlink(from, to); err != nil { + t.Fatalf("failed to create symbolic link from %v to %v: %v", from, to, err) + } +}