groff
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Why does groff require psutils?


From: G. Branden Robinson
Subject: Re: Why does groff require psutils?
Date: Mon, 27 Nov 2023 11:42:03 -0600

At 2023-11-26T15:34:10+0100, Ingo Schwarze wrote:
> not related to the "psutils" questions, but this almost made my
> eyes fall out.

Evidently...

> Alexis wrote on Sun, Nov 26, 2023 at 12:28:25PM +0100:
> 
> > Would replacing the following in src/preproc/html/pre-html.cpp
> >   s = make_string("psselect -q -p%d %s %s\n",
> >        pageno, psFileName, psPageName);
> 
> WHOA.
> 
> What kind of crappy code is that?

The kind that does _not_ appear to put these character pointers under
user control.

> It's really "C Programming 101" that you must *never* do anything
> like that.  Obviously, execve(2) or a similar library function
> that does not suffer from shell argument splitting and shell
> metacharacter issues must be used here.  If we want to continue
> shipping preproc/html, i think this definitely needs to be fixed.

Hang on now.  Before you can declare the above unsafe...and admittedly
we would do well to restore the very next line of context--

  html_system(s, 1);

where `html_system` wraps exactly the standard C library function you
think it does--

we need to consider whether any mayhem-causing characters can get into
psFileName or psPageName in the first place.

 132 # if defined(DEBUGGING) && !defined(DEBUG_FILE_DIR)
 133 /* For a DEBUGGING version, on the Unix host, we can also usually rely
 134    on being able to use '/tmp' for temporary file storage.  (Note that,
 135    as in the __MSDOS__ or _WIN32 case above, the user may override this
 136    by defining
 137
 138      -DDEBUG_FILE_DIR=/path/to/debug/files
 139
 140    in the CPPFLAGS.) */
 141
 142 #  define DEBUG_FILE_DIR /tmp
 143 # endif
 144
 145 #endif /* not __MSDOS__ or _WIN32 */
 146
 147 #ifdef DEBUGGING
 148 // For a DEBUGGING version, we need some additional macros,
 149 // to direct the captured debugging mode output to appropriately named
 150 // files in the specified DEBUG_FILE_DIR.
 151
 152 # define DEBUG_TEXT(text) #text
 153 # define DEBUG_NAME(text) DEBUG_TEXT(text)
 154 # define DEBUG_FILE(name) DEBUG_NAME(DEBUG_FILE_DIR) "/" name
 155 #endif

1703 static void makeTempFiles(void)
1704 {
1705 #if defined(DEBUGGING)
1706   psFileName = DEBUG_FILE("prehtml-ps");

1709   psPageName = DEBUG_FILE("prehtml-psn");

1712 #else /* not DEBUGGING */

1715   // psPageName contains a single page of PostScript.
1716   f = xtmpfile(&psPageName, PS_TEMPLATE_LONG, PS_TEMPLATE_SHORT, true);
1717   if (0 /* nullptr */ == f)
1718     sys_fatal("xtmpfile");
1719   fclose(f);

1728   // psFileName contains a PostScript file of the complete document.
1729   f = xtmpfile(&psFileName, PS_TEMPLATE_LONG, PS_TEMPLATE_SHORT, true);
1730   if (0 /* nullptr */ == f)
1731     sys_fatal("xtmpfile");
1732   fclose(f);

...and that's it.  These character pointers point either to a string
literal embedded in the text section of the executable,[1] or are
returned by `xtmpfile`, which wraps the standard C library's `mkstemp()`
as you might expect.

> I mean, for all i know, there are people running "groff -T html"
> on public web servers to serve manual pages to the general public
> via public CGI interfaces...

That's indeed possible, but you may need to tell this sleepy kid in the
back row of the C Programming 101 lecture hall how there's an obvious
hole for injection of spaces or shell operators in the foregoing.

I've expressed elsewhere my idea for what's necessary to eliminate the
`pre-grohtml` preprocessor altogether, and that is the avenue I would
like to pursue, rather than refactoring to foreclose the possibility of
a security hole that may arise if someone else hacks on the program to
_make_ it vulnerable.

As it stands, this looks to me like a code smell rather than even a
theoretical vulnerability.

I'm open to correction on this point.

Regards,
Branden

[1]  Well, probably.  That's traditionally what would happen.  As this
     is debugging code that is disabled by default, I didn't bother to
     check.  Caveat lector.

[2] src/libs/libgroff/tmpfile.cpp:

// Open a temporary file and with fatal error on failure.

FILE *xtmpfile(char **namep,
               const char *postfix_long, const char *postfix_short,
               int do_unlink)
{
  char *templ = xtmptemplate(postfix_long, postfix_short);
  errno = 0;
  int fd = mkstemp(templ);
  if (fd < 0)
    fatal("cannot create temporary file: %1", strerror(errno));
  errno = 0;
  FILE *fp = fdopen(fd, FOPEN_RWB); // many callers of xtmpfile use binary I/O
  if (!fp)
    fatal("fdopen: %1", strerror(errno));
  if (do_unlink)
    add_tmp_file(templ);
  if (namep)
    *namep = templ;
  else
    delete[] templ;
  return fp;
}


Attachment: signature.asc
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]