From 69af385de62ea68e2e608335cffbb0f4aa3db091 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Wed, 30 Mar 2016 22:48:07 +1100 Subject: [PATCH 1/3] libcontainer: user: always treat numeric ids numerically Most shadow-related tools don't treat numeric ids as potential usernames, so change our behaviour to match that. Previously, using an explicit specification like 111:222 could result in the UID and GID not being 111 and 222 respectively (which is confusing). Signed-off-by: Aleksa Sarai --- libcontainer/user/user.go | 91 +++++++++++++++++++++++++-------------- 1 file changed, 58 insertions(+), 33 deletions(-) diff --git a/libcontainer/user/user.go b/libcontainer/user/user.go index e6375ea4..9f4c2cb5 100644 --- a/libcontainer/user/user.go +++ b/libcontainer/user/user.go @@ -235,10 +235,14 @@ func GetExecUserPath(userSpec string, defaults *ExecUser, passwdPath, groupPath // * "uid:gid // * "user:gid" // * "uid:group" +// +// It should be noted that if you specify a numeric user or group id, they will +// not be evaluated as usernames (only the metadata will be filled). So attempting +// to parse a user with user.Name = "1337" will produce the user with a UID of +// 1337. func GetExecUser(userSpec string, defaults *ExecUser, passwd, group io.Reader) (*ExecUser, error) { var ( userArg, groupArg string - name string ) if defaults == nil { @@ -261,11 +265,22 @@ func GetExecUser(userSpec string, defaults *ExecUser, passwd, group io.Reader) ( // allow for userArg to have either "user" syntax, or optionally "user:group" syntax parseLine(userSpec, &userArg, &groupArg) + // Convert userArg and groupArg to be numeric, so we don't have to execute + // Atoi *twice* for each iteration over lines. + uidArg, uidErr := strconv.Atoi(userArg) + gidArg, gidErr := strconv.Atoi(groupArg) + users, err := ParsePasswdFilter(passwd, func(u User) bool { if userArg == "" { return u.Uid == user.Uid } - return u.Name == userArg || strconv.Itoa(u.Uid) == userArg + + if uidErr == nil { + // If the userArg is numeric, always treat it as a UID. + return uidArg == u.Uid + } + + return u.Name == userArg }) if err != nil && passwd != nil { if userArg == "" { @@ -274,47 +289,55 @@ func GetExecUser(userSpec string, defaults *ExecUser, passwd, group io.Reader) ( return nil, fmt.Errorf("Unable to find user %v: %v", userArg, err) } - haveUser := users != nil && len(users) > 0 - if haveUser { - // if we found any user entries that matched our filter, let's take the first one as "correct" - name = users[0].Name + var matchedUserName string + if len(users) > 0 { + // First match wins, even if there's more than one matching entry. + matchedUserName = users[0].Name user.Uid = users[0].Uid user.Gid = users[0].Gid user.Home = users[0].Home } else if userArg != "" { - // we asked for a user but didn't find them... let's check to see if we wanted a numeric user - user.Uid, err = strconv.Atoi(userArg) - if err != nil { - // not numeric - we have to bail - return nil, fmt.Errorf("Unable to find user %v", userArg) + // If we can't find a user with the given username, the only other valid + // option is if it's a numeric username with no associated entry in passwd. + + if uidErr != nil { + // Not numeric. + return nil, fmt.Errorf("unable to find user %s: %v", userArg, ErrNoPasswdEntries) } + user.Uid = uidArg // Must be inside valid uid range. if user.Uid < minId || user.Uid > maxId { return nil, ErrRange } - // if userArg couldn't be found in /etc/passwd but is numeric, just roll with it - this is legit + // Okay, so it's numeric. We can just roll with this. } - if groupArg != "" || name != "" { + // On to the groups. If we matched a username, we need to do this because of + // the supplementary group IDs. + if groupArg != "" || matchedUserName != "" { groups, err := ParseGroupFilter(group, func(g Group) bool { - // Explicit group format takes precedence. - if groupArg != "" { - return g.Name == groupArg || strconv.Itoa(g.Gid) == groupArg - } - - // Check if user is a member. - for _, u := range g.List { - if u == name { - return true + // If the group argument isn't explicit, we'll just search for it. + if groupArg == "" { + // Check if user is a member of this group. + for _, u := range g.List { + if u == matchedUserName { + return true + } } + return false } - return false + if gidErr == nil { + // If the groupArg is numeric, always treat it as a GID. + return gidArg == g.Gid + } + + return g.Name == groupArg }) if err != nil && group != nil { - return nil, fmt.Errorf("Unable to find groups for user %v: %v", users[0].Name, err) + return nil, fmt.Errorf("unable to find groups for spec %v: %v", matchedUserName, err) } haveGroup := groups != nil && len(groups) > 0 @@ -322,23 +345,25 @@ func GetExecUser(userSpec string, defaults *ExecUser, passwd, group io.Reader) ( if haveGroup { // if we found any group entries that matched our filter, let's take the first one as "correct" user.Gid = groups[0].Gid - } else { - // we asked for a group but didn't find id... let's check to see if we wanted a numeric group - user.Gid, err = strconv.Atoi(groupArg) - if err != nil { - // not numeric - we have to bail - return nil, fmt.Errorf("Unable to find group %v", groupArg) + } else if groupArg != "" { + // If we can't find a group with the given name, the only other valid + // option is if it's a numeric group name with no associated entry in group. + + if gidErr != nil { + // Not numeric. + return nil, fmt.Errorf("unable to find group %s: %v", groupArg, ErrNoGroupEntries) } + user.Gid = gidArg // Ensure gid is inside gid range. if user.Gid < minId || user.Gid > maxId { return nil, ErrRange } - // if groupArg couldn't be found in /etc/group but is numeric, just roll with it - this is legit + // Okay, so it's numeric. We can just roll with this. } - } else if haveGroup { - // If implicit group format, fill supplementary gids. + } else if len(groups) > 0 { + // Supplementary group ids only make sense if in the implicit form. user.Sgids = make([]int, len(groups)) for i, group := range groups { user.Sgids[i] = group.Gid From 4468dd58909888dd939e232ffa1d5db04d7365e4 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Wed, 30 Mar 2016 22:44:04 +1100 Subject: [PATCH 2/3] libcontainer: user: add tests for numeric user specifications Signed-off-by: Aleksa Sarai --- libcontainer/user/user_test.go | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/libcontainer/user/user_test.go b/libcontainer/user/user_test.go index 53b2289b..8cad2f55 100644 --- a/libcontainer/user/user_test.go +++ b/libcontainer/user/user_test.go @@ -101,12 +101,16 @@ func TestValidGetExecUser(t *testing.T) { const passwdContent = ` 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::::: this is just some garbage data ` const groupContent = ` root:x:0:root adm:x:43: grp:x:1234:root,adm +444:x:555:111 +odd:x:444: this is just some garbage data ` defaultExecUser := ExecUser{ @@ -192,6 +196,26 @@ this is just some garbage data Home: defaultExecUser.Home, }, }, + + // Regression tests for #695. + { + ref: "111", + expected: ExecUser{ + Uid: 111, + Gid: 112, + Sgids: defaultExecUser.Sgids, + Home: "/home/odd", + }, + }, + { + ref: "111:444", + expected: ExecUser{ + Uid: 111, + Gid: 444, + Sgids: defaultExecUser.Sgids, + Home: "/home/odd", + }, + }, } for _, test := range tests { @@ -206,6 +230,7 @@ this is just some garbage data } if !reflect.DeepEqual(test.expected, *execUser) { + t.Logf("ref: %v", test.ref) t.Logf("got: %#v", execUser) t.Logf("expected: %#v", test.expected) t.Fail() @@ -218,6 +243,7 @@ func TestInvalidGetExecUser(t *testing.T) { const passwdContent = ` root:x:0:0:root user:/root:/bin/bash adm:x:42:43:adm:/var/adm:/bin/false +-42:x:12:13:broken:/very/broken this is just some garbage data ` const groupContent = ` @@ -240,6 +266,8 @@ this is just some garbage data "-1:0", "0:-3", "-5:-2", + "-42", + "-43", } for _, test := range tests { From 3cfff676b10eb618cca873159b7c277e89b3e593 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Wed, 30 Mar 2016 22:50:14 +1100 Subject: [PATCH 3/3] libcontainer: user: general cleanups Some of the code was quite confusing inside libcontainer/user, so refactor and comment it so future maintainers can understand what's going and what edge cases we have to deal with. Signed-off-by: Aleksa Sarai --- libcontainer/user/user.go | 60 +++++++++++++++++++-------------------- 1 file changed, 29 insertions(+), 31 deletions(-) diff --git a/libcontainer/user/user.go b/libcontainer/user/user.go index 9f4c2cb5..43fd39ef 100644 --- a/libcontainer/user/user.go +++ b/libcontainer/user/user.go @@ -15,7 +15,7 @@ const ( ) var ( - ErrRange = fmt.Errorf("Uids and gids must be in range %d-%d", minId, maxId) + ErrRange = fmt.Errorf("uids and gids must be in range %d-%d", minId, maxId) ) type User struct { @@ -42,29 +42,30 @@ func parseLine(line string, v ...interface{}) { parts := strings.Split(line, ":") for i, p := range parts { + // Ignore cases where we don't have enough fields to populate the arguments. + // Some configuration files like to misbehave. if len(v) <= i { - // if we have more "parts" than we have places to put them, bail for great "tolerance" of naughty configuration files break } + // Use the type of the argument to figure out how to parse it, scanf() style. + // This is legit. switch e := v[i].(type) { case *string: - // "root", "adm", "/bin/bash" *e = p case *int: - // "0", "4", "1000" - // ignore string to int conversion errors, for great "tolerance" of naughty configuration files + // "numbers", with conversion errors ignored because of some misbehaving configuration files. *e, _ = strconv.Atoi(p) case *[]string: - // "", "root", "root,adm,daemon" + // Comma-separated lists. if p != "" { *e = strings.Split(p, ",") } else { *e = []string{} } default: - // panic, because this is a programming/logic error, not a runtime one - panic("parseLine expects only pointers! argument " + strconv.Itoa(i) + " is not a pointer!") + // Someone goof'd when writing code using this function. Scream so they can hear us. + panic(fmt.Sprintf("parseLine only accepts {*string, *int, *[]string} as arguments! %#v is not a pointer!", e)) } } } @@ -106,8 +107,8 @@ func ParsePasswdFilter(r io.Reader, filter func(User) bool) ([]User, error) { return nil, err } - text := strings.TrimSpace(s.Text()) - if text == "" { + line := strings.TrimSpace(s.Text()) + if line == "" { continue } @@ -117,10 +118,7 @@ func ParsePasswdFilter(r io.Reader, filter func(User) bool) ([]User, error) { // root:x:0:0:root:/root:/bin/bash // adm:x:3:4:adm:/var/adm:/bin/false p := User{} - parseLine( - text, - &p.Name, &p.Pass, &p.Uid, &p.Gid, &p.Gecos, &p.Home, &p.Shell, - ) + parseLine(line, &p.Name, &p.Pass, &p.Uid, &p.Gid, &p.Gecos, &p.Home, &p.Shell) if filter == nil || filter(p) { out = append(out, p) @@ -135,6 +133,7 @@ func ParseGroupFile(path string) ([]Group, error) { if err != nil { return nil, err } + defer group.Close() return ParseGroup(group) } @@ -178,10 +177,7 @@ 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(text, &p.Name, &p.Pass, &p.Gid, &p.List) if filter == nil || filter(p) { out = append(out, p) @@ -192,9 +188,10 @@ func ParseGroupFilter(r io.Reader, filter func(Group) bool) ([]Group, error) { } type ExecUser struct { - Uid, Gid int - Sgids []int - Home string + Uid int + Gid int + Sgids []int + Home string } // GetExecUserPath is a wrapper for GetExecUser. It reads data from each of the @@ -241,10 +238,6 @@ func GetExecUserPath(userSpec string, defaults *ExecUser, passwdPath, groupPath // to parse a user with user.Name = "1337" will produce the user with a UID of // 1337. func GetExecUser(userSpec string, defaults *ExecUser, passwd, group io.Reader) (*ExecUser, error) { - var ( - userArg, groupArg string - ) - if defaults == nil { defaults = new(ExecUser) } @@ -262,7 +255,8 @@ func GetExecUser(userSpec string, defaults *ExecUser, passwd, group io.Reader) ( user.Sgids = []int{} } - // allow for userArg to have either "user" syntax, or optionally "user:group" syntax + // Allow for userArg to have either "user" syntax, or optionally "user:group" syntax + var userArg, groupArg string parseLine(userSpec, &userArg, &groupArg) // Convert userArg and groupArg to be numeric, so we don't have to execute @@ -270,8 +264,10 @@ func GetExecUser(userSpec string, defaults *ExecUser, passwd, group io.Reader) ( uidArg, uidErr := strconv.Atoi(userArg) gidArg, gidErr := strconv.Atoi(groupArg) + // Find the matching user. users, err := ParsePasswdFilter(passwd, func(u User) bool { if userArg == "" { + // Default to current state of the user. return u.Uid == user.Uid } @@ -282,11 +278,13 @@ func GetExecUser(userSpec string, defaults *ExecUser, passwd, group io.Reader) ( return u.Name == userArg }) + + // If we can't find the user, we have to bail. if err != nil && passwd != nil { if userArg == "" { userArg = strconv.Itoa(user.Uid) } - return nil, fmt.Errorf("Unable to find user %v: %v", userArg, err) + return nil, fmt.Errorf("unable to find user %s: %v", userArg, err) } var matchedUserName string @@ -340,10 +338,10 @@ func GetExecUser(userSpec string, defaults *ExecUser, passwd, group io.Reader) ( return nil, fmt.Errorf("unable to find groups for spec %v: %v", matchedUserName, err) } - haveGroup := groups != nil && len(groups) > 0 + // Only start modifying user.Gid if it is in explicit form. if groupArg != "" { - if haveGroup { - // if we found any group entries that matched our filter, let's take the first one as "correct" + if len(groups) > 0 { + // First match wins, even if there's more than one matching entry. user.Gid = groups[0].Gid } else if groupArg != "" { // If we can't find a group with the given name, the only other valid @@ -355,7 +353,7 @@ func GetExecUser(userSpec string, defaults *ExecUser, passwd, group io.Reader) ( } user.Gid = gidArg - // Ensure gid is inside gid range. + // Must be inside valid gid range. if user.Gid < minId || user.Gid > maxId { return nil, ErrRange }