Re: master 739593d 3/5: Make gnus-copy-file act like copy-file etc.

Re: master 739593d 3/5: Make gnus-copy-file act like copy-file etc.

[Katsumi Yamaoka]

On Mon, 11 Sep 2017 01:31:30 -0400, Paul Eggert wrote:
> branch: master
> commit 739593d68742f45e4e35dfc99573c47a5031b646
> Author: Paul Eggert <eggert@cs.ucla.edu>
> Commit: Paul Eggert <eggert@cs.ucla.edu>

>     Make gnus-copy-file act like copy-file etc.

>     * etc/NEWS: Mention this.
>     * lisp/gnus/gnus-util.el (gnus-copy-file): Treat the destination
>     as special only if it is a directory name.

Er, isn't it necessary to make `copy-file' allow a destination
directory for the second argument NEWNAME?

M-x gnus-copy-file RET ~/foo RET /tmp RET
 => File already exists: /tmp

Re: master 739593d 3/5: Make gnus-copy-file act like copy-file etc.

[Ken Brown]

On 9/11/2017 7:14 PM, Katsumi Yamaoka wrote:
> On Mon, 11 Sep 2017 01:31:30 -0400, Paul Eggert wrote:
>> branch: master
>> commit 739593d68742f45e4e35dfc99573c47a5031b646
>> Author: Paul Eggert <eggert@cs.ucla.edu>
>> Commit: Paul Eggert <eggert@cs.ucla.edu>
> 
>>      Make gnus-copy-file act like copy-file etc.
> 
>>      * etc/NEWS: Mention this.
>>      * lisp/gnus/gnus-util.el (gnus-copy-file): Treat the destination
>>      as special only if it is a directory name.
> 
> Er, isn't it necessary to make `copy-file' allow a destination
> directory for the second argument NEWNAME?

It has to be a directory name, not just a directory.  See etc/NEWS, "** 
Several functions that create or rename files now treat their...".

> M-x gnus-copy-file RET ~/foo RET /tmp RET
>   => File already exists: /tmp

Try /tmp/ instead of /tmp.

Ken

Re: master 739593d 3/5: Make gnus-copy-file act like copy-file etc.

[Katsumi Yamaoka]

On Mon, 11 Sep 2017 22:12:03 -0400, Ken Brown wrote:
> On 9/11/2017 7:14 PM, Katsumi Yamaoka wrote:
>> On Mon, 11 Sep 2017 01:31:30 -0400, Paul Eggert wrote:
>>>      Make gnus-copy-file act like copy-file etc.

>> Er, isn't it necessary to make `copy-file' allow a destination
>> directory for the second argument NEWNAME?

> It has to be a directory name, not just a directory.  See etc/NEWS,
> "** Several functions that create or rename files now treat their...".

>> M-x gnus-copy-file RET ~/foo RET /tmp RET
>>   => File already exists: /tmp

> Try /tmp/ instead of /tmp.

I see.  Thanks.
But it might have to be documented in also the Gnus manual, as
`gnus-summary-save-article' that uses the function doesn't
specify a destination and a user has to enter the directory name.
IMHO, reverting the change would be better.

Regards,

Re: master 739593d 3/5: Make gnus-copy-file act like copy-file etc.

[Paul Eggert]

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

On 09/11/2017 07:33 PM, Katsumi Yamaoka wrote:
> But it might have to be documented in also the Gnus manual, as
> `gnus-summary-save-article' that uses the function doesn't
> specify a destination and a user has to enter the directory name.

The special behavior with gnus-summary-save-article and directories (or 
directory names) was not documented. Although I'm not a Gnus expert I 
took a stab at documenting the current behavior by installing the 
attached patch. If we revert the Gnus change (or make further changes) 
then this part of the manual will need to be revisited.

> IMHO, reverting the change would be better.

Does file name completion (TAB) cause '/' to be appended to the 
destination directory when you're using Gnus to save files? That should 
reduce the problem, though I admit it does not eliminate it.

The change was prompted by security concerns where attackers can hijack 
your data by planting symlinks at the wrong time in shared areas under 
/tmp and suchlike. We could complicate Emacs somewhat to make it more 
backwards-compatible in common cases such as yours; Eli is suggesting 
something along these lines. The tradeoff between convenience and 
security and maintainability is not obvious, though, and I worry that 
the suggestion as-is will reintroduce security problems for interactive 
users. Perhaps we can think of something better.


[-- Attachment #2: 0001-Tweak-Gnus-doc-re-gnus-copy-file.patch --]
[-- Type: text/x-patch, Size: 1117 bytes --]

From f5671c9c2d20e538f0f0eeaffbb37e592b2d232a Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 12 Sep 2017 12:15:22 -0700
Subject: [PATCH] Tweak Gnus doc re gnus-copy-file

* doc/misc/gnus.texi (Saving Articles):
Document behavior with directory name targets (Bug#27986).
Problem reported by Katsumi Yamaoka in:
http://lists.gnu.org/archive/html/emacs-devel/2017-09/msg00216.html
---
 doc/misc/gnus.texi | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/doc/misc/gnus.texi b/doc/misc/gnus.texi
index 3e9b48ec65..b002f5dea7 100644
--- a/doc/misc/gnus.texi
+++ b/doc/misc/gnus.texi
@@ -7759,7 +7759,9 @@ Saving Articles
 approach (uudecoding, unsharing) you should use @code{gnus-uu}
 (@pxref{Decoding Articles}).
 
-For the commands listed here, the target is a file.  If you want to
+For the commands listed here, the target is a file.
+A directory name (ending in @samp{/}) causes the target
+to be a file under that directory.  If you want to
 save to a group, see the @kbd{B c} (@code{gnus-summary-copy-article})
 command (@pxref{Mail Group Commands}).
 
-- 
2.13.5

Re: master 739593d 3/5: Make gnus-copy-file act like copy-file etc.

[Stefan Monnier]

>> But it might have to be documented in also the Gnus manual, as
>> `gnus-summary-save-article' that uses the function doesn't
>> specify a destination and a user has to enter the directory name.

> The special behavior with gnus-summary-save-article and directories (or
> directory names) was not documented. Although I'm not a Gnus expert
> I took a stab at documenting the current behavior by installing the attached
> patch. If we revert the Gnus change (or make further changes) then this part
> of the manual will need to be revisited.

Maybe the interactive behavior could look as follows:

- when copying/renaming to "/foo/dir", first do it as we do now.
- if it signals an error because "/foo/dir" already exists, check if
  it's a directory.
- if it's a directory, ask the user to confirm that he meant "/foo/dir/"

That would let the user learn that he should have written "/foo/dir/" in
the first place, and so should slowly get him to avoid the prompt.


        Stefan

Re: master 739593d 3/5: Make gnus-copy-file act like copy-file etc.

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Thu, 14 Sep 2017 00:17:06 -0400
> 
> - when copying/renaming to "/foo/dir", first do it as we do now.
> - if it signals an error because "/foo/dir" already exists, check if
>   it's a directory.
> - if it's a directory, ask the user to confirm that he meant "/foo/dir/"

IMO, this would just add insult to injury.

The previous interactive behavior was consistent with 'cp' and 'mv',
so it should be left alone.  We should only fix the noninteractive
behavior, where the danger of some external agent guessing what Emacs
will do is significantly higher.

Re: master 739593d 3/5: Make gnus-copy-file act like copy-file etc.

[Paul Eggert]

Stefan's proposal would work, and I volunteer to implement it if there is 
consensus (which there does not seem to be yet). It's secure enough for 
practical use and it is not intrusive.

Although perhaps someone can come up with a better idea, "Just do interactive 
renames the way we were doing it" is not better, as it would leave interactive 
users vulnerable to the attack. Although users may like to think that they are 
"unpredictable" and are thus relatively invulnerable to the attack, they're not 
really that unpredictable.

Re: master 739593d 3/5: Make gnus-copy-file act like copy-file etc.

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Thu, 14 Sep 2017 10:59:44 -0700
> 
> Although perhaps someone can come up with a better idea, "Just do interactive 
> renames the way we were doing it" is not better, as it would leave interactive 
> users vulnerable to the attack. Although users may like to think that they are 
> "unpredictable" and are thus relatively invulnerable to the attack, they're not 
> really that unpredictable.

Maybe you should again describe the attack scenario.  Last time you
did, it was pretty clear to me that no bot or person can reasonably
know in advance what file or directory the user will copy/rename.

Re: master 739593d 3/5: Make gnus-copy-file act like copy-file etc.

[Paul Eggert]

Eli Zaretskii wrote:
> no bot or person can reasonably
> know in advance what file or directory the user will copy/rename.

Sure they can. Here's a scenario off the top of my head. A sysadmin uses Emacs 
to examine files, and has the bad (but all-too-common) habit of copying files to 
/tmp and examining the copies so that he doesn't mistakenly change the 
originals. A malicious user asks the sysadmin to take a look at "problems" in 
the user's ~/.ssh/known_hosts file. The sysadmin does this:

M-x copy-file RET ~malicious/.ssh/known_hosts RET /tmp/known_hosts RET

but it doesn't seem to work (there's no file in /tmp afterwards), so the tired 
sysadmin figures he mistyped the command, does the copy-file again and this time 
it works so he diagnoses the "problems". Because of the Emacs security bug with 
destination directories, the malicious user has now taken over the sysadmin's 
personal and private known_hosts file.

The scenario works partly because the attacker knows the habits of the victim. 
Such habits are often easy to discover.

One possible solution to all this is to tell ones' sysadmins "Do not use Emacs: 
it has too many security holes". But I'm fond of Emacs, and would rather that 
sysadmins could trust it to do their work.

Re: master 739593d 3/5: Make gnus-copy-file act like copy-file etc.

[Eli Zaretskii]

> Cc: monnier@iro.umontreal.ca, emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Thu, 14 Sep 2017 21:04:16 -0700
> 
> Eli Zaretskii wrote:
> > no bot or person can reasonably
> > know in advance what file or directory the user will copy/rename.
> 
> Sure they can. Here's a scenario off the top of my head. A sysadmin uses Emacs 
> to examine files, and has the bad (but all-too-common) habit of copying files to 
> /tmp and examining the copies so that he doesn't mistakenly change the 
> originals. A malicious user asks the sysadmin to take a look at "problems" in 
> the user's ~/.ssh/known_hosts file. The sysadmin does this:
> 
> M-x copy-file RET ~malicious/.ssh/known_hosts RET /tmp/known_hosts RET
> 
> but it doesn't seem to work (there's no file in /tmp afterwards), so the tired 
> sysadmin figures he mistyped the command, does the copy-file again and this time 
> it works so he diagnoses the "problems". Because of the Emacs security bug with 
> destination directories, the malicious user has now taken over the sysadmin's 
> personal and private known_hosts file.
> 
> The scenario works partly because the attacker knows the habits of the victim. 
> Such habits are often easy to discover.
> 
> One possible solution to all this is to tell ones' sysadmins "Do not use Emacs: 
> it has too many security holes". But I'm fond of Emacs, and would rather that 
> sysadmins could trust it to do their work.

The same sysadmin could do the same with Coreutils, with the same
results, right?

Sorry, Paul, this is not going to fly with me.  I still think that
changing the interactive behavior in these cases is wrong, and we
shouldn't do it.

Re: master 739593d 3/5: Make gnus-copy-file act like copy-file etc.

[Eli Zaretskii]

> From: Ken Brown <kbrown@cornell.edu>
> Date: Mon, 11 Sep 2017 22:12:03 -0400
> 
> > M-x gnus-copy-file RET ~/foo RET /tmp RET
> >   => File already exists: /tmp
> 
> Try /tmp/ instead of /tmp.

I think this is just the first of such reports.  We should leave the
interactive behavior alone, and change only the non-interactive one.

Re: master 739593d 3/5: Make gnus-copy-file act like copy-file etc.

[Lars Ingebrigtsen]

Katsumi Yamaoka <yamaoka@jpl.org> writes:

>>     * etc/NEWS: Mention this.
>>     * lisp/gnus/gnus-util.el (gnus-copy-file): Treat the destination
>>     as special only if it is a directory name.
>
> Er, isn't it necessary to make `copy-file' allow a destination
> directory for the second argument NEWNAME?
>
> M-x gnus-copy-file RET ~/foo RET /tmp RET
>  => File already exists: /tmp

Breaking this ages-old (and convenient) behaviour doesn't seem like a
good idea to me either.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master 739593d 3/5: Make gnus-copy-file act like copy-file etc.

[Paul Eggert]

On 09/13/2017 12:33 PM, Lars Ingebrigtsen wrote:
> Breaking this ages-old (and convenient) behaviour doesn't seem like a
> good idea to me either.

It's an area where convenience and security collide. I'm open to 
suggestions to restoring convenience, but would rather not see security 
holes reintroduced. In mu use I haven't found significant loss of 
convenience (I normally use tab completion, and so type the same 
characters as before), but of course my experience is not everybody's 
(among other things, I don't use Gnus). So it'd be helpful to know 
typical use cases that are causing significant problems.

As a simple and dumb example, if people have "/ t m p RET" hardwired 
into their fingers, we could make an exception for "/tmp" without losing 
security. On real-world hosts the security problem can occur for 
subsidiary directories of /tmp, but not for /tmp itself.

Re: master 739593d 3/5: Make gnus-copy-file act like copy-file etc.

[Lars Ingebrigtsen]

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

> It's an area where convenience and security collide.

What are the security implications of writing the file to the directory
if the user (interactively) types in that directory name?

> As a simple and dumb example, if people have "/ t m p RET" hardwired
> into their fingers, we could make an exception for "/tmp" without
> losing security. On real-world hosts the security problem can occur
> for subsidiary directories of /tmp, but not for /tmp itself.

The user can type anything, like "/home/larsi" and "/var/tmp" and the
behaviour should be the same across directories.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master 739593d 3/5: Make gnus-copy-file act like copy-file etc.

[Paul Eggert]

On 09/13/2017 01:11 PM, Lars Ingebrigtsen wrote:
> What are the security implications of writing the file to the directory
> if the user (interactively) types in that directory name?

If the attacker knows what the user is up to (and this can be guessed 
often enough by looking at what Emacs has done to the file system 
recently), the attacker can hijack the rename. For example, if you type 
'M-x rename-file RET abc RET /tmp/def RET', the attacker can create a 
symlink /tmp/def to a victim directory D so that the file abc is moved 
to D/abc rather than to its intended location /tmp/def.

This attack can happen only when the destination's parent directory 
(/tmp in the above example) is writable to the attacker. So we could 
bring back support for interactive renames to destination directories 
whose parents are writable only by self or root. (Most likely the actual 
rule will be more complicated than this, but the basic idea will work.) 
This would lessen the scope of the change, albeit at the cost of 
complication of the documentation and implementation.

> The user can type anything, like "/home/larsi" and "/var/tmp" and the
> behaviour should be the same across directories.
/home/larsi and /var/tmp should both be safe destinations in the above 
sense, as their parents aren't writable to others. So they would both 
work without the trailing slash, under the above proposal.

I'd rather leave it alone as it's simpler and easier to describe the way 
it is, and I type the same keystrokes as before since I normally use tab 
completion which adds a trailing / which gives me visual feedback that 
it's a move into a directory which is a win. But I can be talked into 
something like the above if it'd be valuable for others' interactive 
use. (Do you type "/ h o m e / l a r s i" by hand a lot? :-)

Re: master 739593d 3/5: Make gnus-copy-file act like copy-file etc.

[Lars Ingebrigtsen]

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

> If the attacker knows what the user is up to (and this can be guessed
> often enough by looking at what Emacs has done to the file system
> recently), the attacker can hijack the rename. For example, if you
> type 'M-x rename-file RET abc RET /tmp/def RET', the attacker can
> create a symlink /tmp/def to a victim directory D so that the file abc
> is moved to D/abc rather than to its intended location /tmp/def.

Hm...  I see...

> This attack can happen only when the destination's parent directory
> (/tmp in the above example) is writable to the attacker. So we could
> bring back support for interactive renames to destination directories
> whose parents are writable only by self or root.

The attack surface you're trying to cover is when the user is writing a
file to a world-writable directory that contains a symlink that has
exactly the same name as the file you're trying to write?

Altering Emacs' way of renaming/copying/saving files everywhere for just
this single (and extremely unlikely) attack seems rather misguided, in
my opinion.  If we want to protect against that case, then we should
instead revert all the changes you've made to these functions and
introduce a new

(barf-if-you're-writing-a-file-with-the-same-name-to-a-symlink-in-a-world-writable-directory FILE)

function that we can slap into the affected functions and leave the
interactive parts working as they have always.

These days nobody lives on shared computers, anyway, so crippling these
common interactive commands to protect against non-existent people
making symlinks in /tmp does not seem like a good idea to me.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master 739593d 3/5: Make gnus-copy-file act like copy-file etc.

[Paul Eggert]

On 09/13/2017 02:10 PM, Lars Ingebrigtsen wrote:
> The attack surface you're trying to cover is when the user is writing a
> file to a world-writable directory that contains a symlink that has
> exactly the same name as the file you're trying to write?

More generally, it's when the attacker can write the destination's 
parent directory. The parent need not be world-writable, and there 
doesn't need to be a symlink there already.

> (barf-if-you're-writing-a-file-with-the-same-name-to-a-symlink-in-a-world-writable-directory FILE)
>
> function that we can slap into the affected functions and leave the
> interactive parts working as they have always.

I'm not following. The attack works against both interactive and 
non-interactive use. If we try to support the old behavior then any code 
that Emacs executes will be vulnerable to the attack, because the 
underlying system calls do not let Emacs test for safety and then rename 
the file before the attacker can act.

Eli is most concerned about interactive use, as am I. I reviewed 
noninteractive calls and fixed the few glitches that I found, so they 
should be OK (though of course I could have made mistakes in my review). 
It's the interactive use that is more of the question mark: it's what 
started this thread.

Another possibility would be to add a variable 'trust-other-users' that 
the user can set to indicate that other users on the computer are 
trusted, and to support the old interactive behavior if this variable is 
set. This approach is easier to document and implement than my previous 
suggestion; the downside is that the user must hassle with the new 
variable, and there will likely be configuration errors when users copy 
.emacs files from nonshared to shared computers. I thought of this long 
ago and rejected it for that reason and still don't like the idea much, 
but if the consensus is to go this direction then I'll volunteer to 
implement it.

> These days nobody lives on shared computers, anyway

I regularly use Emacs on computers shared with users I don't fully 
trust. I've done so every day this week so far. Although I use Emacs 
more often on standalone machines, the shared-machine use case is still 
alive and kicking.

Re: master 739593d 3/5: Make gnus-copy-file act like copy-file etc.

[Lars Ingebrigtsen]

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

> On 09/13/2017 02:10 PM, Lars Ingebrigtsen wrote:
>> The attack surface you're trying to cover is when the user is writing a
>> file to a world-writable directory that contains a symlink that has
>> exactly the same name as the file you're trying to write?
>
> More generally, it's when the attacker can write the destination's
> parent directory. The parent need not be world-writable, and there
> doesn't need to be a symlink there already.

Hm...  then I'm not sure I understand your first explanation.  Your
example was /tmp (word-writable) with a symlink "foo" in /tmp that
points to somewhere the attacker can't write.  So for instance /tmp/foo
symlinks to /home/victim/.ssh/authorized_keys and the victim says
`M-x copy-file RET something RET /tmp/foo RET' *bang*.

But was there a different scenario you were thinking about?

(Timing these attacks is always fun, though, but the barfing can be
postponed until the actual write, I would guess...)

> Eli is most concerned about interactive use, as am I.

Sounds like the opposite going by Eli's last email.  :-)

>> These days nobody lives on shared computers, anyway
>
> I regularly use Emacs on computers shared with users I don't fully
> trust. I've done so every day this week so far. Although I use Emacs
> more often on standalone machines, the shared-machine use case is
> still alive and kicking.

Yeah, "nobody" is an exaggeration, and we can't disregard people on
shared machines.  But the ratio of users we're servicing should also
inform our approach to the severity of how much we're crippling DWIM
functionality for (theoretical) safety reasons.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master 739593d 3/5: Make gnus-copy-file act like copy-file etc.

[Eli Zaretskii]

> From: Paul Eggert <eggert@cs.ucla.edu>
> Date: Wed, 13 Sep 2017 13:07:47 -0700
> Cc: emacs-devel@gnu.org
> 
> On 09/13/2017 12:33 PM, Lars Ingebrigtsen wrote:
> > Breaking this ages-old (and convenient) behaviour doesn't seem like a
> > good idea to me either.
> 
> It's an area where convenience and security collide. I'm open to 
> suggestions to restoring convenience, but would rather not see security 
> holes reintroduced.

I thought we agreed that the security issue is less of a concern in
interactive use of these functions, because interactive invocations
are less predictable, whereas the attack scenario in this case
requires that the attacker know in advance that Emacs will issue a
certain system call.

Based on that, I believe we agreed to leave the interactive behavior
alone, and only change the non-interactive one.