mirror/emacs
1
0
Fork 0
mirror of https://git.savannah.gnu.org/git/emacs.git synced 2024-12-26 10:49:33 +00:00
Code Issues Releases Activity

Fix race with rename-file etc. with dir NEWNAME

Browse Source 
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.
This commit is contained in:
Paul Eggert 2017-09-10 15:39:24 -07:00
parent 52739ffe77
commit 01c885f21f
5 changed files with 58 additions and 41 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
doc/emacs/files.texi
View File

@ -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

8
doc/lispref/files.texi
View File

@ -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

17
etc/NEWS
View File

@ -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

55
src/fileio.c
View File

@ -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)

16
test/lisp/net/tramp-tests.el
View File

@ -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