[XFree86 3.3.6] fix for race conditions in xterm logfile handling

[branden () ECN PURDUE EDU (Branden Robinson)]

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.

-- 
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


<HR NOSHADE>
<UL>
<LI>text/plain attachment: xterm_logging_race_fixes.diff
</UL>

<HR NOSHADE>
<UL>
<LI>application/pgp-signature attachment: stored
</UL>