Fix the fix for CVE-2015-1196v2.7.3

* src/util.c (filename_is_safe): New function split off from name_is_valid().
(symlink_target_is_valid): Explain why we cannot have absolute symlinks or
symlinks with ".." components for now.
(move_file): Move absolute filename check here and explain.
* tests/symlinks: Put test case with ".." symlink in comments for now.
* NEWS: Add CVE number.

5 files changed, 57 insertions, 77 deletions

diff --git a/NEWS b/NEWS
index d3f1c2d..d79cead 100644
--- a/NEWS
+++ b/NEWS
@@ -4,7 +4,7 @@
   deleting".
 * Function names in hunks (from diff -p) are now preserved in reject files.
 * With git-style patches, symlinks that point outside the working directory
-  will no longer be created.
+  will no longer be created (CVE-2015-1196).
 
 Changes in version 2.7.1:
 
diff --git a/src/pch.c b/src/pch.c
index bb39576..028d51f 100644
--- a/src/pch.c
+++ b/src/pch.c
@@ -401,21 +401,7 @@ name_is_valid (char const *name)
 	return false;
     }
 
-  if (IS_ABSOLUTE_FILE_NAME (name))
-    is_valid = false;
-  else
-    for (n = name; *n; )
-      {
-	if (*n == '.' && *++n == '.' && ( ! *++n || ISSLASH (*n)))
-	  {
-	    is_valid = false;
-	    break;
-	  }
-	while (*n && ! ISSLASH (*n))
-	  n++;
-	while (ISSLASH (*n))
-	  n++;
-      }
+  is_valid = filename_is_safe (name);
 
   /* Allow any filename if we are in the filesystem root.  */
   if (! is_valid && cwd_is_root (name))
diff --git a/src/util.c b/src/util.c
index 94c7582..ae05caa 100644
--- a/src/util.c
+++ b/src/util.c
@@ -423,55 +423,18 @@ create_backup (char const *to, const struct stat *to_st, bool leave_original)
     }
 }
 
+/* Only allow symlink targets which are relative and free of ".." components:
+ * otherwise, the operating system may follow one of those symlinks in a
+ * pathname component, leading to a path traversal vulnerability.
+ *
+ * An alternative to disallowing many kinds of symlinks would be to implement
+ * path traversal in user space using openat() without following symlinks
+ * altogether.
+ */
 static bool
 symlink_target_is_valid (char const *target, char const *to)
 {
-  bool is_valid;
-
-  if (IS_ABSOLUTE_FILE_NAME (to))
-    is_valid = true;
-  else if (IS_ABSOLUTE_FILE_NAME (target))
-    is_valid = false;
-  else
-    {
-      unsigned int depth = 0;
-      char const *t;
-
-      is_valid = true;
-      t = to;
-      while (*t)
-	{
-	  while (*t && ! ISSLASH (*t))
-	    t++;
-	  if (ISSLASH (*t))
-	    {
-	      while (ISSLASH (*t))
-		t++;
-	      depth++;
-	    }
-	}
-
-      t = target;
-      while (*t)
-	{
-	  if (*t == '.' && *++t == '.' && (! *++t || ISSLASH (*t)))
-	    {
-	      if (! depth--)
-		{
-		  is_valid = false;
-		  break;
-		}
-	    }
-	  else
-	    {
-	      while (*t && ! ISSLASH (*t))
-		t++;
-	      depth++;
-	    }
-	  while (ISSLASH (*t))
-	    t++;
-	}
-    }
+  bool is_valid = filename_is_safe (target);
 
   /* Allow any symlink target if we are in the filesystem root.  */
   return is_valid || cwd_is_root (to);
@@ -520,7 +483,11 @@ move_file (char const *from, bool *from_needs_removal,
 	    read_fatal ();
 	  buffer[size] = 0;
 
-	  if (! symlink_target_is_valid (buffer, to))
+	  /* If we are allowed to create a file with an absolute path name,
+	     anywhere, we also don't need to worry about symlinks that can
+	     leave the working directory.  */
+	  if (! (IS_ABSOLUTE_FILE_NAME (to)
+		 || symlink_target_is_valid (buffer, to)))
 	    {
 	      fprintf (stderr, "symbolic link target '%s' is invalid\n",
 		       buffer);
@@ -1720,6 +1687,28 @@ int stat_file (char const *filename, struct stat *st)
   return xstat (filename, st) == 0 ? 0 : errno;
 }
 
+/* Check if a filename is relative and free of ".." components.
+   Such a path cannot lead to files outside the working tree
+   as long as the working tree only contains symlinks that are
+   "filename_is_safe" when followed.  */
+bool
+filename_is_safe (char const *name)
+{
+  if (IS_ABSOLUTE_FILE_NAME (name))
+    return false;
+  while (*name)
+    {
+      if (*name == '.' && *++name == '.'
+	  && ( ! *++name || ISSLASH (*name)))
+	return false;
+      while (*name && ! ISSLASH (*name))
+	name++;
+      while (ISSLASH (*name))
+	name++;
+    }
+  return true;
+}
+
 /* Check if we are in the root of a particular filesystem namespace ("/" on
    UNIX or a particular drive's root on DOS-like systems).  */
 bool
diff --git a/src/util.h b/src/util.h
index 579c5de..6b3308a 100644
--- a/src/util.h
+++ b/src/util.h
@@ -69,6 +69,7 @@ enum file_id_type lookup_file_id (struct stat const *);
 void set_queued_output (struct stat const *, bool);
 bool has_queued_output (struct stat const *);
 int stat_file (char const *, struct stat *);
+bool filename_is_safe (char const *);
 bool cwd_is_root (char const *);
 
 enum file_attributes {
diff --git a/tests/symlinks b/tests/symlinks
index 6211026..04a9b73 100644
--- a/tests/symlinks
+++ b/tests/symlinks
@@ -148,20 +148,24 @@ ncheck 'test ! -L symlink'
 
 # Patch should not create symlinks which point outside the working directory.
 
-cat > symlink-target.diff <<EOF
-diff --git a/dir/foo b/dir/foo
-new file mode 120000
-index 0000000..cad2309
---- /dev/null
-+++ b/dir/foo
-@@ -0,0 +1 @@
-+../foo
-\ No newline at end of file
-EOF
-
-check 'patch -p1 < symlink-target.diff || echo "Status: $?"' <<EOF
-patching symbolic link dir/foo
-EOF
+# We cannot even ensure that symlinks with ".." components are safe: we cannot
+# guarantee that they won't end up higher up in the working tree than we think;
+# the path to the symlink may follow symlinks itself.
+#
+#cat > symlink-target.diff <<EOF
+#diff --git a/dir/foo b/dir/foo
+#new file mode 120000
+#index 0000000..cad2309
+#--- /dev/null
+#+++ b/dir/foo
+#@@ -0,0 +1 @@
+#+../foo
+#\ No newline at end of file
+#EOF
+#
+#check 'patch -p1 < symlink-target.diff || echo "Status: $?"' <<EOF
+#patching symbolic link dir/foo
+#EOF
 
 cat > bad-symlink-target1.diff <<EOF
 diff --git a/bar b/bar