Re: [patch] SIGSEGV when adding Connector to Event

[g4-lisz@xxxxxxxxxxxx]

Hi all,

You finally find the test code attached. It's a bit complicated, but it
can reproduce the issue:

1. Connect to an ssh server which supports direct socket tunnelling:

> ./multichannelfw root myserver.com 22

This opens a local listening port at 20021

2. Open two or more simultaneous ssh sessions via tunnel:

> ssh root@localhost -p20021

3. Close one session

4. Open a new one (will stall)

5. Trigger some traffic by typing something in an already open session*

6. Segmentation fault

* This is also affected by my other patch
"ssh_handle_packets_termination() ignores timeout=0". With this patch,
there's no need to trigger traffic to unblock receving messages)

Regards,
Till

On 28.01.19 23:10, g4-lisz@xxxxxxxxxxxx wrote:
>
> Hi all
>
> There's a bug in the connector API when subsequently adding to and
> removing connectors from an event loop.
>
> Here's some dummy code to reproduce it (I will add real code later):
>
> event = ssh_event_new();
>
> /* ADD FIRST connector pair (in/out) */
> ssh_connector in1 = ssh_connector_new(session);
> ssh_connector_set_out_channel(...);
> ssh_connector_set_in_fd(...);
> ssh_event_add_connector(event, in1);
>
>
> ssh_connector out1 = ssh_connector_new(session);
> ssh_connector_set_out_fd(...);
> ....
>
> ssh_event_dopoll(event, 2000);
>
> /* ADD SECOND connector pair */
> ssh_connector in2 = ssh_connector_new(session);
> ssh_connector_set_out_channel(...);
> ssh_connector_set_in_fd(...);
> ssh_event_add_connector(event, in2);
>
>
> ssh_connector out2 = ssh_connector_new(session);
> ssh_connector_set_out_fd(...);
> ....
>
> ssh_event_dopoll(event, 2000);
>
> .... /* work done with connector pair 1 */ 
>
> /* REMOVE FIRST connector pair */
> ssh_event_remove_connector(event, in1);
> ssh_event_remove_connector(event, out1);
> ssh_connector_free(in1);
> ssh_connector_free(in1);
>  ....  
>
> ssh_event_dopoll(event, 2000);
>
> /* ADD THIRD connector pair */
> ssh_connector in3 = ssh_connector_new(session);
> ssh_connector_set_out_channel(...);
> ssh_connector_set_in_fd(...);
> ssh_event_add_connector(event, in3);
> ^^^^ ***SIGSEGV ****          
>
> The underlying problem is ssh_connector_remove_event() where
> connector->in_channel and ->out_channel is NULLed.
>
> The subsequent call to ssh_connector_free() does not remove the
> in_channel and out_channel callbacks, because in_ and out_channel is NULL.
> So we have some sticky callbacks attached to the channel, and
> ssh_connector_remove_event() has removed the poll objects...
>
> Later on, when adding a new connector, ssh_channel_poll_timeout() is
> called at a certain point, which calls the CB function with the
> polling objects removed.
>
> And voila:
>
> Thread 3 "sshfwd" received signal SIGSEGV, Segmentation fault.
> [Switching to Thread 0x7ffff4574700 (LWP 15333)]
> 0x00007ffff7b76a04 in ssh_poll_get_events (p=0x0) at /home/till/libssh/src/poll.c:403
> 403      return p->events;
> (gdb) backtrace
> #0  0x00007ffff7b76a04 in ssh_poll_get_events (p=0x0) at /home/till/libssh/src/poll.c:403
> #1  0x00007ffff7b76ac3 in ssh_poll_add_events (p=0x0, events=4) at /home/till/libssh/src/poll.c:443
> #2  0x00007ffff7b56cd1 in ssh_connector_reset_pollevents (connector=0x7fffec0045b0) at /home/till/libssh/src/connector.c:235
> #3  0x00007ffff7b5757a in ssh_connector_channel_data_cb (session=0x7fffec0008c0, channel=0x7fffec006cc0, data=0x7fffec009940, len=120, 
>     is_stderr=0, userdata=0x7fffec0045b0) at /home/till/libssh/src/connector.c:489
> #4  0x00007ffff7b4cf50 in channel_rcv_data (session=0x7fffec0008c0, type=94 '^', packet=0x7fffec001450, user=0x7fffec0008c0)
>     at /home/till/libssh/src/channels.c:563
> #5  0x00007ffff7b6dfb5 in ssh_packet_process (session=0x7fffec0008c0, type=94 '^') at /home/till/libssh/src/packet.c:1421
> #6  0x00007ffff7b6d9aa in ssh_packet_socket_callback (data=0x7fffec008d40, receivedlen=176, user=0x7fffec0008c0)
>     at /home/till/libssh/src/packet.c:1275
> #7  0x00007ffff7b7ba1d in ssh_socket_pollcallback (p=0x7fffec004cc0, fd=6, revents=1, v_s=0x7fffec001280)
>     at /home/till/libssh/src/socket.c:302
> #8  0x00007ffff7b771ad in ssh_poll_ctx_dopoll (ctx=0x7fffec006900, timeout=-1) at /home/till/libssh/src/poll.c:702
> #9  0x00007ffff7b789eb in ssh_handle_packets (session=0x7fffec0008c0, timeout=-1) at /home/till/libssh/src/session.c:643
> #10 0x00007ffff7b78af1 in ssh_handle_packets_termination (session=0x7fffec0008c0, timeout=0, 
>     fct=0x7ffff7b502c1 <ssh_channel_read_termination>, user=0x7ffff456fb40) at /home/till/libssh/src/session.c:711
> #11 0x00007ffff7b50867 in ssh_channel_poll_timeout (channel=0x7fffec006cc0, timeout=0, is_stderr=0)
>     at /home/till/libssh/src/channels.c:2974
> #12 0x00007ffff7b5798c in ssh_connector_set_event (connector=0x7fffec007bb0, event=0x7fffec0047f0) at /home/till/libssh/src/connector.c:607
> #13 0x00007ffff7b7762d in ssh_event_add_connector (event=0x7fffec0047f0, connector=0x7fffec007bb0) at /home/till/libssh/src/poll.c:937
>
> The big fix is qute simple: Dont' NULL the channel pointers in
> ssh_connector_remove_event(). I don't really see the point why they
> have to be NULLed there. The event should be removed from the
> connector, and not the channels...
>
> Patch attached.
>
> Cheers,
> Till
>

/* gcc -o sshfwd fwtable.c cfglist.c sshfwd.c -lssh -lssh_threads -lpthread */

#include <libssh/libssh.h>
#include <libssh/callbacks.h>

#include <sys/socket.h>
#include <netinet/in.h>
#include <linux/netfilter_ipv4.h>
#include <arpa/inet.h>
#include <sys/time.h>
#include <fcntl.h>
#include <sys/types.h>
#include <sys/stat.h>

#include <stdlib.h>
#include <stdio.h> 
#include <errno.h>
#include <string.h>


#define NONBLOCK_WORKAROUND
#define MAX_FORWARDS 16

static const char *local_listenip = "127.0.0.1";
static const int local_listenport = 20021;

const char *remote_listenhost = "127.0.0.1"; /* resolved by the remote server */
int remote_listenport = 22;

static int verbosity = SSH_LOG_PROTOCOL;

void add_connector(ssh_session session, ssh_event event, ssh_connector *in, ssh_connector *out, ssh_connector *err, ssh_channel channel, int fd) {
#ifdef NONBLOCK_WORKAROUND
	int blocking = ssh_is_blocking(session);
	ssh_set_blocking(session, 0);
#endif

	/* stdinh */
	(*in) = ssh_connector_new(session);
	ssh_connector_set_out_channel((*in), channel, SSH_CONNECTOR_STDOUT);
	ssh_connector_set_in_fd((*in), fd);
	ssh_event_add_connector(event, (*in));

	/* stdout */
	(*out) = ssh_connector_new(session);
	ssh_connector_set_out_fd((*out), fd);
	ssh_connector_set_in_channel((*out), channel, SSH_CONNECTOR_STDOUT);
	ssh_event_add_connector(event, (*out));

	/* stderr */
	(*err) = ssh_connector_new(session);
	ssh_connector_set_out_fd((*err), fileno(stderr));
	ssh_connector_set_in_channel((*err), channel, SSH_CONNECTOR_STDERR);
	ssh_event_add_connector(event, (*err));

#ifdef NONBLOCK_WORKAROUND
	ssh_set_blocking(session, blocking);
#endif
}

static void remove_connector(ssh_event event, ssh_connector in, ssh_connector out, ssh_connector err) {
	ssh_event_remove_connector(event, in);
	ssh_event_remove_connector(event, out);
	ssh_event_remove_connector(event, err);

	ssh_connector_free(in);
	ssh_connector_free(out);
	ssh_connector_free(err);
}

int listen_poll(int listensock, int timout) {
	int forwardsock = -1;
	int max_fd, rc; 
	unsigned sout_len;
	fd_set fds;
	struct timeval tv;

	_ssh_log(SSH_LOG_PROTOCOL, "listen_poll", "Listening for connection on port %d", local_listenport);
	
	while (1) {
		FD_ZERO(&fds);
		FD_SET(listensock, &fds);
		max_fd = listensock + 1;
		tv.tv_sec = 0;
		tv.tv_usec = timout;
		sout_len = 0;
		
		rc = select(max_fd, &fds, NULL, NULL, &tv);

		if (-1 == rc && EINTR == errno) {
			continue;
		}
		else if (rc > 0) {
			if (FD_ISSET(listensock, &fds)) {
				_ssh_log(SSH_LOG_PROTOCOL, "main", "FD_ISSET");
				forwardsock = accept(listensock, NULL, &sout_len);
				if (forwardsock < 0) {
					_ssh_log(SSH_LOG_WARNING, "main", "accept failed: %s", strerror(errno));
				}
				else {
					_ssh_log(SSH_LOG_WARNING, "main", "Local client connected.");
				}
			}
		}
		else if (0 > rc) {
			_ssh_log(SSH_LOG_WARNING, "main", "select failed: %s", strerror(errno));
		}
		
		return forwardsock;
	}
}

int forward_tunnel(ssh_session session, int listensocket) {
	int i, rc, channel_currid = -1, channel_count = 0, channel_count_active = 0, has_closed=0;
	ssh_connector connector_in[MAX_FORWARDS], connector_out[MAX_FORWARDS], connector_err[MAX_FORWARDS];
	ssh_channel channel[MAX_FORWARDS];
	int forwardsock, socket[MAX_FORWARDS];

	_ssh_log(SSH_LOG_FUNCTIONS, "forward_tunnel", "New session");

	ssh_event event = ssh_event_new();
	ssh_event_add_session(event, session);

	do {
		forwardsock = listen_poll(listensocket, 10);

		if (forwardsock > -1) {
			/* New channel has been added */
			channel_count++;
			channel_currid = channel_count - 1;

			channel[channel_currid] = ssh_channel_new(session);
			if (channel[channel_currid] == NULL) {
				_ssh_log(SSH_LOG_WARNING, "forward_tunnel", "Couldn't allocate channel: %s",
								ssh_get_error(session));
				close(forwardsock);
				continue;
			}

			_ssh_log(SSH_LOG_PROTOCOL, "forward_tunnel", "Open channel to: %s:%d", 
										remote_listenhost, remote_listenport);
			rc = ssh_channel_open_forward(channel[channel_currid],
										remote_listenhost, remote_listenport,
										local_listenip, local_listenport);
			if (rc != SSH_OK) {
				_ssh_log(SSH_LOG_WARNING, "forward_tunnel", "Couldn't open forwarding channel %d: %s",
								rc, ssh_get_error(session));
				close(forwardsock);
				ssh_channel_free(channel[channel_currid]);
				channel[channel_currid] = NULL;
				continue;
			}

			socket[channel_currid] = forwardsock;

			_ssh_log(SSH_LOG_FUNCTIONS, "forward_tunnel", "Create Connector for Channel Id#%d",  channel_currid);
			add_connector(session, event, 
						&connector_in[channel_currid], &connector_out[channel_currid], &connector_err[channel_currid], 
						channel[channel_currid], socket[channel_currid]);

			channel_count_active++;
		}

		rc  = ssh_event_dopoll(event, 0);
		//_ssh_log(SSH_LOG_FUNCTIONS, "forward_tunnel", "ssh_event_dopoll ret %d", rc);

		//_ssh_log(SSH_LOG_PROTOCOL, "forward_tunnel", "channel_count_active: %d, channel_currid: %d, channel_count: %d",
		//						channel_count_active, channel_currid, channel_count);

		has_closed = 0;
		for (i=0; i <= channel_currid; i++) {
			if(channel[i] && ssh_channel_is_eof(channel[i])) {
				ssh_channel_send_eof(channel[i]);
			}
			if(channel[i] && ssh_channel_is_closed(channel[i])) {
				remove_connector(event, connector_in[i], connector_out[i], connector_err[i]);
				close(socket[i]);

				channel[i] = NULL;
				has_closed = 1;
				channel_count_active--;

				_ssh_log(SSH_LOG_PROTOCOL, "forward_tunnel", "Channel Id#%d closed. Remaining %d channels.", i, channel_count_active);
			}
		}
		if (has_closed) {
			for (i=0; i <= channel_currid; i++) {
				if (channel[i]) {
					remove_connector(event, connector_in[i], connector_out[i], connector_err[i]);
				}
			}
			for (i=0; i <= channel_currid; i++) {
				if (channel[i]) {
					add_connector(session, event, &connector_in[i], &connector_out[i], &connector_err[i], channel[i], socket[i]);				
				}
			}
			ssh_event_add_session(event, session);
		}

	} while (channel_count == 0 || channel_count_active > 0);

	ssh_event_free(event);

	return rc;
}

ssh_session create_session(const char *host, int port, const char *uname) {
	ssh_session session;
	int rc;

	session = ssh_new();
	if (session == NULL) {
		_ssh_log(SSH_LOG_WARNING, "create_session", "ssh_new failed.");
		return NULL;
	}

	ssh_options_set(session, SSH_OPTIONS_HOST, host);
	ssh_options_set(session, SSH_OPTIONS_USER, uname);
	ssh_options_set(session, SSH_OPTIONS_PORT, &port);

	rc = ssh_connect(session);
	if (rc != SSH_OK)
	{
		_ssh_log(SSH_LOG_WARNING, "create_session", "Error connecting to host: %s",
						ssh_get_error(session));
		ssh_free(session);
		return NULL;
	}
	_ssh_log(SSH_LOG_PROTOCOL, "create_session", "We are connected.");

	rc = ssh_userauth_publickey_auto(session, NULL, NULL);
	if (rc != SSH_AUTH_SUCCESS)
	{
		_ssh_log(SSH_LOG_WARNING, "create_session", "Authentication failed: %s",
						ssh_get_error(session));
		ssh_disconnect(session);
		ssh_free(session);
		return NULL;
	}
	_ssh_log(SSH_LOG_PROTOCOL, "create_session", "We are authenticated.");

	return session;

}

void main_loop(int listensocket, const char *host, int port, const char *uname) {

	ssh_session session = create_session(host, port, uname);

	if (session == NULL) {
		return;
	}

	forward_tunnel(session, listensocket);

	ssh_disconnect(session);
	ssh_free(session);

	_ssh_log(SSH_LOG_PROTOCOL, "main_loop", "Ended.");
}

int main(int argc, char **argv) {
	struct sockaddr_in sin;
	int listensock = -1, dest_port;

	if (argc < 4) {
		printf("Usage: %s <SSH_User> <SSH_Host> <SSH_Port>\n", argv[0]);
		return 0;
	}
	dest_port = atoi(argv[3]);

	ssh_set_log_level(verbosity);
	ssh_init();

	listensock = socket(PF_INET, SOCK_STREAM, IPPROTO_TCP);
	if (listensock == -1) {
		_ssh_log(SSH_LOG_WARNING, "main", "Error creating socket: %s", strerror(errno));
		goto shutdown;
	}

	int optval = 1;
	if (-1 == setsockopt(listensock, SOL_SOCKET, SO_REUSEADDR, &optval, sizeof(int))) {
		_ssh_log(SSH_LOG_WARNING, "main", "Set SO_REUSEADDR failed: %s", strerror(errno));
	}

	sin.sin_family = AF_INET;
	sin.sin_port = htons(local_listenport);
	sin.sin_addr.s_addr = htonl(INADDR_ANY);

	if (-1 == bind(listensock, (struct sockaddr *)&sin, sizeof(struct sockaddr_in))) {
		_ssh_log(SSH_LOG_WARNING, "main", "Bind failed: %s", strerror(errno));
		goto shutdown;
	}

	if (-1 == listen(listensock, 1)) {
		_ssh_log(SSH_LOG_WARNING, "main", "Listen failed: %s", strerror(errno));
		goto shutdown;
	}

	main_loop(listensock, argv[2], dest_port, argv[1]);

shutdown:
	close(listensock);
	printf("END\n");
	return 0;
}