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.
This can't happen, vim_tempname() returns a unique name.
> 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).
No, that is not possible.
> 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.
Yes, that's the missing part.
--
Engineers are always delighted to share wisdom, even in areas in which they
have no experience whatsoever. Their logic provides them with inherent
insight into any field of expertise. This can be a problem when dealing with
the illogical people who believe that knowledge can only be derived through
experience.
(Scott Adams - The Dilbert principle)
/// Bram Moolenaar -- [email protected] -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
--
--
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.
