security fix for maketemp/mkstemp

[Eric Blake]

I've gone ahead and checked in this patch to the branch, even though the Austin 
group has not responded to my query on this issue yet.  The port to head will 
follow shortly.  In short, when creating a temporary file, it is too risky to 
let anything get in the way of corrupting that file name before the user has a 
chance to use the string, even if the odds are extremely small that the mkstemp 
would ever generate a name which, if left unquoted, would trigger unexpected 
macro expansion.

From: Eric Blake <address@hidden>
Date: Fri, 7 Dec 2007 11:55:18 -0700
Subject: [PATCH] Minor security fix: Quote output of mkstemp.

* src/builtin.c (mkstemp_helper): Produce quoted output.
* doc/m4.texinfo (Mkstemp): Update the documentation and tests.
* NEWS: Document this change.

Signed-off-by: Eric Blake <address@hidden>
---
 ChangeLog      |    5 ++++
 NEWS           |    5 ++++
 doc/m4.texinfo |   57 +++++++++++++++++++++++++++++++++++++++++++------------
 src/builtin.c  |   32 ++++++++++++++++--------------
 4 files changed, 71 insertions(+), 28 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 79a133a..8838ac5 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,10 @@
 2007-12-07  Eric Blake  <address@hidden>
 
+       Minor security fix: Quote output of mkstemp.
+       * src/builtin.c (mkstemp_helper): Produce quoted output.
+       * doc/m4.texinfo (Mkstemp): Update the documentation and tests.
+       * NEWS: Document this change.
+
        Stage 5: add notion of quote age.
        * src/input.c: Comment cleanups.
        (current_quote_age): New global variable.
diff --git a/NEWS b/NEWS
index 1762571..051a16a 100644
--- a/NEWS
+++ b/NEWS
@@ -10,6 +10,11 @@ Version 1.4.11 - ?? ??? 2007, by ????  (git version 1.4.10a-
*)
 * Fix regression introduced in 1.4.9b in the `divert' builtin when more
   than 512 kibibytes are saved in diversions on platforms like NetBSD where
   fopen(name,"a+") seeks to the end of the file.
+* The output of the `maketemp' and `mkstemp' builtins is now quoted if a
+  file was created.  This is a minor security fix, because it was possible
+  (although rather unlikely) that an unquoted string could match an
+  existing macro name, such that use of the `mkstemp' output would trigger
+  inadvertent macro expansion and operate on the wrong file name.
 * Enhance the `defn' builtin to support concatenation of multiple text
   arguments, as required by POSIX.  However, at this time, it is not
   possible to concatenate a builtin macro with anything else; a warning is
diff --git a/doc/m4.texinfo b/doc/m4.texinfo
index 803dbf0..93b7696 100644
--- a/doc/m4.texinfo
+++ b/doc/m4.texinfo
@@ -5876,7 +5876,7 @@ builtin macro, @code{mkstemp}, for making a temporary 
file:
 
 @deffn Builtin mkstemp (@var{template})
 @deffnx Builtin 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
@@ -5888,7 +5888,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
@@ -5912,6 +5913,8 @@ chosen:
 @comment ignore
 @example
 $ @kbd{m4}
+define(`tmp', `oops')
address@hidden
 maketemp(`/tmp/fooXXXXXX')
 @result{}/tmp/fooa07346
 ifdef(`mkstemp', `define(`maketemp', defn(`mkstemp'))',
@@ -5929,26 +5932,35 @@ 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
address@hidden M4 1.4.8, which is secure even in traditional mode.
address@hidden 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(`rm -f foo??????')sysval
+define(`foo', `errprint(`oops')')
address@hidden
+syscmd(`rm -f foo-??????')sysval
 @result{}0
-define(`file1', maketemp(`fooXXXXXX'))dnl
-ifelse(esyscmd(`echo foo??????'), `foo??????', `no file', `created')
+define(`file1', maketemp(`foo-XXXXXX'))dnl
+ifelse(esyscmd(`echo \` foo-?????? \''), ` foo-?????? ',
+       `no file', `created')
 @result{}created
-define(`file2', maketemp(`fooXX'))dnl
-define(`file3', mkstemp(`fooXXXXXX'))dnl
-ifelse(len(file1), len(file2), `same length', `different')
+define(`file2', maketemp(`foo-XX'))dnl
+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
@@ -5966,6 +5978,25 @@ len(mkstemp(`fooXXXXX'))
 syscmd(`rm foo??????')sysval
 @result{}0
 @end example
+
address@hidden Likewise, and ensure that traditional mode leaves the result 
unquoted
address@hidden without creating a file.
+
address@hidden options: -G
address@hidden
+syscmd(`rm -f foo-*')sysval
address@hidden
+len(maketemp(`foo-XXXXX'))
address@hidden:stdin:2: Warning: maketemp: recommend using mkstemp instead
address@hidden
+define(`abc', `def')
address@hidden
+maketemp(`foo-abc')
address@hidden
address@hidden:stdin:4: Warning: maketemp: recommend using mkstemp instead
+syscmd(`test -f foo-*')sysval
address@hidden
address@hidden example
 @end ignore
 
 @node Miscellaneous
diff --git a/src/builtin.c b/src/builtin.c
index b4053f1..87f8c2f 100644
--- a/src/builtin.c
+++ b/src/builtin.c
@@ -1433,40 +1433,42 @@ m4_sinclude (struct obstack *obs, int argc, 
macro_arguments *argv)
 | Use the first argument as a template for a temporary file name.  |
 `-----------------------------------------------------------------*/
 
-/* Add trailing 'X' to NAME of length LEN as necessary, then securely
-   create the file, and place the new file name on OBS.  Report errors
-   on behalf of ME.  */
+/* Add trailing 'X' to PATTERN of length LEN as necessary, then
+   securely create the file, and place the quoted new file name on
+   OBS.  Report errors on behalf of ME.  */
 static void
-mkstemp_helper (struct obstack *obs, const char *me, const char *name,
+mkstemp_helper (struct obstack *obs, const char *me, const char *pattern,
                size_t len)
 {
   int fd;
   int i;
+  char *name;
 
   /* Guarantee that there are six trailing 'X' characters, even if the
-     user forgot to supply them.  */
-  obstack_grow (obs, name, len);
+     user forgot to supply them.  Output must be quoted if
+     successful.  */
+  obstack_grow (obs, lquote.string, lquote.length);
+  obstack_grow (obs, pattern, len);
   for (i = 0; len > 0 && i < 6; i++)
-    if (name[--len] != 'X')
+    if (pattern[len - i - 1] != 'X')
       break;
-  for (; i < 6; i++)
-    obstack_1grow (obs, 'X');
-  obstack_1grow (obs, '\0');
+  len += 6 - i;
+  obstack_grow0 (obs, "XXXXXX", 6 - i);
+  name = (char *) obstack_base (obs) + lquote.length;
 
   errno = 0;
-  fd = mkstemp ((char *) obstack_base (obs));
+  fd = mkstemp (name);
   if (fd < 0)
     {
-      m4_warn (errno, me, _("cannot create tempfile `%s'"), name);
+      m4_warn (errno, me, _("cannot create tempfile `%s'"), pattern);
       obstack_free (obs, obstack_finish (obs));
     }
   else
     {
       close (fd);
-      /* Undo trailing NUL.  */
-      /* FIXME - should we be quoting this name, on the tiny chance
-        that the random name generated matches a user's macro?  */
+      /* Remove NUL, then finish quote.  */
       obstack_blank (obs, -1);
+      obstack_grow (obs, rquote.string, rquote.length);
     }
 }
 
-- 
1.5.3.5