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

[Eli Zaretskii]

> Cc: p.stephani2@gmail.com, 27986@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Mon, 14 Aug 2017 16:31:38 -0700
> 
> Currently (rename-file A B) requires at least two system calls to work: one 
> to test whether B is a directory, and the other to actually do the rename. 
> This leads to race conditions if other actors change the file system between 
> the two calls.
> 
> For example, suppose a victim is about to execute (rename-file "/tmp/foo" 
> "/tmp/bar" t), and suppose an attacker wants to destroy the victim's file 
> /home/victim/secret/foo. The attacker can do (make-symbolic-link 
> "/home/victim/secret" "/tmp/bar")

You mean, the attacker creates this symlink between the time Emacs
tests whether /tmp/bar is a directory and the time Emacs calls
'rename'?

> 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?  IOW, files in
/home/victim/secret/bar which don't have identically-named
counterparts in /tmp/foo will not be lost, right?  (I'm not saying
this is not a problem, I'm just trying to understand the meaning of
"lose data" in this case.)

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?

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

  [...]  If the argument NEW is just a directory name, the real new name
  is in that directory, with the same non-directory component as OLD.
  For example, ‘M-x rename-file <RET> ~/foo <RET> /tmp <RET>’ renames
  ‘~/foo’ to ‘/tmp/foo’.

Note that there's no trailing slash in "/tmp".

> There is no good solution to this problem. All solutions are bad, in that 
> either they are not 100% backward compatible with existing behavior, or they 
> continue to encourage insecure Elisp code. The proposed patch attempts to 
> choose the least bad way forward, by making the default behavior more secure, 
> at a relatively minor cost in compatibility. Most uses of rename-file etc. 
> won't care about the change, and the ones that do care are likely to have 
> security problems anyway.

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.  If B doesn't end in a slash and
rename_noreplace fails, and the value of errno from rename_noreplace
doesn't indicate B is a directory, then test if it's a directory and
fall back on the old behavior.  We could then change our code that
calls rename-file to make B end in a slash, thus encouraging secure
code and leaving the insecure behavior only as backward-compatibility
feature.  This will make the insecure code limited to situation which
are already insecure due to a separate call to rename_noreplace.

> The proposed solution improves security, because a common pattern in Lisp 
> code when creating a file BAR "atomically" is to create and write a temporary 
> file FOO and then execute (rename-file FOO BAR). Currently, this approach can 
> be attacked in the way described when BAR's parent directory is /tmp or any 
> similar directory. With the proposed patch, this approach cannot be hijacked 
> in this way, because BAR will be a file name and not a directory name. That 
> is, the call to rename-file will specify whether the destination-directory 
> semantics are desired, rather than relying on the state of the filesystem to 
> specify it. This is more secure because the state of the filesystem is 
> partially under control of attackers.

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?  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.