[Groff] Bug#56702: marked as forwarded ([PATCH] /tmp race in gxditview, suspected poor temp file handling in grohtml, etc)

[Debian Bug Tracking System]

Your message dated Mon, 31 Jan 2000 11:49:07 +0200
with message-id <address@hidden>
and subject line Bug#56702: [PATCH] /tmp race in gxditview, suspected poor temp 
file handling in grohtml, etc
has caused the Debian Bug report #56702,
regarding [PATCH] /tmp race in gxditview, suspected poor temp file handling in 
grohtml, etc
to be marked as having been forwarded to the upstream software
author(s) address@hidden

(NB: If you are a system administrator and have no idea what I am
talking about this indicates a serious mail system misconfiguration
somewhere.  Please contact me immediately.)

Darren Benham
(administrator, Debian Bugs database)

Received: (at submit) by bugs.debian.org; 31 Jan 2000 08:34:10 +0000
Received: (qmail 8351 invoked from network); 31 Jan 2000 08:34:09 -0000
Received: from puce.csi.cam.ac.uk (address@hidden)
  by master.debian.org with SMTP; 31 Jan 2000 08:34:09 -0000
Received: from crp22.trin.cam.ac.uk ([131.111.193.222] ident=mail)
        by puce.csi.cam.ac.uk with esmtp (Exim 3.13 #1)
        id 12FCHb-0000ds-00; Mon, 31 Jan 2000 08:34:03 +0000
Received: from cph by crp22.trin.cam.ac.uk with local (Exim 3.12 #1 (Debian))
        id 12FCHf-0001Cz-00; Mon, 31 Jan 2000 08:34:07 +0000
From: Colin Phipps <address@hidden>
To: Debian Bug Tracking System <address@hidden>
Subject: [PATCH] /tmp race in gxditview, suspected poor temp file handling in 
grohtml, etc
X-Reportbug-Version: ##VERSION##
X-Mailer: reportbug ##VERSION##
Date: Mon, 31 Jan 2000 08:34:07 +0000
Message-Id: <address@hidden>

Package: groff
Version: 1.15-3
Severity: normal

1) The HTML output module of groff uses files in /tmp to store some images;
the files are opened with predictable names without checking that they don't
exist first.

2) xditview creates a temporary file in /tmp, with mktemp and fopen, which
is again insecure because a file could be placed there between the two
calls (as documented in the libc docs for mktemp). 

3) indxbib creates temporary files; it uses mktemp and creat to open these
files, which is not safe since a file could be placed there between the 
two calls.

4) xtotroff (seems to be a helper program for xditview) has a minor race in
opening output files.

(3) is minor as the temporary file isn't in /tmp; (4) is very minor, I just
came across it and fixed it while searching for problems. (2) is a standard
/tmp race, but not too dangerous.

(1) is very serious on paper, from what I'm seeing in the code. But I don't
know much at all about groff, and I don't know how to trigger the code in
question. I would have filed this bug as grave if I could confirm the bug.
I'm hoping you know more, and maybe have some test documents that can be
used to trigger this code. It seems to relate to images in html output of
groff's HTML filter.

Fragment of strace for (2): (from strace -f groff -X cat.1)

698   execve("/usr/bin/X11/gxditview", ["gxditview", "-title",
"/home/cph/cat.1", "-xrm", "*iconName:/home/cph/cat.1", "-filename", "|
groff -Z -X /home/cph/cat.1", "-printCommand", "grops | lpr", "-"], [/* 34
vars */] <unfinished ...>
698   <... execve resumed> )            = 0
...
698   getpid()                          = 698
698   stat("/tmp/dvi01Zqy9", 0xbfffeffc) = -1 ENOENT (No such file or directory)
698   open("/tmp/dvi01Zqy9", O_RDWR|O_CREAT|O_TRUNC, 0666) = 5
698   unlink("/tmp/dvi01Zqy9")          = 0
698   uname({sys="Linux", node="crp22", ...}) = 0
698   write(4, "5\1\4\0\30\0\0\4%\0\0\0\2\0\2\0007\377\6\0\31\0\0\4\30"..., 
652) = 652
698   read(4, "\1\3739\0\0\0\0\0\342\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 32) 
= 32
698   gettimeofday({949303948, 865906}, NULL) = 0
698   getpid()                          = 698
698   stat("/tmp/dvi0Si0L8", 0xbffff4fc) = -1 ENOENT (No such file or 
directory)698   open("/tmp/dvi0Si0L8", O_RDWR|O_CREAT|O_TRUNC, 0666) = 6
698   unlink("/tmp/dvi0Si0L8")          = 0

Fragment of strace for (3): (from indxbib cat.1)

...
getpid()                                = 690
stat("/home/cph/indxbibH3PFF6", 0xbffff9fc) = -1 ENOENT (No such file or 
directory)
rt_sigaction(SIGHUP, {0x804a71c, [], SA_RESTART|0x4000000}, {SIG_DFL}, 8) = 0
rt_sigaction(SIGINT, {0x804a71c, [], SA_RESTART|0x4000000}, {SIG_DFL}, 8) = 0
rt_sigaction(SIGTERM, {0x804a71c, [], SA_RESTART|0x4000000}, {SIG_DFL}, 8) = 0
open("/home/cph/indxbibH3PFF6", O_WRONLY|O_CREAT|O_TRUNC, 0444) = 4
fcntl(4, F_GETFL)                       = 0x1 (flags O_WRONLY)
...

Here is the patch fixing all four problems:

diff -pur groff-1.15-old/grohtml/html.cc groff-1.15/grohtml/html.cc
--- groff-1.15-old/grohtml/html.cc      Tue Dec 14 23:25:41 1999
+++ groff-1.15/grohtml/html.cc  Mon Jan 31 08:03:20 2000
@@ -34,6 +34,9 @@ Foundation, 59 Temple Place - Suite 330,
 #include <unistd.h>
 #endif
 
+#include <stdio.h>
+#include <fcntl.h>
+
 #include "ordered_list.h"
 
 #if !defined(TRUE)
@@ -2128,20 +2131,6 @@ int html_printer::is_less (graphic_glob 
   return( (g->minv < t->minv) || ((g->minv == t->minv) && (g->minh < t->minh)) 
);
 }
 
-static FILE *create_file (char *filename)
-{
-  FILE *f;
-
-  errno = 0;
-  f = fopen(filename, "w");
-  if (f == 0) {
-    error("can't create `%1'", filename);
-    return( 0 );
-  } else {
-    return( f );
-  }
-}
-
 void html_printer::convert_to_image (char *name)
 {
   char buffer[1024];
@@ -2207,9 +2196,13 @@ void html_printer::display_globs (int is
   end_paragraph();
 
   if (! is_to_html) {
+    int fd;
     is_center = html_position_region();
     create_temp_name(name, "troff");
-    f = create_file(name);
+    /* This file is in /tmp, so open carefully */
+    fd = open(name, O_WRONLY | O_CREAT | O_EXCL, 0600);
+    f = fdopen(fd, "w");
+    /* FIXME: No error handling here */
     troff.set_file(f);
     prologue();
     output_style.f = 0;
diff -pur groff-1.15-old/indxbib/indxbib.cc groff-1.15/indxbib/indxbib.cc
--- groff-1.15-old/indxbib/indxbib.cc   Fri May 21 05:50:43 1999
+++ groff-1.15/indxbib/indxbib.cc       Mon Jan 31 08:02:13 2000
@@ -219,10 +219,8 @@ int main(int argc, char **argv)
   else {
     temp_index_file = strsave(TEMP_INDEX_TEMPLATE);
   }
-  if (!mktemp(temp_index_file) || !temp_index_file[0])
-    fatal("cannot create file name for temporary file");
   catch_fatal_signals();
-  int fd = creat(temp_index_file, S_IRUSR|S_IRGRP|S_IROTH);
+  int fd = mkstemp(temp_index_file);
   if (fd < 0)
     fatal("can't create temporary index file: %1", strerror(errno));
   indxfp = fdopen(fd, "w");
diff -pur groff-1.15-old/xditview/Dvi.c groff-1.15/xditview/Dvi.c
--- groff-1.15-old/xditview/Dvi.c       Sat Sep 11 08:06:32 1999
+++ groff-1.15/xditview/Dvi.c   Mon Jan 31 07:55:28 2000
@@ -379,14 +379,9 @@ static void CloseFile (dw)
 static void OpenFile (dw)
        DviWidget       dw;
 {
-       char    tmpName[sizeof ("/tmp/dviXXXXXX")];
-
        dw->dvi.tmpFile = 0;
        if (!dw->dvi.seek) {
-               strcpy (tmpName, "/tmp/dviXXXXXX");
-               mktemp (tmpName);
-               dw->dvi.tmpFile = fopen (tmpName, "w+");
-               unlink (tmpName);
+               dw->dvi.tmpFile = tmpfile();
        }
        dw->dvi.requested_page = 1;
        dw->dvi.last_page = 0;
diff -pur groff-1.15-old/xditview/xtotroff.c groff-1.15/xditview/xtotroff.c
--- groff-1.15-old/xditview/xtotroff.c  Fri May 28 04:20:53 1999
+++ groff-1.15/xditview/xtotroff.c      Mon Jan 31 08:11:45 2000
@@ -7,6 +7,9 @@
 #include       <X11/Xlib.h>
 #include       <stdio.h>
 #include       <ctype.h>
+#include       <unistd.h>
+#include       <stdlib.h>
+#include       <fcntl.h>
 #include       "XFontName.h"
 #include       "DviChar.h"
 
@@ -148,8 +151,12 @@ MapFont (font_name, troff_name)
                
        printf ("%s -> %s\n", names[0], troff_name);
 
-       (void) unlink (troff_name);
-       out = fopen (troff_name, "w");
+       { /* Avoid race opening file */
+         int fd;
+         (void) unlink (troff_name);
+         fd = open (troff_name, O_WRONLY | O_CREAT | O_EXCL, 0600);
+         out = fdopen (fd, "w");
+       }
        if (!out) {
                perror (troff_name);
                return 0;

I've done simple tests that indxbib, xtotroff and gxditview still work and 
that the bug is fixed. The patch to html.cc is untested for the reason given
above, but it compiles cleanly.

Colin

-- System Information
Debian Release: woody
Architecture: i386
Kernel: Linux crp22 2.2.15pre5-cph1 #4 Fri Jan 28 20:21:15 GMT 2000 i686

Versions of packages groff depends on:
ii  libc6                         2.1.2-12   GNU C Library: Shared libraries an
ii  libstdc++2.10                 1:2.95.2-5 The GNU stdc++ library