[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 27 Nov 2017 21:26:12 +0100
From: Solar Designer <solar@...nwall.com>
To: oss-security@...ts.openwall.com
Cc: Bram@...lenaar.net
Subject: Re: Security risk of server side text editing ...
Kurt, Scott -
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-1000382
Kurt 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."
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.
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 ones
Thanks, 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. ;-)
Alexander
Powered by blists - more mailing lists
Please check out the
Open Source Software Security Wiki, which is counterpart to this
mailing list.
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
