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;
}
signature.asc
Description: PGP signature
