From 2b9b045d930b8d724f768fafdd9a46fa453b9a3e Mon Sep 17 00:00:00 2001 From: Damien Miller Date: Sun, 17 Jul 2005 17:19:24 +1000 Subject: [PATCH] - (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@ --- ChangeLog | 10 ++++- channels.c | 10 ++++- channels.h | 4 +- session.c | 106 ++++++++++++++++++++++++++++++++++++++++++++++++++++- session.h | 3 +- 5 files changed, 125 insertions(+), 8 deletions(-) diff --git a/ChangeLog b/ChangeLog index 647ad0160..84b82cccc 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 b7ff85007..8da399b69 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 b89b7c95d..1cb2c3a34 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 @@ -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 13c3b001f..81d7d53e8 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; } @@ -1706,6 +1707,29 @@ session_by_channel(int id) return NULL; } +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) { @@ -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); @@ -2059,10 +2088,67 @@ sig2name(int sig) return "SIG@openssh.com"; } +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 92bd16573..a2598a99c 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 {