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.
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.
>From df885c9be346c3ff6de635655e9c079faf1aec68 Mon Sep 17 00:00:00 2001
Message-Id:
<df885c9be346c3ff6de635655e9c079faf1aec68.1510622481.git.si...@ruderich.org>
From: Simon Ruderich <[email protected]>
Date: Tue, 7 Nov 2017 13:57:35 +0100
Subject: [PATCH 1/2] viminfo: create fallback tempfile with restrictive umask
If the initial tempfile creation fails, possibly during a race condition
with two parallel vims writing the tempfile, then a fallback code is
used. However while the normal code uses a restrictive umask (or the
mode of the existing viminfo), the fallback code uses the default umask
of the user. This can result in a viminfo which is readable by all
users possibly leaking sensitive information.
---
src/ex_cmds.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/src/ex_cmds.c b/src/ex_cmds.c
index f86e141fc..2b19928b2 100644
--- a/src/ex_cmds.c
+++ b/src/ex_cmds.c
@@ -2013,8 +2013,15 @@ write_viminfo(char_u *file, int forceit)
if (fp_out == NULL)
{
vim_free(tempname);
- if ((tempname = vim_tempname('o', TRUE)) != NULL)
+ if ((tempname = vim_tempname('o', TRUE)) != NULL) {
+#if defined(UNIX) || defined(VMS)
+ umask_save = umask(077);
+#endif
fp_out = mch_fopen((char *)tempname, WRITEBIN);
+#if defined(UNIX) || defined(VMS)
+ (void)umask(umask_save);
+#endif
+ }
}
#if defined(UNIX) && defined(HAVE_FCHOWN)
--
2.15.0
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.
