acassen / keepalived

Keepalived
https://www.keepalived.org
GNU General Public License v2.0
4.02k stars 736 forks source link

Dbus interface allows overwriting arbitrary files and insecure permissions are used #1048

Closed jsegitz closed 6 years ago

jsegitz commented 6 years ago

Users can overwrite arbitrary files if PrintData or PrintStats is invoked and fs.protected_symlinks is 0

Reproducer:

As user:

johannes@linux-v0tl:~> ls -lah /passwd
-rw-r--r-- 1 root root 2.9K Oct 25 16:47 /passwd
johannes@linux-v0tl:~> head -n 1 /passwd
at:x:25:25:Batch jobs daemon:/var/spool/atjobs:/bin/bash
johannes@linux-v0tl:~> ln -s /passwd /tmp/keepalived.data

As root:

# fs.protected_symlinks=0
# busctl call org.keepalived.Vrrp1 /org/keepalived/Vrrp1/Vrrp org.keepalived.Vrrp1.Vrrp  PrintData
# head -n1 /passwd
------< Global definitions >------

/tmp/keepalived.data and /tmp/keepalived.stats is also created mode 666, so information can leak/be modified to/by unprivileged users

pqarmitage commented 6 years ago

This is a good catch.

Commit c6247a9 (and 5241e4d) change the default umask to 022 (i.e. removing group and other write permission) and allow the umask both to be specified via a command line option --umask=, and in the configuration (global_defs option umask, which can be set symbolically). This seeks to allow backward compatibility as far as possible, while also allowing users who want more security to be able to limit the file access permissions.

Commit 04f2d32 sets the O_NOFOLLOW flag on all files opened for writing (other than files under /proc), so that the filename part of the pathname cannot be a symbolic link. This is in considerably more places than the examples identified in this issue, so the impact is potentially quite wide ranging.

jsegitz commented 6 years ago

Thank you for the quick and complete fix. I asked MITRE for a CVE and they assigned three: CVE-2018-19044 for https://github.com/acassen/keepalived/commit/04f2d32871bb3b11d7dc024039952f2fe2750306 CVE-2018-19045 for https://github.com/acassen/keepalived/commit/c6247a9ef2c7b33244ab1d3aa5d629ec49f0a067, https://github.com/acassen/keepalived/commit/5241e4d7b177d0b6f073cfc9ed5444bf51ec89d6 CVE-2018-19046 for the case, that a user already created /tmp/keepalived.data or /tmp/keepalived.stats with mode 666, so it's not covered by the umask fix and should still be fixed

pqarmitage commented 6 years ago

Commits ac8e2ef, 26c8d63 and 17f9441 should fully resolve CVE-2018-19046.

How do we get the information at MITRE regarding these CVEs updates to state that they are all now resolved?

jsegitz commented 6 years ago

First of all thank you for your quick reaction.

The CVEs are used mainly for tracking. MITRE will not do that, but e.g. SUSE will use these ids to track the state of our packages. Other distributions do the same and provides data to users so they can check if their installation is already fixed. For you this should be done now.

attritionorg commented 6 years ago

Some other vulnerability databases also update entries with this type of information so it is appreciated!