Use symlink-safe system call replacements

Use the symlink-safe replacements for system calls in many places throughout
the code: In some places this makes patch safe against path traversal attacks;
in other places, it saves the kernel from having to re-traverse the pathnames.
* src/inp.c (plan_b): Use safe_open() + fdopen() instead of fopen().
* src/util.c (copy_attr): Document why we are safe here.
(create_backup): Use safe_open() instead of creat().

4 files changed, 57 insertions, 39 deletions

diff --git a/src/inp.c b/src/inp.c
index a7ad070..4969837 100644
--- a/src/inp.c
+++ b/src/inp.c
@@ -25,6 +25,7 @@
 #undef XTERN
 #define XTERN
 #include <inp.h>
+#include <safe.h>
 
 /* Input-file-with-indexable-lines abstract type */
 
@@ -237,7 +238,7 @@ plan_a (char const *filename)
     {
       if (S_ISREG (instat.st_mode))
         {
-	  int ifd = open (filename, O_RDONLY|binary_transput);
+	  int ifd = safe_open (filename, O_RDONLY|binary_transput, 0);
 	  size_t buffered = 0, n;
 	  if (ifd < 0)
 	    pfatal ("can't open file %s", quotearg (filename));
@@ -268,7 +269,7 @@ plan_a (char const *filename)
       else if (S_ISLNK (instat.st_mode))
 	{
 	  ssize_t n;
-	  n = readlink (filename, buffer, size);
+	  n = safe_readlink (filename, buffer, size);
 	  if (n < 0)
 	    pfatal ("can't read %s %s", "symbolic link", quotearg (filename));
 	  size = n;
@@ -339,6 +340,7 @@ plan_a (char const *filename)
 static void
 plan_b (char const *filename)
 {
+  int ifd;
   FILE *ifp;
   int c;
   size_t len;
@@ -351,7 +353,8 @@ plan_b (char const *filename)
 
   if (instat.st_size == 0)
     filename = NULL_DEVICE;
-  if (! (ifp = fopen (filename, binary_transput ? "rb" : "r")))
+  if ((ifd = safe_open (filename, O_RDONLY | binary_transput, 0)) < 0
+      || ! (ifp = fdopen (ifd, binary_transput ? "rb" : "r")))
     pfatal ("Can't open file %s", quotearg (filename));
   if (TMPINNAME_needs_removal)
     {
diff --git a/src/patch.c b/src/patch.c
index 7dafb54..64f4391 100644
--- a/src/patch.c
+++ b/src/patch.c
@@ -34,6 +34,7 @@
 #include <gl_linked_list.h>
 #include <gl_xlist.h>
 #include <minmax.h>
+#include <safe.h>
 
 /* procedures */
 
@@ -291,7 +292,7 @@ main (int argc, char **argv)
 
       if (read_only_behavior != RO_IGNORE
 	  && ! inerrno && ! S_ISLNK (instat.st_mode)
-	  && access (inname, W_OK) != 0)
+	  && safe_access (inname, W_OK) != 0)
 	{
 	  say ("File %s is read-only; ", quotearg (inname));
 	  if (read_only_behavior == RO_WARN)
@@ -1919,7 +1920,7 @@ output_files (struct stat const *st)
 		       from_st, file_to_output->to,
 		       file_to_output->mode, file_to_output->backup);
       if (file_to_output->to && from_needs_removal)
-	unlink (file_to_output->from);
+	safe_unlink (file_to_output->from);
 
       if (st && st->st_dev == from_st->st_dev && st->st_ino == from_st->st_ino)
 	{
@@ -1949,7 +1950,7 @@ forget_output_files (void)
     {
       const struct file_to_output *file_to_output = elt;
 
-      unlink (file_to_output->from);
+      safe_unlink (file_to_output->from);
     }
   gl_list_iterator_free (&iter);
   gl_list_clear (files_to_output);
@@ -1973,7 +1974,7 @@ remove_if_needed (char const *name, bool *needs_removal)
 {
   if (*needs_removal)
     {
-      unlink (name);
+      safe_unlink (name);
       *needs_removal = false;
     }
 }
diff --git a/src/pch.c b/src/pch.c
index 6640fc3..a831697 100644
--- a/src/pch.c
+++ b/src/pch.c
@@ -32,6 +32,7 @@
 #if HAVE_SETMODE_DOS
 # include <io.h>
 #endif
+#include <safe.h>
 
 #define INITHUNKMAX 125			/* initial dynamic allocation size */
 
@@ -1012,7 +1013,7 @@ prefix_components (char *filename, bool checkdirs)
 	  if (checkdirs)
 	    {
 	      *f = '\0';
-	      stat_result = stat (filename, &stat_buf);
+	      stat_result = safe_stat (filename, &stat_buf);
 	      *f = '/';
 	      if (! (stat_result == 0 && S_ISDIR (stat_buf.st_mode)))
 		break;
diff --git a/src/util.c b/src/util.c
index ae05caa..eba3943 100644
--- a/src/util.c
+++ b/src/util.c
@@ -52,6 +52,8 @@
 # include "verror.h"
 #endif
 
+#include <safe.h>
+
 static void makedirs (char const *);
 
 typedef struct
@@ -216,6 +218,9 @@ copy_attr (char const *src_path, char const *dst_path)
     .quote = copy_attr_quote,
     .quote_free = copy_attr_free
   };
+  /* FIXME: We are copying between files we know we can safely access by
+   * pathname. A safe_ version of attr_copy_file() might still be slightly
+   * more efficient for deep paths. */
   return attr_copy_file (src_path, dst_path, copy_attr_check, &ctx);
 }
 
@@ -244,7 +249,7 @@ set_file_attributes (char const *to, enum file_attributes attr,
 	  times[0] = get_stat_atime (st);
 	  times[1] = get_stat_mtime (st);
 	}
-      if (lutimens (to, times) != 0)
+      if (safe_lutimens (to, times) != 0)
 	pfatal ("Failed to set the timestamps of %s %s",
 		S_ISLNK (mode) ? "symbolic link" : "file",
 		quotearg (to));
@@ -267,10 +272,10 @@ set_file_attributes (char const *to, enum file_attributes attr,
       /* May fail if we are not privileged to set the file owner, or we are
          not in group instat.st_gid.  Ignore those errors.  */
       if ((uid != -1 || gid != -1)
-	  && lchown (to, uid, gid) != 0
+	  && safe_lchown (to, uid, gid) != 0
 	  && (errno != EPERM
 	      || (uid != -1
-		  && lchown (to, (uid = -1), gid) != 0
+		  && safe_lchown (to, (uid = -1), gid) != 0
 		  && errno != EPERM)))
 	pfatal ("Failed to set the %s of %s %s",
 		(uid == -1) ? "owner" : "owning group",
@@ -289,7 +294,7 @@ set_file_attributes (char const *to, enum file_attributes attr,
 	 systems where we could.  */
       if (lchmod (to, mode))
 #else
-      if (! S_ISLNK (mode) && chmod (to, mode) != 0)
+      if (! S_ISLNK (mode) && safe_chmod (to, mode) != 0)
 #endif
 	pfatal ("Failed to set the permissions of %s %s",
 		S_ISLNK (mode) ? "symbolic link" : "file",
@@ -382,8 +387,8 @@ create_backup (char const *to, const struct stat *to_st, bool leave_original)
 	    say ("Creating empty file %s\n", quotearg (bakname));
 
 	  try_makedirs_errno = ENOENT;
-	  unlink (bakname);
-	  while ((fd = creat (bakname, 0666)) < 0)
+	  safe_unlink (bakname);
+	  while ((fd = safe_open (bakname, O_CREAT | O_WRONLY | O_TRUNC, 0666)) < 0)
 	    {
 	      if (errno != try_makedirs_errno)
 		pfatal ("Can't create file %s", quotearg (bakname));
@@ -400,7 +405,7 @@ create_backup (char const *to, const struct stat *to_st, bool leave_original)
 	  if (debug & 4)
 	    say ("Renaming file %s to %s\n",
 		 quotearg_n (0, to), quotearg_n (1, bakname));
-	  while (rename (to, bakname) != 0)
+	  while (safe_rename (to, bakname) != 0)
 	    {
 	      if (errno == try_makedirs_errno)
 		{
@@ -411,7 +416,7 @@ create_backup (char const *to, const struct stat *to_st, bool leave_original)
 		{
 		  create_backup_copy (to, bakname, to_st,
 				      try_makedirs_errno == 0);
-		  unlink (to);
+		  safe_unlink (to);
 		  break;
 		}
 	      else
@@ -475,7 +480,7 @@ move_file (char const *from, bool *from_needs_removal,
 	  char *buffer = xmalloc (PATH_MAX);
 	  int fd, size = 0, i;
 
-	  if ((fd = open (from, O_RDONLY | O_BINARY)) < 0)
+	  if ((fd = safe_open (from, O_RDONLY | O_BINARY, 0)) < 0)
 	    pfatal ("Can't reopen file %s", quotearg (from));
 	  while ((i = read (fd, buffer + size, PATH_MAX - size)) > 0)
 	    size += i;
@@ -496,18 +501,18 @@ move_file (char const *from, bool *from_needs_removal,
 
 	  if (! backup)
 	    {
-	      if (unlink (to) == 0)
+	      if (safe_unlink (to) == 0)
 		to_dir_known_to_exist = true;
 	    }
-	  if (symlink (buffer, to) != 0)
+	  if (safe_symlink (buffer, to) != 0)
 	    {
 	      if (errno == ENOENT && ! to_dir_known_to_exist)
 		makedirs (to);
-	      if (symlink (buffer, to) != 0)
+	      if (safe_symlink (buffer, to) != 0)
 		pfatal ("Can't create %s %s", "symbolic link", to);
 	    }
 	  free (buffer);
-	  if (lstat (to, &to_st) != 0)
+	  if (safe_lstat (to, &to_st) != 0)
 	    pfatal ("Can't get file attributes of %s %s", "symbolic link", to);
 	  insert_file_id (&to_st, CREATED);
 	}
@@ -517,7 +522,7 @@ move_file (char const *from, bool *from_needs_removal,
 	    say ("Renaming file %s to %s\n",
 		 quotearg_n (0, from), quotearg_n (1, to));
 
-	  if (rename (from, to) != 0)
+	  if (safe_rename (from, to) != 0)
 	    {
 	      bool to_dir_known_to_exist = false;
 
@@ -526,7 +531,7 @@ move_file (char const *from, bool *from_needs_removal,
 		{
 		  makedirs (to);
 		  to_dir_known_to_exist = true;
-		  if (rename (from, to) == 0)
+		  if (safe_rename (from, to) == 0)
 		    goto rename_succeeded;
 		}
 
@@ -535,7 +540,7 @@ move_file (char const *from, bool *from_needs_removal,
 		  struct stat tost;
 		  if (! backup)
 		    {
-		      if (unlink (to) == 0)
+		      if (safe_unlink (to) == 0)
 			to_dir_known_to_exist = true;
 		      else if (errno != ENOENT)
 			pfatal ("Can't remove file %s", quotearg (to));
@@ -564,7 +569,7 @@ move_file (char const *from, bool *from_needs_removal,
     {
       if (debug & 4)
 	say ("Removing file %s\n", quotearg (to));
-      if (unlink (to) != 0 && errno != ENOENT)
+      if (safe_unlink (to) != 0 && errno != ENOENT)
 	pfatal ("Can't remove file %s", quotearg (to));
     }
 }
@@ -583,8 +588,8 @@ create_file (char const *file, int open_flags, mode_t mode,
   do
     {
       if (! (O_CREAT && O_TRUNC))
-	close (creat (file, mode));
-      fd = open (file, O_CREAT | O_TRUNC | open_flags, mode);
+	close (safe_open (file, O_CREAT | O_WRONLY | O_TRUNC, mode));
+      fd = safe_open (file, O_CREAT | O_TRUNC | open_flags, mode);
       if (fd < 0)
 	{
 	  char *f;
@@ -605,7 +610,7 @@ copy_to_fd (const char *from, int tofd)
   int fromfd;
   ssize_t i;
 
-  if ((fromfd = open (from, O_RDONLY | O_BINARY)) < 0)
+  if ((fromfd = safe_open (from, O_RDONLY | O_BINARY, 0)) < 0)
     pfatal ("Can't reopen file %s", quotearg (from));
   while ((i = read (fromfd, buf, bufsize)) != 0)
     {
@@ -635,11 +640,11 @@ copy_file (char const *from, char const *to, struct stat *tost,
     {
       char *buffer = xmalloc (PATH_MAX);
 
-      if (readlink (from, buffer, PATH_MAX) < 0)
+      if (safe_readlink (from, buffer, PATH_MAX) < 0)
 	pfatal ("Can't read %s %s", "symbolic link", from);
-      if (symlink (buffer, to) != 0)
+      if (safe_symlink (buffer, to) != 0)
 	pfatal ("Can't create %s %s", "symbolic link", to);
-      if (tost && lstat (to, tost) != 0)
+      if (tost && safe_lstat (to, tost) != 0)
 	pfatal ("Can't get file attributes of %s %s", "symbolic link", to);
       free (buffer);
     }
@@ -663,7 +668,7 @@ append_to_file (char const *from, char const *to)
 {
   int tofd;
 
-  if ((tofd = open (to, O_WRONLY | O_BINARY | O_APPEND)) < 0)
+  if ((tofd = safe_open (to, O_WRONLY | O_BINARY | O_APPEND, 0)) < 0)
     pfatal ("Can't reopen file %s", quotearg (to));
   copy_to_fd (from, tofd);
   if (close (tofd) != 0)
@@ -730,8 +735,8 @@ version_controller (char const *filename, bool readonly,
 
   sprintf (trybuf, "%s/", dir);
 
-#define try1(f,a1)    (sprintf (trybuf + dirlen, f, a1),    stat (trybuf, &cstat) == 0)
-#define try2(f,a1,a2) (sprintf (trybuf + dirlen, f, a1,a2), stat (trybuf, &cstat) == 0)
+#define try1(f,a1)    (sprintf (trybuf + dirlen, f, a1),    safe_stat (trybuf, &cstat) == 0)
+#define try2(f,a1,a2) (sprintf (trybuf + dirlen, f, a1,a2), safe_stat (trybuf, &cstat) == 0)
 
   /* Check that RCS file is not working file.
      Some hosts don't report file name length errors.  */
@@ -862,7 +867,7 @@ version_get (char const *filename, char const *cs, bool exists, bool readonly,
 	     cs, readonly ? "" : " with lock");
       if (systemic (getbuf) != 0)
 	fatal ("Can't get file %s from %s", quotearg (filename), cs);
-      if (stat (filename, filestat) != 0)
+      if (safe_stat (filename, filestat) != 0)
 	pfatal ("%s", quotearg (filename));
     }
 
@@ -1301,6 +1306,9 @@ makedirs (char const *name)
   char *f;
   char *flim = replace_slashes (filename);
 
+  /* FIXME: Now with the pathname lookup cache, there is no reason for
+     deferring the creation of directories. Callers should be updated. */
+
   if (flim)
     {
       /* Create any missing directories, replacing NULs by '/'s.
@@ -1311,7 +1319,7 @@ makedirs (char const *name)
       for (f = filename;  f <= flim;  f++)
 	if (!*f)
 	  {
-	    mkdir (filename,
+	    safe_mkdir (filename,
 		   S_IRUSR|S_IWUSR|S_IXUSR
 		   |S_IRGRP|S_IWGRP|S_IXGRP
 		   |S_IROTH|S_IWOTH|S_IXOTH);
@@ -1341,7 +1349,7 @@ removedirs (char const *name)
 			      || ISSLASH (filename[i - 3])))))))
       {
 	filename[i] = '\0';
-	if (rmdir (filename) == 0 && verbosity == VERBOSE)
+	if (safe_rmdir (filename) == 0 && verbosity == VERBOSE)
 	  say ("Removed empty directory %s\n", quotearg (filename));
 	filename[i] = '/';
       }
@@ -1656,10 +1664,15 @@ make_tempfile (char const **name, char letter, char const *real_name,
     {
       int fd;
 
+      /* gen_tempname(..., GT_NOCREATE) calls lstat() to check if a file
+	 already exists.  In the worst case, this leads to a template that
+	 follows a symbolic link and that we cannot use; safe_open() will
+	 detect that. */
+
       if (gen_tempname (template, 0, flags, GT_NOCREATE))
         pfatal ("Can't create temporary file %s", template);
     retry:
-      fd = open (template, O_CREAT | O_EXCL | flags, mode);
+      fd = safe_open (template, O_CREAT | O_EXCL | flags, mode);
       if (fd == -1)
         {
 	  if (errno == try_makedirs_errno)
@@ -1682,7 +1695,7 @@ make_tempfile (char const **name, char letter, char const *real_name,
 int stat_file (char const *filename, struct stat *st)
 {
   int (*xstat)(char const *, struct stat *) =
-    follow_symlinks ? stat : lstat;
+    follow_symlinks ? safe_stat : safe_lstat;
 
   return xstat (filename, st) == 0 ? 0 : errno;
 }