New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
jsegitz opened this Issue Oct 25, 2018 · 5 comments

Comments

Projects
None yet
3 participants
@jsegitz

jsegitz commented Oct 25, 2018

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 added a commit to pqarmitage/keepalived that referenced this issue Nov 1, 2018

Add command line and configuration option to set umask
Issue acassen#1048 identified that files created by keepalived are created
with mode 0666. This commit changes the default to 0644, and also
allows the umask to be specified in the configuration or as a command
line option.

Signed-off-by: Quentin Armitage <quentin@armitage.org.uk>

pqarmitage added a commit to pqarmitage/keepalived that referenced this issue Nov 1, 2018

When opening files for write, ensure they aren't symbolic links
Issue acassen#1048 identified that if, for example, a non privileged user
created a symbolic link from /etc/keepalvied.data to /etc/passwd,
writing to /etc/keepalived.data (which could be invoked via DBus)
would cause /etc/passwd to be overwritten.

This commit stops keepalived writing to pathnames where the ultimate
component is a symbolic link, by setting O_NOFOLLOW whenever opening
a file for writing.

This might break some setups, where, for example, /etc/keepalived.data
was a symbolic link to /home/fred/keepalived.data. If this was the case,
instead create a symbolic link from /home/fred/keepalived.data to
/tmp/keepalived.data, so that the file is still accessible via
/home/fred/keepalived.data.

There doesn't appear to be a way around this backward incompatibility,
since even checking if the pathname is a symbolic link prior to opening
for writing would create a race condition.

Signed-off-by: Quentin Armitage <quentin@armitage.org.uk>
@pqarmitage

This comment has been minimized.

Collaborator

pqarmitage commented Nov 1, 2018

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.

@pqarmitage pqarmitage closed this Nov 2, 2018

@jsegitz

This comment has been minimized.

jsegitz commented Nov 8, 2018

Thank you for the quick and complete fix. I asked MITRE for a CVE and they assigned three:
CVE-2018-19044 for 04f2d32
CVE-2018-19045 for c6247a9, 5241e4d
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 pqarmitage reopened this Nov 8, 2018

pqarmitage added a commit to pqarmitage/keepalived that referenced this issue Nov 12, 2018

When opening files for write, ensure files can only be read by root
Issue acassen#1048 referred to CVE-2018-19046 regarding files used for
debugging purposes could potentially be read by non root users.

This commit ensures that such log files cannot be opened by non root
users.

Signed-off-by: Quentin Armitage <quentin@armitage.org.uk>
@pqarmitage

This comment has been minimized.

Collaborator

pqarmitage commented Nov 12, 2018

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

This comment has been minimized.

jsegitz commented Nov 12, 2018

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.

@jsegitz jsegitz closed this Nov 12, 2018

@attritionorg

This comment has been minimized.

attritionorg commented Nov 13, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment