osutil/group.go: treat all non-nil errs from user.Lookup{Group,} as Unknown* #9041

anonymouse64 · 2020-07-21T19:26:44Z

Unfortunately the POSIX standard does not specify what the correct error code is
to identify the case where a username or groupname was not found when calling
getpwnam_r or getgrnam_r respectively, so Go's implementation of user.Lookup and
user.LookupGroup's returning UnknownUserError and UnknownGroupError is
fundamentally broken.

To remedy that, we just treat all non-nil errors as UnknownUserError or
UnknownGroupError. This means that other code using this may suffer from less
precise error handling, but that code already will be suffering from that same
problem as it is an upstream problem with the implementation of getpwnam_r and
getgrnam_r.

This is needed to unbreak preseeding on Groovy cloud images before
the preseeding fix can be back-ported to focal, so marking this as Critical.

pedronis · 2020-07-21T19:56:26Z

@anonymouse64 does preseeding work with this applied in the failing? or it just goes bit further? I mean does the adduser etc work then?

anonymouse64 · 2020-07-21T19:59:24Z

@anonymouse64 does preseeding work with this applied in the failing? or it just goes bit further? I mean does the adduser etc work then?

yes preseeding completes with this on groovy, I have not yet booted the resultant image, but preseeding looks correct, and the calls to adduser, etc. all work

anonymouse64 · 2020-07-21T20:22:48Z

Upstream Go issue filed: golang/go#40334

bboozzoo

LGTM

bboozzoo · 2020-07-22T11:04:15Z

osutil/group.go

+		// if there is a real problem finding the user then presumably other
+		// things will fail upon trying to create the user, etc. which will give
+		// more useful and specific errors
+		return 0, user.UnknownUserError(username)


I can almost convince myself it makes sense. The lookup failed, so the user/group is effectively unknown.

I've looked at the code that may be affected and it's seems that only snapshots care about those particular kinds of errors. It does a look by uid, but AFAICT it will eventually hit nsswitch (due to getpwuid_r). If I'm reading this right, snapshotstate.Save() can actually create a snapshot where otherwise it would not create one due to errors from user.LookupId() bubbling up. I don't think it's a big problem though.

I looked at the snapshot case and I agree I don't think it's a big problem, but note that the snapshot code doesn't use osutil.FindUid so it won't hit the code here. As such, I changed the code in snapshot and added this same comment there too

osutil/group.go

pedronis · 2020-07-22T11:40:31Z

It seems we have a couple more places using user.Lookup* in the codebase, I think we need to check what they do with errors.

zyga

LGTM, because cgo doesn't even attempt to map system level error information into anything like UnknownUserError, which is terrible.

anonymouse64 · 2020-07-22T13:57:22Z

It seems we have a couple more places using user.Lookup* in the codebase, I think we need to check what they do with errors.

yes as @bboozzoo points out there are some usages that suffer from the same problem in snapshot code. I will update those in this PR then (and do the requested comment change)

anonymouse64 · 2020-07-22T14:38:46Z

Alright I fixed the other instances of checking for user.Unknown{Group,User}Error here, although one of the cases I would like extra eyes on to confirm that my thinking is correct, specifically around osutil.RealUser, as that inspects SUDO_USER, and while I don't think my changes result in a privilege escalation I would feel better if someone from security could take a (really quick) look at it.

zyga

The comment about being root probably needs a bit of extra eyes. Root can be confined as well. I would not like to allow for a situation when a unprivileged snap service can somehow craft snapshot payload that writes over /root.

osutil/user.go

anonymouse64 · 2020-07-23T14:13:03Z

This is not Critical anymore, as CPC will be able to patch livecd-rootfs to not include systemd as a method for passwd/group lookup in nsswitch.conf in the chroot, and in that situation Go's function returns UnknownUserError correctly.

anonymouse64 · 2020-07-23T20:33:06Z

closing and re-opening to pick up the "run nested" label and run the nested spread tests

setharnold · 2020-07-24T03:17:48Z

overlord/snapshotstate/backend/helpers.go

+			// > does not specify what value errno might have in this situation.
+			// > But that makes it impossible to recognize errors.
+			//
+			// See upstream Go issue: https://github.com/golang/go/issues/40334
 			u, e := userLookupId(username)


What's the difference between userLookup() and userLookupId()? Is it intentional to call the one moments after the other?

userLookup is Go's user.Lookup (saved to a variable for mocking) that takes the username i.e. setharnold as the argument, while userLookupId is Go's user.LookupId, which takes the uid as the argument, i.e. 1000.

It is intentional to call them both like this yes (but note not always, only if the first username call fails). The reason to call them both like this is that here the username variable came from user input a la snap save --users=user1,user2,1000,12345 where each of the arguments could be either uid or a username, so we aren't sure which is which.

setharnold · 2020-07-24T03:22:49Z

osutil/group.go

+		// if there is a real problem finding the user/group then presumably
+		// other things will fail upon trying to create the user, etc. which
+		// will give more useful and specific errors
+		return 0, user.UnknownGroupError(groupname)


I'm not thrilled the the user id being returned in an error case being the all-powerful account on the system. Are there any "sum types" in golang that could be used to clearly differentiate between errors and successes, that would make failing open more difficult?

it seems to me that there are kinda 2 points you're making here which I will respond to separately

yes there is a concept of "sum types", specifically for this case of errors, there is a new system for composing different error types together and being able to inspect the full set of composed error types, which would allow us to return both the original error we got from user.Lookup here, as well as an error of the type user.UnknownGroupError that can be inspected / checked for explicitly by calling code, but I would consider that kind of change a bit invasive for this PR and I think is more a stylistic choice here rather than necessary.

regarding returning the default value of 0 here, this is fairly idiomatic Go, which is just returning the default return value for a function when there is an error, however in this case it is rather unfortunate that the default value of a uint also happens to be a fairly powerful user in the success case. What we could do to avoid this and protect against ignoring the error is we could change the return value on error to something that's not idiomatic Go, and return a very large number instead of 0, or we could change the return type to be a pointer, and on error we return nil, which means that a caller that ignores the error would panic trying to use the value as it would be nil. The issue with the former is that it's unclear to me what the right value is, looking around it seems different OS's have different max uid's, and there's not an easy way to find out without adding a bunch of OS specific code we would rather not do (but probably technically cloud). The issue with the latter is that we would have to change the function signature and all usages, which for this PR is rather invasive, so I would prefer to do that in a followup PR if you think this change needs to be made. I would also have to audit all other uid returning code we have to see if there are other functions similarly affected by this bug of default return value also meaning something very powerful.

It seems this has a single +1 now but no security review +1. Would changing the return value in the error case of "0" to "-1" be sufficient so that @setharnold would do a +1 on this? Or is there more needed?

-1 has also a special meaning for some syscalls, so not sure it's a completely better solution

I think of the choices between 0 and -1, I prefer 0, especially if that's the convention.

I would prefer to see a move to using sum types that can clearly prevent using an error return as a legitimate value but can appreciate that might be a long-term project.

Security team +1 on this. (Jamie's otherwise occupied; if you've got a specific question for him you can ask him more directly.)

Thanks

let's add a TODO to this file to turn the uint results at a later point to something with a distinguished default value (i.e. that can't be confused with uid/gid 0)

done in #9243

pedronis · 2020-07-24T06:57:53Z

@anonymouse64 was it intentional to push the preseed fixes here?

osutil/user.go

anonymouse64 · 2020-07-24T14:52:02Z

@anonymouse64 was it intentional to push the preseed fixes here?

it was but seeing how this grows the PR significantly, due to needing the other preseed fixes from #9053, it seems better to force push those out of existence from here and open a followup PR to this which adds those regression tests after this and 9053 is merged.

…nknown* Unfortunately the POSIX standard does not specify what the correct error code is to identify the case where a username or groupname was not found when calling getpwnam_r or getgrnam_r respectively, so Go's implementation of user.Lookup and user.LookupGroup's returning UnknownUserError and UnknownGroupError is fundamentally broken. To remedy that, we just treat all non-nil errors as UnknownUserError or UnknownGroupError. This means that other code using this may suffer from less precise error handling, but that code already will be suffering from that same problem as it is an upstream problem with the implementation of getpwnam_r and getgrnam_r. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

…ling As with osutil/user.FindUid(), we suffer from an upstream problem where error handling surrounding positively identifying if a given user/group/id does not exist is impossible. As such, treat all non-nil errors as the unknown case, in this case what happens is that when creating snapshots, we may silently miss some users if there are issues looking up their name/id, but that is better behavior than today where we would not create any snapshot if there was an errant /home/not-a-user/ directory and that not-a-user returned a non-0 errno from getpwnam_r. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

…mments The name of this function was confusing, as it is not the real user in the sense of real vs effective uid, but rather either the normal current user, or if we are root via sudo method, then the user calling sudo. Additionally, add a comment about the ineffectiveness of checking for user.UnknownUserError here. Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

PR changed significantly

pedronis

lgtm

…ing-is-hopeless Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

pedronis

let's land this with a TODO

pedronis · 2020-09-28T09:26:53Z

osutil/group.go

+		// if there is a real problem finding the user/group then presumably
+		// other things will fail upon trying to create the user, etc. which
+		// will give more useful and specific errors
+		return 0, user.UnknownGroupError(groupname)


let's add a TODO to this file to turn the uint results at a later point to something with a distinguished default value (i.e. that can't be confused with uid/gid 0)

mvo5 · 2020-09-28T09:44:26Z

Thanks, this looks fine now we will do some followup work as outlined by samuele

anonymouse64 added ⚠ Critical High-priority stuff (e.g. to fix master) Bug labels Jul 21, 2020

anonymouse64 added this to the 2.45 milestone Jul 21, 2020

anonymouse64 requested review from bboozzoo and pedronis July 21, 2020 19:26

zyga self-requested a review July 22, 2020 10:51

bboozzoo previously approved these changes Jul 22, 2020

View reviewed changes

pedronis reviewed Jul 22, 2020

View reviewed changes

osutil/group.go Outdated Show resolved Hide resolved

zyga previously approved these changes Jul 22, 2020

View reviewed changes

anonymouse64 added ⛔ Blocked Needs security review Can only be merged once security gave a :+1: and removed ⛔ Blocked labels Jul 22, 2020

anonymouse64 requested review from zyga and bboozzoo July 22, 2020 14:38

zyga reviewed Jul 22, 2020

View reviewed changes

osutil/user.go Outdated Show resolved Hide resolved

osutil/user.go Outdated Show resolved Hide resolved

pedronis removed this from the 2.45 milestone Jul 23, 2020

anonymouse64 removed the ⚠ Critical High-priority stuff (e.g. to fix master) label Jul 23, 2020

anonymouse64 added the Run nested The PR also runs tests inluded in nested suite label Jul 23, 2020

anonymouse64 closed this Jul 23, 2020

anonymouse64 reopened this Jul 23, 2020

setharnold reviewed Jul 24, 2020

View reviewed changes

pedronis requested changes Jul 24, 2020

View reviewed changes

osutil/user.go Outdated Show resolved Hide resolved

anonymouse64 added 3 commits July 24, 2020 09:56

anonymouse64 force-pushed the feature/osutil-user-group-lookup-err-handling-is-hopeless branch from fee118a to 206bb3a Compare July 24, 2020 17:56

anonymouse64 requested a review from pedronis July 24, 2020 17:57

anonymouse64 removed the Run nested The PR also runs tests inluded in nested suite label Jul 24, 2020

jdstrand self-requested a review July 24, 2020 21:11

pedronis approved these changes Jul 27, 2020

View reviewed changes

anonymouse64 added 2 commits July 31, 2020 13:25

Merge branch 'master' into feature/osutil-user-group-lookup-err-handl…

357d70f

…ing-is-hopeless Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

Merge branch 'master' into feature/osutil-user-group-lookup-err-handl…

83b6b40

…ing-is-hopeless Signed-off-by: Ian Johnson <ian.johnson@canonical.com>

pedronis approved these changes Sep 28, 2020

View reviewed changes

mvo5 merged commit 7ff96a4 into snapcore:master Sep 28, 2020

anonymouse64 deleted the feature/osutil-user-group-lookup-err-handling-is-hopeless branch September 28, 2020 15:04

anonymouse64 mentioned this pull request Sep 28, 2020

tests/nested/manual/preseed: include a system-usernames snap when preseeding #9423

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

osutil/group.go: treat all non-nil errs from user.Lookup{Group,} as Unknown* #9041

osutil/group.go: treat all non-nil errs from user.Lookup{Group,} as Unknown* #9041

anonymouse64 commented Jul 21, 2020 •

edited

pedronis commented Jul 21, 2020

anonymouse64 commented Jul 21, 2020

anonymouse64 commented Jul 21, 2020

bboozzoo left a comment

bboozzoo Jul 22, 2020

anonymouse64 Jul 22, 2020

pedronis commented Jul 22, 2020

zyga left a comment

anonymouse64 commented Jul 22, 2020

anonymouse64 commented Jul 22, 2020

zyga left a comment

anonymouse64 commented Jul 23, 2020

anonymouse64 commented Jul 23, 2020

setharnold Jul 24, 2020

anonymouse64 Jul 24, 2020

setharnold Jul 24, 2020

anonymouse64 Jul 24, 2020

mvo5 Sep 24, 2020

pedronis Sep 24, 2020

setharnold Sep 25, 2020

pedronis Sep 28, 2020

anonymouse64 Sep 28, 2020

pedronis commented Jul 24, 2020

anonymouse64 commented Jul 24, 2020

pedronis left a comment

pedronis left a comment

pedronis Sep 28, 2020

mvo5 commented Sep 28, 2020

osutil/group.go: treat all non-nil errs from user.Lookup{Group,} as Unknown* #9041

osutil/group.go: treat all non-nil errs from user.Lookup{Group,} as Unknown* #9041

Conversation

anonymouse64 commented Jul 21, 2020 • edited

pedronis commented Jul 21, 2020

anonymouse64 commented Jul 21, 2020

anonymouse64 commented Jul 21, 2020

bboozzoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis commented Jul 22, 2020

zyga left a comment

Choose a reason for hiding this comment

anonymouse64 commented Jul 22, 2020

anonymouse64 commented Jul 22, 2020

zyga left a comment

Choose a reason for hiding this comment

anonymouse64 commented Jul 23, 2020

anonymouse64 commented Jul 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis commented Jul 24, 2020

anonymouse64 commented Jul 24, 2020

pedronis left a comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvo5 commented Sep 28, 2020

anonymouse64 commented Jul 21, 2020 •

edited