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

Add sshd options for alternative ~/.ssh/environment and ~/.ssh/rc files #466

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

62832
Copy link
Contributor

@62832 62832 commented Feb 24, 2024

Continuing on from a previous attempt on the mailing list at proposing a mechanism for specifying alternative paths for the environment and rc files used by sshd, I figured it would be worth simply working on it there and then and submitting it as a PR.

The rationale for this comes from the prior mailing list discussion near the start of the month regarding the possibility of built-in fallback paths which would follow the XDG Base Directory Specification, and my own attempt following it at implementing it myself. As it turns out, it was not very feasible to implement full compliance, as some of the files within the ~/.ssh directory — namely, authorized_keys, environment and rc — were not read by the client program, but the server daemon running as a different privileged user. In attempting to implement any solution that required the use of environment variables to define alternative locations, I was not able to find an easy way — if such even exists — of having the sshd user read the authenticated user's existing environment.

Therefore, it would instead make sense to leverage the existing configuration and program invocation options such as ssh -F [config] for a user to enforce compliance as they wish. However, while it is possible to specify alternative paths for sshd to use for authorised key files, ~/.ssh/environment and ~/.ssh/rc remained hard-coded with no option to specify alternative paths. This PR implements this option for both of these files via the UserEnvironmentFile and UserRCFile options respectively for sshd_config, completely eliminating the need for the ~/.ssh directory should the user choose to not use it.

If there are any problems with the PR as it stands, please let me know and I'll be happy to address them.

@62832 62832 force-pushed the alt-env-rc branch 2 times, most recently from 98a3f1d to eb99ce0 Compare March 6, 2024 20:41
@christoofar
Copy link

Great. Another thing everyone has to scan for. Not a fan.

@62832
Copy link
Contributor Author

62832 commented Apr 3, 2024

I don't see the problem. By default, none of these are overridden and work the same as they always have. If people are already scanning for alternate authorized_keys files then why couldn't they do so for environment and rc too?

@christoofar
Copy link

still not a fan.

@christoofar
Copy link

christoofar commented Apr 3, 2024

I don't see the problem. By default, none of these are overridden and work the same as they always have. If people are already scanning for alternate authorized_keys files then why couldn't they do so for environment and rc too?

I will give you a reason. Now I have one cool place to set alias with no UID traceability. environment is an open book. And people who can't afford to buy commercial scanners have yet another setting they have to watch out for in ssh_conf. All this PR does is make CrowdStrike money.

Not. A. Fan.

@occune
Copy link

occune commented Apr 3, 2024

I don't think i follow what are you trying to say here. Can you elaborate what exactly is bothering you and what CrowdStrike (whatever that is) has anything in common with the PR?
Thanks! :)

@christoofar
Copy link

christoofar commented Apr 3, 2024

@62832 i eyeballed your commit. Did you make sure that the environment file specified in the option is locked down to the root user as owner and locked down with chattr +i ? If not... please for the love of God, add it.

@62832
Copy link
Contributor Author

62832 commented Apr 3, 2024

@62832 i eyeballed your commit. Did you make sure that the environment file specified in the option is locked down to the root user as owner and locked down with chattr +i ? If not... please for the love of God, add it.

OpenSSH doesn't even do this with the existing ~/.ssh/environment file prior to this PR. Why is this a requirement all of a sudden and why can this not be addressed in a separate PR if need be?

@christoofar
Copy link

christoofar commented Apr 3, 2024

@62832 i eyeballed your commit. Did you make sure that the environment file specified in the option is locked down to the root user as owner and locked down with chattr +i ? If not... please for the love of God, add it.

OpenSSH doesn't even do this with the existing ~/.ssh/environment file prior to this PR. Why is this a requirement all of a sudden?

sshd enforces flags on the key files in the user folder. Now you want to take the environment file, managed by OS security in the user folder, and put that anywhere.... and if you're determined to centralize the environment file you already have ln sitting on your machine right now for what arguably is just an edge case.

At least refuse to use the file if the underlying fs has no ro flag capability (and abort/ignore if it doesn't), and require ro be set on the file before sucking it up.

@62832
Copy link
Contributor Author

62832 commented Apr 3, 2024

sshd enforces flags on the key files in the user folder.

If that is indeed the case, I'm happy to take another look and address this. I concede that I might have glossed over something important, but I would have better appreciated if this was clarified with less hostility.

and if you're determined to centralize the environment file you already have ln sitting on your machine right now for what arguably is just an edge case.

Symlinks are not the answer. If I want to comply by specifications such as XDGBDS then I want there to be NO ~/.ssh directory, symlinked or not. Otherwise, why even provide the options to move different files and use those to begin with?

At least refuse to use the file if the underlying fs has no ro capability (and abort/ignore if it doesn't), and require ro be set on the file before sucking it up.

As mentioned in the first point, that can be arranged.

@christoofar
Copy link

christoofar commented Apr 3, 2024

At least refuse to use the file if the underlying fs has ro capability (and abort/ignore if it doesn't), and require ro be set on the file before sucking it up.

As mentioned in the first point, that can be arranged.

Thank you... updated out my typo above but you got what I meant. The ro check would give a lot of assurance that whatever option value is set, it's going to get some basket of controls that /etc gets, at least, since the file will be put anywhere by the user and they will be inclined to strip perms and chmod a+rw it.

@christoofar
Copy link

christoofar commented Apr 3, 2024

Thought about this some more...

You have to resolve down the path and examine if the user setting keeps the file in the own user's home folder, because you're assuming this is just going to be set for XDG but user can set it anywhere. If it stays in the user's home folder but no longer on the 0700 .ssh folder, you have to perm check the file to make sure the user did not put on global write/execute on it.

If it's outside the user home folder, then what I said above... file has to be owned by root and +i must be on the file because it's going to be global readable (then it's the user's problem if sensitive stuff is in it)... that would be ok.

I think that gets you in a better position because right now ssh is not checking flags there and the 0700 on .ssh is what's protecting it.

@62832
Copy link
Contributor Author

62832 commented Apr 3, 2024

Sounds good. In that case, I'm thinking that the steps to take would be the following:

  • Check if the (env/rc) file has permissions other than go-rwx and if it does, ignore it
  • Further check if the file resides somewhere in the user's home directory
  • If it does not reside in home, check if the file is owned by root and has the immutable attribute and if not, ignore it.

Is this what you had in mind?

@christoofar
Copy link

christoofar commented Apr 3, 2024

Sounds good. In that case, I'm thinking that the steps to take would be the following:

  • Check if the (env/rc) file has permissions other than go-rwx and if it does, ignore it

0700 anywhere in the $HOME folder, outside the $HOME folder 0755

+i check if it's OUTSIDE the $HOME folder, no check needed if it's inside.

  • If it does not reside in home, check if the file is owned by root and has the immutable attribute and if not, ignore it.

Is this what you had in mind?

Yes if it resolves down to outside the $HOME folder, +i needs to be on it root chown also must be on it and if the underlying fs does not have the ability to do the +i flag, ignore the setting.

That would keep the existing functionality the same when the option has been set, but then if an absolute path or path to somewhere else on the filesystem that makes the file shared needs good write protection.

@christoofar
Copy link

christoofar commented Apr 3, 2024

I'm guessing manpages update needed and/or printf because it's unexpected behavior to set an absolute path for the user folder and the client just ignores it with no information, so the user would need to be told (same way ssh client tells you that perms are wrong on the keys and authorized_keys)

@62832
Copy link
Contributor Author

62832 commented Apr 4, 2024

@christoofar I've so far manage to implement the checks for user ownership and outside permissions within $HOME, and root ownership outside of $HOME.

Unfortunately, I'm at a loss as to how to put together the immutability check considering that file flags work very differently between Linux and *BSD. chattr +i is very much Linux-specific (e2fsprogs) and in order to check for it as such it would rely on the Linux-specific fs.h, whereas on OpenBSD, say, flags set via chflags would be retrieved via OpenBSD's stat struct.

Since I don't know of a particularly easy portable way to check for this, I'd be eager to hear any potential alternatives that might be worth considering instead.

@62832
Copy link
Contributor Author

62832 commented Apr 5, 2024

Further consideration: it probably doesn't make sense to enforce root ownership on an env/rc file outside of $HOME if it happens to reside in a directory that's still owned by the user. Would it be fine in that case to just check the owner of the parent directory and see if it matches with the owner of the file?

@christoofar
Copy link

Further consideration: it probably doesn't make sense to enforce root ownership on an env/rc file outside of $HOME if it happens to reside in a directory that's still owned by the user. Would it be fine in that case to just check the owner of the parent directory and see if it matches with the owner of the file?

Well, ignoring the xz backdoor situation, it's still not a good idea. The keys files are supposed to be just data in the eyes of the user, with no surprises. Although we've learned that instructions can be injected into SSH certificate presentations and the fields can then deposit script. sshd runs as root everywhere someone also wants to login and claim root from their shell.

The environment and rc files are obvious injector points. If you take them out of where the user expects them to be (in the XDG reason you gave, it's still going to live under $HOME), then in my eyes it's not a shared object if 0700 is on the files.

But you take it anywhere outside that expectation, root ownership matters. It matters on the file. A shared environment file is what you're going to have happen by default so you should be throwing on as much security gates as you can.

@christoofar
Copy link

I think it's way better to take the more user-annoying option and enforce to the file and not allow a root->lazyadmin ownership if the file leaves the users area.

They will ask around why it's like that and on a machine that's set up with lots of group ownerships it will stop some priv escalation attacks.

@62832 62832 force-pushed the alt-env-rc branch 2 times, most recently from a670a13 to a57e10f Compare April 7, 2024 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants