From 967d1c009f6743aa75e6829129ea9445ebc06f30 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Sun, 13 Aug 2017 15:20:56 -0700 Subject: [PROPOSED] 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. * 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/lispref/files.texi | 10 +++++--- etc/NEWS | 13 +++++++++++ src/fileio.c | 55 ++++++++++++++++++++++---------------------- test/lisp/net/tramp-tests.el | 16 ++++++------- 4 files changed, 55 insertions(+), 39 deletions(-) diff --git a/doc/lispref/files.texi b/doc/lispref/files.texi index 25f32c231c..8956c4f1b7 100644 --- a/doc/lispref/files.texi +++ b/doc/lispref/files.texi @@ -1553,9 +1553,13 @@ Changing Files made by these functions instead of writing them immediately to secondary storage. @xref{Files and Storage}. - 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 -value of the argument @var{ok-if-already-exists}: + 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. For example, if the old name is +@code{"a/b/c"}, the @var{newname} @code{"d/e/f/"} is treated as if it +were @code{"d/e/f/c"}. Furthermore, if a file already exists with the +new name, the actions taken depend on the value of the argument +@var{ok-if-already-exists}: @itemize @bullet @item diff --git a/etc/NEWS b/etc/NEWS index 3f38153048..947114148b 100644 --- a/etc/NEWS +++ b/etc/NEWS @@ -1204,6 +1204,19 @@ break. ** The arguments LOCKNAME and MUSTBENEW of 'write-region' are propagated to file name handlers now. ++++ +** When the NEWNAME argument of 'copy-file', 'rename-file', +'add-name-to-file', or 'make-symbolic-link' is an existing directory +and the intent is to act on an entry in that directory, NEWNAME should +now be a directory name, e.g., (rename-file "e" "f/") renames to 'f/e'. +Although this formerly happened sometimes even when NEWNAME was not a +directory name, as in (rename-file "e" "f") where 'f' happened to be a +directory, the old behavior had inherent races that led to security +holes, and disagreed with longstanding documentation. A call like +(rename-file A B) that used the old, undocumented behavior can be +written as (rename-file A (file-name-as-directory B)), a formulation +portable to both older and newer versions of Emacs. + * Lisp Changes in Emacs 26.1 diff --git a/src/fileio.c b/src/fileio.c index 69079c6ae4..4bfa915951 100644 --- a/src/fileio.c +++ b/src/fileio.c @@ -594,24 +594,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)); } @@ -1818,7 +1810,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. @@ -2242,6 +2235,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. @@ -2250,7 +2246,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); @@ -2319,22 +2314,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); @@ -2344,6 +2338,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. @@ -2392,11 +2389,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 exists. +An integer third arg means request confirmation if NEWNAME already exists. 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 45cf95fcfe..6aca0a278e 100644 --- a/test/lisp/net/tramp-tests.el +++ b/test/lisp/net/tramp-tests.el @@ -1892,7 +1892,7 @@ tramp--test-backtrace (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)))) @@ -1914,7 +1914,7 @@ tramp--test-backtrace (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)))) @@ -1936,7 +1936,7 @@ tramp--test-backtrace (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)))) @@ -1975,7 +1975,7 @@ tramp--test-backtrace (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 @@ -2002,7 +2002,7 @@ tramp--test-backtrace (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 @@ -2029,7 +2029,7 @@ tramp--test-backtrace (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 @@ -3464,11 +3464,11 @@ tramp--test-check-files (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)) ;; Method "smb" supports `make-symbolic-link' only if the -- 2.13.4