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

Don't trust $USER #8583

Closed
buddhi-deep opened this issue Dec 25, 2021 · 18 comments · Fixed by #8879
Closed

Don't trust $USER #8583

buddhi-deep opened this issue Dec 25, 2021 · 18 comments · Fixed by #8879

Comments

@buddhi-deep
Copy link

fish, version 3.3.1 in chroot

I login with user safety(though it's unsafe at all),why fish shell think im a root user,ash and bash think im not root user

2021-12-25 21-02-23 的螢幕擷圖

2021-12-25 21-04-01 的螢幕擷圖

@faho
Copy link
Member

faho commented Dec 25, 2021

Fish thinks you are the root user because your uid is 0. You are root inside the container.

Bash simply trusts the $USER environment variable and reflects that, fish doesn't if the uid is 0.

@faho faho added the question label Dec 25, 2021
@buddhi-deep
Copy link
Author

But the user safety in container is always 1000,not 0,it was created when init a new container

@buddhi-deep
Copy link
Author

When i do su safety,fish think im not root,but if i do chroot --userspec safety:safety,fish think I'm root

@ridiculousfish
Copy link
Member

What does whoami and echo $USER print?

@faho
Copy link
Member

faho commented Dec 27, 2021

What happens here is that, at some point, fish runs getuid, and that returns 0, and then it runs getpasswd for that, and it tells it the name belonging to UID 0 is "root".

Even a diverging effective user id ("euid") won't help, because you can always set your effective uid to your real uid (even without special permissions).

I have no real idea what is happening here, and what exactly chroot is changing here. All I know is that fish asks the system which user it is, and the system insists it is root with a UID of 0.

Note that this is also broken inside bash - you can see the chosen $USER in the prompt (if you use the \u PS1 escape), but try to cd ~ and it will try to cd /root!

What does whoami and echo $USER print?

@ridiculousfish In both bash and fish: whoami will print the chosen user, $USER will be set to root.

@faho
Copy link
Member

faho commented Dec 27, 2021

Aaah, okay. I think I know what's happening here. I had the failure mode entirely the wrong way around!

Bash won't actually use $USER, ever, while fish will trust it when the uid isn't 0.

We do getuid(), it says some uid that's not 0, and fish then trusts $USER - because this was introduced to work around sus awkwardness of keeping $USER set. If it did give us 0, we would update $USER from passwd, and probably end up with "root". Ironically, in this case, the uid is not 0, but $USER is "root", so there's still a mismatch.

One possibility here is to update $USER, always, instead of ever trusting it. We want to have $USER be meaningful, because we don't have another way of reading it (like bash's \u expansion in $PS1, which does not use $USER, apparently), and that's what your prompt is reading.

@faho faho added enhancement and removed question labels Dec 27, 2021
@faho faho added this to the fish-future milestone Dec 27, 2021
@buddhi-deep
Copy link
Author

buddhi-deep commented Dec 31, 2021

I solved that
Set user=safety
Now my scriptctcontainer is good to go thx

@zanchey zanchey closed this as completed Jan 1, 2022
@faho faho reopened this Jan 1, 2022
@faho
Copy link
Member

faho commented Jan 1, 2022

This is not fixed, there is merely a workaround. The issue is that we can still inherit a wrong $USER.

I think the best thing we can do is fix $USER always instead of just when the uid is 0.

@faho faho changed the title Why fish shell think im a root user Don't trust $USER Jan 7, 2022
@septatrix
Copy link
Contributor

I think fixing USER is the wrong way because when it is changed there are usually reasons that it is that way. Also I am not sure which su behaviour you are referencing.

@faho
Copy link
Member

faho commented Apr 8, 2022

I think fixing USER is the wrong way because when it is changed there are usually reasons that it is that way

The problem is this:

  1. The reasons aren't actually any good (mostly it's "oh you can see your original user" which you quite possibly already knew and which is much less important than your actual current user)
  2. We don't have a special "this is the user value but for the prompt" (because we don't do that $PS1 formatting stuff) and a separate "this is the user value for internal use to find our config file". The way to print the user in your prompt is to use $USER.

So we should, indeed, simply get the value of $USER to line up with what the user actually is.

Also I am not sure which su behaviour you are referencing.

#3916 - some su implementations leave $USER intact in some cases (but not in others, like with su -l). It's a big hairy mess.

@septatrix
Copy link
Contributor

Still I think that silently manipulating the users environment can cause more confusion that it is worth. The less interfering option would be to use id -un in the prompt instead (though this would always require a subprocess which might not be too desirable)

@faho
Copy link
Member

faho commented Apr 8, 2022

If you want to do that you can do that, I'm not going to stop you.

The default here should be that $USER isn't entirely unrelated the the user you currently are, which nicely fixes already existing uses.

@septatrix
Copy link
Contributor

Introducing that change would be a dumpster fire on multi-user nodes where UIDs are shared due to limitations of running out (think universities and other shared clusters). This is already the case on BSD where logging in as toor user shows root as they share UID 0.

Furthermore should a shell not touch environment variables besides the obvious SHELL. This could have disastrous effects in programs and script when using e.g. HOME or LOGNAME (which is btw the more canonical source for the display name as opposed to USER). And please to not start to also set e.g. HOME to keep them in sync. That is simply outside of the shells responsibility.

We are trying to fix other programs legacy/compatibility behavior. There is no reason to use with su without --login/-l/- due to a plethora of other reasons apart from not setting USER/LOGNAME. Same goes for chroot which is a light wrapper around the system call and pushes this responsibility rightful to a login manager (the likes of su --login, getty and sulogin and not a shell).

The default here should be to leave the users environment as is. In my eyes the only reason against using id -un would be that it is not a built-in. Therefore iff you really really really want to change the behaviour to always use the user's name from /etc/passwd I would propose to add a new read-only fish variable fish_logname which could then simply call the required functions from pwd.h. This way the environment can stay as is and if cache is not required it would even update if the entry in the password file changes.
Though given the (in my eyes substantial) drawbacks I would advise against this.

@faho
Copy link
Member

faho commented Apr 15, 2022

$USER is what people will use, so it is the thing that should be fixed.

For advanced people, they can already use id -un today. They can also keep on using $LOGNAME - I've not seen many people bother with that.

Furthermore should a shell not touch environment variables besides the obvious SHELL. This could have disastrous effects in programs and script when using e.g. HOME or LOGNAME (which is btw the more canonical source for the display name as opposed to USER). And please to not start to also set e.g. HOME to keep them in sync.

We already do "touch" environment variables in a bunch of cases.

For instance we already do set $HOME to find the correct place for our config files, because keeping it and changing the user is broken, especially once we start writing universal variables.

This is a simple UX matter - if you can make the thing people reach for first work, then you don't have a need for a second thing.

@faho
Copy link
Member

faho commented Apr 15, 2022

Also:

Introducing that change would be a dumpster fire on multi-user nodes where UID
[...]
This could have disastrous effects

That's overly dramatic, and I would appreciate if you took a few steps back.

Realistically, currently $USER is outdated if the euid was changed via e.g. su. It doesn't match with what permissions you currently have, the value is at most good as a "this is what user you originally logged in as". That's essentially useless.

If we update $USER, the value will be one of the names for the current (e)uid. On systems I'm familiar with, the UID is what is actually checked, so that $USER value now reflects your permissions. Whether it's "root" or "toor" is then a minor cosmetic difference.

We are trying to fix other programs legacy/compatibility behavior.

Yes, in some cases we will try to work around other program's misbehaviors, to improve the overall experience.

@septatrix
Copy link
Contributor

Introducing that change would be a dumpster fire on multi-user nodes where UID
[...]
This could have disastrous effects

That's overly dramatic, and I would appreciate if you took a few steps back.

I apologize if I sounded lurid and did not want to be rude. Still I stand firm in my point that always changing these variables can have unintended consequences.

If I log in to my universities cluster (which is managed using kerberos IIRC) I share a UID with a few thousand other students. We all have different network mounted home directories inside some sort of chroot/container such that we cannot access other users data. If fish now changes the $USER also for UID!=0 I would always have a wrong prompt. Even worse if - as you said fish already also changes the $HOME for them to remain in sync I would be practically locked out of my system because it now refers to another users home which I am unable to access. I would not be surprised if the result inside a chroot as above would be the same.

@faho
Copy link
Member

faho commented Apr 15, 2022

One idea here is to get the passwd entry for the name (if any) via getpwnam(3) and if the uid matches continue.

If not, update the user data - name and home.

faho added a commit to faho/fish-shell that referenced this issue Apr 15, 2022
This gets the passwd entry for $USER (if it is set). If that gives the
same uid that getuid() gives us, we assume the data is correct.

If not, we reset $HOME and $USER from the passwd value for our UID.

Fixes fish-shell#8583
@septatrix
Copy link
Contributor

One idea here is to get the passwd entry for the name (if any) via getpwnam(3) and if the uid matches continue.

If not, update the user data - name and home.

Nice, that seems like a nifty way to do it.

@faho faho closed this as completed in #8879 May 2, 2022
faho added a commit that referenced this issue May 2, 2022
This gets the passwd entry for $USER (if it is set). If that gives the
same uid that geteuid() gives us, we assume the data is correct.

If not, we reset $USER (and $HOME if it's empty) from the passwd value for our UID.

This allows using $USER in a prompt even if you've `su`d. Bash gets around this by having a special escape in its $PS1 DSL that checks passwd instead.

Fixes #8583
@zanchey zanchey modified the milestones: fish-future, fish 3.5.0 May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants