From 20c03a55f6dc0b087dec06e4178d97095c008244 Mon Sep 17 00:00:00 2001 From: Arpit Jain Date: Sun, 19 Jul 2026 00:57:16 +0900 Subject: [PATCH] fix: guard against malformed /etc/group and /etc/passwd lines The group and user assessors split each line on ':' and read data[2] without checking the length first, so a blank line, a comment line, or any entry with fewer than three fields in a scanned image's /etc/group or /etc/passwd panics dockle with an index out of range. The passwd assessor already skips blank/comment lines and length-checks before indexing. Apply the same guard to group.go and user.go, and add tests covering the malformed-line cases and the existing duplicate GID/UID detection. Signed-off-by: Arpit Jain --- pkg/assessor/group/group.go | 7 +++++ pkg/assessor/group/group_test.go | 53 ++++++++++++++++++++++++++++++++ pkg/assessor/user/user.go | 7 +++++ pkg/assessor/user/user_test.go | 53 ++++++++++++++++++++++++++++++++ 4 files changed, 120 insertions(+) create mode 100644 pkg/assessor/group/group_test.go create mode 100644 pkg/assessor/user/user_test.go diff --git a/pkg/assessor/group/group.go b/pkg/assessor/group/group.go index 9fa756e..b9326e9 100644 --- a/pkg/assessor/group/group.go +++ b/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] diff --git a/pkg/assessor/group/group_test.go b/pkg/assessor/group/group_test.go new file mode 100644 index 0000000..39e43a4 --- /dev/null +++ b/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) + } +} diff --git a/pkg/assessor/user/user.go b/pkg/assessor/user/user.go index d3b1974..4c40317 100644 --- a/pkg/assessor/user/user.go +++ b/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] diff --git a/pkg/assessor/user/user_test.go b/pkg/assessor/user/user_test.go new file mode 100644 index 0000000..3e0dd43 --- /dev/null +++ b/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) + } +}