[Emacs-diffs] master 01c885f: Fix race with rename-file etc. with dir NEWNAME

[Paul Eggert]

branch: master
commit 01c885f21f343045783eb9ad1ff5f9b83d6cd789
Author: Paul Eggert <address@hidden>
Commit: Paul Eggert <address@hidden>

    Fix race with rename-file etc. with dir NEWNAME
    
    This changes the behavior of rename-file etc. slightly.
    The old behavior mostly disagreed with the documentation, and had
    a race condition bug that could allow attackers to modify victims'
    write-protected directories (Bug#27986).
    * doc/lispref/files.texi (Changing Files): Document that in
    rename-file etc., NEWFILE is special if it is a directory name.
    * etc/NEWS: Document the change in behavior.
    * src/fileio.c (directory_like): Remove.  All uses removed.
    (expand_cp_target): Test only whether NEWNAME is a directory name,
    not whether it is currently a directory.  This avoids a race.
    (Fcopy_file, Frename_file, Fadd_name_to_file, Fmake_symbolic_link):
    Document behavior if NEWNAME is a directory name.
    (Frename_file): Simplify now that the destdir behavior occurs
    only when NEWNAME is a directory name.
    * test/lisp/net/tramp-tests.el (tramp-test11-copy-file)
    (tramp-test12-rename-file, tramp--test-check-files):
    Adjust tests to match new behavior.
---
 doc/emacs/files.texi         |  3 ++-
 doc/lispref/files.texi       |  8 +++----
 etc/NEWS                     | 17 ++++++++++++++
 src/fileio.c                 | 55 ++++++++++++++++++++++----------------------
 test/lisp/net/tramp-tests.el | 16 ++++++-------
 5 files changed, 58 insertions(+), 41 deletions(-)

diff --git a/doc/emacs/files.texi b/doc/emacs/files.texi
index b9bfbd7..0cf46b6 100644
--- a/doc/emacs/files.texi
+++ b/doc/emacs/files.texi
@@ -1558,7 +1558,8 @@ In all these commands, if the argument @var{new} is just 
a directory
 name, the real new name is in that directory, with the same
 non-directory component as @var{old}.  For example, the command
 @address@hidden rename-file @key{RET} ~/foo @key{RET} /tmp/ @key{RET}}}
-renames @file{~/foo} to @file{/tmp/foo}.  @xref{Directory Names,,,
+renames @file{~/foo} to @file{/tmp/foo}.  On GNU and other POSIX-like
+systems, directory names end in @samp{/}.  @xref{Directory Names,,,
 elisp, the Emacs Lisp Reference Manual}.
 
 All these commands ask for confirmation when the new file name already
diff --git a/doc/lispref/files.texi b/doc/lispref/files.texi
index edee30e..eacaf04 100644
--- a/doc/lispref/files.texi
+++ b/doc/lispref/files.texi
@@ -1563,15 +1563,15 @@ a @code{file-missing} error instead.
 made by these functions instead of writing them immediately to
 secondary storage.  @xref{Files and Storage}.
 
address@hidden FIXME: This paragraph is purposely silent on what happens if
address@hidden @var{newname} is not a directory name but happens to name a
address@hidden directory.  See Bug#27986 for discussion on how to clear this up.
   In the functions that have an argument @var{newname}, if this
 argument is a directory name it is treated as if the nondirectory part
 of the source name were appended.  Typically, a directory name is one
 that ends in @samp{/} (@pxref{Directory Names}).  For example, if the
 old name is @file{a/b/c}, the @var{newname} @file{d/e/f/} is treated
-as if it were @file{d/e/f/c}.
+as if it were @file{d/e/f/c}.  This special treatment does not apply
+if @var{newname} is not a directory name but names a file that is a
+directory; for example, the @var{newname} @file{d/e/f} is left as-is
+even if @file{d/e/f} happens to be a directory.
 
   In the functions that have an argument @var{newname}, if a file by the
 name of @var{newname} already exists, the actions taken depend on the
diff --git a/etc/NEWS b/etc/NEWS
index 3f7feba..4187dd8 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1271,6 +1271,23 @@ handlers now.
 gtk_window_move for moving frames and ignores the value of the
 variable 'x-gtk-use-window-move'.  The variable is now obsolete.
 
++++
+** Several functions that create or rename files now treat their
+destination argument specially only when it is a directory name, i.e.,
+when it ends in '/' on GNU and other POSIX-like systems.  When the
+destination argument D of one of these functions is an existing
+directory and the intent is to act on an entry in that directory, D
+should now be a directory name.  For example, (rename-file "e" "f/")
+renames to 'f/e'.  Although this formerly happened sometimes even when
+D was not a directory name, as in (rename-file "e" "f") where 'f'
+happened to be a directory, the old behavior often contradicted the
+documentation and had inherent races that led to security holes.  A
+call like (rename-file C D) that used the old, undocumented behavior
+can be written as (rename-file C (file-name-as-directory D)), a
+formulation portable to both older and newer versions of Emacs.
+Affected functions include add-name-to-file, copy-file,
+make-symbolic-link, and rename-file.
+
 
 * Lisp Changes in Emacs 26.1
 
diff --git a/src/fileio.c b/src/fileio.c
index a1cea94..3195348 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -595,24 +595,16 @@ DEFUN ("directory-name-p", Fdirectory_name_p, 
Sdirectory_name_p, 1, 1, 0,
   return IS_DIRECTORY_SEP (c) ? Qt : Qnil;
 }
 
-/* Return true if NAME must be that of a directory if it exists.
-   When NAME is a directory name, this avoids system calls compared to
-   just calling Ffile_directory_p.  */
-
-static bool
-directory_like (Lisp_Object name)
-{
-  return !NILP (Fdirectory_name_p (name)) || !NILP (Ffile_directory_p (name));
-}
-
-/* Return the expansion of NEWNAME, except that if NEWNAME is like a
-   directory then return the expansion of FILE's basename under
-   NEWNAME.  This is like how 'cp FILE NEWNAME' works.  */
+/* Return the expansion of NEWNAME, except that if NEWNAME is a
+   directory name then return the expansion of FILE's basename under
+   NEWNAME.  This resembles how 'cp FILE NEWNAME' works, except that
+   it requires NEWNAME to be a directory name (typically, by ending in
+   "/").  */
 
 static Lisp_Object
 expand_cp_target (Lisp_Object file, Lisp_Object newname)
 {
-  return (directory_like (newname)
+  return (!NILP (Fdirectory_name_p (newname))
          ? Fexpand_file_name (Ffile_name_nondirectory (file), newname)
          : Fexpand_file_name (newname, Qnil));
 }
@@ -1833,7 +1825,8 @@ clone_file (int dest, int source)
 DEFUN ("copy-file", Fcopy_file, Scopy_file, 2, 6,
        "fCopy file: \nGCopy %s to file: \np\nP",
        doc: /* Copy FILE to NEWNAME.  Both args must be strings.
-If NEWNAME names a directory, copy FILE there.
+If NEWNAME is a directory name, copy FILE to a like-named file under
+NEWNAME.
 
 This function always sets the file modes of the output file to match
 the input file.
@@ -2257,6 +2250,9 @@ DEFUN ("rename-file", Frename_file, Srename_file, 2, 3,
        "fRename file: \nGRename %s to file: \np",
        doc: /* Rename FILE as NEWNAME.  Both args must be strings.
 If file has names other than FILE, it continues to have those names.
+If NEWNAME is a directory name, rename FILE to a like-named file under
+NEWNAME.
+
 Signal a `file-already-exists' error if a file NEWNAME already exists
 unless optional third argument OK-IF-ALREADY-EXISTS is non-nil.
 An integer third arg means request confirmation if NEWNAME already exists.
@@ -2265,7 +2261,6 @@ This is what happens in interactive use with M-x.  */)
 {
   Lisp_Object handler;
   Lisp_Object encoded_file, encoded_newname, symlink_target;
-  int dirp = -1;
 
   file = Fexpand_file_name (file, Qnil);
 
@@ -2339,22 +2334,21 @@ This is what happens in interactive use with M-x.  */)
   if (rename_errno != EXDEV)
     report_file_errno ("Renaming", list2 (file, newname), rename_errno);
 
-  symlink_target = Ffile_symlink_p (file);
-  if (!NILP (symlink_target))
-    Fmake_symbolic_link (symlink_target, newname, ok_if_already_exists);
+  bool dirp = !NILP (Fdirectory_name_p (file));
+  if (dirp)
+    call4 (Qcopy_directory, file, newname, Qt, Qnil);
   else
     {
-      if (dirp < 0)
-       dirp = directory_like (file);
-      if (dirp)
-       call4 (Qcopy_directory, file, newname, Qt, Qnil);
+      symlink_target = Ffile_symlink_p (file);
+      if (!NILP (symlink_target))
+       Fmake_symbolic_link (symlink_target, newname, ok_if_already_exists);
       else
        Fcopy_file (file, newname, ok_if_already_exists, Qt, Qt, Qt);
     }
 
   ptrdiff_t count = SPECPDL_INDEX ();
   specbind (Qdelete_by_moving_to_trash, Qnil);
-  if (dirp && NILP (symlink_target))
+  if (dirp)
     call2 (Qdelete_directory, file, Qt);
   else
     Fdelete_file (file, Qnil);
@@ -2364,6 +2358,9 @@ This is what happens in interactive use with M-x.  */)
 DEFUN ("add-name-to-file", Fadd_name_to_file, Sadd_name_to_file, 2, 3,
        "fAdd name to file: \nGName to add to %s: \np",
        doc: /* Give FILE additional name NEWNAME.  Both args must be strings.
+If NEWNAME is a directory name, give FILE a like-named new name under
+NEWNAME.
+
 Signal a `file-already-exists' error if a file NEWNAME already exists
 unless optional third argument OK-IF-ALREADY-EXISTS is non-nil.
 An integer third arg means request confirmation if NEWNAME already exists.
@@ -2412,11 +2409,13 @@ This is what happens in interactive use with M-x.  */)
 
 DEFUN ("make-symbolic-link", Fmake_symbolic_link, Smake_symbolic_link, 2, 3,
        "FMake symbolic link to file: \nGMake symbolic link to file %s: \np",
-       doc: /* Make a symbolic link to TARGET, named LINKNAME.
-Both args must be strings.
-Signal a `file-already-exists' error if a file LINKNAME already exists
+       doc: /* Make a symbolic link to TARGET, named NEWNAME.
+If NEWNAME is a directory name, make a like-named symbolic link under
+NEWNAME.
+
+Signal a `file-already-exists' error if a file NEWNAME already exists
 unless optional third argument OK-IF-ALREADY-EXISTS is non-nil.
-An integer third arg means request confirmation if LINKNAME already
+An integer third arg means request confirmation if NEWNAME already
 exists, and expand leading "~" or strip leading "/:" in TARGET.
 This happens for interactive use with M-x.  */)
   (Lisp_Object target, Lisp_Object linkname, Lisp_Object ok_if_already_exists)
diff --git a/test/lisp/net/tramp-tests.el b/test/lisp/net/tramp-tests.el
index c61e5dc..735211c 100644
--- a/test/lisp/net/tramp-tests.el
+++ b/test/lisp/net/tramp-tests.el
@@ -1903,7 +1903,7 @@ This checks also `file-name-as-directory', 
`file-name-directory',
            (should-error (copy-file tmp-name1 tmp-name2))
            (copy-file tmp-name1 tmp-name2 'ok)
            (make-directory tmp-name3)
-           (copy-file tmp-name1 tmp-name3)
+           (copy-file tmp-name1 (file-name-as-directory tmp-name3))
            (should
             (file-exists-p
              (expand-file-name (file-name-nondirectory tmp-name1) tmp-name3))))
@@ -1925,7 +1925,7 @@ This checks also `file-name-as-directory', 
`file-name-directory',
            (should-error (copy-file tmp-name1 tmp-name4))
            (copy-file tmp-name1 tmp-name4 'ok)
            (make-directory tmp-name5)
-           (copy-file tmp-name1 tmp-name5)
+           (copy-file tmp-name1 (file-name-as-directory tmp-name5))
            (should
             (file-exists-p
              (expand-file-name (file-name-nondirectory tmp-name1) tmp-name5))))
@@ -1947,7 +1947,7 @@ This checks also `file-name-as-directory', 
`file-name-directory',
            (should-error (copy-file tmp-name4 tmp-name1))
            (copy-file tmp-name4 tmp-name1 'ok)
            (make-directory tmp-name3)
-           (copy-file tmp-name4 tmp-name3)
+           (copy-file tmp-name4 (file-name-as-directory tmp-name3))
            (should
             (file-exists-p
              (expand-file-name (file-name-nondirectory tmp-name4) tmp-name3))))
@@ -1986,7 +1986,7 @@ This checks also `file-name-as-directory', 
`file-name-directory',
            (should-not (file-exists-p tmp-name1))
            (write-region "foo" nil tmp-name1)
            (make-directory tmp-name3)
-           (rename-file tmp-name1 tmp-name3)
+           (rename-file tmp-name1 (file-name-as-directory tmp-name3))
            (should-not (file-exists-p tmp-name1))
            (should
             (file-exists-p
@@ -2013,7 +2013,7 @@ This checks also `file-name-as-directory', 
`file-name-directory',
            (should-not (file-exists-p tmp-name1))
            (write-region "foo" nil tmp-name1)
            (make-directory tmp-name5)
-           (rename-file tmp-name1 tmp-name5)
+           (rename-file tmp-name1 (file-name-as-directory tmp-name5))
            (should-not (file-exists-p tmp-name1))
            (should
             (file-exists-p
@@ -2040,7 +2040,7 @@ This checks also `file-name-as-directory', 
`file-name-directory',
            (should-not (file-exists-p tmp-name4))
            (write-region "foo" nil tmp-name4 nil 'nomessage)
            (make-directory tmp-name3)
-           (rename-file tmp-name4 tmp-name3)
+           (rename-file tmp-name4 (file-name-as-directory tmp-name3))
            (should-not (file-exists-p tmp-name4))
            (should
             (file-exists-p
@@ -3681,11 +3681,11 @@ This requires restrictions of file name syntax."
                  (should (string-equal (buffer-string) elt)))
 
                ;; Copy file both directions.
-               (copy-file file1 tmp-name2)
+               (copy-file file1 (file-name-as-directory tmp-name2))
                (should (file-exists-p file2))
                (delete-file file1)
                (should-not (file-exists-p file1))
-               (copy-file file2 tmp-name1)
+               (copy-file file2 (file-name-as-directory tmp-name1))
                (should (file-exists-p file1))
 
                (tramp--test-ignore-make-symbolic-link-error