Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

os/user: LookupId() returns unexpected results if $USER doesn't reflect current UID #27524

Closed
caseybarker opened this issue Sep 5, 2018 · 10 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@caseybarker
Copy link

caseybarker commented Sep 5, 2018

What version of Go are you using (go version)?

go1.10.3

Does this issue reproduce with the latest release?

Have not tried it, but the code appears to be the same in 1.11.

What operating system and processor architecture are you using (go env)?

linux/amd64

What did you do?

On some Linux distributions (e.g., CentOS 7), su does not change the $USER variable to root, but preserves the $USER who invoked it. (In this case, su - will change $USER.)

If a Go program is run from an environment like this, wherein $USER does not match the current UID, a call to user.LookupId() with the current UID will return a bad result. The User struct will contain the requested UID, but the username from $USER. (E.g., if I su and then invoke my program to lookup UID 0, I'll get back something like &{0 0 casey /root}.)

This is because of the Current() cache in os/user/lookup.go and the implementation of current() in os/user/lookup_stubs.go, which "looks up" the current user by mixing the current UID with os.Getenv("USER") to build the User struct instead of opening /etc/passwd. Of course, this only affects the current UID, and all other users are correctly scraped from /etc/passwd.

I think I understand the reasoning behind the optimization, but the result seems wrong, particularly when querying a whole range of users on the system. I did not expect os/user to depend on the "correctness" of my environment variables, but just the contents of /etc/passwd.

What did you expect to see?

user.LookupId("0") should return &User{0, 0, root, /root}

What did you see instead?

If $USER is casey but currentUID() is 0, user.LookupId("0") returns &User{0, 0, "casey", "", "/root"}

@xiaoxubeii
Copy link
Contributor

I tested it as follow but did't have this problem:

$ uname -a
Linux 3.10.0-327.18.2.el7.x86_64 #1 SMP Thu May 12 11:03:55 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
$ cat /etc/redhat-release
CentOS Linux release 7.2.1511 (Core)

$ useradd test && passwd test && usermod -aG wheel test
$ su test
$ echo $USER
test
$ sudo su
$ echo $USER
root

Did i miss something?

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 6, 2018
@bcmills bcmills added this to the Go1.12 milestone Sep 6, 2018
@bcmills
Copy link
Contributor

bcmills commented Sep 6, 2018

CC: @bradfitz @kevinburke

@caseybarker
Copy link
Author

caseybarker commented Sep 6, 2018

Here's my repro on a fresh install of CentOS 7.5:

[cbarkerj@localhost ~]$ echo $USER
cbarkerj
[cbarkerj@localhost ~]$ su
Password:
[root@localhost cbarkerj]# echo $USER
cbarkerj
[root@localhost cbarkerj]# uname -a
Linux localhost.localdomain 3.10.0-862.el7.x86_64 #1 SMP Fri Apr 20 16:44:24 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
[root@localhost cbarkerj]# cat /etc/redhat-release
CentOS Linux release 7.5.1804 (Core)

ETA: I noticed here that the initial report isn't quite correct. The repro is just to su not sudo su.

@kevinburke
Copy link
Contributor

To be clear - this is running with cgo disabled, right? With cgo enabled you should get the cgo_lookup_unix behavior, which uses the same process to retrieve users whether they're the current user or not.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/134218 mentions this issue: os/user: retrieve username from /etc/passwd instead of $USER

@RalphCorderoy
Copy link

To muddy the water, POSIX says it's LOGNAME that holds the user's login name and does not define USER. http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html#tag_08

@kevinburke
Copy link
Contributor

Oh man. I'm not sure I've ever seen that defined in the wild.

@RalphCorderoy
Copy link

Oh, it's definitely used. environ(7) here on Arch Linux says it's System-V compared to BSD for USER.
And systemd.exec(5) shows it's set if User= is used.
exec(3p) sets it in its EXAMPLES, unsurprisingly, since that's a POSIX man page.
getlogin(3) says it's read and makes no mention of USER.
libcurl-env(3) uses LOGNAME, falling back to USER.
crontab(5) says LOGNAME is set, and the BSD-ism USER if needed.
sudoers(5) also gives LOGNAME priority over USER.

@kevinburke
Copy link
Contributor

Interesting! What should the priority order be then ? The value in /etc/passwd seems like it's still the best, if it's present.

@RalphCorderoy
Copy link

Hi Kevin, yes, I agree, a match for the numeric UID in /etc/passwd is best, but then I'd suggest POSIX's LOGNAME should trump USER; that seems to be the consensus from a quick survey, the initial results above.

@golang golang locked and limited conversation to collaborators Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants