bug#27986: 26.0.50; 'rename-file' can rename files without confirmation

[Paul Eggert <eggert@cs.ucla.edu>]

[-- Attachment #1: Type: text/plain, Size: 3650 bytes --]

>> and this will cause the victim to lose all the data in /home/victim/secret/bar even though the attacker is supposed to lack access to anything under /home/victim/secret.
> 
> You mean, "lose data" because files in /tmp/foo will overwrite their
> namesakes in /home/victim/secret/bar?

Yes. It's not just losing data of course; the attacker can add files to the 
victim directory.

> If my understanding of the situation is correct, then such an attack
> will still be possible with the proposed change, if /tmp/bar exists,
> because Emacs will still issue two separate system calls, and the
> symlink can be created in-between, albeit at a price of deleting
> /tmp/bar first.  Right?

No, such an attack is not possible. Emacs will run rename_noreplace which will 
fail because /tmp/bar exists, then the attacker will remove /tmp/bar and replace 
it with a symlink, then Emacs will run rename ("/tmp/foo", "/tmp/bar") which 
will fail because a directory like /tmp is sticky and the victim cannot remove 
the attacker's symlink. The attack would fail in a different way if /tmp were 
not sticky: the victim would remove the attacker's symlink.

>> As icing on the cake, the current behavior of (rename-file A B) disagrees with its documentation when B is an existing directory.
> 
> Well, 2/3 of it.  The 3rd instance, in the Emacs manual gets it right:

Unfortunately the manual does not get it right. The main part of that 3rd 
instance agrees with the proposed change, because it says the special case 
occurs if NEWNAME "is just a directory name" (on Unix, this means it ends in 
"/"). You are correct that the example is missing a "/", so I'll update the 
proposed patch to fix that.

That part of the documentation has some other confusion: a phrase "The same rule 
applies to all the remaining commands in this section" is clearly an editing 
error; it must be a revenant of when the section didn't talk about commands like 
insert-file. I just now installed the attached patch to fix that (this patch 
doesn't address the directory issue).

The bigger picture here is that this part of Emacs behavior is so poorly 
documented that it's unclear from the documentation what the intent was.

> How about eating the cake and having it, too?  We could refrain from
> testing whether B is a directory if either (1) B ends in a slash, or
> (2) rename_noreplace succeeds.

That doesn't close the security hole, I'm afraid. For example, the attacker can 
create a nonempty directory B, then rename_noreplace will fail, then Emacs will 
determine that B is a directory, then the attacker can replace B with a symlink 
to the victim directory, and then Emacs will overwrite the victim. I imagine 
other attacks are possible, this is just the first one off the top of my head.

> What I don't quite understand is what will happen under your proposal
> to the calls of the form (rename-file A B) where B names an existing
> directory and doesn't end in slash?  Will it fail, sometimes or
> always?

On POSIX systems rename-file will fail if B is a nonempty directory, and will 
succeed if B names an empty directory (this is all assuming B is not itself a 
directory name). Ideally MS-Windows would be compatible; if not, we'd have to 
document the incompatibility.

AFAIU, the 'rename' call will fail if B is a non-empty
> directory, but what if it's empty, and what does rename_noreplace do
> in these situations?  Your documentation patches don't cover this use
> case; I think we should.

Thanks, good point, I plan to update the proposed patch accordingly and to 
follow up soon.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-New-manual-section-Copying-and-Naming.patch --]
[-- Type: text/x-patch; name="0001-New-manual-section-Copying-and-Naming.patch", Size: 8879 bytes --]

From 5c3d0ce3e09bf070bb3c89caa9d88f25d4a39283 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 15 Aug 2017 10:06:44 -0700
Subject: [PATCH] New manual section "Copying and Naming"

* doc/emacs/files.texi (Copying and Naming):
New section, split off from Misc File Ops and containing the
operations that copy, name or rename files.  This fixes some
confusion caused by the incorrect phrase "The same rule applies
to all the remaining commands in this section" in the old manual.
This change does not affect the confusion about directories (see
Bug#27986 for ongoing discussion).
---
 doc/emacs/custom.texi |   2 +-
 doc/emacs/emacs.texi  |   1 +
 doc/emacs/files.texi  | 123 +++++++++++++++++++++++++++-----------------------
 3 files changed, 69 insertions(+), 57 deletions(-)

diff --git a/doc/emacs/custom.texi b/doc/emacs/custom.texi
index 1c9c14a..824fb6e 100644
--- a/doc/emacs/custom.texi
+++ b/doc/emacs/custom.texi
@@ -1710,7 +1710,7 @@ Init Rebinding
 specify the key sequence.  Using a string is simpler, but only works
 for @acronym{ASCII} characters and Meta-modified @acronym{ASCII}
 characters.  For example, here's how to bind @kbd{C-x M-l} to
-@code{make-symbolic-link} (@pxref{Misc File Ops}):
+@code{make-symbolic-link} (@pxref{Copying and Naming}):
 
 @example
 (global-set-key "\C-x\M-l" 'make-symbolic-link)
diff --git a/doc/emacs/emacs.texi b/doc/emacs/emacs.texi
index a3eb422..f3e6c94 100644
--- a/doc/emacs/emacs.texi
+++ b/doc/emacs/emacs.texi
@@ -453,6 +453,7 @@ Top
 * Directories::         Creating, deleting, and listing file directories.
 * Comparing Files::     Finding where two files differ.
 * Diff Mode::           Mode for editing file differences.
+* Copying and Naming::  Copying, naming and renaming files.
 * Misc File Ops::       Other things you can do on files.
 * Compressed Files::    Accessing compressed files.
 * File Archives::       Operating on tar, zip, jar etc. archive files.
diff --git a/doc/emacs/files.texi b/doc/emacs/files.texi
index 0b4e8ed..7bca988 100644
--- a/doc/emacs/files.texi
+++ b/doc/emacs/files.texi
@@ -33,6 +33,7 @@ Files
 * Directories::         Creating, deleting, and listing file directories.
 * Comparing Files::     Finding where two files differ.
 * Diff Mode::           Mode for editing file differences.
+* Copying and Naming::  Copying, naming and renaming files.
 * Misc File Ops::       Other things you can do on files.
 * Compressed Files::    Accessing compressed files.
 * File Archives::       Operating on tar, zip, jar etc. archive files.
@@ -1545,6 +1546,72 @@ Diff Mode
 displayed in the echo area).  With a prefix argument, it tries to
 modify the original source files rather than the patched source files.
 
+@node Copying and Naming
+@section Copying, Naming and Renaming Files
+
+  Emacs has several commands for copying, naming, and renaming files.
+All of them read two file names @var{old} and @var{new} using the
+minibuffer, and then copy or adjust a file's name accordingly; they do
+not accept wildcard file names.
+
+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, @kbd{M-x
+rename-file @key{RET} ~/foo @key{RET}
+@c FIXME: This part of the example should be '/tmp/' not '/tmp',
+@c because '/tmp' is not "just a directory name".
+/tmp
+@c
+@key{RET}} renames @file{~/foo} to @file{/tmp/foo}.  All these
+commands ask for confirmation when the new file name already exists,
+too.
+
+@findex copy-file
+@cindex copying files
+  @kbd{M-x copy-file} copies the contents of the file @var{old} to the
+file @var{new}.
+
+@findex copy-directory
+  @kbd{M-x copy-directory} copies directories, similar to the
+@command{cp -r} shell command.  If @var{new} is an existing directory,
+it creates a copy of the @var{old} directory and puts it in @var{new}.
+If @var{new} is not an existing directory, it copies all the contents
+of @var{old} into a new directory named @var{new}.
+
+@cindex renaming files
+@findex rename-file
+  @kbd{M-x rename-file} renames file @var{old} as @var{new}.  If the
+file name @var{new} already exists, you must confirm with @kbd{yes} or
+renaming is not done; this is because renaming causes the old meaning
+of the name @var{new} to be lost.  If @var{old} and @var{new} are on
+different file systems, the file @var{old} is copied and deleted.
+
+@ifnottex
+  If a file is under version control (@pxref{Version Control}), you
+should rename it using @kbd{M-x vc-rename-file} instead of @kbd{M-x
+rename-file}.  @xref{VC Delete/Rename}.
+@end ifnottex
+
+@findex add-name-to-file
+@cindex hard links (creation)
+  @kbd{M-x add-name-to-file} adds an additional name to an existing
+file without removing the old name.  The new name is created as a hard
+link to the existing file.  The new name must belong on the same file
+system that the file is on.  On MS-Windows, this command works only if
+the file resides in an NTFS file system.  On MS-DOS, it works by
+copying the file.
+
+@findex make-symbolic-link
+@cindex symbolic links (creation)
+  @kbd{M-x make-symbolic-link} creates a symbolic link named
+@var{new}, which points at @var{target}.  The effect is that future
+attempts to open file @var{new} will refer to whatever file is named
+@var{target} at the time the opening is done, or will get an error if
+the name @var{target} is nonexistent at that time.  This command does
+not expand the argument @var{target}, so that it allows you to specify
+a relative name as the target of the link.  On MS-Windows, this
+command works only on MS Windows Vista and later.
+
 @node Misc File Ops
 @section Miscellaneous File Operations
 
@@ -1581,62 +1648,6 @@ Misc File Ops
 delete-file}.  @xref{VC Delete/Rename}.
 @end ifnottex
 
-@findex copy-file
-@cindex copying files
-  @kbd{M-x copy-file} copies the contents of the file @var{old} to the
-file @var{new}.
-
-@findex copy-directory
-  @kbd{M-x copy-directory} copies directories, similar to the
-@command{cp -r} shell command.  It prompts for a directory @var{old}
-and a destination @var{new}.  If @var{new} is an existing directory,
-it creates a copy of the @var{old} directory and puts it in @var{new}.
-If @var{new} is not an existing directory, it copies all the contents
-of @var{old} into a new directory named @var{new}.
-
-@cindex renaming files
-@findex rename-file
-  @kbd{M-x rename-file} reads two file names @var{old} and @var{new}
-using the minibuffer, then renames file @var{old} as @var{new}.  If
-the file name @var{new} already exists, you must confirm with
-@kbd{yes} or renaming is not done; this is because renaming causes the
-old meaning of the name @var{new} to be lost.  If @var{old} and
-@var{new} are on different file systems, the file @var{old} is copied
-and deleted.  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, @kbd{M-x rename-file @key{RET}
-~/foo @key{RET} /tmp @key{RET}} renames @file{~/foo} to
-@file{/tmp/foo}.  The same rule applies to all the remaining commands
-in this section.  All of them ask for confirmation when the new file
-name already exists, too.
-
-@ifnottex
-  If a file is under version control (@pxref{Version Control}), you
-should rename it using @kbd{M-x vc-rename-file} instead of @kbd{M-x
-rename-file}.  @xref{VC Delete/Rename}.
-@end ifnottex
-
-@findex add-name-to-file
-@cindex hard links (creation)
-  @kbd{M-x add-name-to-file} adds an additional name to an existing
-file without removing its old name.  The new name is created as a
-hard link to the existing file.  The new name must belong on the
-same file system that the file is on.  On MS-Windows, this command
-works only if the file resides in an NTFS file system.  On MS-DOS, it
-works by copying the file.
-
-@findex make-symbolic-link
-@cindex symbolic links (creation)
-  @kbd{M-x make-symbolic-link} reads two file names @var{target} and
-@var{linkname}, then creates a symbolic link named @var{linkname},
-which points at @var{target}.  The effect is that future attempts to
-open file @var{linkname} will refer to whatever file is named
-@var{target} at the time the opening is done, or will get an error if
-the name @var{target} is nonexistent at that time.  This command does
-not expand the argument @var{target}, so that it allows you to specify
-a relative name as the target of the link.  On MS-Windows, this
-command works only on MS Windows Vista and later.
-
 @kindex C-x i
 @findex insert-file
   @kbd{M-x insert-file} (also @kbd{C-x i}) inserts a copy of the
-- 
2.7.4