Re: Fw: [Groff] Bug#56702: [PATCH] security bugfix for grohtml

[Gaius Mulley]

Jeroen Ruigrok/Asmodai <address@hidden> writes:

>> This whole snippet plus some more really reminds of the functions:
>> 
>> mktemp(), mkstemp(), mkstemps() and these should be pretty portable.

Werner writes:

> I second that.  There is a function called xtmpfile() in
> src/libgroff/tmpfile.cc -- maybe it is possible to extend it so that
> it can be used with grohtml also.

fine ideas, yes they would form a better solution.
Here is an attempt, hope it meets with approval?

many thanks,

Gaius

--- groff-cvs/src/devices/grohtml/html.cc       Mon Feb  7 22:50:19 2000
+++ groff-html/src/devices/grohtml/html.cc      Fri Feb 11 15:13:19 2000
@@ -1063,7 +1063,7 @@
   void  save_paragraph                (void);
   void  restore_paragraph             (void);
   void  html_newline                  (void);
-  void  convert_to_image              (char *name);
+  void  convert_to_image              (char *troff_src, char *image_name);
   void  write_title                   (int in_head);
   void  find_title                    (void);
   int   is_bold                       (text_glob *g);
@@ -1075,7 +1075,6 @@
   int   is_a_header                   (text_glob *g);
   int   processed_header              (text_glob *g);
   void  make_new_image_name           (void);
-  void  create_temp_name              (char *name, char *extension);
   void  calculate_region_margins      (region_glob *r);
   void  remove_redundant_regions      (void);
   void  remove_duplicate_regions      (void);
@@ -2408,54 +2407,32 @@
   return( (g->minv < t->minv) || ((g->minv == t->minv) && (g->minh < t->minh)) 
);
 }
 
-/*
- *  create_tmp_file - opens a filename in /tmp carefully checking for failure
- *                    otherwise security could be circumvented.
- */
-
-static FILE *create_tmp_file (char *filename)
-{
-  FILE *f;
-  int   fd;
-
-  errno = 0;
-  /* This file is in /tmp, so open carefully */
-  fd = open(filename, O_WRONLY | O_CREAT | O_EXCL, 0600);
-  if (fd < 0) {
-    fatal("can't create `%1'", filename);
-  }
-  f  = fdopen(fd, "w");
-  if (f == 0) {
-    fatal("can't create `%1'", filename);
-  }
-  return( f );
-}
-
-void html_printer::convert_to_image (char *name)
+void html_printer::convert_to_image (char *troff_src, char *image_name)
 {
   char buffer[1024];
+  char *ps_src = mktemp(xtmptemplate("-ps-"));
 
-  sprintf(buffer, "grops %s > %s.ps\n", name, name);
+  sprintf(buffer, "grops %s > %s\n", troff_src, ps_src);
   if (debug_on) {
-    fprintf(stderr, "%s", buffer);
+    fprintf(stderr, buffer);
   }
   system(buffer);
 
   if (image_type == gif) {
     sprintf(buffer,
-           "echo showpage | gs -q -dSAFER -sDEVICE=ppmraw -r%d -g%dx%d 
-sOutputFile=- %s.ps - | ppmquant 256  2> /dev/null | ppmtogif  2> /dev/null > 
%s.gif \n",
+           "echo showpage | gs -q -dSAFER -sDEVICE=ppmraw -r%d -g%dx%d 
-sOutputFile=- %s - | ppmquant 256  2> /dev/null | ppmtogif  2> /dev/null > 
%s.gif \n",
            image_res,
            
(end_region_hpos-start_region_hpos)*image_res/font::res+IMAGE_BOARDER_PIXELS,
            
(end_region_vpos-start_region_vpos)*image_res/font::res+IMAGE_BOARDER_PIXELS,
-           name, image_name);
+           ps_src, image_name);
   } else {
     sprintf(buffer,
-           "echo showpage | gs -q -dSAFER -sDEVICE=%s -r%d -g%dx%d 
-sOutputFile=- %s.ps - 2> /dev/null > %s.png \n",
+           "echo showpage | gs -q -dSAFER -sDEVICE=%s -r%d -g%dx%d 
-sOutputFile=- %s - 2> /dev/null > %s.png \n",
            image_device,
            image_res,
            
(end_region_hpos-start_region_hpos)*image_res/font::res+IMAGE_BOARDER_PIXELS,
            
(end_region_vpos-start_region_vpos)*image_res/font::res+IMAGE_BOARDER_PIXELS,
-           name, image_name);
+           ps_src, image_name);
 #if 0
     sprintf(buffer,
            "echo showpage | gs -q -dSAFER -sDEVICE=ppmraw -r%d -g%dx%d 
-sOutputFile=- %s.ps - > %s.pnm ; pnmtopng -transparent white %s.pnm > %s.png 
\n",
@@ -2470,12 +2447,8 @@
     fprintf(stderr, "%s", buffer);
   }
   system(buffer);
-  sprintf(buffer, "/bin/rm -f %s %s.ps\n", name, name);
-  if (debug_on) {
-    fprintf(stderr, "%s", buffer);
-  } else {
-    system(buffer);
-  }
+  unlink(ps_src);
+  unlink(troff_src);
 }
 
 void html_printer::prologue (void)
@@ -2485,18 +2458,12 @@
   troff.put_string(" 1 1\nx init\np1\n");
 }
 
-void html_printer::create_temp_name (char *name, char *extension)
-{
-  make_new_image_name();
-  sprintf(name, "/tmp/grohtml-%d-%ld.%s", image_number, (long)getpid(), 
extension);
-}
-
 void html_printer::display_globs (int is_to_html)
 {
   text_glob    *t=0;
   graphic_glob *g=0;
   FILE         *f=0;
-  char          name[MAX_TEMP_NAME];
+  char         *troff_src;
   int           something=FALSE;
   int           is_center=FALSE;
 
@@ -2504,8 +2471,8 @@
 
   if (! is_to_html) {
     is_center = html_position_region();
-    create_temp_name(name, "troff");
-    f = create_tmp_file(name);
+    make_new_image_name();
+    f = xtmpfile(&troff_src, "-troff-", FALSE);
     troff.set_file(f);
     prologue();
     output_style.f = 0;
@@ -2576,7 +2543,7 @@
   if ((! is_to_html) && (f != 0)) {
     fclose(troff.get_file());
     if (something) {
-      convert_to_image(name);
+      convert_to_image(troff_src, image_name);
 
       if (is_center) {
        end_paragraph();
@@ -2602,7 +2569,6 @@
       output_style.f   = 0;
       end_paragraph();
     }
-    // unlink(name);  // remove troff file
   }
 }
 
--- groff-cvs/src/devices/grohtml/ChangeLog     Mon Feb  7 22:50:19 2000
+++ groff-html/src/devices/grohtml/ChangeLog    Fri Feb 11 15:58:43 2000
@@ -1,3 +1,10 @@
+2000-02-11  Gaius Mulley  <address@hidden>
+
+       * html.cc: removed create_tmp_file and replaced any dependant calls
+       for xtmpfile and mktemp(xtmptemplate()) as suggested by Werner
+       and Jeroen Ruigrok/Asmodai <address@hidden>.
+       Also removed create_temp_name.
+
 2000-02-07  Gaius Mulley  <address@hidden>
 
        * html.cc (html_printer::make_new_image_name): Tidied up file and
--- groff-cvs/src/libs/libgroff/tmpfile.cc      Sun Feb  6 09:37:16 2000
+++ groff-html/src/libs/libgroff/tmpfile.cc     Fri Feb 11 15:13:34 2000
@@ -43,11 +43,22 @@
 // Use this as the prefix for temporary filenames.
 #define TMPFILE_PREFIX  "groff"
 
-// Open a temporary file with fatal error on failure.
+/*
+ *  Generate a temporary name template with a, postfix,
+ *  immediately after the TMPFILE_PREFIX.
+ *  It uses the groff preferences for temporary directory.
+ *  Note no file name is either created or opened
+ *  only the *template* is returned.
+ */
 
-FILE *xtmpfile()
+char *xtmptemplate(char *postfix=0)
 {
   const char *dir = getenv(GROFF_TMPDIR_ENVVAR);
+  int     postlen = 0;
+
+  if (postfix)
+    postlen = strlen(postfix);
+    
   if (!dir) {
     dir = getenv(TMPDIR_ENVVAR);
     if (!dir)
@@ -57,13 +68,24 @@
   const char *p = strrchr(dir, '/');
   int needs_slash = (!p || p[1]);
   char *templ = new char[strlen(dir) + needs_slash
-                           + sizeof(TMPFILE_PREFIX) - 1 + 6 + 1];
+                           + sizeof(TMPFILE_PREFIX) - 1 + 6 + 1 + postlen];
   strcpy(templ, dir);
   if (needs_slash)
     strcat(templ, "/");
   strcat(templ, TMPFILE_PREFIX);
+  if (postlen > 0)
+    strcat(templ, postfix);
   strcat(templ, "XXXXXX");
 
+  return( templ );
+}
+
+// Open a temporary file and with fatal error on failure.
+
+FILE *xtmpfile(char **namep=0, char *postfix=0, int do_unlink=1)
+{
+  char *templ = xtmptemplate(postfix);
+
 #ifdef HAVE_MKSTEMP
   errno = 0;
   int fd = mkstemp(templ);
@@ -81,9 +103,13 @@
   if (!fp)
     fatal("cannot open `%1': %2", templ, strerror(errno));
 #endif /* not HAVE_MKSTEMP */
-  if (unlink(templ) < 0)
+  if ((do_unlink) && (unlink(templ) < 0))
     error("cannot unlink `%1': %2", templ, strerror(errno));
-  a_delete templ;
+  if ((namep != 0) && ((*namep) != 0)) {
+    *namep = templ;
+  } else {
+    a_delete templ;
+  }
   return fp;
 }
 
--- groff-cvs/src/include/lib.h Sun Feb  6 09:36:30 2000
+++ groff-html/src/include/lib.h        Fri Feb 11 15:13:41 2000
@@ -53,7 +53,8 @@
 
 #include <stdio.h>
 
-FILE *xtmpfile();
+FILE *xtmpfile(char **namep=0, char *postfix=0, int do_unlink=1);
+char *xtmptemplate(char *extension=0);
 
 #ifndef STDIO_H_DECLARES_POPEN
 
--- groff-cvs/ChangeLog Thu Feb 10 22:22:52 2000
+++ groff-html/ChangeLog        Fri Feb 11 15:51:16 2000
@@ -1,3 +1,14 @@
+2000-02-11  Gaius Mulley  <address@hidden>
+
+       * src/include/lib.h: added xtmptemplate and made xtmpfile
+       parametrically polymorhic.
+
+       * src/libs/libgroff/tmpfile.cc: implemented xtmptemplate
+       and the alterations to xtmpfile.
+       xtmpfile can be requested to return the filename created
+       and asked not to unlink the temp file. The default behviour
+       if parameters are absent is exactly the same as before.
+
 2000-02-09  Werner LEMBERG  <address@hidden>
 
        * src/roff/groff/groff.man: Added an example.