oss-sec mailing list archives
Re: Security risk of server side text editing ...
From: Bram Moolenaar <Bram () moolenaar net>
Date: Tue, 28 Nov 2017 14:19:58 +0100
Solar Designer wrote:
On Mon, Nov 27, 2017 at 02:10:54PM -0500, Scott Court wrote:Here's the summary you asked for. As far as I've been able to tell, there are three vulnerabilities being discussed here:Thank you for trying to do the right thing, but I'm concerned that with the "multiple vulnerabilities in Vim" approach we can identify lots of them and "fix" lots of them yet not achieve a sensible goal - in fact, we already started down that path.1. CVE-2017-1000382Kurt assigned this one to: "Please use CVE-2017-1000382 for VIM version 8.0.1187 (and other versions most likely) ignores umask when creating a swap file (\"[ORIGINAL_FILENAME].swp\") resulting in files that may be world readable or otherwise accessible in ways not intended by the user running the vi binary."
I still haven't found any reason why umask would apply to intermediate or temporary files. It is intended to be used for newly created files. Setting the access of the swap file equal to the edited file is the right thing to do.
While ignoring of umask might be the CVE-worthy issue here, in practice most of the files Hanno found were probably from systems that had most distros' default non-root user umask of 022 or 002 anyway. So fixing this CVE as worded does little to address Hanno's findings.This vulnerability was discovered by Hanno Bock. When editing a text file in Vim, a .swp file is created in the same directory (if you edit "foo", the swap file will be ".foo.swp"). Hanno pointed out that this could create a security vulnerability on PHP enabled webservers as follows: If a user goes to edit a .php file in the public_html directory (say "foo.php"), a swap file will be created in the public_html directory called ".foo.php.swp". This then exposes the contents of the PHP script foo.php to the world. All someone has to do is go to "http://example.com/.foo.php.swp" and he can view the .swp file which contains the contents of the original foo.php file. Hanno pointed out that this causes a problem with Wordpress sites if the site administrator edits the wp-config.php file in Vim: he exposes all of the database credentials. This is made worse if Vim crashes while he is editing it as then the .wp-config.php.swp file sticks around. He claims he has found 750 websites that are vulnerable to this.This is a good summary of the actual issue that we should address, but the CVE is only partially related to it. Fixing the CVE (as described) means honoring the umask, but it would do little to reduce the number of vulnerable sites Hanno would find. Improving Vim to always use 0600 might or might not be considered a fix for the CVE (maybe 0600 & ~umask would be, even if anything stricter than 0600 breaks the functionality), but it would do much more to address Hanno's findings (only leaving out the special case of the web server running as the same (pseudo-)user who edits the files, which I expect is less common than editing with umask 022 or 002). So let's focus on what actually matters rather than on what fits a CVE.2. Vim .swp file group (Doesn't have a CVE ID) This vulnerability was discovered by me. When Vim creates a .swp file, the .swp file is created with the owner and group set to the editor and editor's primary group respectively. The .swp file is the set to the same permissions as the original file (i.e. chmod 640). This creates a security vulnerability when the editor's primary group is not the same as the original file's group. For example, say the root user's primary group is "users", which every user is a member of. If root goes to edit /etc/shadow, the /etc/.shadow.swp file is created with permissions 640 and user:group set to root:users. The original /etc/shadow file had user:group set to root:shadow though; this now exposes the /etc/shadow file (which mind you contains hashes of every user's password) to every user on the system. Originally, I thought this was an extension of CVE-2017-1000382 so I didn't bother trying to get a CVE ID for it; however, upon looking at it for a second time, it seems that this is indeed a different vulnerability. It is possible to patch this vulnerability without patching CVE-2017-1000382.I agree this is a separate issue from ignoring the umask, and is probably a CVE-worthy vulnerability as well (if the first CVE is in fact limited to the umask), but for practical purposes there's just one thing to change in response to both issues: make those files 0600.3. Vim.tiny race condition (Doesn't have a CVE ID as far as I know) I'm not quite sure who discovered this vulnerability (I don't use or follow vim.tiny); however, it has been discussed on here so I will include my limited knowledge of it for completeness sake. This is a race condition in which a world writable SUID binary is temporarily created. This could (or course) theoretically allow an arbitrary user to write to that binary and execute arbitrary code as root; however, there is debate as to whether or not doing this is actually feasible.This not a vulnerability in Vim, and is not CVE-worthy. Most programs, text editors included, are unsafe to use on pathnames in untrusted directories, period. We might want to harden Vim to make it less unsafe when misused like that, but calling that a vulnerability and those changes a fix would encourage the misuses and further misunderstandings. If I understand correctly, the specific example posted by Roman relied on a file being replaced with a symlink, so this falls in the above category - misuse of Vim in an untrusted directory.
I have fixed this anyway, patch 8.0.1300.
OTOH, as an exception, Vim can and should be made safe when editing a file with an untrusted owner if that file (the specific hard link to it corresponding to the pathname being edited) is located in a trusted directory (including all of its parent directories) - e.g., /var/run/foo where only foo itself is under a potential attacker's control. If Vim would sometimes copy the SUID/SGID bits from such file to another (temporary) file that does not yet have the user and group ownership also already copied to it (which would have already been racy as well), then that's a concern and is something that should be fixed - again, by simply setting those files' permissions to 0600 (no need for any fancy logic). I don't know, and am not too interested, whether such an issue currently exists (I think it does not, as I saw code masking the permissions to 0777), nor whether it's CVE-worthy (it might be if it exists). I care more about the trivial proper change we should make in response to all of these non-misuse issues at once, without needing to enumerate and categorize them. If upstream approaches this differently, distros should throw the fancy logic out and hard-code 0600.I believe these are the three big onesThanks, but I think enumerating and categorizing them is a distraction, except to the extent necessary for us to get on the same page and make the trivial change. ;-)
-- I started out with nothing, and I still have most of it. -- Michael Davis -- "Tonight Show" /// Bram Moolenaar -- Bram () 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 ///
Current thread:
- Re: Security risk of server side text editing ... Bram Moolenaar (Nov 17)
- Re: Security risk of server side text editing ... Solar Designer (Nov 22)
- Re: Re: Security risk of server side text editing ... Kurt Seifried (Nov 22)
- Re: Re: Security risk of server side text editing ... Scott Court (Nov 27)
- Re: Security risk of server side text editing ... Solar Designer (Nov 27)
- Re: Security risk of server side text editing ... Bram Moolenaar (Nov 28)
- Re: Re: Security risk of server side text editing ... Simon McVittie (Nov 27)
- Re: Re: Security risk of server side text editing ... Bram Moolenaar (Nov 28)
- Re: Re: Security risk of server side text editing ... Leonid Isaev (Nov 28)
- Re: Re: Security risk of server side text editing ... Scott Court (Dec 01)
- Re: Re: Security risk of server side text editing ... Kurt Seifried (Nov 22)
- Re: Security risk of server side text editing ... Solar Designer (Nov 22)