Date: Thu, 16 Dec 2004 13:28:43 +0100 (CET) From: Jilles Tjoelker <jilles@stack.nl> To: FreeBSD-gnats-submit@FreeBSD.org Subject: ports/75147: [PATCH] Several improvements to security/putty, mainly pterm Message-ID: <20041216122843.A1D281D9BC@turtle.stack.nl> Resent-Message-ID: <200412161230.iBGCUU9S018248@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
>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 <features.h>
+#endif
+#include <sys/param.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
@@ -32,6 +35,10 @@
#include <sys/ioctl.h>
#include <errno.h>
+#ifdef __FreeBSD__
+#include <sys/stat.h>
+#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 <bsd.port.post.mk>
--- putty-Makefile.patch ends here ---
>Release-Note:
>Audit-Trail:
>Unformatted:
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20041216122843.A1D281D9BC>