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 <ebb9@byu.net>

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  <ebb9@byu.net>
 
+	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 <stdlib.h> or <sys/wait.h> 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)))