Subject: Bug#1345: x2goclient overrides user-specified SSH port with wrong default

From: [email-masked]
Date: Wed, 21 Nov 2018 18:43:30 -0800

Package: x2goclient
Version: 4.1.2.2
Severity: Important

OS: Linux Debian Buster
x2goclient:  4.1.2.2-0x2go1~git20181113.1828+10.heuler.1

There is a bug in x2goclient's port handling that was introduced when adding support for libssh
Config file (bug #1121):

sshmasterconnection.cpp uses ssh_options_parse_config in an attempt to obtain config-based
values for hostname and port, in case the values supplied by the user are short-hands or
defaults.  The problem is that sshmasterconnection takes the values returned by
ssh_options_parse_config even when the x2goclient user has given explicit hostnames and ports.

So let's say I want to connect to "frobisher.example.com" which has a ssh server on port 3322,
and I explicitly give these parameters in the x2goclient session.

All works well if "Host *" in /etc/ssh/ssh_config (and ~/.ssh/config) does not have a default port.
But if it does, like this:

Host *
  Port 6789

Then that port is taken and sshmasterconnection tries (and fails) to connect to
frobisher.example.com on port 6789.
The code should not override a port that is specified by the interactive user.
This is the way ssh works: command line options take precedence over config values, and so
system and user files are setup accordingly.

Second, what if the default port in /etc/ssh/ssh_config or ~/.ssh/config is actually 6789 (or
22, or any explicit value)? That fails too, because the following code in
SshMasterConnection::channelLoop() will override x2go's own channel forwarding ports:

/* The host and port might be a shorthand and zero, so fetch the actual data. */
if (ssh_options_parse_config (tmp_session, NULL) < 0) {
x2goDebug << "Warning: unable to parse the SSH config file.";
}

unsigned int inferred_port = 0;
ssh_options_get_port (tmp_session, &inferred_port);
x2goDebug << "Temporary session port after config file parse: " << inferred_port;

char *inferred_host = NULL;
ssh_options_get (tmp_session, SSH_OPTIONS_HOST, &inferred_host);
x2goDebug << "Temporary session host after config file parse: " << inferred_host;

Here is the debugging output of the code above when it works and when it doesn't:

GOOD (default Port commented out in /etc/ssh/ssh_config for "Host *"):

x2go-DEBUG-../src/sshmasterconnection.cpp:2111> Forwarding parameters: from remote
("localhost":49471) to local ("localhost":34508)
x2go-DEBUG-../src/sshmasterconnection.cpp:2148> Temporary session port after config file parse:
49471
x2go-DEBUG-../src/sshmasterconnection.cpp:2152> Temporary session host after config file parse:
localhost
x2go-DEBUG-../src/sshmasterconnection.cpp:2182> New channel forwarded.

BAD (/etc/ssh/ssh_config has an explicit "Port 6789"  for "Host *" ):

x2go-DEBUG-../src/sshmasterconnection.cpp:2111> Forwarding parameters: from remote
("localhost":40052) to local ("localhost":54592)
x2go-DEBUG-../src/sshmasterconnection.cpp:2148> Temporary session port after config file parse:
6789
x2go-DEBUG-../src/sshmasterconnection.cpp:2152> Temporary session host after config file parse:
localhost
x2go-DEBUG-../src/sshmasterconnection.cpp:2182> New channel forwarded.