set-file-extended-attributes and backups

set-file-extended-attributes and backups

[Eli Zaretskii]

Fabrice reported that since the introduction of ACLs into backing up
of files (as part as saving a buffer), he gets errors (on MS-Windows
8).  It turns out his user profile doesn't have the privileges to set
file security descriptors.  This can be fixed by granting the
privileges in the local policy, but the issue I raise is should this
problem be exposed in the context of backing up and saving files?

I think this problem is not Windows-specific.  So I'm asking here:
does it make sense to fail backup-buffer and backup-buffer-copy just
because set-file-extended-attributes fails?  I think we should ignore
such errors in these cases, and propose the changes below.

P.S.  If anyone wants to argue that set-file-acl should ignore this,
then I disagree: it's up to the application to decide whether or not
to ignore such problems.

=== modified file 'lisp/files.el'
--- lisp/files.el	2012-12-17 15:51:49 +0000
+++ lisp/files.el	2012-12-21 12:10:16 +0000
@@ -4022,7 +4022,8 @@ BACKUPNAME is the backup file name, whic
   (and modes
        (set-file-modes to-name (logand modes #o1777)))
   (and extended-attributes
-       (set-file-extended-attributes to-name extended-attributes)))
+       (ignore-errors
+	 (set-file-extended-attributes to-name extended-attributes))))
 
 (defvar file-name-version-regexp
   "\\(?:~\\|\\.~[-[:alnum:]:#@^._]+\\(?:~[[:digit:]]+\\)?~\\)"
@@ -4738,7 +4739,9 @@ Before and after saving the buffer, this
 				    (file-extended-attributes buffer-file-name)
 				    buffer-file-name))
 	       (set-file-modes buffer-file-name (logior (car setmodes) 128))
-	       (set-file-extended-attributes buffer-file-name (nth 1 setmodes)))))
+	       (ignore-errors
+		 (set-file-extended-attributes buffer-file-name
+					       (nth 1 setmodes))))))
 	(let (success)
 	  (unwind-protect
 	      (progn

Re: set-file-extended-attributes and backups

[Paul Eggert]

On 12/21/12 06:53, Eli Zaretskii wrote:
> I think this problem is not Windows-specific.  So I'm asking here:
> does it make sense to fail backup-buffer and backup-buffer-copy just
> because set-file-extended-attributes fails?  I think we should ignore
> such errors

On systems where ACLs can deny access to files, failing to
copy an ACL can mean that the copy has more permissions
than the original, no?  Wouldn't that be a security problem?

As I understand it, Windows ACLs can deny access, just as
Posix ACLs can, so this issue is relevant on Windows too.

The recently-added ACL code has some security holes in
this area, doesn't it?  It's copying file mode separately
from copying ACLs.  Surely the code should just copy ACLs,
as there's a race condition now, where the file is
temporarily exposed between the times the mode and the
ACLs are copied.

Re: set-file-extended-attributes and backups

[Eli Zaretskii]

> Date: Fri, 21 Dec 2012 08:00:01 -0800
> From: Paul Eggert <eggert@cs.ucla.edu>
> Cc: Romain Francoise <romain@orebokech.com>
> 
> On 12/21/12 06:53, Eli Zaretskii wrote:
> > I think this problem is not Windows-specific.  So I'm asking here:
> > does it make sense to fail backup-buffer and backup-buffer-copy just
> > because set-file-extended-attributes fails?  I think we should ignore
> > such errors
> 
> On systems where ACLs can deny access to files, failing to
> copy an ACL can mean that the copy has more permissions
> than the original, no?

Yes, I think so, at least when we are not backing up by renaming.

> Wouldn't that be a security problem?

Maybe.  But Emacs does the same on platforms where ACLs are not
accessible to Emacs, so if there's a security problem, we already have
it, I think.

> As I understand it, Windows ACLs can deny access, just as
> Posix ACLs can, so this issue is relevant on Windows too.

Yes, Windows ACLs can deny access, and yes, it is relevant.

> The recently-added ACL code has some security holes in
> this area, doesn't it?  It's copying file mode separately
> from copying ACLs.  Surely the code should just copy ACLs,
> as there's a race condition now, where the file is
> temporarily exposed between the times the mode and the
> ACLs are copied.

How about if it tried to copy ACLs, and if that failed, attempted to
copy the file modes?  That would DTRT if possible, and fall back on
the pre-ACL method if not.

Re: set-file-extended-attributes and backups

[Paul Eggert]

On 12/21/12 08:44, Eli Zaretskii wrote:
> How about if it tried to copy ACLs, and if that failed, attempted to
> copy the file modes?  That would DTRT if possible, and fall back on
> the pre-ACL method if not.

That could lead to security issues if the file modes are more
permissive than the ACLs.

Is there an easy way to test whether a file's ACLs could deny
access when the file's modes would allow it?  If so, Emacs could
follow your suggestion when that test says "no".  If not, Emacs
could fall back on a conservative approximation to that test.
The simplest conservative approximation that I can think of offhand
is to test whether a file has any nontrivial ACLs.

Whatever test Emacs uses, if the test says "yes" Emacs should
be more cautious: create a destination file with a restrictive
mode (e.g., -rw-------), copy the data, then attempt to copy the ACLs,
and if the ACL copy fails then Emacs should not attempt to change
the mode.

Re: set-file-extended-attributes and backups

[Eli Zaretskii]

> Date: Fri, 21 Dec 2012 09:48:20 -0800
> From: Paul Eggert <eggert@cs.ucla.edu>
> CC: emacs-devel@gnu.org, romain@orebokech.com
> 
> On 12/21/12 08:44, Eli Zaretskii wrote:
> > How about if it tried to copy ACLs, and if that failed, attempted to
> > copy the file modes?  That would DTRT if possible, and fall back on
> > the pre-ACL method if not.
> 
> That could lead to security issues if the file modes are more
> permissive than the ACLs.

But we did that until a week ago.  If we want Emacs to be more secure,
just because it can now access ACLs, this decision should be left to
the user, i.e. be a user option.  Otherwise, we are forcing users the
level of security they not necessary want.

> Is there an easy way to test whether a file's ACLs could deny
> access when the file's modes would allow it?

There are no modes without ACLs.  Systems that support ACLs always
provide ACLs for files, just the default ones.  So what you ask is
whether the default ACLs will allow some access that a specific ACLs
won't.  And the answer to that is "it depends on the user" whose
access we are interested in.  E.g., if the default ACLs allow some
access to the file's group, the answer depends on whether a user
belongs to that group.

> The simplest conservative approximation that I can think of offhand
> is to test whether a file has any nontrivial ACLs.

That's not good enough, I think: if the nontrivial ACLs specify the
same group as the file's group, the modes and the ACLs are equivalent,
although the ACLs are "nontrivial".

> Whatever test Emacs uses, if the test says "yes" Emacs should
> be more cautious: create a destination file with a restrictive
> mode (e.g., -rw-------), copy the data, then attempt to copy the ACLs,
> and if the ACL copy fails then Emacs should not attempt to change
> the mode.

That assumes that -rw------- is secure.  But that assumption is false,
because ACLs can be more restrictive than that, even on Posix
platforms.  E.g., they could disallow write access to the user who
makes the copy, or disallow attributes changes.

Re: set-file-extended-attributes and backups

[Paul Eggert]

On 12/21/12 10:08, Eli Zaretskii wrote:

> But we did that until a week ago.

True, but we're trying to do the right thing with ACLs now,
rather than ignore them and do the wrong thing.

> this decision should be left to the user, i.e. be a user option.

Possibly, but the default should be safe, i.e., it shouldn't
grant access rights that were not already there.

> So what you ask is
> whether the default ACLs will allow some access that a specific ACLs
> won't.  And the answer to that is "it depends ..."

Yes, and the question is whether it's easy to find out
all the dependencies, so that Emacs can tell whether the
default ACLs would allow any access that the correct ACLs
would deny.  My guess is that the answer is "no", unfortunately.

>> The simplest conservative approximation that I can think of offhand
>> is to test whether a file has any nontrivial ACLs.
> 
> That's not good enough, I think: if the nontrivial ACLs specify the
> same group as the file's group, the modes and the ACLs are equivalent,
> although the ACLs are "nontrivial".

Sure, but here it's OK from a security point of view to use a
conservative approximation, i.e., a test that sometimes says
"yes" even when the true answer is "no".  The only downside is
that when the conservative approximation is incorrect, then when
the ACLs cannot be copied the file will end up in mode -rw-------.
That's annoying, but it's safe and I hope it's rare.

> That assumes that -rw------- is secure.  But that assumption is false,
> because ACLs can be more restrictive than that, even on Posix
> platforms.

No, because if an attacker can read and write a file with
permissions -rw-------, then the attacker owns the file
(or is superuser) and can change its ACLs.  ACLs cannot stop
such an attacker.  So long as Emacs doesn't grant permissions
to anybody other than the owner, Emacs is not giving any
secrets away that aren't being given away already.

As a minor nicety, we could use mode ----------, if we prefer being safer
in an advisory sense.  Mode ---------- won't stop an attacker
who is the owner, but it would be more likely to prevent
blundering owners from shooting themselves in the foot.

Re: set-file-extended-attributes and backups

[Romain Francoise]

Eli Zaretskii <eliz@gnu.org> writes:

> How about if it tried to copy ACLs, and if that failed, attempted to
> copy the file modes?  That would DTRT if possible, and fall back on
> the pre-ACL method if not.

If we do that we need to change `set-file-acl' to treat ENOTSUP as an
error otherwise modes won't be copied on filesystems without ACL support.

Re: set-file-extended-attributes and backups

[Eli Zaretskii]

> From: Romain Francoise <romain@orebokech.com>
> Cc: Paul Eggert <eggert@cs.ucla.edu>,  emacs-devel@gnu.org
> Date: Sun, 23 Dec 2012 17:59:52 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > How about if it tried to copy ACLs, and if that failed, attempted to
> > copy the file modes?  That would DTRT if possible, and fall back on
> > the pre-ACL method if not.
> 
> If we do that we need to change `set-file-acl' to treat ENOTSUP as an
> error otherwise modes won't be copied on filesystems without ACL support.

How about changing its return value instead?  Say, return t if ACLs
set, nil otherwise.  It always bothers me to have an API that silently
fails without any tangible indication.

Re: set-file-extended-attributes and backups

[Stefan Monnier]

> How about changing its return value instead?  Say, return t if ACLs
> set, nil otherwise.  It always bothers me to have an API that silently
> fails without any tangible indication.

I think it should fail (i.e. signal an error) for any case where the
file's ACLs were not set as requested.

The error signaled should be different for "ACLs not supported" and "you
do not have the right to set ACL on this file".


        Stefan

Re: set-file-extended-attributes and backups

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Romain Francoise <romain@orebokech.com>,  eggert@cs.ucla.edu,  emacs-devel@gnu.org
> Date: Sun, 23 Dec 2012 19:59:35 -0500
> 
> > How about changing its return value instead?  Say, return t if ACLs
> > set, nil otherwise.  It always bothers me to have an API that silently
> > fails without any tangible indication.
> 
> I think it should fail (i.e. signal an error) for any case where the
> file's ACLs were not set as requested.

ENOTSUP usually means the filesystem doesn't support ACLs.  Having an
error that every single use of it will want to ignore doesn't seem to
be useful.

Re: set-file-extended-attributes and backups

[Stefan Monnier]

> ENOTSUP usually means the filesystem doesn't support ACLs.  Having an
> error that every single use of it will want to ignore doesn't seem to
> be useful.

But if the FS doesn't support ACL, then file-acl should have returned
nil, so we don't need to call set-file-acl at all.  IOW, those errors
should not be frequent.


        Stefan

Re: set-file-extended-attributes and backups

[Michael Albinus]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> ENOTSUP usually means the filesystem doesn't support ACLs.  Having an
>> error that every single use of it will want to ignore doesn't seem to
>> be useful.
>
> But if the FS doesn't support ACL, then file-acl should have returned
> nil, so we don't need to call set-file-acl at all.  IOW, those errors
> should not be frequent.

And Tramp will suppress both errors.

>         Stefan

Best regards, Michael.

Re: set-file-extended-attributes and backups

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: romain@orebokech.com,  eggert@cs.ucla.edu,  emacs-devel@gnu.org
> Date: Mon, 24 Dec 2012 00:18:59 -0500
> 
> > ENOTSUP usually means the filesystem doesn't support ACLs.  Having an
> > error that every single use of it will want to ignore doesn't seem to
> > be useful.
> 
> But if the FS doesn't support ACL, then file-acl should have returned
> nil, so we don't need to call set-file-acl at all.

What about copying files from one FS to another?  Like from the DoK to
your local disk or vice versa?

> IOW, those errors should not be frequent.

Frequency is immaterial here, IMO.  If, when the error happens, we
will always want to ignore it, then raising it is not useful.
Especially that we would need to invent yet another kind of error to
distinguish a not-supported error from something-else-wrong error.

Re: set-file-extended-attributes and backups

[Romain Francoise]

Hi Eli,

Eli Zaretskii <eliz@gnu.org> writes:

> I think this problem is not Windows-specific.  So I'm asking here:
> does it make sense to fail backup-buffer and backup-buffer-copy just
> because set-file-extended-attributes fails?

Probably not. We should follow coreutils and just issue a warning if we
can't copy the acl (which is the default behavior of `mv' and also `cp' if
`--preserve=all' is used).

> I think we should ignore such errors in these cases, and propose the
> changes below.

I think that is too radical, it should display a warning so that the user
can decide what to do. Since Emacs only makes a backup the first time you
save, the warning shouldn't be too annoying.

Re: set-file-extended-attributes and backups

[Fabrice Popineau]

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

>
> I think that is too radical, it should display a warning so that the user
> can decide what to do. Since Emacs only makes a backup the first time you
> save, the warning shouldn't be too annoying.
>

The warning would happen to every single file you edit with emacs.
And I'm running Windows with an account with elevated privileges (so-called
admin account) with default settings. I guess lots of people are in this
situation.
If I run emacs "as administrator", then there is no problem. But this is
far too
dangerous in my opinion.

Eli as offered an interesting option : to avoidt trying to set the acl if
it hasn't changed.
Which should be the case all the time (or most of the time). Combining the
warning
and this other fix could be the best solution.

-- 
Fabrice

[-- Attachment #2: Type: text/html, Size: 1345 bytes --]

Re: set-file-extended-attributes and backups

[Eli Zaretskii]

> From: Fabrice Popineau <fabrice.popineau@supelec.fr>
> Date: Sun, 23 Dec 2012 00:03:54 +0100
> Cc: Eli Zaretskii <eliz@gnu.org>, emacs-devel@gnu.org
> 
> Eli as offered an interesting option : to avoid trying to set the acl if
> it hasn't changed.

I will do this as part of the Windows implementation of the ACL
interfaces.  So if this kind of no-op ACL setting can barf only on
Windows, I think a warning in set-file-extended-attributes should be
enough.

Re: set-file-extended-attributes and backups

[Eli Zaretskii]

> Date: Sun, 23 Dec 2012 05:54:17 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: romain@orebokech.com, emacs-devel@gnu.org
> 
> > From: Fabrice Popineau <fabrice.popineau@supelec.fr>
> > Date: Sun, 23 Dec 2012 00:03:54 +0100
> > Cc: Eli Zaretskii <eliz@gnu.org>, emacs-devel@gnu.org
> > 
> > Eli as offered an interesting option : to avoid trying to set the acl if
> > it hasn't changed.
> 
> I will do this as part of the Windows implementation of the ACL
> interfaces.

Done.

Re: set-file-extended-attributes and backups

[Stefan Monnier]

> +       (ignore-errors
> +	 (set-file-extended-attributes to-name extended-attributes))))
 
I'd use something like with-demoted-errors instead.


        Stefan

Re: set-file-extended-attributes and backups

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: emacs-devel@gnu.org,  Fabrice Popineau <fabrice.popineau@supelec.fr>
> Date: Sat, 22 Dec 2012 11:05:21 -0500
> 
> > +       (ignore-errors
> > +	 (set-file-extended-attributes to-name extended-attributes))))
>  
> I'd use something like with-demoted-errors instead.

What about the race condition pointed out by Paul?  Should we try
set-file-extended-attributes first, and fall back on set-file-modes if
that fails (after displaying a message)?

Re: set-file-extended-attributes and backups

[Stefan Monnier]

>> > +       (ignore-errors
>> > +	 (set-file-extended-attributes to-name extended-attributes))))
>> I'd use something like with-demoted-errors instead.
> What about the race condition pointed out by Paul?  Should we try
> set-file-extended-attributes first, and fall back on set-file-modes if
> that fails (after displaying a message)?

That sounds fine, yes.


        Stefan

Re: set-file-extended-attributes and backups

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: fabrice.popineau@supelec.fr,  emacs-devel@gnu.org
> Date: Sun, 23 Dec 2012 08:37:52 -0500
> 
> >> > +       (ignore-errors
> >> > +	 (set-file-extended-attributes to-name extended-attributes))))
> >> I'd use something like with-demoted-errors instead.
> > What about the race condition pointed out by Paul?  Should we try
> > set-file-extended-attributes first, and fall back on set-file-modes if
> > that fails (after displaying a message)?
> 
> That sounds fine, yes.

Now done on the trunk.

Re: set-file-extended-attributes and backups

[Eli Zaretskii]

> Date: Sat, 29 Dec 2012 19:20:30 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: fabrice.popineau@supelec.fr, emacs-devel@gnu.org
> 
> > From: Stefan Monnier <monnier@iro.umontreal.ca>
> > Cc: fabrice.popineau@supelec.fr,  emacs-devel@gnu.org
> > Date: Sun, 23 Dec 2012 08:37:52 -0500
> > 
> > >> > +       (ignore-errors
> > >> > +	 (set-file-extended-attributes to-name extended-attributes))))
> > >> I'd use something like with-demoted-errors instead.
> > > What about the race condition pointed out by Paul?  Should we try
> > > set-file-extended-attributes first, and fall back on set-file-modes if
> > > that fails (after displaying a message)?
> > 
> > That sounds fine, yes.
> 
> Now done on the trunk.

One side effect of these changes is that set-file-acl and
set-file-selinux-context now return t when they succeeded to set the
ACLs resp. SELinux context, and nil otherwise.  Micheal, could you
please adapt the Tramp handlers to this paradigm?  Thanks.

Re: set-file-extended-attributes and backups

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

> One side effect of these changes is that set-file-acl and
> set-file-selinux-context now return t when they succeeded to set the
> ACLs resp. SELinux context, and nil otherwise.  Micheal, could you
> please adapt the Tramp handlers to this paradigm?  Thanks.

Done for ACL. For SELinux, I'll check later.

Best regards, Michael.

Re: set-file-extended-attributes and backups

[Michael Albinus]

Michael Albinus <michael.albinus@gmx.de> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>> One side effect of these changes is that set-file-acl and
>> set-file-selinux-context now return t when they succeeded to set the
>> ACLs resp. SELinux context, and nil otherwise.  Micheal, could you
>> please adapt the Tramp handlers to this paradigm?  Thanks.
>
> Done for ACL. For SELinux, I'll check later.

SELinux case is also fixed now.

Best regards, Michael.

Re: set-file-extended-attributes and backups

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: monnier@iro.umontreal.ca,  emacs-devel@gnu.org
> Date: Sun, 30 Dec 2012 11:59:54 +0100
> 
> Michael Albinus <michael.albinus@gmx.de> writes:
> 
> > Eli Zaretskii <eliz@gnu.org> writes:
> >
> >> One side effect of these changes is that set-file-acl and
> >> set-file-selinux-context now return t when they succeeded to set the
> >> ACLs resp. SELinux context, and nil otherwise.  Micheal, could you
> >> please adapt the Tramp handlers to this paradigm?  Thanks.
> >
> > Done for ACL. For SELinux, I'll check later.
> 
> SELinux case is also fixed now.

Thank you!