Repository: opencontainers/runc
Pull request #3079 (2021-07-08 20:20:50)

[1.0] libct/user: fix parsing long /etc/group lines
_This is a backport of #3062 to 1.0 branch. Clean cherry-pick, no issues. Original description follows._

Lines in /etc/group longer than 64 characters breaks the current
implementation of group parser. This is caused by bufio.Scanner
buffer limit.
    
Fix by re-using the fix for a similar problem in golang os/user,
namely https://go-review.googlesource.com/c/go/+/283601
(fixing a similar bug in golang os/user, https://github.com/golang/go/issues/43636).
    
Add some tests.

Fixes: #3036 
   
Reported-by: @erict-square 
Co-authored-by: @andreybokhanko
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

## Proposed changelog entry
```
Bugfixes:
* Fixed "unable to find groups ... token too long" error with /etc/group containing lines longer than 64K characters (#3036)
```


Patch:
diff --git a/libcontainer/user/user.go b/libcontainer/user/user.go
index d2c16f7f..cc7a106b 100644
--- a/libcontainer/user/user.go
+++ b/libcontainer/user/user.go
@@ -2,6 +2,7 @@ package user
 
 import (
 	"bufio"
+	"bytes"
 	"errors"
 	"fmt"
 	"io"
@@ -55,11 +56,11 @@ type IDMap struct {
 	Count    int64
 }
 
-func parseLine(line string, v ...interface{}) {
-	parseParts(strings.Split(line, ":"), v...)
+func parseLine(line []byte, v ...interface{}) {
+	parseParts(bytes.Split(line, []byte(":")), v...)
 }
 
-func parseParts(parts []string, v ...interface{}) {
+func parseParts(parts [][]byte, v ...interface{}) {
 	if len(parts) == 0 {
 		return
 	}
@@ -75,16 +76,16 @@ func parseParts(parts []string, v ...interface{}) {
 		// This is legit.
 		switch e := v[i].(type) {
 		case *string:
-			*e = p
+			*e = string(p)
 		case *int:
 			// "numbers", with conversion errors ignored because of some misbehaving configuration files.
-			*e, _ = strconv.Atoi(p)
+			*e, _ = strconv.Atoi(string(p))
 		case *int64:
-			*e, _ = strconv.ParseInt(p, 10, 64)
+			*e, _ = strconv.ParseInt(string(p), 10, 64)
 		case *[]string:
 			// Comma-separated lists.
-			if p != "" {
-				*e = strings.Split(p, ",")
+			if len(p) != 0 {
+				*e = strings.Split(string(p), ",")
 			} else {
 				*e = []string{}
 			}
@@ -128,8 +129,8 @@ func ParsePasswdFilter(r io.Reader, filter func(User) bool) ([]User, error) {
 	)
 
 	for s.Scan() {
-		line := strings.TrimSpace(s.Text())
-		if line == "" {
+		line := bytes.TrimSpace(s.Bytes())
+		if len(line) == 0 {
 			continue
 		}
 
@@ -179,15 +180,53 @@ func ParseGroupFilter(r io.Reader, filter func(Group) bool) ([]Group, error) {
 	if r == nil {
 		return nil, fmt.Errorf("nil source for group-formatted data")
 	}
+	rd := bufio.NewReader(r)
+	out := []Group{}
 
-	var (
-		s   = bufio.NewScanner(r)
-		out = []Group{}
-	)
+	// Read the file line-by-line.
+	for {
+		var (
+			isPrefix  bool
+			wholeLine []byte
+			err       error
+		)
 
-	for s.Scan() {
-		text := s.Text()
-		if text == "" {
+		// Read the next line. We do so in chunks (as much as reader's
+		// buffer is able to keep), check if we read enough columns
+		// already on each step and store final result in wholeLine.
+		for {
+			var line []byte
+			line, isPrefix, err = rd.ReadLine()
+
+			if err != nil {
+				// We should return no error if EOF is reached
+				// without a match.
+				if err == io.EOF { //nolint:errorlint // comparison with io.EOF is legit, https://github.com/polyfloyd/go-errorlint/pull/12
+					err = nil
+				}
+				return out, err
+			}
+
+			// Simple common case: line is short enough to fit in a
+			// single reader's buffer.
+			if !isPrefix && len(wholeLine) == 0 {
+				wholeLine = line
+				break
+			}
+
+			wholeLine = append(wholeLine, line...)
+
+			// Check if we read the whole line already.
+			if !isPrefix {
+				break
+			}
+		}
+
+		// There's no spec for /etc/passwd or /etc/group, but we try to follow
+		// the same rules as the glibc parser, which allows comments and blank
+		// space at the beginning of a line.
+		wholeLine = bytes.TrimSpace(wholeLine)
+		if len(wholeLine) == 0 || wholeLine[0] == '#' {
 			continue
 		}
 
@@ -197,17 +236,12 @@ func ParseGroupFilter(r io.Reader, filter func(Group) bool) ([]Group, error) {
 		//  root:x:0:root
 		//  adm:x:4:root,adm,daemon
 		p := Group{}
-		parseLine(text, &p.Name, &p.Pass, &p.Gid, &p.List)
+		parseLine(wholeLine, &p.Name, &p.Pass, &p.Gid, &p.List)
 
 		if filter == nil || filter(p) {
 			out = append(out, p)
 		}
 	}
-	if err := s.Err(); err != nil {
-		return nil, err
-	}
-
-	return out, nil
 }
 
 type ExecUser struct {
@@ -278,7 +312,7 @@ func GetExecUser(userSpec string, defaults *ExecUser, passwd, group io.Reader) (
 
 	// Allow for userArg to have either "user" syntax, or optionally "user:group" syntax
 	var userArg, groupArg string
-	parseLine(userSpec, &userArg, &groupArg)
+	parseLine([]byte(userSpec), &userArg, &groupArg)
 
 	// Convert userArg and groupArg to be numeric, so we don't have to execute
 	// Atoi *twice* for each iteration over lines.
@@ -496,8 +530,8 @@ func ParseSubIDFilter(r io.Reader, filter func(SubID) bool) ([]SubID, error) {
 	)
 
 	for s.Scan() {
-		line := strings.TrimSpace(s.Text())
-		if line == "" {
+		line := bytes.TrimSpace(s.Bytes())
+		if len(line) == 0 {
 			continue
 		}
 
@@ -549,14 +583,14 @@ func ParseIDMapFilter(r io.Reader, filter func(IDMap) bool) ([]IDMap, error) {
 	)
 
 	for s.Scan() {
-		line := strings.TrimSpace(s.Text())
-		if line == "" {
+		line := bytes.TrimSpace(s.Bytes())
+		if len(line) == 0 {
 			continue
 		}
 
 		// see: man 7 user_namespaces
 		p := IDMap{}
-		parseParts(strings.Fields(line), &p.ID, &p.ParentID, &p.Count)
+		parseParts(bytes.Fields(line), &p.ID, &p.ParentID, &p.Count)
 
 		if filter == nil || filter(p) {
 			out = append(out, p)


Test patch:
diff --git a/libcontainer/user/user_test.go b/libcontainer/user/user_test.go
index 23bf4667..c0c762d6 100644
--- a/libcontainer/user/user_test.go
+++ b/libcontainer/user/user_test.go
@@ -1,6 +1,7 @@
 package user
 
 import (
+	"fmt"
 	"io"
 	"reflect"
 	"sort"
@@ -16,42 +17,42 @@ func TestUserParseLine(t *testing.T) {
 		d    int
 	)
 
-	parseLine("", &a, &b)
+	parseLine([]byte(""), &a, &b)
 	if a != "" || b != "" {
 		t.Fatalf("a and b should be empty ('%v', '%v')", a, b)
 	}
 
-	parseLine("a", &a, &b)
+	parseLine([]byte("a"), &a, &b)
 	if a != "a" || b != "" {
 		t.Fatalf("a should be 'a' and b should be empty ('%v', '%v')", a, b)
 	}
 
-	parseLine("bad boys:corny cows", &a, &b)
+	parseLine([]byte("bad boys:corny cows"), &a, &b)
 	if a != "bad boys" || b != "corny cows" {
 		t.Fatalf("a should be 'bad boys' and b should be 'corny cows' ('%v', '%v')", a, b)
 	}
 
-	parseLine("", &c)
+	parseLine([]byte(""), &c)
 	if len(c) != 0 {
 		t.Fatalf("c should be empty (%#v)", c)
 	}
 
-	parseLine("d,e,f:g:h:i,j,k", &c, &a, &b, &c)
+	parseLine([]byte("d,e,f:g:h:i,j,k"), &c, &a, &b, &c)
 	if a != "g" || b != "h" || len(c) != 3 || c[0] != "i" || c[1] != "j" || c[2] != "k" {
 		t.Fatalf("a should be 'g', b should be 'h', and c should be ['i','j','k'] ('%v', '%v', '%#v')", a, b, c)
 	}
 
-	parseLine("::::::::::", &a, &b, &c)
+	parseLine([]byte("::::::::::"), &a, &b, &c)
 	if a != "" || b != "" || len(c) != 0 {
 		t.Fatalf("a, b, and c should all be empty ('%v', '%v', '%#v')", a, b, c)
 	}
 
-	parseLine("not a number", &d)
+	parseLine([]byte("not a number"), &d)
 	if d != 0 {
 		t.Fatalf("d should be 0 (%v)", d)
 	}
 
-	parseLine("b:12:c", &a, &d, &b)
+	parseLine([]byte("b:12:c"), &a, &d, &b)
 	if a != "b" || b != "c" || d != 12 {
 		t.Fatalf("a should be 'b' and b should be 'c', and d should be 12 ('%v', '%v', %v)", a, b, d)
 	}
@@ -82,12 +83,12 @@ func TestUserParseGroup(t *testing.T) {
 root:x:0:root
 adm:x:4:root,adm,daemon
 this is just some garbage data
-`), nil)
+`+largeGroup()), nil)
 	if err != nil {
 		t.Fatalf("Unexpected error: %v", err)
 	}
-	if len(groups) != 3 {
-		t.Fatalf("Expected 3 groups, got %v", len(groups))
+	if len(groups) != 4 {
+		t.Fatalf("Expected 4 groups, got %v", len(groups))
 	}
 	if groups[0].Gid != 0 || groups[0].Name != "root" || len(groups[0].List) != 1 {
 		t.Fatalf("Expected groups[0] to be 0 - root - 1 member, got %v - %v - %v", groups[0].Gid, groups[0].Name, len(groups[0].List))
@@ -103,16 +104,18 @@ root:x:0:0:root user:/root:/bin/bash
 adm:x:42:43:adm:/var/adm:/bin/false
 111:x:222:333::/var/garbage
 odd:x:111:112::/home/odd:::::
+user7456:x:7456:100:Vasya:/home/user7456
 this is just some garbage data
 `
-	const groupContent = `
+	groupContent := `
 root:x:0:root
 adm:x:43:
-grp:x:1234:root,adm
+grp:x:1234:root,adm,user7456
 444:x:555:111
 odd:x:444:
 this is just some garbage data
-`
+` + largeGroup()
+
 	defaultExecUser := ExecUser{
 		Uid:   8888,
 		Gid:   8888,
@@ -216,6 +219,16 @@ this is just some garbage data
 				Home:  "/home/odd",
 			},
 		},
+		// Test for #3036.
+		{
+			ref: "7456",
+			expected: ExecUser{
+				Uid:   7456,
+				Gid:   100,
+				Sgids: []int{1234, 1000}, // 1000 is largegroup GID
+				Home:  "/home/user7456",
+			},
+		},
 	}
 
 	for _, test := range tests {
@@ -388,13 +401,13 @@ func TestGetAdditionalGroups(t *testing.T) {
 		hasError bool
 	}
 
-	const groupContent = `
+	groupContent := `
 root:x:0:root
 adm:x:43:
 grp:x:1234:root,adm
 adm:x:4343:root,adm-duplicate
 this is just some garbage data
-`
+` + largeGroup()
 	tests := []foo{
 		{
 			// empty group
@@ -444,6 +457,11 @@ this is just some garbage data
 			expected: nil,
 			hasError: true,
 		},
+		{
+			// group with very long list of users
+			groups:   []string{"largegroup"},
+			expected: []int{1000},
+		},
 	}
 
 	for _, test := range tests {
@@ -500,3 +518,13 @@ func TestGetAdditionalGroupsNumeric(t *testing.T) {
 		}
 	}
 }
+
+// Generate a proper "largegroup" entry for group tests.
+func largeGroup() (res string) {
+	var b strings.Builder
+	b.WriteString("largegroup:x:1000:user1")
+	for i := 2; i <= 7500; i++ {
+		fmt.Fprintf(&b, ",user%d", i)
+	}
+	return b.String()
+}