Date: Wed, 1 Mar 2000 18:39:51 -0500
From: Branden Robinson <[email protected]>
To: [email protected]Subject: [XFree86 3.3.6] fix for race conditions in xterm logfile handling
--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 <[email protected]> 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
[email protected] | 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--
