GNU bug report logs - #13125
Fix permissions bugs with setgid directories etc.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 13125 in the body.
You can then email your comments to 13125 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: bug-gnu-emacs <at> gnu.org
Subject: Fix permissions bugs with setgid directories etc.
Date: Sat, 08 Dec 2012 17:13:50 -0800

[Message part 1 (text/plain, inline)]

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.

[setgiddir.txt (text/plain, attachment)]

Message #8 received at 13125 <at> debbugs.gnu.org (full text, mbox):

From: Chong Yidong <cyd <at> gnu.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 13125 <at> debbugs.gnu.org
Subject: Re: bug#13125: Fix permissions bugs with setgid directories etc.
Date: Sun, 09 Dec 2012 11:54:49 +0800

Paul Eggert <eggert <at> cs.ucla.edu> writes:

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

Have you actually done a survey of all the callers to file-attributes to
see if this is true?

> Instead, the patch moves this functionality to
> file-ownership-preserved-p via a new argument GROUP.

Why can't this functionality be kept in file-attributes?

Message #11 received at 13125 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Chong Yidong <cyd <at> gnu.org>
Cc: 13125 <at> debbugs.gnu.org
Subject: Re: bug#13125: Fix permissions bugs with setgid directories etc.
Date: Sat, 08 Dec 2012 23:26:27 -0800

On 12/08/2012 07:54 PM, Chong Yidong wrote:
> Have you actually done a survey of all the callers to file-attributes to
> see if this is true?

Yes, all the callers that are distributed as part of Emacs.

>> > Instead, the patch moves this functionality to
>> > file-ownership-preserved-p via a new argument GROUP.
> Why can't this functionality be kept in file-attributes?

Performance.  The functionality requires statting the parent directory
for each call to file-attributes.

Message #14 received at 13125 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 13125 <at> debbugs.gnu.org
Subject: Re: bug#13125: Fix permissions bugs with setgid directories etc.
Date: Sun, 09 Dec 2012 09:32:32 +0100

Paul Eggert <eggert <at> cs.ucla.edu> writes:

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

You have also patched tramp-sh.el, thanks. However, Tramp has its own
life outside Emacs, the change must be backwards compatible for older
Emacs versions as well as for XEmacs.

Best regards, Michael.

Message #17 received at 13125 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Michael Albinus <michael.albinus <at> gmx.de>
Cc: 13125 <at> debbugs.gnu.org
Subject: Re: bug#13125: Fix permissions bugs with setgid directories etc.
Date: Sun, 09 Dec 2012 00:56:48 -0800

On 12/09/2012 12:32 AM, Michael Albinus wrote:
> the change must be backwards compatible for older
> Emacs versions as well as for XEmacs

Thanks for mentioning that.
The following further patch should suffice:

=== modified file 'lisp/net/tramp-sh.el'
--- lisp/net/tramp-sh.el	2012-12-09 00:50:02 +0000
+++ lisp/net/tramp-sh.el	2012-12-09 08:53:55 +0000
@@ -5024,8 +5024,8 @@
   (if (equal id-format 'integer) (user-uid) (user-login-name)))
 
 (defun tramp-get-local-gid (id-format)
-  (if (equal id-format 'integer)
-      (group-gid)
+  (if (and (fboundp 'group-gid) (equal id-format 'integer))
+      (tramp-compat-funcall 'group-gid)
     (nth 3 (tramp-compat-file-attributes "~/" id-format))))
 
 ;; Some predefined connection properties.

Message #20 received at 13125 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 13125 <at> debbugs.gnu.org
Subject: Re: bug#13125: Fix permissions bugs with setgid directories etc.
Date: Sun, 09 Dec 2012 10:32:07 +0100

Paul Eggert <eggert <at> cs.ucla.edu> writes:

> The following further patch should suffice:
>
> === modified file 'lisp/net/tramp-sh.el'
> --- lisp/net/tramp-sh.el	2012-12-09 00:50:02 +0000
> +++ lisp/net/tramp-sh.el	2012-12-09 08:53:55 +0000
> @@ -5024,8 +5024,8 @@
>    (if (equal id-format 'integer) (user-uid) (user-login-name)))
>  
>  (defun tramp-get-local-gid (id-format)
> -  (if (equal id-format 'integer)
> -      (group-gid)
> +  (if (and (fboundp 'group-gid) (equal id-format 'integer))
> +      (tramp-compat-funcall 'group-gid)
>      (nth 3 (tramp-compat-file-attributes "~/" id-format))))
>  
>  ;; Some predefined connection properties.

Thanks!

Best regards, Michael.

Message #23 received at 13125 <at> debbugs.gnu.org (full text, mbox):

From: Wolfgang Jenkner <wjenkner <at> inode.at>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 13125 <at> debbugs.gnu.org
Subject: Re: bug#13125: Fix permissions bugs with setgid directories etc.
Date: Sun, 09 Dec 2012 17:43:31 +0100

On Sun, Dec 09 2012, Paul Eggert wrote:

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

I understand you are describing here the most common behaviour only for
non-4.2BSD descendants?

I've tested your patch by typing the following in a *shell* buffer.

[[1 ~]]$ uname -rs
FreeBSD 9.1-PRERELEASE
[[2 ~]]$ id
uid=1002(wolfgang) gid=20(staff) groups=20(staff),0(wheel),5(operator)
[[3 ~]]$ ls -ld /tmp
drwxrwxrwt  8 root  wheel  512 Dec  9 16:59 /tmp/
[[4 ~]]$ rm -f /tmp/foo && touch $_
[[5 ~]]$ ls -l $_
-rw-r--r--  1 wolfgang  wheel  0 Dec  9 17:01 /tmp/foo
[[6 ~]]$ 

Then, in the same emacs process, I evaluate

(file-ownership-preserved-p "/tmp/foo")
=> t

which is fine, but

(file-ownership-preserved-p "/tmp/foo" t)
=> nil

is not since /tmp/foo will always be created in the wheel group.
Indeed, in an unpatched emacs, I get the expected

(nth 9 (file-attributes "/tmp/foo"))
=> nil

Now, open(2) on all free BSD descendants invariably, literally and
unconditionally states

     When a new file is created it is given the group of the directory which
     contains it.

So I wonder if the following lightly tested patch (on top of yours)
would give better results in this case (in the absence of races with
other processes).

Wolfgang

=== modified file 'lisp/files.el'
--- lisp/files.el	2012-12-09 15:29:12 +0000
+++ lisp/files.el	2012-12-09 16:25:09 +0000
@@ -4039,6 +4039,7 @@
 		     (and (eq system-type 'windows-nt)
 			  (= (user-uid) 500) (= (nth 2 attributes) 544)))
 		 (or (not group)
+		     (memq system-type '(berkeley-unix darwin))
 		     (= (nth 3 attributes) (group-gid)))
 		 (let* ((parent (or (file-name-directory file) "."))
 			(parent-attributes (file-attributes parent 'integer)))
@@ -4052,7 +4053,10 @@
 			;; 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)))))))))))
+			    (= (nth 3 parent-attributes)
+			       (if (memq system-type '(berkeley-unix darwin))
+				   (nth 3 attributes)
+				 (group-gid))))))))))))
 
 (defun file-name-sans-extension (filename)
   "Return FILENAME sans final \"extension\".

Message #26 received at 13125 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 13125 <at> debbugs.gnu.org, cyd <at> gnu.org
Subject: Re: bug#13125: Fix permissions bugs with setgid directories etc.
Date: Sun, 09 Dec 2012 19:03:04 +0200

> Date: Sat, 08 Dec 2012 23:26:27 -0800
> From: Paul Eggert <eggert <at> cs.ucla.edu>
> Cc: 13125 <at> debbugs.gnu.org
> 
> On 12/08/2012 07:54 PM, Chong Yidong wrote:
> > Have you actually done a survey of all the callers to file-attributes to
> > see if this is true?
> 
> Yes, all the callers that are distributed as part of Emacs.

Did you find _any_ of them that even reference this attribute?  I
didn't, but perhaps I missed something.

Given the "wide" use, it is hard to reason what should be the value of
this attribute after these changes are installed.  You set them to
zero, which is neither nil nor t; thus, code that was testing for
either of these two values explicitly will now fail, while code that
was testing for non-nil will succeed where perhaps it shouldn't have.
For these reasons, I would suggest to leave the value at one of these
two, whichever is more frequent in real life.

Message #29 received at 13125 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Wolfgang Jenkner <wjenkner <at> inode.at>
Cc: 13125 <at> debbugs.gnu.org
Subject: Re: bug#13125: Fix permissions bugs with setgid directories etc.
Date: Sun, 09 Dec 2012 16:46:02 -0800

On 12/09/2012 08:43 AM, Wolfgang Jenkner wrote:

> I understand you are describing here the most common behaviour only for
> non-4.2BSD descendants?

Yes, that's right.

> (in the absence of races with other processes)

Yes, races are a problem, both with current Emacs and with the patch.
It'd be good to fix this separate problem, when someone finds the time.
At least the proposed patch does not make things worse in this respect.

> Now, open(2) on all free BSD descendants invariably, literally and
> unconditionally states
> 
>      When a new file is created it is given the group of the directory which
>      contains it.

I was worried about what happens with a BSD client of an
NFS server running some non-BSD OS.  But if it's safe to
assume BSD semantics even then, your suggestion is a good one,
as it'll make Emacs more efficient.

Two thoughts.  First, shouldn't gnu/kfreebsd be treated as a BSD
system in this respect?  Second, the second part of the test can
be simplified a tad.  So, how about the following patch instead?

=== modified file 'lisp/files.el'
--- lisp/files.el	2012-12-09 00:50:02 +0000
+++ lisp/files.el	2012-12-10 00:38:45 +0000
@@ -4039,6 +4039,9 @@ the group would be preserved too."
 		     (and (eq system-type 'windows-nt)
 			  (= (user-uid) 500) (= (nth 2 attributes) 544)))
 		 (or (not group)
+		     ;; On BSD-derived systems files always inherit the parent
+		     ;; directory's group, so skip the group-gid test.
+		     (memq system-type '(berkeley-unix darwin gnu/kfreebsd))
 		     (= (nth 3 attributes) (group-gid)))
 		 (let* ((parent (or (file-name-directory file) "."))
 			(parent-attributes (file-attributes parent 'integer)))
@@ -4052,7 +4055,8 @@ the group would be preserved too."
 			;; 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)))))))))))
+			    (= (nth 3 parent-attributes)
+			       (nth 3 attributes)))))))))))
 
 (defun file-name-sans-extension (filename)
   "Return FILENAME sans final \"extension\".

Message #32 received at 13125 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 13125 <at> debbugs.gnu.org, cyd <at> gnu.org
Subject: Re: bug#13125: Fix permissions bugs with setgid directories etc.
Date: Sun, 09 Dec 2012 17:08:00 -0800

[Message part 1 (text/plain, inline)]

On 12/09/2012 09:03 AM, Eli Zaretskii wrote:

> Did you find _any_ of them that even reference this attribute?

Yes, just one: backup-buffer.  It's fixed in the proposed patch,
in the first hunk of the lisp/files.el patch.

> Given the "wide" use, it is hard to reason what should be the value of
> this attribute after these changes are installed.  You set them to
> zero, which is neither nil nor t; thus, code that was testing for
> either of these two values explicitly will now fail, while code that
> was testing for non-nil will succeed where perhaps it shouldn't have.

The only example I found, in backup-buffer, was testing for non-nil.
Zero counts as non-nil, so if backup-buffer were not changed, it'd be
treating the value as t.  This would be safe, as it's the nil case
that is dangerous.  (With the further change below, this paragraph is
moot.)

> I would suggest to leave the value at one of these
> two, whichever is more frequent in real life.

If we were to leave the value as one of these two, we should leave it
as t, the safer value.  Here's a further patch to do that, and I'll
attach the updated combined patch (integrating all the further patches
suggested so far), relative to trunk bzr 111167.

=== modified file 'doc/lispintro/ChangeLog'
--- doc/lispintro/ChangeLog	2012-12-09 02:30:06 +0000
+++ doc/lispintro/ChangeLog	2012-12-10 00:56:35 +0000
@@ -1,9 +1,9 @@
-2012-12-09  Paul Eggert  <eggert <at> cs.ucla.edu>
+2012-12-10  Paul Eggert  <eggert <at> cs.ucla.edu>
 
 	Fix permissions bugs with setgid directories etc. (Bug#13125)
 	* emacs-lisp-intro.texi (Files List):
-	directory-files-and-attributes now outputs 0 instead of t for
-	attribute that's now a placeholder.
+	directory-files-and-attributes now outputs t for attribute that's
+	now a placeholder.
 
 2012-12-06  Paul Eggert  <eggert <at> cs.ucla.edu>
 

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

=== modified file 'doc/lispref/files.texi'
--- doc/lispref/files.texi	2012-12-09 00:50:02 +0000
+++ doc/lispref/files.texi	2012-12-10 00:56:35 +0000
@@ -1281,7 +1281,7 @@
           (20000 23 0 0)
           (20614 64555 902289 872000)
           122295 "-rw-rw-rw-"
-          0  (5888 2 . 43978)
+          t (5888 2 . 43978)
           (15479 . 46724))
 @end group
 @end example
@@ -1320,7 +1320,7 @@
 @item "-rw-rw-rw-"
 has a mode of read and write access for the owner, group, and world.
 
-@item 0
+@item t
 is merely a placeholder; it carries no information.
 
 @item (5888 2 . 43978)

=== modified file 'src/dired.c'
--- src/dired.c	2012-12-09 00:50:02 +0000
+++ src/dired.c	2012-12-10 00:56:35 +0000
@@ -955,7 +955,7 @@
 
   filemodestring (&s, modes);
   values[8] = make_string (modes, 10);
-  values[9] = make_number (0);
+  values[9] = Qt;
   values[10] = INTEGER_TO_CONS (s.st_ino);
   values[11] = INTEGER_TO_CONS (s.st_dev);
 

[setgiddir.txt (text/plain, attachment)]

Message #37 received at 13125-done <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: 13125-done <at> debbugs.gnu.org
Subject: Re: bug#13125: Fix permissions bugs with setgid directories etc.
Date: Fri, 14 Dec 2012 11:00:09 -0800

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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.