2017-11-14 1:06 GMT+03:00 Bram Moolenaar <[email protected]>: > > Simon Ruderich wrote: > >> On Sat, Nov 11, 2017 at 04:45:37PM +0100, Bram Moolenaar wrote: >> > Simon Ruderich wrote: >> >> Making viminfo readable by other users is most likely not useful. To >> >> prevent information leakage enforce mode 0600. >> >> >> >> The race condition fixed in the last patch could also cause viminfo >> >> files readable by other uses. Enforcing mode 0600 restores the >> >> originally indented permissions. >> > >> > I still don't see how the original problem could occur. >> >> Is there something I can do to explain it more clearly? I thought >> the patch (the original one, not this one which is just a >> follow-up) and its description explain the problem in enough >> detail. If not please tell me what should be explained better. >> >> > If we can't write a test for it, >> >> We can't write a test for it (or at least I don't know how) >> because it's a race condition which cannot be observed most of >> the time. It only happens if two Vim try to write the viminfo >> concurrently and if the one which can't create the .viminfo.tmp >> (and therefore uses the fallback method to create a new tempfile) >> renames its tempfile (which has the wrong permissions) after the >> first Vim, then the permissions are incorrect (readable by _all_ >> users). > > 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?
Should happen if you use Vim for batch processing without bothering to add `-i NONE`, but with `-N` still present. May be not too unlikely if you e.g. are exploring a way to start running tests in parallel: with a help of apps like `screen` this should be possible, but among other things different Vims there use a single file ./viminfo for storing viminfo. (Though I must say that in my example Vim failure was more likely if tests used `-i viminfo -N`.) One suggestion though: if I were using Python I would use EAFP (try:open; except: …), to C that would translate to “use open(), examine errno, use fstat(). Basically this (except that fstat() lacks f) is what Neovim does with ShaDa: it does not stat() before opening, it opens and examines error. Of course, it is a bit easier for us with libuv wrapper translating error codes, Vim would need a (partial) copy of that. (Not that Neovim is entirely free of race conditions there though, I just used different approach for it being less error-prone.) // BTW, tests for such race conditions could possibly be written with a help of a custom FUSE filesystem: if specific order is needed it should be possible to introduce necessary delays here. Though you will need to be very certain that it is worth the effort; I was thinking about such a thing for Neovim, but for now considering that more a waste of time then something really useful. > >> > I don't see a reason to take away the documented >> > feature, that two users (likely to be the same person with two user >> > accounts) can share a viminfo file. This won't be used often, but can >> > be very annoying if it stops working. >> >> I agree, that's why this patch (which doesn't fix the original >> bug but enforces stricter permissions in general and fixes >> .viminfo files which were affected by the bug in the past) is >> just a follow-up and I only sent it to discuss if this use case >> is relevant enough. >> >> As this doesn't seem to be the case, please just ignore this >> patch and only apply the original one. >> >> > If the problem somehow does occur, then the problem will fix itself with >> > the new Vim when it happens again. >> >> No, it won't because the permissions are kept if the .viminfo >> exists. Therefore the .viminfo will stay readable by _all_ users! >> >> > I'll make the check for the group stricter, like what we did with the >> > swap file. >> >> 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. > >> 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. > > -- > The acknowledged parents of reengineering are Michael Hammer and James Champy. > When I say they're the "parents" I don't mean they had sex - and I apologize > for making you think about it. I mean they wrote the best-selling business > book _Reengineering the Corporation_, which was published in 1993. > Businesses flocked to reengineering like frat boys to a drunken > cheerleader. (This analogy wasn't necessary, but I'm trying to get my mind > off that Hammer and Champy thing.) > (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. -- -- 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.
