Simon Ruderich wrote:
> On Mon, Nov 13, 2017 at 11:06:33PM +0100, Bram Moolenaar wrote:
> > If .viminfo.tmp already exists, Vim will try .viminfz.tmp, .viminfy.tmp,
> > etc.
> >
> > if .viminfo.tmp does not exist at first, but shows up in between the
> > stat() and the open() then the user is doing something really weird.
> > Or there is some corner case I can't imagine?
>
> 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.
> Then all Vims will first stat .viminfo.tmp, not find it, but only
> the first will succeed in creating it, this will trigger the if
> in this code:
>
> /*
> * 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)
>
> But here the umask is not changed when creating the file,
> resulting in a viminfo readable by _all_ users.
>
> >> I just saw the patch and I'm not sure what it's supposed to do.
> >> It only enforces the same user/group but doesn't affect any
> >> permissions and therefore can't fix the original issue.
> >
> > It fixes the case where the user's primary group gives more permissions
> > than intended, and the viminfo has been given group read and/or write
> > permission, and somehow has a different group. Lots of conditions, so
> > no surprise nobody ran into this.
>
> Interesting, thanks.
>
> >> 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.
--
At some point in the project somebody will start whining about the need to
determine the project "requirements". This involves interviewing people who
don't know what they want but, curiously, know exactly when they need it.
(Scott Adams - The Dilbert principle)
/// Bram Moolenaar -- b...@moolenaar.net -- 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 vim_dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
