From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Paul Eggert Newsgroups: gmane.emacs.bugs Subject: bug#27986: 26.0.50; 'rename-file' can rename files without confirmation Date: Sun, 13 Aug 2017 15:42:05 -0700 Organization: UCLA Computer Science Department Message-ID: <1002ee73-0ab5-409b-831f-0c283c322264@cs.ucla.edu> References: <61980dde-3d68-7200-e7f4-98f62e410060@cs.ucla.edu> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------004A3577E8D9E540D7659E5C" X-Trace: blaine.gmane.org 1502664195 30323 195.159.176.226 (13 Aug 2017 22:43:15 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Sun, 13 Aug 2017 22:43:15 +0000 (UTC) User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 Cc: Philipp , 27986@debbugs.gnu.org To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Mon Aug 14 00:43:09 2017 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dh1bA-0007QW-Ac for geb-bug-gnu-emacs@m.gmane.org; Mon, 14 Aug 2017 00:43:08 +0200 Original-Received: from localhost ([::1]:43174 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dh1bF-0001MJ-0P for geb-bug-gnu-emacs@m.gmane.org; Sun, 13 Aug 2017 18:43:13 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:46710) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dh1b7-0001Jd-5k for bug-gnu-emacs@gnu.org; Sun, 13 Aug 2017 18:43:07 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dh1b4-00061L-0J for bug-gnu-emacs@gnu.org; Sun, 13 Aug 2017 18:43:05 -0400 Original-Received: from debbugs.gnu.org ([208.118.235.43]:52755) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1dh1b3-000618-Rt for bug-gnu-emacs@gnu.org; Sun, 13 Aug 2017 18:43:01 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1dh1b3-0001Mw-LQ for bug-gnu-emacs@gnu.org; Sun, 13 Aug 2017 18:43:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Paul Eggert Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sun, 13 Aug 2017 22:43:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 27986 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: Original-Received: via spool by 27986-submit@debbugs.gnu.org id=B27986.15026641365241 (code B ref 27986); Sun, 13 Aug 2017 22:43:01 +0000 Original-Received: (at 27986) by debbugs.gnu.org; 13 Aug 2017 22:42:16 +0000 Original-Received: from localhost ([127.0.0.1]:33201 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dh1aJ-0001MS-Gw for submit@debbugs.gnu.org; Sun, 13 Aug 2017 18:42:16 -0400 Original-Received: from zimbra.cs.ucla.edu ([131.179.128.68]:37198) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dh1aH-0001MM-45 for 27986@debbugs.gnu.org; Sun, 13 Aug 2017 18:42:13 -0400 Original-Received: from localhost (localhost [127.0.0.1]) by zimbra.cs.ucla.edu (Postfix) with ESMTP id 8769C16084B; Sun, 13 Aug 2017 15:42:07 -0700 (PDT) Original-Received: from zimbra.cs.ucla.edu ([127.0.0.1]) by localhost (zimbra.cs.ucla.edu [127.0.0.1]) (amavisd-new, port 10032) with ESMTP id Xz3DGkAfC-HO; Sun, 13 Aug 2017 15:42:06 -0700 (PDT) Original-Received: from localhost (localhost [127.0.0.1]) by zimbra.cs.ucla.edu (Postfix) with ESMTP id 14D2C160759; Sun, 13 Aug 2017 15:42:06 -0700 (PDT) X-Virus-Scanned: amavisd-new at zimbra.cs.ucla.edu Original-Received: from zimbra.cs.ucla.edu ([127.0.0.1]) by localhost (zimbra.cs.ucla.edu [127.0.0.1]) (amavisd-new, port 10026) with ESMTP id mTcamVdTsvuH; Sun, 13 Aug 2017 15:42:05 -0700 (PDT) Original-Received: from [192.168.1.9] (unknown [47.153.184.153]) by zimbra.cs.ucla.edu (Postfix) with ESMTPSA id DA72F160734; Sun, 13 Aug 2017 15:42:05 -0700 (PDT) In-Reply-To: <61980dde-3d68-7200-e7f4-98f62e410060@cs.ucla.edu> Content-Language: en-US X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 208.118.235.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.org gmane.emacs.bugs:135735 Archived-At: This is a multi-part message in MIME format. --------------004A3577E8D9E540D7659E5C Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable Paul Eggert wrote: > there are races on GNU/Linux which can lead to potential security probl= ems.=20 > Perhaps we can't fix these races on MS-Windows but we should be able to= fix them=20 > on a GNUish host. However, we will need to change the semantics of rena= me-file=20 > etc. slightly, since no single system call supports the cp-like target = rewriting=20 > of these functions. I have a fix in mind to do that in a hopefully=20 > compatible-enough way, which I'll try to propose soon. I'll keep=20 > case-insensitive file systems in mind when I do that. Attached is a proposed patch to fix this security problem. If I understan= d=20 things correctly, the fix should work on MS-Windows and on case-insensiti= ve file=20 systems. Since this patch entails an incompatible change to the (undocume= nted)=20 behavior of (rename-file A B) when B is a directory but is not a director= y name,=20 I'll mention the proposed change on emacs-devel. --------------004A3577E8D9E540D7659E5C Content-Type: text/x-patch; name="0001-Fix-race-with-rename-file-etc.-with-dir-NEWNAME.patch" Content-Transfer-Encoding: quoted-printable Content-Disposition: attachment; filename*0="0001-Fix-race-with-rename-file-etc.-with-dir-NEWNAME.patch" =46rom 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}. =20 - 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}: =20 @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. =20 ++++ +** 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. + =0C * Lisp Changes in Emacs 26.1 =20 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, Sdire= ctory_name_p, 1, 1, 0, return IS_DIRECTORY_SEP (c) ? Qt : Qnil; } =20 -/* 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 (n= ame)); -} - -/* 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 + "/"). */ =20 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. =20 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 exist= s. @@ -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 =3D -1; =20 file =3D Fexpand_file_name (file, Qnil); =20 @@ -2319,22 +2314,21 @@ This is what happens in interactive use with M-x.= */) if (rename_errno !=3D EXDEV) report_file_errno ("Renaming", list2 (file, newname), rename_errno);= =20 - symlink_target =3D Ffile_symlink_p (file); - if (!NILP (symlink_target)) - Fmake_symbolic_link (symlink_target, newname, ok_if_already_exists);= + bool dirp =3D !NILP (Fdirectory_name_p (file)); + if (dirp) + call4 (Qcopy_directory, file, newname, Qt, Qnil); else { - if (dirp < 0) - dirp =3D directory_like (file); - if (dirp) - call4 (Qcopy_directory, file, newname, Qt, Qnil); + symlink_target =3D 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); } =20 ptrdiff_t count =3D 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 str= ings. +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 exist= s. @@ -2392,11 +2389,13 @@ This is what happens in interactive use with M-x.= */) =20 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 exis= ts. +An integer third arg means request confirmation if NEWNAME already exist= s. This happens for interactive use with M-x. */) (Lisp_Object target, Lisp_Object linkname, Lisp_Object ok_if_already_e= xists) { 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))) =20 ;; 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)) =20 ;; Method "smb" supports `make-symbolic-link' only if the --=20 2.13.4 --------------004A3577E8D9E540D7659E5C--