This bug covers two vulnerabilities -- one minor, related to the PID file, and the other major, related to the recursive "chown." For the latter I've marked this private. == PID file manipulation == The logstash-bin init script gives ownership of the PID file directory to the same user that the daemon runs as: for d in "${LS_INSTALL_DIR}/data" "$(dirname "${pidfile}")" "${LS_LOG_DIR}"; do checkpath -d -o "${LS_USER}":"${LS_GROUP}" -m750 "$d" chown -R "${LS_USER}":"${LS_GROUP}" "$d" done As a result, the ${LS_USER} user can write whatever he wants into the PID file. Later, that may be exploitable: when the service is stopped, root will call "kill" on the contents of that file. The good news is, you shouldn't need to call checkpath for the PID file at all. With command_background="true", OpenRC creates the PID file as root:root, and the problematic "checkpath" can be avoided if you store the PID file directly in /run. That is, simply set: pidfile="/run/logstash.pid" In bug 628546, I suggested some improvements to the user/group handling; in particular, that you not allow them to change on-the-fly. The reasoning was that, if the user/group change, you need to adjust the ownership and permissions of any existing files from the old LS_USER to the new one. There I hinted at another problem, "to work around that, you'll come up with some recursive chown thing, and introduce a new problem." I'm sorry to say that logstash-bin now has that new problem =) == Root privilege escalation through chown == When you call chown recursively, it follows links (both hard and sym). There are some kernel hardening options that can help you avoid that, but they're not enabled by default in a vanilla linux kernel, so we can't rely on them. Here's the bad scenario: 1. The first time you start logstash-bin, it changes ownership of LS_LOG_DIR to LS_USER:LS_GROUP. 2. LS_USER is a bad guy, and he creates a symlink in LS_LOG_DIR pointing to /root/.bashrc 3. The next time you start logstash-bin, it runs "chown -R LS_USER" on LS_LOG_DIR, and chown follows the symlink, giving ownership of /root/.bashrc to LS_USER. Now LS_USER is root. There is no easy fix for this when LS_USER and LS_GROUP are variables. If at all possible, you should create a restricted user and group for logstash-bin in the ebuild, and then hard-code them in the init script. That way, you never need to fix the permissions on a bunch of pre-existing files and directories.
Thanks for the bug. I'm not that actively maintaining this package anymore in practice, but considering the nature of the issue, I'm more than happy to help out. As far as I remember, the original intention with the init script was to be as close as possible to upstream. I haven't checked recent upstream init scripts, so they may or may not need notification as well (or maybe there's a related report already). Since the ebuild already creates the user and group intended for default usage, it seems easy to remove the chown step from the init script, and ensure (initial) directory ownership and permissions from the ebuild itself. Would that be enough to fix these issues? Also, where is it best to provide patches and in which format?
(In reply to Ferenc Erki from comment #1) > > As far as I remember, the original intention with the init script was to be > as close as possible to upstream. I haven't checked recent upstream init > scripts, so they may or may not need notification as well (or maybe there's > a related report already). Ah, I see that upstream is using LS_USER and LS_GROUP, but it doesn't have the "chown" call so I think it's safe. > Since the ebuild already creates the user and group intended for default > usage, it seems easy to remove the chown step from the init script, and > ensure (initial) directory ownership and permissions from the ebuild itself. Yup, that's how I would fix it: 1. Replace LS_USER and LS_GROUP with "logstash" in the init script, and drop the variables from the conf.d file. 2. Set pidfile="/run/logstash.pid" in the init script. 3. Remove the "chown" call from the "for" loop, and don't loop over the PID file directory at all (i.e. don't call checkpath on /run). > Would that be enough to fix these issues? Also, where is it best to provide > patches and in which format? That should do it. You can post a patch here if you want, in unified format (diff -u) or as the output from one of the git commands. git diff, git format-patch, etc. are all acceptable. You can even submit pull requests on Github if that's most convenient: https://github.com/gentoo/gentoo/ Finally, you could just attach the new files to this bug. The init script and conf.d files will need new revisions, so will need to be named e.g. logstash.initd-r1 and logstash.confd-r1. Then we'll need a revision bump for the newest logstash-bin ebuild that uses the new init script and conf.d file. That way, we can stabilize the new revision, and ensure that all users get the fixed init script. One more thing to consider: I see that the ebuilds are doing, enewuser ${MY_PN} -1 -1 /var/lib/${MY_PN} ${MY_PN} Maybe in light of that, the LS_HOME variable should be hard-coded to /var/lib/logstash, too?
Thanks for the well prepared report. For the PID manipulation case, we just remove the chown and move the pid directly under /run. The second case is a bit tricky. Logstash (like filebeat) may take input as logs (mostly owned by root), so he needs read access. Once we had root as the default user, but since logstash can take network input as well where we don't need root, this was changed to the dedicated logstash user. If we create a special user and one needs to read logs in /var/log then the group permission needs to be updated everywhere which is kind of weird. Probably it's easier to drop the chown and set a warning in the ebuild. Or an alternative would be to prepare a separate script for updating privileges (stripped from the init script)?
The two non-PIDfile directories that get chown'ed at the moment are, LS_INSTALL_DIR="/opt/logstash" LS_LOG_DIR=${LS_LOG_DIR:-/var/log/logstash} If the logstash user needs to read something else under /var/log, like /var/log/messages, then does the current init script even address that? I don't have an easy answer for how to grant access to that stuff. Group permissions weren't flexible enough for me, so I use POSIX ACLs for most things. You could make the logs have group "logstash", or add the logstash user to the root group, or... a hundred other things that all have trade-offs. An automated script might work, but you have to be really careful to never call chown or chmod on a file in a directory that isn't owned by root.
In order to read the logs,you need to specify the user with appropriate permissions. We do not set the permissions on system logs (the ones to be processed by logstash) since they are specified elsewhere in the config. The chown calls are there to ensure that you can switch LS_USER between root and any other user (usually logstash) and it will just work without permission denied warnings. Suppose you first run logstash as root, all the data dir are owned as root and later you wish to run as logstash user and you do not have petmission on data directories. Those chowns handled these situations.
Sorry for long post, but I felt it's worth to share my thoughts on some related details/features which I'd like to see keep preserved for Gentoo. TL;DR: 1. Let's drop chown. 2. Keep LS_USER, LS_GROUP, LS_PIDFILE and LS_HOME configurable for multi-instance scenarios. 2a. Add config option for `--path.data` if needed. 3. Let start-stop-daemon manage pidfiles. Long version: > Ah, I see that upstream is using LS_USER and LS_GROUP, but it doesn't have the > "chown" call so I think it's safe. Yes, upstream seem to be generating init scripts and/or systemd units during installation via their `system-install` script, which in turn seems to be just using `pleaserun`. And the template in there seems to be not affected (has `chown`, but not a recursive one, and only for the logging directory. Maybe we can simply run `system-install` too to generate an init script instead of maintaining and installing a handcrafted one? > 1. Replace LS_USER and LS_GROUP with "logstash" in the init script, > and drop the variables from the conf.d file. If there's any way, I'd strongly prefer to keep them, due to a quite common use case of running multiple logstash instances on the same machine, but with different privileges, configuration, etc (for example having one logstash per product team, and/or service). This is something that is not fully implemented yet for Gentoo init scripts (but e.g. the somewhat related elasticsearch initscripts can do it already), but let's say "almost there", and would be nice to complete. > 2. Set pidfile="/run/logstash.pid" in the init script. For the same reasons, I think it should also be configurable. I'm relatively indifferent about the default location, as long as we can avoid security issues. That being said, I would prefer using the path upstream uses (`/var/run/logstash.pid` as of this moment). I believe the original reason behind using a directory like `/var/run/logstash` for pidfiles was due to the same multi-instance reasons (so `productA.pid` and `teamB.pid` could all live there). Though this can be solved with name prefixing as well (`/run/logstash.productA.pid` and `/run/logstash.teamB.pid`). I think it's good if we let start-stop-daemon to handle the files themselves (which would also mean keeping it owned by root as well, if I understand correctly). > 3. Remove the "chown" call from the "for" loop, and don't loop over the PID file directory at all (i.e. don't call checkpath on /run). Yeah, I think the `chown` must go, at least in its recursive form, but preferably fully. It seems like it was introduced on https://github.com/gentoo/gentoo/commit/bbdc5412061adf598ed935697441a7d6b05f7614, and I believe the previous `checkpath` method was not affected by this issue (maybe the `checkpath -f` should be avoided for the same sym/hardlink modificatin reasons). > Maybe in light of that, the LS_HOME variable should be hard-coded to /var/lib/logstash, too? I'd keep it configurable too, for the same multi-instance reasons. > Logstash (like filebeat) may take input as logs (mostly owned by root), so he needs read access. I'd say it's up to the administrator to ensure permissions are set on log sources (which might not only be files). For product-related services, it was usually already there because the same user, or at least the same group owned all things logstash needed to touch for the given product. Root could decide to run logstash as root for system logs, or more preferably, just set group permissions for the `logstash` user. I guess this is an area that would be hard to handle as a generic case with a tool, and it's best to leave the decision to administrators (and helping them by leaving all the config options within their reach under `/etc/conf.d`). > Probably it's easier to drop the chown and set a warning in the ebuild. Yeah, I think I'd go for that. Some warning along the lines of "If you change privileges for your logstash instance, please make sure it has proper permissions for all the following: 1. inputs it should read from 2. outputs it should write to (including its own logs)" might be sufficient. My only remaining question is about `/opt/logstash/data`, which seems like a more recent addition. It is configurable via the `--path.data` command line option of logstash, and since it seems to be used for logstash persistent queues, I'd expect this should be different for every instance too for a multi-instance scenario. I'd guess it's good enough to set it to `${LS_HOME}/data` as a default (instead of the shared-by-all location of `/opt/logstash/data`).
(In reply to Tomáš Mózes from comment #5) > > The chown calls are there to ensure that you can switch LS_USER between root > and any other user (usually logstash) and it will just work without > permission denied warnings. Ok, that's the situation I've had trouble with in the past. I'm not even sure there is a safe way to do it. It might be possible with a script that gets run only once, and if you are able to rely on GNU chown with its "--from" flag, and if you make sure to traverse the files and directories in a safe order (don't chown a directory before its files). But those points just address the problems that I've thought of -- I'd be willing to bet there are others.
I'm fine with leaving LS_USER, LS_GROUP, and LS_HOME for multiple-instance support; you only run into problems if you try to support people changing them on-the-fly for a single instance. (It's still TBD if this can be solved.) /var/run is a symlink to /run these days, but /run/logstash/<whatever>.pid is also fine. Just be sure to create it as root:root, rather than as LS_USER:LS_GROUP.
Created attachment 490192 [details, diff] logstash-bin-remove-chown.diff Alright, judging by the release notes and recent official package contents, it looks like things around packaging, init scripts handling and also configuration style changed considerably for the 5.x releases. I'm afraid that would be too big of a change to be on par with that, so I'd say let's just fix this bug on its own, and handle the rest separately, if you agree. In my own quick tests, I believe this attached patch solves both issues (no files are set to be automatically owned by LS_USER/LS_GROUP). I aimed for least changes compared to current situation in Gentoo, and it doesn't contain revbumping the init script nor all the current ebuild files in tree yet. I can update it later, or feel free to apply those changes too. Does this look enough to address this bug's concerns?
Created attachment 490210 [details, diff] logstash-bin-fix-628558.diff Hmm, with the patch above, it fails to start up properly because the following locations are created as root during the checkconfig() call in the initscript: - logfiles under LS_LOG_DIR (/var/log/logstash) - contents of the data directory under /opt/logstash/data (with further subdirectories like `queue` and `dead_letter_queue`) while those locations are expected to be writable by LS_USER. After iterating through various approaches, I updated my proposed patch to: - override logdir for the configcheck command to prevent us from having to chown logs created during that step - use checkpath to ensure `root:root` ownership on the directory of the pidfiles to protecting users who already have vulnerable permissions on their pathes - use checkpath to ensure `LS_USER:LS_GROUP` ownership on the (IMHO) minimal set of pathes (to correct for the permissions created during `checkconfig()`) Note: for now, I tried to avoid moving the pidfiles one level up to prevent issues when a logstash instance is already running during installation time. Please review this set of changes. I would be happy to revbump all ebuilds/files involved.
What if we add some check to see if there are any symlinks in /var/lib/logstash or /var/log/logstash? Or what if we use find -type {d,f} instead of chown and skip setting perms on symlinks?
(In reply to Tomáš Mózes from comment #11) > What if we add some check to see if there are any symlinks in > /var/lib/logstash or /var/log/logstash? Or what if we use find -type {d,f} > instead of chown and skip setting perms on symlinks? Depending on how it is done it can be open to race conditions
And regarding /opt/logstash/data and /opt/logstash/config, they are in my todo list for changing. The data dir should move to /var/lib/logstash and the config to /etc/logstash. But that can be done in the next version bump.
(In reply to Kristian Fiskerstrand from comment #12) > (In reply to Tomáš Mózes from comment #11) > > What if we add some check to see if there are any symlinks in > > /var/lib/logstash or /var/log/logstash? Or what if we use find -type {d,f} > > instead of chown and skip setting perms on symlinks? > > Depending on how it is done it can be open to race conditions And is there a correct way to do it? Do we have some package in Gentoo we can build on?
(In reply to Tomáš Mózes from comment #11) > What if we add some check to see if there are any symlinks in > /var/lib/logstash or /var/log/logstash? Or what if we use find -type {d,f} > instead of chown and skip setting perms on symlinks? Hard links have the same problem... > And is there a correct way to do it? Do we have some package in Gentoo we can build on? I haven't seen one yet.
https://github.com/gentoo/gentoo/pull/5665
security@, this has been fixed, and the old versions removed from the tree.
(In reply to Michael Orlitzky from comment #17) > security@, this has been fixed, and the old versions removed from the tree. Have you requested a CVE or do you want Security Project to do this? Lifting restriction, fix is public and allows for using this bug as reference in request.
(In reply to Kristian Fiskerstrand from comment #18) > (In reply to Michael Orlitzky from comment #17) > > security@, this has been fixed, and the old versions removed from the tree. > > Have you requested a CVE or do you want Security Project to do this? > I'll do it today.
(In reply to Michael Orlitzky from comment #19) > (In reply to Kristian Fiskerstrand from comment #18) > > (In reply to Michael Orlitzky from comment #17) > > > security@, this has been fixed, and the old versions removed from the tree. > > > > Have you requested a CVE or do you want Security Project to do this? > > > > I'll do it today. Thanks! Since package is not in stable, there won't be a GLSA for this issue. Since cleanup etc is done, once CVE is assigned we can close the bug.
The "chown" issue in the init script has been assigned CVE-2017-14730.
