Re: Security risk of server side text editing ...

[Solar Designer <solar () openwall com>]

On Fri, Nov 17, 2017 at 11:35:15AM +0100, Bram Moolenaar wrote:
> Please check out patch 8.0.1300.

Thanks.  Personally, I don't have much to add.  This continues to do
what I find are weird and wrong things, so any implementation issues are
secondary to that.  I suppose you have some rationale for preserving the
old behavior of propagating the edited file's permissions onto related
temporary files, but I'm unaware of good reasons for that.

If it's about users' collaboration, then I don't see a good reason for
other users in the group, even if they could access the original file
via group permissions, to also have access to recovery and backup files.

As to the patch itself, aside from it propagating the possibly unsafe
permissions on purpose (I mean unsafe such as in Hanno's original
example, but also applying to backup files), it's also risky in
temporarily setting umask to 0.  On some systems, this could mean libc
or the kernel creating files with unsafe permissions if anything goes
very wrong during this time - e.g., a coredump.  Checking st_ino is OK
as a hardening measure, but might not always be sufficient: inode number
reuse is possible if the original file could have been deleted.
I suppose st_dev is not checked because of the use of O_NOFOLLOW, but I
guess Vim can be built on systems without working O_NOFOLLOW as well?

In case anyone wants to review the patch for real, I've attached it to
this message, and here it is on GitHub (for expanding of the context):

https://github.com/vim/vim/commit/cd142e3369db8888163a511dbe9907bcd138829c

Alexander

Attachment: 8.0.1300
Description: