From 01c885f21f343045783eb9ad1ff5f9b83d6cd789 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Sun, 10 Sep 2017 15:39:24 -0700 Subject: [PATCH] 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 b9bfbd72ce1..0cf46b6df1c 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 @w{@kbd{M-x 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 edee30e5ad7..eacaf046370 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}. -@c FIXME: This paragraph is purposely silent on what happens if -@c @var{newname} is not a directory name but happens to name a -@c 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 3f7feba6dda..4187dd8a30a 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 a1cea94c0b6..3195348a8ce 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 c61e5dc9eb3..735211c3da6 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 -- 2.39.2