Guard against malformed /etc/group and /etc/passwd lines

pkg/assessor/group/group.go

@@ -30,7 +30,14 @@ func (a GroupAssessor) Assess(fileMap types.FileMap) ([]*types.Assessment, error
 
 		for scanner.Scan() {
 			line := scanner.Text()
+			if len(line) == 0 || line[0] == '#' {
+				continue
+			}
 			data := strings.Split(line, ":")
+			if len(data) < 3 {
+				log.Logger.Debug("The group format may be invalid.", line)
+				continue
+			}
 			gname := data[0]
 			gid := data[2]
 

pkg/assessor/group/group_test.go

@@ -0,0 +1,53 @@
+package group
+
+import (
+	"os"
+	"testing"
+
+	"github.com/goodwithtech/dockle/pkg/log"
+	"github.com/goodwithtech/dockle/pkg/types"
+)
+
+func TestMain(m *testing.M) {
+	if err := log.InitLogger(false, true); err != nil {
+		panic(err)
+	}
+	os.Exit(m.Run())
+}
+
+func TestAssessMalformedLines(t *testing.T) {
+	// etc/group body with a valid row, a blank line, a comment line and a
+	// short/truncated entry. The short and blank lines used to panic on data[2].
+	body := "root:x:0:\n\n# a comment\ndaemon:x:1:\nbroken\nalsobroken:x\n"
+	fileMap := types.FileMap{
+		"etc/group": types.FileData{Body: []byte(body)},
+	}
+
+	assessor := GroupAssessor{}
+	if _, err := assessor.Assess(fileMap); err != nil {
+		t.Fatalf("Assess returned an error: %v", err)
+	}
+}
+
+func TestAssessDuplicateGID(t *testing.T) {
+	// two rows share GID 0, so a duplicate-group assessment is expected.
+	body := "root:x:0:\ndup:x:0:\n"
+	fileMap := types.FileMap{
+		"etc/group": types.FileData{Body: []byte(body)},
+	}
+
+	assessor := GroupAssessor{}
+	assesses, err := assessor.Assess(fileMap)
+	if err != nil {
+		t.Fatalf("Assess returned an error: %v", err)
+	}
+	found := false
+	for _, a := range assesses {
+		if a.Code == types.AvoidDuplicateUserGroup && a.Filename == "etc/group" {
+			found = true
+		}
+	}
+	if !found {
+		t.Fatalf("expected a duplicate group assessment, got %v", assesses)
+	}
+}

pkg/assessor/user/user.go

@@ -28,7 +28,14 @@ func (a UserAssessor) Assess(fileMap types.FileMap) ([]*types.Assessment, error)
 		uidMap := map[string]struct{}{}
 		for scanner.Scan() {
 			line := scanner.Text()
+			if len(line) == 0 || line[0] == '#' {
+				continue
+			}
 			data := strings.Split(line, ":")
+			if len(data) < 3 {
+				log.Logger.Debug("The passwd format may be invalid.", line)
+				continue
+			}
 			uname := data[0]
 			uid := data[2]
 

pkg/assessor/user/user_test.go

@@ -0,0 +1,53 @@
+package user
+
+import (
+	"os"
+	"testing"
+
+	"github.com/goodwithtech/dockle/pkg/log"
+	"github.com/goodwithtech/dockle/pkg/types"
+)
+
+func TestMain(m *testing.M) {
+	if err := log.InitLogger(false, true); err != nil {
+		panic(err)
+	}
+	os.Exit(m.Run())
+}
+
+func TestAssessMalformedLines(t *testing.T) {
+	// etc/passwd body with a valid row, a blank line, a comment line and a
+	// short/truncated entry. The short and blank lines used to panic on data[2].
+	body := "root:x:0:0:root:/root:/bin/sh\n\n# a comment\ndaemon:x:1:1:daemon:/:/sbin/nologin\nbroken\nalsobroken:x\n"
+	fileMap := types.FileMap{
+		"etc/passwd": types.FileData{Body: []byte(body)},
+	}
+
+	assessor := UserAssessor{}
+	if _, err := assessor.Assess(fileMap); err != nil {
+		t.Fatalf("Assess returned an error: %v", err)
+	}
+}
+
+func TestAssessDuplicateUID(t *testing.T) {
+	// two rows share UID 0, so a duplicate-user assessment is expected.
+	body := "root:x:0:0:root:/root:/bin/sh\ndup:x:0:0:dup:/:/bin/sh\n"
+	fileMap := types.FileMap{
+		"etc/passwd": types.FileData{Body: []byte(body)},
+	}
+
+	assessor := UserAssessor{}
+	assesses, err := assessor.Assess(fileMap)
+	if err != nil {
+		t.Fatalf("Assess returned an error: %v", err)
+	}
+	found := false
+	for _, a := range assesses {
+		if a.Code == types.AvoidDuplicateUserGroup && a.Filename == "etc/passwd" {
+			found = true
+		}
+	}
+	if !found {
+		t.Fatalf("expected a duplicate user assessment, got %v", assesses)
+	}
+}