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

[Fabrizio Polacco]

[Please onor the reply-to which I set to
address@hidden, address@hidden, address@hidden

----- Forwarded message from Colin Phipps <address@hidden> -----

Delivered-To: address@hidden
Subject: Bug#56702: [PATCH] /tmp race in gxditview, suspected poor temp file 
handling in grohtml, etc
Reply-To: Colin Phipps <address@hidden>, address@hidden
X-Debian-PR-Message: report 56702
X-Debian-PR-Package: groff
X-Debian-PR-Keywords: 
From: Colin Phipps <address@hidden>
To: Debian Bug Tracking System <address@hidden>
X-Reportbug-Version: ##VERSION##
Date: Mon, 31 Jan 2000 08:34:07 +0000

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            

----- End forwarded message -----

-- 
| address@hidden     address@hidden    address@hidden
| 6F7267F5 fingerprint 57 16 C4 ED C9 86 40 7B 1A 69 A1 66 EC FB D2 5E
| address@hidden                  gsm: +358 (0)40 707 2468