security fixes for partially created files

2009-06-25  Paolo Bonzini  <bonzini@gnu.org>

	* execute.c: Do not copy ACLs until the file is copied.
	* utils.c (ck_mkstemp): Set a restrictive umask on temporary files.

4 files changed, 34 insertions, 15 deletions

diff --git a/ChangeLog b/ChangeLog
index b6e90de..e057102 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,8 @@
+2009-06-25  Paolo Bonzini  <bonzini@gnu.org>
+
+	* execute.c: Do not copy ACLs until the file is copied.
+	* utils.c (ck_mkstemp): Set a restrictive umask on temporary files.
+
 2009-06-26  Paolo Bonzini  <bonzini@gnu.org>
 
 	* autoboot: Do not use GIT_CONFIG_LOCAL.
diff --git a/NEWS b/NEWS
index 2c68c73..937ea44 100644
--- a/NEWS
+++ b/NEWS
@@ -2,6 +2,9 @@ Sed 4.2a
 
 * fix parsing of s/[[[[[[[[[]//
 
+* temporary files for sed -i are not made group/world-readable until
+  they are complete
+
 ----------------------------------------------------------------------------
 Sed 4.2
 
diff --git a/sed/execute.c b/sed/execute.c
index bb9b4ae..4326885 100644
--- a/sed/execute.c
+++ b/sed/execute.c
@@ -133,6 +133,9 @@ struct input {
 
   const char *in_file_name;
 
+  /* Owner and mode to be set just before closing the file.  */
+  struct stat st;
+
   /* if NULL, none of the following are valid */
   FILE *fp;
 
@@ -708,9 +711,8 @@ open_next_file(name, input)
 
   if (in_place_extension)
     {
-      int input_fd, output_fd;
+      int input_fd;
       char *tmpdir, *p;
-      struct stat st;
 
       if (follow_symlinks)
 	input->in_file_name = follow_symlink (name);
@@ -728,8 +730,8 @@ open_next_file(name, input)
         panic(_("couldn't edit %s: is a terminal"), input->in_file_name);
 
       input_fd = fileno (input->fp);
-      fstat (input_fd, &st);
-      if (!S_ISREG (st.st_mode))
+      fstat (input_fd, &input->st);
+      if (!S_ISREG (input->st.st_mode))
         panic(_("couldn't edit %s: not a regular file"), input->in_file_name);
 
       output_file.fp = ck_mkstemp (&input->out_file_name, tmpdir, "sed");
@@ -738,15 +740,6 @@ open_next_file(name, input)
 
       if (!output_file.fp)
         panic(_("couldn't open temporary file %s: %s"), input->out_file_name, strerror(errno));
-
-      output_fd = fileno (output_file.fp);
-#ifdef HAVE_FCHOWN
-      if (fchown (output_fd, st.st_uid, st.st_gid) == -1)
-        fchown (output_fd, -1, st.st_gid);
-#endif
-      copy_acl (input->in_file_name, input_fd,
-		input->out_file_name, output_fd,
-		st.st_mode);
     }
   else
     output_file.fp = stdout;
@@ -766,9 +759,21 @@ closedown(input)
   if (in_place_extension && output_file.fp != NULL)
     {
       const char *target_name;
-      ck_fclose (output_file.fp);
+      int input_fd, output_fd;
 
       target_name = input->in_file_name;
+      input_fd = fileno (input->fp);
+      output_fd = fileno (output_file.fp);
+      copy_acl (input->in_file_name, input_fd,
+		input->out_file_name, output_fd,
+		input->st.st_mode);
+#ifdef HAVE_FCHOWN
+      if (fchown (output_fd, input->st.st_uid, input->st.st_gid) == -1)
+        fchown (output_fd, -1, input->st.st_gid);
+#endif
+
+      ck_fclose (input->fp);
+      ck_fclose (output_file.fp);
       if (strcmp(in_place_extension, "*") != 0)
         {
           char *backup_file_name = get_backup_file_name(target_name);
@@ -779,8 +784,9 @@ closedown(input)
       ck_rename (input->out_file_name, target_name, input->out_file_name);
       free (input->out_file_name);
     }
+  else
+    ck_fclose (input->fp);
 
-  ck_fclose (input->fp);
   input->fp = NULL;
 }
 
diff --git a/sed/utils.c b/sed/utils.c
index bf6c6f9..8c009e4 100644
--- a/sed/utils.c
+++ b/sed/utils.c
@@ -200,6 +200,7 @@ ck_mkstemp (p_filename, tmpdir, base)
   char *template;
   FILE *fp;
   int fd;
+  int save_umask;
 
   if (tmpdir == NULL)
     tmpdir = getenv("TMPDIR");
@@ -217,7 +218,11 @@ ck_mkstemp (p_filename, tmpdir, base)
   template = xmalloc (strlen (tmpdir) + strlen (base) + 8);
   sprintf (template, "%s/%sXXXXXX", tmpdir, base);
 
+   /* The ownership might change, so omit some permissions at first
+      so unauthorized users cannot nip in before the file is ready.  */
+  save_umask = umask (0700);
   fd = mkstemp (template);
+  umask (save_umask);
   if (fd == -1)
     panic(_("couldn't open temporary file %s: %s"), template, strerror(errno));