From b8122b55893c4f55a4a64d65744ee78b67b5b6a5 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Fri, 7 Dec 2007 14:11:21 -0700 Subject: Minor security fix: Quote output of mkstemp. * modules/m4.c (m4_make_temp): Produce quoted output. * doc/m4.texinfo (Mkstemp, Mkdtemp): Update the documentation and tests. Signed-off-by: Eric Blake --- ChangeLog | 5 +++++ doc/m4.texinfo | 68 +++++++++++++++++++++++++++++++++++++++------------------- modules/m4.c | 47 ++++++++++++++++++++++++---------------- 3 files changed, 79 insertions(+), 41 deletions(-) diff --git a/ChangeLog b/ChangeLog index 247eb378..07b471d1 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,10 @@ 2007-12-07 Eric Blake + Minor security fix: Quote output of mkstemp. + * modules/m4.c (m4_make_temp): Produce quoted output. + * doc/m4.texinfo (Mkstemp, Mkdtemp): Update the documentation and + tests. + Stage 5: add notion of quote age. * m4/m4module.h (m4_get_symbol_value_quote_age): New prototype. (m4_set_symbol_value_text): Adjust prototype. diff --git a/doc/m4.texinfo b/doc/m4.texinfo index f298973b..dcadc67d 100644 --- a/doc/m4.texinfo +++ b/doc/m4.texinfo @@ -7071,7 +7071,7 @@ builtin macro, @code{mkstemp}, for making a temporary file: @deffn {Builtin (m4)} mkstemp (@var{template}) @deffnx {Builtin (m4)} maketemp (@var{template}) -Expands to a name of a new, empty file, made from the string +Expands to the quoted name of a new, empty file, made from the string @var{template}, which should end with the string @samp{XXXXXX}. The six @samp{X} characters are then replaced with random characters matching the regular expression @samp{[a-zA-Z0-9._-]}, in order to make the file @@ -7083,7 +7083,8 @@ account, and at most only the current user can read and write the file. The traditional behavior, standardized by @acronym{POSIX}, is that @code{maketemp} merely replaces the trailing @samp{X} with the process -id, without creating a file, and without ensuring that the resulting +id, without creating a file or quoting the expansion, and without +ensuring that the resulting string is a unique file name. In part, this means that using the same @var{template} twice in the same input file will result in the same expansion. This behavior is a security hole, as it is very easy for @@ -7112,6 +7113,8 @@ chosen: @comment ignore @example $ @kbd{m4} +define(`tmp', `oops') +@result{} maketemp(`/tmp/fooXXXXXX') @error{}m4:stdin:1: Warning: maketemp: recommend using mkstemp instead @result{}/tmp/fooa07346 @@ -7143,28 +7146,37 @@ Unless you use the @option{--traditional} command line option (or version of @code{maketemp} is secure. This means that using the same template to multiple calls will generate multiple files. However, we recommend that you use the new @code{mkstemp} macro, introduced in -@acronym{GNU} M4 1.4.8, which is secure even in traditional mode. +@acronym{GNU} M4 1.4.8, which is secure even in traditional mode. Also, +as of M4 1.4.11, the secure implementation quotes the resulting file +name, so that you are guaranteed to know what file was created even if +the random file name happens to match an existing macro. Notice that +this example is careful to use @code{defn} to avoid unintended expansion +of @samp{foo}. @example $ @kbd{m4} -syscmd(`echo foo??????')dnl -@result{}foo?????? -define(`file1', maketemp(`fooXXXXXX'))dnl -@error{}m4:stdin:2: Warning: maketemp: recommend using mkstemp instead -ifelse(esyscmd(`echo foo??????'), `foo??????', `no file', `created') +define(`foo', `errprint(`oops')') +@result{} +syscmd(`rm -f foo-??????')sysval +@result{}0 +define(`file1', maketemp(`foo-XXXXXX'))dnl +@error{}m4:stdin:3: Warning: maketemp: recommend using mkstemp instead +ifelse(esyscmd(`echo \` foo-?????? \''), `foo-??????', + `no file', `created') @result{}created -define(`file2', maketemp(`fooXX'))dnl -@error{}m4:stdin:4: Warning: maketemp: recommend using mkstemp instead -define(`file3', mkstemp(`fooXXXXXX'))dnl -ifelse(len(file1), len(file2), `same length', `different') +define(`file2', maketemp(`foo-XX'))dnl +@error{}m4:stdin:6: Warning: maketemp: recommend using mkstemp instead +define(`file3', mkstemp(`foo-XXXXXX'))dnl +ifelse(len(defn(`file1')), len(defn(`file2')), + `same length', `different') @result{}same length -ifelse(file1, file2, `same', `different file') +ifelse(defn(`file1'), defn(`file2'), `same', `different file') @result{}different file -ifelse(file2, file3, `same', `different file') +ifelse(defn(`file2'), defn(`file3'), `same', `different file') @result{}different file -ifelse(file1, file3, `same', `different file') +ifelse(defn(`file1'), defn(`file3'), `same', `different file') @result{}different file -syscmd(`rm 'file1 file2 file3) +syscmd(`rm 'defn(`file1') defn(`file2') defn(`file3')) @result{} sysval @result{}0 @@ -7173,14 +7185,24 @@ sysval @comment options: -G @example $ @kbd{m4 -G} -define(`file1', maketemp(`fooXXXXXX'))dnl -@error{}m4:stdin:1: Warning: maketemp: recommend using mkstemp instead -define(`file2', maketemp(`fooXXXXXX'))dnl +syscmd(`rm -f foo-*')sysval +@result{}0 +define(`file1', maketemp(`foo-XXXXXX'))dnl @error{}m4:stdin:2: Warning: maketemp: recommend using mkstemp instead +define(`file2', maketemp(`foo-XXXXXX'))dnl +@error{}m4:stdin:3: Warning: maketemp: recommend using mkstemp instead ifelse(file1, file2, `same', `different file') @result{}same -syscmd(`echo foo??????')dnl -@result{}foo?????? +len(maketemp(`foo-XXXXX')) +@error{}m4:stdin:5: Warning: maketemp: recommend using mkstemp instead +@result{}9 +define(`abc', `def') +@result{} +maketemp(`foo-abc') +@result{}foo-def +@error{}m4:stdin:7: Warning: maketemp: recommend using mkstemp instead +syscmd(`test -f foo-*')sysval +@result{}1 @end example @node Mkdtemp @@ -7194,7 +7216,7 @@ temporary directory, for holding multiple temporary files; such a directory can be created with @code{mkdtemp}: @deffn {Builtin (gnu)} mkdtemp (@var{template}) -Expands to a name of a new, empty directory, made from the string +Expands to the quoted name of a new, empty directory, made from the string @var{template}, which should end with the string @samp{XXXXXX}. The six @samp{X} characters are then replaced with random characters matching the regular expression @samp{[a-zA-Z0-9._-]}, in order to make the name @@ -7224,6 +7246,8 @@ chosen: @comment ignore @example $ @kbd{m4} +define(`tmp', `oops') +@result{} mkdtemp(`/tmp/fooXXXXXX') @result{}/tmp/foo2h89Vo mkdtemp(`dir) diff --git a/modules/m4.c b/modules/m4.c index ca5cf45c..8d9bb9ae 100644 --- a/modules/m4.c +++ b/modules/m4.c @@ -418,13 +418,13 @@ static int m4_sysval = 0; # define M4_SYSVAL_TERMSIGBITS(status) \ (WIFSIGNALED (status) ? WTERMSIG (status) << 8 : 0) -#else /* ! UNIX && ! defined WEXITSTATUS */ +#else /* !UNIX && !defined WEXITSTATUS */ /* Platforms such as mingw do not support the notion of reporting which signal terminated a process. Furthermore if WEXITSTATUS was not provided, then the exit value is in the low eight bits. */ # define M4_SYSVAL_EXITBITS(status) status # define M4_SYSVAL_TERMSIGBITS(status) 0 -#endif /* ! UNIX && ! defined WEXITSTATUS */ +#endif /* !UNIX && !defined WEXITSTATUS */ /* Fallback definitions if or are inadequate. */ /* FIXME - this may fit better as a gnulib module. */ @@ -687,15 +687,19 @@ M4BUILTIN_HANDLER (sinclude) /* More miscellaneous builtins -- "maketemp", "errprint". */ -/* Use the first argument as at template for a temporary file name. - FIXME - should we add a mkdtemp builtin in the gnu module, then - export this function as a helper to that? */ +/* Add trailing `X' to PATTERN of length LEN as necessary, then + securely create the temporary file system object. If DIR, create a + directory instead of a file. Report errors on behalf of MACRO. If + successful, output the quoted resulting name on OBS. */ void m4_make_temp (m4 *context, m4_obstack *obs, const char *macro, - const char *name, size_t len, bool dir) + const char *pattern, size_t len, bool dir) { int fd; int i; + char *name; + const char *tmp; + size_t qlen; if (m4_get_safer_opt (context)) { @@ -704,17 +708,23 @@ m4_make_temp (m4 *context, m4_obstack *obs, const char *macro, } /* Guarantee that there are six trailing 'X' characters, even if the - user forgot to supply them. */ + user forgot to supply them. Output must be quoted if + successful. */ assert (obstack_object_size (obs) == 0); - obstack_grow (obs, name, len); + tmp = m4_get_syntax_lquote (M4SYNTAX); + qlen = strlen (tmp); + obstack_grow (obs, tmp, qlen); + obstack_grow (obs, pattern, len); for (i = 0; len > 0 && i < 6; i++) - if (name[--len] != 'X') + if (pattern[len - i - 1] != 'X') break; + len += 6 - i; obstack_grow0 (obs, "XXXXXX", 6 - i); + name = (char *) obstack_base (obs) + qlen; /* Make the temporary object. */ errno = 0; - fd = gen_tempname (obstack_base (obs), dir ? GT_DIR : GT_FILE); + fd = gen_tempname (name, dir ? GT_DIR : GT_FILE); if (fd < 0) { /* This use of _() will need to change if xgettext ever changes @@ -723,18 +733,17 @@ m4_make_temp (m4 *context, m4_obstack *obs, const char *macro, m4_error (context, 0, errno, macro, _(dir ? "cannot create directory from template `%s'" : "cannot create file from template `%s'"), - name); + pattern); obstack_free (obs, obstack_finish (obs)); } else { - if (! dir) + if (!dir) close (fd); - /* Undo the trailing NUL. */ - /* FIXME - shouldn't this return a quoted string, on the rather - small chance that the user has a macro matching the random - file name chosen? */ + /* Remove NUL, then finish quote. */ obstack_blank (obs, -1); + tmp = m4_get_syntax_rquote (M4SYNTAX); + obstack_grow (obs, tmp, strlen (tmp)); } } @@ -1025,7 +1034,7 @@ M4BUILTIN_HANDLER (translit) hence the found map. */ for ( ; (ch = *from) != '\0'; from++) { - if (! found[ch]) + if (!found[ch]) { found[ch] = 1; map[ch] = *to; @@ -1036,7 +1045,7 @@ M4BUILTIN_HANDLER (translit) for (data = M4ARG (1); (ch = *data) != '\0'; data++) { - if (! found[ch]) + if (!found[ch]) obstack_1grow (obs, ch); else if (map[ch]) obstack_1grow (obs, map[ch]); @@ -1071,7 +1080,7 @@ M4BUILTIN_HANDLER (translit) #define numb_gt(x, y) ((x) = ((x) > (y))) #define numb_ge(x, y) ((x) = ((x) >= (y))) -#define numb_lnot(x) ((x) = (! (x))) +#define numb_lnot(x) ((x) = (!(x))) #define numb_lior(x, y) ((x) = ((x) || (y))) #define numb_land(x, y) ((x) = ((x) && (y))) -- cgit v1.2.1