On Tue, Nov 14, 2017 at 11:20:03PM +0100, Bram Moolenaar wrote:
> Simon Ruderich wrote:
>> Yes, the corner case is that two (ore more) Vims at the same time
>> concurrently try to write the viminfo. This can for example
>> happen if you logout from your X session but have multiple Vims
>> running which receive a SIGHUP at the same time and while dying
>> will update the viminfo.
>
> OK, I assumed this was really never happening. But indeed, if there are
> multiple Vim processes that all get the same signal at the same time,
> it's much more likely to happen.
>
> The usual solution is to not use stat() and then open(), but use open()
> with the right flags to fail when the file exists, and deal with the
> error. Best is to do both, because some systems don't handle O_EXCL
> properly (see the code for shortname above). And overwriting the
> original file is pretty bad.
Yes, O_EXCL is the best solution to prevent overwrites in a race,
and the code already does this in a one place, but not in others.
The first offender is here:
fp_in = mch_fopen((char *)fname, READBIN);
if (fp_in == NULL)
{
/* if it does exist, but we can't read it, don't try writing */
if (mch_stat((char *)fname, &st_new) == 0)
goto end;
#if defined(UNIX) || defined(VMS)
/*
* For Unix we create the .viminfo non-accessible for others,
* because it may contain text from non-accessible documents.
*/
umask_save = umask(077);
#endif
fp_out = mch_fopen((char *)fname, WRITEBIN);
fname might already exist when we call fopen(3), so open(2) with
O_EXCL should be used.
Assuming VMS has no O_EXCL we can ignore the parts which are
specific to VMS and live with the race there.
The second one is here:
/*
* If we can't create in the same directory, try creating a
* "normal" temp file. This is just an attempt, renaming the
temp
* file might fail as well.
*/
if (fp_out == NULL)
{
vim_free(tempname);
if ((tempname = vim_tempname('o', TRUE)) != NULL)
fp_out = mch_fopen((char *)tempname, WRITEBIN);
}
Although the tempname didn't exist when vim_tempname() checked
it, this may no longer be the case when we write the file.
I agree that these parts should be improved, however that's
unrelated to the issue I reported and which is still unfixed (see
below).
>>>> TL;DR: please ignore this patch and apply "Possible security
>>>> issue: [PATCH] viminfo: create fallback tempfile with restrictive
>>>> umask" (Message-ID:
>>>> <975991f5f00163b45d9ae1bb108a1a0064fa1f09.1510059861.git.si...@ruderich.org>)
>>>> to fix the original race condition which makes the .viminfo
>>>> readable by _all_ users.
>>>
>>> That's already done. But keeping the group bits if possible.
>>
>> No, that's the wrong patch. I'm talking about this one (my
>> original patch). It's required to fix the race condition.
>
> Using the temp file does not have the race condition, since every Vim
> has its own unique temp directory.
It could still be possible that another program writes the file
in this temporary directory (so O_EXCL is still a good idea
here).
However all that is totally unrelated to the issue the patch
mentioned above tries to fix! (My sentence "It's required to fix
the race condition." wasn't perfectly precise, the patch doesn't
fix the race condition, it only ensures that under the mentioned
race condition the viminfo doesn't get readable by all users.)
The issue is that all code paths writing the viminfo file enforce
strict permissions (only readable by the user unless the old
viminfo already allows broader access) except one (which the
patch corrects). So first the places where the code correctly
enforces the permissions:
#if defined(UNIX) || defined(VMS)
/*
* For Unix we create the .viminfo non-accessible for others,
* because it may contain text from non-accessible documents.
*/
umask_save = umask(077);
#endif
fp_out = mch_fopen((char *)fname, WRITEBIN);
#if defined(UNIX) || defined(VMS)
(void)umask(umask_save);
#endif
Correct permissions by using strict umask.
#ifdef VMS
/* fdopen() fails for some reason */
umask_save = umask(077);
fp_out = mch_fopen((char *)tempname, WRITEBIN);
(void)umask(umask_save);
#else
int fd;
/* Use mch_open() to be able to use O_NOFOLLOW and set file
* protection:
* Unix: same as original file, but strip s-bit. Reset umask to
* avoid it getting in the way.
* Others: r&w for user only. */
# ifdef UNIX
umask_save = umask(0);
fd = mch_open((char *)tempname,
O_CREAT|O_EXTRA|O_EXCL|O_WRONLY|O_NOFOLLOW,
(int)((st_old.st_mode & 0777) |
0600));
(void)umask(umask_save);
# else
fd = mch_open((char *)tempname,
O_CREAT|O_EXTRA|O_EXCL|O_WRONLY|O_NOFOLLOW,
0600);
# endif
Correct permissions either by using umask or the mode field of
open(2).
However the last place which may create the viminfo is the
following code part (described as fallback path in the original
patch description):
/*
* If we can't create in the same directory, try creating a
* "normal" temp file. This is just an attempt, renaming the
temp
* file might fail as well.
*/
if (fp_out == NULL)
{
vim_free(tempname);
if ((tempname = vim_tempname('o', TRUE)) != NULL)
fp_out = mch_fopen((char *)tempname, WRITEBIN);
}
Note that no care is taken to prevent the file from becoming
readable for all users! This is what my patch intends to fix.
Regards
Simon
--
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
--
--
You received this message from the "vim_dev" maillist.
Do not top-post! Type your reply below the text you are replying to.
For more information, visit http://www.vim.org/maillist.php
---
You received this message because you are subscribed to the Google Groups
"vim_dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
