Subject: bug#13125: closed (Re: bug#13125: Fix permissions bugs with setgid directories etc.)

From: [email-masked] (GNU bug Tracking System)
Date: Fri, 14 Dec 2012 19:02:02 +0000

Your bug report

#13125: Fix permissions bugs with setgid directories etc.

which was filed against the emacs package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
13125: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=13125
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#13125: Fix permissions bugs with setgid directories etc.
From: Paul Eggert <[email-masked]>
To: [email-masked]
Date: Fri, 14 Dec 2012 11:00:09 -0800
List-Id: <debbugs-submit.debbugs.gnu.org>

No further comment, so I installed the patch as trunk bzr 111233
and am marking the bug as done.

Subject: Fix permissions bugs with setgid directories etc.
From: Paul Eggert <[email-masked]>
To: [email-masked]
Date: Sat, 08 Dec 2012 17:13:50 -0800
List-Id: <debbugs-submit.debbugs.gnu.org>

Tags: patch security

Emacs sometimes mishandles the permissions of files: even if
backup-by-copying-when-mismatch is set, Emacs sometimes replaces a
rewritten file with a file that has the wrong user or group.

Here's some background.

In several places Emacs assumes that on 4.2BSD hosts, a newly created
file is given a group ID equal to its parent directory, and that on
non-4.2BSD hosts the new files are given Emacs's group ID.  Although
this was true long ago, it hasn't been true for many years.  Most
commonly, the old 4.2BSD behavior is now selected by the setgid bit on
directories.  But on some hosts, the behavior is selected as a mount
flag, or (as in 4.2BSD) it's a property of the operating system.  On
network file systems the behavior is sometimes selected by the file
server, sometimes by the client.

To add to the mess, on FreeBSD systems, the setuid bit of directories
can control whether there's a similar inheritance of file ownership.
Luckily this problem is a bit simpler, in that it's not a property
of the OS or a mount flag, as far as I know.

I'm attaching a patch, which changes file-attributes so that it now
outputs a placeholder value instead of the old 9th attribute member,
since the value is rarely needed and almost nobody seems to be using
it or caring that it's wrong.  Instead, the patch moves this
functionality to file-ownership-preserved-p via a new argument GROUP.
The patch also adds new functions group-gid and group-real-gid for use
with the backup-file heuristic.

This patch is relative to trunk bzr 111160.
=== modified file 'ChangeLog'
--- ChangeLog	2012-12-08 17:19:51 +0000
+++ ChangeLog	2012-12-09 00:50:02 +0000
@@ -1,3 +1,8 @@
+2012-12-09  Paul Eggert  <[email-masked]>
+
+	Fix permissions bugs with setgid directories etc.
+	* configure.ac (BSD4_2): Remove; no longer needed.
+
 2012-12-08  Paul Eggert  <[email-masked]>
 
 	Use putenv+unsetenv instead of modifying environ directly (Bug#13070).

=== modified file 'admin/CPP-DEFINES'
--- admin/CPP-DEFINES	2012-12-08 06:56:26 +0000
+++ admin/CPP-DEFINES	2012-12-09 00:50:02 +0000
@@ -9,7 +9,6 @@
 
 AIX
 _AIX
-BSD4_2
 BSD_SYSTEM
 CYGWIN		Compiling the Cygwin port.
 __CYGWIN__	Ditto

=== modified file 'admin/ChangeLog'
--- admin/ChangeLog	2012-12-08 17:19:51 +0000
+++ admin/ChangeLog	2012-12-09 00:50:02 +0000
@@ -1,3 +1,8 @@
+2012-12-09  Paul Eggert  <[email-masked]>
+
+	Fix permissions bugs with setgid directories etc.
+	* CPP-DEFINES (BSD4_2): Remove.
+
 2012-12-08  Paul Eggert  <[email-masked]>
 
 	Use putenv+unsetenv instead of modifying environ directly (Bug#13070).

=== modified file 'configure.ac'
--- configure.ac	2012-12-08 06:56:26 +0000
+++ configure.ac	2012-12-09 00:50:02 +0000
@@ -3814,7 +3814,6 @@
 
 dnl Define symbols to identify the version of Unix this is.
 dnl Define all the symbols that apply correctly.
-AH_TEMPLATE(BSD4_2, [Define if the system is compatible with BSD 4.2.])
 AH_TEMPLATE(BSD_SYSTEM, [Define if the system is compatible with BSD 4.2.])
 AH_TEMPLATE(DOS_NT, [Define if the system is MS DOS or MS Windows.])
 AH_TEMPLATE(MSDOS, [Define if the system is MS DOS.])
@@ -3840,7 +3839,6 @@
 
   darwin)
     dnl BSD4_3 and BSD4_4 are already defined in sys/param.h.
-    AC_DEFINE(BSD4_2, [])
     AC_DEFINE(BSD_SYSTEM, [])
     dnl More specific than the above two.  We cannot use __APPLE__ as this
     dnl may not be defined on non-OSX Darwin, and we cannot define DARWIN
@@ -3850,7 +3848,6 @@
     ;;
 
   freebsd)
-    AC_DEFINE(BSD4_2, [])
     dnl Hack to avoid calling AC_PREPROC_IFELSE multiple times.
     dnl Would not be needed with autoconf >= 2.67, where the
     dnl preprocessed output is accessible in "conftest.i".
@@ -3858,7 +3855,6 @@
     ;;
 
   gnu | netbsd | openbsd )
-    AC_DEFINE(BSD4_2, [])
     AC_PREPROC_IFELSE([AC_LANG_PROGRAM([[
 #ifndef BSD_SYSTEM
 # error "BSD_SYSTEM not defined"

=== modified file 'doc/lispintro/ChangeLog'
--- doc/lispintro/ChangeLog	2012-12-06 08:33:32 +0000
+++ doc/lispintro/ChangeLog	2012-12-09 00:50:02 +0000
@@ -1,3 +1,10 @@
+2012-12-09  Paul Eggert  <[email-masked]>
+
+	Fix permissions bugs with setgid directories etc.
+	* emacs-lisp-intro.texi (Files List):
+	directory-files-and-attributes now outputs 0 instead of t for
+	attribute that's now a placeholder.
+
 2012-12-06  Paul Eggert  <[email-masked]>
 
 	* doclicense.texi: Update to latest version from FSF.

=== modified file 'doc/lispintro/emacs-lisp-intro.texi'
--- doc/lispintro/emacs-lisp-intro.texi	2012-12-05 22:27:56 +0000
+++ doc/lispintro/emacs-lisp-intro.texi	2012-12-09 00:50:02 +0000
@@ -15687,7 +15687,7 @@
 "-rw-r--r--"
 @end group
 @group
-nil
+0
 2971624
 773)
 @end group

=== modified file 'doc/lispref/ChangeLog'
--- doc/lispref/ChangeLog	2012-12-06 08:33:32 +0000
+++ doc/lispref/ChangeLog	2012-12-09 00:50:02 +0000
@@ -1,3 +1,13 @@
+2012-12-09  Paul Eggert  <[email-masked]>
+
+	Fix permissions bugs with setgid directories etc.
+	* files.texi (Testing Accessibility): Document GROUP arg
+	of file-ownership-preserved-p.
+	(File Attributes): Document that 9th element is now
+	just a placeholder.
+	* os.texi (User Identification): Document new functions group-gid,
+	group-real-gid.
+
 2012-12-06  Paul Eggert  <[email-masked]>
 
 	* doclicense.texi, gpl.texi: Update to latest version from FSF.

=== modified file 'doc/lispref/files.texi'
--- doc/lispref/files.texi	2012-12-05 22:27:56 +0000
+++ doc/lispref/files.texi	2012-12-09 00:50:02 +0000
@@ -895,11 +895,14 @@
 using @var{string} as the error message text.
 @end defun
 
-@defun file-ownership-preserved-p filename
+@defun file-ownership-preserved-p filename &optional group
 This function returns @code{t} if deleting the file @var{filename} and
 then creating it anew would keep the file's owner unchanged.  It also
 returns @code{t} for nonexistent files.
 
+If the optional argument @var{group} is non-@code{nil}, this function
+also checks that the file's group would be unchanged.
+
 If @var{filename} is a symbolic link, then, unlike the other functions
 discussed here, @code{file-ownership-preserved-p} does @emph{not}
 replace @var{filename} with its target.  However, it does recursively
@@ -1246,8 +1249,7 @@
 as in @samp{ls -l}.
 
 @item
-@code{t} if the file's @acronym{GID} would change if file were
-deleted and recreated; @code{nil} otherwise.
+An unspecified value, present for backward compatibility.
 
 @item
 The file's inode number.  If possible, this is an integer.  If the
@@ -1279,7 +1281,7 @@
           (20000 23 0 0)
           (20614 64555 902289 872000)
           122295 "-rw-rw-rw-"
-          nil  (5888 2 . 43978)
+          0  (5888 2 . 43978)
           (15479 . 46724))
 @end group
 @end example
@@ -1318,8 +1320,8 @@
 @item "-rw-rw-rw-"
 has a mode of read and write access for the owner, group, and world.
 
-@item nil
-would retain the same @acronym{GID} if it were recreated.
+@item 0
+is merely a placeholder; it carries no information.
 
 @item (5888 2 . 43978)
 has an inode number of 6473924464520138.

=== modified file 'doc/lispref/os.texi'
--- doc/lispref/os.texi	2012-12-06 06:17:10 +0000
+++ doc/lispref/os.texi	2012-12-09 00:50:02 +0000
@@ -1157,6 +1157,16 @@
 The value may be a floating point number.
 @end defun
 
+@defun group-gid
+This function returns the effective @acronym{GID} of the Emacs process.
+The value may be a floating point number.
+@end defun
+
+@defun group-real-gid
+This function returns the real @acronym{GID} of the Emacs process.
+The value may be a floating point number.
+@end defun
+
 @defun system-users
 This function returns a list of strings, listing the user names on the
 system.  If Emacs cannot retrieve this information, the return value

=== modified file 'etc/ChangeLog'
--- etc/ChangeLog	2012-12-06 22:44:05 +0000
+++ etc/ChangeLog	2012-12-09 00:50:02 +0000
@@ -1,3 +1,10 @@
+2012-12-09  Paul Eggert  <[email-masked]>
+
+	Fix permissions bugs with setgid directories etc.
+	* NEWS: Document changes to file-attributes,
+	file-ownership-preserved-p.
+	Mention new functions group-gid, group-real-gid.
+
 2012-12-06  Andreas Schwab  <[email-masked]>
 
 	* themes/leuven-theme.el: Convert to Unix format.

=== modified file 'etc/NEWS'
--- etc/NEWS	2012-12-07 04:37:14 +0000
+++ etc/NEWS	2012-12-09 00:50:02 +0000
@@ -142,6 +142,17 @@
 ** The `defalias-fset-function' property lets you catch calls to defalias
 and redirect them to your own function instead of `fset'.
 
+** The 9th element returned by `file-attributes' is now unspecified.
+Formerly, it was t if the file's gid would change if file were deleted
+and recreated.  This value has been inaccurate for years on many
+platforms, and nobody seems to have noticed or cared.
+
+** The function `file-ownership-preserved-p' now has an optional
+argument GROUP which causes it check for file group too.  This can be
+used in place of the 9th element of `file-attributes'.
+
+** New functions `group-gid' and `group-real-gid'.
+
 * Changes in Emacs 24.4 on non-free operating systems
 
 +++

=== modified file 'lisp/ChangeLog'
--- lisp/ChangeLog	2012-12-08 23:12:08 +0000
+++ lisp/ChangeLog	2012-12-09 00:50:02 +0000
@@ -1,3 +1,19 @@
+2012-12-09  Paul Eggert  <[email-masked]>
+
+	Fix permissions bugs with setgid directories etc.
+	* files.el (backup-buffer): Don't rely on 9th output of
+	file-attributes, as it's now a placeholder.  Instead, use the new
+	optional arg of file-ownership-preserved-p.
+	(file-ownership-preserved-p): New optional arg GROUP.
+	Fix mishandling of setuid directories that would cause this
+	function to return t when it should have returned nil.
+	Document what happens if the file does not exist, and when
+	it's not known whether the ownership will be preserved.
+	* net/tramp-sh.el (tramp-sh-handle-file-ownership-preserved-p):
+	Likewise.
+	(tramp-get-local-gid): Use group-gid for integer, as that's
+	faster and more reliable.
+
 2012-12-08  Juri Linkov  <[email-masked]>
 
 	* info.el (Info-copy-current-node-name, Info-breadcrumbs)

=== modified file 'lisp/files.el'
--- lisp/files.el	2012-12-03 01:08:31 +0000
+++ lisp/files.el	2012-12-09 00:50:02 +0000
@@ -3941,8 +3941,8 @@
 					      (and (integerp (nth 2 attr))
 						   (integerp backup-by-copying-when-privileged-mismatch)
 						   (<= (nth 2 attr) backup-by-copying-when-privileged-mismatch)))
-					  (or (nth 9 attr)
-					      (not (file-ownership-preserved-p real-file-name)))))))
+					  (not (file-ownership-preserved-p
+						real-file-name t))))))
 			  (backup-buffer-copy real-file-name backupname modes context)
 			;; rename-file should delete old backup.
 			(rename-file real-file-name backupname t)
@@ -4019,22 +4019,40 @@
                    (string-match (concat file-name-version-regexp "\\'")
                                  name))))))
 
-(defun file-ownership-preserved-p (file)
-  "Return t if deleting FILE and rewriting it would preserve the owner."
+(defun file-ownership-preserved-p (file &optional group)
+  "Return t if deleting FILE and rewriting it would preserve the owner.
+Return nil if FILE does not exist, or if deleting and recreating it
+might not preserve the owner.  If GROUP is non-nil, check whether
+the group would be preserved too."
   (let ((handler (find-file-name-handler file 'file-ownership-preserved-p)))
     (if handler
-	(funcall handler 'file-ownership-preserved-p file)
+	(funcall handler 'file-ownership-preserved-p file group)
       (let ((attributes (file-attributes file 'integer)))
 	;; Return t if the file doesn't exist, since it's true that no
 	;; information would be lost by an (attempted) delete and create.
 	(or (null attributes)
-	    (= (nth 2 attributes) (user-uid))
-	    ;; Files created on Windows by Administrator (RID=500)
-	    ;; have the Administrators group (RID=544) recorded as
-	    ;; their owner.  Rewriting them will still preserve the
-	    ;; owner.
-	    (and (eq system-type 'windows-nt)
-		 (= (user-uid) 500) (= (nth 2 attributes) 544)))))))
+	    (and (or (= (nth 2 attributes) (user-uid))
+		     ;; Files created on Windows by Administrator (RID=500)
+		     ;; have the Administrators group (RID=544) recorded as
+		     ;; their owner.  Rewriting them will still preserve the
+		     ;; owner.
+		     (and (eq system-type 'windows-nt)
+			  (= (user-uid) 500) (= (nth 2 attributes) 544)))
+		 (or (not group)
+		     (= (nth 3 attributes) (group-gid)))
+		 (let* ((parent (or (file-name-directory file) "."))
+			(parent-attributes (file-attributes parent 'integer)))
+		   (and parent-attributes
+			;; On some systems, a file created in a setuid directory
+			;; inherits that directory's owner.
+			(or
+			 (= (nth 2 parent-attributes) (user-uid))
+			 (string-match "^...[^sS]" (nth 8 parent-attributes)))
+			;; On many systems, a file created in a setgid directory
+			;; inherits that directory's group.  On some systems
+			;; this happens even if the setgid bit is not set.
+			(or (not group)
+			    (= (nth 3 parent-attributes) (group-gid)))))))))))
 
 (defun file-name-sans-extension (filename)
   "Return FILENAME sans final \"extension\".

=== modified file 'lisp/net/tramp-sh.el'
--- lisp/net/tramp-sh.el	2012-12-06 09:15:27 +0000
+++ lisp/net/tramp-sh.el	2012-12-09 00:50:02 +0000
@@ -1616,7 +1616,7 @@
 	(and (tramp-run-test "-d" (file-name-directory filename))
 	     (tramp-run-test "-w" (file-name-directory filename)))))))
 
-(defun tramp-sh-handle-file-ownership-preserved-p (filename)
+(defun tramp-sh-handle-file-ownership-preserved-p (filename &optional group)
   "Like `file-ownership-preserved-p' for Tramp files."
   (with-parsed-tramp-file-name filename nil
     (with-tramp-file-property v localname "file-ownership-preserved-p"
@@ -1624,7 +1624,10 @@
 	;; Return t if the file doesn't exist, since it's true that no
 	;; information would be lost by an (attempted) delete and create.
 	(or (null attributes)
-	    (= (nth 2 attributes) (tramp-get-remote-uid v 'integer)))))))
+	    (and
+	     (= (nth 2 attributes) (tramp-get-remote-uid v 'integer))
+	     (or (not group)
+		 (= (nth 3 attributes) (tramp-get-remote-gid v 'integer)))))))))
 
 ;; Directory listings.
 
@@ -5021,7 +5024,9 @@
   (if (equal id-format 'integer) (user-uid) (user-login-name)))
 
 (defun tramp-get-local-gid (id-format)
-  (nth 3 (tramp-compat-file-attributes "~/" id-format)))
+  (if (equal id-format 'integer)
+      (group-gid)
+    (nth 3 (tramp-compat-file-attributes "~/" id-format))))
 
 ;; Some predefined connection properties.
 (defun tramp-get-inline-compress (vec prop size)

=== modified file 'src/ChangeLog'
--- src/ChangeLog	2012-12-08 18:27:37 +0000
+++ src/ChangeLog	2012-12-09 00:50:02 +0000
@@ -1,3 +1,12 @@
+2012-12-09  Paul Eggert  <[email-masked]>
+
+	Fix permissions bugs with setgid directories etc.
+	* dired.c (Ffile_attributes): Return 0 as the 9th attribute,
+	to mark it as a placeholder.  The old value was often wrong.
+	The only user of this attribute has been changed to use
+	file-ownership-preserved-p instead, with its new group arg.
+	* editfns.c (Fgroup_gid, Fgroup_real_gid): New functions.
+
 2012-12-08  Eli Zaretskii  <[email-masked]>
 
 	* w32.c (unsetenv): Return 0 if the input string is too long.

=== modified file 'src/dired.c'
--- src/dired.c	2012-11-27 05:38:42 +0000
+++ src/dired.c	2012-12-09 00:50:02 +0000
@@ -869,7 +869,7 @@
  7. Size in bytes.
   This is a floating point number if the size is too large for an integer.
  8. File modes, as a string of ten letters or dashes as in ls -l.
- 9. t if file's gid would change if file were deleted and recreated.
+ 9. An unspecified value, present only for backward compatibility.
 10. inode number.  If it is larger than what an Emacs integer can hold,
   this is of the form (HIGH . LOW): first the high bits, then the low 16 bits.
   If even HIGH is too large for an Emacs integer, this is instead of the form
@@ -891,10 +891,6 @@
   Lisp_Object values[12];
   Lisp_Object encoded;
   struct stat s;
-#ifdef BSD4_2
-  Lisp_Object dirname;
-  struct stat sdir;
-#endif /* BSD4_2 */
 
   /* An array to hold the mode string generated by filemodestring,
      including its terminating space and null byte.  */
@@ -959,17 +955,7 @@
 
   filemodestring (&s, modes);
   values[8] = make_string (modes, 10);
-#ifdef BSD4_2 /* file gid will be dir gid */
-  dirname = Ffile_name_directory (filename);
-  if (! NILP (dirname))
-    encoded = ENCODE_FILE (dirname);
-  if (! NILP (dirname) && stat (SDATA (encoded), &sdir) == 0)
-    values[9] = (sdir.st_gid != s.st_gid) ? Qt : Qnil;
-  else					/* if we can't tell, assume worst */
-    values[9] = Qt;
-#else					/* file gid will be egid */
-  values[9] = (s.st_gid != getegid ()) ? Qt : Qnil;
-#endif	/* not BSD4_2 */
+  values[9] = make_number (0);
   values[10] = INTEGER_TO_CONS (s.st_ino);
   values[11] = INTEGER_TO_CONS (s.st_dev);

=== modified file 'src/editfns.c'
--- src/editfns.c	2012-12-08 17:19:51 +0000
+++ src/editfns.c	2012-12-09 00:50:02 +0000
@@ -1272,6 +1272,24 @@
   return make_fixnum_or_float (uid);
 }
 
+DEFUN ("group-gid", Fgroup_gid, Sgroup_gid, 0, 0, 0,
+       doc: /* Return the effective gid of Emacs.
+Value is an integer or a float, depending on the value.  */)
+  (void)
+{
+  gid_t egid = getegid ();
+  return make_fixnum_or_float (egid);
+}
+
+DEFUN ("group-real-gid", Fgroup_real_gid, Sgroup_real_gid, 0, 0, 0,
+       doc: /* Return the real gid of Emacs.
+Value is an integer or a float, depending on the value.  */)
+  (void)
+{
+  gid_t gid = getgid ();
+  return make_fixnum_or_float (gid);
+}
+
 DEFUN ("user-full-name", Fuser_full_name, Suser_full_name, 0, 1, 0,
        doc: /* Return the full name of the user logged in, as a string.
 If the full name corresponding to Emacs's userid is not known,
@@ -4899,6 +4917,8 @@
   defsubr (&Suser_real_login_name);
   defsubr (&Suser_uid);
   defsubr (&Suser_real_uid);
+  defsubr (&Sgroup_gid);
+  defsubr (&Sgroup_real_gid);
   defsubr (&Suser_full_name);
   defsubr (&Semacs_pid);
   defsubr (&Scurrent_time);