--y0Ed1hDcWxc3B7cn Content-Type: multipart/mixed; boundary="w2JjAQZceEVGylhD" Content-Disposition: inline --w2JjAQZceEVGylhD Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Morton Welinder <terraat_private> reported problems with potential race conditions in xterm's log file handling to BugTraq. Via a detour involving Olaf Kirch and the Debian security team, it made its way to me. Here's a possible fix. Olaf had a different one, but it is applicable only to Linux distributions that use Red Hat's utempter apparatus, which I understand is a kind of clone of utmpd. My fix tries to solve things closer to the root of the problem and (hopefully) will work on any architecture xterm builds on. Here's the lowdown: Tekproc.c: There's a logging feature here that doesn't use the creat_as() function defined in misc.c. Changed O_TRUNC to O_EXCL when opening the logfile, which has a timestamp in its name, so it seems excusable to fail on an existing file. main.c: Instead of logging to xterm.debug.log, which is an easily guessable name for a symlink race attack, a datestamp in the same manner as Tekproc.c is appended. If the call to creat_as() fails, the log file is not opened. Also, when I tested this patch, defining DEBUG uncovered an existing error: setfileno() is not a C library function in Linux as far as I can tell. I replaced this call with an fclose and a redirection of the stderr stream. While I was at it I added a #include for the header file that the getpt() function is protyped in to try and silence a compiler warning. I then found out that glibc doesn't actually have a prototype for it in their header files (contrary to their documentation). Oops. This last part has nothing to do with the security fix, I just hate compiler warnings. misc.c: The function creat_as() is used to try an ensure a safe logfile opening. I changed it a bit. It now returns an int instead of a void, reporting whether it's safe to proceed operating on the file or not. Changed O_APPEND to O_EXCL; this seems to make sense now that the logfile names are more unique. The safe creation of the logfile takes place within a child process so I modified the wait() and waitpid() calls to check the exit status accordingly. Modified StartLog() function to check the return value of creat_as(). resize.c: Added a #include to shut up a compiler warning. This one worked. xterm.h: Changed prototype of creat_as() to match its new definition. I'd appreciate any comments or suggestions for improvement of this patch. I should also note that unless a vendor has taken steps to change this, the #defines that activate the logging code aren't even on, except on OS/2. So these race conditions don't pose a danger to most users. Nevertheless, it seems worthwhile to try to fix problems that get written up on Bugtraq. --=20 G. Branden Robinson | I am sorry, but what you have mistaken Debian GNU/Linux | for malicious intent is nothing more brandenat_private | than sheer incompetence! roger.ecn.purdue.edu/~branden/ | -- J. L. Rizzo II --w2JjAQZceEVGylhD Content-Type: text/plain; charset=us-ascii Content-Description: xterm_logging_race_fixes.diff Content-Disposition: attachment; filename="031_xterm_logging_race_fixes.diff" Content-Transfer-Encoding: quoted-printable --- xc/programs/xterm/Tekproc.c.orig Wed Mar 1 09:10:52 2000 +++ xc/programs/xterm/Tekproc.c Wed Mar 1 11:54:58 2000 @@ -1726,7 +1726,8 @@ =20 setgid(screen->gid); setuid(screen->uid); - tekcopyfd =3D open(buf, O_WRONLY | O_CREAT | O_TRUNC, 0666); + /* Close the window of opportunity by opening with O_EXCL. */ + tekcopyfd =3D open(buf, O_WRONLY | O_CREAT | O_EXCL, 0666); if (tekcopyfd < 0) _exit(1); sprintf(initbuf, "%c%c%c%c", --- xc/programs/xterm/main.c.orig Wed Mar 1 09:18:05 2000 +++ xc/programs/xterm/main.c Wed Mar 1 13:05:34 2000 @@ -119,7 +119,11 @@ #define BSDLY 0 #define VTDLY 0 #define FFDLY 0 +#else /* MINIX */ +#ifdef DEBUG +#include <time.h> #endif +#endif /* MINIX */ =20 #ifdef att #define ATT @@ -179,6 +183,7 @@ #undef HAS_LTCHARS #if __GLIBC__ >=3D 2 #include <pty.h> +#include <stdlib.h> /* getpt() */ #endif #endif =20 @@ -1748,10 +1753,20 @@ /* Set up stderr properly. Opening this log file cannot be done securely by a privileged xterm process (although we try), so the debug feature is disabled by default. */ + time_t tstamp; + struct tm *tstruct; + char dbglogfile[35]; int i =3D -1; if(debug) { - creat_as (getuid(), getgid(), "xterm.debug.log", 0666); - i =3D open ("xterm.debug.log", O_WRONLY | O_TRUNC, 0666); + time(&tstamp); + tstruct =3D localtime(&tstamp); + sprintf(dbglogfile, "xterm.debug.log.%d-%02d-%02d.%02d:%02d:%02d", + tstruct->tm_year + 1900, tstruct->tm_mon + 1, + tstruct->tm_mday, tstruct->tm_hour, + tstruct->tm_min, tstruct->tm_sec); + if(creat_as (getuid(), getgid(), dbglogfile, 0666)) { + i =3D open (dbglogfile, O_WRONLY | O_TRUNC, 0666); + } } if(i >=3D 0) { #if defined(USE_SYSV_TERMIO) && !defined(SVR4) && !defined(linux) @@ -1772,7 +1787,8 @@ #ifndef linux stderr->_file =3D i; #else - setfileno(stderr, i); + fclose(stderr); + stderr =3D fdopen(i, "w"); /* you can do this with glibc */ #endif #endif /* USE_SYSV_TERMIO */ =20 --- xc/programs/xterm/misc.c.orig Wed Mar 1 09:19:17 2000 +++ xc/programs/xterm/misc.c Wed Mar 1 12:20:40 2000 @@ -34,6 +34,9 @@ #include <ctype.h> #include <pwd.h> =20 +#include <sys/types.h> +#include <sys/wait.h> + #include <X11/Xatom.h> #include <X11/cursorfont.h> =20 @@ -506,21 +509,26 @@ #if defined(ALLOWLOGGING) || defined(DEBUG) =20 /* - * create a file only if we could with the permissions of the real user id. + * Create a file only if we could with the permissions of the real user id. * We could emulate this with careful use of access() and following * symbolic links, but that is messy and has race conditions. * Forking is messy, too, but we can't count on setreuid() or saved set-ui= ds * being available. * - * Note: when called for user logging, we have ensured that the real and + * Note: When called for user logging, we have ensured that the real and * effective user ids are the same, so this remains as a convenience funct= ion * for the debug logs. + * + * Returns 1 if we can proceed to open the file in relative safety, 0 + * otherwise. */ -void +int creat_as(int uid, int gid, char *pathname, int mode) { int fd; int pid; + int retval =3D 0; + int childstat; #ifndef HAVE_WAITPID int waited; SIGNAL_T (*chldfunc) (int); @@ -534,19 +542,19 @@ case 0: /* child */ setgid(gid); setuid(uid); - fd =3D open(pathname, O_WRONLY|O_CREAT|O_APPEND, mode); + fd =3D open(pathname, O_WRONLY|O_CREAT|O_EXCL, mode); if (fd >=3D 0) { close(fd); _exit(0); } else _exit(1); case -1: /* error */ - return; + return retval; default: /* parent */ #ifdef HAVE_WAITPID - waitpid(pid, NULL, 0); + waitpid(pid, &childstat, 0); #else - waited =3D wait(NULL); + waited =3D wait(&childstat); signal(SIGCHLD, chldfunc); /* Since we had the signal handler uninstalled for a while, @@ -558,6 +566,8 @@ Cleanup(0); while ( (waited=3Dnonblocking_wait()) > 0); #endif + if(WIFEXITED(childstat)) retval =3D 1; + return retval; } } #endif @@ -673,19 +683,17 @@ #endif } else { if(access(screen->logfile, F_OK) !=3D 0) { - if (errno =3D=3D ENOENT) - creat_as(screen->uid, screen->gid, - screen->logfile, 0644); - else - return; + if (errno !=3D ENOENT) return; } =20 + if(!(creat_as(screen->uid, screen->gid, screen->logfile, 0644)) + return; if(access(screen->logfile, F_OK) !=3D 0 || access(screen->logfile, W_OK) !=3D 0) return; if((screen->logfd =3D open(screen->logfile, O_WRONLY | O_APPEND, 0644)) < 0) - return; + return; } screen->logstart =3D CURRENT_EMU_VAL(screen, Tbptr, bptr); screen->logging =3D TRUE; --- xc/programs/xterm/resize.c.orig Wed Mar 1 12:53:48 2000 +++ xc/programs/xterm/resize.c Wed Mar 1 13:00:21 2000 @@ -282,6 +282,7 @@ #endif #else #include <curses.h> +#include <term.h> /* tgetent() */ #endif /* HAVE_TERMCAP_H */ #endif =20 --- xc/programs/xterm/xterm.h.orig Wed Mar 1 11:53:37 2000 +++ xc/programs/xterm/xterm.h Wed Mar 1 11:54:22 2000 @@ -223,6 +223,7 @@ extern int XStrCmp (char *s1, char *s2); extern int xerror (Display *d, XErrorEvent *ev); extern int xioerror (Display *dpy); +extern int creat_as (int uid, int gid, char *pathname, int mode); extern void Bell (int which, int percent); extern void Changename (char *name); extern void Changetitle (char *name); @@ -241,7 +242,6 @@ extern void Setenv (char *var, char *value); extern void SysError (int i); extern void VisualBell (void); -extern void creat_as (int uid, int gid, char *pathname, int mode); extern void do_dcs (Char *buf, size_t len); extern void do_osc (Char *buf, int len); extern void do_xevents (void); --w2JjAQZceEVGylhD-- --y0Ed1hDcWxc3B7cn Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.0.1 (GNU/Linux) Comment: For info see http://www.gnupg.org iEYEARECAAYFAji9qkYACgkQ6kxmHytGonwJrgCfXQVKtJwB7e+eltj3y2Zh5hsX 7TQAn2G67wPYW1lpbzSh38vFwDE/B2QB =vpRj -----END PGP SIGNATURE----- --y0Ed1hDcWxc3B7cn--
This archive was generated by hypermail 2b30 : Fri Apr 13 2001 - 15:38:43 PDT