From: Paul Eggert <eggert@cs.ucla.edu>
To: Eli Zaretskii <eliz@gnu.org>
Cc: Philipp <p.stephani2@gmail.com>, 27986@debbugs.gnu.org
Subject: bug#27986: 26.0.50; 'rename-file' can rename files without confirmation
Date: Sun, 13 Aug 2017 15:42:05 -0700 [thread overview]
Message-ID: <1002ee73-0ab5-409b-831f-0c283c322264@cs.ucla.edu> (raw)
In-Reply-To: <61980dde-3d68-7200-e7f4-98f62e410060@cs.ucla.edu>
[-- Attachment #1: Type: text/plain, Size: 926 bytes --]
Paul Eggert wrote:
> there are races on GNU/Linux which can lead to potential security problems.
> Perhaps we can't fix these races on MS-Windows but we should be able to fix them
> on a GNUish host. However, we will need to change the semantics of rename-file
> etc. slightly, since no single system call supports the cp-like target rewriting
> of these functions. I have a fix in mind to do that in a hopefully
> compatible-enough way, which I'll try to propose soon. I'll keep
> case-insensitive file systems in mind when I do that.
Attached is a proposed patch to fix this security problem. If I understand
things correctly, the fix should work on MS-Windows and on case-insensitive file
systems. Since this patch entails an incompatible change to the (undocumented)
behavior of (rename-file A B) when B is a directory but is not a directory name,
I'll mention the proposed change on emacs-devel.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-race-with-rename-file-etc.-with-dir-NEWNAME.patch --]
[-- Type: text/x-patch; name="0001-Fix-race-with-rename-file-etc.-with-dir-NEWNAME.patch", Size: 11765 bytes --]
From 967d1c009f6743aa75e6829129ea9445ebc06f30 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
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.
+
\f
* 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
next prev parent reply other threads:[~2017-08-13 22:42 UTC|newest]
Thread overview: 55+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-06 15:40 bug#27986: 26.0.50; `rename-file' can rename files without confirmation Philipp
2017-08-06 17:05 ` Eli Zaretskii
2017-08-14 17:09 ` Philipp Stephani
2017-08-14 17:22 ` Eli Zaretskii
2017-08-11 8:15 ` bug#27986: 26.0.50; 'rename-file' " Paul Eggert
2017-08-13 22:42 ` Paul Eggert [this message]
2017-08-14 15:40 ` Eli Zaretskii
2017-08-14 23:31 ` Paul Eggert
2017-08-15 16:04 ` Eli Zaretskii
2017-08-15 17:24 ` Paul Eggert
2017-08-15 17:42 ` Eli Zaretskii
2017-08-15 19:27 ` Paul Eggert
2017-08-16 2:36 ` Eli Zaretskii
2017-08-16 5:06 ` Paul Eggert
2017-08-16 14:21 ` Eli Zaretskii
2017-08-16 15:15 ` Paul Eggert
2017-08-16 16:06 ` Eli Zaretskii
2017-08-16 17:19 ` Paul Eggert
2017-08-16 17:30 ` Eli Zaretskii
2017-08-16 18:06 ` Glenn Morris
2017-08-16 22:31 ` Stefan Monnier
2017-08-16 23:56 ` Paul Eggert
2017-08-17 0:04 ` Stefan Monnier
2017-08-19 6:54 ` Eli Zaretskii
2017-09-10 22:49 ` Paul Eggert
2017-09-11 6:07 ` Paul Eggert
2017-09-11 14:47 ` Eli Zaretskii
2017-09-11 16:45 ` Paul Eggert
2017-09-11 17:09 ` Eli Zaretskii
2017-09-11 17:25 ` Paul Eggert
2017-09-12 9:25 ` Michael Albinus
2017-08-13 23:48 ` Paul Eggert
2017-08-14 13:44 ` Ken Brown
2017-08-14 15:21 ` Eli Zaretskii
2017-08-14 15:34 ` Eli Zaretskii
2017-08-14 16:33 ` Eli Zaretskii
2017-08-14 16:58 ` Philipp Stephani
2017-08-14 17:04 ` Eli Zaretskii
2017-08-14 16:50 ` Philipp Stephani
2017-08-14 23:03 ` Paul Eggert
2017-08-15 1:19 ` Paul Eggert
2017-08-15 2:35 ` Eli Zaretskii
2017-08-15 7:00 ` Paul Eggert
2017-08-15 16:08 ` Eli Zaretskii
2017-08-16 19:33 ` Ken Brown
2017-08-19 21:30 ` Ken Brown
2017-08-19 21:37 ` Paul Eggert
2017-08-19 22:04 ` Ken Brown
2017-08-19 22:38 ` Paul Eggert
2017-08-15 12:45 ` Andy Moreton
2017-08-15 16:18 ` Eli Zaretskii
2017-08-19 21:33 ` bug#27986: 26.0.50; 'rename-file' can rename files without Richard Stallman
2017-08-20 2:37 ` Eli Zaretskii
2017-08-25 20:33 ` John Wiegley
2017-08-26 7:30 ` Eli Zaretskii
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1002ee73-0ab5-409b-831f-0c283c322264@cs.ucla.edu \
--to=eggert@cs.ucla.edu \
--cc=27986@debbugs.gnu.org \
--cc=eliz@gnu.org \
--cc=p.stephani2@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.