COMMAND
xterm
SYSTEMS AFFECTED
xterm
PROBLEM
Morten Welinder found following. It used to be Well Known that
xterm's way of opening a log file was insecure. Well, that was
5+ years ago. Things have changed, but mostly to "different"
rather than "better".
When log files are enabled, they are created in the following way
(checking in XFree86 3.3.6 source; matches Solaris binaries) and
are subject to race conditions:
1. File is checked for existance using access.
2. If file does not exist, it is created in a subprocess using
user's real uid/gid. [ok]
3. File is checked for existance using access.
4. File is checked for write permission using access.
5. File is opened O_WRONLY | O_APPEND. [plonk]
A little symlink magic between 4 and 5 and you have write access
to any file if your xterm is setuid/setgid. General attack idea:
ls -lL `which xterm`
# If not setuid/setgid, you are safe
touch dummy
symlink-flipflop link dummy /.rhosts
xterm -l -lf link -e echo + +
SOLUTION
access() is totally useless for security purposes. Use it only
as a means of providing better error messages (as it might not be
easy to get an error message out from a subprocess).
XFree86 3.3.6 doesn't seem to be vulnerable by default - from
xc/programs/xterm/misc.c:
#ifdef ALLOWLOGGING
/*
* Logging is a security hole, since it allows a setuid program to write
* arbitrary data to an arbitrary file. So it is disabled by default.
*/
Here's a possible fix. 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 tested this patch, defining DEBUG
uncovered an existing error: setfileno() is not a C
library function in Linux as far as one can tell. It
was replaced this call with an fclose and a redirection
of the stderr stream. While Branden Robinson was at
it he added a #include for the header file that the
getpt() function is protyped in to try and silence a
compiler warning. He then found out that glibc doesn't
actually have a prototype for it in their header files
(contrary to their documentation). Yes it does, getpt()
is a GNU extension, therefore you have to define the
_GNU_SOURCE macro in order to get the prototype.
misc.c: The function creat_as() is used to try an ensure a safe
logfile opening. Branden 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 he 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.
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.
--- 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 @@
setgid(screen->gid);
setuid(screen->uid);
- tekcopyfd = open(buf, O_WRONLY | O_CREAT | O_TRUNC, 0666);
+ /* Close the window of opportunity by opening with O_EXCL. */
+ tekcopyfd = 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 */
#ifdef att
#define ATT
@@ -179,6 +183,7 @@
#undef HAS_LTCHARS
#if __GLIBC__ >= 2
#include <pty.h>
+#include <stdlib.h> /* getpt() */
#endif
#endif
@@ -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 = -1;
if(debug) {
- creat_as (getuid(), getgid(), "xterm.debug.log", 0666);
- i = open ("xterm.debug.log", O_WRONLY | O_TRUNC, 0666);
+ time(&tstamp);
+ tstruct = 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 = open (dbglogfile, O_WRONLY | O_TRUNC, 0666);
+ }
}
if(i >= 0) {
#if defined(USE_SYSV_TERMIO) && !defined(SVR4) && !defined(linux)
@@ -1772,7 +1787,8 @@
#ifndef linux
stderr->_file = i;
#else
- setfileno(stderr, i);
+ fclose(stderr);
+ stderr = fdopen(i, "w"); /* you can do this with glibc */
#endif
#endif /* USE_SYSV_TERMIO */
--- 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>
+#include <sys/types.h>
+#include <sys/wait.h>
+
#include <X11/Xatom.h>
#include <X11/cursorfont.h>
@@ -506,21 +509,26 @@
#if defined(ALLOWLOGGING) || defined(DEBUG)
/*
- * 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-uids
* 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 function
* 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 = 0;
+ int childstat;
#ifndef HAVE_WAITPID
int waited;
SIGNAL_T (*chldfunc) (int);
@@ -534,19 +542,19 @@
case 0: /* child */
setgid(gid);
setuid(uid);
- fd = open(pathname, O_WRONLY|O_CREAT|O_APPEND, mode);
+ fd = open(pathname, O_WRONLY|O_CREAT|O_EXCL, mode);
if (fd >= 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 = wait(NULL);
+ waited = wait(&childstat);
signal(SIGCHLD, chldfunc);
/*
Since we had the signal handler uninstalled for a while,
@@ -558,6 +566,8 @@
Cleanup(0);
while ( (waited=nonblocking_wait()) > 0);
#endif
+ if(WIFEXITED(childstat)) retval = 1;
+ return retval;
}
}
#endif
@@ -673,19 +683,17 @@
#endif
} else {
if(access(screen->logfile, F_OK) != 0) {
- if (errno == ENOENT)
- creat_as(screen->uid, screen->gid,
- screen->logfile, 0644);
- else
- return;
+ if (errno != ENOENT) return;
}
+ if(!(creat_as(screen->uid, screen->gid, screen->logfile, 0644))
+ return;
if(access(screen->logfile, F_OK) != 0
|| access(screen->logfile, W_OK) != 0)
return;
if((screen->logfd = open(screen->logfile, O_WRONLY | O_APPEND,
0644)) < 0)
- return;
+ return;
}
screen->logstart = CURRENT_EMU_VAL(screen, Tbptr, bptr);
screen->logging = 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
--- 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);