Merge pull request #241 from ZhenyuTan-amz/oschown-ownership-change

Change ownership of files excluding executable files and Suid Sgid files.

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 <dir>`, 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
 }

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)
 	}
 

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)
 	}
 

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)
+	}
+}