[SCM] GNU M4 source repository branch, branch-1_4, updated. branch-cvs-readonly-28-gbd9900d

[Eric Blake]

This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "GNU M4 source repository".

http://git.sv.gnu.org/gitweb/?p=m4.git;a=commitdiff;h=bd9900d65eb9cd5add0f107e94b513fa267495ba

The branch, branch-1_4 has been updated
       via  bd9900d65eb9cd5add0f107e94b513fa267495ba (commit)
      from  83e9a157eb78afe47c7955a6fa99e0de79a8b40a (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -----------------------------------------------------------------
commit bd9900d65eb9cd5add0f107e94b513fa267495ba
Author: Eric Blake <address@hidden>
Date:   Fri Dec 7 11:55:18 2007 -0700

    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>

-----------------------------------------------------------------------

Summary of changes:
 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);
     }
 }
 


hooks/post-receive
--
GNU M4 source repository