4857545 2000-03-02 19:41 /299 rader/ Postmaster
Mottagare: Bugtraq (import) <10084>
Ärende: [XFree86 3.3.6] fix for race conditions in xterm logfile handling
------------------------------------------------------------
Approved-By: aleph1@SECURITYFOCUS.COM
Delivered-To: bugtraq@lists.securityfocus.com
Delivered-To: BUGTRAQ@SECURITYFOCUS.COM
Mime-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha1
protocol="application/pgp-signature"; boundary="y0Ed1hDcWxc3B7cn"
Content-Disposition: inline
User-Agent: Mutt/1.1.5i
Message-ID: <20000301183950.D14657@ecn.purdue.edu>
Date: Wed, 1 Mar 2000 18:39:51 -0500
Reply-To: Branden Robinson <branden@ECN.PURDUE.EDU>
Sender: Bugtraq List <BUGTRAQ@SECURITYFOCUS.COM>
From: Branden Robinson <branden@ECN.PURDUE.EDU>
X-To: patch@xfree86.org
X-cc: dickey@clark.net, wakkerma@debian.org, BUGTRAQ@SECURITYFOCUS.COM
To: BUGTRAQ@SECURITYFOCUS.COM
--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 <terra@DIKU.DK> 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
branden@ecn.purdue.edu | 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--
(4857545) ------------------------------------------(Ombruten)
4861174 2000-03-03 20:45 /30 rader/ Postmaster
Mottagare: Bugtraq (import) <10098>
Ärende: Re: [XFree86 3.3.6] fix for race conditions in xterm logfil
------------------------------------------------------------
handling
Approved-By: aleph1@SECURITYFOCUS.COM
Delivered-To: bugtraq@lists.securityfocus.com
Delivered-To: BUGTRAQ@securityfocus.com
Mime-Version: 1.0
X-Sender: jk@mail.espy.org
Content-Type: text/plain; charset="us-ascii"
Message-ID: <p04310104b4e4bc630df8@[206.163.71.146]>
Date: Thu, 2 Mar 2000 16:52:09 -0800
Reply-To: Joel Klecker <jk@ESPY.ORG>
Sender: Bugtraq List <BUGTRAQ@SECURITYFOCUS.COM>
From: Joel Klecker <jk@ESPY.ORG>
X-To: Branden Robinson <branden@ECN.PURDUE.EDU>
BUGTRAQ@SECURITYFOCUS.COM
To: BUGTRAQ@SECURITYFOCUS.COM
In-Reply-To: <20000301183950.D14657@ecn.purdue.edu>
At 18:39 -0500 2000-03-01, Branden Robinson wrote:
> 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).
Yes it does, getpt() is a GNU extension, therefore you have to define
the _GNU_SOURCE macro in order to get the prototype.
--
Joel Klecker (aka Espy) Debian GNU/Linux Developer
<URL:mailto:jk@espy.org> <URL:mailto:espy@debian.org>
<URL:http://web.espy.org/> <URL:http://www.debian.org/>
(4861174) ------------------------------------------(Ombruten)
4870821 2000-03-07 10:30 /34 rader/ Postmaster
Mottagare: Bugtraq (import) <10132>
Ärende: Re: [XFree86 3.3.6] fix for race conditions in xterm logfil
------------------------------------------------------------
handling
Approved-By: aleph1@SECURITYFOCUS.COM
Delivered-To: bugtraq@lists.securityfocus.com
Delivered-To: BUGTRAQ@securityfocus.com
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Message-ID: <20000306103505.E16488@monad.swb.de>
Date: Mon, 6 Mar 2000 10:35:05 +0100
Reply-To: Olaf Kirch <okir@CALDERA.DE>
Sender: Bugtraq List <BUGTRAQ@SECURITYFOCUS.COM>
From: Olaf Kirch <okir@CALDERA.DE>
X-To: BUGTRAQ@SECURITYFOCUS.COM
To: BUGTRAQ@SECURITYFOCUS.COM
In-Reply-To: <20000301183950.D14657@ecn.purdue.edu>; fro
branden@ecn.purdue.edu on Wed, Mar 01, 2000 at 06:39:51PM -0500
On Wed, Mar 01, 2000 at 06:39:51PM -0500, Branden Robinson wrote:
> 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.
Just to clarify this a little; the patch I did was not for this
specifiy problem; it's purpose is to do away with the setuid root
requirement for xterm altogether. The only two reasons for requiring
a setuid root bit on xterm are ptys (taken care of by /dev/pts
nowadays), and utmp management (which we're currently doing using
utempter). The same holds for kterm and kconsole, as well as the
gnome counterparts I assume.
Olaf
--
Olaf Kirch | --- o --- Nous sommes du soleil we love when we play
okir@monad.swb.de | / | \ sol.dhoop.naytheet.ah kin.ir.samse.qurax
okir@caldera.de +-------------------- Why Not?! -----------------------
UNIX, n.: Spanish manufacturer of fire extinguishers.
(4870821) ------------------------------------------(Ombruten)