From owner-freebsd-ports-bugs@FreeBSD.ORG Thu Dec 16 12:30:30 2004 Return-Path: Delivered-To: freebsd-ports-bugs@hub.freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id CDDA516A4CF for ; Thu, 16 Dec 2004 12:30:30 +0000 (GMT) Received: from freefall.freebsd.org (freefall.freebsd.org [216.136.204.21]) by mx1.FreeBSD.org (Postfix) with ESMTP id 5E39B43D39 for ; Thu, 16 Dec 2004 12:30:30 +0000 (GMT) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (gnats@localhost [127.0.0.1]) by freefall.freebsd.org (8.13.1/8.13.1) with ESMTP id iBGCUUR1018249 for ; Thu, 16 Dec 2004 12:30:30 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.13.1/8.13.1/Submit) id iBGCUU9S018248; Thu, 16 Dec 2004 12:30:30 GMT (envelope-from gnats) Resent-Date: Thu, 16 Dec 2004 12:30:30 GMT Resent-Message-Id: <200412161230.iBGCUU9S018248@freefall.freebsd.org> Resent-From: FreeBSD-gnats-submit@FreeBSD.org (GNATS Filer) Resent-To: freebsd-ports-bugs@FreeBSD.org Resent-Reply-To: FreeBSD-gnats-submit@FreeBSD.org, Jilles Tjoelker Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 203D716A4CF for ; Thu, 16 Dec 2004 12:28:46 +0000 (GMT) Received: from mailhost.stack.nl (vaak.stack.nl [131.155.140.140]) by mx1.FreeBSD.org (Postfix) with ESMTP id D196443D49 for ; Thu, 16 Dec 2004 12:28:44 +0000 (GMT) (envelope-from jilles@stack.nl) Received: from turtle.stack.nl (turtle.stack.nl [IPv6:2001:610:1108:5010::132]) by mailhost.stack.nl (Postfix) with ESMTP id C23E01F1CE for ; Thu, 16 Dec 2004 13:28:43 +0100 (CET) Received: by turtle.stack.nl (Postfix, from userid 1677) id A1D281D9BC; Thu, 16 Dec 2004 13:28:43 +0100 (CET) Message-Id: <20041216122843.A1D281D9BC@turtle.stack.nl> Date: Thu, 16 Dec 2004 13:28:43 +0100 (CET) From: Jilles Tjoelker To: FreeBSD-gnats-submit@FreeBSD.org X-Send-Pr-Version: 3.113 Subject: ports/75147: [PATCH] Several improvements to security/putty, mainly pterm X-BeenThere: freebsd-ports-bugs@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list Reply-To: Jilles Tjoelker List-Id: Ports bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 16 Dec 2004 12:30:31 -0000 >Number: 75147 >Category: ports >Synopsis: [PATCH] Several improvements to security/putty, mainly pterm >Confidential: no >Severity: serious >Priority: medium >Responsible: freebsd-ports-bugs >State: open >Quarter: >Keywords: >Date-Required: >Class: sw-bug >Submitter-Id: current-users >Arrival-Date: Thu Dec 16 12:30:29 GMT 2004 >Closed-Date: >Last-Modified: >Originator: Jilles Tjoelker >Release: FreeBSD 5.3-RELEASE-p2 i386 >Organization: MCGV Stack >Environment: System: FreeBSD turtle.stack.nl 5.3-RELEASE-p2 FreeBSD 5.3-RELEASE-p2 #5: Thu Dec 2 17:25:55 CET 2004 jilles@snail.stack.nl:/usr/obj/usr/src/sys/SNAIL i386 >Description: This concerns the security/putty port, which includes an xterm-like program called pterm. pterm sometimes fails to set up the pty properly. FreeBSD does not allow most tty ioctls on the master side unless the slave side is open, and pterm does not guarantee that. This patch makes it open the slave side in the parent process, before fork(2), and close it after forking. Also, it fixes a wrong initialization of pty_utmp_helper_pipe, namely to 0 instead of -1. This wrong initialization can be seen in some bogus output when starting pterm. (This latter thing no longer happens since the helper process is used for resetting permissions.) Additionally, ownership of the pty is now treated properly: if you do not set the setuid bit on pterm, it will use if the first free pty with an accessible slave device; it will still leave the slave device wide open (allowing easy terminal emulation exploits and similar nastiness), but that cannot be helped. If you set the setuid bit, pterm will chown/chmod the pty at start and set it back to root:wheel 666 later. The port does not respect PREFIX. Add an option, WITH_SETUID_PTERM, to make pterm setuid root. >How-To-Repeat: Install security/putty. Make next free /dev/tty[pqrsPQRS]* inaccessible for the current user and run non-setuid pterm. (old version will fail, patched version should work) Run setuid pterm, check which pty is used, exit it and check the permissions on the pty. (old version will not restore owner/permissions, patched version should) >Fix: Replace files/patch-pty.c with this one. Apply putty-Makefile.patch to ports/security/putty/Makefile. --- patch-pty.c begins here --- --- pty.c.orig Thu Dec 16 11:04:19 2004 +++ pty.c Thu Dec 16 11:24:21 2004 @@ -14,8 +14,11 @@ #define _XOPEN_SOURCE #define _XOPEN_SOURCE_EXTENDED #define _GNU_SOURCE +#ifndef __FreeBSD__ #include +#endif +#include #include #include #include @@ -32,6 +35,10 @@ #include #include +#ifdef __FreeBSD__ +#include +#endif + #include "putty.h" #ifndef FALSE @@ -68,6 +75,12 @@ #define HAVE_PUTUTLINE #endif #endif +#ifdef BSD +#define RESTORE_PTY_PERM +#endif +#if !defined(OMIT_UTMP) || defined(RESTORE_PTY_PERM) +#define FORK_HELPER +#endif static Config pty_cfg; static int pty_master_fd; @@ -76,7 +89,7 @@ static int pty_signal_pipe[2]; static int pty_stamped_utmp = 0; static int pty_child_pid; -static int pty_utmp_helper_pid, pty_utmp_helper_pipe; +static int pty_utmp_helper_pid, pty_utmp_helper_pipe = -1; static int pty_term_width, pty_term_height; static int pty_child_dead, pty_finished; static int pty_exit_code; @@ -174,6 +187,16 @@ #endif } +static void restore_pty_perm(void) +{ +#ifdef RESTORE_PTY_PERM + if (pty_name[0] == '\0') + return; + chown(pty_name, 0, 0); + chmod(pty_name, 0666); +#endif +} + static void sigchld_handler(int signum) { write(pty_signal_pipe[1], "x", 1); @@ -183,6 +206,7 @@ { putty_signal(signum, SIG_DFL); cleanup_utmp(); + restore_pty_perm(); setuid(getuid()); raise(signum); } @@ -190,8 +214,13 @@ static void pty_open_master(void) { #ifdef BSD_PTYS +#ifdef BSD + const char chars1[] = "pqrsPQRS"; + const char chars2[] = "0123456789abcdefghijklmnopqrstuv"; +#else const char chars1[] = "pqrstuvwxyz"; const char chars2[] = "0123456789abcdef"; +#endif const char *p1, *p2; char master_name[20]; struct group *gp; @@ -201,6 +230,7 @@ sprintf(master_name, "/dev/pty%c%c", *p1, *p2); pty_master_fd = open(master_name, O_RDWR); if (pty_master_fd >= 0) { + master_name[5] = 't'; /* /dev/ptyXX -> /dev/ttyXX */ if (geteuid() == 0 || access(master_name, R_OK | W_OK) == 0) goto got_one; @@ -214,7 +244,6 @@ got_one: strcpy(pty_name, master_name); - pty_name[5] = 't'; /* /dev/ptyXX -> /dev/ttyXX */ /* We need to chown/chmod the /dev/ttyXX device. */ gp = getgrnam("tty"); @@ -270,17 +299,19 @@ pty_open_master(); } -#ifndef OMIT_UTMP +#ifdef FORK_HELPER /* * Fork off the utmp helper. */ if (pipe(pipefd) < 0) { perror("pterm: pipe"); + restore_pty_perm(); exit(1); } pid = fork(); if (pid < 0) { perror("pterm: fork"); + restore_pty_perm(); exit(1); } else if (pid == 0) { char display[128], buffer[128]; @@ -288,6 +319,52 @@ close(pipefd[1]); /* + * Trap as many fatal signals as we can in the + * hope of having the best possible chance to + * clean up utmp and restore the pty permissions + * before termination. We are unfortunately + * unprotected against SIGKILL, but that's life. + */ + putty_signal(SIGHUP, fatal_sig_handler); + putty_signal(SIGINT, fatal_sig_handler); + putty_signal(SIGQUIT, fatal_sig_handler); + putty_signal(SIGILL, fatal_sig_handler); + putty_signal(SIGABRT, fatal_sig_handler); + putty_signal(SIGFPE, fatal_sig_handler); + putty_signal(SIGPIPE, fatal_sig_handler); + putty_signal(SIGALRM, fatal_sig_handler); + putty_signal(SIGTERM, fatal_sig_handler); + putty_signal(SIGSEGV, fatal_sig_handler); + putty_signal(SIGUSR1, fatal_sig_handler); + putty_signal(SIGUSR2, fatal_sig_handler); +#ifdef SIGBUS + putty_signal(SIGBUS, fatal_sig_handler); +#endif +#ifdef SIGPOLL + putty_signal(SIGPOLL, fatal_sig_handler); +#endif +#ifdef SIGPROF + putty_signal(SIGPROF, fatal_sig_handler); +#endif +#ifdef SIGSYS + putty_signal(SIGSYS, fatal_sig_handler); +#endif +#ifdef SIGTRAP + putty_signal(SIGTRAP, fatal_sig_handler); +#endif +#ifdef SIGVTALRM + putty_signal(SIGVTALRM, fatal_sig_handler); +#endif +#ifdef SIGXCPU + putty_signal(SIGXCPU, fatal_sig_handler); +#endif +#ifdef SIGXFSZ + putty_signal(SIGXFSZ, fatal_sig_handler); +#endif +#ifdef SIGIO + putty_signal(SIGIO, fatal_sig_handler); +#endif + /* * Now sit here until we receive a display name from the * other end of the pipe, and then stamp utmp. Unstamp utmp * again, and exit, when the pipe closes. @@ -299,6 +376,7 @@ ret = read(pipefd[0], buffer, lenof(buffer)); if (ret <= 0) { cleanup_utmp(); + restore_pty_perm(); _exit(0); } else if (!pty_stamped_utmp) { if (dlen < lenof(display)) @@ -310,52 +388,6 @@ * it, and stamp utmp. */ display[lenof(display)-1] = '\0'; - /* - * Trap as many fatal signals as we can in the - * hope of having the best possible chance to - * clean up utmp before termination. We are - * unfortunately unprotected against SIGKILL, - * but that's life. - */ - putty_signal(SIGHUP, fatal_sig_handler); - putty_signal(SIGINT, fatal_sig_handler); - putty_signal(SIGQUIT, fatal_sig_handler); - putty_signal(SIGILL, fatal_sig_handler); - putty_signal(SIGABRT, fatal_sig_handler); - putty_signal(SIGFPE, fatal_sig_handler); - putty_signal(SIGPIPE, fatal_sig_handler); - putty_signal(SIGALRM, fatal_sig_handler); - putty_signal(SIGTERM, fatal_sig_handler); - putty_signal(SIGSEGV, fatal_sig_handler); - putty_signal(SIGUSR1, fatal_sig_handler); - putty_signal(SIGUSR2, fatal_sig_handler); -#ifdef SIGBUS - putty_signal(SIGBUS, fatal_sig_handler); -#endif -#ifdef SIGPOLL - putty_signal(SIGPOLL, fatal_sig_handler); -#endif -#ifdef SIGPROF - putty_signal(SIGPROF, fatal_sig_handler); -#endif -#ifdef SIGSYS - putty_signal(SIGSYS, fatal_sig_handler); -#endif -#ifdef SIGTRAP - putty_signal(SIGTRAP, fatal_sig_handler); -#endif -#ifdef SIGVTALRM - putty_signal(SIGVTALRM, fatal_sig_handler); -#endif -#ifdef SIGXCPU - putty_signal(SIGXCPU, fatal_sig_handler); -#endif -#ifdef SIGXFSZ - putty_signal(SIGXFSZ, fatal_sig_handler); -#endif -#ifdef SIGIO - putty_signal(SIGIO, fatal_sig_handler); -#endif setup_utmp(pty_name, display); } } @@ -510,21 +542,15 @@ pty_open_master(); /* - * Set the backspace character to be whichever of ^H and ^? is - * specified by bksp_is_delete. - */ - { - struct termios attrs; - tcgetattr(pty_master_fd, &attrs); - attrs.c_cc[VERASE] = cfg->bksp_is_delete ? '\177' : '\010'; - tcsetattr(pty_master_fd, TCSANOW, &attrs); - } - - /* * Stamp utmp (that is, tell the utmp helper process to do so), * or not. */ +#ifdef FORK_HELPER +#ifndef RESTORE_PTY_PERM if (!cfg->stamp_utmp) { +#else + if (0) { +#endif close(pty_utmp_helper_pipe); /* just let the child process die */ pty_utmp_helper_pipe = -1; } else { @@ -541,10 +567,28 @@ pos += ret; } } +#endif windowid = get_windowid(pty_frontend); /* + * Open the slave in the parent process, that way it is open before we + * attempt to tcsetattr() or TIOCSWINSZ. FreeBSD requires this. + */ + slavefd = open(pty_name, O_RDWR); + + /* + * Set the backspace character to be whichever of ^H and ^? is + * specified by bksp_is_delete. + */ + { + struct termios attrs; + tcgetattr(pty_master_fd, &attrs); + attrs.c_cc[VERASE] = cfg->bksp_is_delete ? '\177' : '\010'; + tcsetattr(pty_master_fd, TCSANOW, &attrs); + } + + /* * Fork and execute the command. */ pid = fork(); @@ -559,7 +603,6 @@ * We are the child. */ - slavefd = open(pty_name, O_RDWR); if (slavefd < 0) { perror("slave pty: open"); _exit(1); @@ -574,9 +617,9 @@ ioctl(slavefd, TIOCSCTTY, 1); pgrp = getpid(); tcsetpgrp(slavefd, pgrp); - setpgrp(); + setpgrp( pgrp, -1 ); close(open(pty_name, O_WRONLY, 0)); - setpgrp(); + setpgrp( pgrp, -1 ); /* Close everything _else_, for tidiness. */ for (i = 3; i < 1024; i++) close(i); @@ -641,6 +684,7 @@ perror("exec"); _exit(127); } else { + close(slavefd); pty_child_pid = pid; pty_child_dead = FALSE; pty_finished = FALSE; --- patch-pty.c ends here --- --- putty-Makefile.patch begins here --- --- Makefile.orig Thu Oct 28 12:17:28 2004 +++ Makefile Thu Dec 16 13:18:15 2004 @@ -18,6 +18,7 @@ USE_REINPLACE= yes WRKSRC= ${WRKDIR}/${DISTNAME}/unix MAKEFILE= Makefile.gtk +MAKE_ARGS= prefix=${PREFIX} CFLAGS+= -DBSD_PTYS -DOMIT_UTMP .ifndef WITHOUT_IPV6 CFLAGS+= -DIPV6 @@ -44,6 +45,12 @@ do-configure: ${CP} ${FILESDIR}/wcrtomb.c ${FILESDIR}/mbrtowc.c \ ${WRKSRC}/ +.endif + +.ifdef WITH_SETUID_PTERM +post-install: + ${CHOWN} root ${PREFIX}/bin/pterm + ${CHMOD} u+s ${PREFIX}/bin/pterm .endif .include --- putty-Makefile.patch ends here --- >Release-Note: >Audit-Trail: >Unformatted: