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

GSS-PROXY support #3407

Closed
abbra opened this issue Jan 4, 2016 · 32 comments
Closed

GSS-PROXY support #3407

abbra opened this issue Jan 4, 2016 · 32 comments
Assignees

Comments

@abbra
Copy link

abbra commented Jan 4, 2016

Cockpit session helper does not work with gss-proxy due to the way it calls into GSSAPI.

When gss-proxy is installed, it provides an interposer plugin in GSSAPI. Upon first use of GSSAPI, the library loads all interposer plugins and runs them. An interposer plugin has ability to route GSSAPI calls differently. In case of gss-proxy, the plugin first checks if GSS_USE_PROXY environmental variable is set to (1, true, or yes) and then activates the processing. To check environmental variables the interposer plugin uses secure_getenv() which returns NULL for applications where effective uid and gid are not the same as real uid and gid.

As result, even if GSS_USE_PROXY is set to 'yes', gss-proxy interposer plugin considers it cannot work with cockpit-session.

We need to change this situation because when gss-proxy is in use, cockpit does not need to access any keytab directly and we can keep HTTP/hostname@REALM keytab in possession of gss-proxy, without copying it into /etc/krb5.keytab. The latter is certainly a security issue for FreeIPA.

@stefwalter
Copy link
Contributor

@simo5 CC

@stefwalter
Copy link
Contributor

Any ideas on how to change this? Why is an environment variable used here? setuid programs (like cockpit-session) should not trust their environment, and typically clear it wholesale when starting.

@stefwalter stefwalter added the question Further information is requested label Jan 4, 2016
@abbra
Copy link
Author

abbra commented Jan 4, 2016

That's why the interposer uses secure_getenv() which will return NULL for any variable in the setuid environment.

However, we cannot use any other means to activate gss-proxy in cockpit-session. So my idea was to have GSSAPI negotiation run before we do set it to setuid.

@mvollmer
Copy link
Member

mvollmer commented Jan 4, 2016

I don't see a way how a setuid program could use gss-proxy. How does sshd do it?

@abbra
Copy link
Author

abbra commented Jan 4, 2016

sshd forks out a separate isolated process that does GSSAPI and then PAM. So one option would be to fork out, drop privileges, and then run GSSAPI there.

@mvollmer
Copy link
Member

mvollmer commented Jan 4, 2016

So one option would be to fork out, drop privileges, and then run GSSAPI there.

Another option would be to call setuid to change the real uid to zero so that secure_getenv is fooled, right?

@abbra
Copy link
Author

abbra commented Jan 4, 2016

sort of, yes, whatever it takes.

@stefwalter
Copy link
Contributor

Well cockpit-ws (the thing that launches) cockpit-session does not run as root. The only thing in Cockpit that runs as root is cockpit-session, and it's a setuid process (unless you're logging in as root, where obviously the cockpit-bridge is also running as root, but that's not involved here).

Another option would be to call setuid to change the real uid to zero so that secure_getenv is fooled, right?

Which would defeat all sorts of other setuid checks, and be a security issue.

sort of, yes, whatever it takes.

Unfortunately, I don't agree with this. We need to do this in a secure way and be very cautious about how we deal with privileges in cockpit-session.

@abbra
Copy link
Author

abbra commented Jan 4, 2016

then you need to fork out unprivileged process for GSSAPI processing.

@stefwalter
Copy link
Contributor

I would say gss-proxy should have a secondary means to turn it on, that is not environment variable based. This will be a common issue with any setuid process. Put another way: Why is gss-proxy using secure_getenv() if it expects the processes calling gssproxy to work around that usage?

In any case, are users expected to screw around with cockpit-ws or its service file to set the GSS_USE_PROXY variable? Where does it originate from?

@stefwalter
Copy link
Contributor

Another option would be to call setuid to change the real uid to zero so that secure_getenv is fooled, right?

I forgot to say, but this won't work anyway. We proactively clear our environment and start afresh ... as does any well written setuid program. We do save certain specific environment variables and pass them to pam and use them to launch cockpit-bridge, but those don't affect cockpit-session, gssapi, or any other process that is forked:

https://github.com/cockpit-project/cockpit/blob/master/src/ws/session.c#L896

@abbra One thing we could do is add GSSAPI_USE_PROXY to the list above ... and gss-proxy would be modified to use pam_getenv() as a fallback for secure_getenv().

@abbra
Copy link
Author

abbra commented Jan 4, 2016

As for GSS_USE_PROXY we thought with Marius that we can set it always in cockpit-session. If gss-proxy is not installed, it will be ignored. If gss-proxy cannot be contacted, it will be ignored. If GSS_USE_PROXY is not set, it will be ignored too. So only when gss-proxy is properly configured it will affect your workflow and will allow to not have HTTP/hostname keys in the default keytab.

As for the list of env_names, we thought about it, no it does not work. We set the GSS_AUTH_PROXY in process_gssapi() instead, same way currently KRB5CCNAME is set. That is fine except for the interposer's use of secure_getenv().

@stefwalter
Copy link
Contributor

As for GSS_USE_PROXY we thought with Marius that we can set it always in cockpit-session. If gss-proxy is not installed, it will be ignored. If gss-proxy cannot be contacted, it will be ignored. If GSS_USE_PROXY is not set, it will be ignored too. So only when gss-proxy is properly configured it will affect your workflow and will allow to not have HTTP/hostname keys in the default keytab.

As for the list of env_names, we thought about it, no it does not work. We set the GSS_AUTH_PROXY in process_gssapi() instead, same way currently KRB5CCNAME is set. That is fine except for the interposer's use of secure_getenv().

Sounds fine with me. As long as there are zero warning messages in the routine case, where gss-proxy is not configured and/or not installed.

@abbra
Copy link
Author

abbra commented Jan 4, 2016

If you look into http://www.citi.umich.edu/u/provos/ssh/priv.jpg -- this is how privilege separation is done in sshd -- you can see that they have couple more stages. When PAM is used, GSSAPI processing is done in a privilege separated process.

As for warnings, there are none. If gss-proxy was successful, you'd get a ticket. If it is not, GSSAPI transparently fails over to read /etc/krb5.keytab (if that is the default keytab) and if the key is not there, it will certainly fail but that is exactly what happens now when you don't have HTTP/hostname keys in /etc/krb5.keytab.

@stefwalter
Copy link
Contributor

If you look into http://www.citi.umich.edu/u/provos/ssh/priv.jpg -- this is how privilege separation is done in sshd -- you can see that they have couple more stages. When PAM is used, GSSAPI processing is done in a privilege separated process.

We are different from sshd, in that the parent process does not run as root. cockpit-session is our forked process, and it's setuid. cockpit-ws isn't even started as root, it never even drops privileges. Never had any real privileges to start with.

As for warnings, there are none. If gss-proxy was successful, you'd get a ticket. If it is not, GSSAPI transparently fails over to read /etc/krb5.keytab (if that is the default keytab) and if the key is not there, it will certainly fail but that is exactly what happens now when you don't have HTTP/hostname keys in /etc/krb5.keytab.

Sounds good.

@abbra
Copy link
Author

abbra commented Jan 4, 2016

Then that's cockpit-session's charge to fork out a process that would be unprivileged and would run its GSSAPI code there.

@stefwalter
Copy link
Contributor

Then that's cockpit-session's charge to fork out a process that would be unprivileged and would run its GSSAPI code there.

What's the point of doing that? Is GSSAPI code considered risky and should only run in an unprivileged process? Note that this has no bearing on the above environment variable. The environment will still be gone in a subprocess, unless we set it manually (which we'll do in process_gssapi(), are you submitting a patch?)

@abbra
Copy link
Author

abbra commented Jan 4, 2016

I'd wait for @simo5 comment on why secure_getenv() was used in the interposer code. In general, I don't want to run GSSAPI code in a privileged process exactly because there are many moving parts below GSSAPI (libkrb5 also loads a number of plugins, like locator or localauth ones) and you are not really getting anything good out of exposing them out for root process address space.

@simo5
Copy link

simo5 commented Jan 4, 2016

We use secure_getenv() because you do not want to risk that a user is able to change the configuration of a root process. I do realize this means gss-proxy is not usable by setuid helpers, but that has never been a problem till now.

One thing we could do is to have a system configuration file, we haven't used that option till now to avoid the overhead of probing and opening files on the first gssapi call, but perhaps we should now add such an option, something modeled after sasl where you can have different configurations based on application/uid used.

I'll open a ticket for that.

@stefwalter
Copy link
Contributor

@abbra In the non-gss-proxy case, cockpit-session's GSSAPI code does need to run as root. It needs access to /etc/krb5.keytab (yes, the system keytab, not a service keytab, cockpit is a system login like ssh). I understand that in the gss-proxy case, keytab access is avoided. However, in the gss-proxy case all the cockpit-session process is doing is talking to gss-proxy over a socket. Does that really need to happen in an unprivileged subprocess?

In any case, if it does need to happen unprivileged, I wouldn't be against contributed patches that handle the gss-proxy case in an unprivileged cockpit-session subprocess. All other cases need to remain running as root handled, obviously.

@simo5
Copy link

simo5 commented Jan 4, 2016

There is no need to be unprivileged, but it is a bit pointless.
The process has direct access to /etc/krb5.keytab if it is running as root, so it technically doesn't need gss-proxy assistance.

@abbra
Copy link
Author

abbra commented Jan 4, 2016

@simo5, the problem I'm trying to solve is to avoid having HTTP/hostname keys in /etc/krb5.keytab where it should not be, really. Up until now use of GSSAPI in cockpit requires to do so.

@stefwalter
Copy link
Contributor

the problem I'm trying to solve is to avoid having HTTP/hostname keys in /etc/krb5.keytab where it should not be, really. Up until now use of GSSAPI in cockpit requires to do so.

That's a great goal by the way. So as a first step, lets just set the appropriate environment variable in process_gssapi(). I'll review and merge your pull request for this, @abbra

@simo5
Copy link

simo5 commented Jan 4, 2016

Well we can either set the gssproixy env var or set the KRB5_KTNAME env var to point to a different keytab.

@abbra
Copy link
Author

abbra commented Jan 4, 2016

@simo5 -- setting the gss-proxy env var does not help because the interposer plugin simply ignores it in setuid application. That's the problem we have.

I'll do a patch that does setenv() in process_gssapi(), but it is effectively no-op from GSSAPI processing point of view because of secure_getenv().

@stefwalter
Copy link
Contributor

I'll do a patch that does setenv() in process_gssapi(), but it is effectively no-op from GSSAPI processing point of view because of secure_getenv().

You're right, duh.

@cgwalters
Copy link
Contributor

BTW, remember calling setenv() is unsafe if the process is multithreaded.

@cgwalters
Copy link
Contributor

(And background threads might call getenv() - see:
https://sourceware.org/bugzilla/show_bug.cgi?id=4887#c9
https://bugs.freedesktop.org/show_bug.cgi?id=65681 )

@stefwalter
Copy link
Contributor

cockpit-session should not be multi-threaded until the PAM stack when all bets are off. So lets make sure not to call setenv() after any pam calls

@stefwalter stefwalter removed the question Further information is requested label Mar 21, 2016
@stefwalter stefwalter self-assigned this Nov 29, 2016
@stefwalter
Copy link
Contributor

@simo5 Fundamentally is it insecure for an unprivileged process (ie: cockpit-ws) to tell a privileged process (ie: cockpit-session) to use gssproxy (ie: via a GSS_USE_PROXY=1) environment variable?

If so, then we would need cockpit-session to start parsing configuration files (ugh ... nasty complexity in a privileged process). Not pretty.

Even with that working it seems that gssproxy assumes that all requests coming from a certain euid should map to a single service. cockpit-session uses an euid of zero during authentication, and only after authentication changes to the user's uid.

The entire way that gssproxy is currently opt-in and requires a janitor^H^H^H^H^H^H^Hadministrator to setup has several usability flaws. I really want to use it as Cockpit's answer to non-standard keytabs.

But back to the basics: Is there a non-environment way to have cockpit-session opt into use of gssproxy?

@simo5
Copy link

simo5 commented Sep 12, 2017

You can set also an env var to specify a different socket to connect to, this way you can have a custome configuration and access to the socket can be controlled via file system permissions too.

I do not see a security issue in passing the env var, worst case you get authentication with the wrong user, but you should check that in cockpit-session I guess ?

At the moment there is no other way to opt in, but in cocpkit-session code you can always sanitize/set the specific env variables you want to (re)set.

HTH.

@marusak
Copy link
Member

marusak commented Feb 14, 2020

This issue is closed due to inactivity.

If this issue is a bug report and you are still able to reproduce it, please
comment here and provide us with an updated reproducer.

If this issue is a request for a new feature, we are sorry, but we won't be able
to address this in any time soon. If you are interested in helping us with
the implementation, please comment here.

@marusak marusak closed this as completed Feb 14, 2020
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

No branches or pull requests

7 participants