- (djm) [auth-pam.c sftp.c] spaces vs. tabs at start of line

   - djm@cvs.openbsd.org 2005/07/17 06:49:04
     [channels.c channels.h session.c session.h]
     Fix a number of X11 forwarding channel leaks:
     1. Refuse multiple X11 forwarding requests on the same session
     2. Clean up all listeners after a single_connection X11 forward, not just
        the one that made the single connection
     3. Destroy X11 listeners when the session owning them goes away
     testing and ok dtucker@

5 files changed, 125 insertions, 8 deletions

diff --git a/ChangeLog b/ChangeLog
index 647ad016..84b82ccc 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -8,6 +8,14 @@
    [cipher-acss.c loginrec.c ssh-rand-helper.c sshd.c] Fix whitespace at EOL 
    in portable too ("perl -p -i -e 's/\s+$/\n/' *.[ch]")
  - (djm) [auth-pam.c sftp.c] spaces vs. tabs at start of line
+   - djm@cvs.openbsd.org 2005/07/17 06:49:04
+     [channels.c channels.h session.c session.h]
+     Fix a number of X11 forwarding channel leaks:
+     1. Refuse multiple X11 forwarding requests on the same session
+     2. Clean up all listeners after a single_connection X11 forward, not just
+        the one that made the single connection
+     3. Destroy X11 listeners when the session owning them goes away
+     testing and ok dtucker@
 
 20050716
  - (dtucker) [auth-pam.c] Ensure that only one side of the authentication
@@ -2841,4 +2849,4 @@
    - (djm) Trim deprecated options from INSTALL. Mention UsePAM
    - (djm) Fix quote handling in sftp; Patch from admorten AT umich.edu
 
-$Id: ChangeLog,v 1.3849 2005/07/17 07:18:49 djm Exp $
+$Id: ChangeLog,v 1.3850 2005/07/17 07:19:24 djm Exp $
diff --git a/channels.c b/channels.c
index b7ff8500..8da399b6 100644
--- a/channels.c
+++ b/channels.c
@@ -39,7 +39,7 @@
  */
 
 #include "includes.h"
-RCSID("$OpenBSD: channels.c,v 1.221 2005/07/16 01:35:24 djm Exp $");
+RCSID("$OpenBSD: channels.c,v 1.222 2005/07/17 06:49:04 djm Exp $");
 
 #include "ssh.h"
 #include "ssh1.h"
@@ -2659,7 +2659,7 @@ channel_send_window_changes(void)
  */
 int
 x11_create_display_inet(int x11_display_offset, int x11_use_localhost,
-    int single_connection, u_int *display_numberp)
+    int single_connection, u_int *display_numberp, int **chanids)
 {
 	Channel *nc = NULL;
 	int display_number, sock;
@@ -2749,6 +2749,8 @@ x11_create_display_inet(int x11_display_offset, int x11_use_localhost,
 	}
 
 	/* Allocate a channel for each socket. */
+	if (chanids != NULL)
+		*chanids = xmalloc(sizeof(**chanids) * (num_socks + 1));
 	for (n = 0; n < num_socks; n++) {
 		sock = socks[n];
 		nc = channel_new("x11 listener",
@@ -2756,7 +2758,11 @@ x11_create_display_inet(int x11_display_offset, int x11_use_localhost,
 		    CHAN_X11_WINDOW_DEFAULT, CHAN_X11_PACKET_DEFAULT,
 		    0, "X11 inet listener", 1);
 		nc->single_connection = single_connection;
+		if (*chanids != NULL)
+			(*chanids)[n] = nc->self;
 	}
+	if (*chanids != NULL)
+		(*chanids)[n] = -1;
 
 	/* Return the display number for the DISPLAY environment variable. */
 	*display_numberp = display_number;
diff --git a/channels.h b/channels.h
index b89b7c95..1cb2c3a3 100644
--- a/channels.h
+++ b/channels.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: channels.h,v 1.78 2005/07/08 09:41:33 markus Exp $	*/
+/*	$OpenBSD: channels.h,v 1.79 2005/07/17 06:49:04 djm Exp $	*/
 
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
@@ -214,7 +214,7 @@ int	 channel_cancel_rport_listener(const char *, u_short);
 /* x11 forwarding */
 
 int	 x11_connect_display(void);
-int	 x11_create_display_inet(int, int, int, u_int *);
+int	 x11_create_display_inet(int, int, int, u_int *, int **);
 void     x11_input_open(int, u_int32_t, void *);
 void	 x11_request_forwarding_with_spoofing(int, const char *, const char *,
 	    const char *);
diff --git a/session.c b/session.c
index 13c3b001..81d7d53e 100644
--- a/session.c
+++ b/session.c
@@ -33,7 +33,7 @@
  */
 
 #include "includes.h"
-RCSID("$OpenBSD: session.c,v 1.183 2005/07/16 01:35:24 djm Exp $");
+RCSID("$OpenBSD: session.c,v 1.184 2005/07/17 06:49:04 djm Exp $");
 
 #include "ssh.h"
 #include "ssh1.h"
@@ -1634,6 +1634,7 @@ session_new(void)
 			s->ttyfd = -1;
 			s->used = 1;
 			s->self = i;
+			s->x11_chanids = NULL;
 			debug("session_new: session %d", i);
 			return s;
 		}
@@ -1707,6 +1708,29 @@ session_by_channel(int id)
 }
 
 static Session *
+session_by_x11_channel(int id)
+{
+	int i, j;
+
+	for (i = 0; i < MAX_SESSIONS; i++) {
+		Session *s = &sessions[i];
+
+		if (s->x11_chanids == NULL || !s->used)
+			continue;
+		for (j = 0; s->x11_chanids[j] != -1; j++) {
+			if (s->x11_chanids[j] == id) {
+				debug("session_by_x11_channel: session %d "
+				    "channel %d", s->self, id);
+				return s;
+			}
+		}
+	}
+	debug("session_by_x11_channel: unknown channel %d", id);
+	session_dump();
+	return NULL;
+}
+
+static Session *
 session_by_pid(pid_t pid)
 {
 	int i;
@@ -1835,6 +1859,11 @@ session_x11_req(Session *s)
 {
 	int success;
 
+	if (s->auth_proto != NULL || s->auth_data != NULL) {
+		error("session_x11_req: session %d: "
+		    "x11 fowarding already active", s->self);
+		return 0;
+	}
 	s->single_connection = packet_get_char();
 	s->auth_proto = packet_get_string(NULL);
 	s->auth_data = packet_get_string(NULL);
@@ -2060,9 +2089,66 @@ sig2name(int sig)
 }
 
 static void
+session_close_x11(int id)
+{
+	Channel *c;
+
+	if ((c = channel_lookup(id)) == NULL) {
+		debug("session_close_x11: x11 channel %d missing", id);
+	} else {
+		/* Detach X11 listener */
+		debug("session_close_x11: detach x11 channel %d", id);
+		channel_cancel_cleanup(id);
+		if (c->ostate != CHAN_OUTPUT_CLOSED)
+			chan_mark_dead(c);
+	}
+}
+
+static void
+session_close_single_x11(int id, void *arg)
+{
+	Session *s;
+	u_int i;
+
+	debug3("session_close_single_x11: channel %d", id);
+	channel_cancel_cleanup(id);
+	if ((s  = session_by_x11_channel(id)) == NULL)
+		fatal("session_close_single_x11: no x11 channel %d", id);
+	for (i = 0; s->x11_chanids[i] != -1; i++) {
+		debug("session_close_single_x11: session %d: "
+		    "closing channel %d", s->self, s->x11_chanids[i]);
+		/*
+		 * The channel "id" is already closing, but make sure we
+		 * close all of its siblings.
+		 */
+		if (s->x11_chanids[i] != id)
+			session_close_x11(s->x11_chanids[i]);
+	}
+	xfree(s->x11_chanids);
+	s->x11_chanids = NULL;
+	if (s->display) {
+		xfree(s->display);
+		s->display = NULL;
+	}
+	if (s->auth_proto) {
+		xfree(s->auth_proto);
+		s->auth_proto = NULL;
+	}
+	if (s->auth_data) {
+		xfree(s->auth_data);
+		s->auth_data = NULL;
+	}
+	if (s->auth_display) {
+		xfree(s->auth_display);
+		s->auth_display = NULL;
+	}
+}
+
+static void
 session_exit_message(Session *s, int status)
 {
 	Channel *c;
+	u_int i;
 
 	if ((c = channel_lookup(s->chanid)) == NULL)
 		fatal("session_exit_message: session %d: no channel %d",
@@ -2102,6 +2188,14 @@ session_exit_message(Session *s, int status)
 	if (c->ostate != CHAN_OUTPUT_CLOSED)
 		chan_write_failed(c);
 	s->chanid = -1;
+
+	/* Close any X11 listeners associated with this session */
+	if (s->x11_chanids != NULL) {
+		for (i = 0; s->x11_chanids[i] != -1; i++) {
+			session_close_x11(s->x11_chanids[i]);
+			s->x11_chanids[i] = -1;
+		}
+	}
 }
 
 void
@@ -2116,6 +2210,8 @@ session_close(Session *s)
 		xfree(s->term);
 	if (s->display)
 		xfree(s->display);
+	if (s->x11_chanids)
+		xfree(s->x11_chanids);
 	if (s->auth_display)
 		xfree(s->auth_display);
 	if (s->auth_data)
@@ -2154,6 +2250,7 @@ void
 session_close_by_channel(int id, void *arg)
 {
 	Session *s = session_by_channel(id);
+
 	if (s == NULL) {
 		debug("session_close_by_channel: no session for id %d", id);
 		return;
@@ -2234,6 +2331,7 @@ session_setup_x11fwd(Session *s)
 	struct stat st;
 	char display[512], auth_display[512];
 	char hostname[MAXHOSTNAMELEN];
+	u_int i;
 
 	if (no_x11_forwarding_flag) {
 		packet_send_debug("X11 forwarding disabled in user configuration file.");
@@ -2259,10 +2357,14 @@ session_setup_x11fwd(Session *s)
 	}
 	if (x11_create_display_inet(options.x11_display_offset,
 	    options.x11_use_localhost, s->single_connection,
-	    &s->display_number) == -1) {
+	    &s->display_number, &s->x11_chanids) == -1) {
 		debug("x11_create_display_inet failed.");
 		return 0;
 	}
+	for (i = 0; s->x11_chanids[i] != -1; i++) {
+		channel_register_cleanup(s->x11_chanids[i],
+		    session_close_single_x11);
+	}
 
 	/* Set up a suitable value for the DISPLAY variable. */
 	if (gethostname(hostname, sizeof(hostname)) < 0)
diff --git a/session.h b/session.h
index 92bd1657..a2598a99 100644
--- a/session.h
+++ b/session.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: session.h,v 1.24 2005/06/17 02:44:33 djm Exp $	*/
+/*	$OpenBSD: session.h,v 1.25 2005/07/17 06:49:04 djm Exp $	*/
 
 /*
  * Copyright (c) 2000, 2001 Markus Friedl.  All rights reserved.
@@ -49,6 +49,7 @@ struct Session {
 	int	single_connection;
 	/* proto 2 */
 	int	chanid;
+	int	*x11_chanids;
 	int	is_subsystem;
 	u_int	num_env;
 	struct {