ws: Disallow direct URL logins with LoginTo=false #18609
Conversation
|
@allisonkarlitskaya Tests didn't finish yet (just my local run of this specific test, and interactively), but I'd appreciate a first review whether this is the right direction. Thanks! |
doc/man/cockpit.conf.xml
Outdated
| on the login screen is visible and allows logging into another server. If this | ||
| option is not specified then it will be automatically detected based on whether | ||
| the <command>cockpit-ssh</command> process is available or not.</para> | ||
| on the login screen is visible and allows logging into another server. When set to |
There was a problem hiding this comment.
The indentation change makes this patch hard to read.
I wonder if we should have the default just be False. It's kinda dangerous, potentially unexpected, and cockpit-ssh is very easy to have installed.
It might break people's working configs on upgrade, but it would be easy to advise how to fix it.
There was a problem hiding this comment.
Err right, that wasn't intended. vim's auto-spacing has an eternal fight with the weird indentation of our doc files. I fixed this.
Changing the default is a much harder decision. Connecting to a remote machine is a rather useful and important feature of cockpit, and just changing it on upgrades breaks existing setups too hard IMHO. Also, in some sense we even want people to do this "remote host" thing -- it's really not encouraged to put cockpit-ws on production servers, but always go through some bastion host. cockpit-desktop/Cockpit Client do exactly that (of course these can get an explicit config). LoginTo also interacts with RequireHost=true -- if that is set, then LoginTo=false does not make sense. So if it's unset, then it should be implicitly true with RequireHost=true.
In other words, this is a mine field -- at some point we need to analyze carefully what makes sense here, and probably clean up this non-orthogonal configuration.
| @@ -1047,6 +1047,13 @@ cockpit_session_launch (CockpitAuth *self, | |||
| return NULL; | |||
| } | |||
|
|
|||
| /* this might be unset, which means "allow if cockpit-ssh is installed"; if it isn't, this will fail later on */ | |||
| if (host && !cockpit_conf_bool ("WebService", "LoginTo", TRUE)) { | |||
There was a problem hiding this comment.
This looks sensible.
There are four cases where we could end up using cockpit-ssh. Two of those involve explicit config and one of them involves a commandline option. This should successfully neutralize the only case the user controls.
There was a problem hiding this comment.
So that's (1) login page with "Connect to:" (potentially dangerous), (2) shell's remote host (dangerous in a different way due to non-isolated frames, but not internal network scanning), (3) cockpit-ws --local-session (harmless). What is (4)?
The current documentation of LoginTo= isn't very specific about what exactly happens with a "false" value; but it is plausible for an admin to assume that "false" would disallow logging into a remote host completely -- not merely hide the "Connect to:" field and then allowing a direct URL login anyway. It is sometimes important to disallow direct SSH logins from the login page on publicly exposed bastion hosts, as this functionality allows unauthenticated remote users to: - scan the internal network for existing hosts, which might otherwise not be accessible directly from the internet (Fixes cockpit-project#18540, https://bugzilla.redhat.com/show_bug.cgi?id=2167006) - scan the cockpit-ws host or internal network hosts for open ports (Fixes cockpit-project#15077, https://bugzilla.redhat.com/show_bug.cgi?id=2018741) So change ws to reject direct URL logins with `LoginTo=false`. This happens most naturally in cockpit_session_launch(), as we still want to allow remote URLs from the shell's host switcher in already authenticated sessions. This will not produce a very friendly error message, but it doesn't have to be -- at that point specifying direct URLs can be considered hacking anyway. Clarify the documentation accordingly.
The current documentation of LoginTo= isn't very specific about what exactly happens with a "false" value; but it is plausible for an admin to assume that "false" would disallow logging into a remote host completely -- not merely hide the "Connect to:" field and then allowing a direct URL login anyway.
It is sometimes important to disallow direct SSH logins from the login page on publicly exposed bastion hosts, as this functionality allows unauthenticated remote users to:
scan the internal network for existing hosts, which might otherwise not be accessible directly from the internet (Fixes cockpit: LoginTo=false does not prevent connections to remote hosts #18540, https://bugzilla.redhat.com/show_bug.cgi?id=2167006)
scan the cockpit-ws host or internal network hosts for open ports (Fixes unauthenticated server-side request forgery - login page allows ssh to to localhost [CVE-2020-35850] #15077, https://bugzilla.redhat.com/show_bug.cgi?id=2018741)
So change ws to reject direct URL logins with
LoginTo=false. This happens most naturally in cockpit_session_launch(), as we still want to allow remote URLs from the shell's host switcher in already authenticated sessions. This will not produce a very friendly error message, but it doesn't have to be -- at that point specifying direct URLs can be considered hacking anyway.Clarify the documentation accordingly.
webserver: Disallow direct URL logins with
LoginTo=falsecockpit.conf has a
LoginTo=option. This allows the admin to disable the login page's "Connect to:" functionality for directly logging into a remote host through SSH. Setting it tofalsepreviously still left the possibility of a remote login through directly specifying an appropriate URL. With this Cockpit version,LoginTo=falsedisallows logins through remote URLs as well.If cockpit-ws is exposed to the public internet, and also has access to a private internal network, it is recommended to explicitly set
LoginTo=false. This prevents unauthenticated remote attackers from scanning the internal network for existing machines and open ports.